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

chrisbobbe · 2024-10-26T00:25:02Z

PIG208

Looks good overall! Left some comments.

PIG208 · 2024-10-29T18:48:32Z

test/model/message_list_test.dart

+        generateMessages: (i) => eg.streamMessage(
+          stream: someStream,
+          topic: someTopic)),
+    ];


Maybe we can refactor these test cases (and the ones added later) with a helper that takes these arguments. See #870 (comment).

PIG208 · 2024-10-29T18:50:05Z

test/example_data.dart

-const String recentZulipVersion = '8.0';
-const int recentZulipFeatureLevel = 185;
+const String recentZulipVersion = '9.0';
+const int recentZulipFeatureLevel = 271;


We should be able to drop this now.

PIG208 · 2024-10-29T19:04:03Z

test/model/message_list_test.dart

+        final fetchFuture = model.fetchInitial();
+        check(model).fetched.isFalse();
+
+        checkNotNotified();
+        await fetchFuture;


Looks like we can use await model.fetchInitial() here and the test below

PIG208 · 2024-10-29T20:09:06Z

lib/model/message_list.dart

+  ///
+  /// See API doc:
+  ///   https://zulip.com/api/construct-narrow#message-ids
+  void _adjustTopicPermalinkNarrow(Narrow narrow_, Message? someFetchedMessageOrNull) {


Looks like the caller passes the same narrow as the narrow that lives on the message list. Should we remove narrow_ and just use narrow instead?

I agree it's odd for this function to have a narrow_ param; let's remove that.

It's convenient in the function's body to not keep using narrow each time we need it, because then we'd have to repeat things like narrow as TopicNarrow even in places where it's clear to us that the value can't be anything other than a TopicNarrow. I think what's happening there is: when we read narrow, the analyzer sees that as calling a getter, not accessing a variable, and it doesn't assume the getter will return the same value / Narrow subtype from one call to the next. And while in theory the analyzer could conclude that this class's narrow getter doesn't have that surprising behavior—it's just a field with the getter defined implicitly—it doesn't want to go and check for subclasses that might override the getter with that surprising behavior.

How about a narrow_ variable at the top of the function body, with a comment.

Yeah, that explanation makes sense. I remember that making narrow private might fix the issue, but I'm not sure. The proposed change sounds good to me too.

Hmm nevermind, I think the local variable would be a better solution here.

See #808 (comment)

Interesting; I like final narrow = this.narrow in that example; I think I like it better than final narrow_ = narrow.

PIG208 · 2024-10-29T20:22:29Z

test/widgets/message_list_test.dart

  }) async {
    addTearDown(testBinding.reset);
    streams ??= subscriptions ??= [eg.subscription(eg.stream(streamId: eg.defaultStreamMessageStreamId))];
    await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot(
      streams: streams, subscriptions: subscriptions, unreadMsgs: unreadMsgs));
    store = await testBinding.globalStore.perAccount(eg.selfAccount.id);
    connection = store.connection as FakeApiConnection;
