Better handle avatars #7498

rullzer · 2017-12-13T19:25:02Z

Do not use placeholder code to avoid flashing of the image
Actually delete generated avatar on displayname change
Refresh avatar after displayname change in settings
Show loading icon
Allow deletion of profile picture Deleting profile pictures is not possible #7500

Future work:

Not all letters are properly centerered

codecov · 2017-12-13T19:54:30Z

Codecov Report

Merging #7498 into master will increase coverage by <.01%.
The diff coverage is 76.19%.

@@             Coverage Diff              @@
##             master    #7498      +/-   ##
============================================
+ Coverage     51.16%   51.17%   +<.01%     
- Complexity    24883    24886       +3     
============================================
  Files          1602     1602              
  Lines         94730    94750      +20     
  Branches       1368     1368              
============================================
+ Hits          48470    48485      +15     
- Misses        46260    46265       +5

Impacted Files	Coverage Δ	Complexity Δ
lib/private/Template/JSConfigHelper.php	`0% <0%> (ø)`	`17 <0> (ø)`	⬇️
lib/private/Avatar.php	`77.65% <66.66%> (-0.56%)`	`46 <3> (+3)`
lib/private/Server.php	`81.55% <90.9%> (+0.09%)`	`134 <2> (ø)`	⬇️

danxuliu · 2017-12-14T01:17:56Z

Do not use placeholder code to avoid flashing of the image

Before, if a display name was given when calling avatar(), a placeholder was shown until the avatar image was loaded; now nothing is shown until the avatar image is loaded. Maybe it would be better to replace the placeholder with a loading icon instead? The same approach could be used too when calling avatar() without giving a display name (even before this change nothing was shown in that case until the image loaded).

Also, if the problem is the flash caused when the placeholder is replaced by the image, another option could be to show the placeholder and then, when the image is loaded, fade out the placeholder and fade in the image.

Or maybe both, I mean, show the loading icon instead of the placeholder and also fade out the loading icon while fading in the avatar image once it is loaded (although as the animation should be pretty short it could end causing a flash anyway...).

In any case, with the current code if a display name is given and the avatar image fails to load no avatar is shown at all, but if no display name is given and the avatar image fails to load then a placeholder is shown; at the very least the behaviour should be the same in both cases.

@nextcloud/designers

rullzer · 2017-12-14T06:45:05Z

@danxuliu I'm afraid my JS and design skills are lacking here. So feel free to hack in a loading animation :)

skjnldsv · 2017-12-14T07:30:40Z

@danxuliu I'm afraid my JS and design skills are lacking here. So feel free to hack in a loading animation :)

Done

rullzer · 2017-12-14T09:17:37Z

Ok let get this in before it becomes to much of a mess :)

MorrisJobke · 2017-12-14T09:28:32Z

lib/public/IAvatar.php

@@ -77,4 +77,10 @@ public function remove();
 	 * @since 9.0.0
 	 */
 	public function getFile($size);
