From b75dc7d688be2ac8b5d6e0c9b4a5767b6f0c123a Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 30 Jun 2023 12:41:49 -0700 Subject: [PATCH 1/8] store [nfc]: Reorder fields to put realm info together Technically maxFileUploadSizeMib is for the whole server, I think, but it seems fine to treat it as realm info for the purpose of ordering these. See Greg's bulleted list here: https://github.com/zulip/zulip-flutter/pull/183#pullrequestreview-1480273242 --- lib/model/store.dart | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/model/store.dart b/lib/model/store.dart index 734784bd46..1a2da64188 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -149,6 +149,7 @@ class PerAccountStore extends ChangeNotifier { required this.connection, required InitialSnapshot initialSnapshot, }) : zulipVersion = initialSnapshot.zulipVersion, + maxFileUploadSizeMib = initialSnapshot.maxFileUploadSizeMib, users = Map.fromEntries( initialSnapshot.realmUsers .followedBy(initialSnapshot.realmNonActiveUsers) @@ -157,18 +158,17 @@ class PerAccountStore extends ChangeNotifier { streams = Map.fromEntries(initialSnapshot.streams.map( (stream) => MapEntry(stream.streamId, stream))), subscriptions = Map.fromEntries(initialSnapshot.subscriptions.map( - (subscription) => MapEntry(subscription.streamId, subscription))), - maxFileUploadSizeMib = initialSnapshot.maxFileUploadSizeMib; + (subscription) => MapEntry(subscription.streamId, subscription))); final Account account; final ApiConnection connection; // TODO(#135): update zulipFeatureLevel with events // TODO(#135): Keep all this data updated by handling Zulip events from the server. final String zulipVersion; // TODO get from account; update there on initial snapshot + final int maxFileUploadSizeMib; // No event for this. final Map users; final Map streams; final Map subscriptions; - final int maxFileUploadSizeMib; // No event for this. // TODO lots more data. When adding, be sure to update handleEvent too. From d7af63ec9ff419b031c22c71ec1fc5185f970312 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 30 Jun 2023 13:04:24 -0700 Subject: [PATCH 2/8] store [nfc]: Add some one-line headings for PerAccountStore data fields --- lib/model/store.dart | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/lib/model/store.dart b/lib/model/store.dart index 1a2da64188..30ce4e13bd 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -164,9 +164,15 @@ class PerAccountStore extends ChangeNotifier { final ApiConnection connection; // TODO(#135): update zulipFeatureLevel with events // TODO(#135): Keep all this data updated by handling Zulip events from the server. + + // Data attached to the realm or the server. final String zulipVersion; // TODO get from account; update there on initial snapshot final int maxFileUploadSizeMib; // No event for this. + + // Users and data about them. final Map users; + + // Streams, topics, and stuff about them. final Map streams; final Map subscriptions; From 68d30ee5702cd91279985fee284564ab22f13a0b Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 3 Jul 2023 11:48:19 -0700 Subject: [PATCH 3/8] test [nfc]: Let callers customize initial snapshot from example data --- test/example_data.dart | 48 +++++++++++++++++++---------- test/model/store_test.dart | 10 +++--- test/widgets/action_sheet_test.dart | 2 +- test/widgets/content_test.dart | 2 +- test/widgets/store_test.dart | 6 ++-- 5 files changed, 42 insertions(+), 26 deletions(-) diff --git a/test/example_data.dart b/test/example_data.dart index fa61f11894..70bbc0f542 100644 --- a/test/example_data.dart +++ b/test/example_data.dart @@ -167,26 +167,42 @@ DmMessage dmMessage( // TODO example data for many more types -final InitialSnapshot initialSnapshot = InitialSnapshot( - queueId: '1:2345', - lastEventId: 1, - zulipFeatureLevel: recentZulipFeatureLevel, - zulipVersion: recentZulipVersion, - zulipMergeBase: recentZulipVersion, - alertWords: ['klaxon'], - customProfileFields: [], - subscriptions: [], // TODO add subscriptions to example initial snapshot - streams: [], // TODO add streams to example initial snapshot - maxFileUploadSizeMib: 25, - realmUsers: [], - realmNonActiveUsers: [], - crossRealmBots: [], -); +InitialSnapshot initialSnapshot({ + String? queueId, + int? lastEventId, + int? zulipFeatureLevel, + String? zulipVersion, + String? zulipMergeBase, + List? alertWords, + List? customProfileFields, + List? subscriptions, + List? streams, + int? maxFileUploadSizeMib, + List? realmUsers, + List? realmNonActiveUsers, + List? crossRealmBots, +}) { + return InitialSnapshot( + queueId: queueId ?? '1:2345', + lastEventId: lastEventId ?? 1, + zulipFeatureLevel: zulipFeatureLevel ?? recentZulipFeatureLevel, + zulipVersion: zulipVersion ?? recentZulipVersion, + zulipMergeBase: zulipMergeBase ?? recentZulipVersion, + alertWords: alertWords ?? ['klaxon'], + customProfileFields: customProfileFields ?? [], + subscriptions: subscriptions ?? [], // TODO add subscriptions to default + streams: streams ?? [], // TODO add streams to default + maxFileUploadSizeMib: maxFileUploadSizeMib ?? 25, + realmUsers: realmUsers ?? [], + realmNonActiveUsers: realmNonActiveUsers ?? [], + crossRealmBots: crossRealmBots ?? [], + ); +} PerAccountStore store() { return PerAccountStore.fromInitialSnapshot( account: selfAccount, connection: FakeApiConnection.fromAccount(selfAccount), - initialSnapshot: initialSnapshot, + initialSnapshot: initialSnapshot(), ); } diff --git a/test/model/store_test.dart b/test/model/store_test.dart index 91e308f497..70105e94e9 100644 --- a/test/model/store_test.dart +++ b/test/model/store_test.dart @@ -22,7 +22,7 @@ void main() { final store1 = PerAccountStore.fromInitialSnapshot( account: account1, connection: FakeApiConnection.fromAccount(account1), - initialSnapshot: eg.initialSnapshot, + initialSnapshot: eg.initialSnapshot(), ); completers(1).single.complete(store1); check(await future1).identicalTo(store1); @@ -33,7 +33,7 @@ void main() { final store2 = PerAccountStore.fromInitialSnapshot( account: account2, connection: FakeApiConnection.fromAccount(account2), - initialSnapshot: eg.initialSnapshot, + initialSnapshot: eg.initialSnapshot(), ); completers(2).single.complete(store2); check(await future2).identicalTo(store2); @@ -60,12 +60,12 @@ void main() { final store1 = PerAccountStore.fromInitialSnapshot( account: account1, connection: FakeApiConnection.fromAccount(account1), - initialSnapshot: eg.initialSnapshot, + initialSnapshot: eg.initialSnapshot(), ); final store2 = PerAccountStore.fromInitialSnapshot( account: account2, connection: FakeApiConnection.fromAccount(account2), - initialSnapshot: eg.initialSnapshot, + initialSnapshot: eg.initialSnapshot(), ); completers(1).single.complete(store1); completers(2).single.complete(store2); @@ -90,7 +90,7 @@ void main() { final store1 = PerAccountStore.fromInitialSnapshot( account: account1, connection: FakeApiConnection.fromAccount(account1), - initialSnapshot: eg.initialSnapshot, + initialSnapshot: eg.initialSnapshot(), ); completers(1).single.complete(store1); await pumpEventQueue(); diff --git a/test/widgets/action_sheet_test.dart b/test/widgets/action_sheet_test.dart index bb3b92a352..29563a72a3 100644 --- a/test/widgets/action_sheet_test.dart +++ b/test/widgets/action_sheet_test.dart @@ -26,7 +26,7 @@ Future setupToMessageActionSheet(WidgetTester tester, { }) async { addTearDown(TestDataBinding.instance.reset); - await TestDataBinding.instance.globalStore.add(eg.selfAccount, eg.initialSnapshot); + await TestDataBinding.instance.globalStore.add(eg.selfAccount, eg.initialSnapshot()); final store = await TestDataBinding.instance.globalStore.perAccount(eg.selfAccount.id); store.addUser(eg.user(userId: message.senderId)); if (message is StreamMessage) { diff --git a/test/widgets/content_test.dart b/test/widgets/content_test.dart index 51f3dd1c1e..5ad9ee894e 100644 --- a/test/widgets/content_test.dart +++ b/test/widgets/content_test.dart @@ -20,7 +20,7 @@ void main() { Future actualAuthHeader(WidgetTester tester, String src) async { final globalStore = TestDataBinding.instance.globalStore; addTearDown(TestDataBinding.instance.reset); - await globalStore.add(eg.selfAccount, eg.initialSnapshot); + await globalStore.add(eg.selfAccount, eg.initialSnapshot()); final httpClient = _FakeHttpClient(); debugNetworkImageHttpClientProvider = () => httpClient; diff --git a/test/widgets/store_test.dart b/test/widgets/store_test.dart index 10a8d5c4bb..bbb9adf859 100644 --- a/test/widgets/store_test.dart +++ b/test/widgets/store_test.dart @@ -31,7 +31,7 @@ void main() { check(tester.any(find.byType(CircularProgressIndicator))).isFalse(); check(globalStore).identicalTo(TestDataBinding.instance.globalStore); - await TestDataBinding.instance.globalStore.add(eg.selfAccount, eg.initialSnapshot); + await TestDataBinding.instance.globalStore.add(eg.selfAccount, eg.initialSnapshot()); check(globalStore).isNotNull() .accountEntries.single .equals((accountId: eg.selfAccount.id, account: eg.selfAccount)); @@ -40,7 +40,7 @@ void main() { testWidgets('PerAccountStoreWidget basic', (tester) async { final globalStore = TestDataBinding.instance.globalStore; addTearDown(TestDataBinding.instance.reset); - await globalStore.add(eg.selfAccount, eg.initialSnapshot); + await globalStore.add(eg.selfAccount, eg.initialSnapshot()); await tester.pumpWidget( Directionality( @@ -62,7 +62,7 @@ void main() { testWidgets('PerAccountStoreWidget immediate data after first loaded', (tester) async { final globalStore = TestDataBinding.instance.globalStore; addTearDown(TestDataBinding.instance.reset); - await globalStore.add(eg.selfAccount, eg.initialSnapshot); + await globalStore.add(eg.selfAccount, eg.initialSnapshot()); await tester.pumpWidget( Directionality( From 8a7d664632326b945baffde4010e2ab017e39383 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 30 Jun 2023 12:33:03 -0700 Subject: [PATCH 4/8] api: Add recentPrivateConversations list in initial snapshot --- lib/api/model/initial_snapshot.dart | 23 +++++++++++++++++++++++ lib/api/model/initial_snapshot.g.dart | 20 ++++++++++++++++++++ test/example_data.dart | 2 ++ 3 files changed, 45 insertions(+) diff --git a/lib/api/model/initial_snapshot.dart b/lib/api/model/initial_snapshot.dart index 86ecd4e9e0..4cd2305647 100644 --- a/lib/api/model/initial_snapshot.dart +++ b/lib/api/model/initial_snapshot.dart @@ -24,6 +24,8 @@ class InitialSnapshot { // TODO etc., etc. + final List recentPrivateConversations; + final List subscriptions; final List streams; @@ -66,6 +68,7 @@ class InitialSnapshot { this.zulipMergeBase, required this.alertWords, required this.customProfileFields, + required this.recentPrivateConversations, required this.subscriptions, required this.streams, required this.maxFileUploadSizeMib, @@ -79,3 +82,23 @@ class InitialSnapshot { Map toJson() => _$InitialSnapshotToJson(this); } + +/// An item in `recent_private_conversations`. +/// +/// For docs, search for "recent_private_conversations:" +/// in . +@JsonSerializable(fieldRename: FieldRename.snake) +class RecentDmConversation { + final int maxMessageId; + final List userIds; + + RecentDmConversation({ + required this.maxMessageId, + required this.userIds, + }); + + factory RecentDmConversation.fromJson(Map json) => + _$RecentDmConversationFromJson(json); + + Map toJson() => _$RecentDmConversationToJson(this); +} diff --git a/lib/api/model/initial_snapshot.g.dart b/lib/api/model/initial_snapshot.g.dart index 6c1f8049d8..34832c30eb 100644 --- a/lib/api/model/initial_snapshot.g.dart +++ b/lib/api/model/initial_snapshot.g.dart @@ -21,6 +21,10 @@ InitialSnapshot _$InitialSnapshotFromJson(Map json) => customProfileFields: (json['custom_profile_fields'] as List) .map((e) => CustomProfileField.fromJson(e as Map)) .toList(), + recentPrivateConversations: (json['recent_private_conversations'] + as List) + .map((e) => RecentDmConversation.fromJson(e as Map)) + .toList(), subscriptions: (json['subscriptions'] as List) .map((e) => Subscription.fromJson(e as Map)) .toList(), @@ -52,6 +56,7 @@ Map _$InitialSnapshotToJson(InitialSnapshot instance) => 'zulip_merge_base': instance.zulipMergeBase, 'alert_words': instance.alertWords, 'custom_profile_fields': instance.customProfileFields, + 'recent_private_conversations': instance.recentPrivateConversations, 'subscriptions': instance.subscriptions, 'streams': instance.streams, 'max_file_upload_size_mib': instance.maxFileUploadSizeMib, @@ -59,3 +64,18 @@ Map _$InitialSnapshotToJson(InitialSnapshot instance) => 'realm_non_active_users': instance.realmNonActiveUsers, 'cross_realm_bots': instance.crossRealmBots, }; + +RecentDmConversation _$RecentDmConversationFromJson( + Map json) => + RecentDmConversation( + maxMessageId: json['max_message_id'] as int, + userIds: + (json['user_ids'] as List).map((e) => e as int).toList(), + ); + +Map _$RecentDmConversationToJson( + RecentDmConversation instance) => + { + 'max_message_id': instance.maxMessageId, + 'user_ids': instance.userIds, + }; diff --git a/test/example_data.dart b/test/example_data.dart index 70bbc0f542..31209c7a6f 100644 --- a/test/example_data.dart +++ b/test/example_data.dart @@ -175,6 +175,7 @@ InitialSnapshot initialSnapshot({ String? zulipMergeBase, List? alertWords, List? customProfileFields, + List? recentPrivateConversations, List? subscriptions, List? streams, int? maxFileUploadSizeMib, @@ -190,6 +191,7 @@ InitialSnapshot initialSnapshot({ zulipMergeBase: zulipMergeBase ?? recentZulipVersion, alertWords: alertWords ?? ['klaxon'], customProfileFields: customProfileFields ?? [], + recentPrivateConversations: recentPrivateConversations ?? [], subscriptions: subscriptions ?? [], // TODO add subscriptions to default streams: streams ?? [], // TODO add streams to default maxFileUploadSizeMib: maxFileUploadSizeMib ?? 25, From ac11ef8b30794579a1daabdad41b5cadd8525c64 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 30 Jun 2023 17:37:21 -0700 Subject: [PATCH 5/8] deps: Add `collection` as direct dependency --- pubspec.lock | 2 +- pubspec.yaml | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/pubspec.lock b/pubspec.lock index 3bcecbde03..ef2b9caeca 100644 --- a/pubspec.lock +++ b/pubspec.lock @@ -178,7 +178,7 @@ packages: source: hosted version: "4.5.0" collection: - dependency: transitive + dependency: "direct main" description: name: collection sha256: f092b211a4319e98e5ff58223576de6c2803db36221657b46c82574721240687 diff --git a/pubspec.yaml b/pubspec.yaml index 94c218e82a..0aabfc054e 100644 --- a/pubspec.yaml +++ b/pubspec.yaml @@ -55,6 +55,7 @@ dependencies: app_settings: ^4.2.0 image_picker: ^0.8.7+3 package_info_plus: ^4.0.1 + collection: ^1.17.2 dev_dependencies: flutter_test: From a7e3529fe0c4c7b8379f62735d3502d3b96303e1 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 3 Jul 2023 13:48:22 -0700 Subject: [PATCH 6/8] test [nfc]: Allow passing id in eg.{stream,dm}Message --- test/example_data.dart | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/test/example_data.dart b/test/example_data.dart index 31209c7a6f..da24aa274d 100644 --- a/test/example_data.dart +++ b/test/example_data.dart @@ -120,8 +120,14 @@ Map _messagePropertiesFromContent(String? content, String? cont const _stream = stream; -StreamMessage streamMessage( - {User? sender, ZulipStream? stream, String? topic, String? content, String? contentMarkdown}) { +StreamMessage streamMessage({ + int? id, + User? sender, + ZulipStream? stream, + String? topic, + String? content, + String? contentMarkdown, +}) { final effectiveStream = stream ?? _stream(); // The use of JSON here is convenient in order to delegate parts of the data // to helper functions. The main downside is that it loses static typing @@ -135,7 +141,7 @@ StreamMessage streamMessage( 'display_recipient': effectiveStream.name, 'stream_id': effectiveStream.streamId, 'flags': [], - 'id': 1234567, // TODO generate example IDs + 'id': id ?? 1234567, // TODO generate example IDs 'subject': topic ?? 'example topic', 'timestamp': 1678139636, 'type': 'stream', @@ -146,8 +152,13 @@ StreamMessage streamMessage( /// /// See also: /// * [streamMessage], to construct an example stream message. -DmMessage dmMessage( - {required User from, required List to, String? content, String? contentMarkdown}) { +DmMessage dmMessage({ + int? id, + required User from, + required List to, + String? content, + String? contentMarkdown, +}) { assert(!to.any((user) => user.userId == from.userId)); return DmMessage.fromJson({ ..._messagePropertiesBase, @@ -158,7 +169,7 @@ DmMessage dmMessage( .toList(growable: false), 'flags': [], - 'id': 1234567, // TODO generate example IDs + 'id': id ?? 1234567, // TODO generate example IDs 'subject': '', 'timestamp': 1678139636, 'type': 'private', From 606975b49ed8b79b634372932863d943a6eab165 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 3 Jul 2023 14:03:52 -0700 Subject: [PATCH 7/8] narrow: Implement `toString` on {Stream,Topic,Dm}Narrow classes This will be helpful for test-failure output, particularly with DmNarrows as we start using those as Map keys, soon. Of our current Narrow subclasses, this just leaves AllMessagesNarrow without a custom `toString` implementation. That's OK, because the default `toString` already says "AllMessagesNarrow", which is all we need to know about the narrow. --- lib/model/narrow.dart | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/lib/model/narrow.dart b/lib/model/narrow.dart index c3b8ea5025..a0b1c740f5 100644 --- a/lib/model/narrow.dart +++ b/lib/model/narrow.dart @@ -70,6 +70,9 @@ class StreamNarrow extends Narrow { @override ApiNarrow apiEncode() => [ApiNarrowStream(streamId)]; + @override + String toString() => 'StreamNarrow($streamId)'; + @override bool operator ==(Object other) { if (other is! StreamNarrow) return false; @@ -102,6 +105,9 @@ class TopicNarrow extends Narrow implements SendableNarrow { @override StreamDestination get destination => StreamDestination(streamId, topic); + @override + String toString() => 'TopicNarrow($streamId, $topic)'; + @override bool operator ==(Object other) { if (other is! TopicNarrow) return false; @@ -207,6 +213,9 @@ class DmNarrow extends Narrow implements SendableNarrow { @override ApiNarrow apiEncode() => [ApiNarrowDm(allRecipientIds)]; + @override + String toString() => 'DmNarrow(allRecipientIds: $allRecipientIds)'; + @override bool operator ==(Object other) { if (other is! DmNarrow) return false; From 3550d87fb21eabde9ab90ae2070b03f92cd902a3 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 30 Jun 2023 17:27:16 -0700 Subject: [PATCH 8/8] store: Add RecentDmConversationsView view-model Toward #119, the list of recent DM conversations. Much of this code was transcribed from zulip-mobile; in particular, from: src/pm-conversations/pmConversationsModel.js src/pm-conversations/__tests__/pmConversationsModel-test.js --- lib/model/narrow.dart | 9 ++ lib/model/recent_dm_conversations.dart | 126 ++++++++++++++++ lib/model/store.dart | 9 +- .../model/recent_dm_conversations_checks.dart | 9 ++ test/model/recent_dm_conversations_test.dart | 142 ++++++++++++++++++ 5 files changed, 294 insertions(+), 1 deletion(-) create mode 100644 lib/model/recent_dm_conversations.dart create mode 100644 test/model/recent_dm_conversations_checks.dart create mode 100644 test/model/recent_dm_conversations_test.dart diff --git a/lib/model/narrow.dart b/lib/model/narrow.dart index a0b1c740f5..6076612561 100644 --- a/lib/model/narrow.dart +++ b/lib/model/narrow.dart @@ -1,4 +1,5 @@ +import '../api/model/initial_snapshot.dart'; import '../api/model/model.dart'; import '../api/model/narrow.dart'; import '../api/route/messages.dart'; @@ -152,6 +153,14 @@ class DmNarrow extends Narrow implements SendableNarrow { ); } + /// A [DmNarrow] from an item in [InitialSnapshot.recentPrivateConversations]. + factory DmNarrow.ofRecentDmConversation(RecentDmConversation conversation, {required int selfUserId}) { + return DmNarrow( + allRecipientIds: [...conversation.userIds, selfUserId]..sort(), + selfUserId: selfUserId, + ); + } + /// The user IDs of everyone in the conversation, sorted. /// /// Each message in the conversation is sent by one of these users diff --git a/lib/model/recent_dm_conversations.dart b/lib/model/recent_dm_conversations.dart new file mode 100644 index 0000000000..95e278c369 --- /dev/null +++ b/lib/model/recent_dm_conversations.dart @@ -0,0 +1,126 @@ +import 'package:collection/collection.dart'; +import 'package:flutter/foundation.dart'; + +import '../api/model/initial_snapshot.dart'; +import '../api/model/model.dart'; +import '../api/model/events.dart'; +import 'narrow.dart'; + +/// A view-model for the recent-DM-conversations UI. +/// +/// This maintains the list of recent DM conversations, +/// plus additional data in order to efficiently maintain the list. +class RecentDmConversationsView extends ChangeNotifier { + factory RecentDmConversationsView({ + required List initial, + required selfUserId, + }) { + final entries = initial.map((conversation) => MapEntry( + DmNarrow.ofRecentDmConversation(conversation, selfUserId: selfUserId), + conversation.maxMessageId, + )).toList()..sort((a, b) => -a.value.compareTo(b.value)); + return RecentDmConversationsView._( + map: Map.fromEntries(entries), + sorted: QueueList.from(entries.map((e) => e.key)), + selfUserId: selfUserId, + ); + } + + RecentDmConversationsView._({ + required this.map, + required this.sorted, + required this.selfUserId, + }); + + /// The latest message ID in each conversation. + final Map map; + + /// The [DmNarrow] keys of the map, sorted by latest message descending. + final QueueList sorted; + + final int selfUserId; + + /// Insert the key at the proper place in [sorted]. + /// + /// Optimized, taking O(1) time, for the case where that place is the start, + /// because that's the common case for a new message. + /// May take O(n) time in general. + void _insertSorted(DmNarrow key, int msgId) { + final i = sorted.indexWhere((k) => map[k]! < msgId); + // QueueList is a deque, with O(1) to add at start or end. + switch (i) { + case == 0: + sorted.addFirst(key); + case < 0: + sorted.addLast(key); + default: + sorted.insert(i, key); + } + } + + /// Handle [MessageEvent], updating [map] and [sorted]. + /// + /// Can take linear time in general. That sounds inefficient... + /// but it's what the webapp does, so must not be catastrophic. 🤷 + /// (In fact the webapp calls `Array#sort`, + /// which takes at *least* linear time, and may be 𝛳(N log N).) + /// + /// The point of the event is that we're learning about the message + /// in real time immediately after it was sent -- + /// so the overwhelmingly common case is that the message + /// is newer than any existing message we know about. (*) + /// That's therefore the case we optimize for, + /// particularly in the helper _insertSorted. + /// + /// (*) In fact at present that's the only possible case. + /// The alternative will become possible when we have the analogue of + /// zulip-mobile's FETCH_MESSAGES_COMPLETE, with fetches done for a + /// [MessageListView] reporting their messages back via the [PerAccountStore] + /// to all our central data structures. + /// Then that can race with a new-message event: for example, + /// say we get a fetch-messages result that includes the just-sent + /// message 1002, and only after that get the event about message 1001, + /// sent moments earlier. The event queue always delivers events in order, so + /// even the race is possible only because we fetch messages outside of the + /// event queue. + void handleMessageEvent(MessageEvent event) { + final message = event.message; + if (message is! DmMessage) { + return; + } + final key = DmNarrow.ofMessage(message, selfUserId: selfUserId); + final prev = map[key]; + if (prev == null) { + // The conversation is new. Add to both `map` and `sorted`. + map[key] = message.id; + _insertSorted(key, message.id); + } else if (prev >= message.id) { + // The conversation already has a newer message. + // This should be impossible as long as we only listen for messages coming + // through the event system, which sends events in order. + // Anyway, do nothing. + } else { + // The conversation needs to be (a) updated in `map`... + map[key] = message.id; + + // ... and (b) possibly moved around in `sorted` to keep the list sorted. + final i = sorted.indexOf(key); + assert(i >= 0, 'key in map should be in sorted'); + if (i == 0) { + // The conversation was already the latest, so no reordering needed. + // (This is likely a common case in practice -- happens every time + // the user gets several DMs in a row in the same thread -- so good to + // optimize.) + } else { + // It wasn't the latest. Just handle the general case. + sorted.removeAt(i); // linear time, ouch + _insertSorted(key, message.id); + } + } + notifyListeners(); + } + + // TODO update from messages loaded in message lists. When doing so, + // review handleMessageEvent so it acknowledges the subtle races that can + // happen when taking data from outside the event system. +} diff --git a/lib/model/store.dart b/lib/model/store.dart index 30ce4e13bd..d6d335f80b 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -16,6 +16,7 @@ import '../log.dart'; import 'autocomplete.dart'; import 'database.dart'; import 'message_list.dart'; +import 'recent_dm_conversations.dart'; export 'package:drift/drift.dart' show Value; export 'database.dart' show Account, AccountsCompanion; @@ -158,7 +159,9 @@ class PerAccountStore extends ChangeNotifier { streams = Map.fromEntries(initialSnapshot.streams.map( (stream) => MapEntry(stream.streamId, stream))), subscriptions = Map.fromEntries(initialSnapshot.subscriptions.map( - (subscription) => MapEntry(subscription.streamId, subscription))); + (subscription) => MapEntry(subscription.streamId, subscription))), + recentDmConversationsView = RecentDmConversationsView( + initial: initialSnapshot.recentPrivateConversations, selfUserId: account.userId); final Account account; final ApiConnection connection; // TODO(#135): update zulipFeatureLevel with events @@ -178,6 +181,9 @@ class PerAccountStore extends ChangeNotifier { // TODO lots more data. When adding, be sure to update handleEvent too. + // TODO call [RecentDmConversationsView.dispose] in [dispose] + final RecentDmConversationsView recentDmConversationsView; + final Set _messageListViews = {}; void registerMessageList(MessageListView view) { @@ -260,6 +266,7 @@ class PerAccountStore extends ChangeNotifier { notifyListeners(); } else if (event is MessageEvent) { assert(debugLog("server event: message ${jsonEncode(event.message.toJson())}")); + recentDmConversationsView.handleMessageEvent(event); for (final view in _messageListViews) { view.maybeAddMessage(event.message); } diff --git a/test/model/recent_dm_conversations_checks.dart b/test/model/recent_dm_conversations_checks.dart new file mode 100644 index 0000000000..af92ca25ac --- /dev/null +++ b/test/model/recent_dm_conversations_checks.dart @@ -0,0 +1,9 @@ +import 'package:checks/checks.dart'; +import 'package:collection/collection.dart'; +import 'package:zulip/model/narrow.dart'; +import 'package:zulip/model/recent_dm_conversations.dart'; + +extension RecentDmConversationsViewChecks on Subject { + Subject> get map => has((v) => v.map, 'map'); + Subject> get sorted => has((v) => v.sorted, 'sorted'); +} diff --git a/test/model/recent_dm_conversations_test.dart b/test/model/recent_dm_conversations_test.dart new file mode 100644 index 0000000000..c09ca53527 --- /dev/null +++ b/test/model/recent_dm_conversations_test.dart @@ -0,0 +1,142 @@ +import 'package:checks/checks.dart'; +import 'package:test/scaffolding.dart'; +import 'package:zulip/api/model/events.dart'; +import 'package:zulip/api/model/initial_snapshot.dart'; +import 'package:zulip/model/narrow.dart'; +import 'package:zulip/model/recent_dm_conversations.dart'; + +import '../example_data.dart' as eg; +import 'recent_dm_conversations_checks.dart'; + +void main() { + group('RecentDmConversationsView', () { + /// Get a [DmNarrow] from a list of recipient IDs excluding self. + DmNarrow key(userIds) { + return DmNarrow( + allRecipientIds: [eg.selfUser.userId, ...userIds]..sort(), + selfUserId: eg.selfUser.userId, + ); + } + + test('construct from initial data', () { + check(RecentDmConversationsView(selfUserId: eg.selfUser.userId, + initial: [])) + ..map.isEmpty() + ..sorted.isEmpty(); + + check(RecentDmConversationsView(selfUserId: eg.selfUser.userId, + initial: [ + RecentDmConversation(userIds: [], maxMessageId: 200), + RecentDmConversation(userIds: [1], maxMessageId: 100), + RecentDmConversation(userIds: [2, 1], maxMessageId: 300), // userIds out of order + ])) + ..map.deepEquals({ + key([1, 2]): 300, + key([]): 200, + key([1]): 100, + }) + ..sorted.deepEquals([key([1, 2]), key([]), key([1])]); + }); + + group('message event (new message)', () { + setupView() { + return RecentDmConversationsView(selfUserId: eg.selfUser.userId, + initial: [ + RecentDmConversation(userIds: [1], maxMessageId: 200), + RecentDmConversation(userIds: [1, 2], maxMessageId: 100), + ]); + } + + test('(check base state)', () { + // This is here mostly for checked documentation of what + // setupView returns, to help in reading the other test cases. + check(setupView()) + ..map.deepEquals({ + key([1]): 200, + key([1, 2]): 100, + }) + ..sorted.deepEquals([key([1]), key([1, 2])]); + }); + + test('stream message -> do nothing', () { + bool listenersNotified = false; + final expected = setupView(); + check(setupView() + ..addListener(() { listenersNotified = true; }) + ..handleMessageEvent(MessageEvent(id: 1, message: eg.streamMessage())) + ) ..map.deepEquals(expected.map) + ..sorted.deepEquals(expected.sorted); + check(listenersNotified).isFalse(); + }); + + test('new conversation, newest message', () { + bool listenersNotified = false; + final message = eg.dmMessage(id: 300, from: eg.selfUser, to: [eg.user(userId: 2)]); + check(setupView() + ..addListener(() { listenersNotified = true; }) + ..handleMessageEvent(MessageEvent(id: 1, message: message)) + ) ..map.deepEquals({ + key([2]): 300, + key([1]): 200, + key([1, 2]): 100, + }) + ..sorted.deepEquals([key([2]), key([1]), key([1, 2])]); + check(listenersNotified).isTrue(); + }); + + test('new conversation, not newest message', () { + bool listenersNotified = false; + final message = eg.dmMessage(id: 150, from: eg.selfUser, to: [eg.user(userId: 2)]); + check(setupView() + ..addListener(() { listenersNotified = true; }) + ..handleMessageEvent(MessageEvent(id: 1, message: message)) + ) ..map.deepEquals({ + key([1]): 200, + key([2]): 150, + key([1, 2]): 100, + }) + ..sorted.deepEquals([key([1]), key([2]), key([1, 2])]); + check(listenersNotified).isTrue(); + }); + + test('existing conversation, newest message', () { + bool listenersNotified = false; + final message = eg.dmMessage(id: 300, from: eg.selfUser, + to: [eg.user(userId: 1), eg.user(userId: 2)]); + check(setupView() + ..addListener(() { listenersNotified = true; }) + ..handleMessageEvent(MessageEvent(id: 1, message: message)) + ) ..map.deepEquals({ + key([1, 2]): 300, + key([1]): 200, + }) + ..sorted.deepEquals([key([1, 2]), key([1])]); + check(listenersNotified).isTrue(); + }); + + test('existing newest conversation, newest message', () { + bool listenersNotified = false; + final message = eg.dmMessage(id: 300, from: eg.selfUser, to: [eg.user(userId: 1)]); + check(setupView() + ..addListener(() { listenersNotified = true; }) + ..handleMessageEvent(MessageEvent(id: 1, message: message)) + ) ..map.deepEquals({ + key([1]): 300, + key([1, 2]): 100, + }) + ..sorted.deepEquals([key([1]), key([1, 2])]); + check(listenersNotified).isTrue(); + }); + + test('existing conversation, not newest in conversation', () { + final message = eg.dmMessage(id: 99, from: eg.selfUser, + to: [eg.user(userId: 1), eg.user(userId: 2)]); + final expected = setupView(); + check(setupView() + ..handleMessageEvent(MessageEvent(id: 1, message: message)) + ) ..map.deepEquals(expected.map) + ..sorted.deepEquals(expected.sorted); + }); + }); + }); +}