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

Introduces avatars for guest users #10248

Merged
merged 4 commits into from
Feb 7, 2019

Conversation

weeman1337
Copy link
Member

The main work done here:

  • Refactoring of the Avatar class: UserAvatar and GuestAvatar
  • The UserAvatar works like the previous Avatar class
  • The GuestAvatar creates an avatar image without an user and the functions to store or update the avatar
  • For the guests there is an ISimpleFile InMemoryFile that isn't stored. I decided for that way because so the controller part can keep being like it's now and there won't be a mass of guest avatar files over time.
  • I saw that ISimpleFile now supports reading/writing via a resource handle. I didn't implement it for the InMemoryFile. I'm not 100% happy with the InMemoryFile. If you have any ideas how to do it without changing all the Avatar interfaces and the file handling through the controller please let me know!
  • I didn't introduce caching, because the controller already does it.

Closes #9457

@weeman1337 weeman1337 changed the title Enhancement/9457 2 Introduces avatars for guest users Jul 15, 2018
@weeman1337 weeman1337 added enhancement 3. to review Waiting for reviews labels Jul 23, 2018
@weeman1337 weeman1337 requested a review from skjnldsv July 23, 2018 18:43
@skjnldsv
Copy link
Member

I'm liking this already!! This is how I viewed the global structure of this feature, seems like a nice job!! 🎉

i'll take a look at it tomorrow :)

@mario
Copy link
Contributor

mario commented Jul 23, 2018

Is it still the same endpoint? How does it work?

@nickvergessen
Copy link
Member

Is it still the same endpoint? How does it work?

We will notify you once this is merged and something you can use ;)

@MorrisJobke MorrisJobke mentioned this pull request Jul 24, 2018
21 tasks
@MorrisJobke MorrisJobke added this to the Nextcloud 14 milestone Jul 24, 2018
@MorrisJobke
Copy link
Member

Most likely nothing for 14 -> moving to 15.

@weeman1337 Sorry that this didn't made it into 14, but at some point we need to make a cut.

@jospoortvliet
Copy link
Member

To make up for that @weeman1337 any chance we can offer you some beers at the nextcloud conference?

@weeman1337
Copy link
Member Author

Most likely nothing for 14 -> moving to 15

I'm okay with that. I'd like more to see a stable release than having avatars for guests now ;)

any chance we can offer you some beers at the nextcloud conference?

At the same weekend the FrOSCon is happening and I'm helping out there at the FSFE booth. There is a chance I can make it to some of the hackdays...

@jospoortvliet
Copy link
Member

At the same weekend the FrOSCon is happening and I'm helping out there at the FSFE booth.

Pretty damn good excuse 👍

There is a chance I can make it to some of the hackdays...

That would 🤘

@ChristophWurst
Copy link
Member

@weeman1337 could you take a look at the failing tests? Is this ready for a final review/integration?

@weeman1337
Copy link
Member Author

@ChristophWurst let me do a rebase and then let's see how the tests are doing. Currently the first test thing failes, but I don't get why.. o_O

@kesselb
Copy link
Contributor

kesselb commented Sep 5, 2018

image

@weeman1337
Copy link
Member Author

Thanks @danielkesselberg . Anyway would be cool to see in the test result why it failed..

@ChristophWurst
Copy link
Member

Anyway would be cool to see in the test result why it failed..

You can find it at https://drone.nextcloud.com/nextcloud/server/9938/86 (line 87), but I agree that it's hard to locate.

@MorrisJobke
Copy link
Member

CI failures are unrelated

lib/public/IAvatar.php Outdated Show resolved Hide resolved
@kesselb
Copy link
Contributor

kesselb commented Dec 9, 2018

@weeman1337 🏓

@kesselb
Copy link
Contributor

kesselb commented Jan 1, 2019

Copy link
Contributor

@kesselb kesselb left a comment

Choose a reason for hiding this comment

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

Works 👍 An avatar is returned instead of 404.

lib/private/Avatar/UserAvatar.php Outdated Show resolved Hide resolved
lib/private/Avatar/UserAvatar.php Show resolved Hide resolved
@weeman1337 weeman1337 added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jan 4, 2019
@weeman1337
Copy link
Member Author

@danielkesselberg all done ✓

@kesselb
Copy link
Contributor

kesselb commented Jan 5, 2019

Mind to have another look? @rullzer @MorrisJobke @nickvergessen @juliushaertl @skjnldsv

lib/private/Avatar/AvatarManager.php Outdated Show resolved Hide resolved
@weeman1337
Copy link
Member Author

The guest avatars are now available under the path /avatar/guest/{guestName}/{size}.

@juliusknorr
Copy link
Member

One other issue I found is that guest avatars are svg while regular avatars are converted to png. We actually should also provide them as png (since iOS doesn't support svg rendering properly iirc) Also fonts are not properly loaded otherwise when using it outside of Nextcloud:

image

@weeman1337
Copy link
Member Author

weeman1337 commented Jan 29, 2019

@juliushaertl the guest avatars are now PNGs.

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Tested and works 👍

@MorrisJobke
Copy link
Member

I pushed a fix for the PHPDoc of the new OCP file. Then this can be merged 👍

@MorrisJobke 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 Feb 1, 2019
*/
public function getFile() {
return $this->file;
}
Copy link
Member

Choose a reason for hiding this comment

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

I don't like this because it is only needed for the tests :/

weeman1337 and others added 4 commits February 7, 2019 14:23
Signed-off-by: Michael Weimann <mail@michael-weimann.eu>
Signed-off-by: Michael Weimann <mail@michael-weimann.eu>
Signed-off-by: Morris Jobke <hey@morrisjobke.de>
Signed-off-by: Morris Jobke <hey@morrisjobke.de>
@MorrisJobke
Copy link
Member

I just pushed a fix for the old composer ClassLoader.php. Thus I will merge this now.

@MorrisJobke MorrisJobke merged commit 32a6f4b into nextcloud:master Feb 7, 2019
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.

10 participants