Add reusable EmojiWidget #1928

chrisbobbe · 2025-10-18T01:32:39Z

Stacked on #1926.

This resolves a few TODOs, and will be useful toward

Apply emoji-display nuances to message content, same as reactions #966

chrisbobbe · 2025-10-21T18:57:53Z

Revision pushed, fixing a bug where I forgot to use neverAnimate in EmojiWidget, and adding a new commit:

ef29c09 settings [nfc]: Centralize logic for checking device animation settings

rajveermalviya

Thanks @chrisbobbe. Overall LGTM, just one comment.

rajveermalviya · 2025-10-23T15:14:45Z

lib/widgets/user.dart

        emojiDisplay: emojiDisplay,
        squareDimension: size,
-        neverAnimateImage: neverAnimate,
+        imageAnimationMode: ImageAnimationMode.animateNever,


Should this respect the value of neverAnimate?

Ooh, yes, thanks for the catch!

chrisbobbe · 2025-10-23T19:54:37Z

Thanks for the review! Revision pushed.

rajveermalviya

Thanks! LGTM, moving over to Greg's review.

gnprice

Thanks for this cleanup! Here's a review of the first 6 commits:
dc5d5dd emoji [nfc]: Dartdoc EmojiStore.emojiDisplayFor
0813931 emoji [nfc]: Pull out textEmojiForEmojiName helper
23985d3 emoji_reaction [nfc]: Inline _UnicodeEmoji
ea3299a emoji_reaction [nfc]: Inline _ImageEmoji
59f4a86 emoji_reaction [nfc]: Give _TextEmoji the emoji name more straightforwardly
6311f4f emoji [nfc]: Add reusable EmojiWidget; use it in ReactionChip

Going AFK for the moment, so I'll leave for a later review the remaining 6 commits:
329c335 emoji [nfc]: Use EmojiWidget in emoji picker and autocomplete
26ac403 emoji_reaction [nfc]: Use EmojiWidget in view-reactions header
29aeecb user status: When we can't load image emoji, lay out a padded empty square
3fece59 user status [nfc]: Pull Padding outside the emojiDisplay switch
30110ba user status [nfc]: Use EmojiWidget
3d7da28 settings [nfc]: Centralize logic for checking device animation settings