+
+	/**
+	 * Is this a generated avatar


It looks like the comment is wrong 😉

MorrisJobke · 2017-12-14T09:31:46Z

core/js/jquery.avatar.js

 			var img = new Image();

 			// If the new image loads successfully set it.
 			img.onload = function() {
-				$div.show();
 				$div.text('');
 				$div.append(img);
 				$div.clearimageplaceholder();


I guess this is then also not needed anymore.

Yes it is because the div is cleared with the loading class as well
I think it could also interfer with the uploading of a new avatar.

MorrisJobke

Code makes sense, tested and works 👍

rullzer · 2017-12-14T11:16:14Z

@skjnldsv you forgot to sign off ;)

skjnldsv · 2017-12-14T11:22:11Z

Rebased and ready to 🚢

danxuliu

The loading icon should be shown too when avatar() is called without a display name.

If a display name is given and the avatar image fails to load no avatar is shown at all, but if no display name is given and the avatar image fails to load then a placeholder is shown; the behaviour should be the same in both cases.

In the user settings the button to delete the avatar image is shown even if the image was auto-generated, and removing the auto-generated image just recreates it again, so it looks like nothing happened when clicking the button (except for a loading icon being shown). However I do not know if we have any way to distinguish between a user uploaded image and an auto-generated image in the avatars... @rullzer

juliusknorr · 2017-12-16T15:51:27Z

I pushed two commits to fix the issues described by @danxuliu. I reduced the complexity of the avatar function quite a bit. We don't rely on the displayname for avatar placeholders anymore, therefore we can totally get rid of the old code path for placeholder generation.

Also for setups without gd installed i keep generating the placeholder in case there is a displayname provided to the avatar plugin.

danxuliu · 2017-12-18T11:09:59Z

I pushed two commits to fix the issues described by @danxuliu.

Nice, thanks! :-D

But... unsigned commit (first one) and failing tests :-)

And another detail: the callback should be called too when an error happens (like it was done in the old code path).

Also I am wondering if we should write a log to the console or maybe a warning when ie8fix and hidedefault parameters are given to make app developers aware that those parameter are no longer used and that they will be removed for Nextcloud 14.

rullzer · 2017-12-19T08:04:47Z

Also I am wondering if we should write a log to the console or maybe a warning when ie8fix and hidedefault parameters are given to make app developers aware that those parameter are no longer used and that they will be removed for Nextcloud 14.

Sure. But this will have to go into a list of breaking changes as it forces app that use this to move to a NC14 only version. But fine by me.

rullzer · 2017-12-19T08:30:48Z

Failing tests are due to image mocking of the image function magic... lets see if we can somehow solve that properly :S

rullzer · 2017-12-19T09:02:37Z

Fixed the tests I think

The js and php code differ ever so slightly. So having the placeholder for a second and then the image is just weird. Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

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

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

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

With the new avatar endpoint there is no difference between unknown users and errors when generating the placeholder avatar. Therefore the avatar function will now show the old placeholder if both a user and displayname was given as parameters. In case there is no displayname provided we cannot build the proper placeholder so the unknown user placeholder is shown. The displayname is not required for the avatar anymore, so we can get rid of the old code path for placeholders. Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Signed-off-by: Julius Härtl <jus@bitgrid.net>

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

Signed-off-by: Julius Härtl <jus@bitgrid.net>

rullzer · 2017-12-19T19:17:51Z

@danxuliu I think your comments are addressed. Can you verify?

Changed code

MorrisJobke

Tested and works 👍

MorrisJobke · 2017-12-20T10:59:24Z

@danxuliu @skjnldsv @blizzz Please review

rullzer added 2. developing Work in progress bug labels Dec 13, 2017

rullzer added this to the Nextcloud 13 milestone Dec 13, 2017

rullzer requested review from nickvergessen, MorrisJobke, ChristophWurst and danxuliu December 13, 2017 19:25

rullzer mentioned this pull request Dec 13, 2017

Generated profile picture does appear a second later and is not positioned correct #7497

Closed

rullzer mentioned this pull request Dec 14, 2017

Deleting profile pictures is not possible #7500

Closed

rullzer added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Dec 14, 2017

rullzer requested a review from skjnldsv December 14, 2017 09:17

skjnldsv previously approved these changes Dec 14, 2017

View reviewed changes

MorrisJobke reviewed Dec 14, 2017

View reviewed changes

rullzer force-pushed the fix_7497 branch from 1efa86b to 531588d Compare December 14, 2017 09:33

MorrisJobke previously approved these changes Dec 14, 2017

View reviewed changes

skjnldsv force-pushed the fix_7497 branch 2 times, most recently from 8df6129 to 4a90ff9 Compare December 14, 2017 11:21

MorrisJobke 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 14, 2017

danxuliu previously requested changes Dec 14, 2017

View reviewed changes

MorrisJobke added the 2. developing Work in progress label Dec 14, 2017

rullzer force-pushed the fix_7497 branch from 22ee9eb to 9158f87 Compare December 19, 2017 08:30

rullzer and others added 10 commits December 19, 2017 18:49

Since we now always generate an avatar do not load the placeholder

6371b76

The js and php code differ ever so slightly. So having the placeholder for a second and then the image is just weird. Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Remove generated avatar on displayname change

a4ca92f

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

Update avatar on displayname change

0f235ae

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

Add loading icon on avatar

a118817

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

Still call callback once avatar is loaded

6b5e3a9

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

Hide delete button if the avatar is a generated one

4c3eb80

Signed-off-by: Julius Härtl <jus@bitgrid.net>

Also do callback on the failing function

328047f

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

Fix tests

325637f

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

Fix failing AvatarTest

0ff3c81

Signed-off-by: Julius Härtl <jus@bitgrid.net>

juliusknorr force-pushed the fix_7497 branch from 1c04131 to 0ff3c81 Compare December 19, 2017 17:49

rullzer added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Dec 19, 2017

MorrisJobke approved these changes Dec 20, 2017

View reviewed changes

skjnldsv approved these changes Dec 20, 2017

View reviewed changes

MorrisJobke merged commit dc8809e into master Dec 20, 2017

MorrisJobke deleted the fix_7497 branch December 20, 2017 11:38

MorrisJobke mentioned this pull request Jan 2, 2018

13.0.0 beta 4 #7664

Merged

30 tasks

juliusknorr mentioned this pull request Jan 5, 2018

Fix avatar display to not overwrite display style #7711

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better handle avatars #7498

Better handle avatars #7498

rullzer commented Dec 13, 2017 •

edited

Loading

codecov bot commented Dec 13, 2017 •

edited

Loading

danxuliu commented Dec 14, 2017 •

edited

Loading

rullzer commented Dec 14, 2017

skjnldsv commented Dec 14, 2017 •

edited

Loading

rullzer commented Dec 14, 2017

MorrisJobke Dec 14, 2017

MorrisJobke Dec 14, 2017

skjnldsv Dec 14, 2017

MorrisJobke left a comment

rullzer commented Dec 14, 2017

skjnldsv commented Dec 14, 2017 •

edited

Loading

danxuliu left a comment •

edited

Loading

juliusknorr commented Dec 16, 2017

danxuliu commented Dec 18, 2017

rullzer commented Dec 19, 2017

rullzer commented Dec 19, 2017

rullzer commented Dec 19, 2017

rullzer commented Dec 19, 2017

MorrisJobke left a comment

MorrisJobke commented Dec 20, 2017

Better handle avatars #7498

Better handle avatars #7498

Conversation

rullzer commented Dec 13, 2017 • edited Loading

codecov bot commented Dec 13, 2017 • edited Loading

Codecov Report

danxuliu commented Dec 14, 2017 • edited Loading

rullzer commented Dec 14, 2017

skjnldsv commented Dec 14, 2017 • edited Loading

rullzer commented Dec 14, 2017

MorrisJobke Dec 14, 2017

Choose a reason for hiding this comment

MorrisJobke Dec 14, 2017

Choose a reason for hiding this comment

skjnldsv Dec 14, 2017

Choose a reason for hiding this comment

MorrisJobke left a comment

Choose a reason for hiding this comment

rullzer commented Dec 14, 2017

skjnldsv commented Dec 14, 2017 • edited Loading

danxuliu left a comment • edited Loading

Choose a reason for hiding this comment

juliusknorr commented Dec 16, 2017

danxuliu commented Dec 18, 2017

rullzer commented Dec 19, 2017

rullzer commented Dec 19, 2017

rullzer commented Dec 19, 2017

rullzer commented Dec 19, 2017

MorrisJobke left a comment

Choose a reason for hiding this comment

MorrisJobke commented Dec 20, 2017

rullzer commented Dec 13, 2017 •

edited

Loading

codecov bot commented Dec 13, 2017 •

edited

Loading

danxuliu commented Dec 14, 2017 •

edited

Loading

skjnldsv commented Dec 14, 2017 •

edited

Loading

skjnldsv commented Dec 14, 2017 •

edited

Loading

danxuliu left a comment •

edited

Loading