From 0292bdf4642e891cdf7be00aab94b883f8076b66 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Sat, 7 Dec 2024 21:43:45 -0800 Subject: [PATCH 1/6] emoji: Add toString on EmojiCandidate and EmojiAutocompleteResult Very useful when these objects appear in test failures. --- lib/model/autocomplete.dart | 5 +++++ lib/model/emoji.dart | 12 ++++++++++++ 2 files changed, 17 insertions(+) diff --git a/lib/model/autocomplete.dart b/lib/model/autocomplete.dart index 6007dd6730..a6285a6ab3 100644 --- a/lib/model/autocomplete.dart +++ b/lib/model/autocomplete.dart @@ -761,6 +761,11 @@ class EmojiAutocompleteResult extends ComposeAutocompleteResult { EmojiAutocompleteResult(this.candidate); final EmojiCandidate candidate; + + @override + String toString() { + return 'EmojiAutocompleteResult(${candidate.description()})'; + } } /// A result from an @-mention autocomplete interaction, diff --git a/lib/model/emoji.dart b/lib/model/emoji.dart index d3fcaad67f..2b363eb983 100644 --- a/lib/model/emoji.dart +++ b/lib/model/emoji.dart @@ -86,6 +86,18 @@ final class EmojiCandidate { required List? aliases, required this.emojiDisplay, }) : _aliases = aliases; + + /// Used for implementing [toString] and [EmojiAutocompleteResult.toString]. + String description() { + final typeLabel = emojiType.name.replaceFirst(RegExp(r'Emoji$'), ''); + return '$typeLabel $emojiCode $emojiName' + '${aliases.isNotEmpty ? ' $aliases' : ''}'; + } + + @override + String toString() { + return 'EmojiCandidate(${description()})'; + } } /// The portion of [PerAccountStore] describing what emoji exist. From 2c276b8cb61741731bb0c0b26f66202e43946ec1 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Sun, 8 Dec 2024 15:55:39 -0800 Subject: [PATCH 2/6] emoji [nfc]: Make realmEmoji internal to EmojiStoreImpl It's a bit of a trap for the unwary (... like me last month, in ecd2cb5d4, causing #1113), because it includes deactivated realm emoji as well as active ones. --- lib/model/emoji.dart | 7 ++----- lib/model/store.dart | 3 --- test/model/store_checks.dart | 1 - 3 files changed, 2 insertions(+), 9 deletions(-) diff --git a/lib/model/emoji.dart b/lib/model/emoji.dart index 2b363eb983..05a6016fc1 100644 --- a/lib/model/emoji.dart +++ b/lib/model/emoji.dart @@ -102,10 +102,6 @@ final class EmojiCandidate { /// The portion of [PerAccountStore] describing what emoji exist. mixin EmojiStore { - /// The realm's custom emoji (for [ReactionType.realmEmoji], - /// indexed by [Reaction.emojiCode]. - Map get realmEmoji; - EmojiDisplay emojiDisplayFor({ required ReactionType emojiType, required String emojiCode, @@ -135,7 +131,8 @@ class EmojiStoreImpl with EmojiStore { /// The same as [PerAccountStore.realmUrl]. final Uri realmUrl; - @override + /// The realm's custom emoji (for [ReactionType.realmEmoji], + /// indexed by [Reaction.emojiCode]. Map realmEmoji; /// The realm-relative URL of the unique "Zulip extra emoji", :zulip:. diff --git a/lib/model/store.dart b/lib/model/store.dart index 05178142e3..2cc261129c 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -386,9 +386,6 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, ChannelStore, Mess //////////////////////////////// // The realm's repertoire of available emoji. - @override - Map get realmEmoji => _emoji.realmEmoji; - @override EmojiDisplay emojiDisplayFor({ required ReactionType emojiType, diff --git a/test/model/store_checks.dart b/test/model/store_checks.dart index e43f98fb99..3495aa0359 100644 --- a/test/model/store_checks.dart +++ b/test/model/store_checks.dart @@ -33,7 +33,6 @@ extension PerAccountStoreChecks on Subject { Subject get zulipVersion => has((x) => x.zulipVersion, 'zulipVersion'); Subject get maxFileUploadSizeMib => has((x) => x.maxFileUploadSizeMib, 'maxFileUploadSizeMib'); Subject> get realmDefaultExternalAccounts => has((x) => x.realmDefaultExternalAccounts, 'realmDefaultExternalAccounts'); - Subject> get realmEmoji => has((x) => x.realmEmoji, 'realmEmoji'); Subject> get customProfileFields => has((x) => x.customProfileFields, 'customProfileFields'); Subject get accountId => has((x) => x.accountId, 'accountId'); Subject get account => has((x) => x.account, 'account'); From 7be2637a80758e84f6457b7d463b89c2951f62c5 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Sun, 8 Dec 2024 16:02:13 -0800 Subject: [PATCH 3/6] emoji [nfc]: Clarify that deactivated realm emoji are present --- lib/model/emoji.dart | 19 +++++++++++-------- lib/model/store.dart | 2 +- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/lib/model/emoji.dart b/lib/model/emoji.dart index 05a6016fc1..e4d52cbedc 100644 --- a/lib/model/emoji.dart +++ b/lib/model/emoji.dart @@ -125,15 +125,18 @@ mixin EmojiStore { class EmojiStoreImpl with EmojiStore { EmojiStoreImpl({ required this.realmUrl, - required this.realmEmoji, + required this.allRealmEmoji, }) : _serverEmojiData = null; // TODO(#974) maybe start from a hard-coded baseline /// The same as [PerAccountStore.realmUrl]. final Uri realmUrl; - /// The realm's custom emoji (for [ReactionType.realmEmoji], - /// indexed by [Reaction.emojiCode]. - Map realmEmoji; + /// The realm's custom emoji, indexed by [Reaction.emojiCode], + /// including deactivated emoji not available for new uses. + /// + /// These are the emoji that can have [ReactionType.realmEmoji]. + // TODO(#1113) limit to active realm emoji where appropriate + Map allRealmEmoji; /// The realm-relative URL of the unique "Zulip extra emoji", :zulip:. static const kZulipEmojiUrl = '/static/generated/emoji/images/emoji/unicode/zulip.png'; @@ -151,7 +154,7 @@ class EmojiStoreImpl with EmojiStore { return UnicodeEmojiDisplay(emojiName: emojiName, emojiUnicode: parsed); case ReactionType.realmEmoji: - final item = realmEmoji[emojiCode]; + final item = allRealmEmoji[emojiCode]; if (item == null) break; // TODO we don't check emojiName matches the known realm emoji; is that right? return _tryImageEmojiDisplay( @@ -215,7 +218,7 @@ class EmojiStoreImpl with EmojiStore { final results = []; final namesOverridden = { - for (final emoji in realmEmoji.values) emoji.name, + for (final emoji in allRealmEmoji.values) emoji.name, 'zulip', }; // TODO(log) if _serverEmojiData missing @@ -239,7 +242,7 @@ class EmojiStoreImpl with EmojiStore { aliases: aliases)); } - for (final entry in realmEmoji.entries) { + for (final entry in allRealmEmoji.entries) { final emojiName = entry.value.name; if (emojiName == 'zulip') { // TODO does 'zulip' really override realm emoji? @@ -272,7 +275,7 @@ class EmojiStoreImpl with EmojiStore { } void handleRealmEmojiUpdateEvent(RealmEmojiUpdateEvent event) { - realmEmoji = event.realmEmoji; + allRealmEmoji = event.realmEmoji; _allEmojiCandidates = null; } } diff --git a/lib/model/store.dart b/lib/model/store.dart index 2cc261129c..3a3b59bea0 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -274,7 +274,7 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, ChannelStore, Mess customProfileFields: _sortCustomProfileFields(initialSnapshot.customProfileFields), emailAddressVisibility: initialSnapshot.emailAddressVisibility, emoji: EmojiStoreImpl( - realmUrl: realmUrl, realmEmoji: initialSnapshot.realmEmoji), + realmUrl: realmUrl, allRealmEmoji: initialSnapshot.realmEmoji), accountId: accountId, selfUserId: account.userId, userSettings: initialSnapshot.userSettings, From e564806489258b6f551d2b215a4b3bc17f8cc8d1 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Sun, 8 Dec 2024 16:08:55 -0800 Subject: [PATCH 4/6] api [nfc]: Rename RealmEmojiItem.id to emojiCode This way the binding encapsulates an important fact about this bit of the API -- namely that these values are the same as the "emoji code" values seen elsewhere in the API -- so that we don't have to re-verify it when working elsewhere in the codebase and using this class. Also edit a doc comment in the model code to take advantage of that understanding. --- lib/api/model/model.dart | 6 +++--- lib/api/model/model.g.dart | 4 ++-- lib/model/emoji.dart | 2 +- test/example_data.dart | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/lib/api/model/model.dart b/lib/api/model/model.dart index a5040010df..d009bb9f29 100644 --- a/lib/api/model/model.dart +++ b/lib/api/model/model.dart @@ -115,7 +115,8 @@ class CustomProfileFieldExternalAccountData { /// in . @JsonSerializable(fieldRename: FieldRename.snake) class RealmEmojiItem { - final String id; + @JsonKey(name: 'id') + final String emojiCode; final String name; final String sourceUrl; final String? stillUrl; @@ -123,7 +124,7 @@ class RealmEmojiItem { final int? authorId; RealmEmojiItem({ - required this.id, + required this.emojiCode, required this.name, required this.sourceUrl, required this.stillUrl, @@ -137,7 +138,6 @@ class RealmEmojiItem { Map toJson() => _$RealmEmojiItemToJson(this); } - /// The name of a user setting that has a property in [UserSettings]. /// /// In Zulip event-handling code (for [UserSettingsUpdateEvent]), diff --git a/lib/api/model/model.g.dart b/lib/api/model/model.g.dart index e128c78b18..3f9e84d042 100644 --- a/lib/api/model/model.g.dart +++ b/lib/api/model/model.g.dart @@ -72,7 +72,7 @@ Map _$CustomProfileFieldExternalAccountDataToJson( RealmEmojiItem _$RealmEmojiItemFromJson(Map json) => RealmEmojiItem( - id: json['id'] as String, + emojiCode: json['id'] as String, name: json['name'] as String, sourceUrl: json['source_url'] as String, stillUrl: json['still_url'] as String?, @@ -82,7 +82,7 @@ RealmEmojiItem _$RealmEmojiItemFromJson(Map json) => Map _$RealmEmojiItemToJson(RealmEmojiItem instance) => { - 'id': instance.id, + 'id': instance.emojiCode, 'name': instance.name, 'source_url': instance.sourceUrl, 'still_url': instance.stillUrl, diff --git a/lib/model/emoji.dart b/lib/model/emoji.dart index e4d52cbedc..c1270eddce 100644 --- a/lib/model/emoji.dart +++ b/lib/model/emoji.dart @@ -131,7 +131,7 @@ class EmojiStoreImpl with EmojiStore { /// The same as [PerAccountStore.realmUrl]. final Uri realmUrl; - /// The realm's custom emoji, indexed by [Reaction.emojiCode], + /// The realm's custom emoji, indexed by their [RealmEmojiItem.emojiCode], /// including deactivated emoji not available for new uses. /// /// These are the emoji that can have [ReactionType.realmEmoji]. diff --git a/test/example_data.dart b/test/example_data.dart index dee5285b06..2339727eb6 100644 --- a/test/example_data.dart +++ b/test/example_data.dart @@ -77,7 +77,7 @@ RealmEmojiItem realmEmojiItem({ }) { assert(RegExp(r'^[1-9][0-9]*$').hasMatch(emojiCode)); return RealmEmojiItem( - id: emojiCode, + emojiCode: emojiCode, name: emojiName, sourceUrl: sourceUrl ?? '/emoji/$emojiCode.png', stillUrl: stillUrl, From f09aab1a3562e7603b29276690ab574767707d92 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Sun, 8 Dec 2024 16:17:19 -0800 Subject: [PATCH 5/6] emoji: Exclude deactivated realm emoji from autocomplete Fixes #1113. --- lib/model/emoji.dart | 17 ++++++++++++----- test/model/emoji_test.dart | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 5 deletions(-) diff --git a/lib/model/emoji.dart b/lib/model/emoji.dart index c1270eddce..534e6e0bde 100644 --- a/lib/model/emoji.dart +++ b/lib/model/emoji.dart @@ -135,9 +135,16 @@ class EmojiStoreImpl with EmojiStore { /// including deactivated emoji not available for new uses. /// /// These are the emoji that can have [ReactionType.realmEmoji]. - // TODO(#1113) limit to active realm emoji where appropriate + /// + /// For emoji available to be newly used, see [activeRealmEmoji]. Map allRealmEmoji; + /// The realm's custom emoji that are available for new uses + /// in messages and reactions. + Iterable get activeRealmEmoji { + return allRealmEmoji.values.where((emoji) => !emoji.deactivated); + } + /// The realm-relative URL of the unique "Zulip extra emoji", :zulip:. static const kZulipEmojiUrl = '/static/generated/emoji/images/emoji/unicode/zulip.png'; @@ -218,7 +225,7 @@ class EmojiStoreImpl with EmojiStore { final results = []; final namesOverridden = { - for (final emoji in allRealmEmoji.values) emoji.name, + for (final emoji in activeRealmEmoji) emoji.name, 'zulip', }; // TODO(log) if _serverEmojiData missing @@ -242,8 +249,8 @@ class EmojiStoreImpl with EmojiStore { aliases: aliases)); } - for (final entry in allRealmEmoji.entries) { - final emojiName = entry.value.name; + for (final emoji in activeRealmEmoji) { + final emojiName = emoji.name; if (emojiName == 'zulip') { // TODO does 'zulip' really override realm emoji? // (This is copied from zulip-mobile's behavior.) @@ -251,7 +258,7 @@ class EmojiStoreImpl with EmojiStore { } results.add(_emojiCandidateFor( emojiType: ReactionType.realmEmoji, - emojiCode: entry.key, emojiName: emojiName, + emojiCode: emoji.emojiCode, emojiName: emojiName, aliases: null)); } diff --git a/test/model/emoji_test.dart b/test/model/emoji_test.dart index 3a0ddc5e95..953d986774 100644 --- a/test/model/emoji_test.dart +++ b/test/model/emoji_test.dart @@ -123,6 +123,31 @@ void main() { return store; } + 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([ + isRealmCandidate(emojiCode: '2', emojiName: 'abcd'), + isZulipCandidate(), + ]); + }); + + test('realm emoji tolerate name collisions', () { + final store = prepare(realmEmoji: { + '1': eg.realmEmojiItem(emojiCode: '1', emojiName: 'test', deactivated: true), + '2': eg.realmEmojiItem(emojiCode: '2', emojiName: 'try', deactivated: true), + '3': eg.realmEmojiItem(emojiCode: '3', emojiName: 'try', deactivated: true), + '4': eg.realmEmojiItem(emojiCode: '4', emojiName: 'try'), + '5': eg.realmEmojiItem(emojiCode: '5', emojiName: 'test', deactivated: true), + }); + check(store.allEmojiCandidates()).deepEquals([ + isRealmCandidate(emojiCode: '4', emojiName: 'try'), + isZulipCandidate(), + ]); + }); + test('realm emoji overrides Unicode emoji', () { final store = prepare(realmEmoji: { '1': eg.realmEmojiItem(emojiCode: '1', emojiName: 'smiley'), @@ -137,6 +162,18 @@ void main() { ]); }); + test('deactivated realm emoji cause no override of Unicode emoji', () { + final store = prepare(realmEmoji: { + '1': eg.realmEmojiItem(emojiCode: '1', emojiName: 'ant', deactivated: true), + }, unicodeEmoji: { + '1f41c': ['ant'], + }); + check(store.allEmojiCandidates()).deepEquals([ + isUnicodeCandidate('1f41c', ['ant']), + isZulipCandidate(), + ]); + }); + test('Unicode emoji with overridden aliases survives with remaining names', () { final store = prepare(realmEmoji: { '1': eg.realmEmojiItem(emojiCode: '1', emojiName: 'tangerine'), From 3da80fdf7c3e9193b0649c07376baab1c622cf79 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Sun, 8 Dec 2024 17:47:37 -0800 Subject: [PATCH 6/6] emoji [nfc]: Compare emoji base ordering with web's, in detail I spent some time reverse-engineering in full detail the behavior web has for building its list of possible emoji to present in the UI (and so for the order that emoji results have when they don't differ on any of the criteria used at the ranking step after applying a query). The first output of that study was discovering #1113, fixed earlier in this branch. This records the rest. --- lib/model/emoji.dart | 36 ++++++++++++++++++++++++++++++++++-- 1 file changed, 34 insertions(+), 2 deletions(-) diff --git a/lib/model/emoji.dart b/lib/model/emoji.dart index 534e6e0bde..07591abd34 100644 --- a/lib/model/emoji.dart +++ b/lib/model/emoji.dart @@ -222,6 +222,39 @@ class EmojiStoreImpl with EmojiStore { } List _generateAllCandidates() { + // 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. + // https://github.com/zulip/zulip/blob/0f59e2e78/web/src/emoji_picker.ts#L132-L185 + // https://github.com/zulip/zulip/blob/0f59e2e78/web/src/composebox_typeahead.ts#L138-L163 + // https://github.com/zulip/zulip/blob/0f59e2e78/web/src/emoji.ts#L232-L278 + // + // Behavior differences we might copy or change, TODO: + // * Web has a particular ordering of Unicode emoji; + // a data file groups them by category and orders within each of those, + // and the code has a list of categories. + // This seems useful; it'll call for expanding the server emoji data API. + // * Both here and in web, the realm emoji appear in whatever order the + // server returned them in; and that order appears to be random, + // presumably the iteration order of some Python dict, + // and to vary over time. + // + // Behavior differences that web should probably fix, TODO(web): + // * Web ranks the realm's custom emoji (and the Zulip extra emoji) at the + // end of the base list, as seen in the emoji picker on an empty query; + // 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 + // 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. + // + // In web on an empty query, :+1: aka :like: comes first, and + // :heart: aka :love: comes later (fourth); but then on the query "l", + // the results begin with :love: and then :like:. They've flipped order, + // even though they're equally good prefix matches to the query. + final results = []; final namesOverridden = { @@ -252,8 +285,7 @@ class EmojiStoreImpl with EmojiStore { for (final emoji in activeRealmEmoji) { final emojiName = emoji.name; if (emojiName == 'zulip') { - // TODO does 'zulip' really override realm emoji? - // (This is copied from zulip-mobile's behavior.) + // :zulip: overrides realm emoji; compare web's `emoji.update_emojis`. continue; } results.add(_emojiCandidateFor(