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/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..07591abd34 100644 --- a/lib/model/emoji.dart +++ b/lib/model/emoji.dart @@ -86,14 +86,22 @@ 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. 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, @@ -117,14 +125,25 @@ 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; - @override - Map realmEmoji; + /// 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]. + /// + /// 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'; @@ -142,7 +161,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( @@ -203,10 +222,43 @@ 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 = { - for (final emoji in realmEmoji.values) emoji.name, + for (final emoji in activeRealmEmoji) emoji.name, 'zulip', }; // TODO(log) if _serverEmojiData missing @@ -230,16 +282,15 @@ class EmojiStoreImpl with EmojiStore { aliases: aliases)); } - for (final entry in realmEmoji.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.) + // :zulip: overrides realm emoji; compare web's `emoji.update_emojis`. continue; } results.add(_emojiCandidateFor( emojiType: ReactionType.realmEmoji, - emojiCode: entry.key, emojiName: emojiName, + emojiCode: emoji.emojiCode, emojiName: emojiName, aliases: null)); } @@ -263,7 +314,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 05178142e3..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, @@ -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/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, 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'), 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');