Skip to content
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.

Sign up for GitHub

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

message: Handle message deletion event. #766

Merged
merged 2 commits into from
Jul 2, 2024
Merged

message: Handle message deletion event. #766

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c4ca1ab
message: Handle message deletion event.
PIG208 Jun 26, 2024
1f97563
message test [nfc]: Tighten message-deletion test slightly
gnprice Jul 2, 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
7 changes: 6 additions & 1 deletion lib/model/message.dart
View file
Open in desktop
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.handleDeleteMessageEvent(event);
}
}

void handleUpdateMessageFlagsEvent(UpdateMessageFlagsEvent event) {
Expand Down
30 changes: 30 additions & 0 deletions lib/model/message_list.dart
View file
Open in desktop
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 = Set<ZulipContent>.identity();
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));
Comment on lines +174 to +175
Copy link
Member

Choose a reason for hiding this comment

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

Neat solution. I think this algorithm works fine.

I might replace it with a direct loop if I find that convenient in the course of building the other features (like #421) that will reuse a version of this method, but I'll cross that bridge if I come to it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it. Leaving it as-is for this revision.

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 @@ -429,6 +453,12 @@ class MessageListView with ChangeNotifier, _MessageSequence {
}
}

void handleDeleteMessageEvent(DeleteMessageEvent event) {
if (_removeMessagesById(event.messageIds)) {
notifyListeners();
}
}

/// Add [MessageEvent.message] to this view, if it belongs here.
void handleMessageEvent(MessageEvent event) {
final message = event.message;
Expand Down
15 changes: 15 additions & 0 deletions test/example_data.dart
View file
Open in desktop
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
60 changes: 60 additions & 0 deletions test/model/message_list_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,66 @@ void main() {
check(model).fetched.isFalse();
});

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

test('in narrow', () async {
PIG208 marked this conversation as resolved.
Show resolved Hide resolved
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('complete message deletion', () async {
await prepare(narrow: StreamNarrow(stream.streamId));
await prepareMessages(foundOldest: true, messages: messages.sublist(0, 25));

check(model).messages.length.equals(25);
await store.handleEvent(eg.deleteMessageEvent(messages));
checkNotifiedOnce();
check(model).messages.length.equals(0);
});

test('non-consecutive message deletion', () async {
await prepare(narrow: StreamNarrow(stream.streamId));
await prepareMessages(foundOldest: true, messages: messages);
final messagesToDelete = messages.sublist(2, 5) + messages.sublist(10, 15);

check(model).messages.length.equals(30);
await store.handleEvent(eg.deleteMessageEvent(messagesToDelete));
checkNotifiedOnce();
check(model.messages.map((message) => message.id)).deepEquals([
...messages.sublist(0, 2),
...messages.sublist(5, 10),
...messages.sublist(15),
].map((message) => message.id));
});
});

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

group('handleDeleteMessageEvent', () {
Copy link
Member

Choose a reason for hiding this comment

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

This will also need tests for the behavior of MessageListView, as well as that of MessageStoreImpl. After all, the former is where most of the code this change introduces is. 🙂

Those tests can go in model/message_list_test.dart; search for MessageEvent there for examples to follow. I think, given that we're using _reprocessAll rather than trying to do something more clever (as handleMessageEvent does), the only tests needed here are analogous to the tests with "MessageEvent" in their names.

Note one key thing those tests do is they call that checkInvariants function, in particular on every notifyListeners. It conducts a rather fierce set of consistency checks on the model, in particular recomputing everything that derives from messages and confirming it all has the right values.

Copy link
Member Author

@PIG208 PIG208 Jun 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Note one key thing those tests do is they call that checkInvariants function, in particular on every notifyListeners. It conducts a rather fierce set of consistency checks on the model, in particular recomputing everything that derives from messages and confirming it all has the right values.

I see that in the 'MessageEvent, before fetch' test, but not the other 'MessageEvent' tests. Is that intended?

Copy link
Member

Choose a reason for hiding this comment

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

It's there 🙂 Via this part:

in particular on every notifyListeners.

See prepare for that call site.

Copy link
Member

Choose a reason for hiding this comment

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

(Quite possibly we should have checkNotNotified call checkInvariants too, and then the individual test cases will be more uniform in not needing to explicitly do so)

Copy link
Member Author

@PIG208 PIG208 Jun 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Oh, I see! So the call is added in a listener. Then the post event invariant check of 'MessageEvent, not in narrow' seems to be missing because it isn't expecting a notification. Yeah it makes sense to add it to checkNotNotified too.

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
Loading