emoji: Generate popular candidates using names from server data #1506

chrisbobbe · 2025-05-08T20:40:50Z

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: #1495

PIG208

Thanks! I went through the PR and the fix looks good to me overall. Left some comments mainly about testing.

PIG208 · 2025-05-09T21:27:39Z

lib/model/emoji.dart

-      candidate('1f6e0', '🛠', ['working_on_it', 'hammer_and_wrench', 'tools']),
-      candidate('1f419', '🐙', ['octopus']),
-    ];
+    if (_serverEmojiData == null) return [];


From what I understand, _generateAllCandidates gets called (and then _generatePopularCandidates) during the first access to store.allEmojiCandidates, and the data stays cached afterwards.

The popular emojis among them, created from _generatePopularCandidates, rely on what _serverEmojiData is at the time we access the emoji candidates. Is it possible for the user to access the picker a bit too early, such that _serverEmojiData is still null when we create the cache? If so, do we have a way to correct that?

Oh, then I saw the lines in setServerEmojiData that sets the cached candidates back to null, so that should be fine. I feel that a test for the part where we invalidate the popular emoji candidates when we do get the data should help,

PIG208 · 2025-05-09T21:43:46Z

test/model/emoji_test.dart

+      final emojiData = addServerDataForPopular
+        ? eg.serverEmojiDataPopularPlus(extraEmojiData)
+        : extraEmojiData;
+      store.setServerEmojiData(emojiData);
      return store;
    }



The test belong "popular emoji appear even when no server emoji data" might need some adjustment, since part of popular emojis now do come from server emoji data.

I think that test should just be deleted, because it's intended that the popular-emoji candidates do not appear when there is no server emoji data. Thanks for flagging!

PIG208 · 2025-05-09T21:54:44Z

lib/model/emoji.dart

