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

Merged
merged 19 commits into from Mar 27, 2018

Conversation

Projects
None yet
6 participants
@rxl881
Contributor

rxl881 commented Mar 14, 2018

No description provided.

rxl881 added some commits Mar 14, 2018

@richvdh

This looks great, really clear!

a few niggles below

.. _module:stickers:
This module allows users to send sticker messages in to rooms or direct messaging sessions.

This comment has been minimized.

This comment has been minimized.

@rxl881

rxl881 Mar 15, 2018

Contributor

Ahh, sorry for missing that. Now wrapped.

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).

This comment has been minimized.

@richvdh

richvdh Mar 14, 2018

Member

might be nice to link to the m.image definition. I think something as simple as `m.image`_ might work.

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.

This comment has been minimized.

@richvdh

richvdh Mar 14, 2018

Member

I think just ``info`` (rather than 'message info.') here.

double-backticks around m.sticker.

s/Im/In/

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.

This comment has been minimized.

@richvdh

richvdh Mar 14, 2018

Member

info as above

This comment has been minimized.

@richvdh

richvdh Mar 14, 2018

Member

I'm failing to grok the idea behind the retina display thing tbh

This comment has been minimized.

@rxl881

rxl881 Mar 15, 2018

Contributor

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.

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.

This comment has been minimized.

@turt2live

turt2live Mar 14, 2018

Member

just for compatibility with telegram, can we bump the recommendation to 512x512?

This comment has been minimized.

@rxl881

rxl881 Mar 15, 2018

Contributor

Sure. Updated.

rxl881 added some commits Mar 15, 2018

@rxl881

This comment has been minimized.

Contributor

rxl881 commented Mar 15, 2018

Thanks @richvdh, @turt2live - All of that feedback should be incorporated now.

@ara4n - Anything else (or are we good to merge)?

@ara4n

This comment has been minimized.

Member

ara4n commented Mar 15, 2018

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.

@ara4n

This comment has been minimized.

Member

ara4n commented Mar 15, 2018

apparently i explicitly asked rick to s/m.room.sticker/m.sticker/g in some flu-riddled malaise last week, so let's stick with it. it may end up being an odd one out, but sobeit.

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

This comment has been minimized.

@turt2live

turt2live Mar 15, 2018

Member

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)

This comment has been minimized.

@rxl881

rxl881 Mar 15, 2018

Contributor

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.

rxl881 added some commits Mar 15, 2018

@heitorPB

This comment has been minimized.

heitorPB commented Mar 18, 2018

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?

@rxl881

This comment has been minimized.

Contributor

rxl881 commented Mar 20, 2018

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.

@maxidor

This comment has been minimized.

Contributor

maxidor commented Mar 20, 2018

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?
It suddenly feels like there is much more to this than just being an fixed size image...

@ara4n

This comment has been minimized.

Member

ara4n commented Mar 20, 2018

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.

@heitorPB

This comment has been minimized.

heitorPB commented Mar 20, 2018

A small description about stickers in Telegram is here:
https://telegram.org/blog/stickers

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 :)

@rxl881 rxl881 assigned richvdh and unassigned ara4n Mar 22, 2018

{{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.

This comment has been minimized.

@richvdh

richvdh Mar 23, 2018

Member

we might want to have some warning about sanitising URLs?

This comment has been minimized.

@rxl881

rxl881 Mar 23, 2018

Contributor

As discussed, hopefully clarifying that we expect these to be valid MXC URIs should be enough(?)

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

This comment has been minimized.

This comment has been minimized.

@richvdh

richvdh Mar 23, 2018

Member

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.
@richvdh

This comment has been minimized.

Member

richvdh commented Mar 23, 2018

sorry for sitting on this for days btw

@richvdh richvdh removed their assignment Mar 23, 2018

@turt2live

GitHub mobile doesn't let me do line comments on their own.

@@ -2,10 +2,14 @@ $schema: http://json-schema.org/draft-04/schema#
description: Metadata about an image.
properties:
h:
description: The height of the image in pixels.
description: |-

This comment has been minimized.

@turt2live

turt2live Mar 23, 2018

Member

This now changes how m.image events are structured. Can stickers not send their actual dimensions like regular images and clients can be responsible for rendering?

This comment has been minimized.

@turt2live

turt2live Mar 23, 2018

Member

For instance, the spec could be changed to prefer the thumbnail instead of having it purely as a fallback. This way the sticker's real dimensions are in the event and the thumbnail is halved.

This comment has been minimized.

@rxl881

rxl881 Mar 23, 2018

Contributor

This isn't intended to change the way in which the event is structured (it should be exactly the same as previously). The additional text is simply intended to clarify that the specified width and height are the display dimensions, rather than needing to be the same as the physical dimensions of the underlying file.

I'm not sure if you saw the update to stickers.rst (which might help to clarify the reasoning behind this)?

The dimensions of the image file should be twice the intended
display size specified in the info object in order to assist
rendering sharp images on higher DPI screens.

This comment has been minimized.

@turt2live

turt2live Mar 23, 2018

Member

I understand that stickers are the target for this change, however this is still a change to m.image due to it being in a shared schema.

The wording has effectively changed from "these are the image dimensions" to "these are the recommended dimensions to display the image", which is a large enough difference that I'd like to see it addressed correctly (at least flagged as a change to m.image, but better handled as stickers being more compatible with the existing verbage).

This comment has been minimized.

@richvdh

richvdh Mar 23, 2018

Member

@turt2live I think the question is over what this field actually meant before, and in particular what the expected behaviour would have been had it been different to the physical height of the underlying file.

I must admit that when I first saw this my reaction was similar to yours, but on reflection I think it would have been nonsensical for clients to take this field as anything other than the intended display size - after all there's not actually much value in telling them the actual height of the image in the file.

So rather than being a change to how m.image works, I think this actually comes down to a clarification of the existing situation.

Nevertheless we should call this out in the changelog. In fact this whole thing needs adding to the changelog. @rxl881 could you add something to https://github.com/matrix-org/matrix-doc/blob/761ad9820ffce34b398cfad0fc1daca51062b66e/changelogs/client_server.rst?

This comment has been minimized.

@rxl881

rxl881 Mar 23, 2018

Contributor

Thanks for clarifying that @richvdh. Sure, will update the changelog now.

This comment has been minimized.

@turt2live

turt2live Mar 23, 2018

Member

Can we also "clarify" the thumbnail info object at the same time then, please?

This comment has been minimized.

@rxl881

rxl881 Mar 23, 2018

Contributor

@turt2live - Sure, good point (I have done so).

Changelog updated (hopefully not too verbose).

@rxl881

This comment has been minimized.

Contributor

rxl881 commented Mar 23, 2018

@richvdh -- Do the latest updates address your concerns / comments?

@@ -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.

This comment has been minimized.

@richvdh

richvdh Mar 23, 2018

Member

well, if we're expecting clients to check this, we should probably say so.

rxl881 added some commits Mar 23, 2018

@rxl881 rxl881 assigned richvdh and unassigned rxl881 Mar 26, 2018

@richvdh

\o/ lgtm

@richvdh richvdh merged commit 8d05f80 into master Mar 27, 2018

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/travis-ci/push The Travis CI build could not complete due to an error
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment