Skip to content

msglist: Support @-mentions narrow #807

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

Merged
merged 7 commits into from
Aug 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -472,6 +472,10 @@
"@combinedFeedPageTitle": {
"description": "Title for the page of combined feed."
},
"mentionsPageTitle": "Mentions",
"@mentionsPageTitle": {
"description": "Title for the page of @-mentions."
},
"notifGroupDmConversationLabel": "{senderFullName} to you and {numOthers, plural, =1{1 other} other{{numOthers} others}}",
"@notifGroupDmConversationLabel": {
"description": "Label for a group DM conversation notification.",
Expand Down
7 changes: 7 additions & 0 deletions lib/api/model/narrow.dart
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,13 @@ class ApiNarrowPmWith extends ApiNarrowDm {
}

// TODO: generalize into ApiNarrowIs
class ApiNarrowIsMentioned extends ApiNarrowElement {
@override String get operator => 'is';
@override String get operand => 'mentioned';

ApiNarrowIsMentioned({super.negated});
}

class ApiNarrowIsUnread extends ApiNarrowElement {
@override String get operator => 'is';
@override String get operand => 'unread';
Expand Down
1 change: 1 addition & 0 deletions lib/model/autocomplete.dart
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,7 @@ class MentionAutocompleteView extends ChangeNotifier {
case DmNarrow():
break;
case CombinedFeedNarrow():
case MentionsNarrow():
assert(false, 'No compose box, thus no autocomplete is available in ${narrow.runtimeType}.');
}
return (userA, userB) => _compareByRelevance(userA, userB,
Expand Down
15 changes: 14 additions & 1 deletion lib/model/internal_link.dart
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,8 @@ Uri narrowLink(PerAccountStore store, Narrow narrow, {int? nearMessageId}) {
fragment.write('${element.operand.join(',')}-$suffix');
case ApiNarrowDm():
assert(false, 'ApiNarrowDm should have been resolved');
case ApiNarrowIsMentioned():
fragment.write(element.operand.toString());
case ApiNarrowIsUnread():
fragment.write(element.operand.toString());
case ApiNarrowMessageId():
Expand Down Expand Up @@ -152,6 +154,7 @@ Narrow? _interpretNarrowSegments(List<String> segments, PerAccountStore store) {
ApiNarrowStream? streamElement;
ApiNarrowTopic? topicElement;
ApiNarrowDm? dmElement;
ApiNarrowIsMentioned? isMentionedElement;

for (var i = 0; i < segments.length; i += 2) {
final (operator, negated) = _parseOperator(segments[i]);
Expand Down Expand Up @@ -179,6 +182,10 @@ Narrow? _interpretNarrowSegments(List<String> segments, PerAccountStore store) {
if (dmIds == null) return null;
dmElement = ApiNarrowDm(dmIds, negated: negated);

case _NarrowOperator.is_:
if (isMentionedElement != null) return null;
if (operand == 'mentioned') isMentionedElement = ApiNarrowIsMentioned();

case _NarrowOperator.near: // TODO(#82): support for near
case _NarrowOperator.with_: // TODO(#683): support for with
continue;
Expand All @@ -188,7 +195,10 @@ Narrow? _interpretNarrowSegments(List<String> segments, PerAccountStore store) {
}
}

if (dmElement != null) {
if (isMentionedElement != null) {
if (streamElement != null || topicElement != null || dmElement != null) return null;
return const MentionsNarrow();
} else if (dmElement != null) {
if (streamElement != null || topicElement != null) return null;
return DmNarrow.withUsers(dmElement.operand, selfUserId: store.selfUserId);
} else if (streamElement != null) {
Expand All @@ -210,6 +220,9 @@ enum _NarrowOperator {
// cannot use `with` as it is a reserved keyword in Dart
@JsonValue('with')
with_,
// cannot use `is` as it is a reserved keyword in Dart
@JsonValue('is')
is_,
pmWith,
stream,
channel,
Expand Down
1 change: 1 addition & 0 deletions lib/model/internal_link.g.dart

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions lib/model/message_list.dart
Original file line number Diff line number Diff line change
Expand Up @@ -370,6 +370,7 @@ class MessageListView with ChangeNotifier, _MessageSequence {

case TopicNarrow():
case DmNarrow():
case MentionsNarrow():
return true;
}
}
Expand All @@ -385,6 +386,7 @@ class MessageListView with ChangeNotifier, _MessageSequence {

case TopicNarrow():
case DmNarrow():
case MentionsNarrow():
return true;
}
}
Expand Down
36 changes: 35 additions & 1 deletion lib/model/narrow.dart
Original file line number Diff line number Diff line change
Expand Up @@ -286,4 +286,38 @@ class DmNarrow extends Narrow implements SendableNarrow {
int get hashCode => Object.hash('DmNarrow', _key);
}

// TODO other narrow types: starred, mentioned; searches; arbitrary
class MentionsNarrow extends Narrow {
const MentionsNarrow();

@override
bool containsMessage(Message message) {
return message.flags.any((flag) {
switch (flag) {
case MessageFlag.mentioned:
case MessageFlag.wildcardMentioned:
return true;

case MessageFlag.read:
case MessageFlag.starred:
case MessageFlag.collapsed:
case MessageFlag.hasAlertWord:
case MessageFlag.historical:
case MessageFlag.unknown:
return false;
}
});
}

@override
ApiNarrow apiEncode() => [ApiNarrowIsMentioned()];

@override
bool operator ==(Object other) {
if (other is! MentionsNarrow) return false;
// Conceptually there's only one value of this type.
return true;
}

@override
int get hashCode => 'MentionsNarrow'.hashCode;
}
4 changes: 4 additions & 0 deletions lib/model/unreads.dart
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,8 @@ class Unreads extends ChangeNotifier {

int countInDmNarrow(DmNarrow narrow) => dms[narrow]?.length ?? 0;

int countInMentionsNarrow() => mentions.length;

int countInNarrow(Narrow narrow) {
switch (narrow) {
case CombinedFeedNarrow():
Expand All @@ -203,6 +205,8 @@ class Unreads extends ChangeNotifier {
return countInTopicNarrow(narrow.streamId, narrow.topic);
case DmNarrow():
return countInDmNarrow(narrow);
case MentionsNarrow():
return countInMentionsNarrow();
}
}

Expand Down
7 changes: 7 additions & 0 deletions lib/widgets/actions.dart
Original file line number Diff line number Diff line change
Expand Up @@ -134,5 +134,12 @@ Future<void> _legacyMarkNarrowAsRead(BuildContext context, Narrow narrow) async
messages: unreadDms,
op: UpdateMessageFlagsOp.add,
flag: MessageFlag.read);
case MentionsNarrow():
final unreadMentions = store.unreads.mentions.toList();
Copy link
Member

Choose a reason for hiding this comment

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

Why the copy (the toList call)?

Copy link
Member Author

Choose a reason for hiding this comment

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

updateMessageFlags (which calls connection.post) expects a List of message ids.

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see, and Unreads.mentions is a Set. Sounds good, then.

if (unreadMentions.isEmpty) return;
await updateMessageFlags(connection,
messages: unreadMentions,
op: UpdateMessageFlagsOp.add,
flag: MessageFlag.read);
}
}
6 changes: 6 additions & 0 deletions lib/widgets/app.dart
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,12 @@ class HomePage extends StatelessWidget {
narrow: const CombinedFeedNarrow())),
child: Text(zulipLocalizations.combinedFeedPageTitle)),
const SizedBox(height: 16),
ElevatedButton(
onPressed: () => Navigator.push(context,
MessageListPage.buildRoute(context: context,
narrow: const MentionsNarrow())),
child: Text(zulipLocalizations.mentionsPageTitle)),
const SizedBox(height: 16),
ElevatedButton(
onPressed: () => Navigator.push(context,
InboxPage.buildRoute(context: context)),
Expand Down
14 changes: 14 additions & 0 deletions lib/widgets/compose_box.dart
Original file line number Diff line number Diff line change
Expand Up @@ -1023,6 +1023,19 @@ class ComposeBox extends StatelessWidget {
final GlobalKey<ComposeBoxController>? controllerKey;
final Narrow narrow;

static bool hasComposeBox(Narrow narrow) {
switch (narrow) {
case ChannelNarrow():
case TopicNarrow():
case DmNarrow():
return true;

case CombinedFeedNarrow():
case MentionsNarrow():
return false;
}
}

@override
Widget build(BuildContext context) {
final narrow = this.narrow;
Expand All @@ -1034,6 +1047,7 @@ class ComposeBox extends StatelessWidget {
case DmNarrow():
return _FixedDestinationComposeBox(key: controllerKey, narrow: narrow);
case CombinedFeedNarrow():
case MentionsNarrow():
return const SizedBox.shrink();
}
}
Expand Down
25 changes: 20 additions & 5 deletions lib/widgets/message_list.dart
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,7 @@ class _MessageListPageState extends State<MessageListPage> implements MessageLis
bool removeAppBarBottomBorder = false;
switch(widget.narrow) {
case CombinedFeedNarrow():
case MentionsNarrow():
appBarBackgroundColor = null; // i.e., inherit

case ChannelNarrow(:final streamId):
Expand Down Expand Up @@ -277,11 +278,9 @@ class _MessageListPageState extends State<MessageListPage> implements MessageLis
context: context,

// The compose box, when present, pads the bottom inset.
// TODO this copies the details of when the compose box is shown;
// if those details get complicated, refactor to avoid copying.
// TODO(#311) If we have a bottom nav, it will pad the bottom
// inset, and this should always be true.
removeBottom: widget.narrow is! CombinedFeedNarrow,
removeBottom: ComposeBox.hasComposeBox(widget.narrow),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Happily I think we can now remove the TODO above this:

              // TODO this copies the details of when the compose box is shown;
              //   if those details get complicated, refactor to avoid copying.


child: Expanded(
child: MessageList(narrow: widget.narrow))),
Expand Down Expand Up @@ -322,6 +321,9 @@ class MessageListAppBarTitle extends StatelessWidget {
case CombinedFeedNarrow():
return Text(zulipLocalizations.combinedFeedPageTitle);

case MentionsNarrow():
return Text(zulipLocalizations.mentionsPageTitle);

case ChannelNarrow(:var streamId):
final store = PerAccountStoreWidget.of(context);
final stream = store.streams[streamId];
Expand Down Expand Up @@ -514,7 +516,7 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
return _buildItem(data, i);
}));

if (widget.narrow is CombinedFeedNarrow) {
if (!ComposeBox.hasComposeBox(widget.narrow)) {
// TODO(#311) If we have a bottom nav, it will pad the bottom
// inset, and this shouldn't be necessary
sliver = SliverSafeArea(sliver: sliver);
Expand Down Expand Up @@ -807,12 +809,25 @@ class RecipientHeader extends StatelessWidget {
final Message message;
final Narrow narrow;

static bool _containsDifferentChannels(Narrow narrow) {
switch (narrow) {
case CombinedFeedNarrow():
case MentionsNarrow():
return true;

case ChannelNarrow():
case TopicNarrow():
case DmNarrow():
return false;
}
}

@override
Widget build(BuildContext context) {
final message = this.message;
return switch (message) {
StreamMessage() => StreamMessageRecipientHeader(message: message,
showStream: narrow is CombinedFeedNarrow),
showStream: _containsDifferentChannels(narrow)),
DmMessage() => DmRecipientHeader(message: message),
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't seem like the right reasoning. We include the stream name in recipient headers just when the message list might include messages in different streams, so the user can know which stream each stream message belongs to. The presence/absence of the compose box isn't really part of that story.

I see that we'd like there to be an exhaustive switch, though. We could do that inline, or perhaps out in a helper function. (Not on the compose box, though, since it's not really relevant, as mentioned.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. I prefer adding a helper with switch cases in this case.

}
Expand Down
3 changes: 3 additions & 0 deletions test/api/route/messages_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,9 @@ void main() {
{'operator': 'stream', 'operand': 12},
{'operator': 'topic', 'operand': 'stuff'},
]));
checkNarrow(const MentionsNarrow().apiEncode(), jsonEncode([
{'operator': 'is', 'operand': 'mentioned'},
]));

checkNarrow([ApiNarrowDm([123, 234])], jsonEncode([
{'operator': 'dm', 'operand': [123, 234]},
Expand Down
7 changes: 7 additions & 0 deletions test/model/autocomplete_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -571,6 +571,13 @@ void main() {
check(() => MentionAutocompleteView.init(store: store, narrow: narrow))
.throws<AssertionError>();
});

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

test('final results end-to-end', () async {
Expand Down
11 changes: 8 additions & 3 deletions test/model/compose_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,14 @@ hello
.equals(store.realmUrl.resolve('#narrow/near/1'));
});

test('MentionsNarrow', () {
final store = eg.store();
check(narrowLink(store, const MentionsNarrow()))
.equals(store.realmUrl.resolve('#narrow/is/mentioned'));
check(narrowLink(store, const MentionsNarrow(), nearMessageId: 1))
.equals(store.realmUrl.resolve('#narrow/is/mentioned/near/1'));
});

test('ChannelNarrow / TopicNarrow', () {
void checkNarrow(String expectedFragment, {
required int streamId,
Expand Down Expand Up @@ -298,9 +306,6 @@ hello
'#narrow/dm/1,2-dm/near/12345',
'#narrow/pm-with/1,2-pm/near/12345');
});

// TODO other Narrow subclasses as we add them:
// starred, mentioned; searches; arbitrary
});

group('mention', () {
Expand Down
14 changes: 14 additions & 0 deletions test/model/internal_link_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,20 @@ void main() {
testExpectedNarrows(testCases, streams: streams);
});

group('"/#narrow/is/mentioned returns expected MentionsNarrow', () {
final testCases = [
('/#narrow/is/mentioned', const MentionsNarrow()),
('/#narrow/is/mentioned/near/1', const MentionsNarrow()),
('/#narrow/is/mentioned/with/2', const MentionsNarrow()),
('/#narrow/channel/7-test-here/is/mentioned', null),
('/#narrow/channel/check/topic/test/is/mentioned', null),
('/#narrow/topic/test/is/mentioned', null),
('/#narrow/dm/17327-Chris-Bobbe-(Test-Account)/is/mentioned', null),
('/#narrow/-is/mentioned', null),
];
testExpectedNarrows(testCases, streams: streams);
});

group('unexpected link shapes are rejected', () {
final testCases = [
('/#narrow/stream/name/topic/', null), // missing operand
Expand Down
Loading