msglist: Double-tap to add/remove 👍 reaction - #2325
Conversation
a3598b3 to
2e96f59
Compare
rajveermalviya
left a comment
There was a problem hiding this comment.
Thanks for working on this @sm-sayedi! Comments below.
| static Future<void> addOrRemoveReaction({ | ||
| required BuildContext context, | ||
| required bool doRemoveReaction, | ||
| static Future<void> addOrRemoveReaction(BuildContext context, { |
There was a problem hiding this comment.
The dartdoc needs to be updated here, about the new behaviour that reaction will be toggled depending on whether it is self-voted or not.
There was a problem hiding this comment.
Also maybe the name should also reflect that it is a toggle now? Not totally sure about that.
There was a problem hiding this comment.
I think the phrase “add or remove” in the name kind of implies the toggle behavior.
| @@ -1394,24 +1383,18 @@ class ReactionButtons extends StatelessWidget { | |||
|
|
|||
| final emoji = await showEmojiPickerSheet(pageContext: pageContext); | |||
| if (emoji == null || !pageContext.mounted) return; | |||
| unawaited(ZulipAction.addOrRemoveReaction( | |||
| context: pageContext, | |||
| doRemoveReaction: false, | |||
There was a problem hiding this comment.
actions [nfc]: Simplify addOrRemoveReaction
…
Previously this was passing doRemoveReaction: false unconditionally, and now it will toggle between add/remove depending on whether that reaction exists, right?
Since this is not an NFC change, it should go in its own commit.
There was a problem hiding this comment.
Correct, thanks for flagging.
| onDoubleTap: () { | ||
| final firstPopularEmoji = store.popularEmojiCandidates().firstOrNull; | ||
| // Popular emojis are not loaded yet; do nothing. | ||
| if (firstPopularEmoji == null) return; |
There was a problem hiding this comment.
nit: Maybe add a TODO(log) here?
| @@ -2516,6 +2528,55 @@ void main() { | |||
| doTest(expected: true, MentionsNarrow(), | |||
| mkMessage: () => eg.streamMessage(flags: [MessageFlag.mentioned])); | |||
| }); | |||
|
|
|||
| testWidgets('Double tap adds/removes +1 reaction', (tester) async { | |||
There was a problem hiding this comment.
Let's have one more test: Double tapping on an interactive element in a message, like an image/link, toggles the reaction instead of opening them.
| // This `onDoubleTap` handler delays firing the `onTap` handler of this | ||
| // GestureDetector and of the child gesture recognizers by the amount of | ||
| // `kDoubleTapTimeout` duration. Keep an eye on that, especially in tests. | ||
| // Related upstream issues: | ||
| // - https://github.com/flutter/flutter/issues/106170 | ||
| // - https://github.com/flutter/flutter/issues/110300 |
There was a problem hiding this comment.
Thanks for flagging this! But maybe this comment belongs in the commit message?
There was a problem hiding this comment.
I am not sure if the commit message would be a better home for it. 🙂 Per the commit discipline guideline, the commit description is generally for highlighting the difference between the old and new versions of the code.
- Use the commit message for information that’s relevant for someone trying to understand the change this commit is making, or the difference between the old version of the code and the new version. In particular, this includes information about why the new version of the code is better than, or not worse than, the old version.
- Use code comments, or the code itself, for information that’s relevant for someone trying to read and understand the new version of the code in the future, without comparing it to the old version.
I think this specific comment aligns more with the second point. Especially, it could be useful info for someone who experiences some odd behavior in tests. Please let me know if it's otherwise!
| // - https://github.com/flutter/flutter/issues/106170 | ||
| // - https://github.com/flutter/flutter/issues/110300 | ||
| onDoubleTap: () { | ||
| final firstPopularEmoji = store.popularEmojiCandidates().firstOrNull; |
There was a problem hiding this comment.
nit: Let's flag in commit message, that actual reaction would be the first one in the "Popular Emoji" list. Which just happens to be the 👍 emoji, currently.
19fc1a1 to
f9298b4
Compare
|
Thanks @rajveermalviya for the review. Pushed a new revision. |
rajveermalviya
left a comment
There was a problem hiding this comment.
Thanks for the revision! All LGTM, moving over to Chris' review.
chrisbobbe
left a comment
There was a problem hiding this comment.
Thanks for taking this on @sm-sayedi! Quick double-tap reactions are a nice
low-friction touch, and this turned out to have more to it than we expected.
The smaller points are inline (dropping the emoji-picker-remove commit and its
follow-on, and gating the double-tap on showAsMuted); the main thing is the
onTap latency you flagged.
The delay is intrinsic to the gesture arena: a DoubleTapGestureRecognizer
holds the arena for kDoubleTapTimeout after the first tap, and that hold is
what delays descendant link/spoiler taps.
The gesture itself isn't in question; it's the feature we wanted, and the
latency is just a wrinkle none of us anticipated until you built it. I'd
detect the double-tap with a Listener instead of onDoubleTap. A Listener
reads raw pointer events without joining the arena, so it never
holds: the descendant taps fire immediately, and a manual timer catches the
double-tap. One tradeoff: a double-tap on a link both reacts and opens the
link, since nothing suppresses it once it fires. I'm fine with that; the
on interactive element test would change to match.
Separately, let's add a lightImpact haptic when the react fires. The reaction
only appears once the request round-trips, so without some immediate feedback,
the double-tap can feel like it didn't register; a light haptic confirms it
landed.
Here's how I'd like to handle it. If you take everything except the latency
(the inline points above, plus the haptic), then stand by and I'll push the
Listener change on top of your branch; I have it prototyped, with tests. After
that, you (and maybe @rajveermalviya?) can review those commits and I'll merge.
One general note, not specific to this PR: when a change has a user-facing
tradeoff, like the tap latency here or the emoji picker now removing a
reaction you've already given, please call it out in the description, even in
a line. Both were clear from the diff, so no harm; I mostly want to know
whether they were deliberate product calls (if so, great, and say why) or just
hadn't come up. :)
| final zulipLocalizations = ZulipLocalizations.of(pageContext); | ||
|
|
||
| final isSelfVoted = store.selfHasVoted(message.id, withEmoji: emoji); | ||
| unawaited(ZulipAction.addOrRemoveReaction( |
There was a problem hiding this comment.
emoji_reaction: Make emoji picker remove a reaction when already reacted
I'm not sure this change makes sense without something else the web app does, which we don't do: in the emoji picker, marking the emojis you have voted for. Since we don't differentiate self-voted emojis in the list, you might tap on one if you've forgotten that you already voted for it, and in that case I think it would be worse to remove the vote (compared to the current behavior, which I think is just an error dialog with a server message like "reaction already exists").
It might make sense to mark self-voted emojis in the list, but I'd leave that out-of-scope for this PR, and just drop this commit.
There was a problem hiding this comment.
Then since the emoji picker still wants an idempotent version of ZulipAction.addOrRemoveReaction—one that just adds the reaction, rather than reading the current state and toggling it—it might also make sense to drop your subsequent commit removing that capability from ZulipAction.addOrRemoveReaction.
There was a problem hiding this comment.
| // Related upstream issues: | ||
| // - https://github.com/flutter/flutter/issues/106170 | ||
| // - https://github.com/flutter/flutter/issues/110300 | ||
| onDoubleTap: () { |
There was a problem hiding this comment.
Like with onLongPress, we shouldn't add the double-tap-to-react handler if showAsMuted is true. When message content is hidden because it's from a muted sender (with a "reveal" button), don't we also hide the reaction chips? An accidental double-tap on a message would add a reaction and you wouldn't know about it unless you happened to hit that "Reveal" button; that could end up being an upsetting consequence. Let's add a test for the desired behavior too.
There was a problem hiding this comment.
Thanks for flagging! I had a plan to add this feature, but it seems that I had forgotten about it. 😀
|
Question: I used Claude (Fable 5) to help me write my review above, carefully reviewing Claude's output and making edits: #2325 (review) . Zulip's AI use policy currently permits this (the "Do not post AI-generated messages" there applies to CZO only), but I'm wondering how it lands and if you have any feedback about it. 🙂 |
aa41ec0 to
be78a3e
Compare
|
Thanks @chrisbobbe for the review. Pushed the changes, PTAL. A few notes:
|
|
(The CI failure seems to be unrelated to the PR; it passes locally. See #mobile-team > Multiple CI failures.) |
|
Ohh, I think I've misread the following sentence in your last review:
Although it seems obvious now, I thought you wanted to push the "haptic feedback" change and for me to do the Listener change for double-tap. (I think I read this review part near my sleep time, hence the confusion.) Please have a look at the new implementation for double-tap recognition in this branch and see if it matches what you'd expected. I will now push the "haptic feedback" change too. |
be78a3e to
e1bbac2
Compare
|
(Pushed the haptic feedback change.) |
eb689cd to
9e0ccb5
Compare
|
Thanks for the review, Chris. Revision pushed; PTAL. |
9e0ccb5 to
9d8b2e1
Compare
chrisbobbe
left a comment
There was a problem hiding this comment.
Thanks! In particular I appreciate your careful work in making multiple separate commits to adapt the upstream code. (I haven't read all those commits yet, but it looks like a commit strategy that'll help make review easier.)
Here's a review from reading the first 10 commits:
af249ec message [nfc]: Move hasSelfVote to MessageStore, rename to selfHasVoted
932c2ae message: Add MessageStore.selfVotes method
6c71bb1 message: Simplify selfHasVoted to use selfVotes
91f7b77 emoji [nfc]: Support highlighting pre-selected emojis in emoji picker
5140eb2 emoji_reaction: Highlight message reaction emojis in reaction emoji picker
64a04c2 emoji_reaction: Make emoji picker remove a reaction when already reacted
b330aac actions [nfc]: Simplify addOrRemoveReaction, rename to toggleReaction
b9f3b46 deps: Add leak_tracker_flutter_testing and meta as direct dev dependencies
4ede3ca double-tap: Introduce DoubleTapRecognizer
ef7d3bc double-tap test: Adjust a test group to practice onTap, not onTapDown
leaving the remaining 8 for a future round:
529d047 double-tap: Make DoubleTapRecognizer not participate in gesture arena
0b2129f double-tap: Remove a few unnecessary properties we don't plan to use
5481a18 double-tap [nfc]: Tighten up the code
5587a5a double-tap test: Tighten up the code
fbead01 double-tap [nfc]: Move DoubleTapRecognizer to the top of the file
21c1e4f double-tap: Cancel _TapTracker's internal timer when appropriate
94dd3ff double-tap: Introduce DoubleTapListener
9d8b2e1 msglist: Double-tap to add/remove +1 reaction
| /// The emojis that the self-user has voted on the given message. | ||
| /// | ||
| /// The emojis are represented by type and code only, ignoring the name; | ||
| /// see [ReactionWithVotes]. | ||
| Iterable<({ReactionType type, String code})> selfVotes(int messageId) { | ||
| final message = messages[messageId]; | ||
| if (message == null) { | ||
| assert(false); // TODO(log) | ||
| return []; | ||
| } | ||
|
|
||
| return message.reactions?.aggregated | ||
| .where((reactionWithVotes) => reactionWithVotes.userIds.contains(selfUserId)) | ||
| .map((e) => (code: e.emojiCode, type: e.reactionType)) ?? []; | ||
| } |
There was a problem hiding this comment.
Iterable.map returns a lazy iterable:
https://api.flutter.dev/flutter/dart-core/Iterable/map.html
The returned iterable is lazy, so it won't iterate the elements of this iterable until it is itself iterated, and then it will apply toElement to create one element at a time. The converted elements are not cached. Iterating multiple times over the returned Iterable will invoke the supplied toElement function once per element for on each iteration.
, so a check for a given self-vote is linear in the number of self-votes. That's not typically large—but if we have a Set<{ReactionType type, String code}> of the self-votes, lookups would be O(1), which is nicer.
In an earlier review (#2325 (comment)) I suggested adding a set like that as a field on Reactions, which we'd keep up-to-date with events. I think that's overkill, though: these UIs don't need to live-update, because they're torn down when you use them to change a self-vote (tap an emoji, and the message action sheet or the emoji picker is closed). Instead, I think I'd add a method like the following on the Reactions class, with a note about not calling it where repeatedly constructing the set might get expensive:
/// The reactions the self-user has voted for, as (type, code) pairs.
///
/// This walks [aggregated] on each call.
/// Callers checking many emoji against the result
/// (e.g. each entry in the emoji picker)
/// should call this once and reuse the returned set,
/// rather than calling it per emoji.
Set<({ReactionType type, String code})> selfVotes(int selfUserId) { ... }
| bool selfHasVoted(int messageId, {required EmojiCandidate withEmoji}) { | ||
| final message = messages[messageId]; | ||
| if (message == null) { | ||
| assert(false); // TODO(log) |
There was a problem hiding this comment.
assert(false) should be unreachable, and be easy to verify as unreachable by reading code.
This one turns out to be reachable, which means it can crash debug builds:
In a debug build from this PR:
- Long-press any message you can delete, to open the message action sheet
- Star the message so that step 3 will cause a rebuild (otherwise do something after step 3 that notifies MessageStore's listeners, like starring a different message)
- Delete the message from another client
- See error:
In another comment I suggested providing a self-votes set on Reactions itself, so with that, I think the fix here is for ReactionButtons to get that set from the message it's passed. So in ReactionButtons.build:
final selfVotes = message.reactions?.selfVotes(store.selfUserId) ?? {};and pass that set to _buildButton.
There was a problem hiding this comment.
Thanks for the details.
This symptom doesn't seem to reproduce at the commit where selfHasVoted was first introduced:
message [nfc]: Move hasSelfVote to MessageStore, rename to selfHasVoted
But rather at a later commit where it uses the store from a different context:
actions [nfc]: Simplify addOrRemoveReaction, rename to toggleReaction
So PerAccountStoreWidget.of(pageContext).selfHasVoted in the first commit
vs.
PerAccountStoreWidget.of(context).selfHasVoted in the later commit.
That means a change in the store from pageContext doesn't call the current context's build method in the first commit. Changed the later commit to use pageContext as before.
But besides that, it turns out assert(false) is still reachable (from the second commit mentioned):
- Long-press any message you can delete to open the message action sheet.
- Tap the "more >" button in
ReactionButtonsto open the emoji picker. - Delete the message from another client.
- Tap an emoji entry in the emoji picker to add a reaction.
- See the assert output in the console.
What about removing the assert? Now that there can be at least one path for the assert to be triggered.
|
|
||
| import 'package:flutter/gestures.dart'; | ||
|
|
||
| // Copied verbatim from the usptream Flutter repo, |
There was a problem hiding this comment.
nit: spelling of "upstream"
| import 'compose_box.dart'; | ||
| import 'content.dart'; | ||
| import 'emoji_reaction.dart'; | ||
| import 'double_tap_listener.dart'; |
There was a problem hiding this comment.
nit: keep alphabetical order of imports
| // Copied from the usptream Flutter repo, but adjusted to not practice | ||
| // participating in the gesture arena. | ||
| // https://github.com/flutter/flutter/blob/ab7eb7aff/packages/flutter/test/gestures/double_tap_test.dart |
There was a problem hiding this comment.
I'm having trouble parsing this sentence: should it say "adjusted to not participate in […]"?
Also nit: spelling of "upstream"
| @@ -0,0 +1,341 @@ | |||
| import 'dart:async'; | |||
There was a problem hiding this comment.
double-tap: Introduce DoubleTapRecognizer
[…]
This is in preparation for creating a double tap recognizer that doesn't
participate in the gesture arena, otherwise DoubleTapGestureRecognizer
is already available through the Flutter API.
[…]
Commit-message nit: fix comma splice, e.g.:
This is in preparation for making a double-tap recognizer that doesn't participate in the gesture arena. (For one that does, Flutter's own DoubleTapGestureRecognizer already works.)
| import 'package:leak_tracker_flutter_testing/leak_tracker_flutter_testing.dart'; | ||
| import 'package:meta/meta.dart'; | ||
|
|
||
| // Copied from the usptream Flutter repo: |
There was a problem hiding this comment.
nit: "upstream" misspelled here too
| // May replace the usages of this class if (one of) the following upstream | ||
| // issues are solved: | ||
| // - https://github.com/flutter/flutter/issues/106170 | ||
| // - https://github.com/flutter/flutter/issues/110300 |
There was a problem hiding this comment.
Let's use our greppable TODO(upstream) form for this, and summarize the problem so the reader doesn't have to follow links to understand it:
// TODO(upstream): Delete this class if Flutter gains a way to detect
// double taps without delaying single taps:
// https://github.com/flutter/flutter/issues/106170
// https://github.com/flutter/flutter/issues/110300| // May replace the usages of this widget if (one of) the following upstream | ||
| // issues are solved: | ||
| // - https://github.com/flutter/flutter/issues/106170 | ||
| // - https://github.com/flutter/flutter/issues/110300 |
There was a problem hiding this comment.
Similarly here:
// TODO(upstream): Delete this class if Flutter gains a way to detect
// double taps without delaying single taps:
// https://github.com/flutter/flutter/issues/106170
// https://github.com/flutter/flutter/issues/110300| // Copied verbatim from the usptream Flutter repo, | ||
| // only renaming DoubleTapGestureRecognizer to DoubleTapRecognizer. | ||
| // https://github.com/flutter/flutter/blob/ab7eb7aff/packages/flutter/lib/src/gestures/multitap.dart#L50-L382 |
There was a problem hiding this comment.
This is a substantial copy-paste of the Flutter framework code, so let's keep and adapt the copyright notice at the top of all three copied files. How about:
// Copyright 2014 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the docs/THIRDPARTY file.
You can create the docs/THIRDPARTY file with this text:
Details
Format: https://www.debian.org/doc/packaging-manuals/copyright-format/1.0/
Upstream-Name: Zulip Flutter
Upstream-Contact: Zulip development community <https://chat.zulip.org>
Source: https://github.com/zulip/zulip-flutter
Comment:
Unless otherwise noted, the Zulip Flutter app is distributed under the
Apache License, Version 2.0; see the LICENSE file. The software includes
some works released by third parties under other free and open source
licenses. Those works are redistributed under the license terms under
which the works were received.
.
(Licenses for fonts and other assets in our asset bundle appear in
LICENSE files alongside the assets themselves, and are shown to users
in the app's licenses page; see lib/licenses.dart.)
Files: *
Copyright: Kandra Labs, Inc., and contributors
License: Apache-2.0
Files: lib/widgets/double_tap_recognizer.dart
test/widgets/double_tap_recognizer_test.dart
test/widgets/gesture_tester.dart
Copyright: 2014 The Flutter Authors
License: BSD-3-clause
Comment:
Copied and adapted from the Flutter framework, at commit ab7eb7aff:
.
https://github.com/flutter/flutter/blob/ab7eb7aff/packages/flutter/lib/src/gestures/multitap.dart#L49-L382
https://github.com/flutter/flutter/blob/ab7eb7aff/packages/flutter/test/gestures/double_tap_test.dart
https://github.com/flutter/flutter/blob/ab7eb7aff/packages/flutter/test/gestures/gesture_tester.dart
.
See the header comments in those files for details of the adaptations.
License: Apache-2.0
The complete text of the Apache License, Version 2.0 can be found in
the LICENSE file in the root of this source tree, or at
https://www.apache.org/licenses/LICENSE-2.0 .
License: BSD-3-clause
Redistribution and use in source and binary forms, with or without modification,
are permitted provided that the following conditions are met:
.
* Redistributions of source code must retain the above copyright
notice, this list of conditions and the following disclaimer.
* Redistributions in binary form must reproduce the above
copyright notice, this list of conditions and the following
disclaimer in the documentation and/or other materials provided
with the distribution.
* Neither the name of Google Inc. nor the names of its
contributors may be used to endorse or promote products derived
from this software without specific prior written permission.
.
THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND
ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR
ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
(INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
There was a problem hiding this comment.
Done. Thanks for preparing this.
b56ec01 to
b48f328
Compare
|
Thanks for the review. New revision pushed. |
The reason for the rename is to follow the naming convention for other self-user-related methods in the store.
A method for getting the reactions the self-user has voted for.
…icker To match the behavior on the web. In the next commits where we remove a reaction when an already-voted emoji is tapped in the emoji picker, this change will highlight that emoji so the user knows the consequence of their tap (adding a reaction by tapping an unhighlighted emoji and removing a reaction by tapping a highlighted one). There seems to be no design in the Figma file for how a highlighted emoji entry should look like in the emoji picker dialog. Currently, we use the same background color used for the self-voted emoji(s) in message action sheet reaction buttons.
Previously, when a message already had a particular reaction, tapping on that reaction through the emoji picker would try to add the same reaction again, which would result in an error from the server. Now, we handle that by removing the reaction. This is what the web does too.
We can compute the values for some of the parameters inside the method itself, so remove those ones from the method declaration. Also, simplify other parts of the code where this method was called. With the simplification, it's also better to rename the method to toggleReaction, as "add or remove" isn't something the caller has control over.
…ncies In the next commit(s), we'll need to import these directly in test/widget/gesture_tester.dart, a file that will be copied verbatim from the upstream repo for testing gestures.
Copies the DoubleTapGestureRecognizer implementation verbatim: https://github.com/flutter/flutter/blob/ab7eb7aff/packages/flutter/lib/src/gestures/multitap.dart#L49-L382 This is in preparation for making a double-tap recognizer that doesn't participate in the gesture arena. (For one that does, Flutter's own DoubleTapGestureRecognizer already works.) The name DoubleTapRecognizer is to distinguish it from the upstream DoubleTapGestureRecognizer that participates in the gesture arena.
The upstream test "A primary double tap recognizer forms competition with a primary tap recognizer" claims that double-tap recognizer delays a tap recognizer's onTapDown callback, but uses a delay of 300 ms (`kDoubleTapTimeout`) to verify this. However, the test also passes with shorter delays. It appears that the actual delay for onTapDown is 100 ms (`kPressTimeout`). We could either reduce the delay in the test to 100 ms, or keep the 300 ms delay and verify the onTap callback instead. This commit takes the latter approach, which also aligns with the goal of the following commits, where we make the double-tap recognizer stop delaying onTap callbacks by up to 300 ms.
So that we can have a double-tap recognizer that doesn't delay TapGestureRecognizer.onTap by up to 300 ms.
Of the removed properties, `gestureSettings` was already unutilized.
This includes using modern syntax and following the Flutter style guide for ordering the class members. https://github.com/flutter/flutter/blob/ab7eb7aff6f/docs/contributing/Style-guide-for-Flutter-repo.md#order-other-class-members-in-a-way-that-makes-sense
This also corrects a few places where an incorrect pointer event was passed to GestureTester.route.
This makes the primary class the first one in the file, improving discoverability when opening it.
Cancel the tracker's internal timer when a second tap is registered and when the tracker is rejected. This avoids leaving pending `kDoubleTapMinTime` timers in tests.
A widget that detects double-tap gestures without participating in the gesture arena. This can be useful when adding an `onDoubleTap` handler to a GestureDetector would be undesirable because it delays `onTap` handlers by up to `kDoubleTapTimeout` (300 ms as of 2026-06) to distinguish between single and double taps.
Fixes zulip#969. The actual reaction would be the first one in the "Popular Emoji" list, which currently is the 👍 emoji.
b48f328 to
fc5b4ee
Compare



Fixes: #969
Screen recording
Screen.Recording.2026-05-22.at.1.55.28.AM.mov