diff --git a/lib/model/autocomplete.dart b/lib/model/autocomplete.dart index 3054fd5348..f87e7650b4 100644 --- a/lib/model/autocomplete.dart +++ b/lib/model/autocomplete.dart @@ -235,14 +235,24 @@ class MentionAutocompleteView extends ChangeNotifier { required String? topic, required PerAccountStore store, }) { + // TODO(#234): give preference to "all", "everyone" or "stream" + + // TODO(#618): give preference to subscribed users first + if (streamId != null) { - final result = compareByRecency(userA, userB, + final recencyResult = compareByRecency(userA, userB, streamId: streamId, topic: topic, store: store); - if (result != 0) return result; + if (recencyResult != 0) return recencyResult; } - return compareByDms(userA, userB, store: store); + final dmsResult = compareByDms(userA, userB, store: store); + if (dmsResult != 0) return dmsResult; + + final botStatusResult = compareByBotStatus(userA, userB); + if (botStatusResult != 0) return botStatusResult; + + return compareByAlphabeticalOrder(userA, userB, store: store); } /// Determines which of the two users has more recent activity (messages sent @@ -310,6 +320,27 @@ class MentionAutocompleteView extends ChangeNotifier { }; } + /// Comparator that puts non-bots before bots. + @visibleForTesting + static int compareByBotStatus(User userA, User userB) { + return switch ((userA.isBot, userB.isBot)) { + (false, true) => -1, + (true, false) => 1, + _ => 0, + }; + } + + /// Comparator that orders alphabetically by [User.fullName]. + @visibleForTesting + static int compareByAlphabeticalOrder(User userA, User userB, + {required PerAccountStore store}) { + final userAName = store.autocompleteViewManager.autocompleteDataCache + .normalizedNameForUser(userA); + final userBName = store.autocompleteViewManager.autocompleteDataCache + .normalizedNameForUser(userB); + return userAName.compareTo(userBName); // TODO(i18n): add locale-aware sorting + } + @override void dispose() { store.autocompleteViewManager.unregisterMentionAutocomplete(this); @@ -439,13 +470,21 @@ class MentionAutocompleteQuery { } class AutocompleteDataCache { + final Map _normalizedNamesByUser = {}; + + /// The lowercase `fullName` of [user]. + String normalizedNameForUser(User user) { + return _normalizedNamesByUser[user.userId] ??= user.fullName.toLowerCase(); + } + final Map> _nameWordsByUser = {}; List nameWordsForUser(User user) { - return _nameWordsByUser[user.userId] ??= user.fullName.toLowerCase().split(' '); + return _nameWordsByUser[user.userId] ??= normalizedNameForUser(user).split(' '); } void invalidateUser(int userId) { + _normalizedNamesByUser.remove(userId); _nameWordsByUser.remove(userId); } } diff --git a/test/model/autocomplete_test.dart b/test/model/autocomplete_test.dart index 54ed729a64..f3c8a30c13 100644 --- a/test/model/autocomplete_test.dart +++ b/test/model/autocomplete_test.dart @@ -490,6 +490,55 @@ void main() { }); }); + group('compareByBotStatus', () { + final humanUser = eg.user(isBot: false); + final botUser = eg.user(isBot: true); + + int compareAB(User a, User b) => MentionAutocompleteView.compareByBotStatus(a, b); + + test('userA is human, userB is bot -> favor userA', () { + check(compareAB(humanUser, botUser)).isLessThan(0); + }); + + test('userA is bot, userB is human -> favor userB', () { + check(compareAB(botUser, humanUser)).isGreaterThan(0); + }); + + test('both users have the same bot status -> favor none', () { + check(compareAB(humanUser, humanUser)).equals(0); + check(compareAB(botUser, botUser)).equals(0); + }); + }); + + group('compareByAlphabeticalOrder', () { + int compareAB(String aName, String bName) => MentionAutocompleteView.compareByAlphabeticalOrder( + eg.user(fullName: aName), eg.user(fullName: bName), store: store); + + test("userA's fullName comes first than userB's fullName -> favor userA", () async { + await prepare(); + check(compareAB('alice', 'brian')).isLessThan(0); + check(compareAB('alice', 'BRIAN')).isLessThan(0); + // TODO(i18n): add locale-aware sorting + // check(compareAB('čarolína', 'david')).isLessThan(0); + }); + + test("userB's fullName comes first than userA's fullName -> favor userB", () async { + await prepare(); + check(compareAB('brian', 'alice')).isGreaterThan(0); + check(compareAB('BRIAN', 'alice')).isGreaterThan(0); + // TODO(i18n): add locale-aware sorting + // check(compareAB('david', 'čarolína')).isGreaterThan(0); + }); + + test('both users have identical fullName -> favor none', () async { + await prepare(); + check(compareAB('alice', 'alice')).equals(0); + check(compareAB('BRIAN', 'brian')).equals(0); + // TODO(i18n): add locale-aware sorting + // check(compareAB('čarolína', 'carolina')).equals(0); + }); + }); + group('ranking across signals', () { void checkPrecedes(Narrow narrow, User userA, Iterable usersB) { final view = MentionAutocompleteView.init(store: store, narrow: narrow); @@ -509,8 +558,21 @@ void main() { } } - test('TopicNarrow: topic recency > stream recency > DM recency', () async { - final users = List.generate(5, (i) => eg.user()); + test('TopicNarrow: topic recency > stream recency > DM recency > human/bot > name', () async { + // The user with the greatest topic recency ranks last on each of the + // other criteria, but comes out first in the end, showing that + // topic recency comes first. Then among the remaining users, the one + // with the greatest stream recency ranks last on each of the remaining + // criteria, but comes out second in the end; and so on. + final users = [ + eg.user(fullName: 'Z', isBot: true), // wins by topic recency + eg.user(fullName: 'Y', isBot: true), // runner-up, by stream recency + eg.user(fullName: 'X', isBot: true), // runner-up, by DM recency + eg.user(fullName: 'W', isBot: false), // runner-up, by human-vs-bot + eg.user(fullName: 'A', isBot: true), // runner-up, by name + eg.user(fullName: 'B', isBot: true), // tied because no remaining criteria + eg.user(fullName: 'b', isBot: true), + ]; final stream = eg.stream(); final narrow = TopicNarrow(stream.streamId, 'this'); await prepare(users: users, messages: [ @@ -518,39 +580,71 @@ void main() { 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[3], to: [...users.skip(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]]); + checkPrecedes(narrow, users[3], users.skip(4)); + checkPrecedes(narrow, users[4], users.skip(5)); + checkRankEqual(narrow, [users[5], users[6]]); }); - test('ChannelNarrow: stream recency > DM recency', () async { - final users = List.generate(4, (i) => eg.user()); + test('ChannelNarrow: stream recency > DM recency > human/bot > name', () async { + // Same principle as for TopicNarrow; see that test case above. + final users = [ + eg.user(fullName: 'Z', isBot: true), // wins by stream recency + eg.user(fullName: 'Y', isBot: true), // runner-up, by DM recency + eg.user(fullName: 'X', isBot: false), // runner-up, by human-vs-bot + eg.user(fullName: 'A', isBot: true), // runner-up, by name + eg.user(fullName: 'B', isBot: true), // tied because no remaining criteria + eg.user(fullName: 'b', isBot: true), + ]; final stream = eg.stream(); final narrow = ChannelNarrow(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[2], to: [...users.skip(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]]); + checkPrecedes(narrow, users[2], users.skip(3)); + checkPrecedes(narrow, users[3], users.skip(4)); + checkRankEqual(narrow, [users[4], users[5]]); }); - test('DmNarrow: DM recency > this-conversation recency or stream recency', () async { - final users = List.generate(4, (i) => eg.user()); + test('DmNarrow: DM recency > human/bot > name, ignore this-conversation recency and stream recency', () async { + // Same principle as for TopicNarrow; see that test case above. + final users = [ + // First user wins by DM recency. + eg.user(fullName: 'Z', isBot: true), + // Next two are runners-up by DM recency, and have a two-way tie + // despite different this-conversation recency (because that doesn't count). + eg.user(fullName: 'Y', isBot: true), + eg.user(fullName: 'y', isBot: true), + // Next user is the runner-up due to DM recency, and comes after the + // above users even when it has greater this-conversation recency + // (because that doesn't count). + eg.user(fullName: 'X', isBot: true), + // Remainder have no DM recency and so come later. + // Next user is the runner-up due to human-vs-bot. + eg.user(fullName: 'W', isBot: false), + // Next user is the runner-up due to name. + eg.user(fullName: 'A', isBot: true), + // Remaining users are tied, even though they differ in stream recency + // (because that doesn't count). + eg.user(fullName: 'B', isBot: true), + eg.user(fullName: 'b', isBot: true), + ]; 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 user in users.skip(1)) + eg.streamMessage(sender: user), ]); for (final narrow in [ DmNarrow.withUser(users[3].userId, selfUserId: eg.selfUser.userId), @@ -562,6 +656,10 @@ void main() { checkRankEqual(narrow, [users[1], users[2]]); checkPrecedes(narrow, users[1], users.skip(3)); checkPrecedes(narrow, users[2], users.skip(3)); + checkPrecedes(narrow, users[3], users.skip(4)); + checkPrecedes(narrow, users[4], users.skip(5)); + checkPrecedes(narrow, users[5], users.skip(6)); + checkRankEqual(narrow, [users[6], users[7]]); } }); @@ -605,6 +703,8 @@ void main() { eg.user(userId: 3, fullName: 'User Three'), eg.user(userId: 4, fullName: 'User Four'), eg.user(userId: 5, fullName: 'User Five'), + eg.user(userId: 6, fullName: 'User Six', isBot: true), + eg.user(userId: 7, fullName: 'User Seven'), ]; await prepare(users: users, messages: [ @@ -620,8 +720,10 @@ void main() { // The order should be: // 1. Users most recent in the current topic/stream. // 2. Users most recent in the DM conversations. + // 3. Human vs. Bot users (human users come first). + // 4. Alphabetical order by name. check(await getResults(topicNarrow, MentionAutocompleteQuery(''))) - .deepEquals([1, 5, 4, 2, 3]); + .deepEquals([1, 5, 4, 2, 7, 3, 6]); // Check the ranking applies also to results filtered by a query. check(await getResults(topicNarrow, MentionAutocompleteQuery('t')))