From 5cf87f82ade7dccbaa90e218922470f91e9032ea Mon Sep 17 00:00:00 2001 From: Rajesh Malviya Date: Wed, 4 Dec 2024 23:22:17 +0530 Subject: [PATCH] action_sheet: Support reacting with popular emojis --- assets/l10n/app_en.arb | 8 ++ lib/generated/l10n/zulip_localizations.dart | 12 ++ .../l10n/zulip_localizations_ar.dart | 6 + .../l10n/zulip_localizations_en.dart | 6 + .../l10n/zulip_localizations_ja.dart | 6 + lib/widgets/action_sheet.dart | 126 ++++++++++++----- lib/widgets/emoji_reaction.dart | 41 ++++++ test/flutter_checks.dart | 4 + test/widgets/action_sheet_test.dart | 131 +++++++++++++----- 9 files changed, 268 insertions(+), 72 deletions(-) diff --git a/assets/l10n/app_en.arb b/assets/l10n/app_en.arb index 1b8e6ff8b8..d66f143151 100644 --- a/assets/l10n/app_en.arb +++ b/assets/l10n/app_en.arb @@ -603,5 +603,13 @@ "errorNotificationOpenAccountMissing": "The account associated with this notification no longer exists.", "@errorNotificationOpenAccountMissing": { "description": "Error message when the account associated with the notification is not found" + }, + "errorReactionAddingFailedTitle": "Adding reaction failed", + "@errorReactionAddingFailedTitle": { + "description": "Error title when adding a message reaction fails" + }, + "errorReactionRemovingFailedTitle": "Removing reaction failed", + "@errorReactionRemovingFailedTitle": { + "description": "Error title when removing a message reaction fails" } } diff --git a/lib/generated/l10n/zulip_localizations.dart b/lib/generated/l10n/zulip_localizations.dart index b058af8f2b..9d293139f8 100644 --- a/lib/generated/l10n/zulip_localizations.dart +++ b/lib/generated/l10n/zulip_localizations.dart @@ -894,6 +894,18 @@ abstract class ZulipLocalizations { /// In en, this message translates to: /// **'The account associated with this notification no longer exists.'** String get errorNotificationOpenAccountMissing; + + /// Error title when adding a message reaction fails + /// + /// In en, this message translates to: + /// **'Adding reaction failed'** + String get errorReactionAddingFailedTitle; + + /// Error title when removing a message reaction fails + /// + /// In en, this message translates to: + /// **'Removing reaction failed'** + String get errorReactionRemovingFailedTitle; } class _ZulipLocalizationsDelegate extends LocalizationsDelegate { diff --git a/lib/generated/l10n/zulip_localizations_ar.dart b/lib/generated/l10n/zulip_localizations_ar.dart index 95ff1d0aea..ae652f3c7e 100644 --- a/lib/generated/l10n/zulip_localizations_ar.dart +++ b/lib/generated/l10n/zulip_localizations_ar.dart @@ -478,4 +478,10 @@ class ZulipLocalizationsAr extends ZulipLocalizations { @override String get errorNotificationOpenAccountMissing => 'The account associated with this notification no longer exists.'; + + @override + String get errorReactionAddingFailedTitle => 'Adding reaction failed'; + + @override + String get errorReactionRemovingFailedTitle => 'Removing reaction failed'; } diff --git a/lib/generated/l10n/zulip_localizations_en.dart b/lib/generated/l10n/zulip_localizations_en.dart index d440ed2b10..fe2c93d236 100644 --- a/lib/generated/l10n/zulip_localizations_en.dart +++ b/lib/generated/l10n/zulip_localizations_en.dart @@ -478,4 +478,10 @@ class ZulipLocalizationsEn extends ZulipLocalizations { @override String get errorNotificationOpenAccountMissing => 'The account associated with this notification no longer exists.'; + + @override + String get errorReactionAddingFailedTitle => 'Adding reaction failed'; + + @override + String get errorReactionRemovingFailedTitle => 'Removing reaction failed'; } diff --git a/lib/generated/l10n/zulip_localizations_ja.dart b/lib/generated/l10n/zulip_localizations_ja.dart index 42128ba024..6d82e0d19c 100644 --- a/lib/generated/l10n/zulip_localizations_ja.dart +++ b/lib/generated/l10n/zulip_localizations_ja.dart @@ -478,4 +478,10 @@ class ZulipLocalizationsJa extends ZulipLocalizations { @override String get errorNotificationOpenAccountMissing => 'The account associated with this notification no longer exists.'; + + @override + String get errorReactionAddingFailedTitle => 'Adding reaction failed'; + + @override + String get errorReactionRemovingFailedTitle => 'Removing reaction failed'; } diff --git a/lib/widgets/action_sheet.dart b/lib/widgets/action_sheet.dart index 1b6c8b2e58..4cc59784f4 100644 --- a/lib/widgets/action_sheet.dart +++ b/lib/widgets/action_sheet.dart @@ -1,5 +1,6 @@ import 'dart:async'; +import 'package:collection/collection.dart'; import 'package:flutter/foundation.dart'; import 'package:flutter/material.dart'; import 'package:flutter/services.dart'; @@ -9,6 +10,7 @@ import '../api/exception.dart'; import '../api/model/model.dart'; import '../api/route/messages.dart'; import '../generated/l10n/zulip_localizations.dart'; +import '../model/emoji.dart'; import '../model/internal_link.dart'; import '../model/narrow.dart'; import 'actions.dart'; @@ -16,6 +18,8 @@ import 'clipboard.dart'; import 'color.dart'; import 'compose_box.dart'; import 'dialog.dart'; +import 'emoji.dart'; +import 'emoji_reaction.dart'; import 'icons.dart'; import 'inset_shadow.dart'; import 'message_list.dart'; @@ -25,7 +29,7 @@ import 'theme.dart'; void _showActionSheet( BuildContext context, { - required List optionButtons, + required List optionButtons, }) { showModalBottomSheet( context: context, @@ -161,16 +165,8 @@ void showMessageActionSheet({required BuildContext context, required Message mes final markAsUnreadSupported = store.connection.zulipFeatureLevel! >= 155; // TODO(server-6) final showMarkAsUnreadButton = markAsUnreadSupported && isMessageRead; - final hasThumbsUpReactionVote = message.reactions - ?.aggregated.any((reactionWithVotes) => - reactionWithVotes.reactionType == ReactionType.unicodeEmoji - && reactionWithVotes.emojiCode == '1f44d' - && reactionWithVotes.userIds.contains(store.selfUserId)) - ?? false; - final optionButtons = [ - if (!hasThumbsUpReactionVote) - AddThumbsUpButton(message: message, pageContext: context), + ReactionButtons(message: message, pageContext: context), StarButton(message: message, pageContext: context), if (isComposeBoxOffered) QuoteAndReplyButton(message: message, pageContext: context), @@ -194,41 +190,95 @@ abstract class MessageActionSheetMenuItemButton extends ActionSheetMenuItemButto final Message message; } -// This button is very temporary, to complete #125 before we have a way to -// choose an arbitrary reaction (#388). So, skipping i18n. -class AddThumbsUpButton extends MessageActionSheetMenuItemButton { - AddThumbsUpButton({super.key, required super.message, required super.pageContext}); +class ReactionButtons extends StatelessWidget { + const ReactionButtons({ + super.key, + required this.message, + required this.pageContext, + }); - @override IconData get icon => ZulipIcons.smile; + final Message message; - @override - String label(ZulipLocalizations zulipLocalizations) { - return 'React with 👍'; // TODO(i18n) skip translation for now + /// A context within the [MessageListPage] this action sheet was + /// triggered from. + final BuildContext pageContext; + + void _handleTapReaction({ + required EmojiCandidate emoji, + required bool isSelfVoted, + }) { + // 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); + doAddOrRemoveReaction( + context: pageContext, + doRemoveReaction: isSelfVoted, + messageId: message.id, + emoji: emoji, + errorDialogTitle: isSelfVoted + ? zulipLocalizations.errorReactionRemovingFailedTitle + : zulipLocalizations.errorReactionAddingFailedTitle); } - @override void onPressed() async { - String? errorMessage; - try { - await addReaction(PerAccountStoreWidget.of(pageContext).connection, - messageId: message.id, - reactionType: ReactionType.unicodeEmoji, - emojiCode: '1f44d', - emojiName: '+1', - ); - } catch (e) { - if (!pageContext.mounted) return; + Widget _buildButton({ + required BuildContext context, + required EmojiCandidate emoji, + required bool isSelfVoted, + required bool isFirst, + }) { + final designVariables = DesignVariables.of(context); + return Flexible(child: InkWell( + onTap: () => _handleTapReaction(emoji: emoji, isSelfVoted: isSelfVoted), + splashFactory: NoSplash.splashFactory, + borderRadius: isFirst + ? const BorderRadius.only(topLeft: Radius.circular(7)) + : null, + overlayColor: WidgetStateColor.resolveWith((states) => + states.any((e) => e == WidgetState.pressed) + ? designVariables.contextMenuItemBg.withFadedAlpha(0.20) + : Colors.transparent), + child: Container( + width: double.infinity, + padding: const EdgeInsets.symmetric(vertical: 12, horizontal: 5), + alignment: Alignment.center, + color: isSelfVoted + ? designVariables.contextMenuItemBg.withFadedAlpha(0.20) + : null, + child: UnicodeEmojiWidget( + emojiDisplay: emoji.emojiDisplay as UnicodeEmojiDisplay, + notoColorEmojiTextSize: 20.1, + size: 24)))); + } - switch (e) { - case ZulipApiException(): - errorMessage = e.message; - // TODO(#741) specific messages for common errors, like network errors - // (support with reusable code) - default: - } + @override + Widget build(BuildContext context) { + assert(EmojiStore.popularEmojiCandidates.every( + (emoji) => emoji.emojiType == ReactionType.unicodeEmoji)); - showErrorDialog(context: pageContext, - title: 'Adding reaction failed', message: errorMessage); + final store = PerAccountStoreWidget.of(pageContext); + 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( + spacing: 1, + children: List.unmodifiable( + EmojiStore.popularEmojiCandidates.mapIndexed((index, emoji) => + _buildButton( + context: context, + emoji: emoji, + isSelfVoted: hasSelfVote(emoji), + isFirst: index == 0))))); } } diff --git a/lib/widgets/emoji_reaction.dart b/lib/widgets/emoji_reaction.dart index 39264eb3b6..00f7e9b334 100644 --- a/lib/widgets/emoji_reaction.dart +++ b/lib/widgets/emoji_reaction.dart @@ -1,9 +1,11 @@ import 'package:flutter/material.dart'; +import '../api/exception.dart'; import '../api/model/model.dart'; import '../api/route/messages.dart'; import '../model/emoji.dart'; import 'color.dart'; +import 'dialog.dart'; import 'emoji.dart'; import 'store.dart'; import 'text.dart'; @@ -360,3 +362,42 @@ class _TextEmoji extends StatelessWidget { text); } } + +/// Adds or removes a reaction on the message corresponding to +/// the [messageId], showing an error dialog on failure. +/// Returns a Future resolving to true if operation succeeds. +Future doAddOrRemoveReaction({ + required BuildContext context, + required bool doRemoveReaction, + required int messageId, + required EmojiCandidate emoji, + required String errorDialogTitle, +}) async { + final store = PerAccountStoreWidget.of(context); + String? errorMessage; + try { + await (doRemoveReaction ? removeReaction : addReaction).call( + store.connection, + messageId: messageId, + reactionType: emoji.emojiType, + emojiCode: emoji.emojiCode, + emojiName: emoji.emojiName, + ); + } catch (e) { + if (!context.mounted) return; + + switch (e) { + case ZulipApiException(): + errorMessage = e.message; + // TODO(#741) specific messages for common errors, like network errors + // (support with reusable code) + default: + // TODO(log) + } + + showErrorDialog(context: context, + title: errorDialogTitle, + message: errorMessage); + return; + } +} diff --git a/test/flutter_checks.dart b/test/flutter_checks.dart index 9d81e8ea20..5e0fe370b7 100644 --- a/test/flutter_checks.dart +++ b/test/flutter_checks.dart @@ -158,3 +158,7 @@ extension TableRowChecks on Subject { extension TableChecks on Subject { Subject> get children => has((x) => x.children, 'children'); } + +extension IconButtonChecks on Subject { + Subject get isSelected => has((x) => x.isSelected, 'isSelected'); +} diff --git a/test/widgets/action_sheet_test.dart b/test/widgets/action_sheet_test.dart index b47a49dba9..aa27c53afb 100644 --- a/test/widgets/action_sheet_test.dart +++ b/test/widgets/action_sheet_test.dart @@ -11,13 +11,16 @@ import 'package:zulip/api/route/channels.dart'; import 'package:zulip/api/route/messages.dart'; import 'package:zulip/model/binding.dart'; import 'package:zulip/model/compose.dart'; +import 'package:zulip/model/emoji.dart'; import 'package:zulip/model/internal_link.dart'; import 'package:zulip/model/localizations.dart'; import 'package:zulip/model/narrow.dart'; import 'package:zulip/model/store.dart'; import 'package:zulip/model/typing_status.dart'; +import 'package:zulip/widgets/action_sheet.dart'; import 'package:zulip/widgets/compose_box.dart'; import 'package:zulip/widgets/content.dart'; +import 'package:zulip/widgets/emoji.dart'; import 'package:zulip/widgets/icons.dart'; import 'package:zulip/widgets/message_list.dart'; import 'package:share_plus_platform_interface/method_channel/method_channel_share.dart'; @@ -26,6 +29,7 @@ import '../api/fake_api.dart'; import '../example_data.dart' as eg; import '../flutter_checks.dart'; import '../model/binding.dart'; +import '../model/emoji_test.dart'; import '../model/test_store.dart'; import '../stdlib_checks.dart'; import '../test_clipboard.dart'; @@ -99,46 +103,101 @@ void main() { connection.prepare(httpStatus: 400, json: fakeResponseJson); } - group('AddThumbsUpButton', () { - Future tapButton(WidgetTester tester) async { - await tester.ensureVisible(find.byIcon(ZulipIcons.smile, skipOffstage: false)); - await tester.tap(find.byIcon(ZulipIcons.smile)); - await tester.pump(); // [MenuItemButton.onPressed] called in a post-frame callback: flutter/flutter@e4a39fa2e - } + group('ReactionButtons', () { + final popularCandidates = EmojiStore.popularEmojiCandidates; - testWidgets('success', (tester) async { - final message = eg.streamMessage(); - await setupToMessageActionSheet(tester, message: message, narrow: TopicNarrow.ofMessage(message)); + group('popular emoji reactions;', () { + testWidgets('ensure all are shown', (tester) async { + final message = eg.streamMessage(); + await setupToMessageActionSheet(tester, message: message, narrow: TopicNarrow.ofMessage(message)); - connection.prepare(json: {}); - await tapButton(tester); - await tester.pump(Duration.zero); + // Ensure all popular emoji buttons are shown. + final emojis = tester.widgetList(find.descendant( + of: find.descendant( + of: find.byType(ReactionButtons), + matching: find.byType(InkWell)), + matching: find.byType(UnicodeEmojiWidget))); + check(emojis).deepEquals(popularCandidates.map>((emoji) { + final emojiDisplay = emoji.emojiDisplay as UnicodeEmojiDisplay; + return (it) => it.isA() + ..emojiDisplay.which((it) => it + ..emojiName.equals(emojiDisplay.emojiName) + ..emojiUnicode.equals(emojiDisplay.emojiUnicode)); + })); + }); - check(connection.lastRequest).isA() - ..method.equals('POST') - ..url.path.equals('/api/v1/messages/${message.id}/reactions') - ..bodyFields.deepEquals({ - 'reaction_type': 'unicode_emoji', - 'emoji_code': '1f44d', - 'emoji_name': '+1', - }); - }); + for (final emoji in popularCandidates) { + final emojiDisplay = emoji.emojiDisplay as UnicodeEmojiDisplay; + + Future tapButton(WidgetTester tester) async { + await tester.tap(find.descendant( + of: find.descendant( + of: find.descendant( + of: find.byType(ReactionButtons), + matching: find.byType(InkWell)), + matching: find.byType(UnicodeEmojiWidget)), + matching: find.text(emojiDisplay.emojiUnicode))); + } + + testWidgets('${emoji.emojiName} adding success', (tester) async { + final message = eg.streamMessage(); + await setupToMessageActionSheet(tester, message: message, narrow: TopicNarrow.ofMessage(message)); + + connection.prepare(json: {}); + await tapButton(tester); + await tester.pump(Duration.zero); + + check(connection.lastRequest).isA() + ..method.equals('POST') + ..url.path.equals('/api/v1/messages/${message.id}/reactions') + ..bodyFields.deepEquals({ + 'reaction_type': 'unicode_emoji', + 'emoji_code': emoji.emojiCode, + 'emoji_name': emoji.emojiName, + }); + }); - testWidgets('request has an error', (tester) async { - final message = eg.streamMessage(); - await setupToMessageActionSheet(tester, message: message, narrow: TopicNarrow.ofMessage(message)); + testWidgets('${emoji.emojiName} removing success', (tester) async { + final message = eg.streamMessage( + reactions: [Reaction( + emojiName: emoji.emojiName, + emojiCode: emoji.emojiCode, + reactionType: ReactionType.unicodeEmoji, + userId: eg.selfAccount.userId)] + ); + await setupToMessageActionSheet(tester, message: message, narrow: TopicNarrow.ofMessage(message)); + + connection.prepare(json: {}); + await tapButton(tester); + await tester.pump(Duration.zero); + + check(connection.lastRequest).isA() + ..method.equals('DELETE') + ..url.path.equals('/api/v1/messages/${message.id}/reactions') + ..bodyFields.deepEquals({ + 'reaction_type': 'unicode_emoji', + 'emoji_code': emoji.emojiCode, + 'emoji_name': emoji.emojiName, + }); + }); - connection.prepare(httpStatus: 400, json: { - 'code': 'BAD_REQUEST', - 'msg': 'Invalid message(s)', - 'result': 'error', - }); - await tapButton(tester); - await tester.pump(Duration.zero); // error arrives; error dialog shows + testWidgets('${emoji.emojiName} request has an error', (tester) async { + final message = eg.streamMessage(); + await setupToMessageActionSheet(tester, message: message, narrow: TopicNarrow.ofMessage(message)); - await tester.tap(find.byWidget(checkErrorDialog(tester, - expectedTitle: 'Adding reaction failed', - expectedMessage: 'Invalid message(s)'))); + connection.prepare(httpStatus: 400, json: { + 'code': 'BAD_REQUEST', + 'msg': 'Invalid message(s)', + 'result': 'error', + }); + await tapButton(tester); + await tester.pump(Duration.zero); // error arrives; error dialog shows + + await tester.tap(find.byWidget(checkErrorDialog(tester, + expectedTitle: 'Adding reaction failed', + expectedMessage: 'Invalid message(s)'))); + }); + } }); }); @@ -700,3 +759,7 @@ void main() { }); }); } + +extension UnicodeEmojiWidgetChecks on Subject { + Subject get emojiDisplay => has((x) => x.emojiDisplay, 'emojiDisplay'); +}