Skip to content

Commit 94e8c9b

Browse files
PIG208PIG208
committed
wip; recent_senders: Handle moved messages
TODO explain implementation Fixes: zulip#901 Signed-off-by: Zixuan James Li <[email protected]>
1 parent b9b058c commit 94e8c9b

File tree

3 files changed

+191
-0
lines changed

3 files changed

+191
-0
lines changed

‎lib/model/recent_senders.dart

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,44 @@ class RecentSenders {
6868
[senderId] ??= MessageIdTracker()).add(messageId);
6969
}
7070

71+
void handleUpdateMessageEvent(UpdateMessageEvent event, Map<int, Message> cachedMessages) {
72+
if (event.moveData == null) {
73+
return;
74+
}
75+
final UpdateMessageMoveData(
76+
:origStreamId, :newStreamId, :origTopic, :newTopic) = event.moveData!;
77+
78+
final messagesByUser = _groupStreamMessageIdsBySender(event.messageIds, cachedMessages);
79+
final sendersInChannel = streamSenders[origStreamId];
80+
final topicsInChannel = topicSenders[origStreamId];
81+
final sendersInTopic = topicsInChannel?[origTopic];
82+
for (final MapEntry(key: senderId, value: messages) in messagesByUser.entries) {
83+
if (origStreamId != newStreamId) {
84+
final streamTracker = sendersInChannel?[senderId];
85+
final movedMessagesInStreamTracker = streamTracker?.popAll(messages);
86+
if (streamTracker?.maxId == null) sendersInChannel?.remove(senderId);
87+
if (movedMessagesInStreamTracker != null) {
88+
((streamSenders[newStreamId] ??= {})
89+
[senderId] ??= MessageIdTracker()).addAll(movedMessagesInStreamTracker);
90+
}
91+
}
92+
93+
// This is an invariant of [UpdateMessageMoveData].
94+
assert(newStreamId != origStreamId || newTopic != origTopic);
95+
96+
final topicTracker = sendersInTopic?[senderId];
97+
final movedMessagesInTopicTracker = topicTracker?.popAll(messages);
98+
if (topicTracker?.maxId == null) sendersInTopic?.remove(senderId);
99+
if (movedMessagesInTopicTracker != null) {
100+
(((topicSenders[newStreamId] ??= {})[newTopic] ??= {})
101+
[senderId] ??= MessageIdTracker()).addAll(movedMessagesInTopicTracker);
102+
}
103+
}
104+
if (sendersInChannel?.isEmpty ?? false) streamSenders.remove(origStreamId);
105+
if (sendersInTopic?.isEmpty ?? false) topicsInChannel?.remove(origTopic);
106+
if (topicsInChannel?.isEmpty ?? false) topicSenders.remove(origStreamId);
107+
}
108+
71109
void handleDeleteMessageEvent(DeleteMessageEvent event, Map<int, Message> cachedMessages) {
72110
if (event.messageType != MessageType.stream) return;
73111

@@ -155,6 +193,36 @@ class MessageIdTracker {
155193
});
156194
}
157195

196+
/// Remove and return a [QueueList] of message IDs found in [idsToRemove] from
197+
/// the tracker list, or `null` if nothing is removed.
198+
///
199+
/// [idsToRemove] should be sorted ascending.
200+
QueueList<int>? popAll(List<int> idsToRemove) {
201+
final retainedMessageIds = ids.whereNot((id) {
202+
final i = lowerBound(idsToRemove, id);
203+
return i < idsToRemove.length && idsToRemove[i] == id;
204+
});
205+
206+
if (retainedMessageIds.isEmpty) {
207+
// All message IDs in this tracker are removed; this is an optimization
208+
// to clear all ids and return the removed ones without making a new copy.
209+
final result = ids;
210+
ids = QueueList();
211+
return result;
212+
}
213+
214+
QueueList<int>? result;
215+
if (retainedMessageIds.length != ids.length) {
216+
result = QueueList.from(ids.where((id) {
217+
final i = lowerBound(idsToRemove, id);
218+
return i < idsToRemove.length && idsToRemove[i] == id;
219+
}));
220+
ids.setRange(0, retainedMessageIds.length, retainedMessageIds);
221+
ids.length = retainedMessageIds.length;
222+
}
223+
return result;
224+
}
225+
158226
@override
159227
String toString() => ids.toString();
160228
}

‎lib/model/store.dart

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -754,6 +754,7 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, UserStore, Channel
754754

755755
case UpdateMessageEvent():
756756
assert(debugLog("server event: update_message ${event.messageId}"));
757+
recentSenders.handleUpdateMessageEvent(event, messages);
757758
_messages.handleUpdateMessageEvent(event);
758759
unreads.handleUpdateMessageEvent(event);
759760

‎test/model/recent_senders_test.dart

Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,9 @@ import 'package:checks/checks.dart';
22
import 'package:flutter_test/flutter_test.dart';
33
import 'package:zulip/api/model/model.dart';
44
import 'package:zulip/model/recent_senders.dart';
5+
import 'package:zulip/model/store.dart';
56
import '../example_data.dart' as eg;
7+
import 'test_store.dart';
68

79
/// [messages] should be sorted by [id] ascending.
810
void checkMatchesMessages(RecentSenders model, List<Message> messages) {
@@ -142,6 +144,126 @@ void main() {
142144
});
143145
});
144146

