Skip to content

Commit

Permalink
message: Handle message deletion event.
Browse files Browse the repository at this point in the history
When handling a message deletion event, we just simply remove the
messages from the per account store and wipe them from the message list
view if they are present.

Because message deletion can affect what items are to be seen (date
separator, recipient header, etc.), if the view has any message deleted, a
`_reprocessAll` call is triggered. It would be ideal to just removed the
affected items for better performance.

Fixes #120.

Signed-off-by: Zixuan James Li <[email protected]>
  • Loading branch information
PIG208 committed Jun 27, 2024
1 parent 0399af6 commit 90b2e6a
Show file tree
Hide file tree
Showing 5 changed files with 122 additions and 1 deletion.
7 changes: 6 additions & 1 deletion lib/model/message.dart
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,12 @@ class MessageStoreImpl with MessageStore {
}

void handleDeleteMessageEvent(DeleteMessageEvent event) {
// TODO handle DeleteMessageEvent, particularly in MessageListView
for (final messageId in event.messageIds) {
messages.remove(messageId);
}
for (final view in _messageListViews) {
view.messagesRemoved(event.messageIds);
}
}

void handleUpdateMessageFlagsEvent(UpdateMessageFlagsEvent event) {
Expand Down
30 changes: 30 additions & 0 deletions lib/model/message_list.dart
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,30 @@ mixin _MessageSequence {
_processMessage(messages.length - 1);
}

/// Removes the given messages, if present.
///
/// Returns true if at least one message was present, false otherwise.
/// If none of [messageIds] are found, this is a no-op.
bool _removeMessagesById(Iterable<int> messageIds) {
final messagesToRemoveById = <int>{};
final contentToRemove = <ZulipContent>{};
for (final messageId in messageIds) {
final index = _findMessageWithId(messageId);
if (index == -1) continue;
messagesToRemoveById.add(messageId);
contentToRemove.add(contents[index]);
}
if (messagesToRemoveById.isEmpty) return false;

assert(contents.length == messages.length);
messages.removeWhere((message) => messagesToRemoveById.contains(message.id));
contents.removeWhere((content) => contentToRemove.contains(content));
assert(contents.length == messages.length);
_reprocessAll();

return true;
}

void _insertAllMessages(int index, Iterable<Message> toInsert) {
// TODO parse/process messages in smaller batches, to not drop frames.
// On a Pixel 5, a batch of 100 messages takes ~15-20ms in _insertAllMessages.
Expand Down Expand Up @@ -478,6 +502,12 @@ class MessageListView with ChangeNotifier, _MessageSequence {
}
}

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

/// Called when the app is reassembled during debugging, e.g. for hot reload.
///
/// This will redo from scratch any computations we can, such as parsing
Expand Down
15 changes: 15 additions & 0 deletions test/example_data.dart
Original file line number Diff line number Diff line change
Expand Up @@ -388,6 +388,21 @@ const _unreadMsgs = unreadMsgs;
// Events.
//

DeleteMessageEvent deleteMessageEvent(List<StreamMessage> messages) {
assert(messages.isNotEmpty);
final streamId = messages.first.streamId;
final topic = messages.first.topic;
assert(messages.every((m) => m.streamId == streamId));
assert(messages.every((m) => m.topic == topic));
return DeleteMessageEvent(
id: 0,
messageIds: messages.map((message) => message.id).toList(),
messageType: MessageType.stream,
streamId: messages[0].streamId,
topic: messages[0].topic,
);
}

UpdateMessageEvent updateMessageEditEvent(
Message origMessage, {
int? userId = -1, // null means null; default is [selfUser.userId]
Expand Down
38 changes: 38 additions & 0 deletions test/model/message_list_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,44 @@ void main() {
checkInvariants(model);
});

group('DeleteMessageEvent', () {
final stream = eg.stream();
final messages = List.generate(30, (i) => eg.streamMessage(stream: stream));

test('in narrow', () async {
await prepare(narrow: StreamNarrow(stream.streamId));
await prepareMessages(foundOldest: true, messages: messages);

check(model).messages.length.equals(30);
await store.handleEvent(eg.deleteMessageEvent(messages.sublist(0, 10)));
checkNotifiedOnce();
check(model).messages.length.equals(20);
checkInvariants(model);
});

test('not all in narrow', () async {
await prepare(narrow: StreamNarrow(stream.streamId));
await prepareMessages(foundOldest: true, messages: messages.sublist(5));

check(model).messages.length.equals(25);
await store.handleEvent(eg.deleteMessageEvent(messages.sublist(0, 10)));
checkNotifiedOnce();
check(model).messages.length.equals(20);
checkInvariants(model);
});

test('not in narrow', () async {
await prepare(narrow: StreamNarrow(stream.streamId));
await prepareMessages(foundOldest: true, messages: messages.sublist(5));

check(model).messages.length.equals(25);
await store.handleEvent(eg.deleteMessageEvent(messages.sublist(0, 5)));
checkNotNotified();
check(model).messages.length.equals(25);
checkInvariants(model);
});
});

group('notifyListenersIfMessagePresent', () {
test('message present', () async {
await prepare(narrow: const CombinedFeedNarrow());
Expand Down
33 changes: 33 additions & 0 deletions test/model/message_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,39 @@ void main() {
});
});

group('handleDeleteMessageEvent', () {
test('delete an unknown message', () async {
final message1 = eg.streamMessage();
final message2 = eg.streamMessage();
await prepare();
await prepareMessages([message1]);
await store.handleEvent(eg.deleteMessageEvent([message2]));
checkNotNotified();
check(store).messages.values.single.id.equals(message1.id);
});

test('delete messages', () async {
final message1 = eg.streamMessage();
final message2 = eg.streamMessage();
await prepare();
await prepareMessages([message1, message2]);
await store.handleEvent(eg.deleteMessageEvent([message1, message2]));
checkNotifiedOnce();
check(store).messages.isEmpty();
});

test('delete an unknown message with a known message', () async {
final message1 = eg.streamMessage();
final message2 = eg.streamMessage();
final message3 = eg.streamMessage();
await prepare();
await prepareMessages([message1, message2]);
await store.handleEvent(eg.deleteMessageEvent([message2, message3]));
checkNotifiedOnce();
check(store).messages.values.single.id.equals(message1.id);
});
});

group('handleReactionEvent', () {
test('add reaction', () async {
final originalMessage = eg.streamMessage(reactions: []);
Expand Down

0 comments on commit 90b2e6a

Please sign in to comment.