Skip to content

narrow: Add starred messages narrow #870

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Aug 21, 2024
Merged

narrow: Add starred messages narrow #870

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a2dc3ef
message [nfc]: Mention that @-mention live-updates have not yet been …
PIG208 Aug 15, 2024
115a5e5
action_sheet test: Create message with the appropriate flag.
PIG208 Aug 20, 2024
fe5dc29
msglist test: Remove redundant copy of a regression test.
PIG208 Aug 21, 2024
5bcb5d4
msglist test [nfc]: Pin down lack of compose box in no-insets regress…
gnprice Aug 21, 2024
9daa940
narrow: Add starred messages narrow.
PIG208 Aug 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions assets/l10n/app_en.arb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.",
Expand Down
1 change: 1 addition & 0 deletions lib/model/autocomplete.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,6 +364,7 @@ class MentionAutocompleteView extends AutocompleteView<MentionAutocompleteQuery,
break;
case CombinedFeedNarrow():
case MentionsNarrow():
case StarredMessagesNarrow():
assert(false, 'No compose box, thus no autocomplete is available in ${narrow.runtimeType}.');
}
return (userA, userB) => _compareByRelevance(userA, userB,
Expand Down
3 changes: 2 additions & 1 deletion lib/model/internal_link.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,10 +199,11 @@ Narrow? _interpretNarrowSegments(List<String> 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:
Expand Down
8 changes: 8 additions & 0 deletions lib/model/message.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,14 @@ 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.

// 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.
}
}
}
Expand Down
4 changes: 4 additions & 0 deletions lib/model/message_list.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -418,6 +418,7 @@ class MessageListView with ChangeNotifier, _MessageSequence {
case TopicNarrow():
case DmNarrow():
case MentionsNarrow():
case StarredMessagesNarrow():
return true;
}
}
Expand All @@ -436,6 +437,7 @@ class MessageListView with ChangeNotifier, _MessageSequence {
case TopicNarrow():
case DmNarrow():
case MentionsNarrow():
case StarredMessagesNarrow():
return VisibilityEffect.none;
}
}
Expand All @@ -452,6 +454,7 @@ class MessageListView with ChangeNotifier, _MessageSequence {
case TopicNarrow():
case DmNarrow():
case MentionsNarrow():
case StarredMessagesNarrow():
return true;
}
}
Expand Down Expand Up @@ -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.
Expand Down
22 changes: 22 additions & 0 deletions lib/model/narrow.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
5 changes: 5 additions & 0 deletions lib/model/unreads.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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():
Expand All @@ -206,6 +209,8 @@ class Unreads extends ChangeNotifier {
return countInDmNarrow(narrow);
case MentionsNarrow():
return countInMentionsNarrow();
case StarredMessagesNarrow():
return countInStarredMessagesNarrow();
}
}

Expand Down
3 changes: 3 additions & 0 deletions lib/widgets/actions.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,5 +214,8 @@ Future<void> _legacyMarkNarrowAsRead(BuildContext context, Narrow narrow) async
messages: unreadMentions,
op: UpdateMessageFlagsOp.add,
flag: MessageFlag.read);
case StarredMessagesNarrow():
// TODO: Implement unreads handling.
return;
}
}
6 changes: 6 additions & 0 deletions lib/widgets/app.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)),
Expand Down
2 changes: 2 additions & 0 deletions lib/widgets/compose_box.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1123,6 +1123,7 @@ class ComposeBox extends StatelessWidget {

case CombinedFeedNarrow():
case MentionsNarrow():
case StarredMessagesNarrow():
return false;
}
}
Expand All @@ -1139,6 +1140,7 @@ class ComposeBox extends StatelessWidget {
return _FixedDestinationComposeBox(key: controllerKey, narrow: narrow);
case CombinedFeedNarrow():
case MentionsNarrow():
case StarredMessagesNarrow():
return const SizedBox.shrink();
}
}
Expand Down
5 changes: 5 additions & 0 deletions lib/widgets/message_list.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,7 @@ class _MessageListPageState extends State<MessageListPage> implements MessageLis
switch(narrow) {
case CombinedFeedNarrow():
case MentionsNarrow():
case StarredMessagesNarrow():
appBarBackgroundColor = null; // i.e., inherit

case ChannelNarrow(:final streamId):
Expand Down Expand Up @@ -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];
Expand Down Expand Up @@ -815,6 +819,7 @@ class RecipientHeader extends StatelessWidget {
switch (narrow) {
case CombinedFeedNarrow():
case MentionsNarrow():
case StarredMessagesNarrow():
return true;

case ChannelNarrow():
Expand Down
3 changes: 3 additions & 0 deletions test/api/route/messages_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]},
Expand Down
7 changes: 7 additions & 0 deletions test/model/autocomplete_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -679,6 +679,13 @@ void main() {
check(() => MentionAutocompleteView.init(store: store, narrow: narrow))
.throws<AssertionError>();
});

test('StarredMessagesNarrow gives error', () async {
await prepare(users: [eg.user(), eg.user()], messages: []);
const narrow = StarredMessagesNarrow();
check(() => MentionAutocompleteView.init(store: store, narrow: narrow))
.throws<AssertionError>();
});
});

test('final results end-to-end', () async {
Expand Down
8 changes: 8 additions & 0 deletions test/model/compose_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
3 changes: 2 additions & 1 deletion test/model/internal_link_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
39 changes: 39 additions & 0 deletions test/model/message_list_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Message> 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 = <int>[];
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 {
Expand Down Expand Up @@ -1693,6 +1731,7 @@ void checkInvariants(MessageListView model) {
case TopicNarrow():
case DmNarrow():
case MentionsNarrow():
case StarredMessagesNarrow():
}
}

Expand Down
11 changes: 11 additions & 0 deletions test/model/narrow_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});
});
}
11 changes: 11 additions & 0 deletions test/model/unreads_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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', () {
Expand Down
8 changes: 7 additions & 1 deletion test/widgets/action_sheet_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,10 +388,16 @@ 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());
Comment on lines -391 to 392
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

action_sheet test: Create message with the appropriate flag.

The message wouldn't otherwise appear in MentionsNarrow.

Hmm indeed. But I'm a bit puzzled then: why wasn't this test already failing?

Ah it's because the code actually will show the message. So this is just a realism fix, making the simulated server response something a non-buggy server might give on this request.

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', () {
Expand Down
29 changes: 15 additions & 14 deletions test/widgets/message_list_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,27 +122,19 @@ 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;

await setupMessageListPage(tester, narrow: const CombinedFeedNarrow(),
messages: [eg.streamMessage(content: ContentExample.codeBlockPlain.html)]);

final element = tester.element(find.byType(CodeBlock));
final padding = MediaQuery.of(element).padding;
check(padding).equals(EdgeInsets.zero);
});

testWidgets('content in MentionsNarrow not asked to consume insets (including bottom)', (tester) async {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh one other commit-message comment:

A test on CombinedFeedNarrow is sufficient for preventing this particular
regression. We could go further and test this on other narrows, but
there is little to gain other than verifying that those other narrows do
not have a compose box, which is covered statically with the exhaustive
check from hasComposeBox.

The last sentence here doesn't sound right, as this test didn't check that the narrow doesn't have a compose box. Rather, if the narrow had a compose box, the test would pass even if the bug were to return on other narrows that don't have the compose box. The original bug reproduced only on CombinedFeedNarrow (the only narrow lacking a compose box at the time), which is why the repro test used that narrow.

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])]);
// 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;
Expand Down Expand Up @@ -735,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),
Expand Down