-
Notifications
You must be signed in to change notification settings - Fork 399
MSC2545: Image Packs (Emoticons & Stickers) #2545
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
base: old_master
Are you sure you want to change the base?
Conversation
People wanting to try out an implementation of custom emotes on the web can use their existing homeserver account through this riot-web instance. |
Fluffychat Android works too :) |
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.
Generally seems to be in a sensible direction, though the duplicated effort for custom emoji is a bit bothersome :(
What about adding a mandatory level of pack indirection ?
and then use what is proposed here inside This allows bundling and updating packs independently, and overcome the 65k limit by quite a margin. |
Not quite sure what you mean, here. That, for room emotes, you can specify other state keys that just extend that one pack? While not explicitly stated in this MSC (yet?), if multiple packs have the same |
Ok nevermind, I misread. I thought |
how hard would it be to add versioning and uuids to emoji packs |
You already have versioning with state events - or do you mean something like That being said, what if multiple packs have the same uuid but different emotes? Merge them? What if you spread one pack over multiple state events to overcome the 65k chars limit and then give them all the same uuid? Should they appear as one pack? How should that work with emote_rooms? |
if emotes are added to a pack, then the version number should be changed, and yeah i meant like a 1.0 thing, if multiple packs have the same uuid and the same version number, but different emotes, then the conflict should be reported to the user, after thinking about it might make more sense to have a last modified date instead of a version number, and it just uses the newest one, because if you want to remove an emote from a pack, then it shouldn't add it back from an older version of the pack, and i don't see how any of this is affected by it being in a room or a user account |
Would a user care about the version of an emote pack? How to determine which version is newer? Grammer for version numbers would need to be defined, etc. |
Like, maybe the uuid and versioning stuff could be added in a separate msc building onto this - the goal of this was to keep things as simple as possible |
ok if one pack was spread over multiple events, i would have them share the uuid and also have part numbers, like "part":1 "part":2 etc for A, mainting the order of emotes, and B, to prevent them from overlapping eachother.
state events have timestamps right, so you could just use the newest state event instead of bothering with version numbers |
origin_server_ts can't be trusted and can easily be forged
What would it need part for? It shouldn't matter if they are ordered correctly or not, emotes are an unordered set |
oh you could just put a modified tag in then with like unixtime or something
the most important reason for the part value is to distinguish continuations of the pack from updates and being able to order the emotes could matter to some pack maintainers, i would want to be able to do that for example |
In a federated system there is no one true timesource. That is a mathematically unsolvable problem.
Updates would replace the previous state event. You don't need any "part" attribute for that. As for ordering, you could say they are ordered alphanumerically by the state key. |
Signed-off-by: SkyePrism [email protected] |
Thank you to @kevincox for bringing this to the attention of the MSC authors.
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.
Hello! I have heavily refactored the MSC in 5663906 to make it clearer.
The one functional change buried in that commit was clarifying that the alt
attribute of a custom emote should be a description of the image, whereas the title
should be the shortcode (what one would expect to appear in a tooltip). This felt more in line with what clients do today, and with the original intention for those attributes in the HTML spec.
Below are some outstanding questions I had after closely reading through the MSC again. Please weigh in to help this MSC move towards FCP 🚂
#### `m.image_pack.rooms` account data event | ||
|
||
The `m.image_pack.rooms` account data event consists of the following: | ||
|
||
- `rooms` **Required, Map[String, Map[String, Object]]**. A map of room ID to | ||
another map: from `state_key` to an empty object. | ||
|
||
This data structure allows specifying a single room image pack given the pack's | ||
room ID and `state_key`. |
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.
Should we combine the m.image_pack
and m.image_pack.rooms
account data events into a single account data event? Such that m.image_pack
would have a rooms
field with the contents of the existing m.image_pack.rooms
account data event?
My initial assumption for why this was originally done was account data event size limits. But actually, I don't think individual account data events have event size limits? Instead, it would make more sense for a homeserver to limit the total amount of account data a user could store.
So, what purpose is there in having two separate account data event types defined?
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 have question(s) about this:
-
Users can define global packs for their account to be used in rooms or users could specify set of emojis only to be used in specific rooms?
-
Would admin/moderator allow and upload what they think its best suiting for room if its community driven
-
Can we limit that specific rooms dont allow emojis or just allow emojis defined by room from question 2
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 wonder, is there precedence for subtyping events like this? I wonder if it is wise to do. That would be an argument in favor of combining.
I would assume this was originally done so m.image_pack
is the same as state and account data event; easier to write parsers and spec. This would be an argument against combining.
Given how widely adopted this MSC is, I wonder how fundamentally we should still change it, or at least consider the impact of these changes to the migrations necessary to implement. I this case the migration should be fairly easy (if .room
s is found in account data, move and delete it).
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.
Users can define global packs for their account to be used in rooms or users could specify set of emojis only to be used in specific rooms?
The former.
Would admin/moderator allow and upload what they think its best suiting for room if its community driven
I'm not sure I understand the question.
Can we limit that specific rooms dont allow emojis or just allow emojis defined by room from question 2
If you're asking whether a room admin can prevent custom emojis from packs that aren't defined in that room, then no. Just as a room admin can't prevent only certain images from being sent in a room today.
They could do it via a bot that analyses messages, but not through the auth rules defined by the spec (at least in today's room versions).
I wonder, is there precedence for subtyping events like this? I wonder if it is wise to do. That would be an argument in favor of combining.
Good question. Looks like there is some precedence similar to the naming convention:
- SSSS:
m.secret_storage.key.<key_id>
,m.cross_signing.master
,m.cross_signing.user_signing
- MSC3890:
org.matrix.msc3890.local_notification_settings.<device_id>
Though I think rather than trying to split up a large event into smaller chunks, this naming is simply a matter of namespacing (i.e. everything really to cross signing should be called m.cross_signing.*
.
So perhaps that's what this proposal was originally aiming to do.
I would assume this was originally done so
m.image_pack
is the same as state and account data event; easier to write parsers and spec. This would be an argument against combining.
Personally I would advise against this, for if m.image_pack
the state event or account data event ever diverge in the spec, such combined implementation will have to be separated again.
I actually don't really see a reason to combine these objects. Plus splitting them up does mean that a client can query them separately if desired. Given it would be more churn for existing implementations of this MSC to switch, I'm minded to keep things how they are.
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.
Looks like there is some precedence similar to the naming convention:
* SSSS: `m.secret_storage.key.<key_id>`, `m.cross_signing.master`, `m.cross_signing.user_signing` * [MSC3890](https://github.com/matrix-org/matrix-doc/pull/3890): `org.matrix.msc3890.local_notification_settings.<device_id>`
These look like different cases? Here in MSC2545 we propose to have both m.image_pack
and m.image_pack.rooms
. Does a m.cross_signing
(not m.cross_signing.*
!) event exist that my Ctrl+F is missing?
Besides that, yes I am sure this proposal was aiming to sort its events into a common namespace, which is more than reasonable.
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.
These look like different cases? Here in MSC2545 we propose to have both
m.image_pack
andm.image_pack.rooms
. Does am.cross_signing
(notm.cross_signing.*
!) event exist that my Ctrl+F is missing?
I suppose they are slightly different.
We could aim for m.image_pack.user
instead of m.image_pack
to be more consistent. It wouldn't break backwards-compatibility either as clients today are using im.ponies.user_emotes
. I'm not too fussed either way though - curious what others think.
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.
🎉 Overall, I can't wait for this to be officially approved. After all, it's shadow-spec for a couple years at this point ;) I do however think that improvements that enable better sharing features in this or a timely followup MSC would be important to enable shipping a complete feature.
#### `m.image_pack.rooms` account data event | ||
|
||
The `m.image_pack.rooms` account data event consists of the following: | ||
|
||
- `rooms` **Required, Map[String, Map[String, Object]]**. A map of room ID to | ||
another map: from `state_key` to an empty object. | ||
|
||
This data structure allows specifying a single room image pack given the pack's | ||
room ID and `state_key`. |
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 wonder, is there precedence for subtyping events like this? I wonder if it is wise to do. That would be an argument in favor of combining.
I would assume this was originally done so m.image_pack
is the same as state and account data event; easier to write parsers and spec. This would be an argument against combining.
Given how widely adopted this MSC is, I wonder how fundamentally we should still change it, or at least consider the impact of these changes to the migrations necessary to implement. I this case the migration should be fairly easy (if .room
s is found in account data, move and delete it).
Co-authored-by: Kim Brose <[email protected]>
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.
Thank you for the feedback, all! I've responded to or incorporated your comments below.
#### `m.image_pack.rooms` account data event | ||
|
||
The `m.image_pack.rooms` account data event consists of the following: | ||
|
||
- `rooms` **Required, Map[String, Map[String, Object]]**. A map of room ID to | ||
another map: from `state_key` to an empty object. | ||
|
||
This data structure allows specifying a single room image pack given the pack's | ||
room ID and `state_key`. |
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.
Users can define global packs for their account to be used in rooms or users could specify set of emojis only to be used in specific rooms?
The former.
Would admin/moderator allow and upload what they think its best suiting for room if its community driven
I'm not sure I understand the question.
Can we limit that specific rooms dont allow emojis or just allow emojis defined by room from question 2
If you're asking whether a room admin can prevent custom emojis from packs that aren't defined in that room, then no. Just as a room admin can't prevent only certain images from being sent in a room today.
They could do it via a bot that analyses messages, but not through the auth rules defined by the spec (at least in today's room versions).
I wonder, is there precedence for subtyping events like this? I wonder if it is wise to do. That would be an argument in favor of combining.
Good question. Looks like there is some precedence similar to the naming convention:
- SSSS:
m.secret_storage.key.<key_id>
,m.cross_signing.master
,m.cross_signing.user_signing
- MSC3890:
org.matrix.msc3890.local_notification_settings.<device_id>
Though I think rather than trying to split up a large event into smaller chunks, this naming is simply a matter of namespacing (i.e. everything really to cross signing should be called m.cross_signing.*
.
So perhaps that's what this proposal was originally aiming to do.
I would assume this was originally done so
m.image_pack
is the same as state and account data event; easier to write parsers and spec. This would be an argument against combining.
Personally I would advise against this, for if m.image_pack
the state event or account data event ever diverge in the spec, such combined implementation will have to be separated again.
I actually don't really see a reason to combine these objects. Plus splitting them up does mean that a client can query them separately if desired. Given it would be more churn for existing implementations of this MSC to switch, I'm minded to keep things how they are.
This comment was marked as off-topic.
This comment was marked as off-topic.
All threads that don't require input from a wider audience have been resolved. As such, I propose this MSC be considered by the rest of the SCT: @mscbot fcp merge |
Team member @mscbot has proposed to merge this. The next step is review by the rest of the tagged people: Concerns:
Once at least 75% of reviewers approve (and there are no outstanding concerns), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for information about what commands tagged team members can give me. |
MSCs proposed for Final Comment Period (FCP) should meet the requirements outlined in the checklist prior to being accepted into the spec. This checklist is a bit long, but aims to reduce the number of follow-on MSCs after a feature lands. SCT members: please check off things you check for, and raise a concern against FCP if the checklist is incomplete. If an item doesn't apply, prefer to check it rather than remove it. Unchecking items is encouraged where applicable. Checklist:
@mscbot concern checklist incomplete |
Mainly so the discussion threads do not get lost.
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.
thanks for working on this. I haven't read the MSC in detail, but the parts I did read look more than sufficient for starting FCP in my opinion.
proposals/2545-emotes.md
Outdated
1. User image pack (defined in the user's account data) | ||
2. Image pack rooms (defined in the `m.image_pack.rooms` user account data | ||
object) | ||
3. Space image packs (defined in the hierarchy of canonical spaces for the | ||
current room) | ||
4. Room image packs (defined in the currently open room's state) | ||
5. Referenced room image packs (defined in the `m.image_pack.rooms` room | ||
state event) |
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 would expect the room to take priority over the space since it is more "local" to the current context.
|
||
### Shortcode grammar | ||
|
||
A shortcode's length MUST not exceed 100 bytes. This restriction MUST be |
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.
Does it need to be valid Unicode?
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.
Good point! I've done some research to try and figure this out. To clarify, there's a difference between "valid UTF-8 byte sequences" and "valid Unicode scalar values".
Not every byte sequence can be decoded as valid UTF-8. Therefore, if we allowed any sequence of bytes (as the proposal currently does), text decoders may panic upon attempting to blindly decode them. For this reason, we absolutely should restrict the grammar to only "byte sequences that can be decoded as UTF-8".
Separately, the set of "valid Unicode scalar values" is a range that has been stabilised since 2003 (RFC 3629) and is unlikely to change. We could restrict the grammar to this range to avoid causing any Unicode parsers to fail. This seems reasonable to me.
The Unicode consortium assign new codepoints every year in each Unicode update. It would be foolish to restrict to only the currently valid set of assigned codepoints, as that would break forwards-compatibility.
Given the above, the grammar rules could be:
- The value must be a well-formed UTF-8 byte sequence;
- After decoding, every code point must be a Unicode scalar (i.e. not a surrogate or noncharacter);
- Length ≤ 100 bytes;
- Must not contain
:
(U+003A
) orU+0020
).
What do people think?
If there are multiple emotes with the same shortcode available, the client could | ||
for example slugify the containing pack's display name and prepend it to each | ||
emote's shortcode with a separator character (i.e. `~`). For instance, the user | ||
could enter `:my-pack~emote:` to select an image with shortcode `emote` in the | ||
pack `my pack`. |
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 could still have conflicts, maybe using : as a separator makes more sense? :my-pack:emote:
?
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.
Indeed, it will conflict, good point! And :
is much easier to type.
It's important to note that this is purely a suggestion for clients to add a feature to auto-complete emotes when typing. It does not need to be a unique representation of the emote. For instance, two image packs could have the same exact name as well.
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.
Due to image pack names not being globally unique, I think this suggestion of the proposal doesn't work:
if the user enters
:emote:
in a message, the client could replace that text with the corresponding emote based on the shortcode.
If there are two image packs with the same name, then the parser won't be able to deterministically figure out which one to pick. The only solution to that is to include the room ID and event ID - but users aren't going to use that :)
I'm going to strip that suggestion as a result and replace it with a warning. But the search UI is still a nice idea (bonus points for fuzzy search!).
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.
Upon giving this some more thought, using :
again as the separator does limit client functionality.
If a user has typed That was funny :smile:
into a message window, upon hitting the second :
, a client's emote UI component would not easily know whether the user:
- Wants to start filtering for results from the "smile" image pack, or
- Close the search UI (and potentially pick the last highlighted emote).
Whereas typing a distinct character, like ~
would signal 1, and typing :
would signal 2.
So, to give clients' more flexibility in designing their UI/UX, I think keeping a distinct character would make sense. However, that does mean that we'll want to exclude that character from the shortcode grammar. Otherwise, as @clokep says, we'll get conflicts if that character is part of the shortcode itself.
Looking at other third-party messaging apps;
- Discord uses
<:name:ID>
(<
and>
are boundaries,:
is the separator) - Slack uses
:pack/shortcode:
I like /
as a separator, more than ~
. ~
is harder to type for more people, and /
already universally indicates "hierarchy" thanks to filesystems and web addresses.
Alternatively, we could use
(space) which is already barred by the grammar (to make shortcodes easier to read). But I feel that's a fairly confusing UX for a separator, versus /
.
Rendered
Signed-off-by: Sorunome [email protected]
Author: @Sorunome
Shepherd: @anoadragon453
Future MSCs
Current implementations
Emote rendering (rendering of the
<img>
tag)data-mx-emoticon
)data-mx-emoticon
)Sending, using the mentioned events here
Bridges
data-mx-emoticon
only)Implementation PRs
FluffyChat
Data model: https://gitlab.com/famedly/company/frontend/libraries/matrix_api_lite/-/merge_requests/26
SDK: https://gitlab.com/famedly/company/frontend/famedlysdk/-/merge_requests/726
Emoticons: https://gitlab.com/famedly/fluffychat/-/merge_requests/433
Stickers: https://gitlab.com/famedly/fluffychat/-/merge_requests/452
Nheko
Stickers: Nheko-Reborn/nheko#648
Sticker editor: Nheko-Reborn/nheko#669
Choosing emoticons: Nheko-Reborn/nheko@ea6b19b
Cinny
Emoticons and Stickers: cinnyapp/cinny#686
kazv
Creating and sending stickers: https://lily-is.land/kazv/kazv/-/merge_requests/71
SCT:
FCP tickyboxes
MSC Checklist