emoji: Generate popular candidates using names from server data #1506
Conversation
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.
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.
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
| candidate('1f6e0', '🛠', ['working_on_it', 'hammer_and_wrench', 'tools']), | ||
| candidate('1f419', '🐙', ['octopus']), | ||
| ]; | ||
| if (_serverEmojiData == null) return []; |
There was a problem hiding this comment.
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,
| final emojiData = addServerDataForPopular | ||
| ? eg.serverEmojiDataPopularPlus(extraEmojiData) | ||
| : extraEmojiData; | ||
| store.setServerEmojiData(emojiData); | ||
| return store; | ||
| } | ||
|
|
There was a problem hiding this comment.
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.
| final result = <EmojiCandidate>[]; | ||
| for (final emojiCode in _popularEmojiCodesList) { | ||
| final names = _serverEmojiData![emojiCode]; | ||
| if (names == null) continue; |
There was a problem hiding this comment.
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?
|
|
||
| /// 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 |
There was a problem hiding this comment.
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
|
|
||
| for (final emoji in popularCandidates) { | ||
| final emojiDisplay = emoji.emojiDisplay as UnicodeEmojiDisplay; | ||
| testWidgets('${emoji.emojiName} removing success', (tester) async { |
There was a problem hiding this comment.
nit: maybe add a suffix with the value of useLegacy like the previous test
| @@ -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) | |||
There was a problem hiding this comment.
I think we can have a test checking when this should not be offered, like we do with quote-and-reply button.
The server change in zulip/zulip#34177, renaming
:smile:to:slight_smile:, broke the corresponding reaction button in themessage 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