diff --git a/lib/model/autocomplete.dart b/lib/model/autocomplete.dart index 5250dba6d4..9a9cea0333 100644 --- a/lib/model/autocomplete.dart +++ b/lib/model/autocomplete.dart @@ -187,9 +187,95 @@ class MentionAutocompleteView extends ChangeNotifier { required PerAccountStore store, required Narrow narrow, }) { - assert(narrow is! CombinedFeedNarrow); return store.users.values.toList() - ..sort((userA, userB) => compareByDms(userA, userB, store: store)); + ..sort(_comparator(store: store, narrow: narrow)); + } + + /// Compare the users the same way they would be sorted as + /// autocomplete candidates. + /// + /// This behaves the same as the comparator used for sorting in + /// [_usersByRelevance], but calling this for each comparison would be a bit + /// less efficient because some of the logic is independent of the users and + /// can be precomputed. + /// + /// This is useful for tests in order to distinguish "A comes before B" + /// from "A ranks equal to B, and the sort happened to put A before B", + /// particularly because [List.sort] makes no guarantees about the order + /// of items that compare equal. + int debugCompareUsers(User userA, User userB) { + return _comparator(store: store, narrow: narrow)(userA, userB); + } + + static int Function(User, User) _comparator({ + required PerAccountStore store, + required Narrow narrow, + }) { + int? streamId; + String? topic; + switch (narrow) { + case StreamNarrow(): + streamId = narrow.streamId; + case TopicNarrow(): + streamId = narrow.streamId; + topic = narrow.topic; + case DmNarrow(): + break; + case CombinedFeedNarrow(): + assert(false, 'No compose box, thus no autocomplete is available in ${narrow.runtimeType}.'); + } + return (userA, userB) => _compareByRelevance(userA, userB, + streamId: streamId, topic: topic, + store: store); + } + + static int _compareByRelevance(User userA, User userB, { + required int? streamId, + required String? topic, + required PerAccountStore store, + }) { + if (streamId != null) { + final result = compareByRecency(userA, userB, + streamId: streamId, + topic: topic, + store: store); + if (result != 0) return result; + } + return compareByDms(userA, userB, store: store); + } + + /// Determines which of the two users has more recent activity (messages sent + /// recently) in the topic/stream. + /// + /// First checks for the activity in [topic] if provided. + /// + /// If no [topic] is provided, or there is no activity in the topic at all, + /// then checks for the activity in the stream with [streamId]. + /// + /// Returns a negative number if [userA] has more recent activity than [userB], + /// returns a positive number if [userB] has more recent activity than [userA], + /// and returns `0` if both [userA] and [userB] have no activity at all. + @visibleForTesting + static int compareByRecency(User userA, User userB, { + required int streamId, + required String? topic, + required PerAccountStore store, + }) { + final recentSenders = store.recentSenders; + if (topic != null) { + final result = -compareRecentMessageIds( + recentSenders.latestMessageIdOfSenderInTopic( + streamId: streamId, topic: topic, senderId: userA.userId), + recentSenders.latestMessageIdOfSenderInTopic( + streamId: streamId, topic: topic, senderId: userB.userId)); + if (result != 0) return result; + } + + return -compareRecentMessageIds( + recentSenders.latestMessageIdOfSenderInStream( + streamId: streamId, senderId: userA.userId), + recentSenders.latestMessageIdOfSenderInStream( + streamId: streamId, senderId: userB.userId)); } /// Determines which of the two users is more recent in DM conversations. @@ -204,10 +290,21 @@ class MentionAutocompleteView extends ChangeNotifier { 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, + return -compareRecentMessageIds(aLatestMessageId, bLatestMessageId); + } + + /// Compares [a] to [b], with null less than all integers. + /// + /// The values should represent the most recent message ID in each of two + /// sets of messages, with null meaning the set is empty. + /// + /// Return values are as with [Comparable.compareTo]. + @visibleForTesting + static int compareRecentMessageIds(int? a, int? b) { + return switch ((a, b)) { + (int a, int b) => a.compareTo(b), + (int(), _) => 1, + (_, int()) => -1, _ => 0, }; } diff --git a/test/model/autocomplete_test.dart b/test/model/autocomplete_test.dart index 5493598200..391d939ba7 100644 --- a/test/model/autocomplete_test.dart +++ b/test/model/autocomplete_test.dart @@ -359,13 +359,83 @@ void main() { Future prepare({ List users = const [], List dmConversations = const [], + List messages = const [], }) async { store = eg.store(initialSnapshot: eg.initialSnapshot( recentPrivateConversations: dmConversations)); await store.addUsers(users); + await store.addMessages(messages); } - group('MentionAutocompleteView.compareByDms', () { + group('compareRecentMessageIds', () { + test('both a and b are non-null', () async { + check(MentionAutocompleteView.compareRecentMessageIds(2, 5)).isLessThan(0); + check(MentionAutocompleteView.compareRecentMessageIds(5, 2)).isGreaterThan(0); + check(MentionAutocompleteView.compareRecentMessageIds(5, 5)).equals(0); + }); + + test('one of a and b is null', () async { + check(MentionAutocompleteView.compareRecentMessageIds(null, 5)).isLessThan(0); + check(MentionAutocompleteView.compareRecentMessageIds(5, null)).isGreaterThan(0); + }); + + test('both of a and b are null', () async { + check(MentionAutocompleteView.compareRecentMessageIds(null, null)).equals(0); + }); + }); + + group('compareByRecency', () { + final userA = eg.otherUser; + final userB = eg.thirdUser; + final stream = eg.stream(); + const topic1 = 'topic1'; + const topic2 = 'topic2'; + + Message message(User sender, String topic) { + return eg.streamMessage(sender: sender, stream: stream, topic: topic); + } + + int compareAB({required String? topic}) { + final resultAB = MentionAutocompleteView.compareByRecency(userA, userB, + streamId: stream.streamId, topic: topic, store: store); + final resultBA = MentionAutocompleteView.compareByRecency(userB, userA, + streamId: stream.streamId, topic: topic, store: store); + switch (resultAB) { + case <0: check(resultBA).isGreaterThan(0); + case >0: check(resultBA).isLessThan(0); + default: check(resultBA).equals(0); + } + return resultAB; + } + + test('favor user most recent in topic', () async { + await prepare(messages: [message(userA, topic1), message(userB, topic1)]); + check(compareAB(topic: topic1)).isGreaterThan(0); + }); + + test('favor most recent in topic ahead of most recent in stream', () async { + await prepare(messages: [ + message(userA, topic1), message(userB, topic1), message(userA, topic2)]); + check(compareAB(topic: topic1)).isGreaterThan(0); + }); + + test('no activity in topic -> favor user most recent in stream', () async { + await prepare(messages: [message(userA, topic1), message(userB, topic1)]); + check(compareAB(topic: topic2)).isGreaterThan(0); + }); + + test('no topic provided -> favor user most recent in stream', () async { + await prepare(messages: [message(userA, topic1), message(userB, topic2)]); + check(compareAB(topic: null)).isGreaterThan(0); + }); + + test('no activity in topic/stream -> favor none', () async { + await prepare(messages: []); + check(compareAB(topic: null)).equals(0); + }); + }); + + group('compareByDms', () { const idA = 1; const idB = 2; @@ -418,61 +488,137 @@ void main() { }); }); - 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); + group('ranking across signals', () { + void checkPrecedes(Narrow narrow, User userA, Iterable usersB) { final view = MentionAutocompleteView.init(store: store, narrow: narrow); + for (final userB in usersB) { + check(view.debugCompareUsers(userA, userB)).isLessThan(0); + check(view.debugCompareUsers(userB, userA)).isGreaterThan(0); + } + } - 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); + void checkRankEqual(Narrow narrow, List users) { + final view = MentionAutocompleteView.init(store: store, narrow: narrow); + for (int i = 0; i < users.length; i++) { + for (int j = i + 1; j < users.length; j++) { + check(view.debugCompareUsers(users[i], users[j])).equals(0); + check(view.debugCompareUsers(users[j], users[i])).equals(0); + } + } } - test('StreamNarrow', () async { - await checkResultsIn(const StreamNarrow(1), expected: [3, 0, 1, 2, 4]); + test('TopicNarrow: topic recency > stream recency > DM recency', () async { + final users = List.generate(5, (i) => eg.user()); + final stream = eg.stream(); + final narrow = TopicNarrow(stream.streamId, 'this'); + await prepare(users: users, messages: [ + eg.streamMessage(sender: users[1], stream: stream, topic: 'this'), + eg.streamMessage(sender: users[0], stream: stream, topic: 'this'), + eg.streamMessage(sender: users[2], stream: stream, topic: 'other'), + eg.streamMessage(sender: users[1], stream: stream, topic: 'other'), + eg.dmMessage(from: users[3], to: [users[4], eg.selfUser]), + eg.dmMessage(from: users[2], to: [eg.selfUser]), + ]); + checkPrecedes(narrow, users[0], users.skip(1)); + checkPrecedes(narrow, users[1], users.skip(2)); + checkPrecedes(narrow, users[2], users.skip(3)); + checkRankEqual(narrow, [users[3], users[4]]); }); - test('TopicNarrow', () async { - await checkResultsIn(const TopicNarrow(1, 'topic'), expected: [3, 0, 1, 2, 4]); + test('StreamNarrow: stream recency > DM recency', () async { + final users = List.generate(4, (i) => eg.user()); + final stream = eg.stream(); + final narrow = StreamNarrow(stream.streamId); + await prepare(users: users, messages: [ + eg.streamMessage(sender: users[1], stream: stream), + eg.streamMessage(sender: users[0], stream: stream), + eg.dmMessage(from: users[2], to: [users[3], eg.selfUser]), + eg.dmMessage(from: users[1], to: [eg.selfUser]), + ]); + checkPrecedes(narrow, users[0], users.skip(1)); + checkPrecedes(narrow, users[1], users.skip(2)); + checkRankEqual(narrow, [users[2], users[3]]); }); - test('DmNarrow', () async { - await checkResultsIn( - DmNarrow.withUser(eg.selfUser.userId, selfUserId: eg.selfUser.userId), - expected: [3, 0, 1, 2, 4], - ); + test('DmNarrow: DM recency > this-conversation recency or stream recency', () async { + final users = List.generate(4, (i) => eg.user()); + await prepare(users: users, messages: [ + eg.dmMessage(from: users[3], to: [eg.selfUser]), + eg.dmMessage(from: users[1], to: [users[2], eg.selfUser]), + eg.dmMessage(from: users[0], to: [eg.selfUser]), + eg.streamMessage(sender: users[1]), + eg.streamMessage(sender: users[2]), + eg.streamMessage(sender: users[3]), + ]); + for (final narrow in [ + DmNarrow.withUser(users[3].userId, selfUserId: eg.selfUser.userId), + DmNarrow.withOtherUsers([users[1].userId, users[2].userId], + selfUserId: eg.selfUser.userId), + DmNarrow.withUser(users[1].userId, selfUserId: eg.selfUser.userId), + ]) { + checkPrecedes(narrow, users[0], users.skip(1)); + checkRankEqual(narrow, [users[1], users[2]]); + checkPrecedes(narrow, users[1], users.skip(3)); + checkPrecedes(narrow, users[2], users.skip(3)); + } }); - 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(); + test('CombinedFeedNarrow gives error', () async { + await prepare(users: [eg.user(), eg.user()], messages: []); + const narrow = CombinedFeedNarrow(); + check(() => MentionAutocompleteView.init(store: store, narrow: narrow)) + .throws(); }); }); + + test('final results end-to-end', () async { + Future> getResults( + Narrow narrow, MentionAutocompleteQuery query) async { + bool done = false; + final view = MentionAutocompleteView.init(store: store, narrow: narrow); + view.addListener(() { done = true; }); + view.query = query; + await Future(() {}); + check(done).isTrue(); + final results = view.results + .map((e) => (e as UserMentionAutocompleteResult).userId); + view.dispose(); + return results; + } + + final stream = eg.stream(); + const topic = 'topic'; + final topicNarrow = TopicNarrow(stream.streamId, topic); + + final users = [ + eg.user(userId: 1, fullName: 'User One'), + eg.user(userId: 2, fullName: 'User Two'), + eg.user(userId: 3, fullName: 'User Three'), + eg.user(userId: 4, fullName: 'User Four'), + eg.user(userId: 5, fullName: 'User Five'), + ]; + + await prepare(users: users, messages: [ + eg.streamMessage(id: 50, sender: users[1-1], stream: stream, topic: topic), + eg.streamMessage(id: 60, sender: users[5-1], stream: stream, topic: 'other $topic'), + ], dmConversations: [ + RecentDmConversation(userIds: [4], maxMessageId: 300), + RecentDmConversation(userIds: [1], maxMessageId: 200), + RecentDmConversation(userIds: [1, 2], maxMessageId: 100), + ]); + + // Check the ranking of the full list of users. + // The order should be: + // 1. Users most recent in the current topic/stream. + // 2. Users most recent in the DM conversations. + check(await getResults(topicNarrow, MentionAutocompleteQuery(''))) + .deepEquals([1, 5, 4, 2, 3]); + + // Check the ranking applies also to results filtered by a query. + check(await getResults(topicNarrow, MentionAutocompleteQuery('t'))) + .deepEquals([2, 3]); + check(await getResults(topicNarrow, MentionAutocompleteQuery('f'))) + .deepEquals([5, 4]); + }); }); }