Add a central message store #648

gnprice · 2024-05-08T05:44:43Z

This is a draft branch. I think I've run out of time I can spend on this area personally right now, but I've sketched out the remaining things that need to happen. So this would be a good branch for someone else to pick up and try to finish from here.

This branch adds a central store.messages map with a single canonical Message object for each of the messages we have the details of. As a result each of the MessageListView view-models just points to those same Message objects, rather than having its own copies (which at present are sometimes duplicated but other times shared with other message lists, causing #455); and when we get any of various events that mean we should update a message, we can do so just once instead of doing so separately on each message list.

This fixes #455. It also gives us a data structure we're likely to want in some other contexts: a map of all the messages. And conversely, it gives us a natural way to report newly-fetched messages to miscellaneous data structures that need them; see the commit that adds TODOs for #649 and #650.

The remaining pieces needed are:

Add more tests, described in the TODOs in the "wip" commits.
Convert and move some of the existing tests in message_list_test.dart to the new message_test.dart. I've done this for the first area — handling ReactionEvent — but it needs doing for UpdateMessageEvent as described in the latter's commit.
Deduplicate the handling of UpdateMessageFlagsEvent. This will be similar in spirit to ReactionEvent and UpdateMessageEvent, which appear in the existing branch. It'll similarly involve converting some existing tests, and writing a few new tests.

gnprice · 2024-05-08T05:51:23Z

(Oh, and the issue that initially got me looking at this area this week was #150, handling moved messages. I think this central message store will be a prerequisite for handling that in a sane way.)

chrisbobbe

Cool, thanks for doing this! Two comments below (including one question), and then would you like me to take this draft PR from here?

chrisbobbe · 2024-05-08T18:33:46Z

lib/model/message.dart

+  /// The list's length will not change, but some entries may be replaced
+  /// by a different [Message] object with the same [Message.id].
+  /// All [Message] objects in the resulting list will be present in [messages].


Hmm. Isn't "the resulting list" actually [messages] itself? And so does this basically mean, "All [Message] objects in [messages] after the mutation will have been present before the mutation"? If that's true, I don't think the first sentence can be true too.

Hmm, this can probably be clearer. 🙂

The "resulting list" is indeed the function's parameter, messages.

But by [messages] here I meant the field (or getter), aka this.messages. (I actually didn't notice when I wrote this that there was a name collision.)

So to unpack: when the function is done, all the Message objects then found in the list will also be present in the messages map on this message store.

Cool, makes sense. So maybe we just change the last word in that sentence from [messages] to [this.messages]?

chrisbobbe · 2024-05-08T18:55:18Z

test/model/message_test.dart

@@ -2,24 +2,68 @@ import 'package:checks/checks.dart';
 import 'package:test/scaffolding.dart';
 import 'package:zulip/api/model/events.dart';
 import 'package:zulip/api/model/model.dart';
+import 'package:zulip/model/message_list.dart';


message test [nfc]: Add a MessageListView, optionally Some of the existing tests of MessageListView in message_list_test.dart are really almost entirely about acting on individual messages rather than message lists. They therefore belong as tests of the overall message store, to live in this file. With that "almost", though, they have a few ties to actually testing the MessageListView itself -- notably, its notification behavior. So to simplify converting them and moving them into this file, we add a MessageListView they can use. This logic is largely copied from message_list_test.dart.

(Ah: by "notification behavior", you must be talking about calls to MessageListView.notifyListeners 😅. At first I thought about push notifications and got confused.)

Indeed 🙂

gnprice · 2024-05-15T00:08:22Z

We discussed this in the office last week, but so it's visible in the thread: yes, please do pick this up from here. Thanks!

chrisbobbe · 2024-05-31T04:33:08Z

Thanks again for your work and discussion to guide me on this! Revision pushed.

One thing I noticed was that the zero-message-list symptom you mentioned in the message-edits commit—

    Here, the previously-buggy behavior is arguably a bit of an inverse
    of 455, but fundamentally the same issue: if there are currently
    *zero* message lists that contain the message, then we would drop
    the update.  Then on later fetching the same message, we'd use the
    version we had (for the good reason explained in reconcileMessages)
    but in fact it'd be stale.

—also applies to the other kinds of message updates: adding/removing reactions, and adding/removing flags. So I've included regression tests for this symptom in all of these kinds of message updates.

gnprice

Thanks for all these revisions! Generally this looks good; various comments below.

gnprice · 2024-05-31T21:05:39Z

lib/model/message.dart

+/// The portion of [PerAccountStore] for messages and message lists.
+mixin MessageStore {
+  /// All known messages, indexed by [Message.id].
+  Map<int, Message> get messages;


message: Store all messages centrally In itself, this exacerbates 455 by making it trigger in more circumstances. That's OK for the moment, because this also gives us a foundation on which to fix that issue soon.

nit in commit message: s/455/#455/

(it was the bare number in my draft too, but with a "wip" message; I have a habit of leaving the # out in drafts in order to reduce the clutter of backlinks in the linked issue thread.)

There's a few more of these; try git log origin.. --grep 455 to find them.

gnprice · 2024-05-31T21:06:50Z

lib/model/message_list.dart

@@ -413,6 +414,8 @@ class MessageListView with ChangeNotifier, _MessageSequence {
        result.messages.removeLast();
      }

+      store.reconcileMessages(result.messages);


Ah indeed, good to put this after the includeAnchor-substitute logic above. (In my draft I see this came before that.)

I caught it when writing tests; I probably wouldn't have caught it if I hadn't done that. 🙂

Hooray for tests!

gnprice · 2024-05-31T21:16:49Z

test/model/message_test.dart

+      store.reconcileMessages([newMessage]);
+      check(store.messages).deepEquals({1: message});


In these reconcileMessages tests, let's also check the behavior of reconcileMessages in mutating the list it's passed. In particular in this test it should be mutated so that it shallow-equals [message].

Would a "shallow-equals [message]" check be accomplished by .deepEquals([message]), which I think compares elements using operator ==?

(As opposed to something like jsonEquals?)

That would work because Message doesn't override ==, but that feels a bit fragile or implicit.

Given there's just one expected, it could be written .single.identical(message) and that's probably clearest. A more general pattern would look like .deepEquals([(it) => it.identical(message)]) (untested — but the idea is that deepEquals accepts condition callbacks as elements).

gnprice · 2024-05-31T21:21:03Z

test/model/message_test.dart

+    store = eg.store()
+      ..addStream(stream)..addSubscription(subscription);


These should await now, like elsewhere.

(What tipped me off to this is that in the git range-diff output, at the commit:
41d7918 message test [nfc]: Move ReactionEvent tests from msglist test file

the red side of the diff looked like:

-- prepare(); +- await prepare(); - await prepareMessages(foundOldest: true, messages: [originalMessage]); - final message = store.messages.values.single; - -- store.handleEvent( +- await store.handleEvent(

but the green side had only the handleEvent lines and not the prepare lines:

-+ store.handleEvent( ++ await store.handleEvent(

)

Ah indeed! Thanks for the catch!

gnprice · 2024-05-31T21:25:31Z

test/model/message_list_test.dart

-      await prepareMessages(foundOldest: true, messages: [originalMessage]);
-
-      model.maybeUpdateMessage(updateEvent);
+    test('message absent (older than window)', () async {


Let's also test where the message ID is in the window — it's between the oldest and newest that appear in the list — but just isn't actually found in the list. That way we have a test that would break if this were oversimplified to just check it's in the range.

gnprice · 2024-05-31T22:41:41Z

test/model/message_list_test.dart

+        checkInvariants(model);
+        doCheckMessageAfterFetch(
+          check(model).messages.single
+            ..identicalTo(updatedMessage)


After the change above to have the fetch return the old version, we can remove references to store.messages from this test entirely by having this just say:

Suggested change

..identicalTo(updatedMessage)

..id.equals(message.id)

Removing references to store.messages is nice because it helps clarify the narrative of what this test is about, focusing it on the message list.

The id check is still helpful, though, because it helps confirm the internal logic of the test: it ensures doCheckMessageAfterFetch isn't accidentally inspecting some other message, one that just happens to have all along satisfied whatever condition the event was supposed to cause the intended message to satisfy.

gnprice · 2024-05-31T22:51:52Z

test/model/message_list_test.dart

+      await store.handleEvent(eg.updateMessageEditEvent(message,
+        renderedContent: '${message.content}<p>edited</p'));
+      checkNotifiedOnce();


~~Let's also check that the message got re-parsed, since that's the other effect of messageContentChanged.~~

(It looks like the existing tests were missing that check already. But its absence gets more conspicuous now, since there's this nice focused test on messageContentChanged and that method's implementation is so short.)

Ah, scratch that — that's the problem that was solved by checkInvariants. I forgot what a good idea that was. 🙂

gnprice · 2024-05-31T23:01:14Z

test/model/message_test.dart

+    const mkRemoveEvent = eg.updateMessageFlagsRemoveEvent;
+
+    group('add flag', () {
+      test('not in list', () async {


nit: the references to "list" in these descriptions no longer fit

gnprice · 2024-05-31T23:08:53Z

lib/model/message_list.dart

+  void notifyListenersUnconditionally() {
    notifyListeners();
  }


How about just letting call sites of this call notifyListeners directly? I'm not sure "unconditionally" really adds anything for the reader — that's normally the baseline assumption about what a method name means.

gnprice · 2024-05-31T23:18:50Z

lib/model/message.dart

+      for (final messageId in event.messages) {
+        final message = messages[messageId];
+        if (message == null) continue; // a message we don't know about yet
+
+        isAdd
+          ? message.flags.add(event.flag)
+          : message.flags.remove(event.flag);
+
+        for (final view in _messageListViews) {
+          view.notifyListenersIfMessagePresent(messageId);


This can cause a lot of notifyListeners calls, if a bunch of messages get marked read or unread in a narrow the user is looking at.

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 is potentially hundreds of times, so that gets me a bit concerned. Instead we could pass the whole list to each MessageListView as a batch, and let it decide whether to notify a single time.

Good point, yeah.

chrisbobbe · 2024-06-04T00:23:59Z

Thanks for the review! Revision pushed.

gnprice

Thanks! Generally this looks great; just a few comments below.

gnprice · 2024-06-04T02:23:04Z

lib/model/message.dart

+/// The portion of [PerAccountStore] for messages and message lists.
+mixin MessageStore {
+  /// All known messages, indexed by [Message.id].
+  Map<int, Message> get messages;


There's a few more of these; try git log origin.. --grep 455 to find them.

gnprice · 2024-06-04T02:55:28Z

lib/model/message_list.dart

+    final isAnyPresent = messageIds
+      .any((messageId) => _findMessageWithId(messageId) != -1);


nit: for a local with a very small scope like this callback parameter, a terser name is nice:

Suggested change

final isAnyPresent = messageIds

.any((messageId) => _findMessageWithId(messageId) != -1);

final isAnyPresent = messageIds.any((id) => _findMessageWithId(id) != -1);

gnprice · 2024-06-04T02:58:02Z

lib/model/message.dart

+        view.notifyListeners();
+      }
+    } else {
+      bool knowAboutAny = false;


Perhaps anyMessageFound?

With this name I find myself asking, "any what?".

gnprice · 2024-06-04T03:04:06Z

lib/model/message_list.dart

+  /// Calls [super.notifyListeners].
  ///
-  /// TODO(#150): Handle message moves.
-  // NB that when handling message moves (#150), recipient headers
-  // may need updating, and consequently showSender too.
-  void maybeUpdateMessage(UpdateMessageEvent event) {
-    final idx = _findMessageWithId(event.messageId);
-    if (idx == -1)  {
-      return;
-    }
-
-    _applyChangesToMessage(event, messages[idx]);
-    _reparseContent(idx);
-    notifyListeners();
+  /// Overridden here without annotations that would prevent us from calling it
+  /// from [MessageStoreImpl].


I think the doc on this method actually comes out clearest if there's no dartdoc directly on it — that way it inherits the superclass's doc.

The remark about overriding and annotations seems most pertinent as an implementation comment anyway. Specifically we're effectively repealing the @protected annotation that applies on the base implementation.

gnprice · 2024-06-04T03:20:00Z

test/model/message_test.dart

+      check(messages)
+        .deepEquals([message1, message2, message3].map((m) => (it) => identical(it, m)));


Hmm, these callbacks don't look right. The deepEquals doc says (via a macro/template from deepCollectionEquals:

/// Elements, keys, or values within [expected] which are `Condition` callbacks /// are run against the value in the same position within [actual]. /// Condition callbacks must take a `Subject<Object?>` or `Subject<dynamic>` and /// may not use a more specific generic. /// Use `(Subject<Object?> s) => s.isA<Type>()` to check expectations for /// specific element types. /// Note also that the argument type `Subject<Object?>` cannot be inferred and /// must be explicit in the function definition.

So the it argument should be a Subject, not a Message itself.

In fact, the test still passes if I write:

check(messages) - .deepEquals([message1, message2, message3].map((m) => (it) => identical(it, m))); + .deepEquals([message1, message2, message3].map((m) => (it) => !identical(it, m)));

Definitely something to fix there, then 🙂

OK, and after #F719 I think the analyzer should complain on this line.

…gible

Like with StreamStore, this gives us more space to add complexity here without overwhelming PerAccountStore.

It should be fine to just iterate through the plain Iterable.

This isn't part of maybeUpdateMessage.

This isn't used. For a test file, it'd be inappropriate if it were, because the only thing `main` itself should do is to register tests, not run any test or app code.

…eEvent

Soon, we'll store all messages centrally on the store, and when we do that, we'll want an invariant that all the message list's messages are represented there. The tests in this file should exercise the app code faithfully so that they produce states that respect that invariant.

In itself, this exacerbates zulip#455 by making it trigger in more circumstances. That's OK for the moment, because this also gives us a foundation on which to fix that issue soon. A key part of testing this is the line added in message_list_test's `checkInvariants`, which checks that all messages tracked by the MessageListView are present in the central message store. Co-authored-by: Chris Bobbe <cbobbe@zulip.com>

Some of the existing tests of MessageListView in message_list_test.dart are really almost entirely about acting on individual messages rather than message lists. They therefore belong as tests of the overall message store, to live in this file. With that "almost", though, they have a few ties to actually testing the MessageListView itself -- notably, its notification behavior. So to simplify converting them and moving them into this file, we add a MessageListView they can use. This logic is largely copied from message_list_test.dart.

These tests aren't really about a message *list*, but about updating individual messages. Edit them to be all in terms of the overall message store, not the MessageListView.

…ists This looks potentially NFC, but in fact it fixes part of zulip#455: it fixes the issue as far as reactions are concerned. The change is NFC only if the message appears in just one message list. In addition to the symptom explained at zulip#455, this fixes another, which exists now that we've added the central message store with `reconcileMessages`. It's arguably a bit of an inverse of zulip#455, but fundamentally the same issue: if there are currently *zero* message lists that contain the message, then we would drop the update. Then on later fetching the same message, we'd use the version we had (for the good reason explained in reconcileMessages) but in fact it'd be stale. This zero-message-lists symptom also reproduces with other kinds of updates that we handle: editing a message, and adding/removing flags. So, in the regression test for this symptom, I've tried to make some reusable test code that we can use when we fix those other ways of updating messages. Co-authored-by: Chris Bobbe <cbobbe@zulip.com> Fixes-partly: zulip#455

… store

…ests

Like a recent commit did for reactions, this fixes the part of zulip#455 that's concerned with message edits. With message edits, unlike reactions, the double-processing issue when there are two or more message lists wasn't user-facing, since the mutation done by message lists was (naturally) idempotent. So there isn't really an easy or helpful regression test to write for that. But we can and do write one against the dropped-update symptom when there are zero message lists open. For an explanation of that symptom, see the recent commit where we dealt with reactions. Co-authored-by: Chris Bobbe <cbobbe@zulip.com> Fixes-partly: zulip#455

… store

…ests

Like a recent commit did for message edits, and one before that did for reactions, this fixes the part of zulip#455 that's concerned with update-message-flags events. Like in the commit that dealt with message edits, this fixes the symptom of a dropped update when there are zero message lists open. In this case I'm *pretty* sure there wasn't a user-facing symptom with the double-processing situation when the message is in two or more open message lists. That's because I believe we don't give different behavior for a flag that's present multiple times in the `flags` list, vs. just once.

…ssages

chrisbobbe · 2024-06-05T19:15:34Z

Thanks for the review! Revision pushed.

gnprice · 2024-06-05T20:07:35Z

Thanks for all your work fleshing out this PR! Looks great; merging.

gnprice added the a-model Implementing our data model (PerAccountStore, etc.) label May 8, 2024

This was referenced May 8, 2024

Update unreads data on fetching messages #649

Open

Update recent-DMs data on fetching messages #650

Open

autocomplete: Sort user-mention autocomplete results #608

Closed

chrisbobbe reviewed May 8, 2024

View reviewed changes

chrisbobbe force-pushed the pr-store-message branch from 2789a1f to 20131f6 Compare May 31, 2024 04:29

chrisbobbe marked this pull request as ready for review May 31, 2024 05:40

gnprice commented May 31, 2024

View reviewed changes

chrisbobbe force-pushed the pr-store-message branch from 20131f6 to 93a8c2d Compare June 4, 2024 00:16

gnprice commented Jun 4, 2024

View reviewed changes

chrisbobbe and others added 16 commits June 5, 2024 12:48

msglist test [nfc]: Use isSortedWithoutDuplicates to be a bit more le…

69ceb14

…gible

message [nfc]: Move message data to a MessageStore class

c6aedb1

Like with StreamStore, this gives us more space to add complexity here without overwhelming PerAccountStore.

message [nfc]: Remove unnecessary .toList()

f8835b1

It should be fine to just iterate through the plain Iterable.

msglist test [nfc]: Fix grouping of "ReactionEvent handling"

8cf71a0

This isn't part of maybeUpdateMessage.

msglist test [nfc]: Cut "async" from main function

b4d9c5d

This isn't used. For a test file, it'd be inappropriate if it were, because the only thing `main` itself should do is to register tests, not run any test or app code.

msglist [nfc]: Rename MessageListView.maybeAddMessage to handleMessag…

95a3e18

…eEvent

msglist test [nfc]: Adapt ReactionEvent tests to refer to message store

b844fea

These tests aren't really about a message *list*, but about updating individual messages. Edit them to be all in terms of the overall message store, not the MessageListView.

message test [nfc]: Move ReactionEvent tests from msglist test file

37c7de8

message test [nfc]: Move make-ReactionEvent handler to example_data

a9b18cd

msglist [nfc]: s/maybeUpdateMessage/handleUpdateMessageEvent/

4fd39c0

msglist test: Adapt UpdateMessageEvent edit tests to go through whole…

62fb55a

… store

message test [nfc]: Move UpdateMessageEvent edit tests from msglist t…

9d54124

…ests

gnprice and others added 6 commits June 5, 2024 15:06

msglist [nfc]: s/maybeUpdateMessageFlags/handleUpdateMessageFlagsEvent/

735410a

msglist test: Adapt UpdateMessageFlagsEvent tests to go through whole…

83d49f1

… store

message test [nfc]: Move UpdateMessageFlagsEvent tests from msglist t…

574fd85

…ests

store [nfc]: Note TODOs to update more data structures on fetching me…

c8d91cf

…ssages

chrisbobbe force-pushed the pr-store-message branch from 93a8c2d to c8d91cf Compare June 5, 2024 19:15

gnprice merged commit c8d91cf into zulip:main Jun 5, 2024
1 check passed

gnprice deleted the pr-store-message branch June 5, 2024 20:18

		store.reconcileMessages([newMessage]);
		check(store.messages).deepEquals({1: message});

		store = eg.store()
		..addStream(stream)..addSubscription(subscription);

		final isAnyPresent = messageIds
		.any((messageId) => _findMessageWithId(messageId) != -1);

	final isAnyPresent = messageIds
	.any((messageId) => _findMessageWithId(messageId) != -1);
	final isAnyPresent = messageIds.any((id) => _findMessageWithId(id) != -1);

		check(messages)
		.deepEquals([message1, message2, message3].map((m) => (it) => identical(it, m)));

Add a central message store #648

Add a central message store #648

Conversation

gnprice commented May 8, 2024 • edited

gnprice commented May 8, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice commented May 15, 2024

chrisbobbe commented May 31, 2024 • edited

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

chrisbobbe Jun 3, 2024 • edited

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 Jun 4, 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

chrisbobbe commented Jun 5, 2024

gnprice commented Jun 5, 2024

gnprice commented May 8, 2024 •

edited

chrisbobbe commented May 31, 2024 •

edited

chrisbobbe Jun 3, 2024 •

edited