emoji_reaction: Add EmojiReactionTheme, including dark variant #805

chrisbobbe · 2024-07-12T00:32:47Z

Screenshot (also showing some unrelated tweaks toward dark-theme that will land separately):

I thought it might be helpful to have a new ThemeExtension for this part of the UI instead of adding all these values to DesignVariables. This way at least we don't have to alphabetize these items along with a lot of attributes from unrelated parts of the UI. I see that we're taking the DesignVariables approach in #762, though, and it's possible that that'll end up being preferred for some reason.

Related: #95

gnprice · 2024-07-12T05:10:05Z

Huh, when I run tests locally at the tip of this, they fail — I think on exactly the RTL half of the cases:

$ flutter test test/widgets/emoji_reaction_test.dart 
00:02 +0: ReactionChipsList smoke (same reaction, different users, with one unknown user): show names when few / text / rtl / text scale: 0.8235
══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞════════════════════════════════════════════════════
The following TestFailure was thrown running a test:
Expected: a TextDirection that:
  equals <TextDirection.rtl>
Actual: <TextDirection.ltr>
Which: are not equal

When the exception was thrown, this was the stack:
#0      check.<anonymous closure> (package:checks/src/checks.dart:85:9)
#1      _TestContext.expect (package:checks/src/checks.dart:708:12)
#2      CoreChecks.equals (package:checks/src/extensions/core.dart:90:13)
#3      main.<anonymous closure>.setupChipsInBox (file:///home/greg/z/flutterz/test/widgets/emoji_reaction_test.dart:74:51)
<asynchronous suspension>
#4      main.<anonymous closure>.runSmokeTest.<anonymous closure> (file:///home/greg/z/flutterz/test/widgets/emoji_reaction_test.dart:132:17)
[…]

That begins at the "Set up whole ZulipApp" commit. It persists even after I run flutter pub get (which I believe should be redundant, I believe flutter test does that).

Clearly it works for you and in CI. So presumably there's something that's getting cached for me locally — like the list of locales, perhaps, so that it doesn't know about ar.

Persists also if I run flutter build apk --config-only, which I would have expected to take care of generating any files it was going to generate.

chrisbobbe · 2024-07-12T05:12:08Z

What does ZulipLocalizations.supportedLocales look like for you?

gnprice · 2024-07-12T05:13:48Z

[en, ja]

chrisbobbe · 2024-07-12T05:14:44Z

Hmm, mine was auto-updating to include ar with flutter run. Or anyway the "play" button in Android Studio, which does flutter run.

gnprice · 2024-07-12T05:17:26Z

Cool, yeah, flutter run does it.

I think that probably means flutter gen-l10n will do it. Ah indeed, and our docs say this:

Then run the app (with flutter run or in your IDE),
or perform a hot reload,
to cause the Dart bindings to be updated based on your
changes to the ARB file.
(You can also trigger an update directly, with flutter gen-l10n.)

I guess we'll have to warn everyone to do that after this gets merged.

gnprice · 2024-07-12T05:18:29Z

(Probably this should be thought of as a bug in the flutter tool.)

gnprice

