From d9a49d2e47c7dcc381c49c49d2b9bc3a4fc1cd00 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Sat, 7 Dec 2024 23:11:04 -0800 Subject: [PATCH 01/18] emoji test: Avoid coincidentally using "popular" emoji We'll soon (for #1068) be adding logic that distinguishes these emoji from other Unicode emoji. That would break some test cases which refer to an emoji that happens to be "popular", like :smile:, when they really just intend the generic behavior that happens to any Unicode emoji. --- test/model/emoji_test.dart | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/test/model/emoji_test.dart b/test/model/emoji_test.dart index 953d986774..6f3c2e2f1b 100644 --- a/test/model/emoji_test.dart +++ b/test/model/emoji_test.dart @@ -152,11 +152,11 @@ void main() { final store = prepare(realmEmoji: { '1': eg.realmEmojiItem(emojiCode: '1', emojiName: 'smiley'), }, unicodeEmoji: { - '1f642': ['smile'], + '1f516': ['bookmark'], '1f603': ['smiley'], }); check(store.allEmojiCandidates()).deepEquals([ - isUnicodeCandidate('1f642', ['smile']), + isUnicodeCandidate('1f516', ['bookmark']), isRealmCandidate(emojiCode: '1', emojiName: 'smiley'), isZulipCandidate(), ]); @@ -207,10 +207,10 @@ void main() { ]); store.setServerEmojiData(ServerEmojiData(codeToNames: { - '1f642': ['smile'], + '1f516': ['bookmark'], })); check(store.allEmojiCandidates()).deepEquals([ - isUnicodeCandidate('1f642', ['smile']), + isUnicodeCandidate('1f516', ['bookmark']), isZulipCandidate(), ]); }); @@ -234,7 +234,7 @@ void main() { final store = prepare(realmEmoji: { '1': eg.realmEmojiItem(emojiCode: '1', emojiName: 'happy'), }, unicodeEmoji: { - '1f642': ['smile'], + '1f516': ['bookmark'], }); final candidates = store.allEmojiCandidates(); check(store.allEmojiCandidates()).identicalTo(candidates); @@ -274,7 +274,7 @@ void main() { test('results can include all three emoji types', () async { final store = prepare( - realmEmoji: {'1': 'happy'}, unicodeEmoji: {'1f642': ['smile']}); + realmEmoji: {'1': 'happy'}, unicodeEmoji: {'1f516': ['bookmark']}); final view = EmojiAutocompleteView.init(store: store, query: EmojiAutocompleteQuery('')); bool done = false; @@ -282,7 +282,7 @@ void main() { await Future(() {}); check(done).isTrue(); check(view.results).deepEquals([ - isUnicodeResult(names: ['smile']), + isUnicodeResult(names: ['bookmark']), isRealmResult(emojiName: 'happy'), isZulipResult(), ]); From 906545f583c25077ddab8d509612770cc620f89c Mon Sep 17 00:00:00 2001 From: Greg Price Date: Sat, 7 Dec 2024 23:26:55 -0800 Subject: [PATCH 02/18] emoji test: Avoid depending on any popular emoji being absent These searches for 'h' and 's' would have many results in a real emoji database; in particular they'd have multiple results even in an emoji database that has just the six "popular" emoji. We'll soon make a change that causes those "popular" emoji to always be present. To prepare these tests, make the queries more specific. --- test/model/emoji_test.dart | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/model/emoji_test.dart b/test/model/emoji_test.dart index 6f3c2e2f1b..312a0f916c 100644 --- a/test/model/emoji_test.dart +++ b/test/model/emoji_test.dart @@ -292,7 +292,7 @@ void main() { final store = prepare( realmEmoji: {'1': 'happy'}, unicodeEmoji: {'1f642': ['smile']}); final view = EmojiAutocompleteView.init(store: store, - query: EmojiAutocompleteQuery('h')); + query: EmojiAutocompleteQuery('hap')); bool done = false; view.addListener(() { done = true; }); await Future(() {}); @@ -301,7 +301,7 @@ void main() { isRealmResult(emojiName: 'happy')); done = false; - view.query = EmojiAutocompleteQuery('s'); + view.query = EmojiAutocompleteQuery('sm'); await Future(() {}); check(done).isTrue(); check(view.results).single.which( From ab50a5d05793f33b77b64bae7ead82afde239776 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Sat, 7 Dec 2024 14:01:20 -0800 Subject: [PATCH 03/18] emoji: Short-circuit matching an empty query This is NFC except for performance. This is an extremely common case -- it happens every time the user opens the emoji picker, and applies to every emoji -- so worth optimizing. --- lib/model/emoji.dart | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/model/emoji.dart b/lib/model/emoji.dart index 07591abd34..15f8d47568 100644 --- a/lib/model/emoji.dart +++ b/lib/model/emoji.dart @@ -364,6 +364,7 @@ class EmojiAutocompleteQuery extends ComposeAutocompleteQuery { // Compare get_emoji_matcher in Zulip web:shared/src/typeahead.ts . bool matches(EmojiCandidate candidate) { + if (_adjusted == '') return true; if (candidate.emojiDisplay case UnicodeEmojiDisplay(:var emojiUnicode)) { if (_adjusted == emojiUnicode) return true; } From e4b5604a25cbaf556b7b626c639f63e4da31b400 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Sat, 7 Dec 2024 15:18:58 -0800 Subject: [PATCH 04/18] emoji: Compute separator-plus-query just once per query This is NFC except for performance: it saves us from having to construct this string again for each emoji name. At this stage there's a (much smaller) performance loss, too: we now compute this string even when we'll never actually use it because the query doesn't contain the separator. But we'll soon start checking for word-aligned matches even for those queries where we'd accept a non-word-aligned match; once we do, we'll need this string for all nonempty queries, for almost all names. --- lib/model/emoji.dart | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/lib/model/emoji.dart b/lib/model/emoji.dart index 15f8d47568..a13d67dace 100644 --- a/lib/model/emoji.dart +++ b/lib/model/emoji.dart @@ -348,11 +348,24 @@ class EmojiAutocompleteView extends AutocompleteView EmojiAutocompleteQuery._(raw, _adjustQuery(raw)); + EmojiAutocompleteQuery._(super.raw, String adjusted) + : _adjusted = adjusted, + _sepAdjusted = _separator + adjusted; + + /// The query string as adjusted for comparing to emoji names, + /// via [_adjustQuery]. final String _adjusted; + /// The concatenation of [_separator] with [_adjusted]. + /// + /// Useful for finding word-aligned matches in an emoji name. + final String _sepAdjusted; + + static const _separator = '_'; + static String _adjustQuery(String raw) { return raw.toLowerCase().replaceAll(' ', '_'); // TODO(#1067) remove diacritics too } @@ -375,9 +388,8 @@ class EmojiAutocompleteQuery extends ComposeAutocompleteQuery { // Compare query_matches_string_in_order in Zulip web:shared/src/typeahead.ts . bool _nameMatches(String emojiName) { // TODO(#1067) this assumes emojiName is already lower-case (and no diacritics) - const String separator = '_'; - if (!_adjusted.contains(separator)) { + if (!_adjusted.contains(_separator)) { // If the query is a single token (doesn't contain a separator), // the match can be anywhere in the string. return emojiName.contains(_adjusted); @@ -388,7 +400,7 @@ class EmojiAutocompleteQuery extends ComposeAutocompleteQuery { // (E.g. for 'ab_cd_ef', query could be 'ab_c' or 'cd_ef', // but not 'b_cd_ef'.) return emojiName.startsWith(_adjusted) - || emojiName.contains(separator + _adjusted); + || emojiName.contains(_sepAdjusted); } @override From b67e172737896f50923612d6a109861671027138 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 6 Dec 2024 22:44:06 -0800 Subject: [PATCH 05/18] algorithms: Add bucketSort, for stable, linear-time sorting Specifically the sort takes linear time so long as there aren't more than linearly-many buckets. For our immediate use case of ranking emoji-autocomplete results, we'll in fact stick to a constant number of buckets. --- lib/model/algorithms.dart | 64 +++++++++++++++++++++++ test/model/algorithms_test.dart | 93 +++++++++++++++++++++++++++++++++ 2 files changed, 157 insertions(+) diff --git a/lib/model/algorithms.dart b/lib/model/algorithms.dart index 7c03a88d80..93bcb188ee 100644 --- a/lib/model/algorithms.dart +++ b/lib/model/algorithms.dart @@ -116,3 +116,67 @@ QueueList setUnion(Iterable xs, Iterable ys) { } return result; } + +/// Sort the items by bucket, stably, +/// and if the buckets are few then in linear time. +/// +/// The returned list will have the same elements as [xs], ordered by bucket, +/// and elements in each bucket will appear in the same order as in [xs]. +/// In other words, the list is the result of a stable sort of [xs] by bucket. +/// (By contrast, Dart's [List.sort] is not guaranteed to be stable.) +/// +/// For each element of [xs], the bucket identified by [bucketOf] +/// must be in the range `0 <= bucket < numBuckets`. +/// Repeated calls to [bucketOf] on the same element must return the same value. +/// +/// If [bucketOf] returns different answers when called twice for some element, +/// this function's behavior is undefined: +/// it may throw, or may return an arbitrary list. +/// +/// The cost of this function is linear in `xs.length` plus [numBuckets]. +/// In particular if [numBuckets] is a constant +/// (or more generally is at most a constant multiple of `xs.length`), +/// then this function sorts the items in linear time, O(n). +/// On the other hand if there are many more buckets than elements, +/// consider using a different sorting algorithm. +List bucketSort(Iterable xs, int Function(T) bucketOf, { + required int numBuckets, +}) { + if (xs.isEmpty) return []; + if (numBuckets <= 0) throw StateError("bucketSort: non-positive numBuckets"); + + final counts = List.generate(numBuckets, (_) => 0); + for (final x in xs) { + final key = bucketOf(x); + _checkBucket(key, numBuckets); + counts[key]++; + } + // Now counts[k] is the number of values with key k. + + var partialSum = 0; + for (var k = 0; k < numBuckets; k++) { + final count = counts[k]; + counts[k] = partialSum; + partialSum += count; + } + assert(partialSum == xs.length); + // Now counts[k] is the index where the first value with key k should go. + + final result = List.generate(xs.length, (_) => xs.first); + for (final x in xs) { + // Each counts[k] is the index where the next value with key k should go. + final key = bucketOf(x); + _checkBucket(key, numBuckets); + final index = counts[key]++; + if (index >= result.length) { + throw StateError("bucketSort: bucketOf gave varying answers on same value"); + } + result[index] = x; + } + return result; +} + +void _checkBucket(int key, int numBuckets) { + if (key < 0) throw StateError("bucketSort: negative bucket"); + if (key >= numBuckets) throw StateError("bucketSort: bucket out of range"); +} diff --git a/test/model/algorithms_test.dart b/test/model/algorithms_test.dart index b6def6393e..4dc3772105 100644 --- a/test/model/algorithms_test.dart +++ b/test/model/algorithms_test.dart @@ -1,5 +1,7 @@ +import 'dart:math'; import 'package:checks/checks.dart'; +import 'package:collection/collection.dart'; import 'package:test/scaffolding.dart'; import 'package:zulip/model/algorithms.dart'; @@ -55,4 +57,95 @@ void main() { }); } }); + + group('bucketSort', () { + /// Same spec as [bucketSort], except slow: N * B time instead of N + B. + List simpleBucketSort(Iterable xs, int Function(T) bucketOf, { + required int numBuckets, + }) { + return Iterable.generate(numBuckets, + (k) => xs.where((s) => bucketOf(s) == k)).flattenedToList; + } + + void checkBucketSort(Iterable xs, { + required int Function(T) bucketOf, required int numBuckets, + }) { + check(bucketSort(xs, bucketOf, numBuckets: numBuckets)).deepEquals( + simpleBucketSort(xs, bucketOf, numBuckets: numBuckets)); + } + + int stringBucket(String s) => s.codeUnits.last - '0'.codeUnits.single; + + test('explicit result, interleaved: 4 elements, 2 buckets', () { + check(bucketSort(['a1', 'd0', 'c1', 'b0'], stringBucket, numBuckets: 2)) + .deepEquals(['d0', 'b0', 'a1', 'c1']); + }); + + List<_SortablePair> generatePairs(Iterable keys) { + var token = 0; + return keys.map((k) => _SortablePair(k, "${token++}")).toList(); + } + + void checkSortPairs(int numBuckets, Iterable keys) { + checkBucketSort(numBuckets: numBuckets, bucketOf: (p) => p.key, + generatePairs(keys)); + } + + test('empty list, zero buckets', () { + checkSortPairs(0, []); + }); + + test('empty, some buckets', () { + checkSortPairs(3, []); + }); + + test('interleaved: 4 elements, 2 buckets', () { + checkSortPairs(2, [1, 0, 1, 0]); + }); + + test('some buckets empty: 10 elements in 3 of 10 buckets', () { + checkSortPairs(10, [9, 9, 9, 5, 5, 5, 1, 1, 1, 1]); + }); + + test('one big bucket', () { + checkSortPairs(1, Iterable.generate(100, (_) => 0)); + }); + + const seed = 4321; + + Iterable randomKeys({required int numBuckets, required int length}) { + final rand = Random(seed); + return Iterable.generate(length, (_) => rand.nextInt(numBuckets)); + } + + test('long random list, 1000 in 2 buckets', () { + checkSortPairs(2, randomKeys(numBuckets: 2, length: 1000)); + }); + + test('long random list, 1000 in 1000 buckets', () { + checkSortPairs(1000, randomKeys(numBuckets: 1000, length: 1000)); + }); + + test('sparse random list, 100 in 1000 buckets', () { + checkSortPairs(1000, randomKeys(numBuckets: 1000, length: 100)); + }); + }); +} + +class _SortablePair { + _SortablePair(this.key, this.tag); + + final int key; + final String tag; + + @override + bool operator ==(Object other) { + return other is _SortablePair && key == other.key && tag == other.tag; + } + + @override + int get hashCode => Object.hash(key, tag); + + @override + String toString() => "$tag:$key"; } From eb26c84aec8ea643c83a6516d1993ab872d04f2d Mon Sep 17 00:00:00 2001 From: Greg Price Date: Sat, 7 Dec 2024 19:43:15 -0800 Subject: [PATCH 06/18] emoji [nfc]: Move _testCandidate logic onto query class This logic will grow to handle ranking, and it's closely tied to the matching logic that lives on the query. --- lib/model/emoji.dart | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/model/emoji.dart b/lib/model/emoji.dart index a13d67dace..e9765a0b78 100644 --- a/lib/model/emoji.dart +++ b/lib/model/emoji.dart @@ -342,8 +342,8 @@ class EmojiAutocompleteView extends AutocompleteView Date: Sat, 7 Dec 2024 19:33:49 -0800 Subject: [PATCH 07/18] emoji [nfc]: Mark query-matches method visibleForTesting, conceptually private In the app, the only entry point to this logic should be through EmojiAutocompleteView. The method is public only because that made it more convenient to write thorough unit tests. --- lib/model/emoji.dart | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/model/emoji.dart b/lib/model/emoji.dart index e9765a0b78..4ece116944 100644 --- a/lib/model/emoji.dart +++ b/lib/model/emoji.dart @@ -380,6 +380,7 @@ class EmojiAutocompleteQuery extends ComposeAutocompleteQuery { } // Compare get_emoji_matcher in Zulip web:shared/src/typeahead.ts . + @visibleForTesting bool matches(EmojiCandidate candidate) { if (_adjusted == '') return true; if (candidate.emojiDisplay case UnicodeEmojiDisplay(:var emojiUnicode)) { From a077e9af450b8a74fd9c7035b9e2e92cd59acb7c Mon Sep 17 00:00:00 2001 From: Greg Price Date: Sat, 7 Dec 2024 19:44:10 -0800 Subject: [PATCH 08/18] emoji [nfc]: Make testCandidate visible for testing We'll need this as we add more logic here. --- lib/model/emoji.dart | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/model/emoji.dart b/lib/model/emoji.dart index 4ece116944..a79fb0c2c7 100644 --- a/lib/model/emoji.dart +++ b/lib/model/emoji.dart @@ -343,7 +343,7 @@ class EmojiAutocompleteView extends AutocompleteView Date: Sat, 7 Dec 2024 18:00:59 -0800 Subject: [PATCH 09/18] emoji test [nfc]: Pull matchesNames up to wider scope --- test/model/emoji_test.dart | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/model/emoji_test.dart b/test/model/emoji_test.dart index 312a0f916c..1035e14fcc 100644 --- a/test/model/emoji_test.dart +++ b/test/model/emoji_test.dart @@ -320,8 +320,12 @@ void main() { emojiUnicode: tryParseEmojiCodeToUnicode(emojiCode)!)); } + bool matchesNames(String query, List names) { + return EmojiAutocompleteQuery(query).matches(unicode(names)); + } + bool matchesName(String query, String emojiName) { - return EmojiAutocompleteQuery(query).matches(unicode([emojiName])); + return matchesNames(query, [emojiName]); } test('one-word query matches anywhere in name', () { @@ -370,10 +374,6 @@ void main() { }); test('query matches aliases same way as primary name', () { - bool matchesNames(String query, List names) { - return EmojiAutocompleteQuery(query).matches(unicode(names)); - } - check(matchesNames('a', ['a', 'b'])).isTrue(); check(matchesNames('b', ['a', 'b'])).isTrue(); check(matchesNames('c', ['a', 'b'])).isFalse(); From 5cc514261d521af0395642d601e9d877e758e327 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Sun, 8 Dec 2024 00:44:29 -0800 Subject: [PATCH 10/18] emoji test [nfc]: Tighten query-matches tests This makes these tests a bit easier to read already, with less noisy repetition; and it prepares them for adding more complexity to how this matching works. --- test/model/emoji_test.dart | 32 +++++++++++++++----------------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/test/model/emoji_test.dart b/test/model/emoji_test.dart index 1035e14fcc..56f7a6ed47 100644 --- a/test/model/emoji_test.dart +++ b/test/model/emoji_test.dart @@ -320,8 +320,12 @@ void main() { emojiUnicode: tryParseEmojiCodeToUnicode(emojiCode)!)); } + bool matches(String query, EmojiCandidate candidate) { + return EmojiAutocompleteQuery(query).matches(candidate); + } + bool matchesNames(String query, List names) { - return EmojiAutocompleteQuery(query).matches(unicode(names)); + return matches(query, unicode(names)); } bool matchesName(String query, String emojiName) { @@ -391,8 +395,7 @@ void main() { test('query matches literal Unicode value', () { bool matchesLiteral(String query, String emojiCode, {required String aka}) { assert(aka == query); - return EmojiAutocompleteQuery(query) - .matches(unicode(['asdf'], emojiCode: emojiCode)); + return matches(query, unicode(['asdf'], emojiCode: emojiCode)); } // Matching the code, in hex, doesn't count. @@ -426,16 +429,11 @@ void main() { resolvedStillUrl: eg.realmUrl.resolve('/emoji/1-still.png'))); } - check(EmojiAutocompleteQuery('eqeq') - .matches(realmCandidate('eqeq'))).isTrue(); - check(EmojiAutocompleteQuery('open_') - .matches(realmCandidate('open_book'))).isTrue(); - check(EmojiAutocompleteQuery('n_b') - .matches(realmCandidate('open_book'))).isFalse(); - check(EmojiAutocompleteQuery('blue dia') - .matches(realmCandidate('large_blue_diamond'))).isTrue(); - check(EmojiAutocompleteQuery('Smi') - .matches(realmCandidate('smile'))).isTrue(); + check(matches('eqeq', realmCandidate('eqeq'))).isTrue(); + check(matches('open_', realmCandidate('open_book'))).isTrue(); + check(matches('n_b', realmCandidate('open_book'))).isFalse(); + check(matches('blue dia', realmCandidate('large_blue_diamond'))).isTrue(); + check(matches('Smi', realmCandidate('smile'))).isTrue(); }); test('can match Zulip extra emoji', () { @@ -447,10 +445,10 @@ void main() { emojiType: ReactionType.zulipExtraEmoji, emojiCode: 'zulip', emojiName: 'zulip')); - check(EmojiAutocompleteQuery('z').matches(zulipCandidate)).isTrue(); - check(EmojiAutocompleteQuery('Zulip').matches(zulipCandidate)).isTrue(); - check(EmojiAutocompleteQuery('p').matches(zulipCandidate)).isTrue(); - check(EmojiAutocompleteQuery('x').matches(zulipCandidate)).isFalse(); + check(matches('z', zulipCandidate)).isTrue(); + check(matches('Zulip', zulipCandidate)).isTrue(); + check(matches('p', zulipCandidate)).isTrue(); + check(matches('x', zulipCandidate)).isFalse(); }); }); } From 70e132b26d1e6e6dbd2cf0b6e72889633c90856a Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 6 Dec 2024 21:17:02 -0800 Subject: [PATCH 11/18] emoji [nfc]: Add ranking framework for emoji autocomplete results --- lib/model/autocomplete.dart | 7 +++- lib/model/emoji.dart | 65 +++++++++++++++++++++++++++++++------ test/model/emoji_test.dart | 28 ++++++++-------- 3 files changed, 75 insertions(+), 25 deletions(-) diff --git a/lib/model/autocomplete.dart b/lib/model/autocomplete.dart index a6285a6ab3..5b66a6d52a 100644 --- a/lib/model/autocomplete.dart +++ b/lib/model/autocomplete.dart @@ -758,10 +758,15 @@ sealed class ComposeAutocompleteResult extends AutocompleteResult {} /// An emoji chosen in an autocomplete interaction, via [EmojiAutocompleteView]. class EmojiAutocompleteResult extends ComposeAutocompleteResult { - EmojiAutocompleteResult(this.candidate); + EmojiAutocompleteResult(this.candidate, this.rank); final EmojiCandidate candidate; + /// A measure of the result's quality in the context of the query. + /// + /// Used internally by [EmojiAutocompleteView] for ranking the results. + final int rank; + @override String toString() { return 'EmojiAutocompleteResult(${candidate.description()})'; diff --git a/lib/model/emoji.dart b/lib/model/emoji.dart index a79fb0c2c7..eb12d9904f 100644 --- a/lib/model/emoji.dart +++ b/lib/model/emoji.dart @@ -5,6 +5,7 @@ import '../api/model/events.dart'; import '../api/model/initial_snapshot.dart'; import '../api/model/model.dart'; import '../api/route/realm.dart'; +import 'algorithms.dart'; import 'autocomplete.dart'; import 'narrow.dart'; import 'store.dart'; @@ -319,6 +320,24 @@ class EmojiStoreImpl with EmojiStore { } } +/// The quality of an emoji's match to an autocomplete query. +/// +/// (Rather vacuous for the moment; this structure will +/// gain more substance in an upcoming commit.) +enum EmojiMatchQuality { + match; + + /// The best possible quality of match. + static const best = match; + + /// The better of the two given qualities of match, + /// where null represents no match at all. + static EmojiMatchQuality? bestOf(EmojiMatchQuality? a, EmojiMatchQuality? b) { + if (b == null) return a; + return b; + } +} + class EmojiAutocompleteView extends AutocompleteView { EmojiAutocompleteView._({required super.store, required super.query}); @@ -333,13 +352,13 @@ class EmojiAutocompleteView extends AutocompleteView?> computeResults() async { - // TODO(#1068): rank emoji results (popular, realm, other; exact match, prefix, other) - final results = []; + final unsorted = []; if (await filterCandidates(filter: _testCandidate, - candidates: store.allEmojiCandidates(), results: results)) { + candidates: store.allEmojiCandidates(), results: unsorted)) { return null; } - return results; + return bucketSort(unsorted, + (r) => r.rank, numBuckets: EmojiAutocompleteQuery._numResultRanks); } static EmojiAutocompleteResult? _testCandidate(EmojiAutocompleteQuery query, EmojiCandidate candidate) { @@ -377,18 +396,32 @@ class EmojiAutocompleteQuery extends ComposeAutocompleteQuery { @visibleForTesting EmojiAutocompleteResult? testCandidate(EmojiCandidate candidate) { - return matches(candidate) ? EmojiAutocompleteResult(candidate) : null; + final matchQuality = match(candidate); + if (matchQuality == null) return null; + return EmojiAutocompleteResult(candidate, _rankResult(matchQuality)); } // Compare get_emoji_matcher in Zulip web:shared/src/typeahead.ts . @visibleForTesting - bool matches(EmojiCandidate candidate) { - if (_adjusted == '') return true; + EmojiMatchQuality? match(EmojiCandidate candidate) { + if (_adjusted == '') return EmojiMatchQuality.match; + if (candidate.emojiDisplay case UnicodeEmojiDisplay(:var emojiUnicode)) { - if (_adjusted == emojiUnicode) return true; + if (_adjusted == emojiUnicode) { + return EmojiMatchQuality.match; + } } - return _nameMatches(candidate.emojiName) - || candidate.aliases.any((alias) => _nameMatches(alias)); + + EmojiMatchQuality? result = _matchName(candidate.emojiName); + for (final alias in candidate.aliases) { + if (result == EmojiMatchQuality.best) return result; + result = EmojiMatchQuality.bestOf(result, _matchName(alias)); + } + return result; + } + + EmojiMatchQuality? _matchName(String emojiName) { + return _nameMatches(emojiName) ? EmojiMatchQuality.match : null; } // Compare query_matches_string_in_order in Zulip web:shared/src/typeahead.ts . @@ -409,6 +442,18 @@ class EmojiAutocompleteQuery extends ComposeAutocompleteQuery { || emojiName.contains(_sepAdjusted); } + /// A measure of the result's quality in the context of the query, + /// ranked from 0 (best) to one less than [_numResultRanks]. + static int _rankResult(EmojiMatchQuality matchQuality) { + // TODO(#1068): rank emoji results (popular, realm, other; exact match, prefix, other) + return switch (matchQuality) { + EmojiMatchQuality.match => 0, + }; + } + + /// The number of possible values returned by [_rankResult]. + static const _numResultRanks = 1; + @override String toString() { return '${objectRuntimeType(this, 'EmojiAutocompleteQuery')}($raw)'; diff --git a/test/model/emoji_test.dart b/test/model/emoji_test.dart index 56f7a6ed47..26df8001a2 100644 --- a/test/model/emoji_test.dart +++ b/test/model/emoji_test.dart @@ -309,7 +309,7 @@ void main() { }); }); - group('EmojiAutocompleteQuery.matches', () { + group('EmojiAutocompleteQuery.match', () { EmojiCandidate unicode(List names, {String? emojiCode}) { emojiCode ??= '10ffff'; return EmojiCandidate(emojiType: ReactionType.unicodeEmoji, @@ -320,12 +320,12 @@ void main() { emojiUnicode: tryParseEmojiCodeToUnicode(emojiCode)!)); } - bool matches(String query, EmojiCandidate candidate) { - return EmojiAutocompleteQuery(query).matches(candidate); + EmojiMatchQuality? matchOf(String query, EmojiCandidate candidate) { + return EmojiAutocompleteQuery(query).match(candidate); } bool matchesNames(String query, List names) { - return matches(query, unicode(names)); + return matchOf(query, unicode(names)) != null; } bool matchesName(String query, String emojiName) { @@ -395,7 +395,7 @@ void main() { test('query matches literal Unicode value', () { bool matchesLiteral(String query, String emojiCode, {required String aka}) { assert(aka == query); - return matches(query, unicode(['asdf'], emojiCode: emojiCode)); + return matchOf(query, unicode(['asdf'], emojiCode: emojiCode)) != null; } // Matching the code, in hex, doesn't count. @@ -429,11 +429,11 @@ void main() { resolvedStillUrl: eg.realmUrl.resolve('/emoji/1-still.png'))); } - check(matches('eqeq', realmCandidate('eqeq'))).isTrue(); - check(matches('open_', realmCandidate('open_book'))).isTrue(); - check(matches('n_b', realmCandidate('open_book'))).isFalse(); - check(matches('blue dia', realmCandidate('large_blue_diamond'))).isTrue(); - check(matches('Smi', realmCandidate('smile'))).isTrue(); + check(matchOf('eqeq', realmCandidate('eqeq'))).isNotNull(); + check(matchOf('open_', realmCandidate('open_book'))).isNotNull(); + check(matchOf('n_b', realmCandidate('open_book'))).isNull(); + check(matchOf('blue dia', realmCandidate('large_blue_diamond'))).isNotNull(); + check(matchOf('Smi', realmCandidate('smile'))).isNotNull(); }); test('can match Zulip extra emoji', () { @@ -445,10 +445,10 @@ void main() { emojiType: ReactionType.zulipExtraEmoji, emojiCode: 'zulip', emojiName: 'zulip')); - check(matches('z', zulipCandidate)).isTrue(); - check(matches('Zulip', zulipCandidate)).isTrue(); - check(matches('p', zulipCandidate)).isTrue(); - check(matches('x', zulipCandidate)).isFalse(); + check(matchOf('z', zulipCandidate)).isNotNull(); + check(matchOf('Zulip', zulipCandidate)).isNotNull(); + check(matchOf('p', zulipCandidate)).isNotNull(); + check(matchOf('x', zulipCandidate)).isNull(); }); }); } From 604ed1ef80cc9aa8bcd3ce7c05d9b620f03e2e68 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Sat, 7 Dec 2024 17:47:57 -0800 Subject: [PATCH 12/18] emoji test [nfc]: Make the EmojiMatchQuality values explicit This actually has a pretty nice effect on the readability of these tests, even at this stage where the enum isn't doing anything! Separate from the parent commit just because this one is a bigger, and almost entirely mechanical and boring, diff. --- test/model/emoji_test.dart | 134 +++++++++++++++++++------------------ 1 file changed, 70 insertions(+), 64 deletions(-) diff --git a/test/model/emoji_test.dart b/test/model/emoji_test.dart index 26df8001a2..054d9ca5dc 100644 --- a/test/model/emoji_test.dart +++ b/test/model/emoji_test.dart @@ -324,98 +324,99 @@ void main() { return EmojiAutocompleteQuery(query).match(candidate); } - bool matchesNames(String query, List names) { - return matchOf(query, unicode(names)) != null; + EmojiMatchQuality? matchOfNames(String query, List names) { + return matchOf(query, unicode(names)); } - bool matchesName(String query, String emojiName) { - return matchesNames(query, [emojiName]); + EmojiMatchQuality? matchOfName(String query, String emojiName) { + return matchOfNames(query, [emojiName]); } test('one-word query matches anywhere in name', () { - check(matchesName('', 'smile')).isTrue(); - check(matchesName('s', 'smile')).isTrue(); - check(matchesName('sm', 'smile')).isTrue(); - check(matchesName('smile', 'smile')).isTrue(); - check(matchesName('m', 'smile')).isTrue(); - check(matchesName('mile', 'smile')).isTrue(); - check(matchesName('e', 'smile')).isTrue(); - - check(matchesName('smiley', 'smile')).isFalse(); - check(matchesName('a', 'smile')).isFalse(); - - check(matchesName('o', 'open_book')).isTrue(); - check(matchesName('open', 'open_book')).isTrue(); - check(matchesName('pe', 'open_book')).isTrue(); - check(matchesName('boo', 'open_book')).isTrue(); - check(matchesName('ok', 'open_book')).isTrue(); + check(matchOfName('', 'smile')).match; + check(matchOfName('s', 'smile')).match; + check(matchOfName('sm', 'smile')).match; + check(matchOfName('smile', 'smile')).match; + check(matchOfName('m', 'smile')).match; + check(matchOfName('mile', 'smile')).match; + check(matchOfName('e', 'smile')).match; + + check(matchOfName('smiley', 'smile')).none; + check(matchOfName('a', 'smile')).none; + + check(matchOfName('o', 'open_book')).match; + check(matchOfName('open', 'open_book')).match; + check(matchOfName('pe', 'open_book')).match; + check(matchOfName('boo', 'open_book')).match; + check(matchOfName('ok', 'open_book')).match; }); test('multi-word query matches from start of a word', () { - check(matchesName('open_', 'open_book')).isTrue(); - check(matchesName('open_b', 'open_book')).isTrue(); - check(matchesName('open_book', 'open_book')).isTrue(); + check(matchOfName('open_', 'open_book')).match; + check(matchOfName('open_b', 'open_book')).match; + check(matchOfName('open_book', 'open_book')).match; - check(matchesName('pen_', 'open_book')).isFalse(); - check(matchesName('n_b', 'open_book')).isFalse(); + check(matchOfName('pen_', 'open_book')).none; + check(matchOfName('n_b', 'open_book')).none; - check(matchesName('blue_dia', 'large_blue_diamond')).isTrue(); + check(matchOfName('blue_dia', 'large_blue_diamond')).match; }); test('spaces in query behave as underscores', () { - check(matchesName('open ', 'open_book')).isTrue(); - check(matchesName('open b', 'open_book')).isTrue(); - check(matchesName('open book', 'open_book')).isTrue(); + check(matchOfName('open ', 'open_book')).match; + check(matchOfName('open b', 'open_book')).match; + check(matchOfName('open book', 'open_book')).match; - check(matchesName('pen ', 'open_book')).isFalse(); - check(matchesName('n b', 'open_book')).isFalse(); + check(matchOfName('pen ', 'open_book')).none; + check(matchOfName('n b', 'open_book')).none; - check(matchesName('blue dia', 'large_blue_diamond')).isTrue(); + check(matchOfName('blue dia', 'large_blue_diamond')).match; }); test('query is lower-cased', () { - check(matchesName('Smi', 'smile')).isTrue(); + check(matchOfName('Smi', 'smile')).match; }); test('query matches aliases same way as primary name', () { - check(matchesNames('a', ['a', 'b'])).isTrue(); - check(matchesNames('b', ['a', 'b'])).isTrue(); - check(matchesNames('c', ['a', 'b'])).isFalse(); + check(matchOfNames('a', ['a', 'b'])).match; + check(matchOfNames('b', ['a', 'b'])).match; + check(matchOfNames('c', ['a', 'b'])).none; - check(matchesNames('pe', ['x', 'open_book'])).isTrue(); - check(matchesNames('ok', ['x', 'open_book'])).isTrue(); + check(matchOfNames('pe', ['x', 'open_book'])).match; + check(matchOfNames('ok', ['x', 'open_book'])).match; - check(matchesNames('open_', ['x', 'open_book'])).isTrue(); - check(matchesNames('open b', ['x', 'open_book'])).isTrue(); - check(matchesNames('pen_', ['x', 'open_book'])).isFalse(); + check(matchOfNames('open_', ['x', 'open_book'])).match; + check(matchOfNames('open b', ['x', 'open_book'])).match; + check(matchOfNames('pen_', ['x', 'open_book'])).none; - check(matchesNames('Smi', ['x', 'smile'])).isTrue(); + check(matchOfNames('Smi', ['x', 'smile'])).match; }); test('query matches literal Unicode value', () { - bool matchesLiteral(String query, String emojiCode, {required String aka}) { + EmojiMatchQuality? matchOfLiteral(String query, String emojiCode, { + required String aka}) { assert(aka == query); - return matchOf(query, unicode(['asdf'], emojiCode: emojiCode)) != null; + return matchOf(query, unicode(['asdf'], emojiCode: emojiCode)); } // Matching the code, in hex, doesn't count. - check(matchesLiteral('1f642', aka: '1f642', '1f642')).isFalse(); + check(matchOfLiteral('1f642', aka: '1f642', '1f642')).none; // Matching the Unicode value the code describes does count… - check(matchesLiteral('πŸ™‚', aka: '\u{1f642}', '1f642')).isTrue(); + check(matchOfLiteral('πŸ™‚', aka: '\u{1f642}', '1f642')).match; // … and failing to match it doesn't make a match. - check(matchesLiteral('πŸ™', aka: '\u{1f641}', '1f642')).isFalse(); + check(matchOfLiteral('πŸ™', aka: '\u{1f641}', '1f642')).none; // Multi-code-point emoji work fine. - check(matchesLiteral('πŸ³β€πŸŒˆ', aka: '\u{1f3f3}\u{200d}\u{1f308}', - '1f3f3-200d-1f308')).isTrue(); + check(matchOfLiteral('πŸ³β€πŸŒˆ', aka: '\u{1f3f3}\u{200d}\u{1f308}', + '1f3f3-200d-1f308')).match; // Only exact matches count; no partial matches. - check(matchesLiteral('🏳', aka: '\u{1f3f3}', - '1f3f3-200d-1f308')).isFalse(); - check(matchesLiteral('β€πŸŒˆ', aka: '\u{200d}\u{1f308}', - '1f3f3-200d-1f308')).isFalse(); - check(matchesLiteral('πŸ³β€πŸŒˆ', aka: '\u{1f3f3}\u{200d}\u{1f308}', - '1f3f3')).isFalse(); + check(matchOfLiteral('🏳', aka: '\u{1f3f3}', + '1f3f3-200d-1f308')).none; + check(matchOfLiteral('β€πŸŒˆ', aka: '\u{200d}\u{1f308}', + '1f3f3-200d-1f308')).none; + check(matchOfLiteral('πŸ³β€πŸŒˆ', aka: '\u{1f3f3}\u{200d}\u{1f308}', + '1f3f3')).none; }); test('can match realm emoji', () { @@ -429,11 +430,11 @@ void main() { resolvedStillUrl: eg.realmUrl.resolve('/emoji/1-still.png'))); } - check(matchOf('eqeq', realmCandidate('eqeq'))).isNotNull(); - check(matchOf('open_', realmCandidate('open_book'))).isNotNull(); - check(matchOf('n_b', realmCandidate('open_book'))).isNull(); - check(matchOf('blue dia', realmCandidate('large_blue_diamond'))).isNotNull(); - check(matchOf('Smi', realmCandidate('smile'))).isNotNull(); + check(matchOf('eqeq', realmCandidate('eqeq'))).match; + check(matchOf('open_', realmCandidate('open_book'))).match; + check(matchOf('n_b', realmCandidate('open_book'))).none; + check(matchOf('blue dia', realmCandidate('large_blue_diamond'))).match; + check(matchOf('Smi', realmCandidate('smile'))).match; }); test('can match Zulip extra emoji', () { @@ -445,10 +446,10 @@ void main() { emojiType: ReactionType.zulipExtraEmoji, emojiCode: 'zulip', emojiName: 'zulip')); - check(matchOf('z', zulipCandidate)).isNotNull(); - check(matchOf('Zulip', zulipCandidate)).isNotNull(); - check(matchOf('p', zulipCandidate)).isNotNull(); - check(matchOf('x', zulipCandidate)).isNull(); + check(matchOf('z', zulipCandidate)).match; + check(matchOf('Zulip', zulipCandidate)).match; + check(matchOf('p', zulipCandidate)).match; + check(matchOf('x', zulipCandidate)).none; }); }); } @@ -474,6 +475,11 @@ extension EmojiCandidateChecks on Subject { Subject get emojiDisplay => has((x) => x.emojiDisplay, 'emojiDisplay'); } +extension EmojiMatchQualityChecks on Subject { + void get match => equals(EmojiMatchQuality.match); + void get none => isNull(); +} + extension EmojiAutocompleteResultChecks on Subject { Subject get candidate => has((x) => x.candidate, 'candidate'); } From 37652dbc9370492e000758f75b73aee7397f324a Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 6 Dec 2024 21:45:33 -0800 Subject: [PATCH 13/18] emoji: Rank by quality of match (exact, prefix, other) Fixes part of #1068. --- lib/model/emoji.dart | 79 +++++++++++++++----- test/model/emoji_test.dart | 144 +++++++++++++++++++++++++++---------- 2 files changed, 167 insertions(+), 56 deletions(-) diff --git a/lib/model/emoji.dart b/lib/model/emoji.dart index eb12d9904f..2e365f812f 100644 --- a/lib/model/emoji.dart +++ b/lib/model/emoji.dart @@ -223,6 +223,9 @@ class EmojiStoreImpl with EmojiStore { } List _generateAllCandidates() { + // See also [EmojiAutocompleteQuery._rankResult]; + // that ranking takes precedence over the order of this list. + // // Compare `emoji_picker.rebuild_catalog` in Zulip web; // `composebox_typeahead.update_emoji_data` which receives its output; // and `emoji.update_emojis` which builds part of its input. @@ -321,20 +324,30 @@ class EmojiStoreImpl with EmojiStore { } /// The quality of an emoji's match to an autocomplete query. -/// -/// (Rather vacuous for the moment; this structure will -/// gain more substance in an upcoming commit.) enum EmojiMatchQuality { - match; + /// The query matches the whole emoji name (or the literal emoji itself). + exact, + + /// The query matches a prefix of the emoji name, but not the whole name. + prefix, + + /// The query matches somewhere in the emoji name, but not at the start. + other; /// The best possible quality of match. - static const best = match; + static const best = exact; /// The better of the two given qualities of match, /// where null represents no match at all. static EmojiMatchQuality? bestOf(EmojiMatchQuality? a, EmojiMatchQuality? b) { if (b == null) return a; - return b; + if (a == null) return b; + return compare(a, b) <= 0 ? a : b; + } + + /// Comparator that puts better matches first. + static int compare(EmojiMatchQuality a, EmojiMatchQuality b) { + return Enum.compareByIndex(a, b); } } @@ -404,11 +417,11 @@ class EmojiAutocompleteQuery extends ComposeAutocompleteQuery { // Compare get_emoji_matcher in Zulip web:shared/src/typeahead.ts . @visibleForTesting EmojiMatchQuality? match(EmojiCandidate candidate) { - if (_adjusted == '') return EmojiMatchQuality.match; + if (_adjusted == '') return EmojiMatchQuality.prefix; if (candidate.emojiDisplay case UnicodeEmojiDisplay(:var emojiUnicode)) { if (_adjusted == emojiUnicode) { - return EmojiMatchQuality.match; + return EmojiMatchQuality.exact; } } @@ -421,13 +434,20 @@ class EmojiAutocompleteQuery extends ComposeAutocompleteQuery { } EmojiMatchQuality? _matchName(String emojiName) { - return _nameMatches(emojiName) ? EmojiMatchQuality.match : null; - } + // Compare query_matches_string_in_order in Zulip web:shared/src/typeahead.ts + // for a Boolean version of this logic (match vs. no match), + // and triage_raw in the same file web:shared/src/typeahead.ts + // for the finer distinctions. + // See also commentary in [_rankResult]. - // Compare query_matches_string_in_order in Zulip web:shared/src/typeahead.ts . - bool _nameMatches(String emojiName) { // TODO(#1067) this assumes emojiName is already lower-case (and no diacritics) + if (emojiName == _adjusted) return EmojiMatchQuality.exact; + if (emojiName.startsWith(_adjusted)) return EmojiMatchQuality.prefix; + if (_nameMatches(emojiName)) return EmojiMatchQuality.other; + return null; + } + bool _nameMatches(String emojiName) { if (!_adjusted.contains(_separator)) { // If the query is a single token (doesn't contain a separator), // the match can be anywhere in the string. @@ -438,21 +458,46 @@ class EmojiAutocompleteQuery extends ComposeAutocompleteQuery { // require the match to start at the start of a token. // (E.g. for 'ab_cd_ef', query could be 'ab_c' or 'cd_ef', // but not 'b_cd_ef'.) - return emojiName.startsWith(_adjusted) - || emojiName.contains(_sepAdjusted); + assert(!emojiName.startsWith(_adjusted)); // checked before calling this method + return emojiName.contains(_sepAdjusted); } /// A measure of the result's quality in the context of the query, /// ranked from 0 (best) to one less than [_numResultRanks]. static int _rankResult(EmojiMatchQuality matchQuality) { - // TODO(#1068): rank emoji results (popular, realm, other; exact match, prefix, other) + // See also [EmojiStoreImpl._generateAllCandidates]; + // emoji which this function ranks equally + // will appear in the order they were put in by that method. + // + // Compare sort_emojis in Zulip web: + // https://github.com/zulip/zulip/blob/83a121c7e/web/shared/src/typeahead.ts#L322-L382 + // + // Behavior differences we should or might copy, TODO(#1068): + // * Web ranks popular emoji > custom emoji > others; we don't yet. + // * Web ranks matches starting at a word boundary ahead of + // other non-prefix matches; we don't yet. + // * Web ranks each name of a Unicode emoji separately. + // + // Behavior differences that web should probably fix, TODO(web): + // * Web starts with only case-sensitive exact matches ("perfect matches"), + // and puts case-insensitive exact matches just ahead of prefix matches; + // it also distinguishes prefix matches by case-sensitive vs. not. + // We use case-insensitive matches throughout; + // case seems unhelpful for emoji search. + // * Web suppresses Unicode emoji names shadowed by a realm emoji + // only if the latter is also a match for the query. That mostly works, + // because emoji with the same name will mostly both match or both not; + // but it breaks if the Unicode emoji was a literal match. + return switch (matchQuality) { - EmojiMatchQuality.match => 0, + EmojiMatchQuality.exact => 0, + EmojiMatchQuality.prefix => 1, + EmojiMatchQuality.other => 2, }; } /// The number of possible values returned by [_rankResult]. - static const _numResultRanks = 1; + static const _numResultRanks = 3; @override String toString() { diff --git a/test/model/emoji_test.dart b/test/model/emoji_test.dart index 054d9ca5dc..416b6ea7b7 100644 --- a/test/model/emoji_test.dart +++ b/test/model/emoji_test.dart @@ -307,9 +307,44 @@ void main() { check(view.results).single.which( isUnicodeResult(names: ['smile'])); }); + + Future> resultsOf( + String query, { + Map realmEmoji = const {}, + Map>? unicodeEmoji, + }) async { + final store = prepare(realmEmoji: realmEmoji, unicodeEmoji: unicodeEmoji); + final view = EmojiAutocompleteView.init(store: store, + query: EmojiAutocompleteQuery(query)); + bool done = false; + view.addListener(() { done = true; }); + await Future(() {}); + check(done).isTrue(); + return view.results; + } + + test('results end-to-end', () async { + final unicodeEmoji = { + '1f4d3': ['notebook'], '1f516': ['bookmark'], '1f4d6': ['book']}; + + // Empty query -> base ordering. + check(await resultsOf('', unicodeEmoji: unicodeEmoji)).deepEquals([ + isUnicodeResult(names: ['notebook']), + isUnicodeResult(names: ['bookmark']), + isUnicodeResult(names: ['book']), + isZulipResult(), + ]); + + // With query, exact match precedes prefix match precedes other. + check(await resultsOf('book', unicodeEmoji: unicodeEmoji)).deepEquals([ + isUnicodeResult(names: ['book']), + isUnicodeResult(names: ['bookmark']), + isUnicodeResult(names: ['notebook']), + ]); + }); }); - group('EmojiAutocompleteQuery.match', () { + group('EmojiAutocompleteQuery', () { EmojiCandidate unicode(List names, {String? emojiCode}) { emojiCode ??= '10ffff'; return EmojiCandidate(emojiType: ReactionType.unicodeEmoji, @@ -333,63 +368,71 @@ void main() { } test('one-word query matches anywhere in name', () { - check(matchOfName('', 'smile')).match; - check(matchOfName('s', 'smile')).match; - check(matchOfName('sm', 'smile')).match; - check(matchOfName('smile', 'smile')).match; - check(matchOfName('m', 'smile')).match; - check(matchOfName('mile', 'smile')).match; - check(matchOfName('e', 'smile')).match; + check(matchOfName('', 'smile')).prefix; + check(matchOfName('s', 'smile')).prefix; + check(matchOfName('sm', 'smile')).prefix; + check(matchOfName('smile', 'smile')).exact; + check(matchOfName('m', 'smile')).other; + check(matchOfName('mile', 'smile')).other; + check(matchOfName('e', 'smile')).other; check(matchOfName('smiley', 'smile')).none; check(matchOfName('a', 'smile')).none; - check(matchOfName('o', 'open_book')).match; - check(matchOfName('open', 'open_book')).match; - check(matchOfName('pe', 'open_book')).match; - check(matchOfName('boo', 'open_book')).match; - check(matchOfName('ok', 'open_book')).match; + check(matchOfName('o', 'open_book')).prefix; + check(matchOfName('open', 'open_book')).prefix; + check(matchOfName('pe', 'open_book')).other; + check(matchOfName('boo', 'open_book')).other; + check(matchOfName('ok', 'open_book')).other; }); test('multi-word query matches from start of a word', () { - check(matchOfName('open_', 'open_book')).match; - check(matchOfName('open_b', 'open_book')).match; - check(matchOfName('open_book', 'open_book')).match; + check(matchOfName('open_', 'open_book')).prefix; + check(matchOfName('open_b', 'open_book')).prefix; + check(matchOfName('open_book', 'open_book')).exact; check(matchOfName('pen_', 'open_book')).none; check(matchOfName('n_b', 'open_book')).none; - check(matchOfName('blue_dia', 'large_blue_diamond')).match; + check(matchOfName('blue_dia', 'large_blue_diamond')).other; }); test('spaces in query behave as underscores', () { - check(matchOfName('open ', 'open_book')).match; - check(matchOfName('open b', 'open_book')).match; - check(matchOfName('open book', 'open_book')).match; + check(matchOfName('open ', 'open_book')).prefix; + check(matchOfName('open b', 'open_book')).prefix; + check(matchOfName('open book', 'open_book')).exact; check(matchOfName('pen ', 'open_book')).none; check(matchOfName('n b', 'open_book')).none; - check(matchOfName('blue dia', 'large_blue_diamond')).match; + check(matchOfName('blue dia', 'large_blue_diamond')).other; }); test('query is lower-cased', () { - check(matchOfName('Smi', 'smile')).match; + check(matchOfName('Smi', 'smile')).prefix; }); test('query matches aliases same way as primary name', () { - check(matchOfNames('a', ['a', 'b'])).match; - check(matchOfNames('b', ['a', 'b'])).match; + check(matchOfNames('a', ['a', 'b'])).exact; + check(matchOfNames('b', ['a', 'b'])).exact; check(matchOfNames('c', ['a', 'b'])).none; - check(matchOfNames('pe', ['x', 'open_book'])).match; - check(matchOfNames('ok', ['x', 'open_book'])).match; + check(matchOfNames('pe', ['x', 'open_book'])).other; + check(matchOfNames('ok', ['x', 'open_book'])).other; - check(matchOfNames('open_', ['x', 'open_book'])).match; - check(matchOfNames('open b', ['x', 'open_book'])).match; + check(matchOfNames('open_', ['x', 'open_book'])).prefix; + check(matchOfNames('open b', ['x', 'open_book'])).prefix; check(matchOfNames('pen_', ['x', 'open_book'])).none; - check(matchOfNames('Smi', ['x', 'smile'])).match; + check(matchOfNames('Smi', ['x', 'smile'])).prefix; + }); + + test('best match among name and aliases prevails', () { + check(matchOfNames('a', ['ab', 'a', 'ba', 'x'])).exact; + check(matchOfNames('a', ['ba', 'ab', 'x'])).prefix; + check(matchOfNames('a', ['ba', 'ab'])).prefix; + check(matchOfNames('a', ['ba', 'x'])).other; + check(matchOfNames('a', ['x', 'y', 'z'])).none; }); test('query matches literal Unicode value', () { @@ -403,13 +446,13 @@ void main() { check(matchOfLiteral('1f642', aka: '1f642', '1f642')).none; // Matching the Unicode value the code describes does count… - check(matchOfLiteral('πŸ™‚', aka: '\u{1f642}', '1f642')).match; + check(matchOfLiteral('πŸ™‚', aka: '\u{1f642}', '1f642')).exact; // … and failing to match it doesn't make a match. check(matchOfLiteral('πŸ™', aka: '\u{1f641}', '1f642')).none; // Multi-code-point emoji work fine. check(matchOfLiteral('πŸ³β€πŸŒˆ', aka: '\u{1f3f3}\u{200d}\u{1f308}', - '1f3f3-200d-1f308')).match; + '1f3f3-200d-1f308')).exact; // Only exact matches count; no partial matches. check(matchOfLiteral('🏳', aka: '\u{1f3f3}', '1f3f3-200d-1f308')).none; @@ -430,11 +473,11 @@ void main() { resolvedStillUrl: eg.realmUrl.resolve('/emoji/1-still.png'))); } - check(matchOf('eqeq', realmCandidate('eqeq'))).match; - check(matchOf('open_', realmCandidate('open_book'))).match; + check(matchOf('eqeq', realmCandidate('eqeq'))).exact; + check(matchOf('open_', realmCandidate('open_book'))).prefix; check(matchOf('n_b', realmCandidate('open_book'))).none; - check(matchOf('blue dia', realmCandidate('large_blue_diamond'))).match; - check(matchOf('Smi', realmCandidate('smile'))).match; + check(matchOf('blue dia', realmCandidate('large_blue_diamond'))).other; + check(matchOf('Smi', realmCandidate('smile'))).prefix; }); test('can match Zulip extra emoji', () { @@ -446,11 +489,32 @@ void main() { emojiType: ReactionType.zulipExtraEmoji, emojiCode: 'zulip', emojiName: 'zulip')); - check(matchOf('z', zulipCandidate)).match; - check(matchOf('Zulip', zulipCandidate)).match; - check(matchOf('p', zulipCandidate)).match; + check(matchOf('z', zulipCandidate)).prefix; + check(matchOf('Zulip', zulipCandidate)).exact; + check(matchOf('p', zulipCandidate)).other; check(matchOf('x', zulipCandidate)).none; }); + + int? rankOf(String query, EmojiCandidate candidate) { + return EmojiAutocompleteQuery(query).testCandidate(candidate)?.rank; + } + + void checkPrecedes(String query, EmojiCandidate a, EmojiCandidate b) { + check(rankOf(query, a)!).isLessThan(rankOf(query, b)!); + } + + test('ranks exact before prefix before other match', () { + checkPrecedes('o', unicode(['o']), unicode(['onion'])); + checkPrecedes('o', unicode(['onion']), unicode(['book'])); + }); + + test('full list of ranks', () { + check([ + rankOf('o', unicode(['o'])), // exact + rankOf('o', unicode(['onion'])), // prefix + rankOf('o', unicode(['book'])), // other + ]).deepEquals([0, 1, 2]); + }); }); } @@ -476,7 +540,9 @@ extension EmojiCandidateChecks on Subject { } extension EmojiMatchQualityChecks on Subject { - void get match => equals(EmojiMatchQuality.match); + void get exact => equals(EmojiMatchQuality.exact); + void get prefix => equals(EmojiMatchQuality.prefix); + void get other => equals(EmojiMatchQuality.other); void get none => isNull(); } From 70004b0c805d9fe9849beebacef8ac661bd69310 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Sat, 7 Dec 2024 20:19:55 -0800 Subject: [PATCH 14/18] emoji test [nfc]: Extract helpers realmCandidate, zulipCandidate --- test/model/emoji_test.dart | 34 ++++++++++++++++++---------------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/test/model/emoji_test.dart b/test/model/emoji_test.dart index 416b6ea7b7..c14d4dfdf9 100644 --- a/test/model/emoji_test.dart +++ b/test/model/emoji_test.dart @@ -462,17 +462,17 @@ void main() { '1f3f3')).none; }); - test('can match realm emoji', () { - EmojiCandidate realmCandidate(String emojiName) { - return EmojiCandidate( - emojiType: ReactionType.realmEmoji, - emojiCode: '1', emojiName: emojiName, aliases: null, - emojiDisplay: ImageEmojiDisplay( - emojiName: emojiName, - resolvedUrl: eg.realmUrl.resolve('/emoji/1.png'), - resolvedStillUrl: eg.realmUrl.resolve('/emoji/1-still.png'))); - } + EmojiCandidate realmCandidate(String emojiName) { + return EmojiCandidate( + emojiType: ReactionType.realmEmoji, + emojiCode: '1', emojiName: emojiName, aliases: null, + emojiDisplay: ImageEmojiDisplay( + emojiName: emojiName, + resolvedUrl: eg.realmUrl.resolve('/emoji/1.png'), + resolvedStillUrl: eg.realmUrl.resolve('/emoji/1-still.png'))); + } + test('can match realm emoji', () { check(matchOf('eqeq', realmCandidate('eqeq'))).exact; check(matchOf('open_', realmCandidate('open_book'))).prefix; check(matchOf('n_b', realmCandidate('open_book'))).none; @@ -480,19 +480,21 @@ void main() { check(matchOf('Smi', realmCandidate('smile'))).prefix; }); - test('can match Zulip extra emoji', () { + EmojiCandidate zulipCandidate() { final store = eg.store(); - final zulipCandidate = EmojiCandidate( + return EmojiCandidate( emojiType: ReactionType.zulipExtraEmoji, emojiCode: 'zulip', emojiName: 'zulip', aliases: null, emojiDisplay: store.emojiDisplayFor( emojiType: ReactionType.zulipExtraEmoji, emojiCode: 'zulip', emojiName: 'zulip')); + } - check(matchOf('z', zulipCandidate)).prefix; - check(matchOf('Zulip', zulipCandidate)).exact; - check(matchOf('p', zulipCandidate)).other; - check(matchOf('x', zulipCandidate)).none; + test('can match Zulip extra emoji', () { + check(matchOf('z', zulipCandidate())).prefix; + check(matchOf('Zulip', zulipCandidate())).exact; + check(matchOf('p', zulipCandidate())).other; + check(matchOf('x', zulipCandidate())).none; }); int? rankOf(String query, EmojiCandidate candidate) { From 6c93b40ec7613acb68d862f63c3d5e91964c224c Mon Sep 17 00:00:00 2001 From: Greg Price Date: Sun, 8 Dec 2024 13:37:08 -0800 Subject: [PATCH 15/18] emoji: Add list of the "popular" emoji --- lib/model/emoji.dart | 39 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-) diff --git a/lib/model/emoji.dart b/lib/model/emoji.dart index 2e365f812f..fcd976a6dd 100644 --- a/lib/model/emoji.dart +++ b/lib/model/emoji.dart @@ -109,6 +109,19 @@ mixin EmojiStore { required String emojiName, }); + /// Zulip's list of "popular" emoji, to be given precedence in + /// offering to users. + /// + /// See description in the web code: + /// https://github.com/zulip/zulip/blob/83a121c7e/web/shared/src/typeahead.ts#L3-L21 + // Someday this list may start varying rather than being hard-coded, + // and then this will become a non-static member on EmojiStore. + // For now, though, the fact it's constant is convenient when writing + // tests of the logic that uses this data; so we guarantee it in the API. + static Iterable get popularEmojiCandidates { + return EmojiStoreImpl._popularCandidates; + } + Iterable allEmojiCandidates(); // TODO cut debugServerEmojiData once we can query for lists of emoji; @@ -207,7 +220,29 @@ class EmojiStoreImpl with EmojiStore { /// retrieving the data. Map>? _serverEmojiData; - List? _allEmojiCandidates; + static final _popularCandidates = _generatePopularCandidates(); + + static List _generatePopularCandidates() { + EmojiCandidate candidate(String emojiCode, String emojiUnicode, + List names) { + final emojiName = names.removeAt(0); + assert(emojiUnicode == tryParseEmojiCodeToUnicode(emojiCode)); + return EmojiCandidate(emojiType: ReactionType.unicodeEmoji, + emojiCode: emojiCode, emojiName: emojiName, aliases: names, + emojiDisplay: UnicodeEmojiDisplay( + emojiName: emojiName, emojiUnicode: emojiUnicode)); + } + return [ + // This list should match web: + // https://github.com/zulip/zulip/blob/83a121c7e/web/shared/src/typeahead.ts#L22-L29 + candidate('1f44d', 'πŸ‘', ['+1', 'thumbs_up', 'like']), + candidate('1f389', 'πŸŽ‰', ['tada']), + candidate('1f642', 'πŸ™‚', ['smile']), + candidate( '2764', '❀', ['heart', 'love', 'love_you']), + candidate('1f6e0', 'πŸ› ', ['working_on_it', 'hammer_and_wrench', 'tools']), + candidate('1f419', 'πŸ™', ['octopus']), + ]; + } EmojiCandidate _emojiCandidateFor({ required ReactionType emojiType, @@ -306,6 +341,8 @@ class EmojiStoreImpl with EmojiStore { return results; } + List? _allEmojiCandidates; + @override Iterable allEmojiCandidates() { return _allEmojiCandidates ??= _generateAllCandidates(); From bb8935a5d8374b25799a5be5e95c30052668c699 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Sat, 7 Dec 2024 00:08:22 -0800 Subject: [PATCH 16/18] emoji: Rank "popular" > custom > other emoji Fixes part of #1068. --- lib/model/emoji.dart | 40 +++++++++++++++++++++++----- test/model/emoji_test.dart | 53 +++++++++++++++++++++++++++++++++----- 2 files changed, 81 insertions(+), 12 deletions(-) diff --git a/lib/model/emoji.dart b/lib/model/emoji.dart index fcd976a6dd..e1b1360676 100644 --- a/lib/model/emoji.dart +++ b/lib/model/emoji.dart @@ -244,6 +244,17 @@ class EmojiStoreImpl with EmojiStore { ]; } + static final _popularEmojiCodes = (() { + assert(_popularCandidates.every((c) => + c.emojiType == ReactionType.unicodeEmoji)); + return Set.of(_popularCandidates.map((c) => c.emojiCode)); + })(); + + static bool _isPopularEmoji(EmojiCandidate candidate) { + return candidate.emojiType == ReactionType.unicodeEmoji + && _popularEmojiCodes.contains(candidate.emojiCode); + } + EmojiCandidate _emojiCandidateFor({ required ReactionType emojiType, required String emojiCode, @@ -448,7 +459,8 @@ class EmojiAutocompleteQuery extends ComposeAutocompleteQuery { EmojiAutocompleteResult? testCandidate(EmojiCandidate candidate) { final matchQuality = match(candidate); if (matchQuality == null) return null; - return EmojiAutocompleteResult(candidate, _rankResult(matchQuality)); + return EmojiAutocompleteResult(candidate, + _rankResult(matchQuality, candidate)); } // Compare get_emoji_matcher in Zulip web:shared/src/typeahead.ts . @@ -501,7 +513,7 @@ class EmojiAutocompleteQuery extends ComposeAutocompleteQuery { /// A measure of the result's quality in the context of the query, /// ranked from 0 (best) to one less than [_numResultRanks]. - static int _rankResult(EmojiMatchQuality matchQuality) { + static int _rankResult(EmojiMatchQuality matchQuality, EmojiCandidate candidate) { // See also [EmojiStoreImpl._generateAllCandidates]; // emoji which this function ranks equally // will appear in the order they were put in by that method. @@ -510,12 +522,19 @@ class EmojiAutocompleteQuery extends ComposeAutocompleteQuery { // https://github.com/zulip/zulip/blob/83a121c7e/web/shared/src/typeahead.ts#L322-L382 // // Behavior differences we should or might copy, TODO(#1068): - // * Web ranks popular emoji > custom emoji > others; we don't yet. // * Web ranks matches starting at a word boundary ahead of // other non-prefix matches; we don't yet. + // * Relatedly, web favors popular emoji only upon a word-aligned match. // * Web ranks each name of a Unicode emoji separately. // // Behavior differences that web should probably fix, TODO(web): + // * Among popular emoji with non-exact matches, + // web doesn't prioritize prefix over word-aligned; we do. + // (This affects just one case: for query "o", + // we put :octopus: before :working_on_it:.) + // * Web only counts an emoji as "popular" for ranking if the query + // is a prefix of a single word in the name; so "thumbs_" or "working_on_i" + // lose the ranking boost for :thumbs_up: and :working_on_it: respectively. // * Web starts with only case-sensitive exact matches ("perfect matches"), // and puts case-insensitive exact matches just ahead of prefix matches; // it also distinguishes prefix matches by case-sensitive vs. not. @@ -526,15 +545,24 @@ class EmojiAutocompleteQuery extends ComposeAutocompleteQuery { // because emoji with the same name will mostly both match or both not; // but it breaks if the Unicode emoji was a literal match. + final isPopular = EmojiStoreImpl._isPopularEmoji(candidate); + final isCustomEmoji = switch (candidate.emojiType) { + // The web implementation calls this condition `is_realm_emoji`, + // but its actual semantics is it's true for the Zulip extra emoji too. + // See `zulip_emoji` in web:src/emoji.ts . + ReactionType.realmEmoji || ReactionType.zulipExtraEmoji => true, + ReactionType.unicodeEmoji => false, + }; return switch (matchQuality) { EmojiMatchQuality.exact => 0, - EmojiMatchQuality.prefix => 1, - EmojiMatchQuality.other => 2, + EmojiMatchQuality.prefix => isPopular ? 1 : isCustomEmoji ? 3 : 4, + // TODO word-boundary vs. not + EmojiMatchQuality.other => isPopular ? 2 : isCustomEmoji ? 5 : 6, }; } /// The number of possible values returned by [_rankResult]. - static const _numResultRanks = 3; + static const _numResultRanks = 7; @override String toString() { diff --git a/test/model/emoji_test.dart b/test/model/emoji_test.dart index c14d4dfdf9..aed566c1c1 100644 --- a/test/model/emoji_test.dart +++ b/test/model/emoji_test.dart @@ -282,9 +282,9 @@ void main() { await Future(() {}); check(done).isTrue(); check(view.results).deepEquals([ - isUnicodeResult(names: ['bookmark']), isRealmResult(emojiName: 'happy'), isZulipResult(), + isUnicodeResult(names: ['bookmark']), ]); }); @@ -324,15 +324,17 @@ void main() { } test('results end-to-end', () async { + // (See more detailed rank tests below, on EmojiAutocompleteQuery.) + final unicodeEmoji = { '1f4d3': ['notebook'], '1f516': ['bookmark'], '1f4d6': ['book']}; // Empty query -> base ordering. check(await resultsOf('', unicodeEmoji: unicodeEmoji)).deepEquals([ + isZulipResult(), isUnicodeResult(names: ['notebook']), isUnicodeResult(names: ['bookmark']), isUnicodeResult(names: ['book']), - isZulipResult(), ]); // With query, exact match precedes prefix match precedes other. @@ -505,17 +507,56 @@ void main() { check(rankOf(query, a)!).isLessThan(rankOf(query, b)!); } + void checkSameRank(String query, EmojiCandidate a, EmojiCandidate b) { + check(rankOf(query, a)!).equals(rankOf(query, b)!); + } + + final octopus = unicode(['octopus'], emojiCode: '1f419'); + final workingOnIt = unicode(['working_on_it'], emojiCode: '1f6e0'); + test('ranks exact before prefix before other match', () { checkPrecedes('o', unicode(['o']), unicode(['onion'])); checkPrecedes('o', unicode(['onion']), unicode(['book'])); }); + test('ranks popular before realm before other Unicode', () { + checkPrecedes('o', octopus, realmCandidate('open_book')); + checkPrecedes('o', realmCandidate('open_book'), unicode(['ok'])); + }); + + test('ranks Zulip extra emoji same as realm emoji', () { + checkSameRank('z', zulipCandidate(), realmCandidate('zounds')); + }); + + test('ranks exact-vs-not more significant than popular/custom/other', () { + // Generic Unicode exact beats popular prefix… + checkPrecedes('o', unicode(['o']), octopus); + // … which really does count as popular, beating realm prefix. + checkPrecedes('o', octopus, realmCandidate('open_book')); + }); + + test('ranks popular-vs-not more significant than prefix/other', () { + // Popular other beats realm prefix. + checkPrecedes('o', workingOnIt, realmCandidate('open_book')); + }); + + test('ranks prefix/other more significant than custom/other', () { + // Generic Unicode prefix beats realm other. + checkPrecedes('o', unicode(['ok']), realmCandidate('yo')); + }); + test('full list of ranks', () { check([ - rankOf('o', unicode(['o'])), // exact - rankOf('o', unicode(['onion'])), // prefix - rankOf('o', unicode(['book'])), // other - ]).deepEquals([0, 1, 2]); + rankOf('o', unicode(['o'])), // exact (generic) + rankOf('o', octopus), // prefix popular + rankOf('o', workingOnIt), // other popular + rankOf('o', realmCandidate('open_book')), // prefix realm + rankOf('z', zulipCandidate()), // == prefix :zulip: + rankOf('o', unicode(['ok'])), // prefix generic + rankOf('o', realmCandidate('yo')), // other realm + rankOf('p', zulipCandidate()), // == other :zulip: + rankOf('o', unicode(['book'])), // other generic + ]).deepEquals([0, 1, 2, 3, 3, 4, 5, 5, 6]); }); }); } From a885520b4f3e555ee5c5caac86f46df042e52e36 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Sat, 7 Dec 2024 01:09:03 -0800 Subject: [PATCH 17/18] emoji: Recognize word-aligned matches in ranking Fixes #1068. --- lib/model/emoji.dart | 50 +++++++++++++++--------------- test/model/emoji_test.dart | 62 ++++++++++++++++++++++++++++---------- 2 files changed, 70 insertions(+), 42 deletions(-) diff --git a/lib/model/emoji.dart b/lib/model/emoji.dart index e1b1360676..9f8f78cb8f 100644 --- a/lib/model/emoji.dart +++ b/lib/model/emoji.dart @@ -379,7 +379,15 @@ enum EmojiMatchQuality { /// The query matches a prefix of the emoji name, but not the whole name. prefix, - /// The query matches somewhere in the emoji name, but not at the start. + /// The query matches starting at the start of a word in the emoji name, + /// but not the start of the whole name. + /// + /// For example a name "ab_cd_ef" would match queries "c" or "cd_e" + /// at this level, but not a query "b_cd_ef". + wordAligned, + + /// The query matches somewhere in the emoji name, + /// but not at the start of any word. other; /// The best possible quality of match. @@ -490,25 +498,17 @@ class EmojiAutocompleteQuery extends ComposeAutocompleteQuery { // See also commentary in [_rankResult]. // TODO(#1067) this assumes emojiName is already lower-case (and no diacritics) - if (emojiName == _adjusted) return EmojiMatchQuality.exact; - if (emojiName.startsWith(_adjusted)) return EmojiMatchQuality.prefix; - if (_nameMatches(emojiName)) return EmojiMatchQuality.other; - return null; - } - - bool _nameMatches(String emojiName) { + if (emojiName == _adjusted) return EmojiMatchQuality.exact; + if (emojiName.startsWith(_adjusted)) return EmojiMatchQuality.prefix; + if (emojiName.contains(_sepAdjusted)) return EmojiMatchQuality.wordAligned; if (!_adjusted.contains(_separator)) { // If the query is a single token (doesn't contain a separator), - // the match can be anywhere in the string. - return emojiName.contains(_adjusted); + // allow a match anywhere in the string, too. + if (emojiName.contains(_adjusted)) return EmojiMatchQuality.other; + } else { + // Otherwise, require at least a word-aligned match. } - - // If there is a separator in the query, then we - // require the match to start at the start of a token. - // (E.g. for 'ab_cd_ef', query could be 'ab_c' or 'cd_ef', - // but not 'b_cd_ef'.) - assert(!emojiName.startsWith(_adjusted)); // checked before calling this method - return emojiName.contains(_sepAdjusted); + return null; } /// A measure of the result's quality in the context of the query, @@ -521,11 +521,9 @@ class EmojiAutocompleteQuery extends ComposeAutocompleteQuery { // Compare sort_emojis in Zulip web: // https://github.com/zulip/zulip/blob/83a121c7e/web/shared/src/typeahead.ts#L322-L382 // - // Behavior differences we should or might copy, TODO(#1068): - // * Web ranks matches starting at a word boundary ahead of - // other non-prefix matches; we don't yet. - // * Relatedly, web favors popular emoji only upon a word-aligned match. + // Behavior differences we might copy, TODO: // * Web ranks each name of a Unicode emoji separately. + // * Web recognizes a word-aligned match starting after [ /-] as well as [_]. // // Behavior differences that web should probably fix, TODO(web): // * Among popular emoji with non-exact matches, @@ -554,15 +552,15 @@ class EmojiAutocompleteQuery extends ComposeAutocompleteQuery { ReactionType.unicodeEmoji => false, }; return switch (matchQuality) { - EmojiMatchQuality.exact => 0, - EmojiMatchQuality.prefix => isPopular ? 1 : isCustomEmoji ? 3 : 4, - // TODO word-boundary vs. not - EmojiMatchQuality.other => isPopular ? 2 : isCustomEmoji ? 5 : 6, + EmojiMatchQuality.exact => 0, + EmojiMatchQuality.prefix => isPopular ? 1 : isCustomEmoji ? 3 : 5, + EmojiMatchQuality.wordAligned => isPopular ? 2 : isCustomEmoji ? 4 : 6, + EmojiMatchQuality.other => isCustomEmoji ? 7 : 8, }; } /// The number of possible values returned by [_rankResult]. - static const _numResultRanks = 7; + static const _numResultRanks = 9; @override String toString() { diff --git a/test/model/emoji_test.dart b/test/model/emoji_test.dart index aed566c1c1..c099be53f3 100644 --- a/test/model/emoji_test.dart +++ b/test/model/emoji_test.dart @@ -384,7 +384,7 @@ void main() { check(matchOfName('o', 'open_book')).prefix; check(matchOfName('open', 'open_book')).prefix; check(matchOfName('pe', 'open_book')).other; - check(matchOfName('boo', 'open_book')).other; + check(matchOfName('boo', 'open_book')).wordAligned; check(matchOfName('ok', 'open_book')).other; }); @@ -396,7 +396,7 @@ void main() { check(matchOfName('pen_', 'open_book')).none; check(matchOfName('n_b', 'open_book')).none; - check(matchOfName('blue_dia', 'large_blue_diamond')).other; + check(matchOfName('blue_dia', 'large_blue_diamond')).wordAligned; }); test('spaces in query behave as underscores', () { @@ -407,7 +407,7 @@ void main() { check(matchOfName('pen ', 'open_book')).none; check(matchOfName('n b', 'open_book')).none; - check(matchOfName('blue dia', 'large_blue_diamond')).other; + check(matchOfName('blue dia', 'large_blue_diamond')).wordAligned; }); test('query is lower-cased', () { @@ -426,13 +426,17 @@ void main() { check(matchOfNames('open b', ['x', 'open_book'])).prefix; check(matchOfNames('pen_', ['x', 'open_book'])).none; + check(matchOfNames('blue_dia', ['x', 'large_blue_diamond'])).wordAligned; + check(matchOfNames('Smi', ['x', 'smile'])).prefix; }); test('best match among name and aliases prevails', () { - check(matchOfNames('a', ['ab', 'a', 'ba', 'x'])).exact; - check(matchOfNames('a', ['ba', 'ab', 'x'])).prefix; - check(matchOfNames('a', ['ba', 'ab'])).prefix; + check(matchOfNames('a', ['ab', 'a', 'b_a', 'ba', 'x'])).exact; + check(matchOfNames('a', ['ba', 'ab', 'b_a', 'x'])).prefix; + check(matchOfNames('a', ['ba', 'ab', 'b_a'])).prefix; + check(matchOfNames('a', ['ba', 'b_a', 'x'])).wordAligned; + check(matchOfNames('a', ['b_a', 'ba'])).wordAligned; check(matchOfNames('a', ['ba', 'x'])).other; check(matchOfNames('a', ['x', 'y', 'z'])).none; }); @@ -478,7 +482,7 @@ void main() { check(matchOf('eqeq', realmCandidate('eqeq'))).exact; check(matchOf('open_', realmCandidate('open_book'))).prefix; check(matchOf('n_b', realmCandidate('open_book'))).none; - check(matchOf('blue dia', realmCandidate('large_blue_diamond'))).other; + check(matchOf('blue dia', realmCandidate('large_blue_diamond'))).wordAligned; check(matchOf('Smi', realmCandidate('smile'))).prefix; }); @@ -513,10 +517,12 @@ void main() { final octopus = unicode(['octopus'], emojiCode: '1f419'); final workingOnIt = unicode(['working_on_it'], emojiCode: '1f6e0'); + final love = unicode(['love'], emojiCode: '2764'); // aka :heart: - test('ranks exact before prefix before other match', () { + test('ranks match quality exact/prefix/word-aligned/other', () { checkPrecedes('o', unicode(['o']), unicode(['onion'])); - checkPrecedes('o', unicode(['onion']), unicode(['book'])); + checkPrecedes('o', unicode(['onion']), unicode(['squared_ok'])); + checkPrecedes('o', unicode(['squared_ok']), unicode(['book'])); }); test('ranks popular before realm before other Unicode', () { @@ -535,28 +541,51 @@ void main() { checkPrecedes('o', octopus, realmCandidate('open_book')); }); - test('ranks popular-vs-not more significant than prefix/other', () { - // Popular other beats realm prefix. + test('ranks popular-vs-not more significant than prefix/word-aligned', () { + // Popular word-aligned beats realm prefix. checkPrecedes('o', workingOnIt, realmCandidate('open_book')); }); - test('ranks prefix/other more significant than custom/other', () { - // Generic Unicode prefix beats realm other. - checkPrecedes('o', unicode(['ok']), realmCandidate('yo')); + test('ranks popular as if generic when non-word-aligned', () { + // Generic word-aligned beats popular other. + checkPrecedes('o', unicode(['squared_ok']), love); + // Popular other ranks below even custom other… + checkPrecedes('o', realmCandidate('yo'), love); + // … and same as generic Unicode other. + checkSameRank('o', love, unicode(['book'])); + + // And that emoji really does count as popular, + // beating custom emoji when both have a prefix match. + checkPrecedes('l', love, realmCandidate('logs')); + }); + + test('ranks custom/other more significant than prefix/word-aligned', () { + // Custom word-aligned beats generic prefix. + checkPrecedes('o', realmCandidate('laughing_blue_octopus'), + unicode(['ok'])); + }); + + test('ranks word-aligned/other more significant than custom/other', () { + // Generic Unicode word-aligned beats realm other. + checkPrecedes('o', unicode(['squared_ok']), realmCandidate('yo')); }); test('full list of ranks', () { check([ rankOf('o', unicode(['o'])), // exact (generic) rankOf('o', octopus), // prefix popular - rankOf('o', workingOnIt), // other 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 - ]).deepEquals([0, 1, 2, 3, 3, 4, 5, 5, 6]); + rankOf('o', love), // == other popular + ]).deepEquals([0, 1, 2, 3, 3, 4, 5, 6, 7, 7, 8, 8]); }); }); } @@ -585,6 +614,7 @@ extension EmojiCandidateChecks on Subject { extension EmojiMatchQualityChecks on Subject { void get exact => equals(EmojiMatchQuality.exact); void get prefix => equals(EmojiMatchQuality.prefix); + void get wordAligned => equals(EmojiMatchQuality.wordAligned); void get other => equals(EmojiMatchQuality.other); void get none => isNull(); } From 7045afef1cc86af408c11587aee184a03b54ae0b Mon Sep 17 00:00:00 2001 From: Greg Price Date: Sat, 7 Dec 2024 15:41:00 -0800 Subject: [PATCH 18/18] emoji: Order "popular" emoji canonically amongst themselves As a bonus, this provides the popular emoji as candidates even when we haven't yet fetched the server's emoji data. --- lib/model/emoji.dart | 8 +++- test/model/emoji_test.dart | 93 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 100 insertions(+), 1 deletion(-) diff --git a/lib/model/emoji.dart b/lib/model/emoji.dart index 9f8f78cb8f..324d259351 100644 --- a/lib/model/emoji.dart +++ b/lib/model/emoji.dart @@ -295,7 +295,7 @@ class EmojiStoreImpl with EmojiStore { // but then ranks them first, after only the six "popular" emoji, // once there's a non-empty query. // * Web gives the six "popular" emoji a set order amongst themselves, - // like we do after #1112; but in web, this order appears only in the + // like we do here; but in web, this order appears only in the // emoji picker on an empty query, and is otherwise lost even when the // emoji are taken out of their home categories and shown instead // together at the front. @@ -307,12 +307,18 @@ class EmojiStoreImpl with EmojiStore { final results = []; + // Include the "popular" emoji, in their canonical order + // relative to each other. + results.addAll(_popularCandidates); + final namesOverridden = { for (final emoji in activeRealmEmoji) emoji.name, 'zulip', }; // TODO(log) if _serverEmojiData missing for (final entry in (_serverEmojiData ?? {}).entries) { + if (_popularEmojiCodes.contains(entry.key)) continue; + final allNames = entry.value; final String emojiName; final List? aliases; diff --git a/test/model/emoji_test.dart b/test/model/emoji_test.dart index c099be53f3..a55f2dc36b 100644 --- a/test/model/emoji_test.dart +++ b/test/model/emoji_test.dart @@ -78,6 +78,8 @@ void main() { }); }); + final popularCandidates = EmojiStore.popularEmojiCandidates; + Condition isUnicodeCandidate(String? emojiCode, List? names) { return (it_) { final it = it_.isA(); @@ -108,6 +110,9 @@ void main() { ..aliases.isEmpty(); } + List> arePopularCandidates = popularCandidates.map( + (c) => isUnicodeCandidate(c.emojiCode, null)).toList(); + group('allEmojiCandidates', () { // TODO test emojiDisplay of candidates matches emojiDisplayFor @@ -123,12 +128,47 @@ void main() { return store; } + test('popular emoji appear even when no server emoji data', () { + final store = prepare(unicodeEmoji: null); + check(store.allEmojiCandidates()).deepEquals([ + ...arePopularCandidates, + isZulipCandidate(), + ]); + }); + + test('popular emoji appear in their canonical order', () { + // In the server's emoji data, have the popular emoji in a permuted order, + // and interspersed with other emoji. + final store = prepare(unicodeEmoji: { + '1f603': ['smiley'], + for (final candidate in popularCandidates.skip(3)) + candidate.emojiCode: [candidate.emojiName, ...candidate.aliases], + '1f34a': ['orange', 'tangerine', 'mandarin'], + for (final candidate in popularCandidates.take(3)) + candidate.emojiCode: [candidate.emojiName, ...candidate.aliases], + '1f516': ['bookmark'], + }); + // In the allEmojiCandidates result, the popular emoji come first + // and are in their canonical order, even though the other Unicode emoji + // are in the same order they were given in. + check(store.allEmojiCandidates()).deepEquals([ + for (final candidate in popularCandidates) + isUnicodeCandidate(candidate.emojiCode, + [candidate.emojiName, ...candidate.aliases]), + isUnicodeCandidate('1f603', ['smiley']), + isUnicodeCandidate('1f34a', ['orange', 'tangerine', 'mandarin']), + isUnicodeCandidate('1f516', ['bookmark']), + isZulipCandidate(), + ]); + }); + test('realm emoji included only when active', () { final store = prepare(realmEmoji: { '1': eg.realmEmojiItem(emojiCode: '1', emojiName: 'abc', deactivated: true), '2': eg.realmEmojiItem(emojiCode: '2', emojiName: 'abcd'), }); check(store.allEmojiCandidates()).deepEquals([ + ...arePopularCandidates, isRealmCandidate(emojiCode: '2', emojiName: 'abcd'), isZulipCandidate(), ]); @@ -143,6 +183,7 @@ void main() { '5': eg.realmEmojiItem(emojiCode: '5', emojiName: 'test', deactivated: true), }); check(store.allEmojiCandidates()).deepEquals([ + ...arePopularCandidates, isRealmCandidate(emojiCode: '4', emojiName: 'try'), isZulipCandidate(), ]); @@ -156,6 +197,7 @@ void main() { '1f603': ['smiley'], }); check(store.allEmojiCandidates()).deepEquals([ + ...arePopularCandidates, isUnicodeCandidate('1f516', ['bookmark']), isRealmCandidate(emojiCode: '1', emojiName: 'smiley'), isZulipCandidate(), @@ -169,6 +211,7 @@ void main() { '1f41c': ['ant'], }); check(store.allEmojiCandidates()).deepEquals([ + ...arePopularCandidates, isUnicodeCandidate('1f41c', ['ant']), isZulipCandidate(), ]); @@ -181,6 +224,7 @@ void main() { '1f34a': ['orange', 'tangerine', 'mandarin'], }); check(store.allEmojiCandidates()).deepEquals([ + ...arePopularCandidates, isUnicodeCandidate('1f34a', ['orange', 'mandarin']), isRealmCandidate(emojiCode: '1', emojiName: 'tangerine'), isZulipCandidate(), @@ -194,6 +238,7 @@ void main() { '1f34a': ['orange', 'tangerine', 'mandarin'], }); check(store.allEmojiCandidates()).deepEquals([ + ...arePopularCandidates, isUnicodeCandidate('1f34a', ['tangerine', 'mandarin']), isRealmCandidate(emojiCode: '1', emojiName: 'orange'), isZulipCandidate(), @@ -203,6 +248,7 @@ void main() { test('updates on setServerEmojiData', () { final store = prepare(); check(store.allEmojiCandidates()).deepEquals([ + ...arePopularCandidates, isZulipCandidate(), ]); @@ -210,6 +256,7 @@ void main() { '1f516': ['bookmark'], })); check(store.allEmojiCandidates()).deepEquals([ + ...arePopularCandidates, isUnicodeCandidate('1f516', ['bookmark']), isZulipCandidate(), ]); @@ -218,6 +265,7 @@ void main() { test('updates on RealmEmojiUpdateEvent', () { final store = prepare(); check(store.allEmojiCandidates()).deepEquals([ + ...arePopularCandidates, isZulipCandidate(), ]); @@ -225,6 +273,7 @@ void main() { '1': eg.realmEmojiItem(emojiCode: '1', emojiName: 'happy'), })); check(store.allEmojiCandidates()).deepEquals([ + ...arePopularCandidates, isRealmCandidate(emojiCode: '1', emojiName: 'happy'), isZulipCandidate(), ]); @@ -257,6 +306,9 @@ void main() { isZulipCandidate()); } + List> arePopularResults = popularCandidates.map( + (c) => isUnicodeResult(emojiCode: c.emojiCode)).toList(); + PerAccountStore prepare({ Map realmEmoji = const {}, Map>? unicodeEmoji, @@ -282,6 +334,7 @@ void main() { await Future(() {}); check(done).isTrue(); check(view.results).deepEquals([ + ...arePopularResults, isRealmResult(emojiName: 'happy'), isZulipResult(), isUnicodeResult(names: ['bookmark']), @@ -323,6 +376,45 @@ void main() { return view.results; } + test('results preserve order of popular emoji within each rank', () async { + // In other words, the sorting by rank is a stable sort. + + // Full results list matches allEmojiCandidates. + check(prepare().allEmojiCandidates()) + .deepEquals([...arePopularCandidates, isZulipCandidate()]); + check(await resultsOf('')) + .deepEquals([...arePopularResults, isZulipResult()]); + + // Same list written out explicitly, for comparison with the cases below. + check(await resultsOf('')).deepEquals([ + isUnicodeResult(names: ['+1', 'thumbs_up', 'like']), + isUnicodeResult(names: ['tada']), + isUnicodeResult(names: ['smile']), + isUnicodeResult(names: ['heart', 'love', 'love_you']), + isUnicodeResult(names: ['working_on_it', 'hammer_and_wrench', 'tools']), + isUnicodeResult(names: ['octopus']), + isZulipResult(), + ]); + + check(await resultsOf('t')).deepEquals([ + // prefix + isUnicodeResult(names: ['+1', 'thumbs_up', 'like']), + isUnicodeResult(names: ['tada']), + isUnicodeResult(names: ['working_on_it', 'hammer_and_wrench', 'tools']), + // other + isUnicodeResult(names: ['heart', 'love', 'love_you']), + isUnicodeResult(names: ['octopus']), + ]); + + check(await resultsOf('h')).deepEquals([ + // prefix + isUnicodeResult(names: ['heart', 'love', 'love_you']), + isUnicodeResult(names: ['working_on_it', 'hammer_and_wrench', 'tools']), + // other + isUnicodeResult(names: ['+1', 'thumbs_up', 'like']), + ]); + }); + test('results end-to-end', () async { // (See more detailed rank tests below, on EmojiAutocompleteQuery.) @@ -331,6 +423,7 @@ void main() { // Empty query -> base ordering. check(await resultsOf('', unicodeEmoji: unicodeEmoji)).deepEquals([ + ...arePopularResults, isZulipResult(), isUnicodeResult(names: ['notebook']), isUnicodeResult(names: ['bookmark']),