widgets: Pull out reusable UnreadCountBadge widget; grow support for stream colors #371

chrisbobbe · 2023-11-08T22:12:29Z

With an experiment in golden tests. As Greg pointed out in today's weekly mobile-team call, golden tests might not provide much extra value. But anyway, here's an experiment with them, in case it's helpful now or for our future testing needs. 🙂

cc @sirpengi, who might like to use this for #187

chrisbobbe · 2023-11-09T14:56:24Z

Oh, hmm, the goldens are failing in CI. They're not failing when I run them on my machine though.

Maybe another reason that golden tests aren't worth the bother?

gnprice · 2023-11-09T20:21:49Z

Chat thread on golden tests: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/golden.20tests/near/1679246

gnprice

Thanks! Just two comments below, apart from the tests — as discussed in that chat thread, we'll skip golden tests here.

I think it'll make sense to go fairly light on tests for this widget. The color calculation will be good for some unit tests; can pull it out as a @visibleForTesting static on the class. Probably none of the rest of the logic really makes sense to test, though. Could have a smoke test that just pumps one of these widgets to confirm nothing crashes.

gnprice · 2023-11-09T20:37:58Z

lib/widgets/unread_count_badge.dart

+      // TODO try LCH; see linked discussion
+      // TODO profiling for expensive computation
+      final asLab = LabColor.fromColor(baseStreamColor!);
+      backgroundColor = asLab
+        .copyWith(lightness: asLab.lightness.clamp(30, 70))
+        .toColor()


Cool, this sounds good for the present.

In the future if we discover this is a hot spot, we can either (a) find or write a more efficient implementation (as discussed in this chat thread, I think the amount of code needed for these isn't a lot; and I haven't looked closely but it may be that not much computation is needed either), or (b) if that's hard for some reason, memoize the results, perhaps on the Subscription objects.

gnprice · 2023-11-09T20:56:13Z

lib/widgets/unread_count_badge.dart

+    super.key,
+    required this.count,
+    this.bold = false,
+    this.baseStreamColor,


Suggested change

this.baseStreamColor,

required this.baseStreamColor,

I think it's not much of a burden for places like the recent-DMs list to explicitly pass null here; and making it explicit will help avoid forgetting to pass a stream color when there is a relevant stream.

chrisbobbe · 2023-11-10T15:48:55Z

Thanks for the review! Revision pushed.

gnprice

Thanks! Small comments in the tests; otherwise all looks good.

gnprice · 2023-11-13T20:29:55Z

test/widgets/unread_count_badge_test.dart

+          child: UnreadCountBadge(count: 1, baseStreamColor: null)),
+      );


Suggested change

child: UnreadCountBadge(count: 1, baseStreamColor: null)),

);

child: UnreadCountBadge(count: 1, baseStreamColor: null)));

tester.widget(find.text("1"));

That just helps the reader not wonder if it's possible that one frame wasn't enough to fully show the widget. (Which only tends to come up in the presence of our GlobalStoreWidget and PerAccountStoreWidget; but we do use those a lot, so it comes to mind as something that can happen.)

gnprice · 2023-11-13T20:37:59Z

test/widgets/unread_count_badge_test.dart

+      // TODO Fix bug causing our implementation's results to differ from the
+      //  replit's. Where they differ, see comment with what the replit gives.


Cool. How did you extract the replit's results?

