-
Notifications
You must be signed in to change notification settings - Fork 379
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
MSC4027: Propose method of specifying custom images in reactions #4027
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Sumner Evans <sumner@beeper.com>
fb1c880
to
d4ff1c7
Compare
Signed-off-by: Sumner Evans <sumner@beeper.com>
I don't think this deserves the |
The Discord bridge already uses com.beeper.reaction.shortcode unstable field name. Signed-off-by: Sumner Evans <sumner@beeper.com>
Some extra details concerning SchildiChat:
|
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 LGTM at a high level.
Once we have a mechanism for image packs, it may be nice to link back to that pack from the reaction. But that change should live in the image packs MSC, instead of this one.
"event_id": "$abcdefg", | ||
"key": "mxc://matrix.org/VOczFYqjdGaUKNwkKsTjDwUa" | ||
}, | ||
"shortcode": ":partyparrot:" |
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 enforce that shortcode's must begin and end with :
? I think it'd be confusing to users if some clients started picking some other character.
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.
Added a "should" for that. It's not enforceable other than by convention.
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.
A "must" snuck back in on line 33.
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 type of the field is fairly easily enforceable by servers. We are already saying that they should verify the length. (Obviously if a rouge server doesn't validate these fields, we could have "bad" events come across federation.)
I think the question here is what part of the verification is "should" vs "must"? Right now, the servers "must" verify type and length of the shortcode, but the leading and trailing :
is "should".
Maybe everything should be "must" and force servers to verify all three things, but leave it up to the server implementation if they want to locally redact invalid events that come across federation?
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 it would eliminate confusion in implementations for all three to be a MUST on the Client-Server API.
leave it up to the server implementation if they want to locally redact invalid events that come across federation?
I would say yes, though I imagine most implementations would not redact such events - instead just allowing any UI bugs in the client that would surface.
I played around with matrix-org/matrix-react-sdk#11087 and it looks good! Removing the |
Signed-off-by: Sumner Evans <sumner@beeper.com>
Signed-off-by: Sumner Evans <sumner@beeper.com>
Signed-off-by: Sumner Evans <sumner@beeper.com>
Signed-off-by: Sumner Evans <sumner@beeper.com>
This proposal is implemented in Out Of Your Element: https://gitdab.com/cadence/out-of-your-element/compare/c7ddf638dbd658057ec6a7a018a23b0693a97890~..044ccc08e06f11b48e16c7f7637d3c088008119a#diff-0f01ec9090ff5700793cc2f6399fb95cf3a7115f It follows the spec and it works with Schildi and Cinny. |
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.
Barring the below concern, I think this MSC is ready for review from the rest of the Spec Core Team.
@mscbot fcp merge
|
||
2. When the annotation's key is an MXC URI, a new (optional) `shortcode` key can | ||
be added to the content of the event with a textual name for the image. This | ||
field must be a string and should start and end with the `:` (colon) |
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.
@mscbot concern Requiring two :
characters in every shortcode string.
Could you explain why we should encourage clients to put :
s in every shortcode instead of restricting :
from being put in the string?
I know we can't enforce it over federation, but the spec could at least enforce it in the Client-Server API. I think the occasional extra :
appearing in clients is worth the bandwidth saving.
For bridges, if they connect to a network that includes :
on the ends of their custom emote shortcodes (or another character), then the bridge can just strip it, no?
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
1 similar comment
This comment was marked as spam.
This comment was marked as spam.
This FCP proposal has been cancelled by #4027 (comment). 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. |
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.
@mscbot concern Needs to be reviewed against other custom emoji/sticker MSCs
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 in particular, this MSC sets up grammar for emoji shortcodes in Matrix, which we'll want to double-check aligns with whatever MSC defines sharable image packs.
@turt2live did you have anything else in mind? The rest is fairly reaction-specific IMO.
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.
Shortcodes and feature as a whole are conflicting, imo. Specific impact to be determined once the remainder of the stack is looked at :)
(I plan to do this over the next 2ish weeks, barring other commitments - will hand over ~next week if things go poorly on my side)
As this is tied to the media linking series and unable to progress otherwise, I'm cancelling FCP for the time being. This does not mean the MSC is unimportant, but rather the focus is elsewhere at the moment. @mscbot fcp cancel |
A proposal to render MXC URIs in reaction keys as images.
Rendered
Implementations
Clients
Bridges
Supercedes #3746
FCP tickyboxes