ui: Provide [StreamColorSwatch]es via DesignVariables, not api/model/ #746

chrisbobbe · 2024-06-18T19:56:51Z

(Instead of putting them in Subscription model objects.)

I wonder if there's a better approach than this. One thing is that having separate subclasses StreamColorSwatchesLight and StreamColorSwatchesDark means some duplicated boilerplate.

Related: #95
Fixes: #393

gnprice

Thanks! Glad to get this moved out of lib/api/. Comments below.

gnprice · 2024-06-18T23:41:34Z

test/widgets/inbox_test.dart

+        testWidgets('uncollapsed header changes background color when [subscription.color] changes', (tester) async {
+          final initialColor = Colors.indigo.value;
+
+          await setupVarious(tester, sub1Color: initialColor);


Instead of setupVarious, how about just setting up the one subscription this uses and one message in that stream?

That'd make this test more self-contained — easier to see how the things it's doing relate to each other, and less potential for interference with the needs of other tests that use the same all-in-one miscellaneous setup.

gnprice · 2024-06-18T23:55:36Z

lib/widgets/stream_colors.dart

+import '../api/model/model.dart';
+import 'color.dart';
+
+/// A caching factory for [StreamColorSwatch]es, from [subscription.color] bases.


I'm not sure what a "caching factory" is. Can you try explaining this without the word "factory"?

You give it a [subscription.color], and it returns a [StreamColorSwatch] for it. If there's one cached, it returns that, otherwise it creates one, caches it, and returns it. This is done through the forBaseColor method.

(I guess "creates one" is just what I was trying to get at with "factory".)

gnprice · 2024-06-18T23:58:24Z

lib/widgets/stream_colors.dart

+  // No public constructor; always use [instance]. Empirically,
+  // [StreamColorSwatches.lerp] is called even when the theme has not changed,
+  // and the short-circuit on [identical] cuts redundant work when that happens.
+  static StreamColorSwatchesLight get instance =>


How about calling this StreamColorSwatches.light, and making this class private?

It can still work the same way, I'm imagining purely a rename. But I think that will look a bit cleaner where it's used than if the consuming code has the various different subclasses to think about.

gnprice · 2024-06-18T23:59:44Z

lib/widgets/stream_colors.dart

+  static StreamColorSwatchesLight get instance =>
+    (_instance ??= StreamColorSwatchesLight._());
+  static StreamColorSwatchesLight? _instance;


Separately, this can be simplified to:

Suggested change

static StreamColorSwatchesLight get instance =>

(_instance ??= StreamColorSwatchesLight._());

static StreamColorSwatchesLight? _instance;

static final StreamColorSwatchesLight instance = StreamColorSwatchesLight._();

which is equivalent. (A static field is initialized lazily when first accessed.)

gnprice · 2024-06-19T00:02:19Z

lib/widgets/stream_colors.dart

+  @override
+  StreamColorSwatch forBaseColor(int base) =>
+    _cache[base] ??= StreamColorSwatch.light(base);


Since the caching logic works the same in all three subclasses, it can be brought up to the base class. Then this can look like:

Suggested change

@override

StreamColorSwatch forBaseColor(int base) =>

_cache[base] ??= StreamColorSwatch.light(base);

@override

StreamColorSwatch computeForBaseColor(int base) => StreamColorSwatch.light(base);

(This is something the Flutter framework does, for things like RenderBox.computeMinIntrinsicWidth.)

gnprice · 2024-06-19T01:11:35Z

test/widgets/stream_colors_test.dart

+      test('from dark to light', () {
+        final instance = StreamColorSwatchesDark.instance
+          .lerp(StreamColorSwatchesLight.instance, 0.4);


I think this test is redundant with the one above it — there isn't any logic in the lerping that works differently between dark and light, so these are just two distinct groups of swatches whichever way around they are.

gnprice · 2024-06-19T01:13:31Z

test/widgets/theme_test.dart

+
+  group('colorSwatchFor', () {
+    void doTest(int baseColor, Brightness brightness) {
+      final description = '${Color(baseColor).toString()}, $brightness';


nit:

Suggested change

final description = '${Color(baseColor).toString()}, $brightness';

final description = '${Color(baseColor)}, $brightness';

gnprice · 2024-06-19T01:19:24Z

test/widgets/theme_test.dart

+        final expectedSwatch = switch (brightness) {
+          Brightness.light => StreamColorSwatch.light(baseColor),
+          Brightness.dark  => StreamColorSwatch.dark(baseColor),
+        };


Do you want to pass an expected parameter instead? I'm not sure how this switch (brightness) will handle the lerping case in the TODO below. (And the dark case is relevant only as part of implementing that TODO.)

chrisbobbe · 2024-06-20T22:45:28Z

Thanks for the review! Revision pushed.

gnprice

Thanks for the revision! Small comments, largely nits.

gnprice · 2024-06-21T00:33:30Z

lib/widgets/stream_colors.dart

+/// A provider for [StreamColorSwatch]es that computes and caches them.
+abstract class StreamColorSwatches {


I think "provider", like "factory", is a word I'd like to avoid here in favor of unpacking what it means 🙂

Working from your explanation at #746 (comment) , here's a version I think would be clear:

Suggested change

/// A provider for [StreamColorSwatch]es that computes and caches them.

abstract class StreamColorSwatches {

/// Computes a [StreamColorSwatch] for any given color, with caching.

abstract class StreamColorSwatches {

Perhaps better yet, here's a slightly different way of looking at it:

Suggested change

/// A provider for [StreamColorSwatch]es that computes and caches them.

abstract class StreamColorSwatches {

/// A lazily-computed map from a stream's base color to a

/// corresponding [StreamColorSwatch].

abstract class StreamColorSwatches {

Cool, yeah; I like the second one.

gnprice · 2024-06-21T00:36:50Z

lib/widgets/stream_colors.dart

+  /// This computation is cached on the instance
+  /// in order to save work building [t]'s animation frame when there are
+  /// multiple UI elements using the same [subscription.color].
+  StreamColorSwatches lerp(StreamColorSwatches? other, double t) {


Perhaps make this static? That seems to be the convention elsewhere, or at least on Color.

(I had this thought when looking at the top of the class declaration, where there's light and dark and what one basically wants to say is that we have three kinds of instances of this class: light, dark, and lerps between them.)

Sure, I can try that. I think I did non-static because that's what ThemeExtension does. (CodeBlockTextStyles came out non-static for that reason as well, I think.)

TextStyle's lerp is also static.

…Hmm, although I guess maybe CodeBlockTextStyles and StreamColorSwatches are more like ThemeExtension than like Color and TextStyle?

One thing about CodeBlockTextStyles and StreamColorSwatches is that their lerps are only ever called by a ThemeExtension's lerp. In that lerp, the a instance (i.e., this) is non-nullable, and that's convenient for _StreamColorSwatchesLerped.computeForBaseColor because it makes it easy to reason locally that its non-nullable StreamColorSwatch return type is correct.

I think I did non-static because that's what ThemeExtension does.

Hmm I see.

I just did a quick survey of the Flutter tree. It looks like the pattern is very consistent, and ThemeExtension is an outlier. So let's follow the typical pattern and make it static.

Specifically the search I did was, in the Flutter repo:

$ git grep ' lerp[<(]' packages/flutter/lib/

That finds definitions of a method named lerp, static or otherwise.

There are a number of non-static lerp methods… on Tween subclasses and the like, that look like this:

packages/flutter/lib/src/rendering/tweens.dart 27: FractionalOffset? lerp(double t) => FractionalOffset.lerp(begin, end, t); 50: Alignment lerp(double t) => Alignment.lerp(begin, end, t)!; 75: AlignmentGeometry? lerp(double t) => AlignmentGeometry.lerp(begin, end, t);

So e.g. AlignmentTween.lerp is non-static; but it's not a lerp between AlignmentTweens, rather a lerp between Alignments, and in fact it forwards to the static Alignment.lerp.

There are something like 90 static lerp methods that take both endpoints as arguments, and like 40 non-static lerp methods that take neither as arguments, getting them both from the receiver (as with those Tween subclasses). And I think ThemeExtension is the sole example of its pattern, where it gets one endpoint from the receiver and the other as an argument.

(This also means that that API choice on ThemeExtension was basically a mistake.)

Thanks for that investigation!

gnprice · 2024-06-21T00:39:33Z

lib/widgets/stream_colors.dart

+  /// Gives the cached [StreamColorSwatch] for a [subscription.color].
+  StreamColorSwatch forBaseColor(int base) =>


Suggested change

/// Gives the cached [StreamColorSwatch] for a [subscription.color].

StreamColorSwatch forBaseColor(int base) =>

/// Gives the [StreamColorSwatch] for a [subscription.color].

StreamColorSwatch forBaseColor(int base) =>

I wouldn't say this gives "the cached swatch", because there may not have been a cached swatch yet when it was called.

gnprice · 2024-06-21T00:40:02Z

lib/widgets/stream_colors.dart

+  StreamColorSwatch forBaseColor(int base) =>
+    _cache[base] ??= computeForBaseColor(base);
+
+  StreamColorSwatch computeForBaseColor(int base);


Could also make this private, as nothing should be calling it directly other than the implementation of forBaseColor.

gnprice · 2024-06-21T00:44:10Z

lib/widgets/stream_colors.dart

+
+/// The [StreamColorSwatches] for the light theme.
+class _StreamColorSwatchesLight extends StreamColorSwatches {
+  _StreamColorSwatchesLight._();


nit:

Suggested change

_StreamColorSwatchesLight._();

_StreamColorSwatchesLight();

Has the same effect since the class is already private, and makes the syntax a bit cleaner at the call site.

gnprice · 2024-06-21T00:44:51Z

lib/widgets/stream_colors.dart

+/// The [StreamColorSwatches] for the light theme.
+class _StreamColorSwatchesLight extends StreamColorSwatches {


nit: move this doc to the static instance, which is public

chrisbobbe · 2024-06-21T19:23:08Z

Thanks for the review! Revision pushed, and I replied above: #746 (comment)

gnprice · 2024-06-21T22:31:53Z

Thanks! All LGTM except that one open thread above.

I'll go ahead and merge this, since that's something straightforward to fix up separately. Then as a followup let's fix both this lerp method and the existing other lerp methods to be static, as discussed in that thread #746 (comment) .

I believe these stopped applying in ebff35f, and they're confusing; remove them.

I started this by writing streamHeaderBackgroundColor just for an upcoming test (checking the stream header responds to update-stream-color events). But it was a nice opportunity to complete some existing TODOs.

This will help verify the upcoming refactor, zulip#393 (storing stream-color variants in DesignVariables instead of Subscription model objects).

We'll want to get these colors from theme data, soon, for zulip#95 (supporting dark theme).

This should help the tests be more representative generally; but in particular, ZulipApp is going to start providing the stream color swatches, instead of code in api/model, for zulip#393. So, we have the test setup use ZulipApp so that we don't get a crash in InboxPage. Related: zulip#393

Just like we did for the InboxPage tests, in the previous commit. Here, there's one test that failed on the assumption that a particular icon was the only one in view; it apparently isn't anymore (probably because of the page's back button), and that assumption isn't important to the goal of the test. So, we adapt by removing that assumption. Related: zulip#393

…ets/ Toward zulip#393, "model: Track/memoize stream-color variants somewhere other than /api/model". But this only moves definitions; we'll reassign the caching/computing logic in an upcoming commit. Related: zulip#393

Instead of putting them in Subscription model objects. This helps toward zulip#95 by bundling this with our other ThemeExtensions that will switch together between light and dark variants when the theme changes. It's also just nicer to separate this very UI-focused code out of api/model/ (zulip#393). Not marking [nfc] just because of differences in caching logic, although I don't expect a user-visible perf effect. Related: zulip#95 Fixes: zulip#393

chrisbobbe · 2024-06-21T22:45:33Z

Cool; sure, I'll follow up with a fix to make them static.

Discussion: zulip#746 (comment)

chrisbobbe added a-model Implementing our data model (PerAccountStore, etc.) integration review Added by maintainers when PR may be ready for integration labels Jun 18, 2024

chrisbobbe requested a review from gnprice June 18, 2024 19:56

gnprice reviewed Jun 19, 2024

View reviewed changes

chrisbobbe force-pushed the pr-stream-color-swatches-design-variables branch from acb9654 to e81f84e Compare June 20, 2024 22:45

gnprice reviewed Jun 21, 2024

View reviewed changes

chrisbobbe force-pushed the pr-stream-color-swatches-design-variables branch from e81f84e to 48f282c Compare June 21, 2024 19:22

chrisbobbe added 9 commits June 21, 2024 15:31

inbox test [nfc]: Cut/fix some outdated comments about StickyHeaderItems

f197242

I believe these stopped applying in ebff35f, and they're confusing; remove them.

inbox test: Test for all-DMs header and stream header background color

10ced60

I started this by writing streamHeaderBackgroundColor just for an upcoming test (checking the stream header responds to update-stream-color events). But it was a nice opportunity to complete some existing TODOs.

inbox test: Test that stream header changes color on color-change event

a9cd5ba

This will help verify the upcoming refactor, zulip#393 (storing stream-color variants in DesignVariables instead of Subscription model objects).

inbox [nfc]: Turn some color getters into functions that take context

e45d78a

We'll want to get these colors from theme data, soon, for zulip#95 (supporting dark theme).

msglist [nfc]: Pull out a variable to minimize an upcoming diff

1686ad5

gnprice force-pushed the pr-stream-color-swatches-design-variables branch from 48f282c to fba909b Compare June 21, 2024 22:32

gnprice merged commit fba909b into zulip:main Jun 21, 2024
1 check passed

chrisbobbe deleted the pr-stream-color-swatches-design-variables branch June 21, 2024 22:43

chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this pull request Jun 21, 2024

code_block [nfc]: Make CodeBlockTextStyles.lerp static

d47ec93

Discussion: zulip#746 (comment)

chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this pull request Jun 21, 2024

stream_colors [nfc]: Make StreamColorSwatches.lerp static

e4159f3

Discussion: zulip#746 (comment)

chrisbobbe mentioned this pull request Jun 21, 2024

Make StreamColorSwatches.lerp and CodeBlockTextStyles.lerp static #759

Merged

chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this pull request Jun 26, 2024

code_block [nfc]: Make CodeBlockTextStyles.lerp static

c6e43cb

Discussion: zulip#746 (comment)

chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this pull request Jun 26, 2024

stream_colors [nfc]: Make StreamColorSwatches.lerp static

d361112

Discussion: zulip#746 (comment)

chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this pull request Jun 26, 2024

code_block [nfc]: Make CodeBlockTextStyles.lerp static

b287770

Discussion: zulip#746 (comment)

chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this pull request Jun 26, 2024

stream_colors [nfc]: Make StreamColorSwatches.lerp static

0399af6

Discussion: zulip#746 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ui: Provide [StreamColorSwatch]es via DesignVariables, not api/model/ #746

ui: Provide [StreamColorSwatch]es via DesignVariables, not api/model/ #746

chrisbobbe commented Jun 18, 2024

gnprice left a comment

gnprice Jun 18, 2024

gnprice Jun 18, 2024

chrisbobbe Jun 20, 2024 •

edited

Loading

gnprice Jun 18, 2024

gnprice Jun 18, 2024

gnprice Jun 19, 2024

gnprice Jun 19, 2024

gnprice Jun 19, 2024

gnprice Jun 19, 2024

chrisbobbe commented Jun 20, 2024

gnprice left a comment

gnprice Jun 21, 2024

chrisbobbe Jun 21, 2024

gnprice Jun 21, 2024

chrisbobbe Jun 21, 2024

chrisbobbe Jun 21, 2024

gnprice Jun 21, 2024

gnprice Jun 21, 2024

chrisbobbe Jun 21, 2024

gnprice Jun 21, 2024

gnprice Jun 21, 2024

gnprice Jun 21, 2024

gnprice Jun 21, 2024

chrisbobbe commented Jun 21, 2024

gnprice commented Jun 21, 2024

chrisbobbe commented Jun 21, 2024

	final description = '${Color(baseColor).toString()}, $brightness';
	final description = '${Color(baseColor)}, $brightness';

		/// A provider for [StreamColorSwatch]es that computes and caches them.
		abstract class StreamColorSwatches {

		/// Gives the cached [StreamColorSwatch] for a [subscription.color].
		StreamColorSwatch forBaseColor(int base) =>

		/// The [StreamColorSwatches] for the light theme.
		class _StreamColorSwatchesLight extends StreamColorSwatches {

ui: Provide [StreamColorSwatch]es via DesignVariables, not api/model/ #746

ui: Provide [StreamColorSwatch]es via DesignVariables, not api/model/ #746

Conversation

chrisbobbe commented Jun 18, 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

chrisbobbe Jun 20, 2024 • edited Loading

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 Jun 20, 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

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 Jun 21, 2024

gnprice commented Jun 21, 2024

chrisbobbe commented Jun 21, 2024

chrisbobbe Jun 20, 2024 •

edited

Loading