I forked the replit (link to the replit) and added some code to run all the colors through getCounterBackgroundColor (since that's what gives the background color for the counters in the replit) and print the results to the console.

Cool. Is it easy to link to that forked replit?

That just seems like it'll be helpful whenever someone (be it you a few months in the future, or someone else) goes to try to resolve this TODO.

Unfortunately I deleted the fork. I couldn't easily figure out version control, and I didn't want to let my future self mix up which code was Vlad's vs. mine.

OK. I added a link to your previous reply, just in the interest of making the information we do have handy now remain handy when someone takes up this TODO.

gnprice · 2023-11-13T20:41:36Z

test/widgets/unread_count_badge_test.dart

+      runCheck(const Color(0xFFFFFF00), const Color(0x4dacb300)); // 0x4dacb200
+      runCheck(const Color(0xFFFF0000), const Color(0x4dff0000));
+      runCheck(const Color(0xFF008000), const Color(0x4d008000));
+      runCheck(const Color(0xFF0000FF), const Color(0x4d0000ff)); // 0x4d0902ff


It looks like this difference of 0x00 vs 0x09 might be the biggest in any channel in any of these test cases, and all the others I spotted were a difference of 1 or 2. Is that right?

If it's almost always 1 or 2, that seems pretty mild, though it'd still ultimately be good to track down and resolve.

I skimmed through them just now, looking for any difference bigger than 0x09. I didn't find one.

Then I realized you also asked if all the rest just had differences of 1 or 2, and I wasn't keeping track of those…I think there were a few that had a difference of 3. Here's one with a difference of 3 and of 5:

runCheck(const Color(0xFF000080), const Color(0x4d492bae)); // 0x4d4b2eb3

We'll use this to follow the Figma spec for the Inbox view, zulip#117.

chrisbobbe · 2023-11-15T05:36:01Z

Thanks for the review! Revision pushed.

…t/end This doesn't seem likely to become asymmetrical, and perhaps we can save a bit of computation with this.

gnprice · 2023-11-15T20:10:35Z

Thanks! Merging, with just an added couple of comment lines per #371 (comment) above.

chrisbobbe · 2023-11-15T20:18:28Z

Thanks!

chrisbobbe requested a review from gnprice November 8, 2023 22:12

gnprice reviewed Nov 9, 2023

View reviewed changes

chrisbobbe force-pushed the pr-unread-count-badge branch from c31fec6 to 48b1fea Compare November 10, 2023 15:48

chrisbobbe mentioned this pull request Nov 13, 2023

inbox: Add "Inbox" page! #381

Merged

gnprice reviewed Nov 13, 2023

View reviewed changes

chrisbobbe added 3 commits November 15, 2023 00:27

widgets: Pull out reusable UnreadCountBadge widget

d9eb0d3

widgets: Give UnreadCountBadge a bold param

a73175f

We'll use this to follow the Figma spec for the Inbox view, zulip#117.

deps: Add flutter_color_models

d990df5

chrisbobbe force-pushed the pr-unread-count-badge branch from 928738e to 539ef32 Compare November 15, 2023 05:35

chrisbobbe added 2 commits November 15, 2023 12:08

widgets: Implement stream-colored UnreadCountBadge

0e33d57

widgets: Remove unnecessary EdgeInsetsDirectional with symmetric star…

255fff3

…t/end This doesn't seem likely to become asymmetrical, and perhaps we can save a bit of computation with this.

gnprice force-pushed the pr-unread-count-badge branch from 539ef32 to 255fff3 Compare November 15, 2023 20:10

gnprice merged commit 255fff3 into zulip:main Nov 15, 2023
1 check passed

chrisbobbe deleted the pr-unread-count-badge branch November 15, 2023 20:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

widgets: Pull out reusable UnreadCountBadge widget; grow support for stream colors #371

widgets: Pull out reusable UnreadCountBadge widget; grow support for stream colors #371

chrisbobbe commented Nov 8, 2023 •

edited

Loading

chrisbobbe commented Nov 9, 2023 •

edited

Loading

gnprice commented Nov 9, 2023

gnprice left a comment

gnprice Nov 9, 2023

gnprice Nov 9, 2023

chrisbobbe commented Nov 10, 2023

gnprice left a comment

gnprice Nov 13, 2023

gnprice Nov 13, 2023

chrisbobbe Nov 15, 2023

gnprice Nov 15, 2023

chrisbobbe Nov 15, 2023

gnprice Nov 15, 2023

gnprice Nov 13, 2023

chrisbobbe Nov 15, 2023

chrisbobbe commented Nov 15, 2023

gnprice commented Nov 15, 2023

chrisbobbe commented Nov 15, 2023

		child: UnreadCountBadge(count: 1, baseStreamColor: null)),
		);

-          child: UnreadCountBadge(count: 1, baseStreamColor: null)),
-      );
+          child: UnreadCountBadge(count: 1, baseStreamColor: null)));
+      tester.widget(find.text("1"));

		// TODO Fix bug causing our implementation's results to differ from the
		// replit's. Where they differ, see comment with what the replit gives.

widgets: Pull out reusable UnreadCountBadge widget; grow support for stream colors #371

widgets: Pull out reusable UnreadCountBadge widget; grow support for stream colors #371

Conversation

chrisbobbe commented Nov 8, 2023 • edited Loading

chrisbobbe commented Nov 9, 2023 • edited Loading

gnprice commented Nov 9, 2023

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

chrisbobbe commented Nov 10, 2023

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

chrisbobbe commented Nov 15, 2023

gnprice commented Nov 15, 2023

chrisbobbe commented Nov 15, 2023

chrisbobbe commented Nov 8, 2023 •

edited

Loading

chrisbobbe commented Nov 9, 2023 •

edited

Loading