Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Render image data in reactions #7903

Conversation

AndrewRyanChama
Copy link

@AndrewRyanChama AndrewRyanChama commented Feb 25, 2022

Many messaging services such as Slack and Discord allow for custom
images for reacts, rather than only standard emoji. Currently Element
will only show the plain text of the reaction key.

This PR updates element to render the media content from reaction
events. This does not add a way for users to add custom reactions yet.

This can be useful in the case of bots and bridges, which will be able
to immediately use this functionality. A picker for Element users will
be added in the future.

This is a solution to element-hq/element-meta#339

Similarly to stickers, the event type should simply be sent with an image content, and it will be displayed appropriately. For example

 "content": {
    "body": "parrot.gif",
    "info": {
      "size": 15010,
      "mimetype": "image/gif",
      "w": 128,
      "h": 128,
      "xyz.amorgan.blurhash": "UKFg2,Ef0f-V0es:}tR*wf$*xZNH$*NaI:bH"
    },
    "url": "mxc://localhost:8008/zyCSlBnBTQCxhTFDEDNpNobe",
    "m.relates_to": {
      "rel_type": "m.annotation",
      "event_id": "$FqS1dRW7gMe_JhXNSBUy2ApAQNMQ4IL-vywYNqbXRlA",
      "key": ":partyparrot:"
    }
  },

Signed-off-by: Andrew Ryan andrewryanchama@clover.club

Original behavior / fallback:
Screen Shot 2022-02-25 at 12 03 45 PM
Updated to display image:
Screen Shot 2022-02-25 at 11 54 12 AM


Here's what your changelog entry will look like:

✨ Features

@AndrewRyanChama AndrewRyanChama requested a review from a team as a code owner February 25, 2022 20:06
Many messaging services such as Slack and Discord allow for custom
images for reacts, rather than only standard emoji. Currently Element
will only show the plain text of the reaction key.

This PR updates element to render the media content from reaction
events. This does not add a way for users to add custom reactions yet.

This can be useful in the case of bots and bridges, which will be able
to immediately use this functinality.  A picker for Element users will
be added in the future.
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for taking a look at this!

Code-wise this is largely okay, though I do somewhat question the approach. Custom emoji also extends into non-reaction messages which can affect how the technical side gets created.

Before it goes for design/product review, I think it'd be best to involve the Matrix Spec process to align a technical solution. There's already some proposals out there for this sort of thing, so it may just be a case of implementing one of them.

The larger context with custom emoji (and stickerpacks) in Matrix is that it's currently in desperate need of review and technical design. #matrix-spec:matrix.org on Matrix can provide more detail about both custom emoji and the proposal process.

@@ -102,7 +102,7 @@ function mightContainEmoji(str: string): boolean {
*/
export function unicodeToShortcode(char: string): string {
const shortcodes = getEmojiFromUnicode(char)?.shortcodes;
return shortcodes?.length ? `:${shortcodes[0]}:` : '';
return shortcodes?.length ? `:${shortcodes[0]}:` : char;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unintentional change?

@@ -0,0 +1,64 @@
/*
Copyright 2018 New Vector Ltd
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be your copyright.

@AndrewRyanChama
Copy link
Author

This change doesn't conflict and is compatible with either matrix-org/matrix-spec-proposals#1951 or matrix-org/matrix-spec-proposals#2545, as both those deal mainly with distributing packs of images and don't cover reactions at all.

I can still put together an msc for this since it might help to make this explicit

@kittykat kittykat requested a review from turt2live May 13, 2022 15:39
@MadLittleMods MadLittleMods added Z-Community-PR Issue is solved by a community member's PR T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements labels Jun 1, 2022
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review was re-requested, however I feel this is blocked on product, spec, and design (in roughly that order, but spec is the more important problem at the moment).

@turt2live turt2live added the X-Blocked The PR cannot move forward in any capacity until an action is made label Jun 2, 2022
@williamkray
Copy link

would it be possible to put this and #8087 behind a labs flag or something so that the people who are itching for this can enable, with the understanding that the work is still being finalized from the spec and product side?

i would love to be able to share with my users that the feature they've been asking for for the last 2+ years is at least making progress, even if it's preliminary. using labs flags seems like a pretty common use case for early implementation of non-finalized spec feature testing in element.

@t3chguy t3chguy mentioned this pull request Jul 6, 2023
3 tasks
@t3chguy t3chguy mentioned this pull request Jul 26, 2023
3 tasks
@Johennes
Copy link
Contributor

Johennes commented Sep 1, 2023

Thank you for your contribution. @daniellekirkwood and I synced up today and have decided to proceed with #11087. I'm closing this pull request because it largely achieves the same thing but has the downside of using the short code as key which has already been criticized in https://github.com/matrix-org/matrix-spec-proposals/pull/3746/files#r866285147.

@Johennes Johennes closed this Sep 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements X-Blocked The PR cannot move forward in any capacity until an action is made Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants