msglist: Show message reactions; support reacting (with 👍, or to join in others' reactions) #410

chrisbobbe · 2023-11-22T09:50:49Z

Fixes #121.
Fixes #125.

Without plain-text-emojis setting	With plain-text-emojis setting

chrisbobbe · 2023-11-22T17:53:12Z

Oh, I've noticed a subtle bug in the model code. When you have two message lists open, and a new-message event arrives, we add the same Message object to both message lists (if it belongs in both), instead of adding copies. This means that reaction events are double-processed on a single Message object. (Processed once for each message list, on the same Message instance.)

Here's a recipe for reproducing one symptom:

On your account, enable "Display names of reacting users when few users have reacted to a message."
Check out this PR and log into your account in zulip-flutter.
Open "All messages".
Open a topic narrow by tapping a recipient header.
Send a message in that topic.
In the web app, add a reaction on the message. (Any reaction, from any user).
In zulip-flutter, see the name of the reacting user (or "You") appears, as expected.
Repeat step 6, but with a different reaction or user.
This time, in zulip-flutter, the reacting user's name is not shown; it just shows numbers. This would be expected if there were three or more reactions on the message, but in this case there are just two.

In step 9, the [Reactions.total] is 4 instead of 2 because of the double-processing.

gnprice · 2023-11-22T18:03:44Z

Interesting, yeah.

I think we can let that bug not block merging this, and handle it later. The fix will probably involve having a central map of messages, akin to state.messages in zulip-mobile.

chrisbobbe · 2023-11-22T19:42:53Z

Oh, also: Unicode emoji and image emoji chips aren't the same height:

I found this kind of tricky to fix, but there might be some mostly-fine way to address this symptom. It's tricky because for Unicode emojis we have to think about text layout, while with image emoji we have a nice convenient square.

chrisbobbe · 2023-11-22T20:45:48Z

Just pushed a revision with a quick add-thumbs-up button in the message action sheet, which we'll plan to replace later:

(And so updated the PR description to say that, for #125, this is a full fix instead of a partial one.)

chrisbobbe · 2023-11-22T21:33:45Z

(Also I'll need to write some tests; I guess I should mark as a draft because of that.)

gnprice

Thanks! Great to see this. Here's a review from without having yet fetched your revision today. Generally all small comments, but one exciting wrinkle when it comes to the font layout.

gnprice · 2023-11-22T19:53:43Z

lib/widgets/emoji_reaction.dart

+import 'store.dart';
+import 'text.dart';
+
+class ReactionChipsList extends StatelessWidget {


And also support: - removing a reaction you've already made, and - joining in on existing reactions that other people have made. It leaves out one part of #125, for now: - joining in on an existing reaction other people have made

I think the latter item means to say adding a thumbs-up reaction 🙂

gnprice · 2023-11-22T19:55:34Z

lib/widgets/emoji_reaction.dart

+  final int messageId;
+  final Reactions reactions;
+
+  const ReactionChipsList({
+    super.key,
+    required this.messageId,
+    required this.reactions,
+  });


nit: constructor before fields

(the exception to that is for "data"-style classes, which generally includes what's in lib/api/ but which doesn't include widgets)

gnprice · 2023-11-22T20:04:53Z

lib/widgets/emoji_reaction.dart

+    return Tooltip(
+        excludeFromSemantics: true, // TODO: Semantics with eg "Reaction: <emoji name>; you and N others: <names>"
+        message: emojiName,
+        child: Material(


nit: indentation

gnprice · 2023-11-22T20:08:14Z

lib/widgets/emoji_reaction.dart

+                  child: (() {
+                    if (emojiset == Emojiset.text) {
+                      return _TextEmoji(emojiName: emojiName, selected: selfVoted);
+                    }
+                    switch (reactionType) {
+                      case ReactionType.unicodeEmoji:


This is probably easiest to read as a local variable:

final Widget emoji; if (emojiset == Emojiset.text) { emoji = _TextEmoji(emojiName: emojiName, selected: selfVoted); } else { switch (reactionType) { // …

gnprice · 2023-11-22T20:11:31Z

lib/widgets/emoji_reaction.dart

+  @override
+  Widget build(BuildContext context) {
+    final parsed = tryParseEmojiCodeToUnicode(emojiCode);
+    if (parsed == null) {


Suggested change

if (parsed == null) {

if (parsed == null) { // TODO(log)

This shouldn't actually be possible.

gnprice · 2023-11-22T20:19:42Z

lib/widgets/emoji_reaction.dart

+    final doNotAnimate = MediaQuery.of(context).disableAnimations
+      || PlatformDispatcher.instance.accessibilityFeatures.reduceMotion;


Hmm interesting — are these two different settings?

In any event the PlatformDispatcher.instance should instead be WidgetsBinding.instance.platformDispatcher. See docs on PlatformDispatcher and PlatformDispatcher.instance.

But then even the binding is a global thing, whereas MediaQuery has the job of exposing much of that information in a more widgets-oriented way. Does MediaQuery not expose the latter setting?

Yes, these are two different settings. See AccessibilityFeatures.reduceMotion and AccessibilityFeatures.disableAnimations: https://api.flutter.dev/flutter/dart-ui/AccessibilityFeatures-class.html

It says "reduce motion" is an iOS-only setting, though. Probably best/safest to only check it on iOS; I'll do that.

"Disable animations" is exposed on MediaQueryData: https://api.flutter.dev/flutter/widgets/MediaQueryData/disableAnimations.html

But "reduce motion" is not. There is an issue to expose it there (flutter/flutter#65874), but it looks like something else might be done instead, like it might be exposed on a new inherited widget.

Cool, thanks.

I think "reduce motion" probably shouldn't control this. Here's what looks to be Apple dev docs for it:
https://developer.apple.com/documentation/swiftui/environmentvalues/accessibilityreducemotion

If this property’s value is true, UI should avoid large animations, especially those that simulate the third dimension.

And the user-facing description, from the screenshot in that upstream image, is:

Reduce the motion of the user interface, including the parallax effect of icons.

So that sounds like a different type of thing from this.

We discussed this further in our call today, and it looks like disableAnimations actually never gets set on iOS. Given that, we should probably consult reduceMotion after all (like the current revision does) so that the user has some way to ask the animations to stop.

Then recent versions of iOS (iOS 17+, so new this year) do have a more closely relevant setting:
https://developer.apple.com/documentation/swiftui/environmentvalues/accessibilityplayanimatedimages
which is documented for users at
https://support.apple.com/guide/iphone/reduce-onscreen-motion-iph0b691d3ed/ios
as:

Auto-Play Animated Images: […] (When on, rapid animated images and moving elements such as GIFs in Messages and Safari play automatically.)

So there should be a Flutter issue upstream to provide that value, either by having it control disableAnimations or by adding a new separate field on AccessibilityFeatures.

If there's such an issue already, we can link to it in a comment here. If not, we can leave a TODO comment for ourselves to file that issue — though actually filing it would be a task for after Beta 1.

Cool; yeah. Thanks for writing that down!

gnprice · 2023-11-22T22:57:24Z

lib/widgets/emoji_reaction.dart

+    final doNotAnimate = MediaQuery.of(context).disableAnimations
+      || PlatformDispatcher.instance.accessibilityFeatures.reduceMotion;
+
+    String src;


Suggested change

String src;

final String src;

That way the compiler verifies that it's only ever set once.

gnprice · 2023-11-22T22:59:09Z

lib/widgets/emoji_reaction.dart

+    final store = PerAccountStoreWidget.of(context);
+
+    // Some people really dislike animated emoji.
+    final doNotAnimate = MediaQuery.of(context).disableAnimations


Suggested change

final doNotAnimate = MediaQuery.of(context).disableAnimations

final doNotAnimate = MediaQuery.disableAnimationsOf(context)

(see its doc)

gnprice · 2023-11-22T23:59:18Z

lib/widgets/emoji_reaction.dart

+    return SizedBox(
+      width: textScaler.scale(17),
+      child: Text(
+        style: const TextStyle(fontSize: 17),
+        strutStyle: const StrutStyle(fontSize: 17, forceStrutHeight: true),
+        parsed));


Fun. So for me on a Pixel, this gives different results from what you're seeing on iOS. Specifically here it is on a message where I've added a bunch of the same reactions:

The Unicode emoji there are crowding the counts. That's because, the inspector tells me, the glyphs are overflowing wider than the 17px of the box they're in. The height is 20px, and if I remove the SizedBox, the layout width is too, reflecting the width they already have visually.

So it seems that for the Noto emoji font (which is what this device has for a system emoji font), a font size of 17 gets glyphs with an actual size of 20px.

From a binary search, if I set the font size to 14.5, then the actual resulting size is 17.0px. If I do that and also take out that one pixel of left padding to match web, then the result is:

which looks a lot better aligned.

@@ -118,4 +118,1 @@ class ReactionChip extends StatelessWidget { - // - // Separately, web has 1px less than this on the left, but that - // asymmetry doesn't seem to help us. - padding: const EdgeInsets.symmetric(horizontal: 5, vertical: 2), + padding: const EdgeInsets.fromLTRB(4, 2, 5, 2), @@ -179,9 +176,10 @@ class _UnicodeEmoji extends StatelessWidget { return _TextEmoji(emojiName: emojiName, selected: selected); } final textScaler = MediaQuery.textScalerOf(context); + const fontSize = 14.5; return SizedBox( width: textScaler.scale(17), child: Text( - style: const TextStyle(fontSize: 17), - strutStyle: const StrutStyle(fontSize: 17, forceStrutHeight: true), + style: const TextStyle(fontSize: fontSize), + strutStyle: const StrutStyle(fontSize: fontSize, forceStrutHeight: true), parsed));

This can partly be seen as a downstream consequence of not bundling an emoji font.

Maybe the right approach to keep things moving is:

Have a defaultTargetPlatform check that makes a couple of things different here between iOS and Android.

On iOS, find whatever value goes instead of 14.5 that gets a size of 17 with Apple's emoji font.

On Android, use that value of 14.5 that gets a size of 17 with the Noto emoji font.

That will make it work exactly as desired on iOS devices and Pixel devices. It might look a little odd on other Android devices — not sure how much.

And then as a followup issue, we'll go back to bundling Noto Color Emoji, but only on non-Apple platforms because Apple platforms aren't compatible with it. Then this will work exactly as desired on all devices.

Ah. Thanks for that investigation and proposed way forward!

Things are different on iOS—a font size of 17 gives an emoji that looks (to me) approximately 17px square…but the Text takes 20px height by 21.3px width, when not constrained by the SizedBox. And the emoji doesn't look centered in the space occupied by the Text.

I think this is flutter/flutter#119623 and flutter/flutter#28894. I'm going to try this workaround for it (which I see you've 👍-reacted before): flutter/flutter#28894 (comment)

I'm going to try this workaround for it (which I see you've 👍-reacted before): […]

Actually I'm not very excited about doing this; I'll try something simpler.

I'm going to try this workaround for it (which I see you've 👍-reacted before): flutter/flutter#28894 (comment)

Actually I'm not very excited about doing this; I'll try something simpler.

Yeah, I think my 👍 there wasn't about the substance of the workaround so much as about returning to on-topic discussion.

I agree that thread looks highly relevant. Here's the key piece of investigation:
flutter/flutter#28894 (comment)

In short I think the behavior we're seeing on Android, where font size 17 makes a glyph that's actually 20px, is the intended behavior on both Android and iOS, because it's what users like for emoji in the middle of text. And Flutter currently isn't getting that behavior on iOS because the way Apple accomplishes it is some kind of hack that isn't in their core text-handling libraries.

gnprice · 2023-11-23T00:08:30Z

lib/widgets/emoji_reaction.dart

+    final resolved = store.account.realmUrl.resolveUri(parsedSrc);
+
+    // Unicode emoji get scaled; it would look weird if image emoji didn't.
+    final size = MediaQuery.textScalerOf(context).scale(17);


It'd be good for this 17 to be a named constant that's shared with the other 17s that are meant to equal it, in _UnicodeEmoji and _TextEmoji.

gnprice

OK, and small comments on the action-sheet item. That UI for it looks good to me!

gnprice · 2023-11-23T00:12:00Z

lib/widgets/action_sheet.dart

+    ?.aggregated.any((reactionWithVotes) =>
+      reactionWithVotes.emojiCode == '1f44d'


Should check reactionType before emojiCode, as the latter is namespaced by the former.

gnprice · 2023-11-23T00:18:57Z

lib/widgets/action_sheet.dart

+
+  @override
+  String label(ZulipLocalizations zulipLocalizations) {
+    return 'React with 👍'; // skip translation for now


Suggested change

return 'React with 👍'; // skip translation for now

return 'React with 👍'; // TODO(i18n) skip translation for now

I'd like this spot to come up in a grep for places that need i18n, even if we don't end up acting on this one in particular before we replace the code.

gnprice · 2023-11-23T00:24:57Z

lib/widgets/action_sheet.dart

+// This button is very temporary, to complete #125 before we have a way to
+// choose an arbitrary reaction (#388). So, skipping tests and i18n.
+class AddThumbsUpButton extends MessageActionSheetMenuItemButton {


I think it wouldn't take much work to write just a test or two for this, so it's worth doing that. The main thing for a test is: tap the icon, check that we made an appropriate API request.

A bonus test would be: prepare an error response, tap the icon, check that there's an error dialog (and so we didn't just crash).

Can drop the "skipping tests" part now :-)

chrisbobbe · 2023-11-27T21:50:47Z

Thanks for the review! Revision pushed. It still needs tests for the reaction-chips UI.

This time I've also included screenshots from the office Android device; see below. This device is old, and it's not using Noto Color Emoji, so it'll look different after #404. But I think the layout turns out OK anyway.

Without plain-text-emojis setting	With plain-text-emojis setting

gnprice · 2023-11-28T19:15:43Z

lib/widgets/emoji_reaction.dart

+    if (defaultTargetPlatform != TargetPlatform.iOS
+      && defaultTargetPlatform != TargetPlatform.macOS) {


Let's use a switch — that way if another value is added (perhaps they bow to Apple's naming and make a separate TargetPlatform.iPadOS?!?), we get exhaustiveness checking to make sure we choose which behavior the new value should get.

gnprice · 2023-11-28T20:48:20Z

lib/widgets/emoji_reaction.dart

+// TODO(#404) Actually bundle Noto Color Emoji with the app. Some newer Android
+//   phones seem to use Noto Color Emoji automatically, but it might not be
+//   universal, and at least the office Android device (which is old) doesn't.


Yeah, this definitely isn't universal among Android devices, even new ones. In particular Samsung has their own emoji font.

See for example Emojipedia's list of glyphs for a random emoji:
https://emojipedia.org/moose#designs

They do say of Noto Color Emoji:

Default emoji set for Android devices created by Google and used on devices by Google, Xiaomi, Oppo, Huawei, Sony, and many others.

gnprice · 2023-11-28T20:56:59Z

lib/widgets/emoji_reaction.dart

+    final doNotAnimate = MediaQuery.of(context).disableAnimations
+      || PlatformDispatcher.instance.accessibilityFeatures.reduceMotion;


Cool, thanks.

I think "reduce motion" probably shouldn't control this. Here's what looks to be Apple dev docs for it:
https://developer.apple.com/documentation/swiftui/environmentvalues/accessibilityreducemotion

If this property’s value is true, UI should avoid large animations, especially those that simulate the third dimension.

And the user-facing description, from the screenshot in that upstream image, is:

Reduce the motion of the user interface, including the parallax effect of icons.

So that sounds like a different type of thing from this.

gnprice · 2023-11-28T21:04:30Z

test/widgets/action_sheet_test.dart

+      TestZulipBinding.ensureInitialized();
+      TestWidgetsFlutterBinding.ensureInitialized();


These only need to be called once, at the top of main.

gnprice · 2023-11-28T22:59:59Z

lib/widgets/emoji_reaction.dart

+  final String emojiName;
+  final bool selected;
+
+  const _TextEmoji({required this.emojiName, required this.selected});


nit: constructor before fields

gnprice · 2023-11-28T23:05:55Z

lib/widgets/emoji_reaction.dart

+    final doNotAnimate = MediaQuery.of(context).disableAnimations
+      || PlatformDispatcher.instance.accessibilityFeatures.reduceMotion;


We discussed this further in our call today, and it looks like disableAnimations actually never gets set on iOS. Given that, we should probably consult reduceMotion after all (like the current revision does) so that the user has some way to ask the animations to stop.

Then recent versions of iOS (iOS 17+, so new this year) do have a more closely relevant setting:
https://developer.apple.com/documentation/swiftui/environmentvalues/accessibilityplayanimatedimages
which is documented for users at
https://support.apple.com/guide/iphone/reduce-onscreen-motion-iph0b691d3ed/ios
as:

Auto-Play Animated Images: […] (When on, rapid animated images and moving elements such as GIFs in Messages and Safari play automatically.)

So there should be a Flutter issue upstream to provide that value, either by having it control disableAnimations or by adding a new separate field on AccessibilityFeatures.

If there's such an issue already, we can link to it in a comment here. If not, we can leave a TODO comment for ourselves to file that issue — though actually filing it would be a task for after Beta 1.

gnprice · 2023-11-28T23:16:19Z

Thanks for the revision! This looks great. Small comments above, and otherwise it just needs a few tests for the UI.

As we discussed in our call, those tests can be kept simple; it's OK if they don't fully exercise all the things this code takes care of, when those seem like they'd be complicated to test. (Like the textWidthBasis: TextWidthBasis.longestLine on the names of people reacting.)

One key useful step for the tests is to just build the UI in situations with long names and such, to force it to overflow one line, and check that it doesn't crash. Because the tests run with debug assertions, that will also have the effect of checking that it doesn't hit buggy layout conditions like having a Row or Column widget overflow.

These calls only need to happen once, at the top of `main`: zulip#410 (comment)

chrisbobbe · 2023-11-30T09:18:15Z

Revision pushed, with some smoke tests this time. So, un-marking as a draft! 🎉

I did make a change that affects :text_emojis:, even though earlier I'd said I would postpone it. In particular, now when the text emoji is long (like :family_man_man_girl_boy: 👨‍👨‍👧‍👦), it wraps, if it needs to make room for users' names:

Previous revision	This revision
(no text scaling)	(no text scaling)
(text scaling up 4 notches)	(text scaling up 4 notches)

This mostly leaves more common cases unchanged:

Without plain-text-emojis setting	With plain-text-emojis setting

but I should point out that as a consequence of the implementation, the label text will linebreak a little bit sooner than is strictly necessary, when the emoji part is square (which for most people it always is):

gnprice

Thanks for the revision! This all looks good. The one high-level comment I have is that this is a more intense set of tests than I had in mind — good to have but probably something quicker to write would have been best given the pair of upcoming releases.

Small comments below, almost all on the tests. (And for the commit that's also #425, I reviewed it there.)

gnprice · 2023-12-01T20:33:27Z

lib/widgets/emoji_reaction.dart

+            child: LayoutBuilder(
+              builder: (context, constraints) {
+                final maxRowWidth = constraints.maxWidth;
+                // To give text emojis some room so they need fewer line breaks
+                // when the label is long
+                final maxLabelWidth = (maxRowWidth - 6) * 0.75; // 6 is padding


Hmm interesting.

I have an intuition that LayoutBuilder potentially comes at a performance cost. It has the flavor of inverting layers — it's a way of re-entering the build phase while in the layout phase.

But I don't see a warning of any kind in its docs:
https://api.flutter.dev/flutter/widgets/LayoutBuilder-class.html

so maybe there's nothing to worry about. I guess we can revisit this in the future if we see evidence of a problem.

Yeah; hmm. Makes sense.

gnprice · 2023-12-01T20:51:44Z

lib/widgets/emoji_reaction.dart

+                // To give text emojis some room so they need fewer line breaks
+                // when the label is long
+                final maxLabelWidth = (maxRowWidth - 6) * 0.75; // 6 is padding


but I should point out that as a consequence of the implementation, the label text will linebreak a little bit sooner than is strictly necessary, when the emoji part is square (which for most people it always is)

Yeah. Hmm I see, and at this point we don't actually know whether we'll end up showing a text emoji, because we use those as fallbacks for error conditions. Including an image not loading, which is something we learn especially late.

Let's file a quick followup issue for this (quick in the effort of filing, not of resolving the issue), milestoned post-launch. And then we'll see if it bugs people; if not, we can just leave it this way indefinitely. If people do complain, the issue will be useful by saving us the effort of debugging to diagnose it or of rediscovering why it's not easy to fix (and what other cases the easy fixes would break).

Sure, makes sense. -> #433

gnprice · 2023-12-01T21:16:58Z

test/widgets/emoji_reaction_test.dart

+void main() {
+  group('ReactionChipsList', () {
+    TestZulipBinding.ensureInitialized();
+
+    late PerAccountStore store;


nit: put binding ensureInitialized at top of main:

Suggested change

void main() {

group('ReactionChipsList', () {

TestZulipBinding.ensureInitialized();

late PerAccountStore store;

void main() {

TestZulipBinding.ensureInitialized();

group('ReactionChipsList', () {

late PerAccountStore store;

That's how it effectively behaves anyway — because it's outside of any test call, it's running regardless of which subset of tests are selected by --name or the like — so might as well have the code reflect that.

gnprice · 2023-12-01T21:20:15Z

test/widgets/emoji_reaction_test.dart

+                addTearDown(() {
+                  tester.platformDispatcher.clearTextScaleFactorTestValue();
+                });


nit: can simplify slightly by using a tear-off:

Suggested change

addTearDown(() {

tester.platformDispatcher.clearTextScaleFactorTestValue();

});

addTearDown(tester.platformDispatcher.clearTextScaleFactorTestValue);

gnprice · 2023-12-01T21:24:33Z

test/widgets/emoji_reaction_test.dart

+                  deactivated: false, sourceUrl: '/foo/3', stillUrl: null),
+            };
+
+            runSmokeTest([


The five runSmokeTest calls end up with colliding test names:

00:02 +105: ReactionChipsList smoke: no names / text / ltr / text scale: 1.0 00:02 +106: ReactionChipsList smoke: no names / text / ltr / text scale: 1.0 00:02 +107: ReactionChipsList smoke: no names / text / ltr / text scale: 1.0 00:02 +108: ReactionChipsList smoke: no names / text / ltr / text scale: 1.0 00:02 +109: ReactionChipsList smoke: no names / text / ltr / text scale: 1.0

Let's do something like:

Suggested change

runSmokeTest([

runSmokeTest('unknown user', [

and then the names can be like "smoke: unknown user / no names / text / ltr / text scale: 1.0".

The names will also be helpful for readers of the test source, to express the purpose of each test case.

gnprice · 2023-12-01T21:26:22Z

test/widgets/emoji_reaction_test.dart

+            final i1 = {'emoji_name': 'twocents', 'emoji_code': '181', 'reaction_type': 'realm_emoji'};
+            final i2 = {'emoji_name': 'twocents', 'emoji_code': '182', 'reaction_type': 'realm_emoji'};
+
+            // Base JSON for the one"Zulip extra emoji" reaction. Just missing user_id.


nit:

Suggested change

// Base JSON for the one"Zulip extra emoji" reaction. Just missing user_id.

// Base JSON for the one "Zulip extra emoji" reaction. Just missing user_id.

gnprice · 2023-12-01T21:27:55Z

test/widgets/emoji_reaction_test.dart

+            // Base JSON for various image emoji reactions. Just missing user_id.
+            final i1 = {'emoji_name': 'twocents', 'emoji_code': '181', 'reaction_type': 'realm_emoji'};
+            final i2 = {'emoji_name': 'twocents', 'emoji_code': '182', 'reaction_type': 'realm_emoji'};
+


The category of "image emoji" includes both the reaction types zulip_extra_emoji and realm_emoji; this list is only the latter, with the former coming later. So e.g.:

Suggested change

// Base JSON for various image emoji reactions. Just missing user_id.

final i1 = {'emoji_name': 'twocents', 'emoji_code': '181', 'reaction_type': 'realm_emoji'};

final i2 = {'emoji_name': 'twocents', 'emoji_code': '182', 'reaction_type': 'realm_emoji'};

// Base JSON for various realm-emoji reactions. Just missing user_id.

final i1 = {'emoji_name': 'twocents', 'emoji_code': '181', 'reaction_type': 'realm_emoji'};

final i2 = {'emoji_name': 'twocents', 'emoji_code': '182', 'reaction_type': 'realm_emoji'};

gnprice · 2023-12-01T21:32:44Z

test/widgets/emoji_reaction_test.dart

+            final i1 = {'emoji_name': 'twocents', 'emoji_code': '181', 'reaction_type': 'realm_emoji'};
+            final i2 = {'emoji_name': 'twocents', 'emoji_code': '182', 'reaction_type': 'realm_emoji'};


Suggested change

final i1 = {'emoji_name': 'twocents', 'emoji_code': '181', 'reaction_type': 'realm_emoji'};

final i2 = {'emoji_name': 'twocents', 'emoji_code': '182', 'reaction_type': 'realm_emoji'};

final i1 = {'emoji_name': 'twocents', 'emoji_code': '181', 'reaction_type': 'realm_emoji'};

final i2 = {'emoji_name': 'threecents', 'emoji_code': '182', 'reaction_type': 'realm_emoji'};

At least I think this is what you meant — it's what matches realmEmoji below.

Yep! Thanks!

These calls only need to happen once, at the top of `main`: zulip#410 (comment)

chrisbobbe · 2023-12-01T22:37:49Z

Thanks! Revision pushed.

…port

And also support: - removing a reaction you've already made, and - joining in on existing reactions that other people have made. As is our habit with the message list, this aims to be faithful to the web app, as accessed today. That should be a good baseline to make mobile-specific adjustments from. (In particular I think we may want larger touch targets.) Unlike the web app, we use a font instead of a sprite sheet to render Unicode emoji. This means that we, unlike web, have to account for text-layout algorithms, and things like font metrics. So if Unicode emoji appear noticeably differently from web, that probably has something to do with it. Fixes: zulip#121 Fixes-partly: zulip#125

These calls only need to happen once, at the top of `main`: zulip#410 (comment)

This teardown is already added in setupToMessageActionSheet, which is called near the top of every test.

Fixes: zulip#125

gnprice · 2023-12-01T23:19:47Z

Thanks! LGTM; merging.

chrisbobbe · 2023-12-01T23:20:37Z

Great! Thanks for all your reviews and help with this.

We should also use this font for Unicode emojis elsewhere, such as message content, users' names, and so on. But the proper layout of these chips is especially sensitive to how these emojis get rendered, and apparently the system emoji font can vary quite a bit on Android (same link as removed in the TODO): zulip#410 (comment) So we'd like to converge on one font here (at least for Android) as soon as possible.

chrisbobbe requested a review from gnprice November 22, 2023 09:50

chrisbobbe force-pushed the pr-reaction-chips-ui branch from a6a0fee to ecce64c Compare November 22, 2023 09:53

chrisbobbe force-pushed the pr-reaction-chips-ui branch from ecce64c to a3a5f5a Compare November 22, 2023 20:44

chrisbobbe changed the title ~~msglist: Show message reactions!~~ msglist: Show message reactions; support reacting (with 👍, or to join in others' reactions) Nov 22, 2023

chrisbobbe marked this pull request as draft November 22, 2023 21:33

gnprice reviewed Nov 23, 2023

View reviewed changes

chrisbobbe force-pushed the pr-reaction-chips-ui branch from a3a5f5a to 00d42b4 Compare November 27, 2023 21:37

gnprice mentioned this pull request Nov 27, 2023

Try bundling an emoji font #404

Closed

gnprice reviewed Nov 28, 2023

View reviewed changes

chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this pull request Nov 30, 2023

action_sheet test: Remove unnecessary binding setup

eaa1cbe

These calls only need to happen once, at the top of `main`: zulip#410 (comment)

chrisbobbe force-pushed the pr-reaction-chips-ui branch from 00d42b4 to c84040a Compare November 30, 2023 07:39

chrisbobbe marked this pull request as ready for review November 30, 2023 07:39

chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this pull request Nov 30, 2023

action_sheet test: Remove unnecessary binding setup

b855b48

These calls only need to happen once, at the top of `main`: zulip#410 (comment)

chrisbobbe force-pushed the pr-reaction-chips-ui branch from c84040a to a3d0070 Compare November 30, 2023 08:55

gnprice mentioned this pull request Dec 1, 2023

test: Generate random, increasing user IDs for example data by default #425

Merged

gnprice reviewed Dec 1, 2023

View reviewed changes

chrisbobbe mentioned this pull request Dec 1, 2023

reactions: Label text can linebreak sooner than necessary #433

Open

chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this pull request Dec 1, 2023

action_sheet test: Remove unnecessary binding setup

d9b2052

These calls only need to happen once, at the top of `main`: zulip#410 (comment)

chrisbobbe force-pushed the pr-reaction-chips-ui branch from a3d0070 to b4b6648 Compare December 1, 2023 22:37

chrisbobbe added 6 commits December 1, 2023 18:19

content [nfc]: Move tryParseEmojiCodeToUnicode out to toplevel, to ex…

56964b1

…port

test: Add userSettings default to eg.initialSnapshot

6354f28

action_sheet test: Remove unnecessary binding setup

8529e14

These calls only need to happen once, at the top of `main`: zulip#410 (comment)

action_sheet test: Remove unnecessary teardowns

12029ee

This teardown is already added in setupToMessageActionSheet, which is called near the top of every test.

msglist: Support adding a thumbs-up reaction

8f43bc6

Fixes: zulip#125

gnprice force-pushed the pr-reaction-chips-ui branch from b4b6648 to 8f43bc6 Compare December 1, 2023 23:19

gnprice merged commit 8f43bc6 into zulip:main Dec 1, 2023

chrisbobbe deleted the pr-reaction-chips-ui branch December 1, 2023 23:20

chrisbobbe mentioned this pull request Dec 15, 2023

model: Message objects can be shared by message-list models, resulting in duplicated work on them #455

Closed

		final doNotAnimate = MediaQuery.of(context).disableAnimations
		\|\| PlatformDispatcher.instance.accessibilityFeatures.reduceMotion;

	final doNotAnimate = MediaQuery.of(context).disableAnimations
	final doNotAnimate = MediaQuery.disableAnimationsOf(context)

		?.aggregated.any((reactionWithVotes) =>
		reactionWithVotes.emojiCode == '1f44d'

	return 'React with 👍'; // skip translation for now
	return 'React with 👍'; // TODO(i18n) skip translation for now

		if (defaultTargetPlatform != TargetPlatform.iOS
		&& defaultTargetPlatform != TargetPlatform.macOS) {

		TestZulipBinding.ensureInitialized();
		TestWidgetsFlutterBinding.ensureInitialized();

	// Base JSON for the one"Zulip extra emoji" reaction. Just missing user_id.
	// Base JSON for the one "Zulip extra emoji" reaction. Just missing user_id.

		final i1 = {'emoji_name': 'twocents', 'emoji_code': '181', 'reaction_type': 'realm_emoji'};
		final i2 = {'emoji_name': 'twocents', 'emoji_code': '182', 'reaction_type': 'realm_emoji'};

msglist: Show message reactions; support reacting (with 👍, or to join in others' reactions) #410

msglist: Show message reactions; support reacting (with 👍, or to join in others' reactions) #410

Uh oh!

Conversation

chrisbobbe commented Nov 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisbobbe commented Nov 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnprice commented Nov 22, 2023

Uh oh!

chrisbobbe commented Nov 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisbobbe commented Nov 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisbobbe commented Nov 22, 2023

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!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Nov 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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 Nov 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

chrisbobbe commented Nov 27, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Nov 22, 2023 •

edited

Loading

chrisbobbe commented Nov 22, 2023 •

edited

Loading

chrisbobbe commented Nov 22, 2023 •

edited

Loading

chrisbobbe commented Nov 22, 2023 •

edited

Loading

chrisbobbe Nov 27, 2023 •

edited

Loading

chrisbobbe Nov 27, 2023 •

edited

Loading

chrisbobbe commented Nov 30, 2023 •

edited

Loading