From a2dc3eff40052ca4e918f42e89f6837fc2f7384f Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Thu, 15 Aug 2024 19:49:05 -0400 Subject: [PATCH 1/5] message [nfc]: Mention that @-mention live-updates have not yet been handled. Signed-off-by: Zixuan James Li --- lib/model/message.dart | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/model/message.dart b/lib/model/message.dart index a3094941f0..36a03525c2 100644 --- a/lib/model/message.dart +++ b/lib/model/message.dart @@ -270,6 +270,8 @@ class MessageStoreImpl with MessageStore { if (anyMessageFound) { for (final view in _messageListViews) { view.notifyListenersIfAnyMessagePresent(event.messages); + // TODO(#818): Support MentionsNarrow live-updates when handling + // @-mention flags. } } } From 115a5e5d1afb907f4acdaacb6acb4d8f026a63ea Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Tue, 20 Aug 2024 19:38:51 -0400 Subject: [PATCH 2/5] action_sheet test: Create message with the appropriate flag. Otherwise this message wouldn't be expected to appear in the messages fetched for MentionsNarrow. Signed-off-by: Zixuan James Li --- test/widgets/action_sheet_test.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/widgets/action_sheet_test.dart b/test/widgets/action_sheet_test.dart index f2a6403edf..945bd15a44 100644 --- a/test/widgets/action_sheet_test.dart +++ b/test/widgets/action_sheet_test.dart @@ -388,7 +388,7 @@ void main() { }); testWidgets('not offered in MentionsNarrow (composing to reply is not yet supported)', (tester) async { - final message = eg.streamMessage(); + final message = eg.streamMessage(flags: [MessageFlag.mentioned]); await setupToMessageActionSheet(tester, message: message, narrow: const MentionsNarrow()); check(findQuoteAndReplyButton(tester)).isNull(); }); From fe5dc29ebeeb9fc48780200db317bce80671ecee Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Tue, 20 Aug 2024 23:52:17 -0400 Subject: [PATCH 3/5] msglist test: Remove redundant copy of a regression test. A test on CombinedFeedNarrow is sufficient for preventing this particular regression. We could go further and test this on other narrows, but there's little to gain because the bug related to the absence of a compose box, not any other detail of the narrow. Signed-off-by: Zixuan James Li --- test/widgets/message_list_test.dart | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index bfe26444de..6f560b8192 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -135,19 +135,6 @@ void main() { final padding = MediaQuery.of(element).padding; check(padding).equals(EdgeInsets.zero); }); - - testWidgets('content in MentionsNarrow not asked to consume insets (including bottom)', (tester) async { - const fakePadding = FakeViewPadding(left: 10, top: 10, right: 10, bottom: 10); - tester.view.viewInsets = fakePadding; - tester.view.padding = fakePadding; - - await setupMessageListPage(tester, narrow: const MentionsNarrow(), - messages: [eg.streamMessage(content: ContentExample.codeBlockPlain.html, flags: [MessageFlag.mentioned])]); - - final element = tester.element(find.byType(CodeBlock)); - final padding = MediaQuery.of(element).padding; - check(padding).equals(EdgeInsets.zero); - }); }); testWidgets('smoke test for light/dark/lerped', (tester) async { From 5bcb5d4cedc815ef7d2497c6d01d1afc5c51ca3e Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 21 Aug 2024 15:02:51 -0700 Subject: [PATCH 4/5] msglist test [nfc]: Pin down lack of compose box in no-insets regression test This way if some future change causes CombinedFeedNarrow to have a compose box, we'll be prompted to update this test, so that it still checks for this bug if there are any other narrows that still lack a compose box. --- test/widgets/message_list_test.dart | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index 6f560b8192..8e2a05d49b 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -122,8 +122,8 @@ void main() { }); group('presents message content appropriately', () { - // regression test for https://github.com/zulip/zulip-flutter/issues/736 - testWidgets('content in "Combined feed" not asked to consume insets (including bottom)', (tester) async { + testWidgets('content not asked to consume insets (including bottom), even without compose box', (tester) async { + // Regression test for: https://github.com/zulip/zulip-flutter/issues/736 const fakePadding = FakeViewPadding(left: 10, top: 10, right: 10, bottom: 10); tester.view.viewInsets = fakePadding; tester.view.padding = fakePadding; @@ -131,6 +131,11 @@ void main() { await setupMessageListPage(tester, narrow: const CombinedFeedNarrow(), messages: [eg.streamMessage(content: ContentExample.codeBlockPlain.html)]); + // Verify this message list lacks a compose box. + // (The original bug wouldn't reproduce with a compose box present.) + final state = MessageListPage.ancestorOf(tester.element(find.text("verb\natim"))); + check(state.composeBoxController).isNull(); + final element = tester.element(find.byType(CodeBlock)); final padding = MediaQuery.of(element).padding; check(padding).equals(EdgeInsets.zero); From 9daa9404892a13780a98c5c827d91b5ab93d1aec Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Wed, 7 Aug 2024 02:19:49 -0400 Subject: [PATCH 5/5] narrow: Add starred messages narrow. "Starred messages narrow" is the user facing name of the narrow, thus we name the Narrow class after it. This narrow is pretty similar to MentionsNarrow. Signed-off-by: Zixuan James Li --- assets/l10n/app_en.arb | 4 +++ lib/model/autocomplete.dart | 1 + lib/model/internal_link.dart | 3 ++- lib/model/message.dart | 6 +++++ lib/model/message_list.dart | 4 +++ lib/model/narrow.dart | 22 ++++++++++++++++ lib/model/unreads.dart | 5 ++++ lib/widgets/actions.dart | 3 +++ lib/widgets/app.dart | 6 +++++ lib/widgets/compose_box.dart | 2 ++ lib/widgets/message_list.dart | 5 ++++ test/api/route/messages_test.dart | 3 +++ test/model/autocomplete_test.dart | 7 ++++++ test/model/compose_test.dart | 8 ++++++ test/model/internal_link_test.dart | 3 ++- test/model/message_list_test.dart | 39 +++++++++++++++++++++++++++++ test/model/narrow_test.dart | 11 ++++++++ test/model/unreads_test.dart | 11 ++++++++ test/widgets/action_sheet_test.dart | 6 +++++ test/widgets/message_list_test.dart | 9 +++++++ 20 files changed, 156 insertions(+), 2 deletions(-) diff --git a/assets/l10n/app_en.arb b/assets/l10n/app_en.arb index 26cd786315..7c8d4981c7 100644 --- a/assets/l10n/app_en.arb +++ b/assets/l10n/app_en.arb @@ -499,6 +499,10 @@ "@mentionsPageTitle": { "description": "Title for the page of @-mentions." }, + "starredMessagesPageTitle": "Starred messages", + "@starredMessagesPageTitle": { + "description": "Title for the page of starred messages." + }, "notifGroupDmConversationLabel": "{senderFullName} to you and {numOthers, plural, =1{1 other} other{{numOthers} others}}", "@notifGroupDmConversationLabel": { "description": "Label for a group DM conversation notification.", diff --git a/lib/model/autocomplete.dart b/lib/model/autocomplete.dart index a227728bae..82186e6d8a 100644 --- a/lib/model/autocomplete.dart +++ b/lib/model/autocomplete.dart @@ -364,6 +364,7 @@ class MentionAutocompleteView extends AutocompleteView _compareByRelevance(userA, userB, diff --git a/lib/model/internal_link.dart b/lib/model/internal_link.dart index 0b9b597816..c132eb0b66 100644 --- a/lib/model/internal_link.dart +++ b/lib/model/internal_link.dart @@ -199,10 +199,11 @@ Narrow? _interpretNarrowSegments(List segments, PerAccountStore store) { switch (isElementOperands.single) { case IsOperand.mentioned: return const MentionsNarrow(); + case IsOperand.starred: + return const StarredMessagesNarrow(); case IsOperand.dm: case IsOperand.private: case IsOperand.alerted: - case IsOperand.starred: case IsOperand.followed: case IsOperand.resolved: case IsOperand.unread: diff --git a/lib/model/message.dart b/lib/model/message.dart index 36a03525c2..2f08519bf5 100644 --- a/lib/model/message.dart +++ b/lib/model/message.dart @@ -272,6 +272,12 @@ class MessageStoreImpl with MessageStore { view.notifyListenersIfAnyMessagePresent(event.messages); // TODO(#818): Support MentionsNarrow live-updates when handling // @-mention flags. + + // To make it easier to re-star a message, we opt-out from supporting + // live-updates when starred flag is removed. + // + // TODO: Support StarredMessagesNarrow live-updates when starred flag + // is added. } } } diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index 95d660d4f5..add55b060d 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -418,6 +418,7 @@ class MessageListView with ChangeNotifier, _MessageSequence { case TopicNarrow(): case DmNarrow(): case MentionsNarrow(): + case StarredMessagesNarrow(): return true; } } @@ -436,6 +437,7 @@ class MessageListView with ChangeNotifier, _MessageSequence { case TopicNarrow(): case DmNarrow(): case MentionsNarrow(): + case StarredMessagesNarrow(): return VisibilityEffect.none; } } @@ -452,6 +454,7 @@ class MessageListView with ChangeNotifier, _MessageSequence { case TopicNarrow(): case DmNarrow(): case MentionsNarrow(): + case StarredMessagesNarrow(): return true; } } @@ -638,6 +641,7 @@ class MessageListView with ChangeNotifier, _MessageSequence { case CombinedFeedNarrow(): case MentionsNarrow(): + case StarredMessagesNarrow(): // The messages were and remain in this narrow. // TODO(#421): … except they may have become muted or not. // We'll handle that at the same time as we handle muting itself changing. diff --git a/lib/model/narrow.dart b/lib/model/narrow.dart index 0d635033be..1553750849 100644 --- a/lib/model/narrow.dart +++ b/lib/model/narrow.dart @@ -321,3 +321,25 @@ class MentionsNarrow extends Narrow { @override int get hashCode => 'MentionsNarrow'.hashCode; } + +class StarredMessagesNarrow extends Narrow { + const StarredMessagesNarrow(); + + @override + ApiNarrow apiEncode() => [ApiNarrowIs(IsOperand.starred)]; + + @override + bool containsMessage(Message message) { + return message.flags.contains(MessageFlag.starred); + } + + @override + bool operator ==(Object other) { + if (other is! StarredMessagesNarrow) return false; + // Conceptually there's only one value of this type. + return true; + } + + @override + int get hashCode => 'StarredMessagesNarrow'.hashCode; +} diff --git a/lib/model/unreads.dart b/lib/model/unreads.dart index 96689a9ffe..76429fb20a 100644 --- a/lib/model/unreads.dart +++ b/lib/model/unreads.dart @@ -194,6 +194,9 @@ class Unreads extends ChangeNotifier { int countInMentionsNarrow() => mentions.length; + // TODO: Implement unreads handling. + int countInStarredMessagesNarrow() => 0; + int countInNarrow(Narrow narrow) { switch (narrow) { case CombinedFeedNarrow(): @@ -206,6 +209,8 @@ class Unreads extends ChangeNotifier { return countInDmNarrow(narrow); case MentionsNarrow(): return countInMentionsNarrow(); + case StarredMessagesNarrow(): + return countInStarredMessagesNarrow(); } } diff --git a/lib/widgets/actions.dart b/lib/widgets/actions.dart index 7ff14a3c64..d65801bc28 100644 --- a/lib/widgets/actions.dart +++ b/lib/widgets/actions.dart @@ -214,5 +214,8 @@ Future _legacyMarkNarrowAsRead(BuildContext context, Narrow narrow) async messages: unreadMentions, op: UpdateMessageFlagsOp.add, flag: MessageFlag.read); + case StarredMessagesNarrow(): + // TODO: Implement unreads handling. + return; } } diff --git a/lib/widgets/app.dart b/lib/widgets/app.dart index 232b8d1de3..38abaad4a7 100644 --- a/lib/widgets/app.dart +++ b/lib/widgets/app.dart @@ -283,6 +283,12 @@ class HomePage extends StatelessWidget { narrow: const MentionsNarrow())), child: Text(zulipLocalizations.mentionsPageTitle)), const SizedBox(height: 16), + ElevatedButton( + onPressed: () => Navigator.push(context, + MessageListPage.buildRoute(context: context, + narrow: const StarredMessagesNarrow())), + child: Text(zulipLocalizations.starredMessagesPageTitle)), + const SizedBox(height: 16), ElevatedButton( onPressed: () => Navigator.push(context, InboxPage.buildRoute(context: context)), diff --git a/lib/widgets/compose_box.dart b/lib/widgets/compose_box.dart index 4897b88629..d20cf9fb4b 100644 --- a/lib/widgets/compose_box.dart +++ b/lib/widgets/compose_box.dart @@ -1123,6 +1123,7 @@ class ComposeBox extends StatelessWidget { case CombinedFeedNarrow(): case MentionsNarrow(): + case StarredMessagesNarrow(): return false; } } @@ -1139,6 +1140,7 @@ class ComposeBox extends StatelessWidget { return _FixedDestinationComposeBox(key: controllerKey, narrow: narrow); case CombinedFeedNarrow(): case MentionsNarrow(): + case StarredMessagesNarrow(): return const SizedBox.shrink(); } } diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 467d901f30..a72709fcfe 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -248,6 +248,7 @@ class _MessageListPageState extends State implements MessageLis switch(narrow) { case CombinedFeedNarrow(): case MentionsNarrow(): + case StarredMessagesNarrow(): appBarBackgroundColor = null; // i.e., inherit case ChannelNarrow(:final streamId): @@ -337,6 +338,9 @@ class MessageListAppBarTitle extends StatelessWidget { case MentionsNarrow(): return Text(zulipLocalizations.mentionsPageTitle); + case StarredMessagesNarrow(): + return Text(zulipLocalizations.starredMessagesPageTitle); + case ChannelNarrow(:var streamId): final store = PerAccountStoreWidget.of(context); final stream = store.streams[streamId]; @@ -815,6 +819,7 @@ class RecipientHeader extends StatelessWidget { switch (narrow) { case CombinedFeedNarrow(): case MentionsNarrow(): + case StarredMessagesNarrow(): return true; case ChannelNarrow(): diff --git a/test/api/route/messages_test.dart b/test/api/route/messages_test.dart index 0af7bb9b30..0802522058 100644 --- a/test/api/route/messages_test.dart +++ b/test/api/route/messages_test.dart @@ -191,6 +191,9 @@ void main() { checkNarrow(const MentionsNarrow().apiEncode(), jsonEncode([ {'operator': 'is', 'operand': 'mentioned'}, ])); + checkNarrow(const StarredMessagesNarrow().apiEncode(), jsonEncode([ + {'operator': 'is', 'operand': 'starred'}, + ])); checkNarrow([ApiNarrowDm([123, 234])], jsonEncode([ {'operator': 'dm', 'operand': [123, 234]}, diff --git a/test/model/autocomplete_test.dart b/test/model/autocomplete_test.dart index df68b58da1..9830b2a734 100644 --- a/test/model/autocomplete_test.dart +++ b/test/model/autocomplete_test.dart @@ -679,6 +679,13 @@ void main() { check(() => MentionAutocompleteView.init(store: store, narrow: narrow)) .throws(); }); + + test('StarredMessagesNarrow gives error', () async { + await prepare(users: [eg.user(), eg.user()], messages: []); + const narrow = StarredMessagesNarrow(); + check(() => MentionAutocompleteView.init(store: store, narrow: narrow)) + .throws(); + }); }); test('final results end-to-end', () async { diff --git a/test/model/compose_test.dart b/test/model/compose_test.dart index b5be162171..37c7c50322 100644 --- a/test/model/compose_test.dart +++ b/test/model/compose_test.dart @@ -239,6 +239,14 @@ hello .equals(store.realmUrl.resolve('#narrow/is/mentioned/near/1')); }); + test('StarredMessagesNarrow', () { + final store = eg.store(); + check(narrowLink(store, const StarredMessagesNarrow())) + .equals(store.realmUrl.resolve('#narrow/is/starred')); + check(narrowLink(store, const StarredMessagesNarrow(), nearMessageId: 1)) + .equals(store.realmUrl.resolve('#narrow/is/starred/near/1')); + }); + test('ChannelNarrow / TopicNarrow', () { void checkNarrow(String expectedFragment, { required int streamId, diff --git a/test/model/internal_link_test.dart b/test/model/internal_link_test.dart index 424f91eda5..a6f0b6647b 100644 --- a/test/model/internal_link_test.dart +++ b/test/model/internal_link_test.dart @@ -242,10 +242,11 @@ void main() { switch (operand) { case IsOperand.mentioned: testCases = sharedCases(const MentionsNarrow()); + case IsOperand.starred: + testCases = sharedCases(const StarredMessagesNarrow()); case IsOperand.dm: case IsOperand.private: case IsOperand.alerted: - case IsOperand.starred: case IsOperand.followed: case IsOperand.resolved: case IsOperand.unread: diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index ccb006e78a..983ae18645 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -1447,6 +1447,44 @@ void main() { check(model.messages.map((m) => m.id)).deepEquals(expected..add(301 + i)); } }); + + test('in StarredMessagesNarrow', () async { + final stream = eg.stream(streamId: 1, name: 'muted stream'); + const mutedTopic = 'muted'; + await prepare(narrow: const StarredMessagesNarrow()); + await store.addStream(stream); + await store.addUserTopic(stream, mutedTopic, UserTopicVisibilityPolicy.muted); + await store.addSubscription(eg.subscription(stream, isMuted: true)); + + List getMessages(int startingId) => [ + eg.streamMessage(id: startingId, + stream: stream, topic: mutedTopic, flags: [MessageFlag.starred]), + eg.dmMessage(id: startingId + 1, + from: eg.otherUser, to: [eg.selfUser], flags: [MessageFlag.starred]), + ]; + + // Check filtering on fetchInitial… + await prepareMessages(foundOldest: false, messages: getMessages(201)); + final expected = []; + check(model.messages.map((m) => m.id)) + .deepEquals(expected..addAll([201, 202])); + + // … and on fetchOlder… + connection.prepare(json: olderResult( + anchor: 201, foundOldest: true, messages: getMessages(101)).toJson()); + await model.fetchOlder(); + checkNotified(count: 2); + check(model.messages.map((m) => m.id)) + .deepEquals(expected..insertAll(0, [101, 102])); + + // … and on MessageEvent. + final messages = getMessages(301); + for (var i = 0; i < 2; i += 1) { + await store.handleEvent(MessageEvent(id: 0, message: messages[i])); + checkNotifiedOnce(); + check(model.messages.map((m) => m.id)).deepEquals(expected..add(301 + i)); + } + }); }); test('recipient headers are maintained consistently', () async { @@ -1693,6 +1731,7 @@ void checkInvariants(MessageListView model) { case TopicNarrow(): case DmNarrow(): case MentionsNarrow(): + case StarredMessagesNarrow(): } } diff --git a/test/model/narrow_test.dart b/test/model/narrow_test.dart index 2f2ff229c3..9d676a531b 100644 --- a/test/model/narrow_test.dart +++ b/test/model/narrow_test.dart @@ -162,4 +162,15 @@ void main() { eg.streamMessage(flags: [MessageFlag.wildcardMentioned]))).isTrue(); }); }); + + group('StarredMessagesNarrow', () { + test('containsMessage', () { + const narrow = StarredMessagesNarrow(); + + check(narrow.containsMessage( + eg.streamMessage(flags: []))).isFalse(); + check(narrow.containsMessage( + eg.streamMessage(flags:[MessageFlag.starred]))).isTrue(); + }); + }); } diff --git a/test/model/unreads_test.dart b/test/model/unreads_test.dart index 1f66b2ad25..47f257dc89 100644 --- a/test/model/unreads_test.dart +++ b/test/model/unreads_test.dart @@ -227,6 +227,17 @@ void main() { ]); check(model.countInMentionsNarrow()).equals(2); }); + + test('countInStarredMessagesNarrow', () async { + final stream = eg.stream(); + prepare(); + await channelStore.addStream(stream); + fillWithMessages([ + eg.streamMessage(stream: stream, flags: []), + eg.streamMessage(stream: stream, flags: [MessageFlag.starred]), + ]); + check(model.countInStarredMessagesNarrow()).equals(0); + }); }); group('handleMessageEvent', () { diff --git a/test/widgets/action_sheet_test.dart b/test/widgets/action_sheet_test.dart index 945bd15a44..b37ca66bc5 100644 --- a/test/widgets/action_sheet_test.dart +++ b/test/widgets/action_sheet_test.dart @@ -392,6 +392,12 @@ void main() { await setupToMessageActionSheet(tester, message: message, narrow: const MentionsNarrow()); check(findQuoteAndReplyButton(tester)).isNull(); }); + + testWidgets('not offered in StarredMessagesNarrow (composing to reply is not yet supported)', (tester) async { + final message = eg.streamMessage(flags: [MessageFlag.starred]); + await setupToMessageActionSheet(tester, message: message, narrow: const StarredMessagesNarrow()); + check(findQuoteAndReplyButton(tester)).isNull(); + }); }); group('MarkAsUnread', () { diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index 8e2a05d49b..0319b93c94 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -727,6 +727,15 @@ void main() { check(findInMessageList('topic name')).length.equals(1); }); + testWidgets('show channel name in StarredMessagesNarrow', (tester) async { + await setupMessageListPage(tester, + narrow: const StarredMessagesNarrow(), + messages: [message], subscriptions: [eg.subscription(stream)]); + await tester.pump(); + check(findInMessageList('stream name')).length.equals(1); + check(findInMessageList('topic name')).length.equals(1); + }); + testWidgets('do not show channel name in ChannelNarrow', (tester) async { await setupMessageListPage(tester, narrow: ChannelNarrow(stream.streamId),