-
Notifications
You must be signed in to change notification settings - Fork 385
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
Sticker messages (m.sticker) #1158
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, really clear!
a few niggles below
specification/modules/stickers.rst
Outdated
|
||
.. _module:stickers: | ||
|
||
This module allows users to send sticker messages in to rooms or direct messaging sessions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please could you wrap at 80 cols as per https://github.com/matrix-org/matrix-doc/blob/master/meta/documentation_style.rst#line-widths?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, sorry for missing that. Now wrapped.
specification/modules/stickers.rst
Outdated
|
||
This module allows users to send sticker messages in to rooms or direct messaging sessions. | ||
|
||
Sticker messages are specialised image messages that are displayed without controls (e.g. no "download" link, or light-box view on click, as would be displayed for for m.room.message#m.image events). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be nice to link to the m.image definition. I think something as simple as `m.image`_ might work.
specification/modules/stickers.rst
Outdated
|
||
Clients supporting this message type should display the image content from the event URL directly in the timeline. | ||
|
||
A thumbnail image should be provided in the message info. object. This is largely intended as a fallback for clients that do not fully support the m.sticker event type. Im most cases it is fine to set the thumbnail URL to the same URL as the main event content. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think just ``info`` (rather than 'message info.') here.
double-backticks around m.sticker.
s/Im/In/
specification/modules/stickers.rst
Outdated
|
||
A thumbnail image should be provided in the message info. object. This is largely intended as a fallback for clients that do not fully support the m.sticker event type. Im most cases it is fine to set the thumbnail URL to the same URL as the main event content. | ||
|
||
It is recommended that sticker image content should be approximately 400x400 pixels in size (or smaller). The image dimensions specified in the info. object of the message should be half of the original image dimensions in order to display correctly on retina displays. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
info as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm failing to grok the idea behind the retina display thing tbh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is intended as a way of ensuring that the images are displayed as crisply as possible on high resolution displays. If the physical image is double the size of the event dimensions metadata it will be scaled down on normal resolution screens (by displaying every second pixel). On 2x (retina screens) the full resolution of the image will be used, resulting in a crisper image on displays that support it. - This was @ara4n's suggestion originally, as a short-term solution. However in the longer term we probably need to improve this by getting the media server to be able to render content for different DPI devices, or by specifying alternate display content in the sticker metadata.
specification/modules/stickers.rst
Outdated
|
||
A thumbnail image should be provided in the message info. object. This is largely intended as a fallback for clients that do not fully support the m.sticker event type. Im most cases it is fine to set the thumbnail URL to the same URL as the main event content. | ||
|
||
It is recommended that sticker image content should be approximately 400x400 pixels in size (or smaller). The image dimensions specified in the info. object of the message should be half of the original image dimensions in order to display correctly on retina displays. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just for compatibility with telegram, can we bump the recommendation to 512x512?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. Updated.
Thanks @richvdh, @turt2live - All of that feedback should be incorporated now. @ara4n - Anything else (or are we good to merge)? |
the only thing that worries me here is whether we're doing m.sticker (which implies that it should be using whatever the final event format proposal is) or m.room.sticker. I think I'm tempted slightly towards m.room.sticker for now given it's more consistent with the other stuff currently and we haven't decided on the final prefix strategy going forwards. |
apparently i explicitly asked rick to |
specification/modules/stickers.rst
Outdated
same URL as the main event content. | ||
|
||
It is recommended that sticker image content should be approximately 512x512 | ||
pixels in size (or smaller). The image dimensions specified in the info. object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the full stop in ...in the info. object...
is making this somewhat hard to read. Maybe make info styled as info
instead (without a full stop)?
As for suggested wording re: retina, how does this sound?
It is recommended that stickers be 512x512 pixels or smaller. The image dimensions specified in the image info should be half of the real dimensions (eg: 256x256) to assist rendering on higher DPI screens.
(incorporates some of the explanation from the lost thread in this area)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated 'info.' to info
(to match the other occurrence in the previous paragraph).
I have also incorporated your suggested wording with some minor tweaks.
In Telegram and Facebook Messenger, the stickers are organized inside packs (Sticker Packs), when you want to "install" an sticker in your device, you have to install the entire pack. This puts the stickers in "categories", in a similar way to the emojis. Ans this is consistent in all clients of these apps (web, desktop, mobile). Maybe this should be addressed here? |
Hi @heitorPB - Thanks for the question. As you suggest, our intention is to organise stickers in to "Sticker packs" - it's certainly how we're going to be doing it in Scalar (the New Vector integration manager). We'll be releasing our implementation of stickers / stickerpacks soon, which should serve as an example of how others might want to implements stickerpacks. However, I am a little reluctant to put details of organising stickers into stickerpacks in to the spec. as that is really up to the integration manager to decide how they want to organise and distribute stickers. |
@rxl881 Could you expand on the above? How is a sticker pack, which is a client-side item, can be present on an integration manager? |
the point is that an m.sticker is just a fixed sized image. the widget (or client or integration mgr or whatever) that you use to organise them and emit them is left undefined for maximum flexibility, at least for now or until we see how folks use them. |
A small description about stickers in Telegram is here: In Telegram, anyone can create a sticker pack. After you create a sticker pack, only you can edit it (add new stickers, remove, reorder, ...). This approach allows some kind of 'organization' for the stickers: if you have a pack named "animals" you dont't expect it to have flags of countries there. Also, in Telegram the stickers have a maximum image size (512x512) and are PNGs, animated stickers are not allowed at the moment. It makes no sense to have huge images as stickers, as they don't fit a mobile screen and their purpose is not to show an image on the screen. And it would be very nice to have the possibility of animated stickers :) |
specification/modules/stickers.rst
Outdated
|
||
{{m_sticker_event}} | ||
|
||
Client behaviour | ||
---------------- | ||
|
||
Clients supporting this message type should display the image content from the event URL directly in the timeline. | ||
Clients supporting this message type should display the image content from the | ||
event URL directly in the timeline. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we might want to have some warning about sanitising URLs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed, hopefully clarifying that we expect these to be valid MXC URIs should be enough(?)
specification/modules/stickers.rst
Outdated
It is recommended that sticker image content should be approximately 400x400 pixels in size (or smaller). The image dimensions specified in the info. object of the message should be half of the original image dimensions in order to display correctly on retina displays. | ||
It is recommended that sticker image content should be 512x512 pixels in size | ||
or smaller. The image dimensions specified in the ``info`` object of the | ||
message should be half of the original image dimensions in order to assist |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as per https://matrix.to/#/!HCXfdvrfksxuYnIFiJ:matrix.org/$1521800047436EfeUZ:sw1v.org, I am still WTFing at this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok I think convo there has clarified my confusion; however i would make two changes:
- Clarify (presumably in the ImageInfo schema) that the dimensions given are the intended display size, as opposed to the intrinsic size of the image file.
- To me, your description "info should be half the image dimensions" is backwards and it would seem more logical to me to phrase it as "the image should be twice the intended display size given in
info
" or words to that effect.
sorry for sitting on this for days btw |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GitHub mobile doesn't let me do line comments on their own.
@richvdh -- Do the latest updates address your concerns / comments? |
event-schemas/schema/m.sticker
Outdated
@@ -18,7 +18,8 @@ properties: | |||
Metadata about the image referred to in ``url`` including a thumbnail | |||
representation. | |||
url: | |||
description: The URL to the sticker image. | |||
description: |- | |||
The URL to the sticker image. This must be a valid ``mxc://`` URI. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, if we're expecting clients to check this, we should probably say so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
\o/ lgtm
…g#1158) * Fix unintentional stateres change added in matrix-org#1042 * Changelog
No description provided.