Skip to content

Check for FreeType functions required for avatars - #7480

Merged
MorrisJobke merged 2 commits into
masterfrom
fix_7454
Dec 13, 2017
Merged

Check for FreeType functions required for avatars#7480
MorrisJobke merged 2 commits into
masterfrom
fix_7454

Conversation

@rullzer

Copy link
Copy Markdown
Member

Fixes#7454

* @return bool
*/
protected function hasFreeTypeSupport() {
if (function_exists('imagettfbbox') && function_exists('imagettftext')) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer the shorter version

returnfunction_exists('imagettfbbox') && function_exists('imagettftext');

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fair enough

@codecov

codecovBot commented Dec 13, 2017

Copy link
Copy Markdown

Codecov Report

Merging #7480 into master will increase coverage by <.01%.
The diff coverage is 60%.

@@ Coverage Diff @@## master #7480 +/- ##
============================================
+ Coverage 51.11% 51.11% +<.01% - Complexity 24901 24903 +2 
============================================
Files 1601 1601 Lines 94774 94778 +4 Branches 1367 1368 +1 ============================================
+ Hits 48441 48444 +3 - Misses 46333 46334 +1
Impacted FilesCoverage ΔComplexity Δ
core/js/setupchecks.js96.29% <100%> (+0.06%)0 <0> (ø)⬇️
settings/Controller/CheckSetupController.php69.88% <33.33%> (-0.64%)58 <2> (+2)
apps/files_trashbin/lib/Trashbin.php72.28% <0%> (-0.25%)136% <0%> (ø)
apps/user_ldap/lib/Access.php35.74% <0%> (+0.04%)315% <0%> (ø)⬇️
lib/private/Files/Cache/Propagator.php96.2% <0%> (+1.26%)16% <0%> (ø)⬇️

@ChristophWurstChristophWurst left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@danxuliudanxuliu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on a system without FreeType support enabled and works as expected 👍

I have noticed, though, that not having FreeType support affects other elements besides just the avatars, for example the user settings UI (if you create a user or modify a user setting the UI will not be updated, although if you refresh the page you can see that it was modified as expected).

Maybe the error message should be something like "This will result in broken avatars and user settings UI" instead ("will" instead of "can" and "user settings UI" included in the message too)?

@rullzer

Copy link
Copy Markdown
MemberAuthor

@danxuliu I don't have a preference. Feel free to change the wording :)

@jancborchardt

Copy link
Copy Markdown
Member

@danxuliu slightly improved:

This will result in broken profile pictures and settings interface.

(We call it »profile picture« in the settings, should not use »avatar« anywhere. Also, no technical abbreviation like »UI«.)

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@danxuliu

Copy link
Copy Markdown
Member

I have amended the commits with @jancborchardt suggestion, thanks!

@ChristophWurstChristophWurst added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Dec 13, 2017
@MorrisJobke
MorrisJobke merged commit 784d256 into masterDec 13, 2017
@MorrisJobke
MorrisJobke deleted the fix_7454 branch December 13, 2017 17:25
@MorrisJobkeMorrisJobke mentioned this pull request Jan 2, 2018
30 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4. to releaseReady to be released and/or waiting for tests to finishenhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@rullzer@jancborchardt@danxuliu@ChristophWurst@MorrisJobke