Always generate avatar #6876

rullzer · 2017-10-19T14:24:34Z

Even if no avatar is set we should just generate the image. This to not
duplicate the code on all the clients. And only server images from the
avatar endpoint.

Todo:

Save avatar
Move this to the actual avatar code instead of the Controller

Future work:

use imgsrc so the browser fetches the right size on hdpi stuff
extend caching

codecov · 2017-10-20T18:09:19Z

Codecov Report

Merging #6876 into master will increase coverage by 0.03%.
The diff coverage is 81.19%.

@@             Coverage Diff              @@
##             master    #6876      +/-   ##
============================================
+ Coverage     50.85%   50.89%   +0.03%     
- Complexity    24539    24557      +18     
============================================
  Files          1584     1584              
  Lines         93798    93891      +93     
  Branches       1358     1358              
============================================
+ Hits          47704    47788      +84     
- Misses        46094    46103       +9

Impacted Files	Coverage Δ	Complexity Δ
core/Controller/AvatarController.php	`81.16% <100%> (+2.64%)`	`31 <0> (-1)`	⬇️
lib/private/Avatar.php	`78.08% <80.7%> (+1.77%)`	`43 <18> (+19)`	⬆️
apps/files_trashbin/lib/Trashbin.php	`72.53% <0%> (+0.24%)`	`136% <0%> (ø)`	⬇️
core/js/js.js	`63.55% <0%> (+0.56%)`	`0% <0%> (ø)`	⬇️

rullzer · 2017-11-02T09:58:42Z

Ah mmm so upating the JS is a tiny bit tricky as there is also some code to handle unknown users etc.

rullzer · 2017-11-02T09:59:26Z

So @tobiasKaminsky this is what you wanted right?

@jancborchardt please checkout this branch and try to share etc. To see if there are no weird avatar hickups now.

tobiasKaminsky · 2017-11-02T13:50:12Z

@rullzer this is great. So we can drop avatar generation once NC 12 has end of life :-)

LukasReschke · 2017-11-03T13:16:18Z

