Start using rank field on mention autocomplete
#1748
Conversation
Like we do in emoji autocomplete. This commit doesn't make any changes to the results ordering; bucketSort sorts stably, and the input is still just the wildcard results followed by the user results. Soon, though, we'd like to rank by match quality and add user-group results (for zulip#233) interleaved with user results. Bucket sorting will help us do this without making many intermediate copies of lists of results; see discussion: https://chat.zulip.org/#narrow/channel/48-mobile/topic/user-group.20mentions.20.23F233/near/2216353
Since 9815022, this method already takes the whole store, which it uses in order to identify muted users. So it can get the autocomplete-data cache from the store just as well as its caller can.
… test This will make it more smoothly generalize to situations where different types of results are more intermingled.
This makes this function better match the end-to-end behavior of our autocomplete ordering, now that that involves this ranking notion.
Now that I've spelled this out, complete with its consequences as seen in the tests, it doesn't actually sound desirable: if there's a bot participating in the current thread and I start typing its name, why wouldn't we want that as a top result? Anyway, this demonstrates how such a change would work out in the tests, though.
| test('ChannelNarrow: stream recency > DM recency > human/bot > name', () async { | ||
| test('ChannelNarrow: human/bot > stream recency > DM recency > 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: 'Z', isBot: false), // wins by human-vs-bot | ||
| 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: 'A', isBot: true), // runner-up, by name |
There was a problem hiding this comment.
These test changes happen when putting human-vs-bot in the match ranking, because there are these existing tests that check how that distinction compares in ranking priority with other criteria. The outcome there changes, so it seems good to reflect that in the tests.
But for introducing user groups to these results (#233), as from #1745, I wouldn't recommend adding more tests of this kind. Instead…
test/model/autocomplete_test.dart
Outdated
| test('wildcards, then users', () { | ||
| test('wildcards, non-bot users, then bots', () { | ||
| checkSameRank('', WildcardMentionOption.all, WildcardMentionOption.topic); | ||
| checkPrecedes('', WildcardMentionOption.topic, eg.user()); | ||
| checkSameRank('', eg.user(), eg.user()); | ||
| checkPrecedes('', WildcardMentionOption.topic, eg.user(isBot: false)); | ||
| checkPrecedes('', eg.user(isBot: false), eg.user(isBot: true)); | ||
| }); |
There was a problem hiding this comment.
… I think the ranking of user groups vs. users can be tested well with just this class of test. The tests in test/model/emoji_test.dart like "ranks match quality exact/prefix/word-aligned/other" and its neighbors provide a useful set of examples.
The structure of the result ordering (IIUC) is that the rank prevails, and when the ranks are equal the other ordering is effectively used as a tie-breaker. So the tests can rely on that structure: tests of this kind focus purely on the ranks; and then for all the criteria that sort users of equal match rank, the tests above (the ones that call the various compareByFoo methods and debugCompareUsers) can remain in place covering those.
| checkPrecedes('', WildcardMentionOption.topic, eg.user()); | ||
| checkSameRank('', eg.user(), eg.user()); | ||
| checkPrecedes('', WildcardMentionOption.topic, eg.user(isBot: false)); | ||
| checkPrecedes('', eg.user(isBot: false), eg.user(isBot: true)); | ||
| }); | ||
|
|
||
| test('wildcard-vs-user more significant than match quality', () { |
There was a problem hiding this comment.
Oh, and rereading those rank tests in test/model/emoji_test.dart reminds me of one other type of test that would be good to include:
test('full list of ranks', () {
check([
rankOf('o', unicode(['o'])), // exact (generic)
rankOf('o', octopus), // prefix popular
rankOf('o', workingOnIt), // word-aligned popular
rankOf('o', realmCandidate('open_book')), // prefix realm
rankOf('z', zulipCandidate()), // == prefix :zulip:
rankOf('y', realmCandidate('thank_you')), // word-aligned realm
// (word-aligned :zulip: is impossible because the name is one word)
rankOf('o', unicode(['ok'])), // prefix generic
rankOf('o', unicode(['squared_ok'])), // word-aligned generic
rankOf('o', realmCandidate('yo')), // other realm
rankOf('p', zulipCandidate()), // == other :zulip:
rankOf('o', unicode(['book'])), // other generic
rankOf('o', love), // == other popular
]).deepEquals([0, 1, 2, 3, 3, 4, 5, 6, 7, 7, 8, 8]);
});That can be introduced at the outset even though it'll be trivial, with just two items; then it can grow as more cases and criteria are added.
Stacked atop #1745. This makes some refactors to the tests for the ordering of autocomplete results, and then demonstrates changing that ordering by moving the human-vs-bot distinction into the "ranking" layer, as suggested in #1745.
Now that I've spelled that out, complete with its consequences as seen in the tests, it doesn't actually sound desirable as a product change: if there's a bot participating in the current thread and I start typing its name, wouldn't we want that as a top result? The distinctions in the "ranking" layer take priority over those in the initial sorting of candidates, so this change causes bots to always come after all other matches.
Anyway, this demonstrates how such a change would work out in the tests, though. And if that really is the behavior that web has, then we should probably just do it — in order to minimize avoidable differences from web's logic, to better clear the way to proceeding with user groups as autocomplete results, #233 — and consider coming back and changing it on both platforms later.