Skip to content

Add a central message store #648

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 22 commits into from
Jun 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
69ceb14
msglist test [nfc]: Use isSortedWithoutDuplicates to be a bit more le…
chrisbobbe May 29, 2024
c6aedb1
message [nfc]: Move message data to a MessageStore class
gnprice May 5, 2024
f8835b1
message [nfc]: Remove unnecessary `.toList()`
chrisbobbe May 28, 2024
8cf71a0
msglist test [nfc]: Fix grouping of "ReactionEvent handling"
gnprice May 8, 2024
b4d9c5d
msglist test [nfc]: Cut "async" from main function
gnprice May 8, 2024
95a3e18
msglist [nfc]: Rename MessageListView.maybeAddMessage to handleMessag…
chrisbobbe May 29, 2024
6eb9498
msglist test: Process MessageEvents through whole store, not just model
chrisbobbe May 29, 2024
d8949c1
message: Store all messages centrally
gnprice May 5, 2024
197f9f4
message test [nfc]: Add a MessageListView, optionally
gnprice May 8, 2024
b844fea
msglist test [nfc]: Adapt ReactionEvent tests to refer to message store
gnprice May 8, 2024
37c7de8
message test [nfc]: Move ReactionEvent tests from msglist test file
gnprice May 8, 2024
a9b18cd
message test [nfc]: Move make-ReactionEvent handler to example_data
chrisbobbe Jun 3, 2024
273e464
message: Mutate messages for reaction events here, instead of in msgl…
gnprice May 5, 2024
4fd39c0
msglist [nfc]: s/maybeUpdateMessage/handleUpdateMessageEvent/
chrisbobbe May 30, 2024
62fb55a
msglist test: Adapt UpdateMessageEvent edit tests to go through whole…
chrisbobbe May 30, 2024
9d54124
message test [nfc]: Move UpdateMessageEvent edit tests from msglist t…
chrisbobbe May 30, 2024
4a59f87
message: Mutate messages for message edits here, instead of in msglists
gnprice May 5, 2024
735410a
msglist [nfc]: s/maybeUpdateMessageFlags/handleUpdateMessageFlagsEvent/
chrisbobbe May 31, 2024
83d49f1
msglist test: Adapt UpdateMessageFlagsEvent tests to go through whole…
chrisbobbe May 31, 2024
574fd85
message test [nfc]: Move UpdateMessageFlagsEvent tests from msglist t…
chrisbobbe May 31, 2024
d6b001c
message: Mutate messages for flags events here, instead of in msglists
chrisbobbe May 31, 2024
c8d91cf
store [nfc]: Note TODOs to update more data structures on fetching me…
gnprice May 8, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
196 changes: 196 additions & 0 deletions lib/model/message.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,196 @@
import '../api/model/events.dart';
import '../api/model/model.dart';
import 'message_list.dart';

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


void registerMessageList(MessageListView view);
void unregisterMessageList(MessageListView view);

/// Reconcile a batch of just-fetched messages with the store,
/// mutating the list.
///
/// This is called after a [getMessages] request to report the result
/// to the store.
///
/// 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
/// [this.messages].
void reconcileMessages(List<Message> messages);
}

