Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check for FreeType functions required for avatars #7480

Merged
merged 2 commits into from
Dec 13, 2017
Merged

Conversation

rullzer
Copy link
Member

@rullzer rullzer commented Dec 13, 2017

Fixes #7454

* @return bool
*/
protected function hasFreeTypeSupport() {
if (function_exists('imagettfbbox') && function_exists('imagettftext')) {
Copy link
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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

fair enough

@codecov
Copy link

codecov bot commented Dec 13, 2017

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 Files Coverage Δ Complexity Δ
core/js/setupchecks.js 96.29% <100%> (+0.06%) 0 <0> (ø) ⬇️
settings/Controller/CheckSetupController.php 69.88% <33.33%> (-0.64%) 58 <2> (+2)
apps/files_trashbin/lib/Trashbin.php 72.28% <0%> (-0.25%) 136% <0%> (ø)
apps/user_ldap/lib/Access.php 35.74% <0%> (+0.04%) 315% <0%> (ø) ⬇️
lib/private/Files/Cache/Propagator.php 96.2% <0%> (+1.26%) 16% <0%> (ø) ⬇️

Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Looks good!

Copy link
Member

@danxuliu danxuliu left a comment

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
Member Author

rullzer commented Dec 13, 2017

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

@jancborchardt
Copy link
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
Member

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

@ChristophWurst ChristophWurst 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 master Dec 13, 2017
@MorrisJobke MorrisJobke deleted the fix_7454 branch December 13, 2017 17:25
@MorrisJobke MorrisJobke mentioned this pull request Jan 2, 2018
30 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants