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

feat: add discord > matrix reaction support #862

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

SethFalco
Copy link
Contributor

@SethFalco SethFalco commented Oct 9, 2022

Related

Adds reaction support in one direction (Discord → Matrix).

Supports:

  • Adding reactions
  • Removing reactions
  • Removing all reactions

There is an "edge-case" with some emotes.

Here is a thumbs-up emoji on Discord: 👍 (Base64: 8J+RjQo=)
Here is a thumbs-up emoji on Element: 👍️ (Base64: 8J+Rje+4jwo=)

If someone reacts with a thumbs-up from Discord, then someone clicks that reaction in Element (and presumably any Matrix client) it's handled perfectly fine.

However, if a user separately goes through the Element interface to do a thumbs-up emoji, because it's technically a different character, you'll see 2 separate reactions for a thumbs-up.

image

When a Discord user does a thumbs-up, and an Element users manually performs a thumbs-up without clicking the pre-existing reaction.

Notes

There are a few ways we can approach bridging reactions with custom emojis.

name But on Discord custom emotes can have the same name, so if the same user does multiple reactions with emojis with the same name, it'll only come through as one. If they do 2 reactions, then remove one, Discord will show the other, while Matrix will show neither.
name:id But on Discord, people can rename emotes. So if they react, then rename the emote, then remove the reaction, it will count as a different emote and won't be removed on the Matrix side.
id But on Matrix, this is basically a meaningless number, at that point we might as well not bridge custom emojis at all.

It looks like in future it should be supported to use external images as reactions via MXC URLs. I don't do this yet because the shortcode is not exposed in the SDK and it's not clearly documented behavior, but in future can update the bridge to support that.

Based on what I've seen, others use the format :name: for the shortcode property. Based on this, I've decided to use the same format already. That way when we're in a position to support MXC URLs and the shortcode property, we can just move the value of key to shortcode for those cases.

Screenshots

image

A message and 3 reactions all sent from Discord.

image

How the same message/reactions appear on Matrix when bridged.

Notes

  • getRelationsForEvent was moved out of unstableApis in the main branch for matrix-bot-sdk. Eventually, when we update to a version of matrix-appservice-bridge that uses that version we should switch it out stop using it from unstableApis.

@tezlm
Copy link

tezlm commented Oct 9, 2022

discord custom emojis can be done by setting a m.relates_to key to an mxc:// url and content-level shortcode to the shortcode

{
    "shortcode": "something",
    "m.relates_to": {
        "event_id": "$abc123",
        "key": "mxc://servername/someidhere",
        "rel_type": "m.annotation"
    }
}

@SethFalco
Copy link
Contributor Author

SethFalco commented Oct 10, 2022

That's interesting. The MXC URL can be done already, but the SDK doesn't expose the shortcode property.

I'll leave this as-is for now, but I'll check what you said and verify if that's actually how it works. If so, I can open a PR to matrix-bot-sdk to include this, then come back to fix it here in a 2nd iteration if that's cool with maintainers.

Any chance you can link to where doing images as reactions is documented/defined?

@dali99
Copy link

dali99 commented Oct 11, 2022

I believe the closest thing to documentation on that is the MSC: matrix-org/matrix-spec-proposals#3746

A bigger argument is probably that mx-puppet-discord, cinny, fluffychat, beeper, some element patches floating about, and probably more clients, all special case seeing an mxc url and render it as an image

src/bot.ts Outdated Show resolved Hide resolved
src/bot.ts Outdated Show resolved Hide resolved
src/bot.ts Outdated Show resolved Hide resolved
src/bot.ts Show resolved Hide resolved
src/bot.ts Outdated Show resolved Hide resolved
src/bot.ts Outdated Show resolved Hide resolved
src/bot.ts Show resolved Hide resolved
src/bot.ts Show resolved Hide resolved
src/bot.ts Show resolved Hide resolved
src/db/dbdataevent.ts Outdated Show resolved Hide resolved
@mangofeet
Copy link

I was testing this and it all seems to be working fine, but I'm getting this error

Exception thrown while handling "messageReactionRemove" event MatrixError: M_FORBIDDEN: User @_discord_<discord id>:example.com not in room !<room id>:example.com, and room previews are disabled

The room in question is a private room that has the bridge bot in it with admin access. Despite this error, everything appears to be functioning correctly.

@SethFalco
Copy link
Contributor Author

Hey @mangofeet, thanks for testing this!

Any chance you could provide more in-depth steps to reproduce or a stack trace?
If not, I'll get my environment up when I have time and try to check this out.

@salixor
Copy link

salixor commented May 24, 2023

Hi there ! I'm currently using this change on two servers with a few people. Thanks a lot for it !

