Skip to content

autocomplete: Add "human vs. bot user" and "Alphabetical order" criteria #849

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Aug 9, 2024
47 changes: 43 additions & 4 deletions lib/model/autocomplete.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -439,13 +470,21 @@ class MentionAutocompleteQuery {
}

class AutocompleteDataCache {
final Map<int, String> _normalizedNamesByUser = {};

/// The lowercase `fullName` of [user].
String normalizedNameForUser(User user) {
return _normalizedNamesByUser[user.userId] ??= user.fullName.toLowerCase();
}

final Map<int, List<String>> _nameWordsByUser = {};

List<String> 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);
}
}
Expand Down
130 changes: 116 additions & 14 deletions test/model/autocomplete_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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<User> usersB) {
final view = MentionAutocompleteView.init(store: store, narrow: narrow);
Expand All @@ -509,48 +558,93 @@ 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: [
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[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),
Expand All @@ -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]]);
}
});

Expand Down Expand Up @@ -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: [
Expand All @@ -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')))
Expand Down