-
Notifications
You must be signed in to change notification settings - Fork 416
MSC4356: Recently used emoji #4356
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: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Johannes Marbach <[email protected]>
a5ff1ba to
ad61e4f
Compare
|
|
||
| ## Proposal | ||
|
|
||
| A new global account data event `m.recent_emoji` is introduced. In `content`, it contains a single |
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 know that the plural form of emoji can be emoji but to avoid confusion, maybe using m.recent_emojis help to clarify that we have a list of emojis?
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.
Client developers should be able to determine the type of a field from the spec, rather than relying on the field name.
As a native English speaker, recent_emoji sounds best to me.
Signed-off-by: Johannes Marbach <[email protected]>
Signed-off-by: Johannes Marbach <[email protected]>
anoadragon453
left a comment
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 writing this up! 😁
|
|
||
| ## Proposal | ||
|
|
||
| A new global account data event `m.recent_emoji` is introduced. In `content`, it contains a single |
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.
Client developers should be able to determine the type of a field from the spec, rather than relying on the field name.
As a native English speaker, recent_emoji sounds best to me.
Co-authored-by: Andrew Morgan <[email protected]>
Signed-off-by: Johannes Marbach <[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.
I think this is practically ready to go.
|
@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. MSC authors: feel free to ask in a thread on your MSC or in the#matrix-spec:matrix.org room for clarification of any of these points.
|
|
@mscbot concern checklist incomplete (any SCT member can resolve this) |
Signed-off-by: Johannes Marbach <[email protected]>
| ``` json5 | ||
| { | ||
| "type": "m.recent_emoji", | ||
| "content": { | ||
| "recent_emoji": [ | ||
| [ "😅", 7 ], // Most recently used, 7 times overall | ||
| [ "👍", 84 ], // Second most recently used, 84 times overall | ||
| ... | ||
| } | ||
| } | ||
| ``` |
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 proposal was written by reverse-engineering existing implementations in the Element clients which use this format. One glaring question here is if the nested-array storage is actually a good idea or not?
This scheme is very compact but given request compression and the typical amount of emoji, any performance benefits resulting from it are probably negligible.
Furthermore, the nested array doesn't lend itself well to future extension. We could just append more array elements and assign special semantics to certain array indexes. This feels somewhat brittle and unnecessarily implicit, however.
I cannot currently think of a possible future extension of the tracked data. So maybe this concern is moot to begin with. I'd still be interested to hear other thoughts on this though.
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 agree; as for possible extensions, I can imagine adding dates, or required custom fonts/emoji packs, so the inner array certainly better be a dictionary. As for the outer one, I first thought of a dictionary keyed with the emoji but then custom fonts may encode different emojis with the same code point; so probably an array is appropriate here. Another benefit of the array on the outer level is that a naïve client implementation can just take it as is while a dictionary would have to then be sorted by date or frecency or whatever.
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.
Tracking custom emoji (MSC2545) is a good shout. Having an emoji act as the keys to a dictionary doesn't quite work in that case.
If we wanted to keep the nested array structure, one could say:
-
If the first array element is a string, then assume that this is a unicode emoji 🙂
-
If the first array element is an object, then assume a non-unicode emoji and the following structure:
{ "image_url": "mxc://example.org/blah", "image_pack_room": "!mDgU8OWQ6CAgEK-Fb2Dhv0fR0nHs_tEM_Na93SSIhu0", "image_pack_event": "$OMn1hZchIFSrWDNgBQj6MQkojvnGPR6tL7su8IkHW_Y" }
Example:
{
"type": "m.recent_emoji",
"content": {
"recent_emoji": [
[ "😅", 7 ],
[
{
"image_url": "mxc://example.org/blah",
"image_pack_room": "!mDgU8OWQ6CAgEK-Fb2Dhv0fR0nHs_tEM_Na93SSIhu0",
"image_pack_event": "$OMn1hZchIFSrWDNgBQj6MQkojvnGPR6tL7su8IkHW_Y"
},
84
]
]
}
}or similar. With a dictionary inside an array:
{
"type": "m.recent_emoji",
"content": {
"recent_emoji": [
{
"emoji": "😅",
"total": 7
},
{
"image_url": "mxc://example.org/blah",
"image_pack_room": "!mDgU8OWQ6CAgEK-Fb2Dhv0fR0nHs_tEM_Na93SSIhu0",
"image_pack_event": "$OMn1hZchIFSrWDNgBQj6MQkojvnGPR6tL7su8IkHW_Y",
"total": 84
}
]
}
}I took a look at what the byte count difference after gzip has been applied to the two above data structures. Turns out it's exactly the same!
Click to view byte sizes
➜ cat <<'EOF' | gzip -cn9 | wc -c
➜ {
"type": "m.recent_emoji",
"content": {
"recent_emoji": [
{
"emoji": "😅",
"total": 7
},
{
"image_url": "mxc://example.org/blah",
"image_pack_room": "!mDgU8OWQ6CAgEK-Fb2Dhv0fR0nHs_tEM_Na93SSIhu0",
"image_pack_event": "$OMn1hZchIFSrWDNgBQj6MQkojvnGPR6tL7su8IkHW_Y",
"total": 84
}
]
}
}
➜ EOF
257
➜ cat <<'EOF' | gzip -cn9 | wc -c
➜ {
"type": "m.recent_emoji",
"content": {
"recent_emoji": [
[ "😅", 7 ],
[
{
"image_url": "mxc://example.org/blah",
"image_pack_room": "!mDgU8OWQ6CAgEK-Fb2Dhv0fR0nHs_tEM_Na93SSIhu0",
"image_pack_event": "$OMn1hZchIFSrWDNgBQj6MQkojvnGPR6tL7su8IkHW_Y"
},
84
]
]
}
}
➜ EOF
257
Given the negligible difference in size, I'm in favour of the easier to work with dictionary-inside-an-array approach.
(None of that needs to go in the current proposal, of course. What's important is that it can be extended.)
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.
Note that custom emojis are already supported in this MSC. The solution is as trivial as MSC4027 for reactions: just use the mxc:// URI as the key instead of an unicode emoji. gomuks web already has that implemented in the io.element.recent_emoji event (and element doesn't mind the funny entries)
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.
What I had in mind, though, is not having custom emojis stored as images but as a part of an emoji font. In that case, the same code point in different fonts could look different; you also have to specify which font to use. Anyway, as already mentioned above, all this is just meaning to support turning the inner array to a dictionary.
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.
Note that custom emojis are already supported in this MSC. The solution is as trivial as MSC4027 for reactions: just use the
mxc://URI as the key instead of an unicode emoji. gomuks web already has that implemented in theio.element.recent_emojievent (and element doesn't mind the funny entries)
Good to know, and the existing support is noted.
The solution I presented above would allow you to link back to the image pack containing the custom emoji however, which could be valuable for a client to display.
| ## Potential issues | ||
|
|
||
| Clients could choose wildly different ways to generate recommendations from the shared storage | ||
| leading to significantly different UX across clients. |
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.
Nitpick: another sentence could be added here on why the author believes it's not worth it to specify client behaviour further (my take is that this is an area where UX exploration should still take place as we don't know the "best" way to sort recent emojis).
turt2live
left a comment
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 this lgtm - just a couple of suggestions for tidying it up.
I'd also recommend resolving some of the other outstanding threads.
| When an emoji is used in a message or an annotation, the sending client moves (or adds) it to the | ||
| beginning of the `recent_emoji` array and increments (or initializes) its counter. |
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 should be an implementation detail rather than a requirement of a client. Not all clients will track an emoji in a message as "used".
|
@mscbot resolve checklist incomplete |
Rendered
Implementations:
SCT Stuff:
FCP tickyboxes
MSC checklist