feat: implement reactions

[Bloeckchengrafik]

Hi y'all,

I'm currently working on implementing reactions. If I implement any UI, i'll share it up here.

Current ToDo:

• Load reactions for non-"live" messages
• Load reactions for "live" messages and reaction updates
• Show reactions in the UI
• Load users and display them on hover
• Allow adding reactions to messages

In a later PR:

• Show "super"-Reactions in the UI
• Allow adding "super"-reactions
• Add entirely new reactions using an emoji picker

Update: I have something to show you: Some part of the UI is previewable!

Default emojis only work with an emoji font installed on the system, custom emojis work everywhere!

As always, if you have any feedback about the code style, just message me. It helps both me and the project and I'm infinitely grateful for it!

Greetings,
Christian B.

[DovidP]

Serenity has its own reaction struct so it would probably be best to use that. We haven't really discussed architecture yet though.

In serenity there are ReactionAdd, ReactionRemove, ReactionRemoveAll, and ReactionRemoveEmoji events. Implement these in the EventHandler for "live" updates. (Note: make sure to handle the case where there is a reaction to a message which is not in the message cache).

[Bloeckchengrafik]

Thanks for the info about the events! Currently, I just try to copy style from whatever is currently there and it seems like serenity is currently not a dependency of the ui crate; all communication between the two seem to be going through the chat rate right now. Do you think serenity should be a dep of the ui crate? I don't really think so because it kinda goes against what's currently implemented in terms of seperation of concerns (although gpui seems to be a dep of the chat and discord crate as well but i'll ignore this)

I'm really unsure which way is the "correct" way to handle this. Any ideas?

[DovidP]

We discussed this last night. We are working on refactoring everything and everything should become clear. Serenity will not be a dependency of chat. The discord structs (DiscordMessageAuthor) for example will use serenity objects, and the methods will return chat objects.

[Bloeckchengrafik]

i think i get it by looking at a263442 assuming this is the refactor you were talking about.

I'll get back to work :)

[Bloeckchengrafik]

@DovidP After some fiddling around, I think it is better not to use the serenity model in this case:

Serenity has two reaction models: One when a reaction is loaded with a message and one when reaction details are retrieved or a reaction is modified. The first one has reduced information.

When we store reactions on a message, we'll generally want to use the reduced model since discord sends this with the message content, so we can load them without another http request. But these can't be instantiated (#[non_exhaustive]) so we'll always need to get them from serenity like this. For discord to send a model like this, we'll need to reload the message whenever something with reactions happen. Using a custom model, this is just modifying some value. Many client-side-only operations will need to send more http requests like this (example: adding a reaction normally takes one, in this case it would take two since we'll need to reload the message). One could also use a list of enum elements with the different models (serenity msg, serenity reaction, local) but in this case we'd still need a local model (optimistic updates, no message reloads for reactions) and the code would get messy very quickly. A change in serenity could only fix it if it'd be a model that unites the two reaction models, the change proposed to be in the scope-discord crate right now, so I don't think modifying serenity here is the best way of doing things.

So I think, in this case, using the "old" model works best in this particular usecase. What do you think? Anything that could fix this dilemma while keeping the code similar?

Thanks for any feedback!

[DovidP]

I think the plan is to have two message caches, one using the serenity message cache for detailed information, and then a "chat" message cache for actually displaying the messages. We should be using the serenity message struct for retrieving reactions. The only time we would need detailed reaction information is when the user wants to know who reacted. In that case I think it's fine to hit the api.

I won't be by my computer until next week, but I think for now the serenity caching implementation doesn't cache the reaction events. Submit an issue on our fork of serenity to implement caching of message reactions and I will have a look.

[roobscoob]

I actually had to deal with this same issue in my refactor, specifically in regards to message data, since the manually-constructed version of message data and the version we received from discord were so divergent, I created two different variants for message data.

I would say from the situation you described, this sounds like the perfect approach. Not only that, but it allows for further development that I've had in mind for a while (like being able to display a preview of who reacted to a message by async loading that information in and swapping states)

[bvvst]

oops didn't mean to do that

[Bloeckchengrafik]

Sorry for the delay. Unfortunately, I don’t have much time at the moment, but I haven’t forgotten about the PR. It'll just take a bit longer before I can continue working on this.

[Bloeckchengrafik]

I'm done in terms of features for this PR. The code is not optimal but I need to think about how to refactor this, but that will only maybe happen in a different PR because a whole lot of the app would need to be refactored.

Please let me know if you have any questions, either here or in discord!

[roobscoob]

A quick open question I want to pitch: This PR handles Unicode emojis by letting your OS render them. Is this the desired behavior for the app generally? or should we consider using twemoji?

I don't think there's a right answer here, but I'm personally on the side of using twemoji.

[roobscoob]

Other than that, this PR is pretty great. I'm in agreement with you about the general messy-ness of the codebase. But that's out-of-scope for this PR and I don't think I have any other issues before merge.

[Bloeckchengrafik]

Interesting idea. I think using twemoji would be better for compatibility and parity with discord, although custom (themed) emoji fonts could be a nice niche feature. Not sure tho

https://github.com/13rac1/twemoji-color-font would be an easy way to integrate it, just let the OS render it again but with twemoji

This is a very minor change and could even be implemented with a feature flag if someone wants to use os rendering directly

[Bloeckchengrafik]

I tried doing what I proposed in my previous message. Tldr: Didn't work. Gpui doesn't seem to like that kind of font or something. Using twemoji directly should work but I haven't tried that yet. I'll get to it today or later this week

[Bloeckchengrafik]

Done! Twemoji support can be configured via the twemoji feature. It is enabled per default