Skip to content

model [nfc]: Remove StreamColorSwatch; just use ColorSwatch<StreamColorVariant> #642

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from

Conversation

chrisbobbe
Copy link
Collaborator

This indirection was mildly helpful; it let us consume these colors
with slightly more concise code, and without needing ! to satisfy
the analyzer.

But we get the same correct functionality without the indirection,
and a bit more transparently. The more substantive benefit of using
ColorSwatch directly is that we now have a ready-made lerp function
-- ColorSwatch.lerp -- that can operate on our swatch instances.
That should be helpful later when we want stream colors to animate
smoothly when dark mode is toggled, along with various other style
attributes that will naturally animate too.


Quoting that ColorSwatch.lerp:

  /// Linearly interpolate between two [ColorSwatch]es.
  ///
  /// It delegates to [Color.lerp] to interpolate the different colors of the
  /// swatch.
  ///
  /// If either color is null, this function linearly interpolates from a
  /// transparent instance of the other color.
  ///
  /// The `t` argument represents position on the timeline, with 0.0 meaning
  /// that the interpolation has not started, returning `a` (or something
  /// equivalent to `a`), 1.0 meaning that the interpolation has finished,
  /// returning `b` (or something equivalent to `b`), and values in between
  /// meaning that the interpolation is at the relevant point on the timeline
  /// between `a` and `b`. The interpolation can be extrapolated beyond 0.0 and
  /// 1.0, so negative values and values greater than 1.0 are valid (and can
  /// easily be generated by curves such as [Curves.elasticInOut]). Each channel
  /// will be clamped to the range 0 to 255.
  ///
  /// Values for `t` are usually obtained from an [Animation<double>], such as
  /// an [AnimationController].
  static ColorSwatch<T>? lerp<T>(ColorSwatch<T>? a, ColorSwatch<T>? b, double t) {
    if (identical(a, b)) {
      return a;
    }
    final Map<T, Color> swatch;
    if (b == null) {
      swatch = a!._swatch.map((T key, Color color) => MapEntry<T, Color>(key, Color.lerp(color, null, t)!));
    } else {
      if (a == null) {
        swatch = b._swatch.map((T key, Color color) => MapEntry<T, Color>(key, Color.lerp(null, color, t)!));
      } else {
        swatch = a._swatch.map((T key, Color color) => MapEntry<T, Color>(key, Color.lerp(color, b[key], t)!));
      }
    }
    return ColorSwatch<T>(Color.lerp(a, b, t)!.value, swatch);
  }

chrisbobbe added 2 commits May 3, 2024 13:50
We'll soon make this enum public, for consuming code to use directly
to get individual colors from the swatch. Move the dartdocs now, to
simplify the diff in that upcoming change.
@chrisbobbe chrisbobbe requested a review from gnprice May 3, 2024 21:45
chrisbobbe added 2 commits May 3, 2024 14:48
…iant>

This indirection was mildly helpful; it let us consume these colors
with slightly more concise code, and without needing `!` to satisfy
the analyzer.

But we get the same correct functionality without the indirection,
and a bit more transparently. The more substantive benefit of using
ColorSwatch directly is that we now have a ready-made lerp function
-- `ColorSwatch.lerp` -- that can operate on our swatch instances.
That should be helpful later when we want stream colors to animate
smoothly when dark mode is toggled, along with various other style
attributes that will naturally animate too.
The old name seems longer than it needs to be.
@chrisbobbe chrisbobbe force-pushed the pr-stream-color-swatch branch from 3b658bd to 689ac37 Compare May 3, 2024 21:48
@chrisbobbe chrisbobbe changed the title model: Remove StreamColorSwatch; just use ColorSwatch<StreamColorVariant> model [nfc]: Remove StreamColorSwatch; just use ColorSwatch<StreamColorVariant> May 3, 2024
@gnprice
Copy link
Member

gnprice commented May 8, 2024

Hmm, it does feel like the code that's using this class generally all gets worse this way.

For the lerp, how about we just copy that method's implementation? It doesn't seem like it's doing anything all that complicated.

In order to have the map lying around, we could save it on our own private field in addition to passing it to the superclass. The redundancy is mildly annoying, but it's only duplicating one pointer, so it's not a material cost at runtime.

@chrisbobbe
Copy link
Collaborator Author

chrisbobbe commented May 9, 2024

Sure, makes sense. Since that approach will be pretty different from what's in this PR, I'll close this PR and open a new one.

-> #654

@chrisbobbe chrisbobbe closed this May 9, 2024
@chrisbobbe chrisbobbe deleted the pr-stream-color-swatch branch May 9, 2024 20:43
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this pull request May 9, 2024
It's convenient in this method to access the colors as they're
stored in a Map, like the superclass ColorSwatch does. So, in our
own private field, we save the same Map that we pass along to the
superclass constructor. As Greg says at
  zulip#642 (comment) :

> The redundancy is mildly annoying, but it's only duplicating one
> pointer, so it's not a material cost at runtime.
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this pull request May 9, 2024
It's convenient in this method to access the colors as they're
stored in a Map, like the superclass ColorSwatch does. So, in our
own private field, we save the same Map that we pass along to the
superclass constructor. As Greg says at
  zulip#642 (comment) :

> The redundancy is mildly annoying, but it's only duplicating one
> pointer, so it's not a material cost at runtime.
gnprice pushed a commit to chrisbobbe/zulip-flutter that referenced this pull request May 10, 2024
It's convenient in this method to access the colors as they're
stored in a Map, like the superclass ColorSwatch does. So, in our
own private field, we save the same Map that we pass along to the
superclass constructor. As Greg says at
  zulip#642 (comment) :

> The redundancy is mildly annoying, but it's only duplicating one
> pointer, so it's not a material cost at runtime.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants