Emoji autocomplete #1069

gnprice · 2024-11-21T02:42:00Z

This adds autocomplete for emoji in the compose box, so e.g. typing :zu offers an autocomplete option of :zulip:.

The first part of the branch makes some refactors to the autocomplete subsystem, in order to prepare to accommodate emoji autocomplete alongside @-mention autocomplete. The changes are all in basically the direction we had in mind when initially building the system; mostly we left some concepts collapsed that we knew we'd eventually need to separate, deferring the details of actually separating them until we had a concrete situation where they came apart.

Fixes: #669
Fixes: #670

Selected commit messages

autocomplete [nfc]: Document how query, view-model, and results classes relate

autocomplete [nfc]: Separate ComposeAutocompleteQuery/Result from Mention-etc.

The two concepts have meant the same set of possible values so far,
because @-mentions are the only type of autocomplete we've had so far
in the compose box's content input. As a result we've taken some
shortcuts by conflating them.

But as we introduce other types of autocomplete in the content input,
like for emoji and #-mentions, we have some places that will need to
refer to the more general concept while others refer to the more
specific one. So separate them out.

emoji [nfc]: Factor out ImageEmojiWidget

emoji [nfc]: Factor out UnicodeEmojiWidget

emoji: Make list of emoji to consider for autocomplete or emoji picker

This leaves the emojiDisplay field of these objects untested. I
skipped that because it seems like pretty boring low-risk code,
just invoking emojiDisplayFor. (And emojiDisplayFor has its own
tests.) But included a TODO comment for completeness in thinking
about what logic there is to test here.

Fixes: #669

autocomplete: Identify when the user intends an emoji autocomplete

The "forbid preceding ':'" wrinkle is one I discovered because of
writing tests: I wrote down the '::^' test, was surprised to find
that it failed, and then went back and extended the regexp to
make it pass.

[…]

emoji: Finish emoji autocomplete for compose box

Fixes: #670

chrisbobbe

Thanks, this will be great to have!! Small comments below.

chrisbobbe · 2024-11-23T01:27:34Z

lib/widgets/autocomplete.dart

-            Text(label),
-          ])));
+
+    return Padding(


Just FYI that this will conflict with PR #995.

chrisbobbe · 2024-11-23T03:50:50Z

test/model/autocomplete_test.dart

    doTest('If @chris is around, please ask him.^', null); // @ sign is too far away from cursor
    doTest('If @_chris is around, please ask him.^', null); // @ sign is too far away from cursor
+
+    // Emoji (":-mentions").


Ha—I guess ":-mentions" is one way we could refer to them :)

I think it might be wiser not to, though; the @-mention feature is complicated, and readers might be happier if they don't have to worry about whether the message-emoji feature is tied up with it as well. Maybe simply

// Emoji (":smile:").

or something?

chrisbobbe · 2024-11-23T03:55:21Z

test/model/autocomplete_test.dart

+    // Basic positive examples, to contrast with all the negative examples below.
+    doTest('~:^', emoji(''));
+    doTest('~:a^', emoji('a'));
+    doTest('~:a ^', emoji('a '));
+    doTest('~:a_^', emoji('a_'));
+    doTest('ok ~:s^', emoji('s'));


How about a case like

doTest('this: ~:s^', emoji('s'));

to catch any issues with the logic that excludes ::?

Ooh how about also:

doTest('~:a b^', emoji('a b'));

because spaces are allowed in the middle of a query.

chrisbobbe · 2024-11-23T04:04:01Z

test/model/autocomplete_test.dart

+    // Avoid interpreting colons in normal prose as queries.
+    doTest(': ^', null);
+    doTest(':\n^', null);
+    doTest('this:^', null);
+    doTest('this: ^', null);
+    doTest('là ~:^', emoji('')); // ambiguous in French prose, tant pis
+    doTest('là : ^', null);


Ooh and how about times:

doTest('8:30^', null);

chrisbobbe · 2024-11-23T04:11:53Z

