From 870bae64c61cd887843b2fcce8f8735783cd7e1b Mon Sep 17 00:00:00 2001 From: chimnayajith Date: Wed, 19 Feb 2025 21:02:03 +0530 Subject: [PATCH 1/2] actions: Add specific handling for ZulipApiException --- lib/widgets/actions.dart | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/lib/widgets/actions.dart b/lib/widgets/actions.dart index d49ce690d0..e39617ac33 100644 --- a/lib/widgets/actions.dart +++ b/lib/widgets/actions.dart @@ -2,6 +2,7 @@ import 'dart:async'; import 'package:flutter/material.dart'; +import '../api/exception.dart'; import '../api/model/model.dart'; import '../api/model/narrow.dart'; import '../api/route/messages.dart'; @@ -32,9 +33,13 @@ abstract final class ZulipAction { return; } catch (e) { if (!context.mounted) return; + final message = switch (e) { + ZulipApiException() => zulipLocalizations.errorServerMessage(e.message), + _ => e.toString(), // TODO(#741): extract user-facing message better + }; showErrorDialog(context: context, title: zulipLocalizations.errorMarkAsReadFailedTitle, - message: e.toString()); // TODO(#741): extract user-facing message better + message: message); return; } } @@ -189,9 +194,14 @@ abstract final class ZulipAction { } } catch (e) { if (!context.mounted) return false; + final zulipLocalizations = ZulipLocalizations.of(context); + final message = switch (e) { + ZulipApiException() => zulipLocalizations.errorServerMessage(e.message), + _ => e.toString(), // TODO(#741): extract user-facing message better + }; showErrorDialog(context: context, title: onFailedTitle, - message: e.toString()); // TODO(#741): extract user-facing message better + message: message); return false; } } From 191a571d08c6ef18fd7f49dfa6cb36ae98ef99d6 Mon Sep 17 00:00:00 2001 From: chimnayajith Date: Wed, 19 Feb 2025 21:02:16 +0530 Subject: [PATCH 2/2] action_sheet: Add channel action sheet with mark as read option Fixes: #1226 --- assets/l10n/app_en.arb | 4 + lib/generated/l10n/zulip_localizations.dart | 6 + .../l10n/zulip_localizations_ar.dart | 3 + .../l10n/zulip_localizations_en.dart | 3 + .../l10n/zulip_localizations_ja.dart | 3 + .../l10n/zulip_localizations_nb.dart | 3 + .../l10n/zulip_localizations_pl.dart | 3 + .../l10n/zulip_localizations_ru.dart | 3 + .../l10n/zulip_localizations_sk.dart | 3 + lib/widgets/action_sheet.dart | 51 +++++ lib/widgets/inbox.dart | 16 +- lib/widgets/message_list.dart | 67 ++++--- lib/widgets/subscription_list.dart | 2 + test/widgets/action_sheet_test.dart | 185 ++++++++++++++++++ 14 files changed, 327 insertions(+), 25 deletions(-) diff --git a/assets/l10n/app_en.arb b/assets/l10n/app_en.arb index b0c9404bc8..f453ef1adc 100644 --- a/assets/l10n/app_en.arb +++ b/assets/l10n/app_en.arb @@ -76,6 +76,10 @@ "@permissionsDeniedReadExternalStorage": { "description": "Message for dialog asking the user to grant permissions for external storage read access." }, + "actionSheetOptionMarkChannelAsRead": "Mark channel as read", + "@actionSheetOptionMarkChannelAsRead": { + "description": "Label for marking a channel as read." + }, "actionSheetOptionMuteTopic": "Mute topic", "@actionSheetOptionMuteTopic": { "description": "Label for muting a topic on action sheet." diff --git a/lib/generated/l10n/zulip_localizations.dart b/lib/generated/l10n/zulip_localizations.dart index 38fa4c6a1d..123fe10847 100644 --- a/lib/generated/l10n/zulip_localizations.dart +++ b/lib/generated/l10n/zulip_localizations.dart @@ -219,6 +219,12 @@ abstract class ZulipLocalizations { /// **'To upload files, please grant Zulip additional permissions in Settings.'** String get permissionsDeniedReadExternalStorage; + /// Label for marking a channel as read. + /// + /// In en, this message translates to: + /// **'Mark channel as read'** + String get actionSheetOptionMarkChannelAsRead; + /// Label for muting a topic on action sheet. /// /// In en, this message translates to: diff --git a/lib/generated/l10n/zulip_localizations_ar.dart b/lib/generated/l10n/zulip_localizations_ar.dart index 19967cf591..fc1b2dea93 100644 --- a/lib/generated/l10n/zulip_localizations_ar.dart +++ b/lib/generated/l10n/zulip_localizations_ar.dart @@ -67,6 +67,9 @@ class ZulipLocalizationsAr extends ZulipLocalizations { @override String get permissionsDeniedReadExternalStorage => 'To upload files, please grant Zulip additional permissions in Settings.'; + @override + String get actionSheetOptionMarkChannelAsRead => 'Mark channel as read'; + @override String get actionSheetOptionMuteTopic => 'Mute topic'; diff --git a/lib/generated/l10n/zulip_localizations_en.dart b/lib/generated/l10n/zulip_localizations_en.dart index 9e34bdd7fe..d83387b2b8 100644 --- a/lib/generated/l10n/zulip_localizations_en.dart +++ b/lib/generated/l10n/zulip_localizations_en.dart @@ -67,6 +67,9 @@ class ZulipLocalizationsEn extends ZulipLocalizations { @override String get permissionsDeniedReadExternalStorage => 'To upload files, please grant Zulip additional permissions in Settings.'; + @override + String get actionSheetOptionMarkChannelAsRead => 'Mark channel as read'; + @override String get actionSheetOptionMuteTopic => 'Mute topic'; diff --git a/lib/generated/l10n/zulip_localizations_ja.dart b/lib/generated/l10n/zulip_localizations_ja.dart index d9d90a8ad0..39e230f859 100644 --- a/lib/generated/l10n/zulip_localizations_ja.dart +++ b/lib/generated/l10n/zulip_localizations_ja.dart @@ -67,6 +67,9 @@ class ZulipLocalizationsJa extends ZulipLocalizations { @override String get permissionsDeniedReadExternalStorage => 'To upload files, please grant Zulip additional permissions in Settings.'; + @override + String get actionSheetOptionMarkChannelAsRead => 'Mark channel as read'; + @override String get actionSheetOptionMuteTopic => 'Mute topic'; diff --git a/lib/generated/l10n/zulip_localizations_nb.dart b/lib/generated/l10n/zulip_localizations_nb.dart index 28cb2769e8..bc6796667f 100644 --- a/lib/generated/l10n/zulip_localizations_nb.dart +++ b/lib/generated/l10n/zulip_localizations_nb.dart @@ -67,6 +67,9 @@ class ZulipLocalizationsNb extends ZulipLocalizations { @override String get permissionsDeniedReadExternalStorage => 'To upload files, please grant Zulip additional permissions in Settings.'; + @override + String get actionSheetOptionMarkChannelAsRead => 'Mark channel as read'; + @override String get actionSheetOptionMuteTopic => 'Mute topic'; diff --git a/lib/generated/l10n/zulip_localizations_pl.dart b/lib/generated/l10n/zulip_localizations_pl.dart index 2eaadcd704..45957536e8 100644 --- a/lib/generated/l10n/zulip_localizations_pl.dart +++ b/lib/generated/l10n/zulip_localizations_pl.dart @@ -67,6 +67,9 @@ class ZulipLocalizationsPl extends ZulipLocalizations { @override String get permissionsDeniedReadExternalStorage => 'Aby odebrać pliki Zulip musi uzyskać dodatkowe uprawnienia w Ustawieniach.'; + @override + String get actionSheetOptionMarkChannelAsRead => 'Mark channel as read'; + @override String get actionSheetOptionMuteTopic => 'Wycisz wątek'; diff --git a/lib/generated/l10n/zulip_localizations_ru.dart b/lib/generated/l10n/zulip_localizations_ru.dart index 061c3f29f3..37cff93bbd 100644 --- a/lib/generated/l10n/zulip_localizations_ru.dart +++ b/lib/generated/l10n/zulip_localizations_ru.dart @@ -67,6 +67,9 @@ class ZulipLocalizationsRu extends ZulipLocalizations { @override String get permissionsDeniedReadExternalStorage => 'Для загрузки файлов, пожалуйста, предоставьте Zulip дополнительные разрешения в настройках.'; + @override + String get actionSheetOptionMarkChannelAsRead => 'Mark channel as read'; + @override String get actionSheetOptionMuteTopic => 'Отключить тему'; diff --git a/lib/generated/l10n/zulip_localizations_sk.dart b/lib/generated/l10n/zulip_localizations_sk.dart index 7378638f4a..1bcd7578c1 100644 --- a/lib/generated/l10n/zulip_localizations_sk.dart +++ b/lib/generated/l10n/zulip_localizations_sk.dart @@ -67,6 +67,9 @@ class ZulipLocalizationsSk extends ZulipLocalizations { @override String get permissionsDeniedReadExternalStorage => 'To upload files, please grant Zulip additional permissions in Settings.'; + @override + String get actionSheetOptionMarkChannelAsRead => 'Mark channel as read'; + @override String get actionSheetOptionMuteTopic => 'Stlmiť tému'; diff --git a/lib/widgets/action_sheet.dart b/lib/widgets/action_sheet.dart index c5068384f1..1d3cbad495 100644 --- a/lib/widgets/action_sheet.dart +++ b/lib/widgets/action_sheet.dart @@ -163,6 +163,57 @@ class ActionSheetCancelButton extends StatelessWidget { } } +/// Show a sheet of actions you can take on a channel. +/// +/// Needs a [PageRoot] ancestor. +void showChannelActionSheet(BuildContext context, { + required int channelId, +}) { + final pageContext = PageRoot.contextOf(context); + final store = PerAccountStoreWidget.of(pageContext); + + final optionButtons = []; + final unreadCount = store.unreads.countInChannelNarrow(channelId); + if (unreadCount > 0) { + optionButtons.add( + MarkChannelAsReadButton(pageContext: pageContext, channelId: channelId)); + } + if (optionButtons.isEmpty) { + // TODO(a11y): This case makes a no-op gesture handler; as a consequence, + // we're presenting some UI (to people who use screen-reader software) as + // though it offers a gesture interaction that it doesn't meaningfully + // offer, which is confusing. The solution here is probably to remove this + // is-empty case by having at least one button that's always present, + // such as "copy link to channel". + return; + } + _showActionSheet(pageContext, optionButtons: optionButtons); +} + +class MarkChannelAsReadButton extends ActionSheetMenuItemButton { + const MarkChannelAsReadButton({ + super.key, + required this.channelId, + required super.pageContext, + }); + + final int channelId; + + @override + IconData get icon => ZulipIcons.message_checked; + + @override + String label(ZulipLocalizations zulipLocalizations) { + return zulipLocalizations.actionSheetOptionMarkChannelAsRead; + } + + @override + void onPressed() async { + final narrow = ChannelNarrow(channelId); + await ZulipAction.markNarrowAsRead(pageContext, narrow); + } +} + /// Show a sheet of actions you can take on a topic. /// /// Needs a [PageRoot] ancestor. diff --git a/lib/widgets/inbox.dart b/lib/widgets/inbox.dart index 81133cde20..a8c0c12b59 100644 --- a/lib/widgets/inbox.dart +++ b/lib/widgets/inbox.dart @@ -272,6 +272,9 @@ abstract class _HeaderItem extends StatelessWidget { // But that's in tension with the Figma, which gives these header rows // 40px min height. onTap: onCollapseButtonTap, + onLongPress: this is _LongPressable + ? (this as _LongPressable).onLongPress + : null, child: Row(crossAxisAlignment: CrossAxisAlignment.center, children: [ Padding(padding: const EdgeInsets.all(10), child: Icon(size: 20, color: designVariables.sectionCollapseIcon, @@ -425,7 +428,13 @@ class _DmItem extends StatelessWidget { } } -class _StreamHeaderItem extends _HeaderItem { +mixin _LongPressable on _HeaderItem { + // TODO(#1272) move to _HeaderItem base class + // when DM headers become long-pressable; remove mixin + Future onLongPress(); +} + +class _StreamHeaderItem extends _HeaderItem with _LongPressable { final Subscription subscription; const _StreamHeaderItem({ @@ -458,6 +467,11 @@ class _StreamHeaderItem extends _HeaderItem { } } @override Future onRowTap() => onCollapseButtonTap(); // TODO open channel narrow + + @override + Future onLongPress() async { + showChannelActionSheet(sectionContext, channelId: subscription.streamId); + } } class _StreamSection extends StatelessWidget { diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 681d9cae61..7a95d10177 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -370,36 +370,54 @@ class MessageListAppBarTitle extends StatelessWidget { case ChannelNarrow(:var streamId): final store = PerAccountStoreWidget.of(context); final stream = store.streams[streamId]; - return _buildStreamRow(context, stream: stream); - - case TopicNarrow(:var streamId, :var topic): - final store = PerAccountStoreWidget.of(context); - final stream = store.streams[streamId]; + final alignment = willCenterTitle + ? Alignment.center + : AlignmentDirectional.centerStart; return SizedBox( width: double.infinity, child: GestureDetector( behavior: HitTestBehavior.translucent, onLongPress: () { - final someMessage = MessageListPage.ancestorOf(context) - .model?.messages.firstOrNull; - // If someMessage is null, the topic action sheet won't have a - // resolve/unresolve button. That seems OK; in that case we're - // either still fetching messages (and the user can reopen the - // sheet after that finishes) or there aren't any messages to - // act on anyway. - assert(someMessage == null || narrow.containsMessage(someMessage)); - showTopicActionSheet(context, - channelId: streamId, - topic: topic, - someMessageIdInTopic: someMessage?.id); + showChannelActionSheet(context, channelId: streamId); }, - child: Column( - crossAxisAlignment: willCenterTitle ? CrossAxisAlignment.center - : CrossAxisAlignment.start, - children: [ - _buildStreamRow(context, stream: stream), - _buildTopicRow(context, stream: stream, topic: topic), - ]))); + child: Align(alignment: alignment, + child: _buildStreamRow(context, stream: stream)))); + + case TopicNarrow(:var streamId, :var topic): + final store = PerAccountStoreWidget.of(context); + final stream = store.streams[streamId]; + final alignment = willCenterTitle + ? Alignment.center + : AlignmentDirectional.centerStart; + return Column( + crossAxisAlignment: CrossAxisAlignment.stretch, + children: [ + GestureDetector( + behavior: HitTestBehavior.translucent, + onLongPress: () { + showChannelActionSheet(context, channelId: streamId); + }, + child: Align(alignment: alignment, + child: _buildStreamRow(context, stream: stream))), + GestureDetector( + behavior: HitTestBehavior.translucent, + onLongPress: () { + final someMessage = MessageListPage.ancestorOf(context) + .model?.messages.firstOrNull; + // If someMessage is null, the topic action sheet won't have a + // resolve/unresolve button. That seems OK; in that case we're + // either still fetching messages (and the user can reopen the + // sheet after that finishes) or there aren't any messages to + // act on anyway. + assert(someMessage == null || narrow.containsMessage(someMessage)); + showTopicActionSheet(context, + channelId: streamId, + topic: topic, + someMessageIdInTopic: someMessage?.id); + }, + child: Align(alignment: alignment, + child: _buildTopicRow(context, stream: stream, topic: topic))), + ]); case DmNarrow(:var otherRecipientIds): final store = PerAccountStoreWidget.of(context); @@ -1061,6 +1079,7 @@ class StreamMessageRecipientHeader extends StatelessWidget { onTap: () => Navigator.push(context, MessageListPage.buildRoute(context: context, narrow: ChannelNarrow(message.streamId))), + onLongPress: () => showChannelActionSheet(context, channelId: message.streamId), child: Row( crossAxisAlignment: CrossAxisAlignment.center, children: [ diff --git a/lib/widgets/subscription_list.dart b/lib/widgets/subscription_list.dart index a51e722b51..062bb9743e 100644 --- a/lib/widgets/subscription_list.dart +++ b/lib/widgets/subscription_list.dart @@ -4,6 +4,7 @@ import '../api/model/model.dart'; import '../generated/l10n/zulip_localizations.dart'; import '../model/narrow.dart'; import '../model/unreads.dart'; +import 'action_sheet.dart'; import 'icons.dart'; import 'message_list.dart'; import 'store.dart'; @@ -230,6 +231,7 @@ class SubscriptionItem extends StatelessWidget { MessageListPage.buildRoute(context: context, narrow: ChannelNarrow(subscription.streamId))); }, + onLongPress: () => showChannelActionSheet(context, channelId: subscription.streamId), child: Row(crossAxisAlignment: CrossAxisAlignment.center, children: [ const SizedBox(width: 16), Padding( diff --git a/test/widgets/action_sheet_test.dart b/test/widgets/action_sheet_test.dart index 94ddd64cc3..dc611d3ca9 100644 --- a/test/widgets/action_sheet_test.dart +++ b/test/widgets/action_sheet_test.dart @@ -31,6 +31,7 @@ import 'package:zulip/widgets/icons.dart'; import 'package:zulip/widgets/inbox.dart'; import 'package:zulip/widgets/message_list.dart'; import 'package:share_plus_platform_interface/method_channel/method_channel_share.dart'; +import 'package:zulip/widgets/subscription_list.dart'; import '../api/fake_api.dart'; import '../example_data.dart' as eg; @@ -104,6 +105,190 @@ void main() { connection.prepare(apiException: eg.apiBadRequest(message: 'Invalid message(s)')); } + group('channel action sheet', () { + late ZulipStream someChannel; + const someTopic = 'my topic'; + late StreamMessage someMessage; + + Future prepare({bool hasUnreadMessages = true}) async { + someChannel = eg.stream(); + someMessage = eg.streamMessage( + stream: someChannel, topic: someTopic, sender: eg.otherUser, + flags: hasUnreadMessages ? [] : [MessageFlag.read]); + addTearDown(testBinding.reset); + + await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot()); + store = await testBinding.globalStore.perAccount(eg.selfAccount.id); + connection = store.connection as FakeApiConnection; + + await store.addUser(eg.selfUser); + await store.addUser(eg.otherUser); + await store.addStream(someChannel); + await store.addSubscription(eg.subscription(someChannel)); + await store.addMessage(someMessage); + } + + Future showFromInbox(WidgetTester tester) async { + await tester.pumpWidget(TestZulipApp(accountId: eg.selfAccount.id, + child: const HomePage())); + await tester.pump(); + check(find.byType(InboxPageBody)).findsOne(); + + await tester.longPress(find.text(someChannel.name).hitTestable()); + await tester.pump(const Duration(milliseconds: 250)); + } + + Future showFromSubscriptionList(WidgetTester tester) async { + await tester.pumpWidget(TestZulipApp(accountId: eg.selfAccount.id, + child: const HomePage())); + await tester.pump(); + await tester.tap(find.byIcon(ZulipIcons.hash_italic)); + await tester.pump(); + check(find.byType(SubscriptionListPageBody)).findsOne(); + + await tester.longPress(find.text(someChannel.name).hitTestable()); + await tester.pump(const Duration(milliseconds: 250)); + } + + Future showFromAppBar(WidgetTester tester, { + ZulipStream? channel, + List? messages, + required Narrow narrow, + }) async { + channel ??= someChannel; + messages ??= [someMessage]; + + connection.prepare(json: eg.newestGetMessagesResult( + foundOldest: true, messages: messages).toJson()); + await tester.pumpWidget(TestZulipApp( + accountId: eg.selfAccount.id, + child: MessageListPage( + initNarrow: narrow))); + await tester.pumpAndSettle(); + + await tester.longPress(find.descendant( + of: find.byType(ZulipAppBar), + matching: find.text(channel.name))); + await tester.pump(const Duration(milliseconds: 250)); + } + + Future showFromRecipientHeader(WidgetTester tester, { + StreamMessage? message, + }) async { + message ??= someMessage; + + connection.prepare(json: eg.newestGetMessagesResult( + foundOldest: true, messages: [message]).toJson()); + await tester.pumpWidget(TestZulipApp(accountId: eg.selfAccount.id, + child: const MessageListPage(initNarrow: CombinedFeedNarrow()))); + await tester.pumpAndSettle(); + + await tester.longPress(find.descendant( + of: find.byType(RecipientHeader), + matching: find.text(message.displayRecipient ?? ''))); + await tester.pump(const Duration(milliseconds: 250)); + } + + final actionSheetFinder = find.byType(BottomSheet); + Finder findButtonForLabel(String label) => + find.descendant(of: actionSheetFinder, matching: find.text(label)); + + void checkButton(String label) { + check(findButtonForLabel(label)).findsOne(); + } + + group('showChannelActionSheet', () { + void checkButtons() { + check(actionSheetFinder).findsOne(); + checkButton('Mark channel as read'); + } + + testWidgets('show from inbox', (tester) async { + await prepare(); + await showFromInbox(tester); + checkButtons(); + }); + + testWidgets('show from subscription list', (tester) async { + await prepare(); + await showFromSubscriptionList(tester); + checkButtons(); + }); + + testWidgets('show with no unread messages', (tester) async { + await prepare(hasUnreadMessages: false); + await showFromSubscriptionList(tester); + check(actionSheetFinder).findsNothing(); + }); + + testWidgets('show from app bar in channel narrow', (tester) async { + await prepare(); + final narrow = ChannelNarrow(someChannel.streamId); + await showFromAppBar(tester, narrow: narrow); + checkButtons(); + }); + + testWidgets('show from app bar in topic narrow', (tester) async { + await prepare(); + final narrow = eg.topicNarrow(someChannel.streamId, someTopic); + await showFromAppBar(tester, narrow: narrow); + checkButtons(); + }); + + testWidgets('show from recipient header', (tester) async { + await prepare(); + await showFromRecipientHeader(tester, message: someMessage); + checkButtons(); + }); + }); + + group('MarkChannelAsReadButton', () { + void checkRequest(int channelId) { + check(connection.takeRequests()).single.isA() + ..method.equals('POST') + ..url.path.equals('/api/v1/messages/flags/narrow') + ..bodyFields.deepEquals({ + 'anchor': 'oldest', + 'include_anchor': 'false', + 'num_before': '0', + 'num_after': '1000', + 'narrow': jsonEncode([ + {'operator': 'stream', 'operand': channelId}, + {'operator': 'is', 'operand': 'unread'}, + ]), + 'op': 'add', + 'flag': 'read', + }); + } + + testWidgets('happy path from inbox', (tester) async { + await prepare(); + final message = eg.streamMessage(stream: someChannel, topic: someTopic); + await store.addMessage(message); + await showFromInbox(tester); + connection.prepare(json: UpdateMessageFlagsForNarrowResult( + processedCount: 1, updatedCount: 1, + firstProcessedId: message.id, lastProcessedId: message.id, + foundOldest: true, foundNewest: true).toJson()); + await tester.tap(findButtonForLabel('Mark channel as read')); + await tester.pumpAndSettle(); + checkRequest(someChannel.streamId); + checkNoErrorDialog(tester); + }); + + testWidgets('request fails', (tester) async { + await prepare(); + await showFromInbox(tester); + connection.prepare(httpException: http.ClientException('Oops')); + await tester.tap(findButtonForLabel('Mark channel as read')); + await tester.pumpAndSettle(); + checkRequest(someChannel.streamId); + checkErrorDialog(tester, + expectedTitle: "Mark as read failed"); + }); + }); + }); + group('topic action sheet', () { final someChannel = eg.stream(); const someTopic = 'my topic';