narrow: Add starred messages narrow #870

PIG208 · 2024-08-07T08:05:29Z

Unreads can probably be handled in a separate PR, because it will probably grow in complexity.

This is stacked on top of #869

PIG208 · 2024-08-08T00:49:51Z

This does not implement live updates for newly starred/unstarred messages. We should able to handle #818 together with this as a post-launch follow-up.

PIG208

We will leave unreads for starred messages for now, as it is pretty uncommon to have unreads for them.

PIG208 · 2024-08-08T17:49:40Z

Skipping the first 3 commits, this should be reviewable independently from #869.

chrisbobbe · 2024-08-12T21:45:52Z

Looks like CI is failing.

PIG208 · 2024-08-12T22:35:37Z

Should be fixed now. It was an error emerged from moved messages support across narrows.

chrisbobbe · 2024-08-13T05:07:16Z

Thanks! Ah, but it looks like some conflicts appeared after you pushed that revision; could you resolve those please? :)

PIG208 · 2024-08-13T18:59:53Z

Thanks! Just rebased it.

PIG208 · 2024-08-15T06:24:38Z

(Pushed again to fix another merge conflict.)

chrisbobbe

Thanks! Small comments below, including one about a bug I noticed.

In manual testing, I noticed that when I'm viewing the starred-messages narrow, when I unstar a message, it remains in the list. At first I thought this was a bug, but Greg pointed out that it's actually a product decision; it lets you easily undo the unstar action if you want. So that seems fine.

—But let's put a comment in the code, clarifying that it's intentional (to offer undo), so readers don't have to wonder. This point already applies for messages with @-mentions in the mentions narrow, I think, so it would be good to mention that narrow too, in a separate, earlier commit. I think the right place to comment on these things would be in MessageStoreImpl.handleUpdateMessageFlagsEvent, near where it operates on the message-list views (by having them notify listeners but not remove messages).

…Hmm. Thinking more, actually…The current behavior for remove-star events does seem helpful, but wouldn't it also be helpful if MessageStoreImpl.handleUpdateMessageFlagsEvent acted on add-star events by making the message appear in the starred-narrow message list? Similarly for mentioned messages in the mentions narrow.

chrisbobbe · 2024-08-14T22:55:54Z

test/widgets/action_sheet_test.dart

