From 6dc2eca1bafdb0c5889c59f73dc524f054a2c964 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Wed, 26 Jun 2024 16:42:19 -0400 Subject: [PATCH] message: Handle message deletion event. 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 --- lib/model/message.dart | 7 ++++++- lib/model/message_list.dart | 30 ++++++++++++++++++++++++++ test/example_data.dart | 15 +++++++++++++ test/model/message_list_test.dart | 35 +++++++++++++++++++++++++++++++ test/model/message_test.dart | 33 +++++++++++++++++++++++++++++ 5 files changed, 119 insertions(+), 1 deletion(-) diff --git a/lib/model/message.dart b/lib/model/message.dart index 4ccf45d2ca0..8e862cf75a0 100644 --- a/lib/model/message.dart +++ b/lib/model/message.dart @@ -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) { diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index 117b30e614e..7075dd1ccce 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -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 messageIds) { + final messagesToRemoveById = {}; + final contentToRemove = {}; + 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 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. @@ -478,6 +502,12 @@ class MessageListView with ChangeNotifier, _MessageSequence { } } + void messagesRemoved(List 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 diff --git a/test/example_data.dart b/test/example_data.dart index 36fc60fec05..8de15b91593 100644 --- a/test/example_data.dart +++ b/test/example_data.dart @@ -388,6 +388,21 @@ const _unreadMsgs = unreadMsgs; // Events. // +DeleteMessageEvent deleteMessageEvent(List 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] diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index de0c7f32a7d..998bd6c991a 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -261,6 +261,41 @@ void main() { check(model).messages.length.equals(30); }); + 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); + }); + + 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); + }); + + 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); + }); + }); + test('MessageEvent, before fetch', () async { final stream = eg.stream(); await prepare(narrow: StreamNarrow(stream.streamId)); diff --git a/test/model/message_test.dart b/test/model/message_test.dart index 10e4334fcf3..248d9fcfc3e 100644 --- a/test/model/message_test.dart +++ b/test/model/message_test.dart @@ -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: []);