lib/widgets/autocomplete.dart

+
+    final label = candidate.aliases.isEmpty
+      ? candidate.emojiName
+      : [candidate.emojiName, ...candidate.aliases].join(", ");


Suggested change

: [candidate.emojiName, ...candidate.aliases].join(", ");

// TODO(i18n): List formatting, like you can do in JavaScript:

// new Intl.ListFormat('ja').format(['Chris', 'Greg', 'Alya', 'Zixuan'])

// // 'Chris、Greg、Alya、Zixuan'

: [candidate.emojiName, ...candidate.aliases].join(", ");

Mmm, actually I've just gone and filed an issue for this, #1080 🙂 so we don't have to take as much space about it. So, TODO(#1080), I guess.

chrisbobbe · 2024-11-23T04:27:07Z

test/widgets/autocomplete_test.dart

-    Finder findNetworkImage(String url) {
-      return find.byWidgetPredicate((widget) => switch(widget) {
-        Image(image: NetworkImage(url: var imageUrl)) when imageUrl == url
-          => true,
-        _ => false,
-      });
-    }


I think this is another thing that'll conflict with #995.

chrisbobbe · 2024-11-23T04:52:55Z

lib/widgets/autocomplete.dart

+    return Padding(
+      padding: const EdgeInsets.symmetric(horizontal: 16.0, vertical: 8.0),
+      child: Row(children: [
+        if (glyph != null) ...[
+          glyph,
+          const SizedBox(width: 8),
+        ],
+        Expanded(
+          child: Text(
+            maxLines: 2,
+            overflow: TextOverflow.ellipsis,
+            label)),
+      ]));


I guess the Figma doesn't show an example of emoji autocomplete, but it does show @-mention autocomplete, with examples of a user result and a user-group result:

Could we kind of extrapolate from what the Figma says for those? #995 is in progress for @-mention items (#913), and in my last review it was aligned with the Figma. There are some things there that seem like they would apply here too, like removing the splash effect for #417. Probably we'd want the label to start at the same distance from the left edge (46px) as the Figma has for user and user-group items.

Yeah, that'd all be good. I think of it as basically part of merging these changes with #995. Right now this version matches the design we have for user results, and after #995 we'll want them to continue to match.

chrisbobbe · 2024-11-23T05:10:08Z

lib/widgets/autocomplete.dart

+        UnicodeEmojiWidget(
+          size: _size, notoColorEmojiTextSize: _notoColorEmojiTextSize,
+          emojiDisplay: emojiDisplay),
+      TextEmojiDisplay() => null, // The text is already shown separately.


It's true that the text is shown, but in a way that's more confusing than it needs to be, I think:

I'll tap "person_tipping_hand" if it looks appealing, but then I might be confused/unhappy to see ":information_desk_person:" written instead.

Is there a nice way to give the "emoji name" some distinct formatting? Maybe borrowing the label/sublabel design from the Figma on @-mention autocomplete:

where the first line says ":information_desk_person:" (or maybe without the colons) and the second says "person_tipping_hand" (plus more if the list is longer)? That same design might be helpful for unicode- and image-display emoji too (though less necessary), so the code could just cleanly apply it unconditionally. This would grow some of the items vertically (splitting what could be one line of text into two), but I think that's an OK tradeoff.

Also I could imagine someone using the "plain text" setting because they don't like seeing emoji everywhere, but they still want to see the emoji in the emoji picker, so they know how it'll look to people who don't use the setting.

where the first line says ":information_desk_person:" (or maybe without the colons) and the second says "person_tipping_hand" (plus more if the list is longer)?

I'd rather keep this aligned with the way the information appears in the emoji picker for a reaction — those seem good to keep consistent. (Zulip web has them pretty disconnected from each other but I don't think that's a good thing.)

That design says:
https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=3059-60682&node-type=frame&t=YWeakAugQUL0uoJa-0

so IOW the full two lines are used for the whole list of names, wrapped.

Part of the rationale for that design, I think, is that we don't want to put too much emphasis on any of the names — as far as most readers are concerned, the emoji is the glyph, not any of the names. (And someday perhaps Zulip's model of emoji will better reflect that — zulip/zulip#18121 — but it's already how the product mainly works and always has.)

Also I could imagine someone using the "plain text" setting because they don't like seeing emoji everywhere, but they still want to see the emoji in the emoji picker, so they know how it'll look to people who don't use the setting.

Plausible. I don't really have a clear sense of what motivations people have for choosing the "plain text" setting.

In any case this is something we can follow up on with future changes if we hear product feedback about it from people who use that setting.

Sure, that all sounds fine.

chrisbobbe · 2024-11-23T05:21:14Z

lib/model/autocomplete.dart

+  /// Characters that might be meant as part of (a query for) an emoji's name,
+  /// other than whitespace.
+  const nameCharacters = r'_\p{Letter}\p{Number}';


Hmm, how do we know which characters these should be? For the corresponding part in _mentionIntentRegex we exclude characters, not include them.

Here, if we don't include \p{Emoji} and I guess maybe the ZWJ (maybe more?) then we defeat the part of EmojiAutocompleteQuery.matches that's supposed to enable searching by literal emoji.

I don't think that feature is really useful here actually. If I want a specific emoji from the iOS emoji picker, I'll just open that directly, right, instead of typing ":" first. Searching by literal emoji seems more helpful when you're choosing an emoji reaction than when you're typing an emoji into a message.

I guess I don't mind offering the feature. 🤷‍♂️ It might help someone who wants their platform's emoji picker but has already typed ":" because they weren't sure if they needed to.

Yeah, I guess this means searching by literal emoji isn't actually offered in the message-content autocomplete. Which is fine because as you say, it's not that useful here.

What that feature is really for is for adding an emoji reaction — it effectively gives the user the option of using their phone keyboard's emoji picker instead of ours.

Hmm, how do we know which characters these should be? For the corresponding part in _mentionIntentRegex we exclude characters, not include them.

Yeah, which matches the fact that users' names can be almost anything, with a small list of character exclusions. Emoji names are more constrained.

But I hadn't actually looked up what those exact constraints are. Turns out it's more constrained even than I'd guessed:

def check_valid_emoji_name(emoji_name: str) -> None: if emoji_name: if re.match(r"^[0-9a-z\-_]+(?<![\-_])$", emoji_name): return # [… otherwise raise one error or other.]

So just ASCII lowercase alnum, underscore, and dash; plus has to end with alnum rather than underscore or dash.

The server applies that constraint when uploading a custom emoji. But we get a bit more general with the Unicode emoji — there are a few of them that have a name breaching that rule:

$ jq <tmp/emoji_api.54c0e268d836.json '.code_to_names | values[] | join("__")' -r \ | grep -P '[^a-z0-9_-]' flag_åland_islands__flag_aland_islands flag_st_barthélemy__flag_st_barthelemy flag_côte_divoire__flag_cote_divoire flag_curaçao__flag_curacao flag_réunion__flag_reunion flag_são_tomé_and_príncipe__flag_sao_tome_and_principe flag_türkiye__flag_turkiye japanese_here_button__here__ココ japanese_service_charge_button__service_charge__サ japanese_reserved_button__reserved__指 japanese_vacancy_button__vacancy__空 +1__thumbs_up__like dumpling__empanada__gyōza__jiaozi__pierogi__potsticker__gyoza coconut__piña_colada__pina_colada moon_cake__autumn__festival__yuèbǐng__yuebing llama__alpaca__guanaco__vicuña__wool__vicuna red_envelope__good_luck__hóngbāo__lai_see__hongbao thong_sandal__beach_sandals__sandals__thong_sandals__thongs__zōri__zori piñata__pinata

I'll see about including dash in this set. Then I think it's pretty comprehensive for what appears in emoji names now and even what might in the future.

gnprice · 2024-11-25T08:52:36Z

Thanks for the thoughtful review! Replied to some subthreads above, and just pushed a revision; PTAL.

chrisbobbe

Thanks! One nit below, then please merge at will.

chrisbobbe · 2024-11-25T19:17:57Z

test/model/autocomplete_test.dart

+    // (A few appear in the server's list of Unicode emoji.
+    // many more might be in a given realm's custom emoji.)


nit: sentence starting with lowercase

Using just `fakeAsync`, when this hit an `await` it just stopped and didn't finish the remainder of the test, so didn't get to the point of testing what it's meant to test. I believe the test worked correctly when first committed, as it had no `await` of its own; but later was accidentally defeated by eca33f9 introducing an `await` for `store.addUsers`. Using our `awaitFakeAsync` fixes the problem. This was the only call to `fakeAsync` in our codebase, so I believe this commit fixes the whole problem.

…es relate

At first glance this appears to make a functional change: it causes the search to begin as soon as the AutocompleteView is constructed, rather than later when `query` is set to non-null. But in fact the only times (outside tests) that we ultimately construct an AutocompleteView are by calling an implementation of AutocompleteField.initViewModel... and those call sites are immediately followed by setting a non-null query. So this is an NFC commit after all.

…ew-model

…tion-etc. The two concepts have meant the same set of possible values so far, because @-mentions are the only type of autocomplete we've had so far in the compose box's content input. As a result we've taken some shortcuts by conflating them. But as we introduce other types of autocomplete in the content input, like for emoji and #-mentions, we have some places that will need to refer to the more general concept while others refer to the more specific one. So separate them out.

This is NFC because when this condition is true, the only return statements below that can actually be reached are the ones that return null. In the code as it is, this makes a small optimization. But it also will help simplify an upcoming refactor.

In particular the regex is effectively private to this logic, so we can make it a private variable and then simplify its name.

This makes room for this loop to look for other autocomplete markers besides `@`.

This puts much less control flow inside the `@` case. That will help simplify things as we add more kinds of autocomplete beyond @-mentions.

This field is no longer used, since 4f6ad02.

…on-mentions

This wasn't needed in the topic-autocomplete test -- we only send typing notifications when editing the content input, not the topic.

This leaves the emojiDisplay field of these objects untested. I skipped that because it seems like pretty boring low-risk code, just invoking emojiDisplayFor. (And emojiDisplayFor has its own tests.) But included a TODO comment for completeness in thinking about what logic there is to test here. Fixes: zulip#669

The initViewModel method can't yet be called, because these EmojiAutocompleteQuery objects aren't yet ever constructed in the actual app. So for the moment it throws UnimplementedError, letting us save for a later commit the implementation of the class it should return an instance of.

As of this commit, it's not yet possible in the app to initiate an emoji autocomplete interaction. So in the widgets code that would consume the results of such an interaction, we just throw for now, leaving that to be implemented in a later commit.

The "forbid preceding ':'" wrinkle is one I discovered because of writing tests: I wrote down the '::^' test, was surprised to find that it failed, and then went back and extended the regexp to make it pass. For this commit we temporarily intercept the query at the AutocompleteField widget, to avoid invoking the widgets that are still unimplemented. That lets us defer those widgets' logic to a separate later commit.

This makes this logic a bit more complicated, but also more explicitly related to the set of possible emoji names, and more fully aligned with that set.

Fixes: zulip#670

gnprice · 2024-11-25T20:09:58Z

Thanks! Fixed and merged.

gnprice added the maintainer review PR ready for review by Zulip maintainers label Nov 21, 2024

chrisbobbe self-assigned this Nov 23, 2024

chrisbobbe self-requested a review November 23, 2024 00:58

chrisbobbe reviewed Nov 23, 2024

View reviewed changes

gnprice force-pushed the pr-emoji branch from a8de429 to ab1def5 Compare November 25, 2024 08:51

chrisbobbe approved these changes Nov 25, 2024

View reviewed changes

gnprice added 21 commits November 25, 2024 12:08

autocomplete [nfc]: Document how query, view-model, and results class…

e561f3a

…es relate

autocomplete [nfc]: Document semantics of query and results on vi…

13385fe

…ew-model

autocomplete [nfc]: Simplify intent-finding loop slightly

0b28227

autocomplete [nfc]: Shorten local names in intent-finding

ce5beaa

In particular the regex is effectively private to this logic, so we can make it a private variable and then simplify its name.

autocomplete [nfc]: Generalize find-intent loop slightly

6b3d426

This makes room for this loop to look for other autocomplete markers besides `@`.

autocomplete [nfc]: Split intent-finding loop to within/before selection

54b1add

This puts much less control flow inside the `@` case. That will help simplify things as we add more kinds of autocomplete beyond @-mentions.

emoji [nfc]: Factor out ImageEmojiWidget

ecd3138

emoji [nfc]: Factor out UnicodeEmojiWidget

1485720

emoji [nfc]: Cut disused selected field on _UnicodeEmoji

8127b0a

This field is no longer used, since 4f6ad02.

autocomplete test [nfc]: Prepare autocompleteIntent tests to handle n…

0abe2b7

…on-mentions

autocomplete test [nfc]: Move TypingNotifier disable to setup helper

a2399a8

This wasn't needed in the topic-autocomplete test -- we only send typing notifications when editing the content input, not the topic.

autocomplete: Accept emoji queries for 👍 and with dashes

8d84178

This makes this logic a bit more complicated, but also more explicitly related to the set of possible emoji names, and more fully aligned with that set.

emoji: Finish emoji autocomplete for compose box

0fd1d64

Fixes: zulip#670

gnprice force-pushed the pr-emoji branch from ab1def5 to 0fd1d64 Compare November 25, 2024 20:09

gnprice merged commit 0fd1d64 into zulip:main Nov 25, 2024
1 check passed

gnprice mentioned this pull request Nov 25, 2024

autocomplete: Implement new design for @-mention autocomplete items #995

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Emoji autocomplete #1069

Emoji autocomplete #1069

gnprice commented Nov 21, 2024

chrisbobbe left a comment

chrisbobbe Nov 23, 2024

chrisbobbe Nov 23, 2024

chrisbobbe Nov 23, 2024

chrisbobbe Nov 23, 2024

chrisbobbe Nov 23, 2024

chrisbobbe Nov 23, 2024

chrisbobbe Nov 23, 2024

chrisbobbe Nov 23, 2024

chrisbobbe Nov 23, 2024

gnprice Nov 25, 2024

chrisbobbe Nov 23, 2024

gnprice Nov 25, 2024

chrisbobbe Nov 25, 2024

chrisbobbe Nov 23, 2024 •

edited

Loading

gnprice Nov 25, 2024

gnprice Nov 25, 2024

gnprice commented Nov 25, 2024

chrisbobbe left a comment

chrisbobbe Nov 25, 2024

gnprice commented Nov 25, 2024

-      : [candidate.emojiName, ...candidate.aliases].join(", ");
+      // TODO(i18n): List formatting, like you can do in JavaScript:
+      //   new Intl.ListFormat('ja').format(['Chris', 'Greg', 'Alya', 'Zixuan'])
+      //   // 'Chris、Greg、Alya、Zixuan'
+      : [candidate.emojiName, ...candidate.aliases].join(", ");

		// (A few appear in the server's list of Unicode emoji.
		// many more might be in a given realm's custom emoji.)

Emoji autocomplete #1069

Emoji autocomplete #1069

Conversation

gnprice commented Nov 21, 2024

Selected commit messages

chrisbobbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Nov 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice commented Nov 25, 2024

chrisbobbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice commented Nov 25, 2024

chrisbobbe Nov 23, 2024 •

edited

Loading