Handle moved messages in MessageStore and message lists

[gnprice]

Fixes most of #150. (After this lands we should do a quick scan to identify anything else that needs updating on message moves — there are several new data structures in the store that we've merged in recent weeks.)

A couple of months ago I spent a few hours writing part of an implementation of #150. It's mostly complete, apart from the lack of tests… and any bugs it has that will be found by writing those tests.

@PIG208 since you're picking up #150 shortly, I spent a small amount of time just now cleaning up that draft branch and rebasing atop the changes from #750, and writing down what remains to be done to finish it. So please start from this, which will let you avoid duplicating that work.

I believe there are three remaining things this needs:

• Tests. As usual, these should go in the same commit that makes the changes.

  There's a variety of kinds of test cases this needs,
  but here's one in particular that may not be apparent:

  If the generation += 1 line is commented out, the message list
  has a race bug where a fetchOlder starts; we reset (because
  messages were moved into the narrow); and then the fetch returns
  and appends in the wrong spot. Also a related race bug where it's
  fetchInitial, and it lacks the moved messages. So there should be
  test cases exercising each of those situations, which would fail if
  that line gets commented out.

• Update StreamMessage.displayRecipient. This goes in a followup commit.

  If we wanted, we could update it by looking up the new stream's
  name. To enable MessageStoreImpl to do that, we'd pass it a
  StreamStore, the same way we do with Unreads.

  But in fact the only time displayRecipient is useful is when we
  don't have data on that stream. So there's not much point in
  looking it up here. Instead, make the field nullable, and just
  set it to null when the message is moved between streams.

• Handle UpdateMessageEvent.propagateMode. This also goes in a followup commit. If it gets complex (or if the rest of the branch before it gets complex), let's split it out as a followup PR.

  This probably means
  _MessageListPageState having its own notion of the current narrow,
  which is initialized with widget.narrow but can change.
  Then _MessageListState._modelChanged can check if the model's
  narrow has changed, and if so tell the page state to update.

  For comparison, see zulip-mobile's src/events/doEventActionSideEffects.js,
  or web.

[PIG208]

Thanks for the notes. I think I will just push to your branch and continue using this PR.

[PIG208]

Added the test cases.
This is still failing because we always notifies the listener if there are affected message present, and message moves might also notify the listener. I think we want to let the caller handle notifications instead.

[PIG208]

Not sure if we want to skip the finally block as well. At least we shouldn't need to notify the listeners again.

    } finally {
      _fetchingOlder = false;
      _updateEndMarkers();
      notifyListeners();
    }

[gnprice]

In the current revision:

     } finally {
       _fetchingOlder = false;
-      _updateEndMarkers();
-      notifyListeners();
+      if (this.generation == generation) {
+        _updateEndMarkers();
+        notifyListeners();
+      }
     }

The thing is that the reason there's a notifyListeners there, and an _updateEndMarkers, is as a followup to the _fetchingOlder update. Changing _fetchingOlder changes what set of end markers should be present; and when the end markers change, listeners should be notified (so that e.g. the UI can update to reflect the change).

We could get an effect like this, because _fetchingOlder may already be false. But the way to do that would be with an explicit check on _fetchingOlder: no _updateEndMarkers etc. if we're not actually changing the value.

In fact, this actually points to a bug (that was introduced in my draft): if we abort because the generation advanced, then we should not set _fetchingOlder to false. It might happen to be true by the time we reach this point, because the user might have scrolled up again after the new fetchInitial completed, causing a new fetchOlder. If it is true after the generation advanced, then the fact that it's true doesn't belong to this fetchOlder call, but instead to that newer fetchOlder call, and so this fetchOlder call shouldn't be interfering.

So please fix that bug from my draft 🙂, and also add a test that would reproduce the bug.

[PIG208]

Thanks for the elaboration. I think encapsulating _fetchingOlder update with the generation check should be sufficient as a fix.

[gnprice]

This is still failing because we always notifies the listener if there are affected message present, and message moves might also notify the listener.

Hmm I see, so the listeners end up getting notified twice?

I think this is probably a case where we should just be OK with the duplication. Here's what I wrote on a different PR a few weeks ago (#648 (comment)):

Generally I think it's OK to have a little duplication of those calls — for example if some event can cause it to be called two or three times, and it'd take a lot of refactoring or bookkeeping to prevent that and call just once, then that's OK. If some listener callback is expensive enough to make that a problem, then it should have appropriate memoization, or defer the real work until the next frame starts, or something, in order to mitigate that.

But this [in that other PR] is potentially hundreds of times, so that gets me a bit concerned.