(Also on an early skim the last commit looks like it'd be best as a separate PR — seems nontrivial, and thematically rather separate from the rest.)

gnprice · 2025-10-30T20:27:48Z

lib/model/emoji.dart

+  /// May be a [TextEmojiDisplay] even if the emojiset is not [Emojiset.text];
+  /// this happens for image emojis whose URLs don't parse.
  EmojiDisplay emojiDisplayFor({


Looking at the implementation, there are a few other cases where this happens, right? Seems like more generally it's when we can't understand the data that describes the emoji.

gnprice · 2025-10-30T20:35:33Z

lib/widgets/emoji.dart

+      UnicodeEmojiDisplay() => UnicodeEmojiWidget(
+        size: squareDimension,
+        emojiDisplay: emojiDisplay),


It looks like this drops textScaler.

gnprice · 2025-10-30T20:41:44Z

lib/widgets/emoji.dart

+  ImageErrorWidgetBuilder _getImageErrorBuilder(double effectiveSize) {
+    return (_, _, _) => switch (imagePlaceholderStyle) {


Seems like this indirection of a function returning a function can be simplified: we can wait to compute effectiveSize until the point where it turns out we need it.

chrisbobbe · 2025-10-30T21:44:08Z

Thanks! Revision pushed, and I'll send that last commit in a separate PR. -> #1957

gnprice

Thanks! Just nits below.

gnprice · 2025-10-31T01:00:52Z

lib/widgets/autocomplete.dart

+        buildCustomTextEmoji: () {
+          // emojiDisplay is not TextEmojiDisplay,
+          // and imagePlaceholderStyle is not EmojiImagePlaceholderStyle.text.
+          assert(false);
+          return SizedBox.shrink();
+        },


Since this case can't happen, seems simpler to pass over it in silence 🙂 — I think the reader here won't naturally feel a need to think about it if the code doesn't bring it up

Particularly as if this case does happen, and the reader works out what this SizedBox.shrink produces in the layout, it seems like it'd be glitchy — we'd still have the 6px padding below but now around a zero-sized box.

bump :)

(you cut the other similar case, but there's this one too)

gnprice · 2025-10-31T01:11:00Z

lib/widgets/user.dart

+      child: EmojiWidget(
+        emojiDisplay: emojiDisplay,


nit: over-indented as of the penultimate commit:

+ child: switch (emojiDisplay) { + UnicodeEmojiDisplay() => UnicodeEmojiWidget(size: size, emojiDisplay: emojiDisplay), + ImageEmojiDisplay() => ImageEmojiWidget(

gnprice · 2025-10-31T01:16:08Z

lib/widgets/user.dart

-          neverAnimate: neverAnimate,
-          // If image emoji fails to load, show nothing.
-          errorBuilder: (_, _, _) => placeholder)),
-      // The user-status feature doesn't support a :text_emoji:-style display.


I think this line would be good as a replacement for the "Web doesn't seem to respect …" comment above. This one is stating a product/design choice we've made, which is what I think has effectively happened here.

chrisbobbe · 2025-10-31T21:34:51Z

Thanks for the review! Revision pushed.

gnprice

Thanks! One nit remaining.

gnprice · 2025-10-31T22:10:04Z

lib/widgets/autocomplete.dart

+        buildCustomTextEmoji: () {
+          // emojiDisplay is not TextEmojiDisplay,
+          // and imagePlaceholderStyle is not EmojiImagePlaceholderStyle.text.
+          assert(false);
+          return SizedBox.shrink();
+        },


bump :)

(you cut the other similar case, but there's this one too)

chrisbobbe · 2025-10-31T23:09:41Z

Indeed, thanks—revision pushed.

This makes the caller a bit more verbose, but we're about to make it simpler than it was before.

…wardly

There are several places where we switch on EmojiDisplay, mostly with TODOs to factor the switch out to something central. Here's a widget to accomplish that factoring, used first in ReactionChip.

…quare Instead of laying out - just padding, in the case where the network request failed, and - nothing, in the case where the URL didn't parse.

gnprice · 2025-11-01T00:14:01Z

Thanks! Looks good; merging.

chrisbobbe requested a review from rajveermalviya October 18, 2025 01:32

chrisbobbe assigned rajveermalviya Oct 18, 2025

chrisbobbe added the maintainer review PR ready for review by Zulip maintainers label Oct 18, 2025

chrisbobbe force-pushed the pr-emoji-widget branch from f25380a to ef29c09 Compare October 21, 2025 18:57

chrisbobbe force-pushed the pr-emoji-widget branch from ef29c09 to 9ab45a0 Compare October 21, 2025 18:59

rajveermalviya approved these changes Oct 23, 2025

View reviewed changes

chrisbobbe force-pushed the pr-emoji-widget branch from 9ab45a0 to 3d7da28 Compare October 23, 2025 19:54

rajveermalviya approved these changes Oct 24, 2025

View reviewed changes

rajveermalviya 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 Oct 24, 2025

rajveermalviya assigned gnprice and unassigned rajveermalviya Oct 24, 2025

rajveermalviya requested a review from gnprice October 24, 2025 14:52

gnprice reviewed Oct 30, 2025

View reviewed changes

chrisbobbe force-pushed the pr-emoji-widget branch from 3d7da28 to 1f92dfd Compare October 30, 2025 21:43

chrisbobbe mentioned this pull request Oct 30, 2025

settings [nfc]: Centralize logic for checking device animation settings #1957

Merged

gnprice reviewed Oct 31, 2025

View reviewed changes

chrisbobbe force-pushed the pr-emoji-widget branch from 1f92dfd to 42b3a5d Compare October 31, 2025 21:34

gnprice reviewed Oct 31, 2025

View reviewed changes

chrisbobbe force-pushed the pr-emoji-widget branch from 42b3a5d to 175084d Compare October 31, 2025 23:09

chrisbobbe added 5 commits October 31, 2025 17:13

emoji [nfc]: Dartdoc EmojiStore.emojiDisplayFor

29a1f38

emoji [nfc]: Pull out textEmojiForEmojiName helper

07aeb5a

emoji_reaction [nfc]: Inline _UnicodeEmoji

5a94337

This makes the caller a bit more verbose, but we're about to make it simpler than it was before.

emoji_reaction [nfc]: Inline _ImageEmoji

19080d7

This makes the caller a bit more verbose, but we're about to make it simpler than it was before.

emoji_reaction [nfc]: Give _TextEmoji the emoji name more straightfor…

cc69cce

…wardly

chrisbobbe added 6 commits October 31, 2025 17:13

emoji [nfc]: Add reusable EmojiWidget; use it in ReactionChip

e137566

There are several places where we switch on EmojiDisplay, mostly with TODOs to factor the switch out to something central. Here's a widget to accomplish that factoring, used first in ReactionChip.

emoji [nfc]: Use EmojiWidget in emoji picker and autocomplete

6d55b2c

emoji_reaction [nfc]: Use EmojiWidget in view-reactions header

d3e7363

user status: When we can't load image emoji, lay out a padded empty s…

e7afb83

…quare Instead of laying out - just padding, in the case where the network request failed, and - nothing, in the case where the URL didn't parse.

user status [nfc]: Pull Padding outside the emojiDisplay switch

14c5483

user status [nfc]: Use EmojiWidget

6613892

gnprice force-pushed the pr-emoji-widget branch from 175084d to 6613892 Compare November 1, 2025 00:14

gnprice merged commit 6613892 into zulip:main Nov 1, 2025

chrisbobbe deleted the pr-emoji-widget branch November 1, 2025 00:17

		ImageErrorWidgetBuilder _getImageErrorBuilder(double effectiveSize) {
		return (_, _, _) => switch (imagePlaceholderStyle) {

Add reusable EmojiWidget #1928

Add reusable EmojiWidget #1928

Conversation

chrisbobbe commented Oct 18, 2025

Uh oh!

chrisbobbe commented Oct 21, 2025

Uh oh!

rajveermalviya left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Oct 23, 2025

Uh oh!

rajveermalviya left a comment

Choose a reason for hiding this comment

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Oct 31, 2025

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Oct 31, 2025

Uh oh!

gnprice commented Nov 1, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

chrisbobbe commented Oct 30, 2025 •

edited

Loading