diff --git a/lib/model/autocomplete.dart b/lib/model/autocomplete.dart index 560b69a3e9..5250dba6d4 100644 --- a/lib/model/autocomplete.dart +++ b/lib/model/autocomplete.dart @@ -125,23 +125,11 @@ class AutocompleteViewManager { assert(removed); } - void handleRealmUserAddEvent(RealmUserAddEvent event) { - for (final view in _mentionAutocompleteViews) { - view.refreshStaleUserResults(); - } - } - void handleRealmUserRemoveEvent(RealmUserRemoveEvent event) { - for (final view in _mentionAutocompleteViews) { - view.refreshStaleUserResults(); - } autocompleteDataCache.invalidateUser(event.userId); } void handleRealmUserUpdateEvent(RealmUserUpdateEvent event) { - for (final view in _mentionAutocompleteViews) { - view.refreshStaleUserResults(); - } autocompleteDataCache.invalidateUser(event.userId); } @@ -176,17 +164,54 @@ class AutocompleteViewManager { /// * When the object will no longer be used, call [dispose] to free /// resources on the [PerAccountStore]. class MentionAutocompleteView extends ChangeNotifier { - MentionAutocompleteView._({required this.store, required this.narrow}); + MentionAutocompleteView._({ + required this.store, + required this.narrow, + required this.sortedUsers, + }); factory MentionAutocompleteView.init({ required PerAccountStore store, required Narrow narrow, }) { - final view = MentionAutocompleteView._(store: store, narrow: narrow); + final view = MentionAutocompleteView._( + store: store, + narrow: narrow, + sortedUsers: _usersByRelevance(store: store, narrow: narrow), + ); store.autocompleteViewManager.registerMentionAutocomplete(view); return view; } + static List _usersByRelevance({ + required PerAccountStore store, + required Narrow narrow, + }) { + assert(narrow is! CombinedFeedNarrow); + return store.users.values.toList() + ..sort((userA, userB) => compareByDms(userA, userB, store: store)); + } + + /// Determines which of the two users is more recent in DM conversations. + /// + /// Returns a negative number if [userA] is more recent than [userB], + /// returns a positive number if [userB] is more recent than [userA], + /// and returns `0` if both [userA] and [userB] are equally recent + /// or there is no DM exchanged with them whatsoever. + @visibleForTesting + static int compareByDms(User userA, User userB, {required PerAccountStore store}) { + final recentDms = store.recentDmConversationsView; + final aLatestMessageId = recentDms.latestMessagesByRecipient[userA.userId]; + final bLatestMessageId = recentDms.latestMessagesByRecipient[userB.userId]; + + return switch((aLatestMessageId, bLatestMessageId)) { + (int a, int b) => -a.compareTo(b), + (int(), _) => -1, + (_, int()) => 1, + _ => 0, + }; + } + @override void dispose() { store.autocompleteViewManager.unregisterMentionAutocomplete(this); @@ -198,6 +223,7 @@ class MentionAutocompleteView extends ChangeNotifier { final PerAccountStore store; final Narrow narrow; + final List sortedUsers; MentionAutocompleteQuery? get query => _query; MentionAutocompleteQuery? _query; @@ -208,15 +234,6 @@ class MentionAutocompleteView extends ChangeNotifier { } } - /// Recompute user results for the current query, if any. - /// - /// Called in particular when we get a [RealmUserEvent]. - void refreshStaleUserResults() { - if (_query != null) { - _startSearch(_query!); - } - } - /// Called when the app is reassembled during debugging, e.g. for hot reload. /// /// This will redo the search from scratch for the current query, if any. @@ -230,18 +247,7 @@ class MentionAutocompleteView extends ChangeNotifier { List _results = []; Future _startSearch(MentionAutocompleteQuery query) async { - List? newResults; - - while (true) { - try { - newResults = await _computeResults(query); - break; - } on ConcurrentModificationError { - // Retry - // TODO backoff? - } - } - + final newResults = await _computeResults(query); if (newResults == null) { // Query was old; new search is in progress. Or, no listeners to notify. return; @@ -253,9 +259,7 @@ class MentionAutocompleteView extends ChangeNotifier { Future?> _computeResults(MentionAutocompleteQuery query) async { final List results = []; - final Iterable users = store.users.values; - - final iterator = users.iterator; + final iterator = sortedUsers.iterator; bool isDone = false; while (!isDone) { // CPU perf: End this task; enqueue a new one for resuming this work @@ -266,7 +270,7 @@ class MentionAutocompleteView extends ChangeNotifier { } for (int i = 0; i < 1000; i++) { - if (!iterator.moveNext()) { // Can throw ConcurrentModificationError + if (!iterator.moveNext()) { isDone = true; break; } @@ -277,7 +281,7 @@ class MentionAutocompleteView extends ChangeNotifier { } } } - return results; // TODO(#228) sort for most relevant first + return results; } } diff --git a/lib/model/recent_dm_conversations.dart b/lib/model/recent_dm_conversations.dart index 6c93b84db0..b38610be15 100644 --- a/lib/model/recent_dm_conversations.dart +++ b/lib/model/recent_dm_conversations.dart @@ -1,3 +1,5 @@ +import 'dart:math'; + import 'package:collection/collection.dart'; import 'package:flutter/foundation.dart'; @@ -19,9 +21,21 @@ class RecentDmConversationsView extends ChangeNotifier { DmNarrow.ofRecentDmConversation(conversation, selfUserId: selfUserId), conversation.maxMessageId, )).toList()..sort((a, b) => -a.value.compareTo(b.value)); + + final latestMessagesByRecipient = {}; + for (final entry in entries) { + final dmNarrow = entry.key; + final maxMessageId = entry.value; + for (final userId in dmNarrow.otherRecipientIds) { + // Only take the latest message of a user across all the conversations. + latestMessagesByRecipient.putIfAbsent(userId, () => maxMessageId); + } + } + return RecentDmConversationsView._( map: Map.fromEntries(entries), sorted: QueueList.from(entries.map((e) => e.key)), + latestMessagesByRecipient: latestMessagesByRecipient, selfUserId: selfUserId, ); } @@ -29,6 +43,7 @@ class RecentDmConversationsView extends ChangeNotifier { RecentDmConversationsView._({ required this.map, required this.sorted, + required this.latestMessagesByRecipient, required this.selfUserId, }); @@ -38,6 +53,15 @@ class RecentDmConversationsView extends ChangeNotifier { /// The [DmNarrow] keys of [map], sorted by latest message descending. final QueueList sorted; + /// Map from user ID to the latest message ID in any conversation with the user. + /// + /// Both 1:1 and group DM conversations are considered. + /// The self-user ID is excluded even if there is a self-DM conversation. + /// + /// (The identified message was not necessarily sent by the identified user; + /// it might have been sent by anyone in its conversation.) + final Map latestMessagesByRecipient; + final int selfUserId; /// Insert the key at the proper place in [sorted]. @@ -58,7 +82,7 @@ class RecentDmConversationsView extends ChangeNotifier { } } - /// Handle [MessageEvent], updating [map] and [sorted]. + /// Handle [MessageEvent], updating [map], [sorted], and [latestMessagesByRecipient]. /// /// Can take linear time in general. That sounds inefficient... /// but it's what the webapp does, so must not be catastrophic. 🤷 @@ -89,6 +113,8 @@ class RecentDmConversationsView extends ChangeNotifier { return; } final key = DmNarrow.ofMessage(message, selfUserId: selfUserId); + + // Update [map] and [sorted]. final prev = map[key]; if (prev == null) { // The conversation is new. Add to both `map` and `sorted`. @@ -117,6 +143,16 @@ class RecentDmConversationsView extends ChangeNotifier { _insertSorted(key, message.id); } } + + // Update [latestMessagesByRecipient]. + for (final recipient in key.otherRecipientIds) { + latestMessagesByRecipient.update( + recipient, + (latestMessageId) => max(message.id, latestMessageId), + ifAbsent: () => message.id, + ); + } + notifyListeners(); } diff --git a/lib/model/store.dart b/lib/model/store.dart index baae805051..27c7bd2f42 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -418,7 +418,6 @@ class PerAccountStore extends ChangeNotifier with StreamStore, MessageStore { } else if (event is RealmUserAddEvent) { assert(debugLog("server event: realm_user/add")); users[event.person.userId] = event.person; - autocompleteViewManager.handleRealmUserAddEvent(event); notifyListeners(); } else if (event is RealmUserRemoveEvent) { assert(debugLog("server event: realm_user/remove")); diff --git a/test/model/autocomplete_test.dart b/test/model/autocomplete_test.dart index 576a6aa262..03fbaa9c28 100644 --- a/test/model/autocomplete_test.dart +++ b/test/model/autocomplete_test.dart @@ -5,9 +5,11 @@ import 'package:checks/checks.dart'; import 'package:fake_async/fake_async.dart'; import 'package:flutter/cupertino.dart'; import 'package:test/scaffolding.dart'; +import 'package:zulip/api/model/initial_snapshot.dart'; import 'package:zulip/api/model/model.dart'; import 'package:zulip/model/autocomplete.dart'; import 'package:zulip/model/narrow.dart'; +import 'package:zulip/model/store.dart'; import 'package:zulip/widgets/compose_box.dart'; import '../example_data.dart' as eg; @@ -166,7 +168,7 @@ void main() { }); test('MentionAutocompleteView misc', () async { - const narrow = CombinedFeedNarrow(); + const narrow = StreamNarrow(1); final store = eg.store(); await store.addUsers([eg.selfUser, eg.otherUser, eg.thirdUser]); final view = MentionAutocompleteView.init(store: store, narrow: narrow); @@ -183,7 +185,7 @@ void main() { test('MentionAutocompleteView not starve timers', () { fakeAsync((binding) async { - const narrow = CombinedFeedNarrow(); + const narrow = StreamNarrow(1); final store = eg.store(); await store.addUsers([eg.selfUser, eg.otherUser, eg.thirdUser]); final view = MentionAutocompleteView.init(store: store, narrow: narrow); @@ -218,7 +220,7 @@ void main() { }); test('MentionAutocompleteView yield between batches of 1000', () async { - const narrow = CombinedFeedNarrow(); + const narrow = StreamNarrow(1); final store = eg.store(); for (int i = 0; i < 2500; i++) { await store.addUser(eg.user(userId: i, email: 'user$i@example.com', fullName: 'User $i')); @@ -241,7 +243,7 @@ void main() { }); test('MentionAutocompleteView new query during computation replaces old', () async { - const narrow = CombinedFeedNarrow(); + const narrow = StreamNarrow(1); final store = eg.store(); for (int i = 0; i < 1500; i++) { await store.addUser(eg.user(userId: i, email: 'user$i@example.com', fullName: 'User $i')); @@ -274,35 +276,35 @@ void main() { } }); - test('MentionAutocompleteView mutating store.users while in progress causes retry', () async { - const narrow = CombinedFeedNarrow(); + test('MentionAutocompleteView mutating store.users while in progress does not ' + 'prevent query from finishing', () async { + const narrow = StreamNarrow(1); final store = eg.store(); - for (int i = 0; i < 1500; i++) { + for (int i = 0; i < 2500; i++) { await store.addUser(eg.user(userId: i, email: 'user$i@example.com', fullName: 'User $i')); } final view = MentionAutocompleteView.init(store: store, narrow: narrow); bool done = false; view.addListener(() { done = true; }); - view.query = MentionAutocompleteQuery('User 10000'); + view.query = MentionAutocompleteQuery('User 110'); await Future(() {}); check(done).isFalse(); - await store.addUser(eg.user(userId: 10000, email: 'user10000@example.com', fullName: 'User 10000')); + await store.addUser(eg.user(userId: 11000, email: 'user11000@example.com', fullName: 'User 11000')); await Future(() {}); check(done).isFalse(); - await Future(() {}); - check(done).isTrue(); - check(view.results).single - .isA() - .userId.equals(10000); - // new result sticks; no "zombie" result from `store.users` pre-mutation - for (int i = 0; i < 10; i++) { // for good measure + for (int i = 0; i < 3; i++) { await Future(() {}); - check(view.results).single - .isA() - .userId.equals(10000); } + check(done).isTrue(); + final results = view.results + .map((e) => (e as UserMentionAutocompleteResult).userId); + check(results) + ..contains(110) + ..contains(1100) + // Does not include the newly-added user as we finish the query with stale users. + ..not((results) => results.contains(11000)); }); group('MentionAutocompleteQuery.testUser', () { @@ -349,4 +351,127 @@ void main() { doCheck('Four F', eg.user(fullName: 'Full Name Four Words'), false); }); }); + + group('MentionAutocompleteView sorting users results', () { + late PerAccountStore store; + + Future prepare({ + List users = const [], + List dmConversations = const [], + }) async { + store = eg.store(initialSnapshot: eg.initialSnapshot( + recentPrivateConversations: dmConversations)); + await store.addUsers(users); + } + + group('MentionAutocompleteView.compareByDms', () { + const idA = 1; + const idB = 2; + + int compareAB() => MentionAutocompleteView.compareByDms( + eg.user(userId: idA), + eg.user(userId: idB), + store: store, + ); + + test('has DMs with userA and userB, latest with userA -> prioritizes userA', () async { + await prepare(dmConversations: [ + RecentDmConversation(userIds: [idA], maxMessageId: 200), + RecentDmConversation(userIds: [idA, idB], maxMessageId: 100), + ]); + check(compareAB()).isLessThan(0); + }); + + test('has DMs with userA and userB, latest with userB -> prioritizes userB', () async { + await prepare(dmConversations: [ + RecentDmConversation(userIds: [idB], maxMessageId: 200), + RecentDmConversation(userIds: [idA, idB], maxMessageId: 100), + ]); + check(compareAB()).isGreaterThan(0); + }); + + test('has DMs with userA and userB, equally recent -> prioritizes neither', () async { + await prepare(dmConversations: [ + RecentDmConversation(userIds: [idA, idB], maxMessageId: 100), + ]); + check(compareAB()).equals(0); + }); + + test('has DMs with userA but not userB -> prioritizes userA', () async { + await prepare(dmConversations: [ + RecentDmConversation(userIds: [idA], maxMessageId: 100), + ]); + check(compareAB()).isLessThan(0); + }); + + test('has DMs with userB but not userA -> prioritizes userB', () async { + await prepare(dmConversations: [ + RecentDmConversation(userIds: [idB], maxMessageId: 100), + ]); + check(compareAB()).isGreaterThan(0); + }); + + test('has no DMs with userA or userB -> prioritizes neither', () async { + await prepare(dmConversations: []); + check(compareAB()).equals(0); + }); + }); + + group('autocomplete suggests relevant users in the intended order', () { + // The order should be: + // 1. Users most recent in the DM conversations + + Future checkResultsIn(Narrow narrow, {required List expected}) async { + final users = [ + eg.user(userId: 0), + eg.user(userId: 1), + eg.user(userId: 2), + eg.user(userId: 3), + eg.user(userId: 4), + ]; + + final dmConversations = [ + RecentDmConversation(userIds: [3], maxMessageId: 300), + RecentDmConversation(userIds: [0], maxMessageId: 200), + RecentDmConversation(userIds: [0, 1], maxMessageId: 100), + ]; + + await prepare(users: users, dmConversations: dmConversations); + final view = MentionAutocompleteView.init(store: store, narrow: narrow); + + bool done = false; + view.addListener(() { done = true; }); + view.query = MentionAutocompleteQuery(''); + await Future(() {}); + check(done).isTrue(); + final results = view.results + .map((e) => (e as UserMentionAutocompleteResult).userId); + check(results).deepEquals(expected); + } + + test('StreamNarrow', () async { + await checkResultsIn(const StreamNarrow(1), expected: [3, 0, 1, 2, 4]); + }); + + test('TopicNarrow', () async { + await checkResultsIn(const TopicNarrow(1, 'topic'), expected: [3, 0, 1, 2, 4]); + }); + + test('DmNarrow', () async { + await checkResultsIn( + DmNarrow.withUser(eg.selfUser.userId, selfUserId: eg.selfUser.userId), + expected: [3, 0, 1, 2, 4], + ); + }); + + test('CombinedFeedNarrow', () async { + // As we do not expect a compose box in [CombinedFeedNarrow], it should + // not proceed to show any results. + await check(checkResultsIn( + const CombinedFeedNarrow(), + expected: [0, 1, 2, 3, 4]) + ).throws(); + }); + }); + }); } diff --git a/test/model/recent_dm_conversations_checks.dart b/test/model/recent_dm_conversations_checks.dart index af92ca25ac..bbb78593f9 100644 --- a/test/model/recent_dm_conversations_checks.dart +++ b/test/model/recent_dm_conversations_checks.dart @@ -6,4 +6,6 @@ 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'); + Subject> get latestMessagesByRecipient => has( + (v) => v.latestMessagesByRecipient, 'latestMessagesByRecipient'); } diff --git a/test/model/recent_dm_conversations_test.dart b/test/model/recent_dm_conversations_test.dart index f4ae547ee4..5ef12a130b 100644 --- a/test/model/recent_dm_conversations_test.dart +++ b/test/model/recent_dm_conversations_test.dart @@ -22,7 +22,8 @@ void main() { check(RecentDmConversationsView(selfUserId: eg.selfUser.userId, initial: [])) ..map.isEmpty() - ..sorted.isEmpty(); + ..sorted.isEmpty() + ..latestMessagesByRecipient.isEmpty(); check(RecentDmConversationsView(selfUserId: eg.selfUser.userId, initial: [ @@ -35,7 +36,8 @@ void main() { key([]): 200, key([1]): 100, }) - ..sorted.deepEquals([key([1, 2]), key([]), key([1])]); + ..sorted.deepEquals([key([1, 2]), key([]), key([1])]) + ..latestMessagesByRecipient.deepEquals({1: 300, 2: 300}); }); group('message event (new message)', () { @@ -55,7 +57,8 @@ void main() { key([1]): 200, key([1, 2]): 100, }) - ..sorted.deepEquals([key([1]), key([1, 2])]); + ..sorted.deepEquals([key([1]), key([1, 2])]) + ..latestMessagesByRecipient.deepEquals({1: 200, 2: 100}); }); test('stream message -> do nothing', () { @@ -65,7 +68,8 @@ void main() { ..addListener(() { listenersNotified = true; }) ..handleMessageEvent(MessageEvent(id: 1, message: eg.streamMessage())) ) ..map.deepEquals(expected.map) - ..sorted.deepEquals(expected.sorted); + ..sorted.deepEquals(expected.sorted) + ..latestMessagesByRecipient.deepEquals(expected.latestMessagesByRecipient); check(listenersNotified).isFalse(); }); @@ -80,7 +84,8 @@ void main() { key([1]): 200, key([1, 2]): 100, }) - ..sorted.deepEquals([key([2]), key([1]), key([1, 2])]); + ..sorted.deepEquals([key([2]), key([1]), key([1, 2])]) + ..latestMessagesByRecipient.deepEquals({2: 300, 1: 200}); check(listenersNotified).isTrue(); }); @@ -95,7 +100,8 @@ void main() { key([2]): 150, key([1, 2]): 100, }) - ..sorted.deepEquals([key([1]), key([2]), key([1, 2])]); + ..sorted.deepEquals([key([1]), key([2]), key([1, 2])]) + ..latestMessagesByRecipient.deepEquals({1: 200, 2: 150}); check(listenersNotified).isTrue(); }); @@ -110,7 +116,8 @@ void main() { key([1, 2]): 300, key([1]): 200, }) - ..sorted.deepEquals([key([1, 2]), key([1])]); + ..sorted.deepEquals([key([1, 2]), key([1])]) + ..latestMessagesByRecipient.deepEquals({1: 300, 2: 300}); check(listenersNotified).isTrue(); }); @@ -124,18 +131,58 @@ void main() { key([1]): 300, key([1, 2]): 100, }) - ..sorted.deepEquals([key([1]), key([1, 2])]); + ..sorted.deepEquals([key([1]), key([1, 2])]) + ..latestMessagesByRecipient.deepEquals({1: 300, 2: 100}); check(listenersNotified).isTrue(); }); test('existing conversation, not newest in conversation', () { + // bool listenersNotified = false; final message = eg.dmMessage(id: 99, from: eg.selfUser, to: [eg.user(userId: 1), eg.user(userId: 2)]); final expected = setupView(); check(setupView() + // ..addListener(() { listenersNotified = true; }) ..handleMessageEvent(MessageEvent(id: 1, message: message)) ) ..map.deepEquals(expected.map) - ..sorted.deepEquals(expected.sorted); + ..sorted.deepEquals(expected.sorted) + ..latestMessagesByRecipient.deepEquals(expected.latestMessagesByRecipient); + // (listeners are notified unnecessarily, but that's OK) + // check(listenersNotified).isFalse(); + }); + + test('new conversation with one existing and one new user, newest message', () { + bool listenersNotified = false; + final message = eg.dmMessage(id: 300, from: eg.selfUser, + to: [eg.user(userId: 1), eg.user(userId: 3)]); + check(setupView() + ..addListener(() { listenersNotified = true; }) + ..handleMessageEvent(MessageEvent(id: 1, message: message)) + ) ..map.deepEquals({ + key([1, 3]): 300, + key([1]): 200, + key([1, 2]): 100, + }) + ..sorted.deepEquals([key([1, 3]), key([1]), key([1, 2])]) + ..latestMessagesByRecipient.deepEquals({1: 300, 3: 300, 2: 100}); + check(listenersNotified).isTrue(); + }); + + test('new conversation with one existing and one new user, not newest message', () { + bool listenersNotified = false; + final message = eg.dmMessage(id: 150, from: eg.selfUser, + to: [eg.user(userId: 1), eg.user(userId: 3)]); + check(setupView() + ..addListener(() { listenersNotified = true; }) + ..handleMessageEvent(MessageEvent(id: 1, message: message)) + ) ..map.deepEquals({ + key([1]): 200, + key([1, 3]): 150, + key([1, 2]): 100, + }) + ..sorted.deepEquals([key([1]), key([1, 3]), key([1, 2])]) + ..latestMessagesByRecipient.deepEquals({1: 200, 3: 150, 2: 100}); + check(listenersNotified).isTrue(); }); }); });