Skip to content

Commit 1a40f27

Browse files
chrisbobbechrisbobbe
committed
emoji: Generate popular candidates using names from server data
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
1 parent 3d341b8 commit 1a40f27

File tree

5 files changed

+170
-88
lines changed

5 files changed

+170
-88
lines changed

‎lib/model/emoji.dart

Lines changed: 14 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -212,9 +212,9 @@ class EmojiStoreImpl extends PerAccountStoreBase with EmojiStore {
212212
/// retrieving the data.
213213
Map<String, List<String>>? _serverEmojiData;
214214

215-
static final _popularCandidates = _generatePopularCandidates();
215+
List<EmojiCandidate>? _popularCandidates;
216216

217-
static List<EmojiCandidate> _generatePopularCandidates() {
217+
List<EmojiCandidate> _generatePopularCandidates() {
218218
EmojiCandidate candidate(String emojiCode, List<String> names) {
219219
final [emojiName, ...aliases] = names;
220220
final emojiUnicode = tryParseEmojiCodeToUnicode(emojiCode);
@@ -224,25 +224,25 @@ class EmojiStoreImpl extends PerAccountStoreBase with EmojiStore {
224224
emojiDisplay: UnicodeEmojiDisplay(
225225
emojiName: emojiName, emojiUnicode: emojiUnicode!));
226226
}
227-
final list = _popularEmojiCodesList;
228-
return [
229-
candidate(list[0], ['+1', 'thumbs_up', 'like']),
230-
candidate(list[1], ['tada']),
231-
candidate(list[2], ['smile']),
232-
candidate(list[3], ['heart', 'love', 'love_you']),
233-
candidate(list[4], ['working_on_it', 'hammer_and_wrench', 'tools']),
234-
candidate(list[5], ['octopus']),
235-
];
227+
if (_serverEmojiData == null) return [];
228+
229+
final result = <EmojiCandidate>[];
230+
for (final emojiCode in _popularEmojiCodesList) {
231+
final names = _serverEmojiData![emojiCode];
232+
if (names == null) continue; // TODO(log)
233+
result.add(candidate(emojiCode, names));
234+
}
235+
return result;
236236
}
237237

238238
@override
239239
Iterable<EmojiCandidate> popularEmojiCandidates() {
240-
return _popularCandidates;
240+
return _popularCandidates ??= _generatePopularCandidates();
241241
}
242242

243243
/// Codes for the popular emoji, in order; all are Unicode emoji.
244244
// This list should match web:
245-
// https://github.com/zulip/zulip/blob/83a121c7e/web/shared/src/typeahead.ts#L22-L29
245+
// https://github.com/zulip/zulip/blob/9feba0f16/web/shared/src/typeahead.ts#L22-L29
246246
static final List<String> _popularEmojiCodesList = (() {
247247
String check(String emojiCode, String emojiUnicode) {
248248
assert(emojiUnicode == tryParseEmojiCodeToUnicode(emojiCode));
@@ -378,6 +378,7 @@ class EmojiStoreImpl extends PerAccountStoreBase with EmojiStore {
378378
@override
379379
void setServerEmojiData(ServerEmojiData data) {
380380
_serverEmojiData = data.codeToNames;
381+
_popularCandidates = null;
381382
_allEmojiCandidates = null;
382383
}
383384

‎lib/widgets/action_sheet.dart

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -554,6 +554,8 @@ void showMessageActionSheet({required BuildContext context, required Message mes
554554
final pageContext = PageRoot.contextOf(context);
555555
final store = PerAccountStoreWidget.of(pageContext);
556556

557+
final popularEmojiLoaded = store.popularEmojiCandidates().isNotEmpty;
558+
557559
// The UI that's conditioned on this won't live-update during this appearance
558560
// of the action sheet (we avoid calling composeBoxControllerOf in a build
559561
// method; see its doc).
@@ -567,7 +569,8 @@ void showMessageActionSheet({required BuildContext context, required Message mes
567569
final showMarkAsUnreadButton = markAsUnreadSupported && isMessageRead;
568570

569571
final optionButtons = [
570-
ReactionButtons(message: message, pageContext: pageContext),
572+
if (popularEmojiLoaded)
573+
ReactionButtons(message: message, pageContext: pageContext),
571574
StarButton(message: message, pageContext: pageContext),
572575
if (isComposeBoxOffered)
573576
QuoteAndReplyButton(message: message, pageContext: pageContext),
@@ -669,6 +672,12 @@ class ReactionButtons extends StatelessWidget {
669672
final popularEmojiCandidates = store.popularEmojiCandidates();
670673
assert(popularEmojiCandidates.every(
671674
(emoji) => emoji.emojiType == ReactionType.unicodeEmoji));
675+
// (if this is empty, the widget isn't built in the first place)
676+
assert(popularEmojiCandidates.isNotEmpty);
677+
// UI not designed to handle more than 6 popular emoji.
678+
// (We might have fewer if ServerEmojiData is lacking expected data,
679+
// but that looks fine in manual testing, even when there's just one.)
680+
assert(popularEmojiCandidates.length <= 6);
672681

673682
final zulipLocalizations = ZulipLocalizations.of(context);
674683
final designVariables = DesignVariables.of(context);

‎test/example_data.dart

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -151,6 +151,20 @@ ServerEmojiData serverEmojiDataPopularPlus(ServerEmojiData data) {
151151
return result;
152152
}
153153

154+
/// Like [serverEmojiDataPopular], but with the modern '1f642': ['slight_smile']
155+
/// instead of '1f642': ['smile']; see zulip/zulip@9feba0f16f.
156+
///
157+
/// zulip/zulip@9feba0f16f is a Server 11 commit.
158+
// TODO(server-11) can drop legacy data
159+
ServerEmojiData serverEmojiDataPopularModern = ServerEmojiData(codeToNames: {
160+
'1f44d': ['+1', 'thumbs_up', 'like'],
161+
'1f389': ['tada'],
162+
'1f642': ['slight_smile'],
163+
'2764': ['heart', 'love', 'love_you'],
164+
'1f6e0': ['working_on_it', 'hammer_and_wrench', 'tools'],
165+
'1f419': ['octopus'],
166+
});
167+
154168
RealmEmojiItem realmEmojiItem({
155169
required String emojiCode,
156170
required String emojiName,

‎test/model/emoji_test.dart

Lines changed: 38 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -135,14 +135,6 @@ void main() {
135135
group('allEmojiCandidates', () {
136136
// TODO test emojiDisplay of candidates matches emojiDisplayFor
137137

138-
test('popular emoji appear even when no server emoji data', () {
139-
final store = prepare(unicodeEmoji: null, addServerDataForPopular: false);
140-
check(store.allEmojiCandidates()).deepEquals([
141-
...arePopularCandidates,
142-
isZulipCandidate(),
143-
]);
144-
});
145-
146138
test('popular emoji appear in their canonical order', () {
147139
// In the server's emoji data, have the popular emoji in a permuted order,
148140
// and interspersed with other emoji.
@@ -299,6 +291,44 @@ void main() {
299291
});
300292
});
301293

294+
group('popularEmojiCandidates', () {
295+
test('memoizes result, before setServerEmojiData', () {
296+
final store = eg.store();
297+
check(store.debugServerEmojiData).isNull();
298+
final candidates = store.popularEmojiCandidates();
299+
check(store.popularEmojiCandidates())
300+
..isEmpty()..identicalTo(candidates);
301+
});
302+
303+
test('memoizes result, after setServerEmojiData', () {
304+
final store = prepare();
305+
check(store.debugServerEmojiData).isNotNull();
306+
final candidates = store.popularEmojiCandidates();
307+
check(store.popularEmojiCandidates())
308+
..isNotEmpty()..identicalTo(candidates);
309+
});
310+
311+
test('updates on first and subsequent setServerEmojiData', () {
312+
final store = eg.store();
313+
check(store.debugServerEmojiData).isNull();
314+
315+
final candidates1 = store.popularEmojiCandidates();
316+
check(candidates1).isEmpty();
317+
318+
store.setServerEmojiData(eg.serverEmojiDataPopular);
319+
final candidates2 = store.popularEmojiCandidates();
320+
check(candidates2)
321+
..isNotEmpty()
322+
..not((it) => it.identicalTo(candidates1));
323+
324+
store.setServerEmojiData(eg.serverEmojiDataPopularModern);
325+
final candidates3 = store.popularEmojiCandidates();
326+
check(candidates3)
327+
..isNotEmpty()
328+
..not((it) => it.identicalTo(candidates2));
329+
});
330+
});
331+
302332
group('EmojiAutocompleteView', () {
303333
Condition<Object?> isUnicodeResult({String? emojiCode, List<String>? names}) {
304334
return (it) => it.isA<EmojiAutocompleteResult>().candidate.which(

‎test/widgets/action_sheet_test.dart

Lines changed: 94 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,8 @@ late FakeApiConnection connection;
5252
Future<void> setupToMessageActionSheet(WidgetTester tester, {
5353
required Message message,
5454
required Narrow narrow,
55+
bool shouldSetServerEmojiData = true,
56+
bool useLegacyServerEmojiData = false,
5557
}) async {
5658
addTearDown(testBinding.reset);
5759
assert(narrow.containsMessage(message));
@@ -70,7 +72,11 @@ Future<void> setupToMessageActionSheet(WidgetTester tester, {
7072
await store.addSubscription(eg.subscription(stream));
7173
}
7274
connection = store.connection as FakeApiConnection;
73-
store.setServerEmojiData(eg.serverEmojiDataPopular);
75+
if (shouldSetServerEmojiData) {
76+
store.setServerEmojiData(useLegacyServerEmojiData
77+
? eg.serverEmojiDataPopular
78+
: eg.serverEmojiDataPopularModern);
79+
}
7480

7581
connection.prepare(json: eg.newestGetMessagesResult(
7682
foundOldest: true, messages: [message]).toJson());
@@ -811,74 +817,96 @@ void main() {
811817

812818
group('message action sheet', () {
813819
group('ReactionButtons', () {
814-
final popularCandidates =
815-
(eg.store()..setServerEmojiData(eg.serverEmojiDataPopular))
816-
.popularEmojiCandidates();
817-
818-
for (final emoji in popularCandidates) {
819-
final emojiDisplay = emoji.emojiDisplay as UnicodeEmojiDisplay;
820-
821-
Future<void> tapButton(WidgetTester tester) async {
822-
await tester.tap(find.descendant(
823-
of: find.byType(BottomSheet),
824-
matching: find.text(emojiDisplay.emojiUnicode)));
825-
}
826-
827-
testWidgets('${emoji.emojiName} adding success', (tester) async {
828-
final message = eg.streamMessage();
829-
await setupToMessageActionSheet(tester, message: message, narrow: TopicNarrow.ofMessage(message));
830-
831-
connection.prepare(json: {});
832-
await tapButton(tester);
833-
await tester.pump(Duration.zero);
834-
835-
check(connection.lastRequest).isA<http.Request>()
836-
..method.equals('POST')
837-
..url.path.equals('/api/v1/messages/${message.id}/reactions')
838-
..bodyFields.deepEquals({
839-
'reaction_type': 'unicode_emoji',
840-
'emoji_code': emoji.emojiCode,
841-
'emoji_name': emoji.emojiName,
842-
});
843-
});
844-
845-
testWidgets('${emoji.emojiName} removing success', (tester) async {
846-
final message = eg.streamMessage(
847-
reactions: [Reaction(
848-
emojiName: emoji.emojiName,
849-
emojiCode: emoji.emojiCode,
850-
reactionType: ReactionType.unicodeEmoji,
851-
userId: eg.selfAccount.userId)]
852-
);
853-
await setupToMessageActionSheet(tester, message: message, narrow: TopicNarrow.ofMessage(message));
854-
855-
connection.prepare(json: {});
856-
await tapButton(tester);
857-
await tester.pump(Duration.zero);
858-
859-
check(connection.lastRequest).isA<http.Request>()
860-
..method.equals('DELETE')
861-
..url.path.equals('/api/v1/messages/${message.id}/reactions')
862-
..bodyFields.deepEquals({
863-
'reaction_type': 'unicode_emoji',
864-
'emoji_code': emoji.emojiCode,
865-
'emoji_name': emoji.emojiName,
866-
});
867-
});
820+
testWidgets('absent if ServerEmojiData not loaded', (tester) async {
821+
final message = eg.streamMessage();
822+
await setupToMessageActionSheet(tester,
823+
message: message,
824+
narrow: TopicNarrow.ofMessage(message),
825+
shouldSetServerEmojiData: false);
826+
check(find.byType(ReactionButtons)).findsNothing();
827+
});
868828

869-
testWidgets('${emoji.emojiName} request has an error', (tester) async {
870-
final message = eg.streamMessage();
871-
await setupToMessageActionSheet(tester, message: message, narrow: TopicNarrow.ofMessage(message));
829+
for (final useLegacy in [false, true]) {
830+
final popularCandidates =
831+
(eg.store()..setServerEmojiData(
832+
useLegacy
833+
? eg.serverEmojiDataPopular
834+
: eg.serverEmojiDataPopularModern))
835+
.popularEmojiCandidates();
836+
for (final emoji in popularCandidates) {
837+
final emojiDisplay = emoji.emojiDisplay as UnicodeEmojiDisplay;
838+
839+
Future<void> tapButton(WidgetTester tester) async {
840+
await tester.tap(find.descendant(
841+
of: find.byType(BottomSheet),
842+
matching: find.text(emojiDisplay.emojiUnicode)));
843+
}
844+
845+
testWidgets('${emoji.emojiName} adding success; useLegacy: $useLegacy', (tester) async {
846+
final message = eg.streamMessage();
847+
await setupToMessageActionSheet(tester,
848+
message: message,
849+
narrow: TopicNarrow.ofMessage(message),
850+
useLegacyServerEmojiData: useLegacy);
851+
852+
connection.prepare(json: {});
853+
await tapButton(tester);
854+
await tester.pump(Duration.zero);
855+
856+
check(connection.lastRequest).isA<http.Request>()
857+
..method.equals('POST')
858+
..url.path.equals('/api/v1/messages/${message.id}/reactions')
859+
..bodyFields.deepEquals({
860+
'reaction_type': 'unicode_emoji',
861+
'emoji_code': emoji.emojiCode,
862+
'emoji_name': emoji.emojiName,
863+
});
864+
});
872865

873-
connection.prepare(
874-
apiException: eg.apiBadRequest(message: 'Invalid message(s)'));
875-
await tapButton(tester);
876-
await tester.pump(Duration.zero); // error arrives; error dialog shows
866+
testWidgets('${emoji.emojiName} removing success; useLegacy: $useLegacy', (tester) async {
867+
final message = eg.streamMessage(
868+
reactions: [Reaction(
869+
emojiName: emoji.emojiName,
870+
emojiCode: emoji.emojiCode,
871+
reactionType: ReactionType.unicodeEmoji,
872+
userId: eg.selfAccount.userId)]
873+
);
874+
await setupToMessageActionSheet(tester,
875+
message: message,
876+
narrow: TopicNarrow.ofMessage(message),
877+
useLegacyServerEmojiData: useLegacy);
878+
879+
connection.prepare(json: {});
880+
await tapButton(tester);
881+
await tester.pump(Duration.zero);
882+
883+
check(connection.lastRequest).isA<http.Request>()
884+
..method.equals('DELETE')
885+
..url.path.equals('/api/v1/messages/${message.id}/reactions')
886+
..bodyFields.deepEquals({
887+
'reaction_type': 'unicode_emoji',
888+
'emoji_code': emoji.emojiCode,
889+
'emoji_name': emoji.emojiName,
890+
});
891+
});
877892

878-
await tester.tap(find.byWidget(checkErrorDialog(tester,
879-
expectedTitle: 'Adding reaction failed',
880-
expectedMessage: 'Invalid message(s)')));
881-
});
893+
testWidgets('${emoji.emojiName} request has an error; useLegacy: $useLegacy', (tester) async {
894+
final message = eg.streamMessage();
895+
await setupToMessageActionSheet(tester,
896+
message: message,
897+
narrow: TopicNarrow.ofMessage(message),
898+
useLegacyServerEmojiData: useLegacy);
899+
900+
connection.prepare(
901+
apiException: eg.apiBadRequest(message: 'Invalid message(s)'));
902+
await tapButton(tester);
903+
await tester.pump(Duration.zero); // error arrives; error dialog shows
904+
905+
await tester.tap(find.byWidget(checkErrorDialog(tester,
906+
expectedTitle: 'Adding reaction failed',
907+
expectedMessage: 'Invalid message(s)')));
908+
});
909+
}
882910
}
883911
});
884912

0 commit comments

Comments
 (0)