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

Add backend for conversation avatars #4737

Closed
wants to merge 12 commits into from

Conversation

danxuliu
Copy link
Member

@danxuliu danxuliu commented Dec 10, 2020

Requires nextcloud/server#24579 The generic avatar system will not be available in Nextcloud 21, so a specific Talk endpoint was added instead.

Fixes (the backend part of) #927

The avatar endpoint returns custom room avatars (that is, avatars that have been explicitly set by a participant) as well as one-to-one room avatars (the avatar of the other participant in the conversation). Other room avatars are just default icons (icon-public, icon-contacts...) and getting the avatar in that case returns an error 404.

Room list entries now also return avatarId and avatarVersion parameters. The first can be used to know the type of avatar (custom, user, icon-public...), and the second to know when an avatar has been updated. Due to all this a client should get the avatar whenever the version changes, but only if the avatar id is custom or user.

As the avatar is provided in an OCS endpoint and thus requires the OCS-APIRequest header to be set it is not possible to just set the endpoint URL as the URL of an HTML image element or a CSS background image. Instead the request needs to be done specifying that a Blob is expected and then getting the URL for that Blob. For example, something like:

try {
	const avatar = await axios.get(generateOcsUrl('apps/spreed/api/v3/avatar', 2) + token + '/' + size + '?version=' + version, {
		responseType: 'blob',
	})
	avatar = URL.createObjectURL(avatar.data)
} catch (exception) {
}

The original idea was that the endpoint would return default avatars too. Unfortunately this was more complex than anticipated; IAvatar does not support SVGs, and converting from SVG to PNG (which might be needed even if SVGs can be used for the maximum compatibility with clients) is only possible when ImageMagick is available. As the default avatars are common to all the conversations of the same type and the clients should be able to provide the icons by themselves for the time being this should be good enough.

Pending:

  • Fix migration
  • Bump room avatar version when the user avatar of a one-to-one conversation changes
  • Extend OC\Avatar\Avatar instead of implementing OCP\IAvatar to get the implementation of [avatarBackgroundColor](https://github.com/nextcloud/server/blob/caff1023ea72bb2ea94130e18a2a6e2ccf819e5f/lib/private/Avatar/Avatar.php#L298]? I am not a fan of extending private classes, but maybe in this case it would be acceptable to avoid code duplication - The method is not really needed, so a dummy implementation was added instead
  • Prevent participants other than moderators to modify the avatar
  • Find a folder name that can not clash with a user id
  • Return avatar id (icon-public, icon-group, icon-mime-audio, icon-mime-text, custom...) when fetching room information
    • This would allow the clients to avoid fetching the same default icons for different rooms
  • Add system message when the room avatar is changed

Future:

  • Return default avatars (user avatar in one-to-one, mime type icons in file/password request rooms, public and group icons in others)
  • Convert transparency to white in PNGs - That would work only when a pixel is fully transparent, but if a pixel is half-transparent making it white would look like a bug to the user. Moreover, if the alpha channel is just removed the resulting pixel will have whichever colour it had, which is not necessarily white; that would require extra code. Also the regular icons all have a transparent background, so the WebUI (I have not checked the mobile clients) adds an explicit background. Due to all that converting the transparency to white will not be done, at least, in this initial pull request.

Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

Find a folder name that can not clash with a user id

Talk in appdata is fine

Add system message when the room avatar is changed

Yes

lib/Avatar/RoomAvatarProvider.php Outdated Show resolved Hide resolved
@danxuliu danxuliu force-pushed the add-backend-for-conversation-avatars branch 2 times, most recently from 09ab900 to 37c62f5 Compare December 31, 2020 01:13
@danxuliu danxuliu marked this pull request as ready for review December 31, 2020 01:24
@nickvergessen
Copy link
Member

nickvergessen commented Jan 4, 2021

Conflicting files
tests/php/CapabilitiesTest.php

Afterwards it will have a conflict with #4870 => merged

Also someone brought up that the OCS endpoint for the avatar is a bad idea, because it needs the OCS-APIRequest: true header which prevents caching or something like this in mobile clients/browsers. Now I can't remember who it was, but it sounds like a valid draw back to me, and maybe the get-endpoint needs to be a public one on an index.php route instead?

The RoomAvatar does not provide (yet) a default avatar for a room if it
has not been set.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Now the avatar of the other user will be returned when getting the
avatar of a one-to-one conversation.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Although the avatar itself is stored in the app data the avatar id and
its version are stored in the database for convenience.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
The image itself is always got directly from the user avatar, so it is
always up to date. However, as the version is specific to the room
avatar and stored in the database it needs to be explicitly bumped.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
The generic avatar system will not be included in Nextcloud 21, so for
the time being Talk needs to provide its own endpoints for room avatars.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@danxuliu
Copy link
Member Author

danxuliu commented Jan 4, 2021

At least in the browser OCS requests are cached (I have verified it on #4872). If mobile apps can not cache OCS requests then yes, it would be better to move the get endpoint to something cacheable. But it would be good to get a confirmation that it does not work before doing the change. Or should it be a index.php route even if the cache works for mobile apps?

@danxuliu danxuliu force-pushed the add-backend-for-conversation-avatars branch from 7332d60 to 74cdd1e Compare January 4, 2021 16:34
@nickvergessen nickvergessen removed this from the 💚 Next Alpha (21) milestone Jan 7, 2021
@nickvergessen
Copy link
Member

I will close the PR for now (as there was no active development since 6 months). Anyone can pick it up whenever they want and reopen the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants