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.

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.

…rrows Not NFC just because the test is identified differently in the output (under a new "smoke" group, and with the name "CombinedFeedNarrow").

Fixes: zulip#1028

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

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 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 added 7 commits October 30, 2024 10:21

msglist test: Make groups for fetchInitial and fetchOlder tests

9958ae1

msglist test: Refactor fetchInitial smoke test, preparing for more na…

a8dd4da

…rrows Not NFC just because the test is identified differently in the output (under a new "smoke" group, and with the name "CombinedFeedNarrow").

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

d49d62a

msglist test [nfc]: Factor out findAppBarTitleWithChannelTopic helper

9822bc5

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

ff6cc8a

msglist: Follow /with/ links through message moves

0131961

Fixes: zulip#1028

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

chrisbobbe commented Oct 26, 2024

PIG208 left a comment

PIG208 Oct 29, 2024

PIG208 Oct 29, 2024

PIG208 Oct 29, 2024

PIG208 Oct 29, 2024

chrisbobbe Oct 30, 2024

PIG208 Oct 30, 2024 •

edited

Loading

PIG208 Oct 30, 2024

chrisbobbe Oct 30, 2024

PIG208 Oct 29, 2024

chrisbobbe commented Oct 30, 2024

PIG208 commented Oct 30, 2024

gnprice left a comment

gnprice Nov 1, 2024

gnprice Nov 1, 2024

gnprice Nov 1, 2024

gnprice Nov 2, 2024

gnprice Nov 2, 2024

gnprice Nov 2, 2024

gnprice Nov 2, 2024

gnprice Nov 2, 2024

gnprice Nov 2, 2024

gnprice Nov 2, 2024

	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 {

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

Are you sure you want to change the base?

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

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

edited

Loading