From e2b989d1774c18285f9765a827a14deff6d1413a Mon Sep 17 00:00:00 2001 From: Sayed Mahmood Sayedi Date: Thu, 27 Jun 2024 13:41:17 +0430 Subject: [PATCH 01/11] autocomplete test [nfc]: Break the loop when event-queue wait is not necessary In this specific case, this change is not crucial for the test performance as the loop iterations are way less to cause an issue, but it is a good habit to add it as one would copy this part of the code in the future and repurpose it with large loop iterations; in which it would be a test performance issue. --- test/model/autocomplete_test.dart | 1 + 1 file changed, 1 insertion(+) diff --git a/test/model/autocomplete_test.dart b/test/model/autocomplete_test.dart index 03fbaa9c28..5493598200 100644 --- a/test/model/autocomplete_test.dart +++ b/test/model/autocomplete_test.dart @@ -296,6 +296,7 @@ void main() { check(done).isFalse(); for (int i = 0; i < 3; i++) { await Future(() {}); + if (done) break; } check(done).isTrue(); final results = view.results From 2ff240624c6355ed38d30767edc76bf39dc665de Mon Sep 17 00:00:00 2001 From: Sayed Mahmood Sayedi Date: Sat, 18 May 2024 15:12:38 +0430 Subject: [PATCH 02/11] recent_senders: Introduce `MessageIdTracker` and `RecentSenders` data structures MessageIdTracker data structure is used to keep track of message ids in an ascending sorted list. It is used in RecentSenders data structure. RecentSenders data structure is used to keep track of user messages in topics and streams. Much of this code is transcribed from Zulip web; in particular, from: https://github.com/zulip/zulip/blob/bd04a30bbc6dc5bd7c20940a3d1d34cf8c8c6f28/web/src/recent_senders.ts --- lib/model/message_list.dart | 2 + lib/model/recent_senders.dart | 148 +++++++++++++ lib/model/store.dart | 11 + test/model/message_list_test.dart | 41 ++++ test/model/recent_senders_test.dart | 329 ++++++++++++++++++++++++++++ 5 files changed, 531 insertions(+) create mode 100644 lib/model/recent_senders.dart create mode 100644 test/model/recent_senders_test.dart diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index 4f555f7124..a9535136b2 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -403,6 +403,7 @@ class MessageListView with ChangeNotifier, _MessageSequence { numAfter: 0, ); store.reconcileMessages(result.messages); + store.recentSenders.handleMessages(result.messages); // TODO(#824) for (final message in result.messages) { if (_messageVisible(message)) { _addMessage(message); @@ -439,6 +440,7 @@ class MessageListView with ChangeNotifier, _MessageSequence { } store.reconcileMessages(result.messages); + store.recentSenders.handleMessages(result.messages); // TODO(#824) final fetchedMessages = _allMessagesVisible ? result.messages // Avoid unnecessarily copying the list. diff --git a/lib/model/recent_senders.dart b/lib/model/recent_senders.dart new file mode 100644 index 0000000000..683828d243 --- /dev/null +++ b/lib/model/recent_senders.dart @@ -0,0 +1,148 @@ +import 'package:collection/collection.dart'; +import 'package:flutter/foundation.dart'; + +import '../api/model/events.dart'; +import '../api/model/model.dart'; +import 'algorithms.dart'; + +/// A data structure to keep track of stream and topic messages of users (senders). +/// +/// Use [latestMessageIdOfSenderInStream] and [latestMessageIdOfSenderInTopic] +/// to get the relevant data. +class RecentSenders { + // streamSenders[streamId][senderId] = MessageIdTracker + @visibleForTesting + final Map> streamSenders = {}; + + // topicSenders[streamId][topic][senderId] = MessageIdTracker + @visibleForTesting + final Map>> topicSenders = {}; + + int? latestMessageIdOfSenderInStream({ + required int streamId, + required int senderId, + }) => streamSenders[streamId]?[senderId]?.maxId; + + int? latestMessageIdOfSenderInTopic({ + required int streamId, + required String topic, + required int senderId, + }) => topicSenders[streamId]?[topic]?[senderId]?.maxId; + + /// Records the necessary data from each message if it is a [StreamMessage]. + /// + /// [messages] should be sorted by [id] ascendingly, which are, the way app + /// receives and handles messages. + void handleMessages(List messages) { + final messagesByUserInStream = <(int, int), List>{}; + final messagesByUserInTopic = <(int, String, int), List>{}; + for (final message in messages) { + if (message is! StreamMessage) continue; + final StreamMessage(:streamId, :topic, :senderId, id: int messageId) = message; + (messagesByUserInStream[(streamId, senderId)] ??= []).add(messageId); + (messagesByUserInTopic[(streamId, topic, senderId)] ??= []).add(messageId); + } + + for (final entry in messagesByUserInStream.entries) { + final (streamId, senderId) = entry.key; + ((streamSenders[streamId] ??= {}) + [senderId] ??= MessageIdTracker()).addAll(entry.value); + } + for (final entry in messagesByUserInTopic.entries) { + final (streamId, topic, senderId) = entry.key; + (((topicSenders[streamId] ??= {})[topic] ??= {}) + [senderId] ??= MessageIdTracker()).addAll(entry.value); + } + } + + /// Records the necessary data from [message] if it is a [StreamMessage]. + /// + /// If [message] is not a [StreamMessage], this is a no-op. + void handleMessage(Message message) { + if (message is! StreamMessage) return; + final StreamMessage(:streamId, :topic, :senderId, id: int messageId) = message; + ((streamSenders[streamId] ??= {}) + [senderId] ??= MessageIdTracker()).add(messageId); + (((topicSenders[streamId] ??= {})[topic] ??= {}) + [senderId] ??= MessageIdTracker()).add(messageId); + } + + void handleDeleteMessageEvent(DeleteMessageEvent event, Map cachedMessages) { + if (event.messageType != MessageType.stream) return; + + final messagesByUser = >{}; + for (final id in event.messageIds) { + final message = cachedMessages[id] as StreamMessage?; + if (message == null) continue; + (messagesByUser[message.senderId] ??= []).add(id); + } + + final DeleteMessageEvent(:streamId!, :topic!) = event; + final sendersByStream = streamSenders[streamId]; + final topicsByStream = topicSenders[streamId]; + final sendersByTopic = topicsByStream?[topic]; + for (final entry in messagesByUser.entries) { + final MapEntry(key: senderId, value: messages) = entry; + + final messagesBySenderInStream = sendersByStream?[senderId]; + messagesBySenderInStream?.removeAll(messages); + if (messagesBySenderInStream?.maxId == null) sendersByStream?.remove(senderId); + + final messagesBySenderInTopic = sendersByTopic?[senderId]; + messagesBySenderInTopic?.removeAll(messages); + if (messagesBySenderInTopic?.maxId == null) sendersByTopic?.remove(senderId); + } + if (sendersByStream?.isEmpty ?? false) streamSenders.remove(streamId); + if (sendersByTopic?.isEmpty ?? false) topicsByStream?.remove(topic); + if (topicsByStream?.isEmpty ?? false) topicSenders.remove(streamId); + } +} + +@visibleForTesting +class MessageIdTracker { + /// A list of distinct message IDs, sorted ascendingly. + @visibleForTesting + QueueList ids = QueueList(); + + /// The maximum id in the tracker list, or `null` if the list is empty. + int? get maxId => ids.lastOrNull; + + /// Add the message ID to the tracker list at the proper place, if not present. + /// + /// Optimized, taking O(1) time for the case where that place is the end, + /// because that's the common case for a message that is received through + /// [PerAccountStore.handleEvent]. May take O(n) time in some rare cases. + void add(int id) { + if (ids.isEmpty || id > ids.last) { + ids.addLast(id); + return; + } + final i = lowerBound(ids, id); + if (i < ids.length && ids[i] == id) { + // The ID is already present. Nothing to do. + return; + } + ids.insert(i, id); + } + + /// Add the messages IDs to the tracker list at the proper place, if not present. + /// + /// [newIds] should be sorted ascendingly. + void addAll(List newIds) { + if (ids.isEmpty) { + ids = QueueList.from(newIds); + return; + } + ids = setUnion(ids, newIds); + } + + void removeAll(List idsToRemove) { + ids.removeWhere((id) { + final i = lowerBound(idsToRemove, id); + return i < idsToRemove.length && idsToRemove[i] == id; + }); + } + + @override + String toString() => ids.toString(); +} diff --git a/lib/model/store.dart b/lib/model/store.dart index 0b27a91e9e..953d71283e 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -23,6 +23,7 @@ import 'database.dart'; import 'message.dart'; import 'message_list.dart'; import 'recent_dm_conversations.dart'; +import 'recent_senders.dart'; import 'channel.dart'; import 'typing_status.dart'; import 'unreads.dart'; @@ -256,6 +257,7 @@ class PerAccountStore extends ChangeNotifier with ChannelStore, MessageStore { ), recentDmConversationsView: RecentDmConversationsView( initial: initialSnapshot.recentPrivateConversations, selfUserId: account.userId), + recentSenders: RecentSenders(), ); } @@ -276,6 +278,7 @@ class PerAccountStore extends ChangeNotifier with ChannelStore, MessageStore { required MessageStoreImpl messages, required this.unreads, required this.recentDmConversationsView, + required this.recentSenders, }) : assert(selfUserId == globalStore.getAccount(accountId)!.userId), assert(realmUrl == globalStore.getAccount(accountId)!.realmUrl), assert(realmUrl == connection.realmUrl), @@ -369,6 +372,8 @@ class PerAccountStore extends ChangeNotifier with ChannelStore, MessageStore { final RecentDmConversationsView recentDmConversationsView; + final RecentSenders recentSenders; + //////////////////////////////// // Other digests of data. @@ -492,6 +497,7 @@ class PerAccountStore extends ChangeNotifier with ChannelStore, MessageStore { _messages.handleMessageEvent(event); unreads.handleMessageEvent(event); recentDmConversationsView.handleMessageEvent(event); + recentSenders.handleMessage(event.message); // TODO(#824) // When adding anything here (to handle [MessageEvent]), // it probably belongs in [reconcileMessages] too. @@ -502,6 +508,11 @@ class PerAccountStore extends ChangeNotifier with ChannelStore, MessageStore { case DeleteMessageEvent(): assert(debugLog("server event: delete_message ${event.messageIds}")); + // This should be called before [_messages.handleDeleteMessageEvent(event)], + // as we need to know about each message for [event.messageIds], + // specifically, their `senderId`s. By calling this after the + // aforementioned line, we'll lose reference to those messages. + recentSenders.handleDeleteMessageEvent(event, messages); _messages.handleDeleteMessageEvent(event); unreads.handleDeleteMessageEvent(event); diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index 812a8bcdda..65fda053c5 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -18,6 +18,7 @@ import '../api/model/model_checks.dart'; import '../example_data.dart' as eg; import '../stdlib_checks.dart'; import 'content_checks.dart'; +import 'recent_senders_test.dart' as recent_senders_test; import 'test_store.dart'; void main() { @@ -141,6 +142,25 @@ void main() { ..haveOldest.isTrue(); }); + // TODO(#824): move this test + test('fetchInitial, recent senders track all the messages', () async { + const narrow = CombinedFeedNarrow(); + await prepare(narrow: narrow); + final messages = [ + eg.streamMessage(), + // Not subscribed to the stream with id 10. + eg.streamMessage(stream: eg.stream(streamId: 10)), + ]; + connection.prepare(json: newestResult( + foundOldest: false, + messages: messages, + ).toJson()); + await model.fetchInitial(); + + check(model).messages.length.equals(1); + recent_senders_test.checkMatchesMessages(store.recentSenders, messages); + }); + test('fetchOlder', () async { const narrow = CombinedFeedNarrow(); await prepare(narrow: narrow); @@ -233,6 +253,27 @@ void main() { ..messages.length.equals(200); }); + // TODO(#824): move this test + test('fetchOlder, recent senders track all the messages', () async { + const narrow = CombinedFeedNarrow(); + await prepare(narrow: narrow); + final initialMessages = List.generate(10, (i) => eg.streamMessage(id: 100 + i)); + await prepareMessages(foundOldest: false, messages: initialMessages); + + final oldMessages = List.generate(10, (i) => eg.streamMessage(id: 89 + i)) + // Not subscribed to the stream with id 10. + ..add(eg.streamMessage(id: 99, stream: eg.stream(streamId: 10))); + connection.prepare(json: olderResult( + anchor: 100, foundOldest: false, + messages: oldMessages, + ).toJson()); + await model.fetchOlder(); + + check(model).messages.length.equals(20); + recent_senders_test.checkMatchesMessages(store.recentSenders, + [...initialMessages, ...oldMessages]); + }); + test('MessageEvent', () async { final stream = eg.stream(); await prepare(narrow: StreamNarrow(stream.streamId)); diff --git a/test/model/recent_senders_test.dart b/test/model/recent_senders_test.dart new file mode 100644 index 0000000000..b06f98bc19 --- /dev/null +++ b/test/model/recent_senders_test.dart @@ -0,0 +1,329 @@ +import 'package:checks/checks.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:zulip/api/model/model.dart'; +import 'package:zulip/model/recent_senders.dart'; +import '../example_data.dart' as eg; + +/// [messages] should be sorted by [id] ascendingly. +void checkMatchesMessages(RecentSenders model, List messages) { + final Map>> messagesByUserInStream = {}; + final Map>>> messagesByUserInTopic = {}; + for (final message in messages) { + if (message is! StreamMessage) { + throw UnsupportedError('Message of type ${message.runtimeType} is not expected.'); + } + + final StreamMessage(:streamId, :topic, :senderId, id: int messageId) = message; + + ((messagesByUserInStream[streamId] ??= {}) + [senderId] ??= {}).add(messageId); + (((messagesByUserInTopic[streamId] ??= {})[topic] ??= {}) + [senderId] ??= {}).add(messageId); + } + + final actualMessagesByUserInStream = model.streamSenders.map((streamId, sendersByStream) => + MapEntry(streamId, sendersByStream.map((senderId, tracker) => + MapEntry(senderId, Set.from(tracker.ids))))); + final actualMessagesByUserInTopic = model.topicSenders.map((streamId, topicsByStream) => + MapEntry(streamId, topicsByStream.map((topic, sendersByTopic) => + MapEntry(topic, sendersByTopic.map((senderId, tracker) => + MapEntry(senderId, Set.from(tracker.ids))))))); + + check(actualMessagesByUserInStream).deepEquals(messagesByUserInStream); + check(actualMessagesByUserInTopic).deepEquals(messagesByUserInTopic); +} + +void main() { + test('starts with empty stream and topic senders', () { + final model = RecentSenders(); + checkMatchesMessages(model, []); + }); + + group('RecentSenders.handleMessage', () { + test('stream message gets included', () { + final model = RecentSenders(); + final streamMessage = eg.streamMessage(); + model.handleMessage(streamMessage); + checkMatchesMessages(model, [streamMessage]); + }); + + test('DM message gets ignored', () { + final model = RecentSenders(); + final dmMessage = eg.dmMessage(from: eg.selfUser, to: [eg.otherUser]); + model.handleMessage(dmMessage); + checkMatchesMessages(model, []); + }); + }); + + group('RecentSenders.handleMessages', () { + late RecentSenders model; + final stream1 = eg.stream(streamId: 1); + final user10 = eg.user(userId: 10); + + void setupModel(List messages) { + model = RecentSenders(); + for (final message in messages) { + model.handleMessage(message); + } + } + + group('single tracker', () { + test('batch goes before the existing messages', () { + final existingMessages = [ + eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 300), + eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 400), + ]; + setupModel(existingMessages); + + final messages = [ + eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 100), + eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 200), + ]; + model.handleMessages(messages); + + checkMatchesMessages(model, + [...existingMessages, ...messages]..sort((m1, m2) => m1.id.compareTo(m2.id))); + }); + + test('batch goes after the existing messages', () { + final existingMessages = [ + eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 300), + eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 400), + ]; + setupModel(existingMessages); + + final messages = [ + eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 500), + eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 600), + ]; + model.handleMessages(messages); + + checkMatchesMessages(model, + [...existingMessages, ...messages]..sort((m1, m2) => m1.id.compareTo(m2.id))); + }); + + test('batch is interspersed among the existing messages', () { + final existingMessages = [ + eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 200), + eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 400), + ]; + setupModel(existingMessages); + + final messages = [ + eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 100), + eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 300), + eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 500), + ]; + model.handleMessages(messages); + + checkMatchesMessages(model, + [...existingMessages, ...messages]..sort((m1, m2) => m1.id.compareTo(m2.id))); + }); + + test('batch contains some of already-existing messages', () { + final existingMessages = [ + eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 200), + eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 300), + eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 400), + ]; + setupModel(existingMessages); + + final messages = [ + eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 100), + eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 200), + eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 400), + eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 500), + ]; + model.handleMessages(messages); + + checkMatchesMessages(model, + [...existingMessages, ...messages]..sort((m1, m2) => m1.id.compareTo(m2.id))); + }); + + test('batch with both DM and stream messages -> ignores DM, processes stream messages', () { + final existingMessages = [ + eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 100), + eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 200), + ]; + setupModel(existingMessages); + + final dmMessage = eg.dmMessage(from: eg.otherUser, to: [eg.selfUser], id: 400); + final messages = [ + eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 300), + dmMessage, + eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 500), + ]; + model.handleMessages(messages); + + checkMatchesMessages(model, + [...existingMessages, ...messages..remove(dmMessage)]..sort((m1, m2) => m1.id.compareTo(m2.id))); + }); + }); + + group('multiple trackers', () { + test('batch goes before the existing messages', () { + final existingMessages = [ + eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 300), + eg.streamMessage(stream: stream1, topic: 'b', sender: user10, id: 500), + eg.streamMessage(stream: stream1, topic: 'b', sender: user10, id: 600), + ]; + setupModel(existingMessages); + + final messages = [ + eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 200), + eg.streamMessage(stream: stream1, topic: 'b', sender: user10, id: 400), + ]; + model.handleMessages(messages); + + checkMatchesMessages(model, + [...existingMessages, ...messages]..sort((m1, m2) => m1.id.compareTo(m2.id))); + }); + + test('batch goes after the existing messages', () { + final existingMessages = [ + eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 300), + eg.streamMessage(stream: stream1, topic: 'b', sender: user10, id: 600), + ]; + setupModel(existingMessages); + + final messages = [ + eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 400), + eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 500), + eg.streamMessage(stream: stream1, topic: 'b', sender: user10, id: 700), + ]; + model.handleMessages(messages); + + checkMatchesMessages(model, + [...existingMessages, ...messages]..sort((m1, m2) => m1.id.compareTo(m2.id))); + }); + + test('batch is interspersed among the existing messages', () { + final existingMessages = [ + eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 100), + eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 300), + eg.streamMessage(stream: stream1, topic: 'b', sender: user10, id: 500), + eg.streamMessage(stream: stream1, topic: 'b', sender: user10, id: 700), + ]; + setupModel(existingMessages); + + final messages = [ + eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 200), + eg.streamMessage(stream: stream1, topic: 'b', sender: user10, id: 400), + eg.streamMessage(stream: stream1, topic: 'b', sender: user10, id: 600), + eg.streamMessage(stream: stream1, topic: 'b', sender: user10, id: 800), + ]; + model.handleMessages(messages); + + checkMatchesMessages(model, + [...existingMessages, ...messages]..sort((m1, m2) => m1.id.compareTo(m2.id))); + }); + + test('batch contains some of already-existing messages', () { + final existingMessages = [ + eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 200), + eg.streamMessage(stream: stream1, topic: 'b', sender: user10, id: 300), + eg.streamMessage(stream: stream1, topic: 'b', sender: user10, id: 400), + ]; + setupModel(existingMessages); + + final messages = [ + eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 200), + eg.streamMessage(stream: stream1, topic: 'b', sender: user10, id: 400), + eg.streamMessage(stream: stream1, topic: 'b', sender: user10, id: 500), + ]; + model.handleMessages(messages); + + checkMatchesMessages(model, + [...existingMessages, ...messages]..sort((m1, m2) => m1.id.compareTo(m2.id))); + }); + + test('batch with both DM and stream messages -> ignores DM, processes stream messages', () { + final existingMessages = [ + eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 100), + eg.streamMessage(stream: stream1, topic: 'b', sender: user10, id: 200), + ]; + setupModel(existingMessages); + + final dmMessage = eg.dmMessage(from: eg.otherUser, to: [eg.selfUser], id: 200); + final messages = [ + eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 300), + dmMessage, + eg.streamMessage(stream: stream1, topic: 'b', sender: user10, id: 400), + ]; + model.handleMessages(messages); + + checkMatchesMessages(model, + [...existingMessages, ...messages..remove(dmMessage)]..sort((m1, m2) => m1.id.compareTo(m2.id))); + }); + }); + }); + + test('RecentSenders.handleDeleteMessageEvent', () { + final model = RecentSenders(); + final stream1 = eg.stream(streamId: 1); + final user1 = eg.user(userId: 1); + final user2 = eg.user(userId: 2); + + final messages = [ + eg.streamMessage(stream: stream1, topic: 'a', sender: user1, id: 100), + eg.streamMessage(stream: stream1, topic: 'b', sender: user1, id: 200), + eg.streamMessage(stream: stream1, topic: 'a', sender: user2, id: 300), + ]; + + model.handleMessages(messages); + checkMatchesMessages(model, messages); + + model.handleDeleteMessageEvent(eg.deleteMessageEvent([messages[0], messages[2]]), + Map.fromEntries(messages.map((msg) => MapEntry(msg.id, msg)))); + + checkMatchesMessages(model, [messages[1]]); + }); + + test('RecentSenders.latestMessageIdOfSenderInStream', () { + final model = RecentSenders(); + final stream1 = eg.stream(streamId: 1); + final user10 = eg.user(userId: 10); + + final messages = [ + eg.streamMessage(stream: stream1, sender: user10, id: 100), + eg.streamMessage(stream: stream1, sender: user10, id: 200), + eg.streamMessage(stream: stream1, sender: user10, id: 300), + ]; + + model.handleMessages(messages); + + check(model.latestMessageIdOfSenderInStream( + streamId: 1, senderId: 10)).equals(300); + // No message of user 20 in stream1. + check(model.latestMessageIdOfSenderInStream( + streamId: 1, senderId: 20)).equals(null); + // No message in stream 2 at all. + check(model.latestMessageIdOfSenderInStream( + streamId: 2, senderId: 10)).equals(null); + }); + + test('RecentSenders.latestMessageIdOfSenderInTopic', () { + final model = RecentSenders(); + final stream1 = eg.stream(streamId: 1); + final user10 = eg.user(userId: 10); + + final messages = [ + eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 200), + eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 300), + ]; + + model.handleMessages(messages); + + check(model.latestMessageIdOfSenderInTopic(streamId: 1, + topic: 'a', senderId: 10)).equals(300); + // No message of user 20 in topic "a". + check(model.latestMessageIdOfSenderInTopic(streamId: 1, + topic: 'a', senderId: 20)).equals(null); + // No message in topic "b" at all. + check(model.latestMessageIdOfSenderInTopic(streamId: 1, + topic: 'b', senderId: 10)).equals(null); + // No message in stream 2 at all. + check(model.latestMessageIdOfSenderInTopic(streamId: 2, + topic: 'a', senderId: 10)).equals(null); + }); +} From 76d1404b187776d3ac62b1cfca9aca9d3003c2e9 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 19 Jul 2024 14:57:38 -0700 Subject: [PATCH 03/11] recent_senders test [nfc]: Pull out a checkHandleMessages helper --- test/model/recent_senders_test.dart | 145 ++++++++-------------------- 1 file changed, 41 insertions(+), 104 deletions(-) diff --git a/test/model/recent_senders_test.dart b/test/model/recent_senders_test.dart index b06f98bc19..8cccce9426 100644 --- a/test/model/recent_senders_test.dart +++ b/test/model/recent_senders_test.dart @@ -67,193 +67,130 @@ void main() { } } + void checkHandleMessages(List oldMessages, List newMessages) { + setupModel(oldMessages); + model.handleMessages(newMessages); + final expectedMessages = [...oldMessages, ...newMessages] + ..removeWhere((m) => m is! StreamMessage) + ..sort((m1, m2) => m1.id.compareTo(m2.id)); + checkMatchesMessages(model, expectedMessages); + } + group('single tracker', () { test('batch goes before the existing messages', () { - final existingMessages = [ + checkHandleMessages([ eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 300), eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 400), - ]; - setupModel(existingMessages); - - final messages = [ + ], [ eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 100), eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 200), - ]; - model.handleMessages(messages); - - checkMatchesMessages(model, - [...existingMessages, ...messages]..sort((m1, m2) => m1.id.compareTo(m2.id))); + ]); }); test('batch goes after the existing messages', () { - final existingMessages = [ + checkHandleMessages([ eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 300), eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 400), - ]; - setupModel(existingMessages); - - final messages = [ + ], [ eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 500), eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 600), - ]; - model.handleMessages(messages); - - checkMatchesMessages(model, - [...existingMessages, ...messages]..sort((m1, m2) => m1.id.compareTo(m2.id))); + ]); }); test('batch is interspersed among the existing messages', () { - final existingMessages = [ + checkHandleMessages([ eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 200), eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 400), - ]; - setupModel(existingMessages); - - final messages = [ + ], [ eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 100), eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 300), eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 500), - ]; - model.handleMessages(messages); - - checkMatchesMessages(model, - [...existingMessages, ...messages]..sort((m1, m2) => m1.id.compareTo(m2.id))); + ]); }); test('batch contains some of already-existing messages', () { - final existingMessages = [ + checkHandleMessages([ eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 200), eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 300), eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 400), - ]; - setupModel(existingMessages); - - final messages = [ + ], [ eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 100), eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 200), eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 400), eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 500), - ]; - model.handleMessages(messages); - - checkMatchesMessages(model, - [...existingMessages, ...messages]..sort((m1, m2) => m1.id.compareTo(m2.id))); + ]); }); test('batch with both DM and stream messages -> ignores DM, processes stream messages', () { - final existingMessages = [ + checkHandleMessages([ eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 100), eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 200), - ]; - setupModel(existingMessages); - - final dmMessage = eg.dmMessage(from: eg.otherUser, to: [eg.selfUser], id: 400); - final messages = [ + ], [ eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 300), - dmMessage, + eg.dmMessage(from: eg.otherUser, to: [eg.selfUser], id: 400), eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 500), - ]; - model.handleMessages(messages); - - checkMatchesMessages(model, - [...existingMessages, ...messages..remove(dmMessage)]..sort((m1, m2) => m1.id.compareTo(m2.id))); + ]); }); }); group('multiple trackers', () { test('batch goes before the existing messages', () { - final existingMessages = [ + checkHandleMessages([ eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 300), eg.streamMessage(stream: stream1, topic: 'b', sender: user10, id: 500), eg.streamMessage(stream: stream1, topic: 'b', sender: user10, id: 600), - ]; - setupModel(existingMessages); - - final messages = [ + ], [ eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 200), eg.streamMessage(stream: stream1, topic: 'b', sender: user10, id: 400), - ]; - model.handleMessages(messages); - - checkMatchesMessages(model, - [...existingMessages, ...messages]..sort((m1, m2) => m1.id.compareTo(m2.id))); + ]); }); test('batch goes after the existing messages', () { - final existingMessages = [ + checkHandleMessages([ eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 300), eg.streamMessage(stream: stream1, topic: 'b', sender: user10, id: 600), - ]; - setupModel(existingMessages); - - final messages = [ + ], [ eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 400), eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 500), eg.streamMessage(stream: stream1, topic: 'b', sender: user10, id: 700), - ]; - model.handleMessages(messages); - - checkMatchesMessages(model, - [...existingMessages, ...messages]..sort((m1, m2) => m1.id.compareTo(m2.id))); + ]); }); test('batch is interspersed among the existing messages', () { - final existingMessages = [ + checkHandleMessages([ eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 100), eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 300), eg.streamMessage(stream: stream1, topic: 'b', sender: user10, id: 500), eg.streamMessage(stream: stream1, topic: 'b', sender: user10, id: 700), - ]; - setupModel(existingMessages); - - final messages = [ + ], [ eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 200), eg.streamMessage(stream: stream1, topic: 'b', sender: user10, id: 400), eg.streamMessage(stream: stream1, topic: 'b', sender: user10, id: 600), eg.streamMessage(stream: stream1, topic: 'b', sender: user10, id: 800), - ]; - model.handleMessages(messages); - - checkMatchesMessages(model, - [...existingMessages, ...messages]..sort((m1, m2) => m1.id.compareTo(m2.id))); + ]); }); test('batch contains some of already-existing messages', () { - final existingMessages = [ + checkHandleMessages([ eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 200), eg.streamMessage(stream: stream1, topic: 'b', sender: user10, id: 300), eg.streamMessage(stream: stream1, topic: 'b', sender: user10, id: 400), - ]; - setupModel(existingMessages); - - final messages = [ + ], [ eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 200), eg.streamMessage(stream: stream1, topic: 'b', sender: user10, id: 400), eg.streamMessage(stream: stream1, topic: 'b', sender: user10, id: 500), - ]; - model.handleMessages(messages); - - checkMatchesMessages(model, - [...existingMessages, ...messages]..sort((m1, m2) => m1.id.compareTo(m2.id))); + ]); }); test('batch with both DM and stream messages -> ignores DM, processes stream messages', () { - final existingMessages = [ + checkHandleMessages([ eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 100), eg.streamMessage(stream: stream1, topic: 'b', sender: user10, id: 200), - ]; - setupModel(existingMessages); - - final dmMessage = eg.dmMessage(from: eg.otherUser, to: [eg.selfUser], id: 200); - final messages = [ + ], [ eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 300), - dmMessage, + eg.dmMessage(from: eg.otherUser, to: [eg.selfUser], id: 200), eg.streamMessage(stream: stream1, topic: 'b', sender: user10, id: 400), - ]; - model.handleMessages(messages); - - checkMatchesMessages(model, - [...existingMessages, ...messages..remove(dmMessage)]..sort((m1, m2) => m1.id.compareTo(m2.id))); + ]); }); }); }); From e875302f55b71a96deb6213d0e354242f0d0eb90 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 19 Jul 2024 15:04:04 -0700 Subject: [PATCH 04/11] recent_senders test [nfc]: Pull out checkHandleMessagesSingle helper --- test/model/recent_senders_test.dart | 62 ++++++++++------------------- 1 file changed, 22 insertions(+), 40 deletions(-) diff --git a/test/model/recent_senders_test.dart b/test/model/recent_senders_test.dart index 8cccce9426..8280f03041 100644 --- a/test/model/recent_senders_test.dart +++ b/test/model/recent_senders_test.dart @@ -77,60 +77,42 @@ void main() { } group('single tracker', () { - test('batch goes before the existing messages', () { + void checkHandleMessagesSingle(List oldIds, List newIds) { checkHandleMessages([ - eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 300), - eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 400), + for (final id in oldIds) + eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: id), ], [ - eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 100), - eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 200), + for (final id in newIds) + eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: id), ]); + } + + test('batch goes before the existing messages', () { + checkHandleMessagesSingle([300, 400], [100, 200]); }); test('batch goes after the existing messages', () { - checkHandleMessages([ - eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 300), - eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 400), - ], [ - eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 500), - eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 600), - ]); + checkHandleMessagesSingle([300, 400], [500, 600]); }); test('batch is interspersed among the existing messages', () { - checkHandleMessages([ - eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 200), - eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 400), - ], [ - eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 100), - eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 300), - eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 500), - ]); + checkHandleMessagesSingle([200, 400], [100, 300, 500]); }); test('batch contains some of already-existing messages', () { - checkHandleMessages([ - eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 200), - eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 300), - eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 400), - ], [ - eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 100), - eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 200), - eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 400), - eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 500), - ]); + checkHandleMessagesSingle([200, 300, 400], [100, 200, 400, 500]); }); + }); - test('batch with both DM and stream messages -> ignores DM, processes stream messages', () { - checkHandleMessages([ - eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 100), - eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 200), - ], [ - eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 300), - eg.dmMessage(from: eg.otherUser, to: [eg.selfUser], id: 400), - eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 500), - ]); - }); + test('batch with both DM and stream messages -> ignores DM, processes stream messages', () { + checkHandleMessages([ + eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 100), + eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 200), + ], [ + eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 300), + eg.dmMessage(from: eg.otherUser, to: [eg.selfUser], id: 400), + eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 500), + ]); }); group('multiple trackers', () { From 3a834c419668903531932e096cd33fdab3946a52 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 19 Jul 2024 15:06:14 -0700 Subject: [PATCH 05/11] recent_senders test [nfc]: Cut redundant sort The function we're passing this to doesn't notice the order anyway -- it just puts these into sets. --- test/model/recent_senders_test.dart | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/model/recent_senders_test.dart b/test/model/recent_senders_test.dart index 8280f03041..fbb574abfa 100644 --- a/test/model/recent_senders_test.dart +++ b/test/model/recent_senders_test.dart @@ -71,8 +71,7 @@ void main() { setupModel(oldMessages); model.handleMessages(newMessages); final expectedMessages = [...oldMessages, ...newMessages] - ..removeWhere((m) => m is! StreamMessage) - ..sort((m1, m2) => m1.id.compareTo(m2.id)); + ..removeWhere((m) => m is! StreamMessage); checkMatchesMessages(model, expectedMessages); } From 00ebe6e2617c20f341ba6110facc95dba1ecf279 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 19 Jul 2024 15:37:01 -0700 Subject: [PATCH 06/11] recent_senders test: Test handleMessages implementation This replaces the "multiple trackers" group with test cases that are written to exercise the various scenarios relevant to the logic in RecentSenders.handleMessages. See also discussion here: https://github.com/zulip/zulip-flutter/pull/692#discussion_r1685065783 --- test/model/recent_senders_test.dart | 92 +++++++++-------------------- 1 file changed, 29 insertions(+), 63 deletions(-) diff --git a/test/model/recent_senders_test.dart b/test/model/recent_senders_test.dart index fbb574abfa..d144f15db2 100644 --- a/test/model/recent_senders_test.dart +++ b/test/model/recent_senders_test.dart @@ -58,7 +58,9 @@ void main() { group('RecentSenders.handleMessages', () { late RecentSenders model; final stream1 = eg.stream(streamId: 1); + final stream2 = eg.stream(streamId: 2); final user10 = eg.user(userId: 10); + final user20 = eg.user(userId: 20); void setupModel(List messages) { model = RecentSenders(); @@ -104,75 +106,39 @@ void main() { }); test('batch with both DM and stream messages -> ignores DM, processes stream messages', () { - checkHandleMessages([ - eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 100), - eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 200), - ], [ - eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 300), - eg.dmMessage(from: eg.otherUser, to: [eg.selfUser], id: 400), - eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 500), + checkHandleMessages([], [ + eg.streamMessage(stream: stream1, topic: 'a', sender: user10), + eg.dmMessage(from: eg.otherUser, to: [eg.selfUser]), + eg.streamMessage(stream: stream1, topic: 'a', sender: user10), ]); }); - group('multiple trackers', () { - test('batch goes before the existing messages', () { - checkHandleMessages([ - eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 300), - eg.streamMessage(stream: stream1, topic: 'b', sender: user10, id: 500), - eg.streamMessage(stream: stream1, topic: 'b', sender: user10, id: 600), - ], [ - eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 200), - eg.streamMessage(stream: stream1, topic: 'b', sender: user10, id: 400), - ]); - }); - - test('batch goes after the existing messages', () { - checkHandleMessages([ - eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 300), - eg.streamMessage(stream: stream1, topic: 'b', sender: user10, id: 600), - ], [ - eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 400), - eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 500), - eg.streamMessage(stream: stream1, topic: 'b', sender: user10, id: 700), - ]); - }); + test('add new sender', () { + checkHandleMessages( + [eg.streamMessage(stream: stream1, topic: 'a', sender: user10)], + [eg.streamMessage(stream: stream1, topic: 'a', sender: user20)]); + }); - test('batch is interspersed among the existing messages', () { - checkHandleMessages([ - eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 100), - eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 300), - eg.streamMessage(stream: stream1, topic: 'b', sender: user10, id: 500), - eg.streamMessage(stream: stream1, topic: 'b', sender: user10, id: 700), - ], [ - eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 200), - eg.streamMessage(stream: stream1, topic: 'b', sender: user10, id: 400), - eg.streamMessage(stream: stream1, topic: 'b', sender: user10, id: 600), - eg.streamMessage(stream: stream1, topic: 'b', sender: user10, id: 800), - ]); - }); + test('add new topic', () { + checkHandleMessages( + [eg.streamMessage(stream: stream1, topic: 'a', sender: user10)], + [eg.streamMessage(stream: stream1, topic: 'b', sender: user10)]); + }); - test('batch contains some of already-existing messages', () { - checkHandleMessages([ - eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 200), - eg.streamMessage(stream: stream1, topic: 'b', sender: user10, id: 300), - eg.streamMessage(stream: stream1, topic: 'b', sender: user10, id: 400), - ], [ - eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 200), - eg.streamMessage(stream: stream1, topic: 'b', sender: user10, id: 400), - eg.streamMessage(stream: stream1, topic: 'b', sender: user10, id: 500), - ]); - }); + test('add new stream', () { + checkHandleMessages( + [eg.streamMessage(stream: stream1, topic: 'a', sender: user10)], + [eg.streamMessage(stream: stream2, topic: 'a', sender: user10)]); + }); - test('batch with both DM and stream messages -> ignores DM, processes stream messages', () { - checkHandleMessages([ - eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 100), - eg.streamMessage(stream: stream1, topic: 'b', sender: user10, id: 200), - ], [ - eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: 300), - eg.dmMessage(from: eg.otherUser, to: [eg.selfUser], id: 200), - eg.streamMessage(stream: stream1, topic: 'b', sender: user10, id: 400), - ]); - }); + test('multiple conversations and senders interspersed', () { + checkHandleMessages([], [ + eg.streamMessage(stream: stream1, topic: 'a', sender: user10), + eg.streamMessage(stream: stream1, topic: 'b', sender: user10), + eg.streamMessage(stream: stream2, topic: 'a', sender: user10), + eg.streamMessage(stream: stream1, topic: 'a', sender: user20), + eg.streamMessage(stream: stream1, topic: 'a', sender: user10), + ]); }); }); From f74933b21132513fc460d7cf5b4c20a1ae935d06 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 19 Jul 2024 15:39:09 -0700 Subject: [PATCH 07/11] recent_senders test [nfc]: Cut unneeded IDs; make names look more different Most of these tests don't use the specific stream IDs, user IDs, or message IDs, so cut them out to reduce the number of details for the reader to read. Then also rename the streams, topics, and users so that they're a bit more visually distinct from each other. That helps in seeing what's going on in the tests that are all about which bits of data are the same as, or different from, which other ones. --- test/model/recent_senders_test.dart | 50 ++++++++++++++--------------- 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/test/model/recent_senders_test.dart b/test/model/recent_senders_test.dart index d144f15db2..e69cbcd091 100644 --- a/test/model/recent_senders_test.dart +++ b/test/model/recent_senders_test.dart @@ -57,10 +57,10 @@ void main() { group('RecentSenders.handleMessages', () { late RecentSenders model; - final stream1 = eg.stream(streamId: 1); - final stream2 = eg.stream(streamId: 2); - final user10 = eg.user(userId: 10); - final user20 = eg.user(userId: 20); + final streamA = eg.stream(); + final streamB = eg.stream(); + final userX = eg.user(); + final userY = eg.user(); void setupModel(List messages) { model = RecentSenders(); @@ -81,10 +81,10 @@ void main() { void checkHandleMessagesSingle(List oldIds, List newIds) { checkHandleMessages([ for (final id in oldIds) - eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: id), + eg.streamMessage(stream: streamA, topic: 'a', sender: userX, id: id), ], [ for (final id in newIds) - eg.streamMessage(stream: stream1, topic: 'a', sender: user10, id: id), + eg.streamMessage(stream: streamA, topic: 'a', sender: userX, id: id), ]); } @@ -107,51 +107,51 @@ void main() { test('batch with both DM and stream messages -> ignores DM, processes stream messages', () { checkHandleMessages([], [ - eg.streamMessage(stream: stream1, topic: 'a', sender: user10), + eg.streamMessage(stream: streamA, topic: 'thing', sender: userX), eg.dmMessage(from: eg.otherUser, to: [eg.selfUser]), - eg.streamMessage(stream: stream1, topic: 'a', sender: user10), + eg.streamMessage(stream: streamA, topic: 'thing', sender: userX), ]); }); test('add new sender', () { checkHandleMessages( - [eg.streamMessage(stream: stream1, topic: 'a', sender: user10)], - [eg.streamMessage(stream: stream1, topic: 'a', sender: user20)]); + [eg.streamMessage(stream: streamA, topic: 'thing', sender: userX)], + [eg.streamMessage(stream: streamA, topic: 'thing', sender: userY)]); }); test('add new topic', () { checkHandleMessages( - [eg.streamMessage(stream: stream1, topic: 'a', sender: user10)], - [eg.streamMessage(stream: stream1, topic: 'b', sender: user10)]); + [eg.streamMessage(stream: streamA, topic: 'thing', sender: userX)], + [eg.streamMessage(stream: streamA, topic: 'other', sender: userX)]); }); test('add new stream', () { checkHandleMessages( - [eg.streamMessage(stream: stream1, topic: 'a', sender: user10)], - [eg.streamMessage(stream: stream2, topic: 'a', sender: user10)]); + [eg.streamMessage(stream: streamA, topic: 'thing', sender: userX)], + [eg.streamMessage(stream: streamB, topic: 'thing', sender: userX)]); }); test('multiple conversations and senders interspersed', () { checkHandleMessages([], [ - eg.streamMessage(stream: stream1, topic: 'a', sender: user10), - eg.streamMessage(stream: stream1, topic: 'b', sender: user10), - eg.streamMessage(stream: stream2, topic: 'a', sender: user10), - eg.streamMessage(stream: stream1, topic: 'a', sender: user20), - eg.streamMessage(stream: stream1, topic: 'a', sender: user10), + eg.streamMessage(stream: streamA, topic: 'thing', sender: userX), + eg.streamMessage(stream: streamA, topic: 'other', sender: userX), + eg.streamMessage(stream: streamB, topic: 'thing', sender: userX), + eg.streamMessage(stream: streamA, topic: 'thing', sender: userY), + eg.streamMessage(stream: streamA, topic: 'thing', sender: userX), ]); }); }); test('RecentSenders.handleDeleteMessageEvent', () { final model = RecentSenders(); - final stream1 = eg.stream(streamId: 1); - final user1 = eg.user(userId: 1); - final user2 = eg.user(userId: 2); + final stream = eg.stream(); + final userX = eg.user(); + final userY = eg.user(); final messages = [ - eg.streamMessage(stream: stream1, topic: 'a', sender: user1, id: 100), - eg.streamMessage(stream: stream1, topic: 'b', sender: user1, id: 200), - eg.streamMessage(stream: stream1, topic: 'a', sender: user2, id: 300), + eg.streamMessage(stream: stream, topic: 'thing', sender: userX), + eg.streamMessage(stream: stream, topic: 'other', sender: userX), + eg.streamMessage(stream: stream, topic: 'thing', sender: userY), ]; model.handleMessages(messages); From d3d3774834e1bf387c25d28e2c95c6dbce464447 Mon Sep 17 00:00:00 2001 From: Sayed Mahmood Sayedi Date: Mon, 22 Jul 2024 23:45:08 +0430 Subject: [PATCH 08/11] recent_senders [nfc]: Make `addAll` take `QueueList` instead of `List` This will avoid copying the list on each call to `addAll`. --- lib/model/recent_senders.dart | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/model/recent_senders.dart b/lib/model/recent_senders.dart index 683828d243..e458c822b8 100644 --- a/lib/model/recent_senders.dart +++ b/lib/model/recent_senders.dart @@ -34,13 +34,13 @@ class RecentSenders { /// [messages] should be sorted by [id] ascendingly, which are, the way app /// receives and handles messages. void handleMessages(List messages) { - final messagesByUserInStream = <(int, int), List>{}; - final messagesByUserInTopic = <(int, String, int), List>{}; + final messagesByUserInStream = <(int, int), QueueList>{}; + final messagesByUserInTopic = <(int, String, int), QueueList>{}; for (final message in messages) { if (message is! StreamMessage) continue; final StreamMessage(:streamId, :topic, :senderId, id: int messageId) = message; - (messagesByUserInStream[(streamId, senderId)] ??= []).add(messageId); - (messagesByUserInTopic[(streamId, topic, senderId)] ??= []).add(messageId); + (messagesByUserInStream[(streamId, senderId)] ??= QueueList()).add(messageId); + (messagesByUserInTopic[(streamId, topic, senderId)] ??= QueueList()).add(messageId); } for (final entry in messagesByUserInStream.entries) { @@ -128,9 +128,9 @@ class MessageIdTracker { /// Add the messages IDs to the tracker list at the proper place, if not present. /// /// [newIds] should be sorted ascendingly. - void addAll(List newIds) { + void addAll(QueueList newIds) { if (ids.isEmpty) { - ids = QueueList.from(newIds); + ids = newIds; return; } ids = setUnion(ids, newIds); From 73c827cd823d476d702d34e0701a01d835563ef7 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 23 Jul 2024 10:25:38 -0700 Subject: [PATCH 09/11] recent_senders [nfc]: Rename some foosByBar that mean foosInBar A map of "foos by bar" is a map where each key is a "bar", and each value is a "foo" -- the phrase can be read as an abbreviation of "foos indexed by bar" or "foos, where each foo is indexed by its respective bar". So in this function `messagesByUser` is accurately named because a key represents a user and a value represents some messages. But these other variables should be e.g. `topicsInStream`, not `topicsByStream`. That one isn't a map where the key is a stream and the value is a topic or topics; rather it's a collection of topics (and more data about them) which is the collection specifically of topics in the stream we're acting on. --- lib/model/recent_senders.dart | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/lib/model/recent_senders.dart b/lib/model/recent_senders.dart index e458c822b8..6386e7a87e 100644 --- a/lib/model/recent_senders.dart +++ b/lib/model/recent_senders.dart @@ -78,23 +78,23 @@ class RecentSenders { } final DeleteMessageEvent(:streamId!, :topic!) = event; - final sendersByStream = streamSenders[streamId]; - final topicsByStream = topicSenders[streamId]; - final sendersByTopic = topicsByStream?[topic]; + final sendersInStream = streamSenders[streamId]; + final topicsInStream = topicSenders[streamId]; + final sendersInTopic = topicsInStream?[topic]; for (final entry in messagesByUser.entries) { final MapEntry(key: senderId, value: messages) = entry; - final messagesBySenderInStream = sendersByStream?[senderId]; - messagesBySenderInStream?.removeAll(messages); - if (messagesBySenderInStream?.maxId == null) sendersByStream?.remove(senderId); + final streamTracker = sendersInStream?[senderId]; + streamTracker?.removeAll(messages); + if (streamTracker?.maxId == null) sendersInStream?.remove(senderId); - final messagesBySenderInTopic = sendersByTopic?[senderId]; - messagesBySenderInTopic?.removeAll(messages); - if (messagesBySenderInTopic?.maxId == null) sendersByTopic?.remove(senderId); + final topicTracker = sendersInTopic?[senderId]; + topicTracker?.removeAll(messages); + if (topicTracker?.maxId == null) sendersInTopic?.remove(senderId); } - if (sendersByStream?.isEmpty ?? false) streamSenders.remove(streamId); - if (sendersByTopic?.isEmpty ?? false) topicsByStream?.remove(topic); - if (topicsByStream?.isEmpty ?? false) topicSenders.remove(streamId); + if (sendersInStream?.isEmpty ?? false) streamSenders.remove(streamId); + if (sendersInTopic?.isEmpty ?? false) topicsInStream?.remove(topic); + if (topicsInStream?.isEmpty ?? false) topicSenders.remove(streamId); } } From eca8af9dd8c11dbad08ce6c366bbea6e7bf16c3d Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 23 Jul 2024 10:42:51 -0700 Subject: [PATCH 10/11] recent_senders [nfc]: Add and revise some docs --- lib/model/recent_senders.dart | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/lib/model/recent_senders.dart b/lib/model/recent_senders.dart index 6386e7a87e..a9fb7550a6 100644 --- a/lib/model/recent_senders.dart +++ b/lib/model/recent_senders.dart @@ -5,10 +5,10 @@ import '../api/model/events.dart'; import '../api/model/model.dart'; import 'algorithms.dart'; -/// A data structure to keep track of stream and topic messages of users (senders). +/// Tracks the latest messages sent by each user, in each stream and topic. /// /// Use [latestMessageIdOfSenderInStream] and [latestMessageIdOfSenderInTopic] -/// to get the relevant data. +/// for queries. class RecentSenders { // streamSenders[streamId][senderId] = MessageIdTracker @visibleForTesting @@ -18,21 +18,24 @@ class RecentSenders { @visibleForTesting final Map>> topicSenders = {}; + /// The latest message the given user sent to the given stream, + /// or null if no such message is known. int? latestMessageIdOfSenderInStream({ required int streamId, required int senderId, }) => streamSenders[streamId]?[senderId]?.maxId; + /// The latest message the given user sent to the given topic, + /// or null if no such message is known. int? latestMessageIdOfSenderInTopic({ required int streamId, required String topic, required int senderId, }) => topicSenders[streamId]?[topic]?[senderId]?.maxId; - /// Records the necessary data from each message if it is a [StreamMessage]. + /// Records the necessary data from a batch of just-fetched messages. /// - /// [messages] should be sorted by [id] ascendingly, which are, the way app - /// receives and handles messages. + /// The messages must be sorted by [Message.id] ascending. void handleMessages(List messages) { final messagesByUserInStream = <(int, int), QueueList>{}; final messagesByUserInTopic = <(int, String, int), QueueList>{}; @@ -55,9 +58,7 @@ class RecentSenders { } } - /// Records the necessary data from [message] if it is a [StreamMessage]. - /// - /// If [message] is not a [StreamMessage], this is a no-op. + /// Records the necessary data from a new message. void handleMessage(Message message) { if (message is! StreamMessage) return; final StreamMessage(:streamId, :topic, :senderId, id: int messageId) = message; From dbb2dcfbf4e8e2239b2dca0142bbc3fab3cc0640 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 23 Jul 2024 10:43:57 -0700 Subject: [PATCH 11/11] recent_senders [nfc]: Write "sorted ascending" --- lib/model/recent_senders.dart | 4 ++-- test/model/recent_senders_test.dart | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/model/recent_senders.dart b/lib/model/recent_senders.dart index a9fb7550a6..d075d05eee 100644 --- a/lib/model/recent_senders.dart +++ b/lib/model/recent_senders.dart @@ -101,7 +101,7 @@ class RecentSenders { @visibleForTesting class MessageIdTracker { - /// A list of distinct message IDs, sorted ascendingly. + /// A list of distinct message IDs, sorted ascending. @visibleForTesting QueueList ids = QueueList(); @@ -128,7 +128,7 @@ class MessageIdTracker { /// Add the messages IDs to the tracker list at the proper place, if not present. /// - /// [newIds] should be sorted ascendingly. + /// [newIds] should be sorted ascending. void addAll(QueueList newIds) { if (ids.isEmpty) { ids = newIds; diff --git a/test/model/recent_senders_test.dart b/test/model/recent_senders_test.dart index e69cbcd091..2da516e11c 100644 --- a/test/model/recent_senders_test.dart +++ b/test/model/recent_senders_test.dart @@ -4,7 +4,7 @@ import 'package:zulip/api/model/model.dart'; import 'package:zulip/model/recent_senders.dart'; import '../example_data.dart' as eg; -/// [messages] should be sorted by [id] ascendingly. +/// [messages] should be sorted by [id] ascending. void checkMatchesMessages(RecentSenders model, List messages) { final Map>> messagesByUserInStream = {}; final Map>>> messagesByUserInTopic = {};