Add Support for Emoji Reactions (Replaces #1980 and #2127)

[kescherCode]

Yet another attempt.

I've gone ahead and rewrote the history a little bit to remove some of the erroneous contents some commits had when they were cherry-picked from Catstodon.

Thanks @fef1312 and @neatchee for maintaining the previous iterations of this PR.

[Plastikmensch]

This doesn't make any sense to me.
I see that this comes from announcement reactions, but there it checks for if name.present? instead.
Right now it just tries to find a custom emoji with no shortcode, when there is no name given.
Though I still have no idea what this is for.

[Plastikmensch]

Suggested change

	def set_custom_emoji
	self.custom_emoji = CustomEmoji.find_by(shortcode: name, domain: account.domain) if name.blank?
	end
	def set_custom_emoji
	self.custom_emoji = CustomEmoji.find_by(disabled: false, shortcode: name, domain: account.domain) if name.present?
	end

Oh I see what this is for.
This is supposed to prevent reacting with a disabled emoji and also sets custom_emoji when a custom emoji shortcode is given, but no custom_emoji object.

[kescherCode]

Hmmm... Once this change is applied, you can no longer react with foreign custom emoji when such a reaction already existed on the status (made by a foreign instance's user). I will revert the change for now.

[Plastikmensch]

Ah, yeah, because it looks for an emoji with the reactor's domain :/
It's something I unfortunately can't test for, but is the name passed as [email protected] to this? In that case, it also should split between name and domain.
Alternatively, account.domain could be replaced with custom_emoji.domain, though it should have a default of nil, so it doesn't throw a NoMethodError.

[kescherCode]

True. In fact, I've just observed incorrect behaviour happening due to account.domain being used...

[kescherCode]

...but when removing the reaction on Mastodon, it will disappear on Akkoma as well, indicating that at least, the emoji is federated incorrectly consistently.

[Plastikmensch]

Honestly that confuses the hell outta me, because an emoji, which doesn't exist, should not the attached to the payload, unless I'm missing or misunderstand something.

[Plastikmensch]

Oh no... This seems to be a bigger issue with the way custom emojis work.
It is always assumed the domain of a custom emoji is the domain of the account the custom emoji was first seen in...
So that leaves 3 options:
1. Prevent being able to react with foreign emojis
2. Change how custom emojis are serialised and add domain to the payload.
3. Leave it as it is

I'm for option 1.
Option 2 would require all other ActivityPub software to also change their serialisation to look for domain in the tag, otherwise they would have the same problem.
Same would go for changing content to be [email protected], which no other software would currently process.

[Plastikmensch]

Oddly, even with this change, whenever I react with my "annoying_dog" emoji, and someone else on an Akkoma instance then reacts on the same reaction, a faux "[email protected]" reaction is created (that will also never be able to go away).

Which means, this is not a bug with reactions, but intended behaviour by vanilla.
Not being able to unreact is a separate issue, which affects all custom emoji reactions.

Wait, how does the Akkoma instance react with the same emoji? It should not even display on Akkoma as reactions on toots don't and shouldn't federate.
So an [email protected] is actually expected behaviour here.

[Plastikmensch]

Suggested change

	def set_custom_emoji
	self.custom_emoji = CustomEmoji.find_by(shortcode: name, domain: account.domain) if name.blank?
	end
	def set_custom_emoji
	self.custom_emoji = CustomEmoji.find_by(disabled: false, shortcode: name, domain: custom_emoji.domain) if name.present? && custom_emoji.present?
	end

I actually don't think it should set a custom emoji when it is nil, as I see it as violation of the spec. Also it leads to unintended behaviour. It doesn't really matter for announcement reactions which this is based on, since they don't handle remote content, but it does matter here.

Though, as established custom_emoji.domain will essentially always equal account.domain anyway.
Actually no, when using the rest API, it is actually possible that custom_emoji.domain is different from account.domain

[kescherCode]

Is it sufficient for reacted? to be called with merely a name? After all, the name does not include the domain when it's a custom emoji, as it stands currently.

[Plastikmensch]

I think it is sufficient as name is saved as [email protected], though the content payload only contains the shortcode, so it should also check @json['tag'] for an emoji and pass [email protected] to reacted? if the tag payload also includes a custom emoji

[kescherCode]

The issue is, reacted? in activity.rb does check for name as it's saved in the database... and the database never saves the domain part anywhere.

[Plastikmensch]

It should

[kescherCode]

But it doesn't. My database does not contain any "emoji@domain" under the name column, only ever the shortcode.

[Plastikmensch]

Ooooh, you're right, sorry, I misunderstood the code.
Yeah, reacted? needs an optional emoji attribute.

[Plastikmensch]

Wait, no, name is case sensitive and since even foreign custom emoji are always treated as coming from the reactors domain, the same account can't have two different reaction with the same name attached to a toot.

Yeah, reacted? needs an optional emoji attribute.

That would break the Undo activity though, as it doesn't contain tags.
I don't know how other software handles Undo though.
I missed how Undo works. It does contain the same payload as the activity it is undoing, so it also needs to process tags.

btw this also looks for :shortcode: in the database.

[kescherCode]

...but when removing the reaction on Mastodon, it will disappear on Akkoma as well, indicating that at least, the emoji is federated incorrectly consistently.

[Plastikmensch]

reaction is nil when name is a custom emoji shortcode, because the query looks for :shortcode: in the database, so reactions with custom emoji are never deleted.

[Plastikmensch]

Suggested change

	reaction = status.status_reactions.where(account: @account, name: name).first
	reaction = status.status_reactions.where(account: @account, name: name.delete(':')).first

facepalm
I typo'd this like 3 times now...

Though for performance, it should be name = name.delete(':') before this block. (or name.delete! ':')

[Plastikmensch]

There is no reason to do this.
Both Misskeys Like activity as well as the EmojiReact activity only expect a single tag object.

[github-actions]

This pull request has merge conflicts that must be resolved before it can be merged.

[Plastikmensch]

I made a PR with some fixes for the backend: CatCatNya#4

[github-actions]

This pull request has resolved merge conflicts and is ready for review.

[kescherCode]

@Plastikmensch the following issue is outstanding from your PR:

the signature of the process_emoji_tags method in activity.rb has changed. But in both emoji_react.rb and like.rb, their usage hasn't, meaning there is now a mismatching amount of arguments.

[Plastikmensch]

@Plastikmensch the following issue is outstanding from your PR:

the signature of the process_emoji_tags method in activity.rb has changed. But in both emoji_react.rb and like.rb, there usage hasn't, meaning there is now a mismatching amount of arguments.

Oops, looks like I missed that while porting changes. Sorry!
Will create a follow-up shortly.

[Plastikmensch]

Follow-up available at CatCatNya#5

[kescherCode]

Wondering if the Ruby linting failure at undo.rb should be ignored by us, or if we can even do much about it given that's where the additional functionality is needed.

[Plastikmensch]

Wondering if the Ruby linting failure at undo.rb should be ignored by us, or if we can even do much about it given that's where the additional functionality is needed.

I think that cop is counterproductive and should be disabled.

[kescherCode]

Marked as draft again as I forgot that reactions coming from e.g. Akkoma still do their weird thing

[Plastikmensch]

Marked as draft again as I forgot that reactions coming from e.g. Akkoma still do their weird thing

Can you give more info on that, including what is send in the activities?
I unfortunately don't have an Akkoma instance to test with.

[github-actions]

This pull request has merge conflicts that must be resolved before it can be merged.