Repository navigation
msglist: Double-tap to add/remove 👍 reaction #2325
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?
Changes from all commits
759b401
dba3284
9c20ffd
926ea0c
2f33d8b
b8f407c
b4987d4
53556ab
1c0069b
70a2e14
25de342
41a4cde
d69eb18
217a233
e18c3b3
946dd87
0d0581e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,62 @@ | ||
| 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. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1355,23 +1355,12 @@ class ReactionButtons extends StatelessWidget { | |
| /// triggered from. | ||
| final BuildContext pageContext; | ||
|
|
||
| void _handleTapReaction({ | ||
| required EmojiCandidate emoji, | ||
| required bool isSelfVoted, | ||
| }) { | ||
| void _handleTapReaction(EmojiCandidate emoji) { | ||
| // Dismiss the enclosing action sheet immediately, | ||
| // for swift UI feedback that the user's selection was received. | ||
| Navigator.pop(pageContext); | ||
|
|
||
| final zulipLocalizations = ZulipLocalizations.of(pageContext); | ||
| ZulipAction.addOrRemoveReaction( | ||
| context: pageContext, | ||
| doRemoveReaction: isSelfVoted, | ||
| messageId: message.id, | ||
| emoji: emoji, | ||
| errorDialogTitle: isSelfVoted | ||
| ? zulipLocalizations.errorReactionRemovingFailedTitle | ||
| : zulipLocalizations.errorReactionAddingFailedTitle); | ||
| ZulipAction.toggleReaction(pageContext, messageId: message.id, emoji: emoji); | ||
| } | ||
|
|
||
| void _handleTapMore() async { | ||
|
|
@@ -1381,26 +1370,22 @@ class ReactionButtons extends StatelessWidget { | |
| // Dismiss current action sheet before opening emoji picker sheet. | ||
| Navigator.of(pageContext).pop(); | ||
|
|
||
| final emoji = await showEmojiPickerSheet(pageContext: pageContext); | ||
| final emoji = await showEmojiPickerSheet( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Commit-message nits:
|
||
| pageContext: pageContext, | ||
| selectedEmojis: message.reactions?.selfVotes(PerAccountStoreWidget.of(pageContext).selfUserId) ?? {}); | ||
| if (emoji == null || !pageContext.mounted) return; | ||
| unawaited(ZulipAction.addOrRemoveReaction( | ||
| context: pageContext, | ||
| doRemoveReaction: false, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Previously this was passing
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct, thanks for flagging. |
||
| messageId: message.id, | ||
| emoji: emoji, | ||
| errorDialogTitle: | ||
| ZulipLocalizations.of(pageContext).errorReactionAddingFailedTitle)); | ||
| unawaited(ZulipAction.toggleReaction(pageContext, | ||
| messageId: message.id, emoji: emoji)); | ||
| } | ||
|
|
||
| Widget _buildButton({ | ||
| required BuildContext context, | ||
| Widget _buildButton(BuildContext context, { | ||
| required EmojiCandidate emoji, | ||
| required bool isSelfVoted, | ||
| required bool isFirst, | ||
| }) { | ||
| final store = PerAccountStoreWidget.of(pageContext); | ||
| final designVariables = DesignVariables.of(context); | ||
| return Flexible(child: InkWell( | ||
| onTap: () => _handleTapReaction(emoji: emoji, isSelfVoted: isSelfVoted), | ||
| onTap: () => _handleTapReaction(emoji), | ||
| splashFactory: NoSplash.splashFactory, | ||
| borderRadius: isFirst | ||
| ? const BorderRadiusDirectional.only(topStart: Radius.circular(7)) | ||
|
|
@@ -1414,7 +1399,7 @@ class ReactionButtons extends StatelessWidget { | |
| width: double.infinity, | ||
| padding: const EdgeInsets.symmetric(vertical: 12, horizontal: 5), | ||
| alignment: Alignment.center, | ||
| color: isSelfVoted | ||
| color: store.selfHasVoted(message.id, withEmoji: emoji) | ||
| ? designVariables.contextMenuItemBg.withFadedAlpha(0.20) | ||
| : null, | ||
| child: UnicodeEmojiWidget( | ||
|
|
@@ -1439,25 +1424,13 @@ class ReactionButtons extends StatelessWidget { | |
| final zulipLocalizations = ZulipLocalizations.of(context); | ||
| final designVariables = DesignVariables.of(context); | ||
|
|
||
| bool hasSelfVote(EmojiCandidate emoji) { | ||
| return message.reactions?.aggregated.any((reactionWithVotes) { | ||
| return reactionWithVotes.reactionType == ReactionType.unicodeEmoji | ||
| && reactionWithVotes.emojiCode == emoji.emojiCode | ||
| && reactionWithVotes.userIds.contains(store.selfUserId); | ||
| }) ?? false; | ||
| } | ||
|
|
||
| return Container( | ||
| decoration: BoxDecoration( | ||
| color: designVariables.contextMenuItemBg.withFadedAlpha(0.12)), | ||
| child: Row(children: [ | ||
| Flexible(child: Row(spacing: 1, children: List.unmodifiable( | ||
| popularEmojiCandidates.mapIndexed((index, emoji) => | ||
| _buildButton( | ||
| context: context, | ||
| emoji: emoji, | ||
| isSelfVoted: hasSelfVote(emoji), | ||
| isFirst: index == 0))))), | ||
| _buildButton(context, emoji: emoji, isFirst: index == 0))))), | ||
| InkWell( | ||
| onTap: _handleTapMore, | ||
| splashFactory: NoSplash.splashFactory, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,95 @@ | ||
| import 'dart:ui'; | ||
|
|
||
| import 'package:flutter/widgets.dart'; | ||
|
|
||
| import 'double_tap_recognizer.dart'; | ||
|
|
||
| /// 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] to distinguish between single and double taps. | ||
| /// | ||
| /// The tradeoff is that a double tap may trigger both this widget's callback | ||
| /// and another gesture handler in the tree that responds to one or both taps. | ||
| /// | ||
| /// See also: | ||
| /// * [DoubleTapRecognizer], which this widget uses internally to recognize | ||
| /// double taps without participating in the gesture arena. | ||
| // | ||
| // 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 | ||
| class DoubleTapListener extends StatefulWidget { | ||
| const DoubleTapListener({ | ||
| super.key, | ||
| this.behavior = .translucent, | ||
| this.supportedDevices, | ||
| this.onDoubleTap, | ||
| this.child, | ||
| }); | ||
|
|
||
| /// How this listener should behave during hit testing when deciding | ||
| /// how the hit test propagates to children and whether to consider targets | ||
| /// behind this one. | ||
| /// | ||
| /// Defaults to [HitTestBehavior.translucent]. | ||
| /// | ||
| /// See [HitTestBehavior] for the allowed values and their meanings. | ||
| final HitTestBehavior behavior; | ||
|
|
||
| /// The kind of devices that are allowed to be recognized. | ||
| /// | ||
| /// If set to null, events from all device types will be recognized. Defaults to null. | ||
| final Set<PointerDeviceKind>? supportedDevices; | ||
|
|
||
| /// Called when the user performs a double tap. | ||
| /// | ||
| /// If null, this widget does not recognize double taps. | ||
| final VoidCallback? onDoubleTap; | ||
|
|
||
| /// The widget below this widget in the tree. | ||
| /// | ||
| /// {@macro flutter.widgets.ProxyWidget.child} | ||
| final Widget? child; | ||
|
|
||
| @override | ||
| State<DoubleTapListener> createState() => _DoubleTapListenerState(); | ||
| } | ||
|
|
||
| class _DoubleTapListenerState extends State<DoubleTapListener> { | ||
| late final DoubleTapRecognizer _recognizer; | ||
|
|
||
| @override | ||
| void initState() { | ||
| super.initState(); | ||
| _recognizer = DoubleTapRecognizer() | ||
| ..supportedDevices = widget.supportedDevices | ||
| ..onDoubleTap = widget.onDoubleTap; | ||
| } | ||
|
|
||
| @override | ||
| void didUpdateWidget(covariant DoubleTapListener oldWidget) { | ||
| super.didUpdateWidget(oldWidget); | ||
| _recognizer | ||
| ..supportedDevices = widget.supportedDevices | ||
| ..onDoubleTap = widget.onDoubleTap; | ||
| } | ||
|
|
||
| @override | ||
| void dispose() { | ||
| _recognizer.dispose(); | ||
| super.dispose(); | ||
| } | ||
|
|
||
| @override | ||
| Widget build(BuildContext context) { | ||
| return Listener( | ||
| onPointerDown: _recognizer.addPointer, | ||
| behavior: widget.behavior, | ||
| child: widget.child, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
In another comment I suggested providing a self-votes set on
Reactionsitself, so with that, I think the fix here is forReactionButtonsto get that set from the message it's passed. So inReactionButtons.build:and pass that set to
_buildButton.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the details.
This symptom doesn't seem to reproduce at the commit where
selfHasVotedwas first introduced:But rather at a later commit where it uses the store from a different context:
So
PerAccountStoreWidget.of(pageContext).selfHasVotedin the first commitvs.
PerAccountStoreWidget.of(context).selfHasVotedin the later commit.That means a change in the store from
pageContextdoesn't call the current context'sbuildmethod in the first commit. Changed the later commit to usepageContextas before.But besides that, it turns out
assert(false)is still reachable (from the second commit mentioned):ReactionButtonsto open the emoji picker.What about removing the assert? Now that there can be at least one path for the assert to be triggered.