class MessageStoreImpl with MessageStore {
MessageStoreImpl()
// There are no messages in InitialSnapshot, so we don't have
// a use case for initializing MessageStore with nonempty [messages].
: messages = {};

@override
final Map<int, Message> messages;

final Set<MessageListView> _messageListViews = {};

@override
void registerMessageList(MessageListView view) {
final added = _messageListViews.add(view);
assert(added);
}

@override
void unregisterMessageList(MessageListView view) {
final removed = _messageListViews.remove(view);
assert(removed);
}

void reassemble() {
for (final view in _messageListViews) {
view.reassemble();
}
}

void dispose() {
for (final view in _messageListViews) {
view.dispose();
}
}

@override
void reconcileMessages(List<Message> messages) {
// What to do when some of the just-fetched messages are already known?
// This is common and normal: in particular it happens when one message list
// overlaps another, e.g. a stream and a topic within it.
//
// Most often, the just-fetched message will look just like the one we
// already have. But they can differ: message fetching happens out of band
// from the event queue, so there's inherently a race.
//
// If the fetched message reflects changes we haven't yet heard from the
// event queue, then it doesn't much matter which version we use: we'll
// soon get the corresponding events and apply the changes anyway.
// But if it lacks changes we've already heard from the event queue, then
// we won't hear those events again; the only way to wind up with an
// updated message is to use the version we have, that already reflects
// those events' changes. So we always stick with the version we have.
for (int i = 0; i < messages.length; i++) {
final message = messages[i];
messages[i] = this.messages.putIfAbsent(message.id, () => message);
}
}

void handleMessageEvent(MessageEvent event) {
// If the message is one we already know about (from a fetch),
// clobber it with the one from the event system.
// See [fetchedMessages] for reasoning.
messages[event.message.id] = event.message;

for (final view in _messageListViews) {
view.handleMessageEvent(event);
}
}

void handleUpdateMessageEvent(UpdateMessageEvent event) {
_handleUpdateMessageEventContent(event);
// TODO(#150): Handle message moves. The views' recipient headers
// may need updating, and consequently showSender too.
}

void _handleUpdateMessageEventContent(UpdateMessageEvent event) {
final message = messages[event.messageId];
if (message == null) return;

// TODO(server-5): Cut this fallback; rely on renderingOnly from FL 114
final isRenderingOnly = event.renderingOnly ?? (event.userId == null);
if (event.editTimestamp != null && !isRenderingOnly) {
// A rendering-only update gets omitted from the message edit history,
// and [Message.lastEditTimestamp] is the last timestamp of that history.
// So on a rendering-only update, the timestamp doesn't get updated.
message.lastEditTimestamp = event.editTimestamp;
}

message.flags = event.flags;

if (event.renderedContent != null) {
assert(message.contentType == 'text/html',
"Message contentType was ${message.contentType}; expected text/html.");
message.content = event.renderedContent!;
}

if (event.isMeMessage != null) {
message.isMeMessage = event.isMeMessage!;
}

for (final view in _messageListViews) {
view.messageContentChanged(event.messageId);
}
}

void handleDeleteMessageEvent(DeleteMessageEvent event) {
// TODO handle DeleteMessageEvent, particularly in MessageListView
}

void handleUpdateMessageFlagsEvent(UpdateMessageFlagsEvent event) {
final isAdd = switch (event) {
UpdateMessageFlagsAddEvent() => true,
UpdateMessageFlagsRemoveEvent() => false,
};

if (isAdd && (event as UpdateMessageFlagsAddEvent).all) {
for (final message in messages.values) {
message.flags.add(event.flag);
}

for (final view in _messageListViews) {
if (view.messages.isEmpty) continue;
view.notifyListeners();
}
} else {
bool anyMessageFound = false;
for (final messageId in event.messages) {
final message = messages[messageId];
if (message == null) continue; // a message we don't know about yet
anyMessageFound = true;

isAdd
? message.flags.add(event.flag)
: message.flags.remove(event.flag);
}
if (anyMessageFound) {
for (final view in _messageListViews) {
view.notifyListenersIfAnyMessagePresent(event.messages);
}
}
}
}

void handleReactionEvent(ReactionEvent event) {
final message = messages[event.messageId];
if (message == null) return;

switch (event.op) {
case ReactionOp.add:
(message.reactions ??= Reactions([])).add(Reaction(
emojiName: event.emojiName,
emojiCode: event.emojiCode,
reactionType: event.reactionType,
userId: event.userId,
));
case ReactionOp.remove:
if (message.reactions == null) { // TODO(log)
return;
}
message.reactions!.remove(
reactionType: event.reactionType,
emojiCode: event.emojiCode,
userId: event.userId,
);
}

for (final view in _messageListViews) {
view.notifyListenersIfMessagePresent(event.messageId);
}
}
}
119 changes: 26 additions & 93 deletions lib/model/message_list.dart
Original file line number Diff line number Diff line change
Expand Up @@ -378,6 +378,7 @@ class MessageListView with ChangeNotifier, _MessageSequence {
numBefore: kMessageListFetchBatchSize,
numAfter: 0,
);
store.reconcileMessages(result.messages);
for (final message in result.messages) {
if (_messageVisible(message)) {
_addMessage(message);
Expand Down Expand Up @@ -413,6 +414,8 @@ class MessageListView with ChangeNotifier, _MessageSequence {
result.messages.removeLast();
}

store.reconcileMessages(result.messages);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hooray for tests!


final fetchedMessages = _allMessagesVisible
? result.messages // Avoid unnecessarily copying the list.
: result.messages.where(_messageVisible);
Expand All @@ -426,10 +429,9 @@ class MessageListView with ChangeNotifier, _MessageSequence {
}
}

/// Add [message] to this view, if it belongs here.
///
/// Called in particular when we get a [MessageEvent].
void maybeAddMessage(Message message) {
/// Add [MessageEvent.message] to this view, if it belongs here.
void handleMessageEvent(MessageEvent event) {
final message = event.message;
if (!narrow.containsMessage(message) || !_messageVisible(message)) {
return;
}
Expand All @@ -442,104 +444,35 @@ class MessageListView with ChangeNotifier, _MessageSequence {
notifyListeners();
}

static void _applyChangesToMessage(UpdateMessageEvent event, Message message) {
// TODO(server-5): Cut this fallback; rely on renderingOnly from FL 114
final isRenderingOnly = event.renderingOnly ?? (event.userId == null);
if (event.editTimestamp != null && !isRenderingOnly) {
// A rendering-only update gets omitted from the message edit history,
// and [Message.lastEditTimestamp] is the last timestamp of that history.
// So on a rendering-only update, the timestamp doesn't get updated.
message.lastEditTimestamp = event.editTimestamp;
}

message.flags = event.flags;

if (event.renderedContent != null) {
assert(message.contentType == 'text/html',
"Message contentType was ${message.contentType}; expected text/html.");
message.content = event.renderedContent!;
}

if (event.isMeMessage != null) {
message.isMeMessage = event.isMeMessage!;
}
// Repeal the `@protected` annotation that applies on the base implementation,
// so we can call this method from [MessageStoreImpl].
@override
void notifyListeners() {
super.notifyListeners();
}

/// Update the message the given event applies to, if present in this view.
///
/// This method only handles the case where the message's contents
/// were changed, and ignores any changes to its stream or topic.
///
/// 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;
/// Notify listeners if the given message is present in this view.
void notifyListenersIfMessagePresent(int messageId) {
final index = _findMessageWithId(messageId);
if (index != -1) {
notifyListeners();
}

_applyChangesToMessage(event, messages[idx]);
_reparseContent(idx);
notifyListeners();
}

void maybeUpdateMessageFlags(UpdateMessageFlagsEvent event) {
final isAdd = switch (event) {
UpdateMessageFlagsAddEvent() => true,
UpdateMessageFlagsRemoveEvent() => false,
};

bool didUpdateAny = false;
if (isAdd && (event as UpdateMessageFlagsAddEvent).all) {
for (final message in messages) {
message.flags.add(event.flag);
didUpdateAny = true;
}
} else {
for (final messageId in event.messages) {
final index = _findMessageWithId(messageId);
if (index != -1) {
final message = messages[index];
isAdd ? message.flags.add(event.flag) : message.flags.remove(event.flag);
didUpdateAny = true;
}
}
}
if (!didUpdateAny) {
return;
/// Notify listeners if any of the given messages is present in this view.
void notifyListenersIfAnyMessagePresent(Iterable<int> messageIds) {
final isAnyPresent = messageIds.any((id) => _findMessageWithId(id) != -1);
if (isAnyPresent) {
notifyListeners();
}

notifyListeners();
}

void maybeUpdateMessageReactions(ReactionEvent event) {
final index = _findMessageWithId(event.messageId);
if (index == -1) {
return;
}

final message = messages[index];
switch (event.op) {
case ReactionOp.add:
(message.reactions ??= Reactions([])).add(Reaction(
emojiName: event.emojiName,
emojiCode: event.emojiCode,
reactionType: event.reactionType,
userId: event.userId,
));
case ReactionOp.remove:
if (message.reactions == null) { // TODO(log)
return;
}
message.reactions!.remove(
reactionType: event.reactionType,
emojiCode: event.emojiCode,
userId: event.userId,
);
void messageContentChanged(int messageId) {
final index = _findMessageWithId(messageId);
if (index != -1) {
_reparseContent(index);
notifyListeners();
}

notifyListeners();
}

/// Called when the app is reassembled during debugging, e.g. for hot reload.
Expand Down
Loading
Loading