+    final result = <EmojiCandidate>[];
+    for (final emojiCode in _popularEmojiCodesList) {
+      final names = _serverEmojiData![emojiCode];
+      if (names == null) continue;


nit: it seems unusual for the emoji to be ever missing from _serverEmojiData, if that data is present; do we want to log it if that happens?

PIG208 · 2025-05-09T21:56:44Z

lib/model/emoji.dart

+
+  /// Codes for the popular emoji, in order; all are Unicode emoji.
+  // This list should match web:
+  //   https://github.com/zulip/zulip/blob/83a121c7e/web/shared/src/typeahead.ts#L22-L29


We should update the link since "1f642" is now referred to as slight_smile:

https://github.com/zulip/zulip/blob/9feba0f16/web/shared/src/typeahead.ts#L22-L29

PIG208 · 2025-05-09T22:04:47Z

test/widgets/action_sheet_test.dart


-      for (final emoji in popularCandidates) {
-        final emojiDisplay = emoji.emojiDisplay as UnicodeEmojiDisplay;
+          testWidgets('${emoji.emojiName} removing success', (tester) async {


nit: maybe add a suffix with the value of useLegacy like the previous test

PIG208 · 2025-05-09T22:07:39Z

lib/widgets/action_sheet.dart

@@ -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)


I think we can have a test checking when this should not be offered, like we do with quote-and-reply button.

For zulip#1495, we'll want to keep the hard-coded list of emoji codes but get the names from ServerEmojiData. This refactor prepares for that by separating where we source the codes and the names.

Later in this series, we'll make _popularCandidates nullable, as a caching implementation detail behind popularEmojiCandidates. (Both will become non-static.)

For parallelism with allEmojiCandidates, which is similar.

…ixin

We're about to change `prepare`'s unicodeEmoji param so that it expects only non-popular emoji.

…t it This brings the tests closer to being representative, because this data (and more) is part of the app's setup for an account on startup, via fetchServerEmojiData. For zulip#1495, the app will start depending on this data for the names of popular emoji. So, prepare for that by filling in the data in tests that would break without it.

We're about to change _generatePopularCandidates so it looks up the emoji names in the ServerEmojiData, and we don't want to mutate ServerEmojiData. This isn't a hot codepath (it's called at most once per server-emoji-data fetch), so creating a new List isn't a problem.

The message action sheet is about to condition the appearance of the reactions row on whether we have ServerEmojiData for any of the popular emoji. This test-setup code taps "more" on that row to launch the emoji picker, so the data must be present by the time the action sheet opens.

We'd like to deduplicate and share this setup code.

This shorthand wasn't saving *that* much boilerplate; seems okay to remove it. The two `prepare` functions are now identical; we'll deduplicate them next.

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

chrisbobbe · 2025-05-16T00:33:30Z

Thanks for the review! Revision pushed.

PIG208 · 2025-05-16T01:11:17Z

Looks good to me. Thanks! Marking this for Greg's review.

chrisbobbe requested a review from PIG208 May 8, 2025 20:40

chrisbobbe assigned PIG208 May 8, 2025

chrisbobbe added the maintainer review PR ready for review by Zulip maintainers label May 8, 2025

PIG208 reviewed May 9, 2025

View reviewed changes

chrisbobbe added 14 commits May 15, 2025 16:21

emoji [nfc]: Move list of popular emoji codes to its own static field

bbbaa53

For zulip#1495, we'll want to keep the hard-coded list of emoji codes but get the names from ServerEmojiData. This refactor prepares for that by separating where we source the codes and the names.

emoji [nfc]: Read _popularCandidates only through public getter

5973fec

Later in this series, we'll make _popularCandidates nullable, as a caching implementation detail behind popularEmojiCandidates. (Both will become non-static.)

emoji [nfc]: Make popularEmojiCandidates non-static

ecb4f7d

emoji [nfc]: Make popularEmojiCandidates a method, not getter

a60ff3f

For parallelism with allEmojiCandidates, which is similar.

emoji [nfc]: Move popularEmojiCandidates implementation down out of m…

e8728f5

…ixin

emoji test: Change one autocomplete test to use a non-popular emoji

13292a9

We're about to change `prepare`'s unicodeEmoji param so that it expects only non-popular emoji.

emoji test [nfc]: Make a nested prepare more like another (1/2)

b8abc6b

We'd like to deduplicate and share this setup code.

emoji test [nfc]: Make a nested prepare more like another (2/2)

d8d9e58

This shorthand wasn't saving *that* much boilerplate; seems okay to remove it. The two `prepare` functions are now identical; we'll deduplicate them next.

emoji test [nfc]: Pull out prepare function for reuse

3d341b8

test: Change '1f642': 'smile' emoji to 'slight_smile' where it appears

076b689

chrisbobbe force-pushed the pr-fix-smile-emoji-button branch from f3023a2 to 076b689 Compare May 16, 2025 00:33

PIG208 added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels May 16, 2025

PIG208 assigned gnprice and unassigned PIG208 May 16, 2025

PIG208 requested a review from gnprice May 16, 2025 01:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

emoji: Generate popular candidates using names from server data #1506

emoji: Generate popular candidates using names from server data #1506

chrisbobbe commented May 8, 2025

PIG208 left a comment

PIG208 May 9, 2025

PIG208 May 9, 2025

chrisbobbe May 16, 2025 •

edited

Loading

PIG208 May 9, 2025

PIG208 May 9, 2025

PIG208 May 9, 2025

PIG208 May 9, 2025

chrisbobbe commented May 16, 2025

PIG208 commented May 16, 2025

emoji: Generate popular candidates using names from server data #1506

Are you sure you want to change the base?

emoji: Generate popular candidates using names from server data #1506

Conversation

chrisbobbe commented May 8, 2025

PIG208 left a comment

Choose a reason for hiding this comment

PIG208 May 9, 2025

Choose a reason for hiding this comment

PIG208 May 9, 2025

Choose a reason for hiding this comment

chrisbobbe May 16, 2025 • edited Loading

Choose a reason for hiding this comment

PIG208 May 9, 2025

Choose a reason for hiding this comment

PIG208 May 9, 2025

Choose a reason for hiding this comment

PIG208 May 9, 2025

Choose a reason for hiding this comment

PIG208 May 9, 2025

Choose a reason for hiding this comment

chrisbobbe commented May 16, 2025

PIG208 commented May 16, 2025

chrisbobbe May 16, 2025 •

edited

Loading