emoji: Generate popular candidates using names from server data

chrisbobbe · chrisbobbe · commit 38fd31d114d4 · 2025-05-08T13:27:13.000-07:00
The server change in zulip/zulip#34177, renaming `:smile:` to `:slight_smile:`, broke the corresponding reaction button in the message action sheet. We've been sending the add/remove-reaction request with the old name 'smile', which modern servers reject. To fix, take the popular emoji names from ServerEmojiData, so that we'll use the correct name on servers before and after the change. API-design discussion: https://chat.zulip.org/#narrow/channel/378-api-design/topic/.23F1495.20smile.2Fslight_smile.20change.20broke.20reaction.20button/near/2170354 Fixes: zulip#1495
diff --git a/lib/model/emoji.dart b/lib/model/emoji.dart
@@ -212,9 +212,9 @@ class EmojiStoreImpl extends PerAccountStoreBase with EmojiStore {
   /// retrieving the data.
   Map<String, List<String>>? _serverEmojiData;
 
-  static final _popularCandidates = _generatePopularCandidates();
+  List<EmojiCandidate>? _popularCandidates;
 
-  static List<EmojiCandidate> _generatePopularCandidates() {
+  List<EmojiCandidate> _generatePopularCandidates() {
     EmojiCandidate candidate(String emojiCode, List<String> names) {
       final [emojiName, ...aliases] = names;
       final emojiUnicode = tryParseEmojiCodeToUnicode(emojiCode);
@@ -224,20 +224,20 @@ class EmojiStoreImpl extends PerAccountStoreBase with EmojiStore {
         emojiDisplay: UnicodeEmojiDisplay(
           emojiName: emojiName, emojiUnicode: emojiUnicode!));
     }
-    final list = _popularEmojiCodesList;
-    return [
-      candidate(list[0], ['+1', 'thumbs_up', 'like']),
-      candidate(list[1], ['tada']),
-      candidate(list[2], ['smile']),
-      candidate(list[3], ['heart', 'love', 'love_you']),
-      candidate(list[4], ['working_on_it', 'hammer_and_wrench', 'tools']),
-      candidate(list[5], ['octopus']),
-    ];
+    if (_serverEmojiData == null) return [];
+
+    final result = <EmojiCandidate>[];
+    for (final emojiCode in _popularEmojiCodesList) {
+      final names = _serverEmojiData![emojiCode];
+      if (names == null) continue;
+      result.add(candidate(emojiCode, names));
+    }
+    return result;
   }
 
   @override
   Iterable<EmojiCandidate> popularEmojiCandidates() {
-    return _popularCandidates;
+    return _popularCandidates ??= _generatePopularCandidates();
   }
 
   /// Codes for the popular emoji, in order; all are Unicode emoji.
@@ -378,6 +378,7 @@ class EmojiStoreImpl extends PerAccountStoreBase with EmojiStore {
   @override
   void setServerEmojiData(ServerEmojiData data) {
     _serverEmojiData = data.codeToNames;
+    _popularCandidates = null;
     _allEmojiCandidates = null;
   }
 
diff --git a/lib/widgets/action_sheet.dart b/lib/widgets/action_sheet.dart
@@ -556,6 +556,8 @@ void showMessageActionSheet({required BuildContext context, required Message mes
   final pageContext = PageRoot.contextOf(context);
   final store = PerAccountStoreWidget.of(pageContext);
 
+  final popularEmojiLoaded = store.popularEmojiCandidates().isNotEmpty;
+
   // The UI that's conditioned on this won't live-update during this appearance
   // of the action sheet (we avoid calling composeBoxControllerOf in a build
   // method; see its doc).
@@ -569,7 +571,8 @@ void showMessageActionSheet({required BuildContext context, required Message mes
   final showMarkAsUnreadButton = markAsUnreadSupported && isMessageRead;
 
   final optionButtons = [
-    ReactionButtons(message: message, pageContext: pageContext),
+    if (popularEmojiLoaded)
+      ReactionButtons(message: message, pageContext: pageContext),
     StarButton(message: message, pageContext: pageContext),
     if (isComposeBoxOffered)
       QuoteAndReplyButton(message: message, pageContext: pageContext),
@@ -671,6 +674,12 @@ class ReactionButtons extends StatelessWidget {
     final popularEmojiCandidates = store.popularEmojiCandidates();
     assert(popularEmojiCandidates.every(
       (emoji) => emoji.emojiType == ReactionType.unicodeEmoji));
+    // (if this is empty, the widget isn't built in the first place)
+    assert(popularEmojiCandidates.isNotEmpty);
+    // UI not designed to handle more than 6 popular emoji.
+    // (We might have fewer if ServerEmojiData is lacking expected data,
+    // but that looks fine in manual testing, even when there's just one.)
+    assert(popularEmojiCandidates.length <= 6);
 
     final zulipLocalizations = ZulipLocalizations.of(context);
     final designVariables = DesignVariables.of(context);
diff --git a/test/example_data.dart b/test/example_data.dart
@@ -137,6 +137,20 @@ ServerEmojiData serverEmojiDataPopularPlus(ServerEmojiData data) {
   return result;
 }
 
+/// Like [serverEmojiDataPopular], but with the modern '1f642': ['slight_smile']
+/// instead of '1f642': ['smile']; see zulip/zulip@9feba0f16f.
+///
+/// zulip/zulip@9feba0f16f is a Server 11 commit.
+// TODO(server-11) can drop legacy data
+ServerEmojiData serverEmojiDataPopularModern = ServerEmojiData(codeToNames: {
+  '1f44d': ['+1', 'thumbs_up', 'like'],
+  '1f389': ['tada'],
+  '1f642': ['slight_smile'],
+  '2764': ['heart', 'love', 'love_you'],
+  '1f6e0': ['working_on_it', 'hammer_and_wrench', 'tools'],
+  '1f419': ['octopus'],
+});
+
 RealmEmojiItem realmEmojiItem({
   required String emojiCode,
   required String emojiName,
diff --git a/test/widgets/action_sheet_test.dart b/test/widgets/action_sheet_test.dart
@@ -52,6 +52,7 @@ late FakeApiConnection connection;
 Future<void> setupToMessageActionSheet(WidgetTester tester, {
   required Message message,
   required Narrow narrow,
+  bool useLegacy = false,
 }) async {
   addTearDown(testBinding.reset);
   assert(narrow.containsMessage(message));
@@ -70,7 +71,9 @@ Future<void> setupToMessageActionSheet(WidgetTester tester, {
     await store.addSubscription(eg.subscription(stream));
   }
   connection = store.connection as FakeApiConnection;
-  store.setServerEmojiData(eg.serverEmojiDataPopular);
+  store.setServerEmojiData(useLegacy
+    ? eg.serverEmojiDataPopular
+    : eg.serverEmojiDataPopularModern);
 
   connection.prepare(json: eg.newestGetMessagesResult(
     foundOldest: true, messages: [message]).toJson());
@@ -812,74 +815,87 @@ void main() {
 
   group('message action sheet', () {
     group('ReactionButtons', () {
-      final popularCandidates =
-        (eg.store()..setServerEmojiData(eg.serverEmojiDataPopular))
-          .popularEmojiCandidates();
+      for (final useLegacy in [false, true]) {
+        final popularCandidates =
+          (eg.store()..setServerEmojiData(
+            useLegacy
+              ? eg.serverEmojiDataPopular
+              : eg.serverEmojiDataPopularModern))
+            .popularEmojiCandidates();
+        for (final emoji in popularCandidates) {
+          final emojiDisplay = emoji.emojiDisplay as UnicodeEmojiDisplay;
+
+          Future<void> tapButton(WidgetTester tester) async {
+            await tester.tap(find.descendant(
+              of: find.byType(BottomSheet),
+              matching: find.text(emojiDisplay.emojiUnicode)));
+          }
+
+          testWidgets('${emoji.emojiName} adding success; useLegacy: $useLegacy', (tester) async {
+            final message = eg.streamMessage();
+            await setupToMessageActionSheet(tester,
+              message: message,
+              narrow: TopicNarrow.ofMessage(message),
+              useLegacy: useLegacy);
+
+            connection.prepare(json: {});
+            await tapButton(tester);
+            await tester.pump(Duration.zero);
+
+            check(connection.lastRequest).isA<http.Request>()
+              ..method.equals('POST')
+              ..url.path.equals('/api/v1/messages/${message.id}/reactions')
+              ..bodyFields.deepEquals({
+                  'reaction_type': 'unicode_emoji',
+                  'emoji_code': emoji.emojiCode,
+                  'emoji_name': emoji.emojiName,
+                });
+          });
 
-      for (final emoji in popularCandidates) {
-        final emojiDisplay = emoji.emojiDisplay as UnicodeEmojiDisplay;
+          testWidgets('${emoji.emojiName} removing success', (tester) async {
+            final message = eg.streamMessage(
+              reactions: [Reaction(
+                emojiName: emoji.emojiName,
+                emojiCode: emoji.emojiCode,
+                reactionType: ReactionType.unicodeEmoji,
+                userId: eg.selfAccount.userId)]
+            );
+            await setupToMessageActionSheet(tester,
+              message: message,
+              narrow: TopicNarrow.ofMessage(message),
+              useLegacy: useLegacy);
+
+            connection.prepare(json: {});
+            await tapButton(tester);
+            await tester.pump(Duration.zero);
+
+            check(connection.lastRequest).isA<http.Request>()
+              ..method.equals('DELETE')
+              ..url.path.equals('/api/v1/messages/${message.id}/reactions')
+              ..bodyFields.deepEquals({
+                  'reaction_type': 'unicode_emoji',
+                  'emoji_code': emoji.emojiCode,
+                  'emoji_name': emoji.emojiName,
+                });
+          });
 
-        Future<void> tapButton(WidgetTester tester) async {
-          await tester.tap(find.descendant(
-            of: find.byType(BottomSheet),
-            matching: find.text(emojiDisplay.emojiUnicode)));
+          testWidgets('${emoji.emojiName} request has an error', (tester) async {
+            final message = eg.streamMessage();
+            await setupToMessageActionSheet(tester,
+              message: message,
+              narrow: TopicNarrow.ofMessage(message),
+              useLegacy: useLegacy);
+
+            connection.prepare(
+              apiException: eg.apiBadRequest(message: 'Invalid message(s)'));
+            await tapButton(tester);
+            await tester.pump(Duration.zero); // error arrives; error dialog shows
+
+            await tester.tap(find.byWidget(checkErrorDialog(tester,
+              expectedTitle: 'Adding reaction failed',
+              expectedMessage: 'Invalid message(s)')));
+          });
         }
-
-        testWidgets('${emoji.emojiName} adding success', (tester) async {
-          final message = eg.streamMessage();
-          await setupToMessageActionSheet(tester, message: message, narrow: TopicNarrow.ofMessage(message));
-
-          connection.prepare(json: {});
-          await tapButton(tester);
-          await tester.pump(Duration.zero);
-
-          check(connection.lastRequest).isA<http.Request>()
-            ..method.equals('POST')
-            ..url.path.equals('/api/v1/messages/${message.id}/reactions')
-            ..bodyFields.deepEquals({
-                'reaction_type': 'unicode_emoji',
-                'emoji_code': emoji.emojiCode,
-                'emoji_name': emoji.emojiName,
-              });
-        });
-
-        testWidgets('${emoji.emojiName} removing success', (tester) async {
-          final message = eg.streamMessage(
-            reactions: [Reaction(
-              emojiName: emoji.emojiName,
-              emojiCode: emoji.emojiCode,
-              reactionType: ReactionType.unicodeEmoji,
-              userId: eg.selfAccount.userId)]
-          );
-          await setupToMessageActionSheet(tester, message: message, narrow: TopicNarrow.ofMessage(message));
-
-          connection.prepare(json: {});
-          await tapButton(tester);
-          await tester.pump(Duration.zero);
-
-          check(connection.lastRequest).isA<http.Request>()
-            ..method.equals('DELETE')
-            ..url.path.equals('/api/v1/messages/${message.id}/reactions')
-            ..bodyFields.deepEquals({
-                'reaction_type': 'unicode_emoji',
-                'emoji_code': emoji.emojiCode,
-                'emoji_name': emoji.emojiName,
-              });
-        });
-
-        testWidgets('${emoji.emojiName} request has an error', (tester) async {
-          final message = eg.streamMessage();
-          await setupToMessageActionSheet(tester, message: message, narrow: TopicNarrow.ofMessage(message));
-
-          connection.prepare(
-            apiException: eg.apiBadRequest(message: 'Invalid message(s)'));
-          await tapButton(tester);
-          await tester.pump(Duration.zero); // error arrives; error dialog shows
-
-          await tester.tap(find.byWidget(checkErrorDialog(tester,
-            expectedTitle: 'Adding reaction failed',
-            expectedMessage: 'Invalid message(s)')));
-        });
       }
     });
 
