Skip to content
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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

sumnerevans
Copy link
Contributor

@sumnerevans sumnerevans commented Jun 10, 2023

@sumnerevans sumnerevans changed the title MXCXXXX: Propose method of specifying custom images in reactions MXC4027: Propose method of specifying custom images in reactions Jun 10, 2023
Signed-off-by: Sumner Evans <sumner@beeper.com>
@turt2live turt2live added proposal A matrix spec change proposal client-server Client-Server API kind:feature MSC for not-core and not-maintenance stuff needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. labels Jun 10, 2023
@turt2live turt2live changed the title MXC4027: Propose method of specifying custom images in reactions MSC4027: Propose method of specifying custom images in reactions Jun 10, 2023
Signed-off-by: Sumner Evans <sumner@beeper.com>
@sumnerevans
Copy link
Contributor Author

I don't think this deserves the needs-implementation label as it is already implemented across multiple clients and at least one bridge.

The Discord bridge already uses com.beeper.reaction.shortcode unstable
field name.

Signed-off-by: Sumner Evans <sumner@beeper.com>
@SpiritCroc
Copy link

Some extra details concerning SchildiChat:

Copy link
Member

@anoadragon453 anoadragon453 left a 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.

proposals/4027-custom-images-in-reactions.md Outdated Show resolved Hide resolved
"event_id": "$abcdefg",
"key": "mxc://matrix.org/VOczFYqjdGaUKNwkKsTjDwUa"
},
"shortcode": ":partyparrot:"
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

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.

@anoadragon453
Copy link
Member

anoadragon453 commented Jun 16, 2023

I played around with matrix-org/matrix-react-sdk#11087 and it looks good! Removing the needs-implementation label.

@anoadragon453 anoadragon453 removed the needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. label Jun 16, 2023
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>
@cloudrac3r
Copy link

cloudrac3r commented Sep 22, 2023

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.

@kimabjorkede
Copy link

It seems like Element now has this setting in Labs, and it works very well.
image

Copy link
Member

@anoadragon453 anoadragon453 left a 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)
Copy link
Member

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.

This comment was marked as off-topic.

@mscbot

This comment was marked as off-topic.

1 similar comment
@mscbot

This comment was marked as spam.

@mscbot
Copy link
Collaborator

mscbot commented Sep 25, 2023

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:

  • Requiring two : characters in every shortcode string.
  • Needs to be reviewed against other custom emoji/sticker MSCs

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.

@mscbot mscbot added proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. disposition-merge unresolved-concerns This proposal has at least one outstanding concern labels Sep 25, 2023
Copy link
Member

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

Copy link
Member

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.

Copy link
Member

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)

@turt2live
Copy link
Member

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

@mscbot mscbot added proposal-in-review and removed proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. disposition-merge labels Apr 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client-server Client-Server API kind:feature MSC for not-core and not-maintenance stuff proposal A matrix spec change proposal proposal-in-review unresolved-concerns This proposal has at least one outstanding concern
Projects
Status: Scheduled - v1.10
Development

Successfully merging this pull request may close these issues.