core/Controller/AvatarController.php

 	/**
 	 * @NoAdminRequired
 	 * @NoCSRFRequired
-	 * @NoSameSiteCookieRequired


Why did you remove this? This is for example required when Collabora is embedding avatars since it runs under a different origin :)

Good question

LukasReschke · 2017-11-03T13:16:55Z

lib/private/Avatar.php

+
+		$hsl = $this->rgbToHsl($rgb[0], $rgb[1], $rgb[2]);
+
+		// Classic formulla to check the brigtness for our eye


/formulla/formula/

LukasReschke · 2017-11-03T13:17:07Z

lib/private/Avatar.php

+	 * @param double $h Heu in range [0, 1]
+	 * @param double $s Saturation in range [0, 1]
+	 * @param double $l Lightness in range [0, 1]
+	 * @return int[] Array containging r g b in the range [0, 255]


/containging/containing/

LukasReschke · 2017-11-03T13:17:12Z

lib/private/Avatar.php

+	}
+
+	/**
+	 * @param double $h Heu in range [0, 1]


/Heu/Hue/ maybe?

LukasReschke · 2017-11-03T13:17:37Z

lib/private/Avatar.php

@@ -210,4 +230,160 @@ private function getExtension() {
 		}
 		throw new NotFoundException;
 	}
+
+	private function generateAvatar($userDisplayName, $size) {


PHPDoc on the parameters would be awesome 😄

Addressed

Even if no avatar is set we should just generate the image. This to not duplicate the code on all the clients. And only server images from the avtar endpoint. Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

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

Signed-off-by: Morris Jobke <hey@morrisjobke.de>

MorrisJobke

I made the font use semibold weight and made it a bit better centered, because it was slightly off.

MorrisJobke · 2017-12-08T15:26:12Z

The only thing that may be a bit different is, that we now always have an avatar in the top right - cc @nextcloud/designers

skjnldsv · 2017-12-08T16:00:30Z

I'm OK with that. This is also pretty nice for the top right menu list drop-down positioning issue we have here and there :)

MariusBluem · 2017-12-08T16:28:19Z

I like always having the avatar at the top 👍

MorrisJobke · 2017-12-08T22:57:46Z

Discussed with @LukasReschke on IRC and good to get in.

MorrisJobke · 2017-12-10T21:29:57Z

@rullzer Seems to break the integration tests for sharing :/ See @danxuliu's comment in #6715 (comment)

rullzer · 2017-12-11T10:55:21Z

I know. I have a patch for that already

MorrisJobke · 2017-12-11T11:07:52Z

I know. I have a patch for that already

#7342

jancborchardt · 2017-12-11T22:47:10Z

Hm, in the past @karlitschek and I discussed this with avatar placeholder in top right and we didn’t really like it as it’s very present and colorful. However it kind of nudges you to set a proper image, so I guess it’s fine after all. :)

jancborchardt · 2017-12-15T15:41:18Z

Actually this causes a confusion with new users or installations. Cause in the top right the avatar is shown and not the settings icon, it is not immediately obvious where the settings are:

#7535

(When you are used to the UI and added a picture, you also know where the settings are.)

MariusBluem · 2017-12-15T16:39:34Z

I disagree with you @jancborchardt ... this behaviour is known by the users from so many services ... not only by Nextcloud.

To make it crystal clear we could show some arrows with descriptions in the firstrunwizard :)

jancborchardt · 2017-12-15T20:51:50Z

To make it crystal clear we could show some arrows with descriptions in the firstrunwizard :)

If it's known, we don't need a pointer for such an obvious menu in the first run wizard. ;)

MorrisJobke · 2017-12-16T11:33:06Z

Actually this causes a confusion with new users or installations. Cause in the top right the avatar is shown and not the settings icon, it is not immediately obvious where the settings are:

I guess the main problem is, that we don't have an indicator for menus. On github they put for example a little arrow beside the avatar to indicate the menu:

MariusBluem · 2017-12-16T11:56:44Z

Dropbox:

Microsoft:

No indicator ;)

rullzer added 2. developing Work in progress enhancement labels Oct 19, 2017

rullzer added this to the Nextcloud 13 milestone Oct 19, 2017

rullzer requested a review from LukasReschke October 19, 2017 14:24

LukasReschke previously requested changes Nov 3, 2017

View reviewed changes

rullzer force-pushed the always_img_avatar branch from 706ed5b to 949fdc8 Compare November 8, 2017 12:20

rullzer force-pushed the always_img_avatar branch from 949fdc8 to 3e2ca98 Compare November 9, 2017 10:35

rullzer force-pushed the always_img_avatar branch from 3e2ca98 to 2c67928 Compare November 27, 2017 19:10

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

rullzer added 5 commits November 29, 2017 14:23

Always generate avatar

604596e

Even if no avatar is set we should just generate the image. This to not duplicate the code on all the clients. And only server images from the avtar endpoint. Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Move avatar code to Avatar Class

ba648ee

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

Sinc we use AppData the regex is not needed anymore

03d9e7e

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

Since we update the avatar in the sync service remove the generated file

d5496dc

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

Fix tests

8e8fe6b

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

rullzer force-pushed the always_img_avatar branch from 2c67928 to 8e8fe6b Compare November 29, 2017 13:23

rullzer mentioned this pull request Nov 29, 2017

CardDAV convertor check should not be to wide #7342

Merged

tobiasKaminsky mentioned this pull request Dec 7, 2017

Avatar Etags nextcloud/android#1658

Merged

Better center the letter and use semibold font type

66f523e

Signed-off-by: Morris Jobke <hey@morrisjobke.de>

MorrisJobke approved these changes Dec 8, 2017

View reviewed changes

MorrisJobke merged commit ed7beb9 into master Dec 8, 2017

MorrisJobke deleted the always_img_avatar branch December 8, 2017 22:58

This was referenced Dec 11, 2017

Load avatars via the new standard API nextcloud/android#779

Open

Use avatars nextcloud/android#1202

Closed

Ivansss mentioned this pull request Dec 11, 2017

13.0.0 beta 2 #7425

Merged

28 tasks

danxuliu mentioned this pull request Dec 11, 2017

Nextcloud 13 expects FreeType support to be enabled in PHP GD #7454

Closed

MorrisJobke mentioned this pull request Dec 12, 2017

Avatar API: generate image if no user defined image is available & versioning #5750

Closed

MariusBluem mentioned this pull request Dec 13, 2017

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

Closed

jancborchardt mentioned this pull request Dec 15, 2017

Lose Admin icon on Top Line #7535

Closed

danxuliu mentioned this pull request Jan 31, 2018

[stable12] Fix own avatar not updated when display name is changed #8130

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Always generate avatar #6876

Always generate avatar #6876

rullzer commented Oct 19, 2017 •

edited

Loading

codecov bot commented Oct 20, 2017 •

edited

Loading

rullzer commented Nov 2, 2017

rullzer commented Nov 2, 2017

tobiasKaminsky commented Nov 2, 2017

LukasReschke Nov 3, 2017

rullzer Nov 3, 2017

LukasReschke Nov 3, 2017

LukasReschke Nov 3, 2017

LukasReschke Nov 3, 2017

LukasReschke Nov 3, 2017

MorrisJobke left a comment

MorrisJobke commented Dec 8, 2017

skjnldsv commented Dec 8, 2017

MariusBluem commented Dec 8, 2017

MorrisJobke commented Dec 8, 2017

MorrisJobke commented Dec 10, 2017

rullzer commented Dec 11, 2017

MorrisJobke commented Dec 11, 2017

jancborchardt commented Dec 11, 2017

jancborchardt commented Dec 15, 2017

MariusBluem commented Dec 15, 2017

jancborchardt commented Dec 15, 2017

MorrisJobke commented Dec 16, 2017

MariusBluem commented Dec 16, 2017


		$hsl = $this->rgbToHsl($rgb[0], $rgb[1], $rgb[2]);

		// Classic formulla to check the brigtness for our eye

Always generate avatar #6876

Always generate avatar #6876

Conversation

rullzer commented Oct 19, 2017 • edited Loading

codecov bot commented Oct 20, 2017 • edited Loading

Codecov Report

rullzer commented Nov 2, 2017

rullzer commented Nov 2, 2017

tobiasKaminsky commented Nov 2, 2017

LukasReschke Nov 3, 2017

Choose a reason for hiding this comment

rullzer Nov 3, 2017

Choose a reason for hiding this comment

LukasReschke Nov 3, 2017

Choose a reason for hiding this comment

LukasReschke Nov 3, 2017

Choose a reason for hiding this comment

LukasReschke Nov 3, 2017

Choose a reason for hiding this comment

LukasReschke Nov 3, 2017

Choose a reason for hiding this comment

MorrisJobke left a comment

Choose a reason for hiding this comment

MorrisJobke commented Dec 8, 2017

skjnldsv commented Dec 8, 2017

MariusBluem commented Dec 8, 2017

MorrisJobke commented Dec 8, 2017

MorrisJobke commented Dec 10, 2017

rullzer commented Dec 11, 2017

MorrisJobke commented Dec 11, 2017

jancborchardt commented Dec 11, 2017

jancborchardt commented Dec 15, 2017

MariusBluem commented Dec 15, 2017

jancborchardt commented Dec 15, 2017

MorrisJobke commented Dec 16, 2017

MariusBluem commented Dec 16, 2017

rullzer commented Oct 19, 2017 •

edited

Loading

codecov bot commented Oct 20, 2017 •

edited

Loading