(Haven't yet read the main commit at the end.)

gnprice · 2024-07-12T05:28:21Z

test/widgets/emoji_reaction_test.dart

+      addTearDown(tester.platformDispatcher.clearLocaleTestValue);
+      addTearDown(tester.platformDispatcher.clearLocalesTestValue);
+
+      await tester.pumpWidget(const ZulipApp());


This commit turns out to make these tests quite a bit slower:

# before: $ time flutter test test/widgets/emoji_reaction_test.dart 00:05 +160: All tests passed! time: 6.078s wall (7.313s u, 0.940s s) # after: $ time flutter test test/widgets/emoji_reaction_test.dart 00:09 +160: All tests passed! time: 10.528s wall (12.424s u, 1.321s s)

The slowdown happens with this commit:
fa3f012 emoji_reaction test: Set up whole ZulipApp, not just MaterialApp

It'd be good to try to pin down where that time is going. The strategy of reusing ZulipApp in tests makes a lot of sense in principle, so we're likely to try using it in more and more tests… and I'd like to figure out how to do so without slowing all our tests down by 75%.

The strategy I'd try is to just start commenting out parts of ZulipApp's functionality, and rerunning this one test file, and see which pieces are contributing the time.

Then once we know which pieces are at issue, we can see about finding hacks for tests to cut those out, or strip them down to a minimum.

gnprice

Thanks @chrisbobbe! Generally this looks good.

The same two small comments as at theme: Implement DesignVariables.dark #804 apply here.
Let's also investigate that test slowdown above, and see if we can find an easy workaround for it.

And then repeating here just so the action items are in one place:

On merging this, we'll need to make a PSA in #mobile-team that people need to run flutter gen-l10n (or just flutter run) after rebasing in order to get tests working again.
- And as a bonus followup it'd be nice to find or file an upstream flutter-tool issue about the need for that.

gnprice · 2024-07-12T05:39:06Z

I thought it might be helpful to have a new ThemeExtension for this part of the UI instead of adding all these values to DesignVariables. This way at least we don't have to alphabetize these items along with a lot of attributes from unrelated parts of the UI. I see that we're taking the DesignVariables approach in #762, though, and it's possible that that'll end up being preferred for some reason.

Yeah, I'm not sure. Happy to have both approaches going, and we can see which we like better and optionally refactor later to a single approach.

Greg noticed an instance where using ZulipApp directly was causing tests to slow down by some 75%: zulip#805 (comment) Specifically, that was when we converted the emoji-reaction widget tests over to use ZulipApp instead of mimicking it with a MaterialApp, etc. It's helpful to use something approaching the real ZulipApp. But we'd like to avoid that slowdown, especially if it's happening in code that most tests don't need to exercise. So, here's a stripped-down version that does that. It takes roughly the same time as the MaterialApp approach in those emoji-reaction tests. (11s on my machine, vs. 16s with ZulipApp.) This commit just converts the tests that were using ZulipApp. Next, we'll sweep through and treat the ones using the ad hoc MaterialApp approach.

chrisbobbe · 2024-07-16T23:58:23Z

Thanks for the review! Revision pushed.

gnprice

Thanks! Here's a partial review before I switch to another task.

Small comments below, and I tried out the performance effects of these two commits:
0a2b459 test: Add TestZulipApp; replace uses of ZulipApp
c026fce test: Use TestZulipApp everywhere else it makes sense

on flutter test test/widgets on my machine, measuring the fastest of three runs.

Before:
time: 15.837s wall (68.737s u, 9.182s s)

After replacing ZulipApp uses:
time: 15.389s wall (64.851s u, 8.641s s)

After using TestZulipApp more widely:
time: 15.684s wall (64.010s u, 9.070s s)

So replacing ZulipApp made a small speedup, about 7% less total CPU time. Then using TestZulipApp more widely had roughly zero impact, as hoped.

I haven't yet read the last four commits (or revisions to them, at least) in detail:
c026fce test: Use TestZulipApp everywhere else it makes sense
5967ff0 emoji_reaction test [nfc]: Move some setup code out next to similar code
309cff5 emoji_reaction test: Add another convenient test-setup check
7dd90d1 emoji_reaction: Add EmojiReactionTheme, including dark variant

gnprice · 2024-07-17T03:11:05Z

lib/widgets/emoji_reaction.dart

+  factory EmojiReactionTheme.light() {
+    return EmojiReactionTheme._(


bump on redirecting constructor (one of those two #804 comments I mentioned)

gnprice · 2024-07-17T03:13:45Z

test/widgets/test_app.dart

+    this.navigatorObservers,
+    this.child = const Placeholder(),


nit: this final ordering is good but the reordering should get squashed into the commit that introduces the widget

gnprice · 2024-07-17T03:16:17Z

test/widgets/text_test.dart

-        await tester.pumpAndSettle();
+        await tester.pump();


Nice that a lot of pumpAndSettle calls seem to become unnecessary in favor of just pump.

gnprice · 2024-07-19T19:46:54Z

Noting here to help us remember it: #827 introduces another spot that could benefit from TestZulipApp:
#827 (comment)

so whichever of these PRs lands first, we can update that spot in the other PR.

edit: Also #835.

gnprice

OK, and here's the rest of a review on this revision. Generally looks great — several nits, and two other comments on tests which should be quick to fix.

gnprice · 2024-07-19T19:56:43Z

test/widgets/app_test.dart

-          supportedLocales: ZulipLocalizations.supportedLocales,
-          home: GlobalStoreWidget(
-            child: ChooseAccountPage())));
+      await tester.pumpWidget(const TestZulipApp(child: ChooseAccountPage()));


nit: the specific page is the main interesting bit here, and I think a bit easier to spot if it starts a line:

Suggested change

await tester.pumpWidget(const TestZulipApp(child: ChooseAccountPage()));

await tester.pumpWidget(const TestZulipApp(

child: ChooseAccountPage()));

gnprice · 2024-07-19T20:04:08Z

test/widgets/emoji_reaction_test.dart

-          home: Directionality(
-            textDirection: textDirection ?? TextDirection.ltr,


nit: currently these get turned into the tester.platformDispatcher logic in the big conversion commit:
c026fce test: Use TestZulipApp everywhere else it makes sense

Probably that'd be better as a separate commit coming after that one, since it's different from the otherwise-repetitive other changes in the commit.

gnprice · 2024-07-19T20:07:27Z

test/widgets/text_test.dart

+        tester.platformDispatcher.accessibilityFeaturesTestValue =
+          FakeAccessibilityFeatures(boldText: platformRequestsBold);
+        addTearDown(tester.platformDispatcher.clearAccessibilityFeaturesTestValue);


This seems fine to include in the same commit, though. It's fairly straightforwardly just another way of expressing the effect of that MediaQuery.

gnprice · 2024-07-19T20:09:46Z

test/widgets/theme_test.dart

+        await tester.pump();
+        final buttonTextFinder = find.text(buttonText);
+        final context = tester.element(buttonTextFinder);
+        final text = tester.renderObject<RenderParagraph>(buttonTextFinder).text;


small simplification:

Suggested change

final text = tester.renderObject<RenderParagraph>(buttonTextFinder).text;

final text = (context.renderObject as RenderParagraph).text;

Then also buttonTextFinder is used just once and can be inlined.

also nit: reorder this line to just before the check, after computing the expected values; or perhaps even just inline it into the check call

gnprice · 2024-07-19T20:19:26Z

test/widgets/emoji_reaction_test.dart

@@ -204,6 +215,45 @@ void main() {
        }
      }
    }
+
+    testWidgets('Smoke test for light/dark/lerped', (tester) async {


nit: nested in unintended group

The 'ReactionChipsList' group? It uses some helpers that are in that group. Those could be moved out of the group, though.

Yeah, this test didn't seem to be about the ReactionChipsList widget.

I guess that is the widget that test helper is setting up, though, so maybe the grouping is appropriate.

gnprice · 2024-07-19T20:20:13Z

test/widgets/emoji_reaction_test.dart

+      await prepare();
+      await store.addUsers([eg.selfUser, eg.otherUser]);
+
+      debugFollowPlatformBrightness = true;


needs teardown

Greg noticed an instance where using ZulipApp directly was causing tests to slow down by some 75%: zulip#805 (comment) Specifically, that was when we converted the emoji-reaction widget tests over to use ZulipApp instead of mimicking it with a MaterialApp, etc. It's helpful to use something approaching the real ZulipApp. But we'd like to avoid that slowdown, especially if it's happening in code that most tests don't need to exercise. So, here's a stripped-down version that does that. It takes roughly the same time as the MaterialApp approach in those emoji-reaction tests. (11s on my machine, vs. 16s with ZulipApp.) This commit just converts the tests that were using ZulipApp. Next, we'll sweep through and treat the ones using the ad hoc MaterialApp approach.

chrisbobbe · 2024-07-26T01:47:41Z

Thanks for the review! Revision pushed.

This will let us simplify how we simulate RTL in the emoji-reaction widget tests, coming up. Fixes: zulip#803

We're about to refactor the way RTL gets applied. While we're at it, also check the width of the ReactionChipsList element.

Greg noticed an instance where using ZulipApp directly was causing tests to slow down by some 75%: zulip#805 (comment) Specifically, that was when we converted the emoji-reaction widget tests over to use ZulipApp instead of mimicking it with a MaterialApp, etc. It's helpful to use something approaching the real ZulipApp. But we'd like to avoid that slowdown, especially if it's happening in code that most tests don't need to exercise. So, here's a stripped-down version that does that. It takes roughly the same time as the MaterialApp approach in those emoji-reaction tests. (11s on my machine, vs. 16s with ZulipApp.) This commit just converts the tests that were using ZulipApp. Next, we'll sweep through and treat the ones using the ad hoc MaterialApp approach.

Related: zulip#95

gnprice · 2024-07-26T21:10:48Z

Thanks! Looks good — merging.

chrisbobbe added the integration review Added by maintainers when PR may be ready for integration label Jul 12, 2024

gnprice reviewed Jul 12, 2024

View reviewed changes

chrisbobbe force-pushed the pr-emoji-reaction-dark-theme branch from cf3ef49 to 7dd90d1 Compare July 16, 2024 23:58

gnprice reviewed Jul 17, 2024

View reviewed changes

gnprice mentioned this pull request Jul 19, 2024

Move markNarrowAsRead to new "actions" file; port most of its tests #827

Merged

gnprice reviewed Jul 19, 2024

View reviewed changes

chrisbobbe force-pushed the pr-emoji-reaction-dark-theme branch 2 times, most recently from 3a4af4d to 2f804d5 Compare July 26, 2024 01:47

chrisbobbe added 9 commits July 26, 2024 12:48

i18n: Add app_ar.arb file

9a846ef

This will let us simplify how we simulate RTL in the emoji-reaction widget tests, coming up. Fixes: zulip#803

emoji_reaction test [nfc]: Refactor helper default params

551c61e

emoji_reaction test: Check that RTL layout takes effect in test setup

eaa2a68

We're about to refactor the way RTL gets applied. While we're at it, also check the width of the ReactionChipsList element.

test: Use TestZulipApp everywhere else it makes sense

2253978

emoji_reaction test: Set up text direction more realistically

ccefc0e

emoji_reaction test [nfc]: Move some setup code out next to similar code

5605e2e

emoji_reaction test: Add another convenient test-setup check

ae476b2

emoji_reaction test [nfc]: Move some helper logic out of its group

0bae1dc

emoji_reaction: Add EmojiReactionTheme, including dark variant

6324bf3

Related: zulip#95

gnprice force-pushed the pr-emoji-reaction-dark-theme branch from 2f804d5 to 6324bf3 Compare July 26, 2024 21:10

gnprice merged commit 6324bf3 into zulip:main Jul 26, 2024

chrisbobbe deleted the pr-emoji-reaction-dark-theme branch July 26, 2024 21:12

This was referenced Jul 26, 2024

autocomplete: Introduce topic input autocomplete #627

Merged

compose_box: Replace compose box with a banner in DMs with deactivated users #816

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

emoji_reaction: Add EmojiReactionTheme, including dark variant #805

emoji_reaction: Add EmojiReactionTheme, including dark variant #805

chrisbobbe commented Jul 12, 2024 •

edited

Loading

gnprice commented Jul 12, 2024

chrisbobbe commented Jul 12, 2024

gnprice commented Jul 12, 2024

chrisbobbe commented Jul 12, 2024 •

edited

Loading

gnprice commented Jul 12, 2024

gnprice commented Jul 12, 2024

gnprice left a comment

gnprice Jul 12, 2024

gnprice left a comment

gnprice commented Jul 12, 2024

chrisbobbe commented Jul 16, 2024

gnprice left a comment

gnprice Jul 17, 2024

gnprice Jul 17, 2024

gnprice Jul 17, 2024

gnprice commented Jul 19, 2024 •

edited

Loading

gnprice left a comment

gnprice Jul 19, 2024

gnprice Jul 19, 2024

gnprice Jul 19, 2024

gnprice Jul 19, 2024

gnprice Jul 19, 2024

gnprice Jul 19, 2024

chrisbobbe Jul 26, 2024

gnprice Jul 26, 2024

gnprice Jul 19, 2024

chrisbobbe commented Jul 26, 2024

gnprice commented Jul 26, 2024

		factory EmojiReactionTheme.light() {
		return EmojiReactionTheme._(

	await tester.pumpWidget(const TestZulipApp(child: ChooseAccountPage()));
	await tester.pumpWidget(const TestZulipApp(
	child: ChooseAccountPage()));

		home: Directionality(
		textDirection: textDirection ?? TextDirection.ltr,

	final text = tester.renderObject<RenderParagraph>(buttonTextFinder).text;
	final text = (context.renderObject as RenderParagraph).text;

emoji_reaction: Add EmojiReactionTheme, including dark variant #805

emoji_reaction: Add EmojiReactionTheme, including dark variant #805

Conversation

chrisbobbe commented Jul 12, 2024 • edited Loading

gnprice commented Jul 12, 2024

chrisbobbe commented Jul 12, 2024

gnprice commented Jul 12, 2024

chrisbobbe commented Jul 12, 2024 • edited Loading

gnprice commented Jul 12, 2024

gnprice commented Jul 12, 2024

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

gnprice commented Jul 12, 2024

chrisbobbe commented Jul 16, 2024

gnprice 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

gnprice commented Jul 19, 2024 • edited Loading

gnprice 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

chrisbobbe commented Jul 26, 2024

gnprice commented Jul 26, 2024

chrisbobbe commented Jul 12, 2024 •

edited

Loading

chrisbobbe commented Jul 12, 2024 •

edited

Loading

gnprice commented Jul 19, 2024 •

edited

Loading