msglist: Support @-mentions narrow #807

PIG208 · 2024-07-12T21:19:42Z

Early prototype to support @-mentions narrow.

TODO:

Organize the commits
~~Evaluate ways to support wildcardMetnioned, channelWildcardMentioned and topicWildcardMentioned with backwards/forward compatibility.~~ Deferred to Support @topic wildcard mentions #817
Test cases

Fixes #250

gnprice · 2024-07-12T22:19:22Z

lib/model/autocomplete.dart

-    assert(narrow is! CombinedFeedNarrow);
+    assert(narrow is SendableNarrow);


This is too strong — this class should work on a stream narrow, too. The point is it should work on any narrow where we offer a compose box.

(Do we have an existing test that fails with this change? If not, a good small side fix would be to add one.)

For purposes of this PR, we can just add one more case to this assert, and probably a one-line comment explaining why it's here. There might be a refactor to do if we start prolifierating to three or four such cases.

We do have test cases that fail this, which pass StreamNarrows.

PIG208 · 2024-07-16T17:49:44Z

Reading this chat thread gives me some background on why we need the topic_wildcard_mentioned flag (which is to provide necessary information to determine if the @topic mention needs to be highlighted for the user).

Looks like we don't support rendering topic mentions yet, but I think we can skip that for now as @-mentions narrow doesn't rely on this feature.

PIG208 · 2024-07-16T18:05:42Z

Regarding whether we should support both stream_wildcard_mention and the potential future channel_wildcard_mention at the same time, I think the answer is no if the server is going with this migration strategy. Likely at that point we will add a client capability, but we shouldn't worry about it now.

gnprice · 2024-07-16T18:06:47Z

Looks like we don't support rendering topic mentions yet, but I think we can skip that for now as @-mentions narrow doesn't rely on this feature.

Yeah, it's fine to leave that flag out if it isn't needed for this feature.

If it's easy to include it, though, then it's probably less total work if we include it now than if we add the other flag now and that flag separately later.

gnprice · 2024-07-16T18:07:38Z

Yeah agreed, let's not try to include support for a future channel_wildcard_mention flag at this point.

PIG208 · 2024-07-16T23:56:40Z

When working on this, I found exhaustiveness checks useful to covering places that need to be updated when adding a narrow. For addings tests, having each file corresponds to a test file helped, but I also needed to do a sweep by searching for CombinedFeedNarrow.

Bookkeeping comments like "TODO other narrow types: starred, mentioned; searches; arbitrary" will be a lot more helpful if we use them more consistently at places. However, it is understandable that the cases for different narrows can be pretty different. We can't know in advance all the time. We likely won't be adding a lot of narrows, so it should be fine without other actions taken.

PIG208 · 2024-07-17T00:08:27Z

I read the comment on the weird corner-case when messages might be absent in mentions, and it appears to me that it wouldn't interfere with this feature change.

This PR should be ready for an initial review now.

gnprice · 2024-07-17T02:55:19Z

For addings tests, having each file corresponds to a test file helped, but I also needed to do a sweep by searching for CombinedFeedNarrow.

Bookkeeping comments like "TODO other narrow types: starred, mentioned; searches; arbitrary" will be a lot more helpful if we use them more consistently at places. However, it is understandable that the cases for different narrows can be pretty different. We can't know in advance all the time.

Yeah, I think it's OK to have to search for existing similar cases (like CombinedFeedNarrow) when adding a new type like this.

That TODO line wasn't meant for the purpose of finding all the different places that different types of narrows will need to be handled. It'd be pretty tough to have such comments comprehensively enough to rely much on them — as you saw, there are a lot of places that need updating. If you run the command git log -p -G 'TODO other' lib/model/narrow.dart (and use my "secret" for reading git log -p output), you can see the history of that line; it was added as part of a very early sketch, and at this point should probably just be deleted in favor of the issues we have for adding more narrow types.

It would be good to make it so that those exhaustiveness checks were a more comprehensive guide. I think that mostly means factoring out all the conditions that look like this:
narrow is CombinedFeedNarrow || narrow is MentionsNarrow
into a method somewhere that says whether we offer a compose box for a narrow, and that is exhaustive by construction.

