Skip to content

Commit

Permalink
message: Handle moved messages from UpdateMessageEvent.
Browse files Browse the repository at this point in the history
We already handle the case where only a message's content is edited.
This handles the case where messages are moved too, between topics
and/or channels.

This introduces more notifyListener calls, and the listeners can be
notified more than once per UpdateMessage event. This is expected.

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. These races are detailed in the "fetch
races" test group.
  • Loading branch information
gnprice committed Jul 17, 2024
1 parent aef0ab1 commit 9c50ba5
Show file tree
Hide file tree
Showing 5 changed files with 524 additions and 44 deletions.
4 changes: 2 additions & 2 deletions lib/api/model/model.dart
Original file line number Diff line number Diff line change
Expand Up @@ -480,7 +480,7 @@ sealed class Message {
final int senderId;
final String senderRealmStr;
@JsonKey(name: 'subject')
final String topic;
String topic;
// final List<string> submessages; // TODO handle
final int timestamp;
String get type;
Expand Down Expand Up @@ -577,7 +577,7 @@ class StreamMessage extends Message {
String get type => 'stream';

final String displayRecipient;
final int streamId;
int streamId;

StreamMessage({
required super.client,
Expand Down
42 changes: 30 additions & 12 deletions lib/model/message.dart
Original file line number Diff line number Diff line change
Expand Up @@ -178,22 +178,40 @@ class MessageStoreImpl with MessageStore {
return;
}

if (newStreamId == null
&& MessageEditState.topicMoveWasResolveOrUnresolve(origTopic, newTopic)) {
// The topic was only resolved/unresolved.
// No change to the messages' editState.
return;
}
final wasResolveOrUnresolve = (newStreamId == null
&& MessageEditState.topicMoveWasResolveOrUnresolve(origTopic, newTopic));

// TODO(#150): Handle message moves. The views' recipient headers
// may need updating, and consequently showSender too.
// Currently only editState gets updated.
for (final messageId in event.messageIds) {
final message = messages[messageId];
if (message == null) continue;
// Do not override the edited marker if the message has also been moved.
if (message.editState == MessageEditState.edited) continue;
message.editState = MessageEditState.moved;

if (message is! StreamMessage) {
assert(debugLog('Bad UpdateMessageEvent: stream/topic move on a DM')); // TODO(log)
continue;
}

if (newStreamId != null) {
message.streamId = newStreamId;
// TODO update [StreamMessage.displayRecipient]; doesn't usually
// matter, because we only consult it when the stream is unknown
}

message.topic = newTopic;

if (!wasResolveOrUnresolve
&& message.editState == MessageEditState.none) {
message.editState = MessageEditState.moved;
}
}

for (final view in _messageListViews) {
view.messagesMoved(
origStreamId: origStreamId,
newStreamId: newStreamId ?? origStreamId,
origTopic: origTopic,
newTopic: newTopic,
messageIds: event.messageIds,
);
}
}

Expand Down
92 changes: 89 additions & 3 deletions lib/model/message_list.dart
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,9 @@ class MessageListHistoryStartItem extends MessageListItem {
///
/// This comprises much of the guts of [MessageListView].
mixin _MessageSequence {
/// A sequence number for invalidating stale fetches.
int generation = 0;

/// The messages.
///
/// See also [contents] and [items].
Expand Down Expand Up @@ -192,6 +195,17 @@ mixin _MessageSequence {
_reprocessAll();
}

/// Reset all [_MessageSequence] data, and cancel any active fetches.
void _reset() {
generation += 1;
messages.clear();
_fetched = false;
_haveOldest = false;
_fetchingOlder = false;
contents.clear();
items.clear();
}

/// Redo all computations from scratch, based on [messages].
void _recompute() {
assert(contents.length == messages.length);
Expand Down Expand Up @@ -396,12 +410,14 @@ class MessageListView with ChangeNotifier, _MessageSequence {
assert(!fetched && !haveOldest && !fetchingOlder);
assert(messages.isEmpty && contents.isEmpty);
// TODO schedule all this in another isolate
final generation = this.generation;
final result = await getMessages(store.connection,
narrow: narrow.apiEncode(),
anchor: AnchorCode.newest,
numBefore: kMessageListFetchBatchSize,
numAfter: 0,
);
if (this.generation > generation) return;
store.reconcileMessages(result.messages);
for (final message in result.messages) {
if (_messageVisible(message)) {
Expand All @@ -423,6 +439,7 @@ class MessageListView with ChangeNotifier, _MessageSequence {
_fetchingOlder = true;
_updateEndMarkers();
notifyListeners();
final generation = this.generation;
try {
final result = await getMessages(store.connection,
narrow: narrow.apiEncode(),
Expand All @@ -431,6 +448,7 @@ class MessageListView with ChangeNotifier, _MessageSequence {
numBefore: kMessageListFetchBatchSize,
numAfter: 0,
);
if (this.generation > generation) return;

if (result.messages.isNotEmpty
&& result.messages.last.id == messages[0].id) {
Expand All @@ -447,9 +465,11 @@ class MessageListView with ChangeNotifier, _MessageSequence {
_insertAllMessages(0, fetchedMessages);
_haveOldest = result.foundOldest;
} finally {
_fetchingOlder = false;
_updateEndMarkers();
notifyListeners();
if (this.generation == generation) {
_fetchingOlder = false;
_updateEndMarkers();
notifyListeners();
}
}
}

Expand Down Expand Up @@ -485,6 +505,72 @@ class MessageListView with ChangeNotifier, _MessageSequence {
}
}

void _messagesMovedInternally(List<int> messageIds) {
for (final messageId in messageIds) {
if (_findMessageWithId(messageId) != -1) {
_reprocessAll();
notifyListeners();
return;
}
}
}

void _messagesMovedIntoNarrow() {
// If there are some messages we don't have in [MessageStore], and they
// occur later than the messages we have here, then we just have to
// re-fetch from scratch. That's always valid, so just do that always.
// TODO in cases where we do have data to do better, do better.
_reset();
notifyListeners();
fetchInitial();
}

void _messagesMovedFromNarrow(List<int> messageIds) {
if (_removeMessagesById(messageIds)) {
notifyListeners();
}
}

void messagesMoved({
required int origStreamId,
required int newStreamId,
required String origTopic,
required String newTopic,
required List<int> messageIds,
}) {
switch (narrow) {
case DmNarrow():
// DMs can't be moved (nor created by moves),
// so the messages weren't in this narrow and still aren't.
return;

case CombinedFeedNarrow():
// The messages were and remain in this narrow.
// TODO(#421): … except they may have become muted or not.
// We'll handle that at the same time as we handle muting itself changing.
// Recipient headers, and downstream of those, may change, though.
_messagesMovedInternally(messageIds);

case StreamNarrow(:final streamId):
switch ((origStreamId == streamId, newStreamId == streamId)) {
case (false, false): return;
case (true, true ): _messagesMovedInternally(messageIds);
case (false, true ): _messagesMovedIntoNarrow();
case (true, false): _messagesMovedFromNarrow(messageIds);
}

case TopicNarrow(:final streamId, :final topic):
final oldMatch = (origStreamId == streamId && origTopic == topic);
final newMatch = (newStreamId == streamId && newTopic == topic);
switch ((oldMatch, newMatch)) {
case (false, false): return;
case (true, true ): _messagesMovedInternally(messageIds);
case (false, true ): _messagesMovedIntoNarrow();
case (true, false): _messagesMovedFromNarrow(messageIds); // TODO handle propagateMode
}
}
}

// Repeal the `@protected` annotation that applies on the base implementation,
// so we can call this method from [MessageStoreImpl].
@override
Expand Down
Loading

0 comments on commit 9c50ba5

Please sign in to comment.