-      final message = eg.streamMessage();
-      await setupToMessageActionSheet(tester, message: message, narrow: const MentionsNarrow());
-      check(findQuoteAndReplyButton(tester)).isNull();
+    group('composing to reply is not yet supported', () {


test [nfc]: Parameterize some narrow-related tests.

I would leave off the [nfc]. It's true that application code doesn't change, but with the test prefix, I would reserve [nfc] for commits that don't change observable behavior when we run the tests. This does change behavior, by nesting some tests in a new group.

chrisbobbe · 2024-08-14T22:56:19Z

test/widgets/action_sheet_test.dart

+
+      for (final (narrowName, narrow, message) in testCases) {
+        assert(narrow.containsMessage(message));
+        testWidgets('not offered in $narrowName (composing to reply is not yet supported)', (tester) async {


Is the "(composing to reply is not yet supported)" necessary? It repeats what's in the group description.

chrisbobbe · 2024-08-14T23:00:32Z

test/widgets/message_list_test.dart

+      ("Combined feed", const CombinedFeedNarrow(), <MessageFlag>[]),
+      ("MentionsNarrow", const MentionsNarrow(),    [MessageFlag.mentioned]),


For the narrowName, let's be consistent about using the identifier ("CombinedFeedNarrow", "MentionsNarrow") or not ("Combined feed", "Mentions narrow").

chrisbobbe · 2024-08-15T18:04:05Z

lib/model/internal_link.dart

@@ -203,6 +203,7 @@ Narrow? _interpretNarrowSegments(List<String> segments, PerAccountStore store) {
      case IsOperand.private:
      case IsOperand.alerted:
      case IsOperand.starred:
+        return const StarredMessagesNarrow();


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

This is a bug. :) Since cases before IsOperand.starred are empty, they'll also give const StarredMessagesNarrow(), just like the IsOperand.starred case.

https://dart.dev/language/branches#switch-statements

Non-empty case clauses jump to the end of the switch after completion. They do not require a break statement.

It's a common pattern to want multiple cases to give the same thing, and to express that with empty case clauses, as here.

To fix (here and in the test, which has the same bug), I would just reorder:

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.followed: case IsOperand.resolved: case IsOperand.unread: case IsOperand.unknown: return null; }

PIG208 · 2024-08-15T23:46:30Z

Regarding

…Hmm. Thinking more, actually…The current behavior for remove-star events does seem helpful, but wouldn't it also be helpful if MessageStoreImpl.handleUpdateMessageFlagsEvent acted on add-star events by making the message appear in the starred-narrow message list? Similarly for mentioned messages in the mentions narrow.

Live-updates for starred messages and @-mentions are post-launch features, so we leave them out for now:

This does not implement live updates for newly starred/unstarred messages. We should able to handle #818 together with this as a post-launch follow-up.

PIG208 · 2024-08-15T23:46:48Z

Having comments for now should be suitable.

PIG208 · 2024-08-15T23:56:10Z

Updated the PR. Thanks for the review!

chrisbobbe · 2024-08-17T01:27:57Z

Thanks, LGTM! Over to Greg, then.

gnprice

Thanks @PIG208, and thanks @chrisbobbe for the previous review!

Generally this looks great. Comments below on the tests, mostly just on their structure.

Separately, I think the last three commits make most sense squashed together:
8d8841e narrow: Add starred messages narrow.
3b3b4c3 internal_link: Parse "/is/starred" internal links.
bf0759d nav: Add a button for starred messages page.

The latter two are small changes adding pieces that really should be there in order to say we support the starred-messages narrow. It seems like the main difference between those and most of the changes in the main commit is just that the main commit has the changes that are prompted by exhaustive-switch checking once the StarredMessagesNarrow type is added. That's a very useful tool but doesn't need to decide our commit structure.

(Even in the other direction: if the changes required to satisfy exhaustive switches added up to a big change that we didn't want to put in a single commit, we could split it up by making some of the new cases just throw UnimplementedError() or the like, with TODO comments pointing to the relevant issue.)

gnprice · 2024-08-20T22:19:09Z

test/widgets/message_list_test.dart

-      testWidgets('show stream name in CombinedFeedNarrow', (tester) async {
-        await setupMessageListPage(tester,
-          narrow: const CombinedFeedNarrow(),
-          messages: [message], subscriptions: [eg.subscription(stream)]);
-        await tester.pump();
-        check(findInMessageList('stream name')).length.equals(1);
-        check(findInMessageList('topic name')).length.equals(1);
-      });


I think these tests (and the other two sets of tests that get parametrized in the second commit) are actually better off in the form they were in, where each one has its own separate testWidgets call in the source code.

One reason is that that helps make each test case straightforward to read, by having fewer layers of indirection and conditionals to go through.

It also makes the set of test cases visible to the IDE (which does only some kind of very simple analysis looking for test and testWidgets calls), which is handy for running and debugging tests from there.

Instead, the preferable way to tighten up a set of related test cases is to factor out helper methods from their body, so that they can each call those helpers.

That solves the visibility-to-the-IDE problem. And though it does mean another layer of indirection, it's typically a simpler indirection — it doesn't have to cover computing a name for the test, and conditionals like the one below:

if (showChannelName) { check(findInMessageList('stream name')).single; } else { check(findInMessageList('stream name')).isEmpty(); }

get expressed by different test cases just doing one thing or the other.

Here, a small helper for the setupMessageListPage and tester.pump calls would work fine, though I think the existing tests are also fine without it.

The main situations where we put the test or testWidgets calls themselves inside a loop are:

when there's a bunch of variables we want to systematically cover all combinations of, as in test/widgets/sticky_header_test.dart

when there's a very large number of cases we want to test, as in test/model/internal_link_test.dart

The same downsides still apply, but those are where the payoff of that approach is large enough to make it worth it.

Thanks. sticky_header_test.dart is an interesting example. Dropped the parameterization commit.

gnprice · 2024-08-20T22:31:40Z

test/widgets/message_list_test.dart

  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 {
-      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);


This setup and checking gets lengthy enough that I agree it's a bit annoying to repeat it three times. That could be solved with a helper that the test cases call (as in my preceding comment)… or, looking closer at what these particular tests are about, it could really just be a single test case using any one of these narrows (probably CombinedFeedNarrow). It's a regression test for #736, and to do that job it really just needs some message list that doesn't have a compose box.

For good measure the test could check there is no compose box — that'd make it robust to a hypothetical future where one particular narrow grows a compose box, by ensuring that in that case we'd update the test so it continues to be effective for any other narrows that lack a compose box.

Yeah, this interpretation makes sense me. I decided to remove the MentionsNarrow test, because I think they will be largely verifying the same code path. It is reasonable to check for the absence of compose box in different narrows, but we are more interested in testing the inset fix here.

PIG208 · 2024-08-21T04:23:57Z

Updated the PR. Thanks for the review!

Made separate functions for one test each in action_sheet_test.dart and message_list_test.dart, and removed the extra regression test. I also squashed the later commits together as suggested.

gnprice

Thanks for the revision!

A few new nits below — otherwise all looks good. I'll fix those up and merge.

gnprice · 2024-08-21T21:41:23Z

test/widgets/action_sheet_test.dart

-      final message = eg.streamMessage();
+      final message = eg.streamMessage(flags: [MessageFlag.mentioned]);
      await setupToMessageActionSheet(tester, message: message, narrow: const MentionsNarrow());


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.

gnprice · 2024-08-21T21:43:16Z

lib/model/message.dart

+          // TODO(#818): Support MentionsNarrow live-updates when handling
+          // @-mentioned flags.


nit: indent any continuation lines on a TODO comment:

Suggested change

// TODO(#818): Support MentionsNarrow live-updates when handling

// @-mentioned flags.

// TODO(#818): Support MentionsNarrow live-updates when handling

// @-mentioned flags.

(also "@-mentioned flags" reads to me as about flags that were @-mentioned; could say "mention flags" or "@-mention flags")

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

Otherwise this message wouldn't be expected to appear in the messages fetched for MentionsNarrow. Signed-off-by: Zixuan James Li <[email protected]>

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 <[email protected]>

gnprice · 2024-08-21T21:54:36Z

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 {


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.

…ion 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.

"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 <[email protected]>

gnprice · 2024-08-21T22:08:23Z

I'll fix those up and merge.

Done. I also added a commit to better pin down that existing regression test's assumptions:
5bcb5d4 msglist test [nfc]: Pin down lack of compose box in no-insets regression test

PIG208 force-pushed the starred branch from 8fb007f to c6cf517 Compare August 8, 2024 00:36

PIG208 changed the title ~~WIP narrow: Add starred messages narrow~~ narrow: Add starred messages narrow Aug 8, 2024

PIG208 marked this pull request as ready for review August 8, 2024 00:36

PIG208 force-pushed the starred branch from c6cf517 to 529b7da Compare August 8, 2024 00:41

PIG208 commented Aug 8, 2024

View reviewed changes

PIG208 added the maintainer review PR ready for review by Zulip maintainers label Aug 8, 2024

PIG208 requested a review from chrisbobbe August 8, 2024 17:49

PIG208 force-pushed the starred branch 2 times, most recently from 6604aff to 6784d55 Compare August 12, 2024 19:03

PIG208 force-pushed the starred branch from 6784d55 to d72b5dc Compare August 12, 2024 22:35

PIG208 force-pushed the starred branch from d72b5dc to d0b3abb Compare August 13, 2024 18:55

PIG208 force-pushed the starred branch from d0b3abb to 8c36dfc Compare August 15, 2024 06:23

chrisbobbe requested changes Aug 15, 2024

View reviewed changes

chrisbobbe mentioned this pull request Aug 15, 2024

Show poll-failure details #868

Merged

PIG208 force-pushed the starred branch from 8c36dfc to efc2092 Compare August 15, 2024 23:44

PIG208 force-pushed the starred branch 2 times, most recently from 689e153 to bf0759d Compare August 15, 2024 23:55

chrisbobbe added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Aug 17, 2024

gnprice reviewed Aug 20, 2024

View reviewed changes

PIG208 force-pushed the starred branch 3 times, most recently from 36272bf to 6c79685 Compare August 21, 2024 04:20

gnprice reviewed Aug 21, 2024

View reviewed changes

PIG208 added 3 commits August 21, 2024 14:49

message [nfc]: Mention that @-mention live-updates have not yet been …

a2dc3ef

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

action_sheet test: Create message with the appropriate flag.

115a5e5

Otherwise this message wouldn't be expected to appear in the messages fetched for MentionsNarrow. Signed-off-by: Zixuan James Li <[email protected]>

gnprice reviewed Aug 21, 2024

View reviewed changes

PIG208 force-pushed the starred branch from 6c79685 to 94a4229 Compare August 21, 2024 22:06

narrow: Add starred messages narrow.

9daa940

"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 <[email protected]>

PIG208 force-pushed the starred branch from 94a4229 to 1e17ca1 Compare August 21, 2024 22:07

gnprice force-pushed the starred branch from 1e17ca1 to 9daa940 Compare August 21, 2024 22:07

gnprice merged commit 9daa940 into zulip:main Aug 21, 2024

PIG208 deleted the starred branch August 21, 2024 22:10

PIG208 mentioned this pull request Aug 26, 2024

msglist: Offer starred-messages narrow #251

Closed

PIG208 mentioned this pull request Oct 29, 2024

msglist: Follow /with/ links through message moves #1029

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

narrow: Add starred messages narrow #870

narrow: Add starred messages narrow #870

PIG208 commented Aug 7, 2024 •

edited

Loading

PIG208 commented Aug 8, 2024 •

edited

Loading

PIG208 left a comment

PIG208 commented Aug 8, 2024

chrisbobbe commented Aug 12, 2024

PIG208 commented Aug 12, 2024

chrisbobbe commented Aug 13, 2024

PIG208 commented Aug 13, 2024

PIG208 commented Aug 15, 2024

chrisbobbe left a comment

chrisbobbe Aug 14, 2024

chrisbobbe Aug 14, 2024

chrisbobbe Aug 14, 2024

chrisbobbe Aug 15, 2024

PIG208 commented Aug 15, 2024

PIG208 commented Aug 15, 2024

PIG208 commented Aug 15, 2024

chrisbobbe commented Aug 17, 2024

gnprice left a comment

gnprice Aug 20, 2024

gnprice Aug 20, 2024

PIG208 Aug 21, 2024

gnprice Aug 20, 2024

PIG208 Aug 21, 2024

PIG208 commented Aug 21, 2024

gnprice left a comment

gnprice Aug 21, 2024

gnprice Aug 21, 2024

gnprice Aug 21, 2024

gnprice Aug 21, 2024

gnprice commented Aug 21, 2024

		("Combined feed", const CombinedFeedNarrow(), <MessageFlag>[]),
		("MentionsNarrow", const MentionsNarrow(), [MessageFlag.mentioned]),

		// TODO(#818): Support MentionsNarrow live-updates when handling
		// @-mentioned flags.

narrow: Add starred messages narrow #870

narrow: Add starred messages narrow #870

Conversation

PIG208 commented Aug 7, 2024 • edited Loading

PIG208 commented Aug 8, 2024 • edited Loading

PIG208 left a comment

Choose a reason for hiding this comment

PIG208 commented Aug 8, 2024

chrisbobbe commented Aug 12, 2024

PIG208 commented Aug 12, 2024

chrisbobbe commented Aug 13, 2024

PIG208 commented Aug 13, 2024

PIG208 commented Aug 15, 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

Choose a reason for hiding this comment

PIG208 commented Aug 15, 2024

PIG208 commented Aug 15, 2024

PIG208 commented Aug 15, 2024

chrisbobbe commented Aug 17, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 commented Aug 21, 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

Choose a reason for hiding this comment

gnprice commented Aug 21, 2024

PIG208 commented Aug 7, 2024 •

edited

Loading

PIG208 commented Aug 8, 2024 •

edited

Loading