-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
feature: fixie wixied the emoji reactions to be sooper dooper!!1 ✨🎉 #7441
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
base: main
Are you sure you want to change the base?
Conversation
Only replace reactions with the same emoji from the same user
Just show all reactions with emojis - no grouping by user so not to interfere with the multiples
added code to check if you already reacted in order to allow the toggling behaviour
Keep emoji for both add and remove to enable the multiple emoji reactions and not end up with undefined
add messages for config option for multiple emoji reactions, only EN locale, not sure how i should go about for the rest of them
added preferences option for Multiple Emoji Reactions
have default behaviour by default and if the preferences option is enabled then do the multiple emoji reactions
updated code to have both old and new behaviour depending on the preferences option
add preference option for multipleEmojiReactions
added the multipleEmojiReactions boolean for the preferences of having Multiple Emoji Reactions enabled or not
related to #4617 |
absolute cinema of a PR please merge this |
question from a non-dev: what does this look like for clients that do not yet have this setting (for example Android)? does it just keep the old behaviour where a new emoji overrides the old one? |
@nehemiagurl ohayo nehemiagurl-chan~! (◕ᴗ◕✿) suuuper good question desu!! let me expwain~ OwO ✧・゚: ✧・゚: ✧・゚: ✧・゚: ✧・゚: ✧・゚: for cwients without the setting (wike android-kun): 。・::・゚★,。・::・゚☆ android sends reactions 。・::・゚★,。・::・゚☆ android: 💖 → 😭 (wepwaces on their side) (ノ◕ヮ◕)ノ*:・゚✧ no breaking changes! ✧・゚: ✧・゚:(ノ◕ヮ◕)ノ evewything is backwards compatibwe!! (´。• ᵕ •。`) ♡
・゚゚・。 everyone is happy! 。・゚゚・ (づ。◕‿‿◕。)づ so basicawwy, mobile cwients awen't affected at aww nehemiagurl-chan!! they'ww get the feature when they'we weady~ uwu nyaa~ ♡(˃͈ દ ˂͈ ༶ ) |
nuzzles PR (´ ω `♡) |
i see one slight issue, which is that a change of reaction on Android gets interpreted as an additonal reaction on Desktop. this will also happen with older desktop clients and people with the toggle still off (basically anyone who hadn't moved forward to enable the feature). |
i didnt understand anything |
Instead of handling single-reaction behavior during receipt (which causes inconsistencies), handle it during sending by clearing existing reactions before adding new ones when multipleEmojiReactions is disabled. Changes: - Reverted "replace on receive" logic in reaction handlers - Added "clear before send" logic in enqueueReactionForSend - When single reaction mode is active, removes all existing reactions from the user before adding the new one - Ensures consistent behavior regardless of other users' settings This approach is protocol-compatible and avoids the ambiguity of trying to guess user intent during reaction removal.
@nehemiagurl omg thanksies for bringing that up!! ✧・゚: ✧・゚:(ノ◕ヮ◕)ノ sync issue is like super important >w< i'm testing a new approach that should handle it better across clients~ (=^・ω・^=) nyaa~ debugging mode activated! ur totally right about the replace vs add thing creating chaos between android and desktop owo appreciate u catching that edge case!! 💕✨ |
@mikku69420 glad i could help, i want to see this feature go through so catching all the issued that are likely to be raised by the Signal team in advance is the best way to go about it. |
- Removes existing reactions before adding a new one from the same user - Handles toggle behavior (clicking same emoji twice removes it) - Prevents duplicate reactions from accumulating Changes: - enqueueReactionForSend.ts: Add logic to remove existing reactions in single reaction mode - Reactions.ts: Fix reaction handling for both add and remove operations - util.ts: Check multipleEmojiReactions setting in markOutgoingReactionSent - message.ts: Simplify reaction display logic (duplicate prevention at storage level) - sendReaction.ts: Clean up reaction state management This ensures reactions work as expected when multiple reactions are disabled, maintaining backwards compatibility while fixing the accumulation bug that I experienced in the new approach. Testing Steps used: 1. With multipleEmojiReactions enabled : - user A reacts with 👍 → adds 👍 - user A reacts with ❤️ → adds ❤️ (now you have both 👍 and ❤️) - user A reacts with 😂 → adds 😂 (now you have 👍, ❤️, and 😂) - user A clicks 👍 again → removes only 👍 (you still have ❤️ and 😂) 2. With multipleEmojiReactions disabled (legacy behavior): - user B reacts with 👍 → adds 👍 - user B reacts ❤️ → replaces 👍 with ❤️ (only ❤️ shown) - user B reacts 😂 → replaces ❤️ with 😂 (only 😂 shown) - user B clicks 😂 again → removes 😂 (no reactions) What Each User Sees: You (with patch) send multiple reactions: - You see: All your reactions (👍, ❤️, 😂) - Legacy users see: Only your latest reaction (😂) Legacy user sends reactions to you: - They see: Only their latest reaction (normal behavior) - You see: Only their latest reaction (they can't send multiple) Another patched user sends multiple reactions: - They see: All their reactions - You see: All their reactions - Legacy users see: Only the sender's latest reaction
…eriod When receiver has multipleEmojiReactions enabled, detect if sender might be in single reaction mode by checking if they're replacing their existing reactions. This prevents the emoji accumulation issue during transition period where some users have the feature enabled and other don't. Changes: - In multiple reaction mode, check if sender is replacing reactions - If sender has existing reactions and sends a different emoji, replace their previous reactions instead of accumulating
UwU Multi-Emoji Weaction Twansition Pwan OwO~
Phase 1a: Compatibility Layer for Signal-Desktop (this PR) uwu~
What's depwoyed: OwO
Behaviow: UwU
Phase 1b: Compatibility Layer for Signal-Android (another PR) UwU~
What's depwoyed: OwO
Behaviow:
Phase 1c: Compatibility Layer for Signal-iOS (another PR) OwO~
What's depwoyed: uwu
Behaviow:
Phase 2: Enable Multiple Reactions (another PR later) UwU!!!~
When: Aftew aww pwatfowms (Desktop, Andwoid, iOS) have updated OwO~ What changes: uwu~
Behaviow: UwU~
Nyotes~ OwOThe smawt detection wogic is pawtastic fow Phase 1! It ensuwes that even if some usews enabwe muwtipwe weactions eawwy, they won't see confusing accumuwation fwom usews stiww in singwe weaction mode~ (=`ω´=) ✨
This appwoach is supew kawaii and pwevents the emoji accumuwation issue duwing the twansition pewiod~ UwU! 💖
Nyext pawsteps~ OwO
|
If this doesn't get pushed, I don't know what's going to happen and quite frankly I don't want to find out. I'm obsessed with this PR |
I have no choice but to stan. Some initial thoughts... I appreciate the twansition pwan you've outlined. We do need to implement "receive" support on all clients for at least 90 days before we can start sending multiple reactions. (And this would have to be gated by a feature flag rather than a setting in the UI) These timers are necessary in order to avoid two people seeing different results on different devices depending on which version they are on. But I think we also need to consider the case that we have today where you "replace" an emoji. Today if you select a reaction, and then choose another reaction, we replace it simply by sending a new reaction without sending a "delete" message for the old one. So if we rolled out support for receiving multiple reactions, since it never received a delete message, you would end up with two reactions instead of replacing them. So we actually need two 90-day timers unless we can figure something else out:
Alternatively, maybe there's an additional field that we could add when sending multiple reactions that would let us distinguish the current "replace" behavior from sending multiple reactions: message Reaction {
optional string emoji = 1;
optional bool remove = 2;
// ...
optional uint32 counter = 7;
}
// Reaction { emoji: ":cat:", counter: 0 | null } -- first reaction
// Reaction { emoji: ":dog:", counter: 0 | null } -- replacing first reaction
// Reaction { emoji: ":cow:", counter: 1 } -- second reaction
// Reaction { emoji: ":pig:", counter: 2 } -- third reaction Also, I don't think we'd want this to be a user-configurable setting. I think if we support multiple reactions, we'll want all clients to support them all of the time. I'll have to discuss this with some other teams though |
why is there a 90 day period needed between 1 and 2? if you can gate the behaviour behind a feature flag, is it not possible to make it such that within the 90 day period the capability for multiple reactions is there but inactive and in the meantime the client interprets it as a replace (as was @mikku69420 's suggestion for a case with a config and that config being disabled)? or am i totally misunderstanding what feature flags can do and this is beyond their scope? |
absolute cinema |
First time contributor checklist:
Contributor checklist:
main
branchpnpm run ready
run passes successfully (more about tests here)Description
Hii~~ ╰(´︶`)╯♡
I fixie wixied the emoji reaction fucksy wucksie!!! xpp
NEW!! Config option!! 🎮✨
Added a toggie woggie in Settings > Chats!!
What I changey wangey'd: ✨
1. Settings UI - Added the magic switch!! 🔧
Now there's a pretty checkbox that lets users CHOOSE their destiny!! Default OFF means no breaking existing behavior!! Safety first!! ٩(◕‿◕)۶
2.
util.ts
- Made reactions play nice together!!The meanie code was like "ONE EMOJI PER PERSON ONLY!!" but I made it check BOTH the person AND the emoji so now diffewent emojis can be fwends!! UwU
BUT WAIT!! Now it checks the setting first!! If the toggle is OFF, it stays a meanie!! If ON, it becomes a softie!! (´。• ᵕ •。`)
3.
message.ts
- Stopped the Map bully!!The sewector was using a Map that was being a dictator keeping only the NEWEST reaction per person!! I made it CHECK THE SETTING!! If multipleEmojiReactions is ON, I YEET that Map into the void!! If OFF, the Map stays in charge like before!! Democracy wins!! 🗳️✨
4.
TimelineMessage.tsx
- Added toggie woggie magic!! ⚡Added code to check if u already weacted wiff that emoji using
reactions.some()
- if yes then REMOVE, if no then ADD!! It's like a light switch but for emojis!! xD5.
Reactions.ts
- Fixed the undefined ouchie!! 💔→❤️The remove reactions had
emoji: reaction.remove ? undefined : reaction.emoji
which made the emoji DISAPPEAR when removing!! That's illegal!! Fixed to always keep the emoji so the filter can find it!!starts twerking
The Magic Formula: 🪄
Testing stuffs: ฅ^•ﻌ•^ฅ
Can u pwease merge my pwull wequest senpai?!! This will make Signal Desktop kawaii AF!!
nuzzles your codebase UwU
P.S. - Now with SETTINGS!! 6 files changed total!! The feature is OFF by default so no breaky existing users!! Opt-in chaos!! (。♥‿♥。)
happy coding noises (◕‿◕)♡