147+
group('RecentSenders.handleUpdateMessageUpdate', () {
148+
late PerAccountStore store;
149+
late RecentSenders model;
150+
151+
final origChannel = eg.stream(); final newChannel = eg.stream();
152+
final origTopic = 'origTopic'; final newTopic = 'newTopic';
153+
final userX = eg.user(); final userY = eg.user();
154+
155+
Future<void> prepare(List<Message> messages) async {
156+
store = eg.store();
157+
await store.addMessages(messages);
158+
await store.addStreams([origChannel, newChannel]);
159+
await store.addUsers([userX, userY]);
160+
model = store.recentSenders;
161+
}
162+
163+
List<StreamMessage> copyMessagesWith(Iterable<StreamMessage> messages, {
164+
ZulipStream? newChannel,
165+
String? newTopic,
166+
}) {
167+
assert(newChannel != null || newTopic != null);
168+
return messages.map((message) => StreamMessage.fromJson(
169+
message.toJson()
170+
..['stream_id'] = newChannel?.streamId ?? message.streamId
171+
// See [StreamMessage.displayRecipient] for why this is needed.
172+
..['display_recipient'] = newChannel?.name ?? message.displayRecipient!
173+
174+
..['subject'] = newTopic ?? message.topic
175+
)).toList();
176+
}
177+
178+
test('move a conversation exactly', () async {
179+
final messages = List.generate(10, (i) => eg.streamMessage(
180+
stream: origChannel, topic: origTopic, sender: userX));
181+
await prepare(messages);
182+
final streamSenderIdsBefore = model.streamSenders
183+
[origChannel.streamId]![userX.userId]!.ids;
184+
final topicSenderIdsBefore = model.topicSenders
185+
[origChannel.streamId]![TopicName(origTopic)]![userX.userId]!.ids;
186+
187+
await store.handleEvent(eg.updateMessageEventMoveFrom(
188+
origMessages: messages,
189+
newStreamId: newChannel.streamId,
190+
newTopicStr: newTopic));
191+
checkMatchesMessages(model, copyMessagesWith(
192+
messages, newChannel: newChannel, newTopic: newTopic));
193+
// Check we avoided creating a new list for the moved message IDs.
194+
final streamSenderIdsAfter = model.streamSenders
195+
[newChannel.streamId]![userX.userId]!.ids;
196+
final topicSenderIdsAfter = model.topicSenders
197+
[newChannel.streamId]![TopicName(newTopic)]![userX.userId]!.ids;
198+
check(streamSenderIdsBefore).identicalTo(streamSenderIdsAfter);
199+
check(topicSenderIdsBefore).identicalTo(topicSenderIdsAfter);
200+
});
201+
202+
test('move a conversation partially to a different channel', () async {
203+
final messages = List.generate(10, (i) => eg.streamMessage(
204+
stream: origChannel, topic: origTopic));
205+
final movedMessages = messages.take(5).toList();
206+
final otherMessages = messages.skip(5);
207+
await prepare(messages);
208+
209+
await store.handleEvent(eg.updateMessageEventMoveFrom(
210+
origMessages: movedMessages,
211+
newStreamId: newChannel.streamId));
212+
checkMatchesMessages(model, [
213+
...copyMessagesWith(movedMessages, newChannel: newChannel),
214+
...otherMessages,
215+
]);
216+
});
217+
218+
test('move a conversation partially to a different topic, within the same channel', () async {
219+
final messages = List.generate(10, (i) => eg.streamMessage(
220+
stream: origChannel, topic: origTopic, sender: userX));
221+
final movedMessages = messages.take(5).toList();
222+
final otherMessages = messages.skip(5);
223+
await prepare(messages);
224+
final streamSenderIdsBefore = model.streamSenders
225+
[origChannel.streamId]![userX.userId]!.ids;
226+
227+
await store.handleEvent(eg.updateMessageEventMoveFrom(
228+
origMessages: movedMessages,
229+
newTopicStr: newTopic));
230+
checkMatchesMessages(model, [
231+
...copyMessagesWith(movedMessages, newTopic: newTopic),
232+
...otherMessages,
233+
]);
234+
235+
// Check that we did not touch stream message IDs tracker
236+
// when there wasn't a stream move.
237+
final streamSenderIdsAfter = model.streamSenders
238+
[origChannel.streamId]![userX.userId]!.ids;
239+
check(streamSenderIdsBefore).identicalTo(streamSenderIdsAfter);
240+
});
241+
242+
test('move a conversation with multiple senders', () async {
243+
final messages = [
244+
eg.streamMessage(stream: origChannel, topic: origTopic, sender: userX),
245+
eg.streamMessage(stream: origChannel, topic: origTopic, sender: userX),
246+
eg.streamMessage(stream: origChannel, topic: origTopic, sender: userY),
247+
];
248+
await prepare(messages);
249+
250+
await store.handleEvent(eg.updateMessageEventMoveFrom(
251+
origMessages: messages,
252+
newStreamId: newChannel.streamId));
253+
checkMatchesMessages(model, copyMessagesWith(
254+
messages, newChannel: newChannel));
255+
});
256+
257+
test('message edit update without move', () async {
258+
final messages = List.generate(10, (i) => eg.streamMessage(
259+
stream: origChannel, topic: origTopic));
260+
await prepare(messages);
261+
262+
await store.handleEvent(eg.updateMessageEditEvent(messages[0]));
263+
checkMatchesMessages(model, messages);
264+
});
265+
});
266+
145267
test('RecentSenders.handleDeleteMessageEvent', () {
146268
final model = RecentSenders();
147269
final stream = eg.stream();

0 commit comments

Comments
 (0)