Always generate avatar by rullzer · Pull Request #6876 · nextcloud/server

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

 	/**
 * @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

+
+ $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

+ * @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

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


/Heu/Hue/ maybe?

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

 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 949fdc8Compare November 8, 2017 12:20

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

rullzer force-pushed the always_img_avatar branch from 3e2ca98 to 2c67928Compare 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 8e8fe6bCompare 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 masterDec 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


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

		// Classic formulla to check the brigtness for our eye

Uh oh!

Uh oh!

Conversation

rullzer commented Oct 19, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecovBot commented Oct 20, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

rullzer commented Nov 2, 2017

Uh oh!

rullzer commented Nov 2, 2017

Uh oh!

tobiasKaminsky commented Nov 2, 2017

Uh oh!

LukasReschkeNov 3, 2017

Choose a reason for hiding this comment

Uh oh!

rullzerNov 3, 2017

Choose a reason for hiding this comment

Uh oh!

LukasReschkeNov 3, 2017

Choose a reason for hiding this comment

Uh oh!

LukasReschkeNov 3, 2017

Choose a reason for hiding this comment

Uh oh!

LukasReschkeNov 3, 2017

Choose a reason for hiding this comment

Uh oh!

LukasReschkeNov 3, 2017

Choose a reason for hiding this comment

Uh oh!

MorrisJobke left a comment

Choose a reason for hiding this comment

Uh oh!

MorrisJobke commented Dec 8, 2017

Uh oh!

skjnldsv commented Dec 8, 2017

Uh oh!

MariusBluem commented Dec 8, 2017

Uh oh!

MorrisJobke commented Dec 8, 2017

Uh oh!

MorrisJobke commented Dec 10, 2017

Uh oh!

rullzer commented Dec 11, 2017

Uh oh!

MorrisJobke commented Dec 11, 2017

Uh oh!

jancborchardt commented Dec 11, 2017

Uh oh!

jancborchardt commented Dec 15, 2017

Uh oh!

MariusBluem commented Dec 15, 2017

Uh oh!

jancborchardt commented Dec 15, 2017

Uh oh!

MorrisJobke commented Dec 16, 2017

Uh oh!

MariusBluem commented Dec 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

rullzer commented Oct 19, 2017•
edited
Loading

codecovBot commented Oct 20, 2017•
edited
Loading