Probably the most natural home for that is as a static on ComposeBox. That'll mean the one spot in lib/model/ (on autocomplete) can't use it; but that one spot isn't really used right now anyway, and I think once it is it'll naturally involve an exhaustive switch too.

PIG208 · 2024-07-18T18:01:26Z

Rebased this atop #808, and extracted the is CombinedNarrow checks to ComposeBox.hasComposeBox. I have also deleted the TODO lines now that we have the equivalent issues.

gnprice · 2024-07-18T18:46:23Z

lib/widgets/compose_box.dart

+  static bool hasComposeBox(Narrow narrow) {
+    switch (narrow) {
+      case CombinedFeedNarrow():
+        return true;


This is inverted, right?

Right. Pushed a fix.

PIG208 · 2024-07-20T06:13:10Z

Rebased to resolve the merge conflicts from the markNarrowAsUnread refactor.

chrisbobbe

Thanks! Small comments below.

Then one question I had was if we want to update parseInternalLink so that it can return a MentionsNarrow (I guess just when the link is /#narrow/is/mentioned). I don't think it's common to want to post a link to the mentions narrow, but it crossed my mind as something that would now be possible to support.

chrisbobbe · 2024-07-24T21:58:33Z

lib/widgets/message_list.dart

@@ -541,7 +541,7 @@ class RecipientHeader extends StatelessWidget {
    final message = this.message;
    return switch (message) {
      StreamMessage() => StreamMessageRecipientHeader(message: message,
-        showStream: narrow is CombinedFeedNarrow),
+        showStream: !ComposeBox.hasComposeBox(narrow)),
      DmMessage() => DmRecipientHeader(message: message),
    };


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.)

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

chrisbobbe · 2024-07-24T21:59:19Z

lib/widgets/message_list.dart

@@ -129,7 +129,7 @@ class _MessageListPageState extends State<MessageListPage> implements MessageLis
              //   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),


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.

gnprice

Thanks @PIG208 for building this, and thanks @chrisbobbe for the previous review! Generally this looks great. Comments below.

Also, would you add screenshots of the UI changes? The button on the home page, and the new message-list page.

gnprice · 2024-07-26T21:56:49Z

assets/l10n/app_en.arb

@@ -344,6 +344,10 @@
  "@loginErrorMissingUsername": {
    "description": "Error message when an empty username was provided."
  },
+  "mentionsPageTitle": "Mentions",


nit: put next to combinedFeedPageTitle, the most related existing string

Ordering them to be consistent with MessageListAppBarTitle.

gnprice · 2024-07-26T21:58:34Z

lib/model/narrow.dart

+  }
+
+  @override
+  int get hashCode => 'MentionedNarrow'.hashCode;


nit: match name of class

gnprice · 2024-07-26T21:59:22Z

lib/widgets/actions.dart

@@ -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();


Why the copy (the toList call)?

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

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

gnprice · 2024-07-26T22:00:26Z

lib/widgets/compose_box.dart

+  static bool hasComposeBox(Narrow narrow) {
+    switch (narrow) {
+      case CombinedFeedNarrow():
+      case MentionsNarrow():
+        return false;
+
+      case StreamNarrow():


nit: order this switch the same way as the one in build just below — this is effectively a summary of that one and needs to be kept in sync, so keeping the same ordering helps with verifying that at a glance

gnprice · 2024-07-26T22:01:06Z

lib/widgets/message_list.dart

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


nit: put this case next to CombinedFeedNarrow, as that's the most similar

gnprice · 2024-07-26T22:13:44Z

test/model/message_list_test.dart

+        eg.streamMessage(id: startingId,
+          stream: stream1, topic: "A", flags: [MessageFlag.wildcardMentioned]),
+        eg.streamMessage(id: startingId + 1,
+          stream: stream1, topic: "B", flags: [MessageFlag.wildcardMentioned]),
+        eg.streamMessage(id: startingId + 2,
+          stream: stream2, topic: "C", flags: [MessageFlag.wildcardMentioned]),


When writing a test, a question to ask is: what story does this test need to tell? What is it checking, and what scenarios should it be exercising?

Here, the story that's needed is: messages get included in the view, even messages you might think wouldn't be. Specifically that means: including even a message in a muted topic and muted channel, including even DMs, and including even messages that have flag wildcardMentioned but not mentioned.

(The other half of the story would be: messages get excluded from the view, even messages you might think would be included. But for this narrow there are no messages that get excluded, so there's nothing to say in that direction.)

So then the test data should be written to focus on that story. In particular, the messages here that are to an unmuted channel or topic aren't needed — if messages are included even when muted, surely they are when unmuted too.

(It looks like this set of data is based on the one for CombinedFeedNarrow. But there the answer is different for muted vs. unmuted, so it's important to cover both sides.)

gnprice · 2024-07-26T22:20:47Z

test/model/narrow_test.dart

+      check(narrow.containsMessage(eg.streamMessage(
+        flags: []))).isFalse();
+      check(narrow.containsMessage(eg.streamMessage(
+        flags:[MessageFlag.mentioned]))).isTrue();
+      check(narrow.containsMessage(eg.streamMessage(
+        flags: [MessageFlag.wildcardMentioned]))).isTrue();


nit: this is a bit hard to read because the isFalse and isTrue, which are the most critical bits of information, get visually buried.

This reformatting helps make them stick out:

Suggested change

check(narrow.containsMessage(eg.streamMessage(

flags: []))).isFalse();

check(narrow.containsMessage(eg.streamMessage(

flags:[MessageFlag.mentioned]))).isTrue();

check(narrow.containsMessage(eg.streamMessage(

flags: [MessageFlag.wildcardMentioned]))).isTrue();

check(narrow.containsMessage(

eg.streamMessage(flags: []))).isFalse();

check(narrow.containsMessage(

eg.streamMessage(flags: [MessageFlag.mentioned]))).isTrue();

check(narrow.containsMessage(

eg.streamMessage(flags: [MessageFlag.wildcardMentioned]))).isTrue();

Stepping back, though, the main purpose of a test is to keep the code from getting accidentally broken by future changes. Here, I think the biggest risk for accidental breakage is when we add another mentions-related flag, that we might forget to handle it in MentionsNarrow.containsMessage. This test as written won't help in that case — it won't get broken by that.

Probably the most effective thing to do is to make the method's implementation use an exhaustive switch rather than a pair of equality checks. Then it's fine to still have this test, but there's no longer so much relying on it.

gnprice · 2024-07-26T22:25:53Z

test/widgets/actions_test.dart

+            'op': 'add',
+            'flag': 'read',
+          });
+        await tester.pumpAndSettle();


nit: overindented; but also do we need this line? the similar test just above doesn't seem to have it

gnprice · 2024-07-26T22:41:22Z

test/model/internal_link_test.dart

+        ('/#narrow/stream/7-test-here/is/mentioned',                  null),
+        ('/#narrow/stream/check/topic/test/is/mentioned',             null),


One more test case to add:

Suggested change

('/#narrow/stream/7-test-here/is/mentioned', null),

('/#narrow/stream/check/topic/test/is/mentioned', null),

('/#narrow/stream/7-test-here/is/mentioned', null),

('/#narrow/stream/check/topic/test/is/mentioned', null),

('/#narrow/topic/test/is/mentioned', null),

This is again on the principle of exercising the cases that are closest to the line between one answer and another, most at risk of going the wrong way. If for example the topicElement != null check were omitted in the lines this commit adds to the implementation, then I don't think any other test cases would fail, but this one would.

gnprice · 2024-07-26T22:41:35Z

lib/model/internal_link.dart

@@ -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;


nit:

Suggested change

if (streamElement != null || topicElement != null || dmElement != null) return null;

if (streamElement != null || topicElement != null || dmElement != null) return null;

PIG208 · 2024-07-27T06:36:56Z

Will get back to this next week and complete the update.

PIG208 · 2024-07-29T17:27:07Z

Home menu	Mentions page

PIG208 · 2024-07-29T18:03:52Z

Updated the PR to

resolve the review comments
take care of some places where "channel" can be used, depending on the context
make the last commit to fix the issue instead

gnprice

Thanks for the revision! Just one comment below — otherwise all looks good.

gnprice · 2024-07-31T00:39:31Z

lib/model/narrow.dart

+      switch (flag) {
+        MessageFlag.mentioned => true,
+        MessageFlag.wildcardMentioned => true,
+        _ => false


So in #807 (comment) the point was to make sure that when we add a new mentions-related flag, we don't forget to handle it here. This version doesn't really help with that. 🙂

Instead, list all the flags. That incurs a small cost when we add a non-mentions-related flag, but it's worth it for preventing the class of bug where we forget this for a mentions-related flag.

Good thought, thanks!

gnprice · 2024-07-31T00:41:29Z

lib/widgets/actions.dart

@@ -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();


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

PIG208 · 2024-07-31T19:13:37Z

Pushed to address the switch case change. Should be ready once it passes the tests.

Using an exhaustive switch here so we can be reminded of handling new narrows. Signed-off-by: Zixuan James Li <[email protected]>

This will later help ensure the exhaustiveness of similar checks performed on a Narrow. The change is motivated by the need to support new narrows. Signed-off-by: Zixuan James Li <[email protected]>

Maybe we can try to generalize these "ApiNarrowIs"s classes when we support more "is:" operands. Signed-off-by: Zixuan James Li <[email protected]>

Because of how narrow interacts with the entire app, there are test updates all over the place. To check for the completeness of this change, looking at places where CombinedFeedNarrow is referenced can help, because MentionsNarrow is similar to it in many ways. Signed-off-by: Zixuan James Li <[email protected]>

This makes more visually clear the division between the list of flags that count, and the list of flags that don't.

Signed-off-by: Zixuan James Li <[email protected]>

As of now, we don't have generic for the "is" operator, but it will be a easy refactor once we do. Fixes zulip#250. Signed-off-by: Zixuan James Li <[email protected]>

gnprice · 2024-08-02T01:24:36Z

Thanks! Looks good — merging, with a couple of tweaks. See the added commit; and I fixed a couple of summary prefixes from "internal" and "internal_links" to "internal_link". ("Internal" sounds like some much more general meaning — like anything internal to the app.)

PIG208 added the a-msglist The message-list screen, except what's label:a-content label Jul 12, 2024

PIG208 mentioned this pull request Jul 12, 2024

dart [nfc]: Replace else-if's with switch. #808

Merged

gnprice reviewed Jul 12, 2024

View reviewed changes

PIG208 force-pushed the at-narrow branch 2 times, most recently from 9c5dd67 to 68af8a9 Compare July 16, 2024 05:28

PIG208 force-pushed the at-narrow branch 3 times, most recently from f1df782 to d51d274 Compare July 16, 2024 23:50

PIG208 force-pushed the at-narrow branch 2 times, most recently from 39cf1e0 to dcd451b Compare July 17, 2024 00:00

PIG208 marked this pull request as ready for review July 17, 2024 00:01

PIG208 force-pushed the at-narrow branch from dcd451b to d74633e Compare July 17, 2024 00:06

PIG208 added the maintainer review PR ready for review by Zulip maintainers label Jul 17, 2024

PIG208 requested a review from chrisbobbe July 17, 2024 00:09

PIG208 changed the title ~~WIP msglist: Support @-mentions narrow~~ msglist: Support @-mentions narrow Jul 17, 2024

PIG208 force-pushed the at-narrow branch from d74633e to b3086ea Compare July 18, 2024 18:00

gnprice reviewed Jul 18, 2024

View reviewed changes

PIG208 force-pushed the at-narrow branch 3 times, most recently from 179237e to 5cd83c2 Compare July 20, 2024 06:12

chrisbobbe reviewed Jul 24, 2024

View reviewed changes

PIG208 force-pushed the at-narrow branch 2 times, most recently from 4047c85 to b89070c Compare July 25, 2024 22:01

gnprice reviewed Jul 26, 2024

View reviewed changes

PIG208 force-pushed the at-narrow branch from b89070c to b8bacd0 Compare July 27, 2024 06:36

PIG208 force-pushed the at-narrow branch from b8bacd0 to 02d7995 Compare July 29, 2024 17:24

PIG208 force-pushed the at-narrow branch 3 times, most recently from 01a03df to 9caf247 Compare July 29, 2024 17:57

PIG208 force-pushed the at-narrow branch 2 times, most recently from 7a753cb to c769b23 Compare July 29, 2024 18:07

PIG208 requested a review from gnprice July 29, 2024 20:00

gnprice reviewed Jul 31, 2024

View reviewed changes

PIG208 force-pushed the at-narrow branch 2 times, most recently from d6a64c7 to 2e61521 Compare July 31, 2024 19:11

PIG208 and others added 7 commits August 1, 2024 18:22

msglist [nfc]: Compute showStream with an helper.

799162c

Using an exhaustive switch here so we can be reminded of handling new narrows. Signed-off-by: Zixuan James Li <[email protected]>

compose_box [nfc]: Avoid inline checks on narrows.

205a85a

This will later help ensure the exhaustiveness of similar checks performed on a Narrow. The change is motivated by the need to support new narrows. Signed-off-by: Zixuan James Li <[email protected]>

internal_link: Add ApiNarrowIsMentioned.

147ea7e

Maybe we can try to generalize these "ApiNarrowIs"s classes when we support more "is:" operands. Signed-off-by: Zixuan James Li <[email protected]>

narrow [nfc]: Use a switch statement in MentionsNarrow.containsMessage

028e2a7

This makes more visually clear the division between the list of flags that count, and the list of flags that don't.

nav: Add a button for MentionsNarrow.

3e5c438

Signed-off-by: Zixuan James Li <[email protected]>

internal_link: Parse "/is/mentioned" internal links.

71b23f5

As of now, we don't have generic for the "is" operator, but it will be a easy refactor once we do. Fixes zulip#250. Signed-off-by: Zixuan James Li <[email protected]>

gnprice force-pushed the at-narrow branch from 2e61521 to 71b23f5 Compare August 2, 2024 01:24

gnprice merged commit 71b23f5 into zulip:main Aug 2, 2024
1 check passed

PIG208 deleted the at-narrow branch August 2, 2024 17:24

gnprice mentioned this pull request Aug 2, 2024

Handle moved messages in MessageStore and message lists #787

Merged

		assert(narrow is! CombinedFeedNarrow);
		assert(narrow is SendableNarrow);

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

		('/#narrow/stream/7-test-here/is/mentioned', null),
		('/#narrow/stream/check/topic/test/is/mentioned', null),

	if (streamElement != null \|\| topicElement != null \|\| dmElement != null) return null;
	if (streamElement != null \|\| topicElement != null \|\| dmElement != null) return null;

msglist: Support @-mentions narrow #807

msglist: Support @-mentions narrow #807

Conversation

PIG208 commented Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 commented Jul 16, 2024

PIG208 commented Jul 16, 2024

gnprice commented Jul 16, 2024

gnprice commented Jul 16, 2024

PIG208 commented Jul 16, 2024

PIG208 commented Jul 17, 2024 • edited Loading

gnprice commented Jul 17, 2024 • edited Loading

PIG208 commented Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 commented Jul 20, 2024

chrisbobbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 commented Jul 27, 2024

PIG208 commented Jul 29, 2024

PIG208 commented Jul 29, 2024

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 commented Jul 31, 2024

gnprice commented Aug 2, 2024

PIG208 commented Jul 12, 2024 •

edited

Loading

PIG208 commented Jul 17, 2024 •

edited

Loading

gnprice commented Jul 17, 2024 •

edited

Loading

PIG208 commented Jul 18, 2024 •

edited

Loading