I have two minor issues with it.

The first one, as @mangofeet reported, is that we are getting the same errors :

  • Exception thrown while handling "messageReactionAdd" event MatrixError: M_FORBIDDEN: User @_discord_<id>:<homeserver> not in room <room>
  • Exception thrown while handling "messageReactionRemove" event MatrixError: M_FORBIDDEN: User @_discord_<id>:<homeserver> not in room <room>, and room previews are disabled

A way to replicate the issue :

  • bridge a channel
  • have a Discord user which never posted in this channel since the briding react on a message
  • have the same user remove their reaction (still without sending any message)
  • the reaction is not bridged and the error is found in logs

Shouldn't the ghost be invited before trying to create a reaction on Matrix's side ?


The second issue has to do with Discord bot users. It seems their reactions are not bridged at all. And any subsequent Discord user which tries to react will simply be ignored. This specific emoji can never be seen on Matrix side. Having a Discord user use another emoji to react does work.

src/bot.ts Outdated Show resolved Hide resolved
@SethFalco
Copy link
Contributor Author

SethFalco commented Jul 6, 2023

@salixor @mangofeet I've rebased with develop, and resolved the exceptions when users aren't in the room when they add/remove reactions now.
I'd appreciate it if you could test it, just to confirm everything is good. 👍

I also found another bug. Removing all reactions in the Discord interface should bridge to Matrix correctly now.

@SethFalco SethFalco force-pushed the reactions branch 3 times, most recently from ba3d10b to 2ac843f Compare July 6, 2023 00:47
@salixor
Copy link

salixor commented Jul 14, 2023

Hey there, haven't had much time last week to try the changes out, but I haven't forgotten !

I'm glad to say the first issue is solved !

The second issue is still a weird one.
The reactions have been bridged on one message, but not the other - with no apparent error.
On the first message, adding the same reaction myself removes the reaction of the Discord bot user on Matrix's side, as shown below :
image
image
It's a very niche use case however, so I'd say it works well enough. At least actual user reactions are always bridged, which is the most important imo.

For the multiple room issue, we only stumbled on this use case because of an old room which bridge was kinda jumbled, and I don't really know how to replicate the same use case - sorry ! Also a niche use case, and the changes seem reasonable.

Really hope it gets merged someday, and thanks a lot for the work !
Gonna run my bridge with these new changes, and I'll report if something weird happens.

PS : I tried the remove all feature but it doesn't seem to interface properly. I get no errors but the reaction stay on Matrix's side

@GitMensch
Copy link

Is there anything open but the rebase (same question goes for the related #877)?
Who would handle the merge?

return false;
}

return event.content["m.relates_to"].key === reaction.emoji.name;

Choose a reason for hiding this comment

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

you should use reactionName here in the same way as in OnMessageReactionAdd

discordBot = getDiscordBot();
const intent = mockBridge.getIntent(author.id);

intent.underlyingClient.unstableApis.getRelationsForEvent = async () => {

Choose a reason for hiding this comment

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

this is not unstable anymore, using it via .unstableApis is deprecated

Suggested change
intent.underlyingClient.unstableApis.getRelationsForEvent = async () => {
intent.underlyingClient.getRelationsForEvent = async () => {


const underlyingClient = intent.underlyingClient;

const { chunk } = await underlyingClient.unstableApis.getRelationsForEvent(

Choose a reason for hiding this comment

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

Suggested change
const { chunk } = await underlyingClient.unstableApis.getRelationsForEvent(
const { chunk } = await underlyingClient.getRelationsForEvent(

const [ eventId, roomId ] = storeEvent.MatrixId.split(";");
const underlyingClient = this.bridge.botIntent.underlyingClient;

const { chunk } = await underlyingClient.unstableApis.getRelationsForEvent(

Choose a reason for hiding this comment

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

Suggested change
const { chunk } = await underlyingClient.unstableApis.getRelationsForEvent(
const { chunk } = await underlyingClient.getRelationsForEvent(

Comment on lines +292 to +294
public async getRelationsForEvent(roomId: string, eventId: string, relationType?: string, eventType?: string): Promise<any> {
this.funcCalled("getRelationsForEvent", roomId, eventId, relationType, eventType);
}

Choose a reason for hiding this comment

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

move to MatrixClientMock

@SethFalco
Copy link
Contributor Author

SethFalco commented Jun 11, 2024

I've actually deleted my Discord account since creating this PR.
Would anyone care to take it off my hands to continue maintaining it?

Edit: I won't close it unless someone wants to take over. In future, I may create a dummy account just to update the PRs if no one else wants to maintain it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Send m.annotation events for message reactions
8 participants