Skip to content

Commit 34a562a

Browse files
chrisbobbegnprice
chrisbobbe
authored and
gnprice
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 e55cebd commit 34a562a

File tree

5 files changed

+163
-83
lines changed

5 files changed

+163
-83
lines changed

‎lib/model/emoji.dart

Lines changed: 14 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -212,14 +212,14 @@ 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

217217
@override
218218
Iterable<EmojiCandidate> popularEmojiCandidates() {
219-
return _popularCandidates;
219+
return _popularCandidates ??= _generatePopularCandidates();
220220
}
221221

222-
static List<EmojiCandidate> _generatePopularCandidates() {
222+
List<EmojiCandidate> _generatePopularCandidates() {
223223
EmojiCandidate candidate(String emojiCode, List<String> names) {
224224
final [emojiName, ...aliases] = names;
225225
final emojiUnicode = tryParseEmojiCodeToUnicode(emojiCode)!;
@@ -228,20 +228,20 @@ class EmojiStoreImpl extends PerAccountStoreBase with EmojiStore {
228228
emojiDisplay: UnicodeEmojiDisplay(
229229
emojiName: emojiName, emojiUnicode: emojiUnicode));
230230
}
231-
final list = _popularEmojiCodesList;
232-
return [
233-
candidate(list[0], ['+1', 'thumbs_up', 'like']),
234-
candidate(list[1], ['tada']),
235-
candidate(list[2], ['smile']),
236-
candidate(list[3], ['heart', 'love', 'love_you']),
237-
candidate(list[4], ['working_on_it', 'hammer_and_wrench', 'tools']),
238-
candidate(list[5], ['octopus']),
239-
];
231+
if (_serverEmojiData == null) return [];
232+
233+
final result = <EmojiCandidate>[];
234+
for (final emojiCode in _popularEmojiCodesList) {
235+
final names = _serverEmojiData![emojiCode];
236+
if (names == null) continue; // TODO(log)
237+
result.add(candidate(emojiCode, names));
238+
}
239+
return result;
240240
}
241241

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

‎lib/widgets/action_sheet.dart

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

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

570572
final optionButtons = [
571-
ReactionButtons(message: message, pageContext: pageContext),
573+
if (popularEmojiLoaded)
574+
ReactionButtons(message: message, pageContext: pageContext),
572575
StarButton(message: message, pageContext: pageContext),
573576
if (isComposeBoxOffered)
574577
QuoteAndReplyButton(message: message, pageContext: pageContext),
@@ -702,6 +705,12 @@ class ReactionButtons extends StatelessWidget {
702705
final popularEmojiCandidates = store.popularEmojiCandidates();
703706
assert(popularEmojiCandidates.every(
704707
(emoji) => emoji.emojiType == ReactionType.unicodeEmoji));
708+
// (if this is empty, the widget isn't built in the first place)
709+
assert(popularEmojiCandidates.isNotEmpty);
710+
// UI not designed to handle more than 6 popular emoji.
711+
// (We might have fewer if ServerEmojiData is lacking expected data,
712+
// but that looks fine in manual testing, even when there's just one.)
713+
assert(popularEmojiCandidates.length <= 6);
705714

706715
final zulipLocalizations = ZulipLocalizations.of(context);
707716
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 & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -137,15 +137,6 @@ void main() {
137137
group('allEmojiCandidates', () {
138138
// TODO test emojiDisplay of candidates matches emojiDisplayFor
139139

140-
test('popular emoji appear even when no server emoji data', () {
141-
final store = prepare(unicodeEmoji: null, addServerDataForPopular: false);
142-
check(store.debugServerEmojiData).isNull();
143-
check(store.allEmojiCandidates()).deepEquals([
144-
...arePopularCandidates,
145-
isZulipCandidate(),
146-
]);
147-
});
148-
149140
test('popular emoji appear in their canonical order', () {
150141
// In the server's emoji data, have the popular emoji in a permuted order,
151142
// and interspersed with other emoji.
@@ -260,7 +251,6 @@ void main() {
260251
final store = prepare(unicodeEmoji: null, addServerDataForPopular: false);
261252
check(store.debugServerEmojiData).isNull();
262253
check(store.allEmojiCandidates()).deepEquals([
263-
...arePopularCandidates,
264254
isZulipCandidate(),
265255
]);
266256

@@ -303,6 +293,44 @@ void main() {
303293
});
304294
});
305295

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

‎test/widgets/action_sheet_test.dart

Lines changed: 87 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,8 @@ Future<void> setupToMessageActionSheet(WidgetTester tester, {
5555
required Narrow narrow,
5656
bool? realmAllowMessageEditing,
5757
int? realmMessageContentEditLimitSeconds,
58+
bool shouldSetServerEmojiData = true,
59+
bool useLegacyServerEmojiData = false,
5860
}) async {
5961
addTearDown(testBinding.reset);
6062
assert(narrow.containsMessage(message));
@@ -78,7 +80,11 @@ Future<void> setupToMessageActionSheet(WidgetTester tester, {
7880
await store.addSubscription(eg.subscription(stream));
7981
}
8082
connection = store.connection as FakeApiConnection;
81-
store.setServerEmojiData(eg.serverEmojiDataPopular);
83+
if (shouldSetServerEmojiData) {
84+
store.setServerEmojiData(useLegacyServerEmojiData
85+
? eg.serverEmojiDataPopular
86+
: eg.serverEmojiDataPopularModern);
87+
}
8288

8389
connection.prepare(json: eg.newestGetMessagesResult(
8490
foundOldest: true, messages: [message]).toJson());
@@ -830,74 +836,96 @@ void main() {
830836

831837
group('message action sheet', () {
832838
group('ReactionButtons', () {
833-
final popularCandidates =
834-
(eg.store()..setServerEmojiData(eg.serverEmojiDataPopular))
835-
.popularEmojiCandidates();
836-
837-
for (final emoji in popularCandidates) {
838-
final emojiDisplay = emoji.emojiDisplay as UnicodeEmojiDisplay;
839+
testWidgets('absent if ServerEmojiData not loaded', (tester) async {
840+
final message = eg.streamMessage();
841+
await setupToMessageActionSheet(tester,
842+
message: message,
843+
narrow: TopicNarrow.ofMessage(message),
844+
shouldSetServerEmojiData: false);
845+
check(find.byType(ReactionButtons)).findsNothing();
846+
});
839847

840-
Future<void> tapButton(WidgetTester tester) async {
841-
await tester.tap(find.descendant(
842-
of: find.byType(BottomSheet),
843-
matching: find.text(emojiDisplay.emojiUnicode)));
844-
}
848+
for (final useLegacy in [false, true]) {
849+
final popularCandidates =
850+
(eg.store()..setServerEmojiData(
851+
useLegacy
852+
? eg.serverEmojiDataPopular
853+
: eg.serverEmojiDataPopularModern))
854+
.popularEmojiCandidates();
855+
for (final emoji in popularCandidates) {
856+
final emojiDisplay = emoji.emojiDisplay as UnicodeEmojiDisplay;
857+
858+
Future<void> tapButton(WidgetTester tester) async {
859+
await tester.tap(find.descendant(
860+
of: find.byType(BottomSheet),
861+
matching: find.text(emojiDisplay.emojiUnicode)));
862+
}
845863

846-
testWidgets('${emoji.emojiName} adding success', (tester) async {
847-
final message = eg.streamMessage();
848-
await setupToMessageActionSheet(tester, message: message, narrow: TopicNarrow.ofMessage(message));
864+
testWidgets('${emoji.emojiName} adding success; useLegacy: $useLegacy', (tester) async {
865+
final message = eg.streamMessage();
866+
await setupToMessageActionSheet(tester,
867+
message: message,
868+
narrow: TopicNarrow.ofMessage(message),
869+
useLegacyServerEmojiData: useLegacy);
849870

850-
connection.prepare(json: {});
851-
await tapButton(tester);
852-
await tester.pump(Duration.zero);
871+
connection.prepare(json: {});
872+
await tapButton(tester);
873+
await tester.pump(Duration.zero);
853874

854-
check(connection.lastRequest).isA<http.Request>()
855-
..method.equals('POST')
856-
..url.path.equals('/api/v1/messages/${message.id}/reactions')
857-
..bodyFields.deepEquals({
858-
'reaction_type': 'unicode_emoji',
859-
'emoji_code': emoji.emojiCode,
860-
'emoji_name': emoji.emojiName,
861-
});
862-
});
875+
check(connection.lastRequest).isA<http.Request>()
876+
..method.equals('POST')
877+
..url.path.equals('/api/v1/messages/${message.id}/reactions')
878+
..bodyFields.deepEquals({
879+
'reaction_type': 'unicode_emoji',
880+
'emoji_code': emoji.emojiCode,
881+
'emoji_name': emoji.emojiName,
882+
});
883+
});
863884

864-
testWidgets('${emoji.emojiName} removing success', (tester) async {
865-
final message = eg.streamMessage(
866-
reactions: [Reaction(
867-
emojiName: emoji.emojiName,
868-
emojiCode: emoji.emojiCode,
869-
reactionType: ReactionType.unicodeEmoji,
870-
userId: eg.selfAccount.userId)]
871-
);
872-
await setupToMessageActionSheet(tester, message: message, narrow: TopicNarrow.ofMessage(message));
885+
testWidgets('${emoji.emojiName} removing success; useLegacy: $useLegacy', (tester) async {
886+
final message = eg.streamMessage(
887+
reactions: [Reaction(
888+
emojiName: emoji.emojiName,
889+
emojiCode: emoji.emojiCode,
890+
reactionType: ReactionType.unicodeEmoji,
891+
userId: eg.selfAccount.userId)]
892+
);
893+
await setupToMessageActionSheet(tester,
894+
message: message,
895+
narrow: TopicNarrow.ofMessage(message),
896+
useLegacyServerEmojiData: useLegacy);
873897

874-
connection.prepare(json: {});
875-
await tapButton(tester);
876-
await tester.pump(Duration.zero);
898+
connection.prepare(json: {});
899+
await tapButton(tester);
900+
await tester.pump(Duration.zero);
877901

878-
check(connection.lastRequest).isA<http.Request>()
879-
..method.equals('DELETE')
880-
..url.path.equals('/api/v1/messages/${message.id}/reactions')
881-
..bodyFields.deepEquals({
882-
'reaction_type': 'unicode_emoji',
883-
'emoji_code': emoji.emojiCode,
884-
'emoji_name': emoji.emojiName,
885-
});
886-
});
902+
check(connection.lastRequest).isA<http.Request>()
903+
..method.equals('DELETE')
904+
..url.path.equals('/api/v1/messages/${message.id}/reactions')
905+
..bodyFields.deepEquals({
906+
'reaction_type': 'unicode_emoji',
907+
'emoji_code': emoji.emojiCode,
908+
'emoji_name': emoji.emojiName,
909+
});
910+
});
887911

888-
testWidgets('${emoji.emojiName} request has an error', (tester) async {
889-
final message = eg.streamMessage();
890-
await setupToMessageActionSheet(tester, message: message, narrow: TopicNarrow.ofMessage(message));
912+
testWidgets('${emoji.emojiName} request has an error; useLegacy: $useLegacy', (tester) async {
913+
final message = eg.streamMessage();
914+
await setupToMessageActionSheet(tester,
915+
message: message,
916+
narrow: TopicNarrow.ofMessage(message),
917+
useLegacyServerEmojiData: useLegacy);
891918

892-
connection.prepare(
893-
apiException: eg.apiBadRequest(message: 'Invalid message(s)'));
894-
await tapButton(tester);
895-
await tester.pump(Duration.zero); // error arrives; error dialog shows
919+
connection.prepare(
920+
apiException: eg.apiBadRequest(message: 'Invalid message(s)'));
921+
await tapButton(tester);
922+
await tester.pump(Duration.zero); // error arrives; error dialog shows
896923

897-
await tester.tap(find.byWidget(checkErrorDialog(tester,
898-
expectedTitle: 'Adding reaction failed',
899-
expectedMessage: 'Invalid message(s)')));
900-
});
924+
await tester.tap(find.byWidget(checkErrorDialog(tester,
925+
expectedTitle: 'Adding reaction failed',
926+
expectedMessage: 'Invalid message(s)')));
927+
});
928+
}
901929
}
902930
});
903931

0 commit comments

Comments
 (0)