+    if (zulipFeatureLevel != null) {
+      connection.zulipFeatureLevel = zulipFeatureLevel;


Hmm, I think eg.selfAccount carries a different zulipFeatureLevel if we only override the feature level on the connection.

chrisbobbe · 2024-10-30T20:12:43Z

Thanks for the review! Revision pushed.

PIG208 · 2024-10-30T20:27:35Z

Thanks! This revision looks good to me.

gnprice

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

Generally this looks great. Comments below, most of them small.

gnprice · 2024-11-01T23:43:01Z

test/model/message_list_test.dart

-      ..messages.length.equals(30)
-      ..haveOldest.isTrue();
-  });
+    group('smoke', () {


nit:

msglist test: Refactor fetchInitial smoke test, preparing for more narrows Not NFC just because the test is identified differently in the output (under a new "smoke" group, and with the name "CombinedFeedNarrow").

We've sometimes gone back and forth on this, I think; but I'd rather consider this kind of change NFC. So the names of the test cases, and their grouping, don't count as "functional", even though they are observable with flutter test.

I think a good definition of what does count as "functional", for test code, is: a functional change is one that could affect whether the tests would pass or fail, on some hypothetical future change to the code under test.

gnprice · 2024-11-01T23:46:08Z

test/widgets/message_list_test.dart

  }) async {
    addTearDown(testBinding.reset);
    streams ??= subscriptions ??= [eg.subscription(eg.stream(streamId: eg.defaultStreamMessageStreamId))];
-    await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot(
+    final effectiveFeatureLevel = zulipFeatureLevel ?? eg.recentZulipFeatureLevel;


nit:

Suggested change

final effectiveFeatureLevel = zulipFeatureLevel ?? eg.recentZulipFeatureLevel;

zulipFeatureLevel ??= eg.recentZulipFeatureLevel;

and then subsequent lines get to say zulipFeatureLevel: zulipFeatureLevel instead of needing a different, longer name

Interesting; I'll keep this pattern in mind. I'd always thought it was a code smell to reassign a function param.

Yeah — just like mutating a local variable, it requires a bit of care to avoid making things confusing. But I think doing so (as here) right at the top of the function body, where it's effectively still parsing its arguments, is one pattern where it's generally pretty clean.

In this case it's also what this function is already doing with two other parameters on the previous line, so this might as well follow the same pattern.

gnprice · 2024-11-01T23:53:21Z

lib/api/model/narrow.dart

  }
+  if (!hasDmElement && !hasWithElement) return narrow;


The goal of the corresponding early return in the existing code is so we don't make a copy of the list in cases where we don't need to. So we should preserve that property.

In this version, I think it will end up making a no-op copy if there's a /with/ element on a recent server.

gnprice · 2024-11-02T00:02:35Z

lib/api/model/narrow.dart

+    }).toList();
+  }
+  if (hasWithElement && !supportsOperatorWith) {
+    result.removeWhere((element) => element is ApiNarrowWith);


This mutates result. If the argument narrow didn't happen to have an ApiNarrowDm in it, then that mutates the argument.

gnprice · 2024-11-02T00:04:45Z

lib/api/model/narrow.dart

+  ApiNarrow result = narrow;
+  if (hasDmElement) {
+    result = narrow.map((element) => switch (element) {
+      ApiNarrowDm() => element.resolve(legacy: !supportsOperatorDm),
+      _             => element,
+    }).toList();
+  }
+  if (hasWithElement && !supportsOperatorWith) {
+    result.removeWhere((element) => element is ApiNarrowWith);
+  }


I think these are probably cleanest as one loop that builds up result from empty, iterating through narrow.

(By the time we reach this point we should already know that the loop is going to end up producing a result that's different from narrow, so we can't avoid producing a fresh list and don't need further conditions like if (hasDmElement) as an optimization.)

gnprice · 2024-11-02T00:30:51Z

lib/model/message_list.dart

+      case StreamMessage(:final streamId, :final topic):
+        this.narrow = TopicNarrow(streamId, topic);


nit:

Suggested change

case StreamMessage(:final streamId, :final topic):

this.narrow = TopicNarrow(streamId, topic);

case StreamMessage():

this.narrow = TopicNarrow.ofMessage(someFetchedMessageOrNull);

This feels a bit more direct.

gnprice · 2024-11-02T00:43:09Z

test/model/message_list_test.dart

+        check(model).narrow.isA<TopicNarrow>()
+          ..streamId.equals(otherStream.streamId)
+          ..topic.equals(otherTopic)
+          ..with_.isNull();


nit:

Suggested change

check(model).narrow.isA<TopicNarrow>()

..streamId.equals(otherStream.streamId)

..topic.equals(otherTopic)

..with_.isNull();

check(model).narrow

.equals(TopicNarrow(otherStream.streamId, otherTopic));

TopicNarrow has an == override, so we may as well use it here.

gnprice · 2024-11-02T00:45:14Z

test/model/message_list_test.dart

+        checkNotNotified();
+        await model.fetchInitial();


nit:

Suggested change

checkNotNotified();

await model.fetchInitial();

await model.fetchInitial();

and no blank line above

The checkNotNotified is redundant because we haven't done anything yet up to that point. (We called prepare, but it ends by ensuring the notification count is clean; other test cases rely on that too.)

The connection.prepare belongs right before the fetchInitial call, because it's preparing for a request that call makes.

gnprice · 2024-11-02T00:53:23Z

test/widgets/message_list_test.dart

+      for (final useLegacy in [false, true]) {
+        testWidgets('without message move; ${useLegacy ? 'legacy' : 'modern'}', (tester) async {


The legacy case doesn't really change anything in this test, does it? It seems like all it does is mean we don't send the with element to the server.

The fact that we don't send that is already covered by the API tests. So we can leave it out here.

gnprice · 2024-11-02T01:02:13Z

lib/api/model/narrow.dart

-  if (!narrow.any((element) => element is ApiNarrowDm)) {
-    return narrow;
+// TODO(server-7) remove [ApiNarrowDm] reference in dartdoc
+ApiNarrow resolveApiNarrowElements(ApiNarrow narrow, int zulipFeatureLevel) {


This name doesn't sound right to me for the expanded scope of this function. (In particular "elements" in the name feels redundant; it doesn't seem to convey any more information than resolveApiNarrow would.)

Here's a version:

/// Adapt the given narrow to be sent to the given Zulip server version. /// /// Any elements that take a different name on old vs. new servers /// will be resolved to the specific name to use. /// Any elements that are unknown to old servers and can /// reasonably be omitted will be omitted. ApiNarrow resolveApiNarrowForServer(ApiNarrow narrow, int zulipFeatureLevel) {

chrisbobbe · 2025-02-14T20:48:10Z

Thanks for the review and patience! Revision pushed. 🎉

gnprice

Thanks for the revision! I think this is close; comments below.

gnprice · 2025-02-14T23:53:32Z

test/model/message_list_test.dart

-    ).toJson());
-    final fetchFuture = model.fetchInitial();
-    check(model).fetched.isFalse();
+  group('fetchInitial', () {


nit:

msglist test: Make groups for fetchInitial and fetchOlder tests

NFC, similarly to #1029 (comment)

gnprice · 2025-02-15T00:03:53Z

lib/api/model/narrow.dart

+      case ApiNarrowWith():
+        assert(!supportsOperatorWith);


What if there's both an ApiNarrowWith and an ApiNarrowDm? Then I think this assertion can fail.

gnprice · 2025-02-15T00:06:46Z

lib/api/model/narrow.dart

+  if (!hasDmElement && (!hasWithElement || supportsOperatorWith)) {
    return narrow;


Related to my previous comment just below: this condition ought to be equivalent to "the loop below will produce a result different from narrow" — that's the basis for the optimization this represents.

I think this condition is correct but the logic below doesn't match it.

Ah right—thanks for the catch.

gnprice · 2025-02-15T00:18:29Z

lib/model/internal_link.dart

+  ApiNarrowWith? withElement;
  ApiNarrowDm? dmElement;


nit: put these, and the cases below, in the same order as each other and as the respective ApiNarrowElement subclasses

gnprice · 2025-02-15T00:20:00Z

lib/model/internal_link.dart

        continue;

+      case _NarrowOperator.with_:
+        if (withElement != null) return null;
+        withElement = ApiNarrowWith(int.parse(operand, radix: 10));


on parse failure should return null, not throw

gnprice · 2025-02-15T00:30:41Z

lib/model/message_list.dart

+        // and if it is, it'll appear in the result, making it non-empty.)
+        this.narrow = narrow.sansWith();
+      case StreamMessage():
+        this.narrow = TopicNarrow.ofMessage(someFetchedMessageOrNull);


Over on the widget state there's a comment which should get updated:

void _modelChanged() { if (model!.narrow != widget.narrow) { // A message move event occurred, where propagate mode is // [PropagateMode.changeAll] or [PropagateMode.changeLater]. widget.onNarrowChanged(model!.narrow);

Ooh thanks for the catch.

gnprice · 2025-02-15T00:34:07Z

test/model/message_list_test.dart

-      numAfter: 0,
-    );
-  });
+    final otherStream = eg.stream();


nit: otherChannel, to match the neighboring someChannel

gnprice · 2025-02-15T00:39:30Z

test/model/message_list_test.dart

-    final fetchFuture = model.fetchOlder();
-    checkNotifiedOnce();
-    check(model).fetchingOlder.isTrue();
+      test('topic permalink, message was moved', () async {


I think this test case and the one above it can probably be left out — it doesn't seem like they add anything that isn't covered either by the neighboring tests, or the two test cases added below.

gnprice · 2025-02-15T00:44:42Z

test/widgets/message_list_test.dart

+            'narrow': jsonEncode([
+              ApiNarrowStream(someStream.streamId),
+              ApiNarrowTopic(eg.t(someTopic)),
+              ApiNarrowWith(1),
+            ]),


This can use narrow.apiEncode() like the other test case does, right? Or is there a difference I'm missing?

Oh right! That makes sense.

gnprice · 2025-02-15T00:46:52Z

test/widgets/message_list_test.dart

+          // server sends the /with/<id> message in its current, different location
+          messages: [eg.streamMessage(id: 1, stream: otherStream, topic: otherTopic)],
+          streams: [someStream, otherStream],
+          subscriptions: [eg.subscription(someStream), eg.subscription(otherStream)],


nit: I think the subscriptions aren't needed here

(some other tests need them because they use CombinedFeedNarrow)

chrisbobbe · 2025-02-15T02:10:29Z

Thanks for the review! Revision pushed, this time with tests for resolveApiNarrowForServer, since some bugs snuck in (thank you for catching them in #1029 (comment) !).

gnprice

Thanks! Looks good; just a few comments.

gnprice · 2025-02-19T06:32:01Z

test/api/model/narrow_test.dart

+      final zulipFeatureLevel = 176;
+      doTest('dm (legacy)', zulipFeatureLevel: zulipFeatureLevel,
+        [dm], [dm.resolve(legacy: true)]);
+      doTest('topic, not permalink', [stream, topic], [stream, topic]);


Is this meant to pass zulipFeatureLevel too?

gnprice · 2025-02-19T06:37:08Z

lib/api/model/narrow.dart

@@ -150,6 +173,22 @@ class ApiNarrowPmWith extends ApiNarrowDm {
  ApiNarrowPmWith._(super.operand, {super.negated});
 }

+/// An [ApiNarrowElement] with the 'with' operator.
+///
+/// If part of [ApiNarrow] use [resolveApiNarrowElements].


nit: update name for revision

gnprice · 2025-02-19T06:38:19Z

lib/model/internal_link.dart

@@ -78,6 +78,8 @@ Uri narrowLink(PerAccountStore store, Narrow narrow, {int? nearMessageId}) {
        fragment.write('$streamId-$slugifiedName');
      case ApiNarrowTopic():
        fragment.write(_encodeHashComponent(element.operand.apiName));
+      case ApiNarrowWith():


nit: match order of cases

gnprice · 2025-02-19T06:42:15Z

test/api/model/narrow_test.dart

+import 'model_checks.dart';
+
+void main() {
+  group('resolveApiNarrowForServer', () {


This should get a comment pointing to the existing set of tests that cover this method, which are in test/api/route/messages_test.dart, particularly the Narrow.toJson test.

Ah indeed! Thanks for pointing out that existing coverage—which I see I even added to, since an earlier revision, but forgot about that because I wrote that revision a while ago. 🙂

gnprice · 2025-02-19T06:43:32Z

(Oh and there are some unused imports the analyzer is complaining about.)

chrisbobbe · 2025-02-20T02:41:36Z

Thanks for the review! Revision pushed.

gnprice

Thanks for the revision! Looks good; a few small comments.

gnprice · 2025-02-20T07:10:59Z

test/api/route/messages_test.dart

+        {'operator': 'pm-with', 'operand': [123, 234]},
+      ]));
+
+      connection.zulipFeatureLevel = 270;


nit: put this section above the FL 170 section — that way there's more of a monotonic range here from newest to oldish to older

gnprice · 2025-02-20T07:12:29Z

test/api/route/messages_test.dart

      ]));
+
+      // Unlikely to occur in the wild but should still be handled correctly
+      checkNarrow([ApiNarrowDm([123, 234]), ApiNarrowWith(1)], jsonEncode([


Suggested change

]));

// Unlikely to occur in the wild but should still be handled correctly

checkNarrow([ApiNarrowDm([123, 234]), ApiNarrowWith(1)], jsonEncode([

]));

checkNarrow([ApiNarrowDm([123, 234]), ApiNarrowWith(1)], jsonEncode([

I think the apology isn't needed 🙂

gnprice · 2025-02-20T07:14:40Z

test/api/route/messages_test.dart

      ]));

      connection.zulipFeatureLevel = 176;
+
+      checkNarrow(eg.topicNarrow(12, 'stuff', with_: 1).apiEncode(), jsonEncode([


nit: and then without those other comments, the change to the ZFL can be comfortably kept in the same stanza with the checks that rely on it

Suggested change

]));

connection.zulipFeatureLevel = 176;

checkNarrow(eg.topicNarrow(12, 'stuff', with_: 1).apiEncode(), jsonEncode([

]));

connection.zulipFeatureLevel = 176;

checkNarrow(eg.topicNarrow(12, 'stuff', with_: 1).apiEncode(), jsonEncode([

I think that substantially helps to mitigate the risk of confusion from this use of mutable state, where one otherwise might see one of those later checkNarrow calls in isolation and wonder why the expected behavior is what it is

gnprice · 2025-02-20T07:20:27Z

lib/api/model/narrow.dart

+      default:
+    }
+  }
+  if (!hasDmElement && (!hasWithElement || supportsOperatorWith)) {


nit: use a De Morgan transform of this:

Suggested change

if (!hasDmElement && (!hasWithElement || supportsOperatorWith)) {

if (!hasDmElement && !(hasWithElement && !supportsOperatorWith)) {

That way it more directly corresponds to the condition that's written in the switch cases below.

(You could even then apply De Morgan to the outer operator too — write "not (A or B)" instead of "not-A and not-B" — and that might be clearer still.)

And refactor the implementation to prepare for a new [ApiNarrow] feature. We're about to add ApiNarrowWith, which new in Zulip Server 9. This function seems like a good place to resolve ApiNarrows into the expected form for servers before 9 (without "with") and 9 or later (with "with"). First we have to make its name and dartdoc more generic, as done here.

…arrows

We'll use this for some new tests, coming up. Also use it now for one test that's only been checking the topic in the app bar. That test now checks the channel name too, as it was before b1767b2, which dropped that specificity when we split the app bar's channel name and topic onto two rows.

…checks And fix the name of the test.

Fixes: zulip#1028

chrisbobbe · 2025-02-21T00:13:13Z

Thanks! Revision pushed.

gnprice · 2025-02-21T00:22:12Z

Thanks! Looks good; merging.

chrisbobbe added the maintainer review PR ready for review by Zulip maintainers label Oct 26, 2024

chrisbobbe requested a review from PIG208 October 26, 2024 00:25

chrisbobbe assigned PIG208 Oct 26, 2024

PIG208 reviewed Oct 29, 2024

View reviewed changes

chrisbobbe force-pushed the pr-follow-with-links branch from 08b8e2f to 0131961 Compare October 30, 2024 20:12

PIG208 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 Oct 30, 2024

PIG208 assigned gnprice and unassigned PIG208 Oct 30, 2024

gnprice reviewed Nov 2, 2024

View reviewed changes

chrisbobbe force-pushed the pr-follow-with-links branch from 0131961 to 6e195fa Compare February 14, 2025 20:47

gnprice reviewed Feb 15, 2025

View reviewed changes

chrisbobbe force-pushed the pr-follow-with-links branch from 6e195fa to b17769f Compare February 15, 2025 02:09

gnprice reviewed Feb 19, 2025

View reviewed changes

chrisbobbe force-pushed the pr-follow-with-links branch from b17769f to 03c261f Compare February 20, 2025 02:40

gnprice reviewed Feb 20, 2025

View reviewed changes

chrisbobbe mentioned this pull request Feb 21, 2025

action_sheet: Offer "Mark channel as read" in channel action sheet #1317

Open

chrisbobbe added 7 commits February 20, 2025 16:04

msglist test [nfc]: Make groups for fetchInitial and fetchOlder tests

48da972

msglist test [nfc]: Refactor fetchInitial smoke, preparing for more n…

1b59036

…arrows

msglist test: Add fetchInitial smoke test that uses a topic narrow

c655631

msglist test [nfc]: Remove indentation on an empty line

327ae54

msglist test [nfc]: Add feature-level param to setupMessageListPage

a458ac2

chrisbobbe added 2 commits February 20, 2025 16:04

api test [nfc]: Separate some checks on "special" narrows from other …

5f3a682

…checks And fix the name of the test.

msglist: Follow /with/ links through message moves

c8494a8

Fixes: zulip#1028

chrisbobbe force-pushed the pr-follow-with-links branch from 03c261f to c8494a8 Compare February 21, 2025 00:13

gnprice merged commit c8494a8 into zulip:main Feb 21, 2025
1 check passed

	final effectiveFeatureLevel = zulipFeatureLevel ?? eg.recentZulipFeatureLevel;
	zulipFeatureLevel ??= eg.recentZulipFeatureLevel;

		case StreamMessage(:final streamId, :final topic):
		this.narrow = TopicNarrow(streamId, topic);

	checkNotNotified();
	await model.fetchInitial();
	await model.fetchInitial();

		for (final useLegacy in [false, true]) {
		testWidgets('without message move; ${useLegacy ? 'legacy' : 'modern'}', (tester) async {

		if (!hasDmElement && (!hasWithElement \|\| supportsOperatorWith)) {
		return narrow;

	if (!hasDmElement && (!hasWithElement \|\| supportsOperatorWith)) {
	if (!hasDmElement && !(hasWithElement && !supportsOperatorWith)) {

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

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

Conversation

chrisbobbe commented Oct 26, 2024

PIG208 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 Oct 30, 2024 • edited Loading

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

chrisbobbe commented Oct 30, 2024

PIG208 commented Oct 30, 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

gnprice Feb 14, 2025 • edited Loading

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

chrisbobbe commented Feb 14, 2025

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

chrisbobbe commented Feb 15, 2025

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

gnprice commented Feb 19, 2025

chrisbobbe commented Feb 20, 2025

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

chrisbobbe commented Feb 21, 2025

gnprice commented Feb 21, 2025

PIG208 Oct 30, 2024 •

edited

Loading

gnprice Feb 14, 2025 •

edited

Loading