(Possibly I should put that in docs somewhere. The way I found it just now is by a Zulip search within #mobile-github — that's been a side benefit I've quite enjoyed from having that GitHub firehose appear in Zulip.)

[PIG208]

This probably means
_MessageListPageState having its own notion of the current narrow,
which is initialized with widget.narrow but can change.
Then _MessageListState._modelChanged can check if the model's
narrow has changed, and if so tell the page state to update.

Relying on the usual updates through dependencies does not work here because MessageListPage is a parent of MessageList. At first I was thinking if we want to push a route for MessageListPage to Navigator (similar to zulip/zulip-mobile#5251), as a way to notify _MessageListPageState from _MessageListState.

But it might not be necessary. An alternative would be having MessageListPage pass a callback function to MessageList, and call it when _modelChanged happens to detect change in narrow.

[gnprice]

having MessageListPage pass a callback function to MessageList, and call it when _modelChanged happens to detect change in narrow.

Yeah, exactly.

We wouldn't want to push a new route — that'd leave the old one still on the nav stack, and make a new _MessageListPage State with a new _MessageListState and MessageListView. What zulip-mobile does is to mutate the route parameters on the existing route. If there's a way to do that with Flutter's navigator API then that could also be a good solution; I haven't looked into that.

[PIG208]

I wonder how this is possibly a move. Does internal move within a topic narrow actually happen?

[gnprice]

Hmm. Yeah, this case seems like it should be a no-op and shouldn't happen. So maybe it should just look like this (in the implementation):

          case (true,  true ): return; // TODO(log) no-op move

[PIG208]

Not sure if this comment warrants dart doc given how it is mainly explaining the motivation for the @JsonKey here.

[gnprice]

Yeah, I think a plain implementation comment (like here) is fine.

[PIG208]

Pushed to fix the CI error.

[chrisbobbe]

Ah it looks like there's a rebase conflict. Could one of you please resolve that; maybe Zixuan?

[PIG208]

Conflict resolved. (It was related to 7a2c9d2).

[PIG208]

This should be ready for review now. Thanks!

[gnprice]

Thanks @PIG208 for the revision! Just a handful of small comments now; then this will be all ready to merge.

[gnprice]

This implicitly assumes that eg.streamMessage will by default pick a topic name of "example topic". That's to be avoided because it makes this test code non-self-contained for the reader — the tests only work because of that arbitrary choice that's made inside eg.streamMessage.

Instead, the test should make explicit that this narrow is supposed to contain the messages that appear later. A straightforward way to do that is to make a local variable topic and use it in all the places that need to be talking about the same topic.

(Probably we should make eg.streamMessage generate the topic name at random, to help prevent this pattern, the same way we do stream/channel IDs and message IDs and so on. From a quick experiment just now, there's a handful of existing tests that would need fixing for that; and none of those assume what the default topic name is, only that it's some constant value, which is also an assumption to avoid making implicit but is a different one.)

[gnprice]

A straightforward way to do that is to make a local variable topic and use it in all the places that need to be talking about the same topic.

The other easy way that comes to mind is to not specify the topic name at all, but get it from the message with message.topic (and/or indirect versions of the same, like TopicNarrow.ofMessage).

[gnprice]

Probably we should make eg.streamMessage generate the topic name at random, to help prevent this pattern, the same way we do stream/channel IDs and message IDs and so on.

Sent #932 for this.

and none of those assume what the default topic name is, only that it's some constant value, which is also an assumption to avoid making implicit but is a different one.

I ended up deciding this assumption was fine, though; it's convenient in a variety of tests. So I made the randomness happen only once per run, and documented that.

[gnprice]

Cool, good idea adding this check — it makes a baseline for the reader to compare the check below to.

[gnprice]

nit:

Suggested change

	..controller.isNotNull().text.equals('Some text');
	prepareGetMessageResponse([message]);
	..controller.isNotNull().text.equals('Some text');
	prepareGetMessageResponse([message]);

Generally in a test case when there's a sequence like

• make something happen or change
• check the state of the world
• make something else happen or change
• check the new state of the world
• etc.

I find it helpful, for the reader, to group those change/check pairs into stanzas using blank lines.

[gnprice]

nit: group this with the other _messagesMoved… methods, above _handlePropagateMode

[PIG208]

Thanks. Updated the PR.

[gnprice]

Thanks for the revision! Looks good — merging.

I made a few commit-message tweaks, all in the summary prefixes:

-    model: Expect the absence of newTopic for certain moves.
+    message: Expect the absence of newTopic for certain moves.

-    message_test [nfc]: Rename message move test for accuracy.
+    message test [nfc]: Rename message move test for accuracy.

-    model [nfc]: Make displayRecipient nullable.
+    message [nfc]: Make displayRecipient nullable.

The "test" modifier in a summary prefix is always separated by a space, not underscore.

Rather than "model", these can be more specific by saying "message", referring to the MessageStore part of the model. (The first commit is all about that code directly. The later displayRecipient commit doesn't touch that code itself… but the change it's making to Message can be thought of as being entirely for the purpose of Message's role in MessageStore, where those objects get mutated.)