Skip to content
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

model: Track/memoize stream-color variants somewhere other than /api/model #393

Closed
chrisbobbe opened this issue Nov 18, 2023 · 5 comments · Fixed by #746
Closed

model: Track/memoize stream-color variants somewhere other than /api/model #393

chrisbobbe opened this issue Nov 18, 2023 · 5 comments · Fixed by #746
Labels
a-model Implementing our data model (PerAccountStore, etc.)
Milestone

Comments

@chrisbobbe
Copy link
Collaborator

Currently (well, after #381), the Subscription class has a colorSwatch method to return a memoized StreamColorSwatch. That code is in /api/model.

Probably there's a better, more UI-focused place to do those computations (adjusting lightness, opacity, etc.) and provide them to consumers: #381 (comment)

@chrisbobbe chrisbobbe added this to the Beta 2 milestone Nov 18, 2023
@gnprice gnprice added the a-model Implementing our data model (PerAccountStore, etc.) label Nov 20, 2023
@gnprice gnprice removed their assignment Nov 22, 2023
@chrisbobbe
Copy link
Collaborator Author

chrisbobbe commented May 2, 2024

What if we made one simple global cache, as a Map<int, StreamColorSwatch> keyed by Subscription.color values? This might make sense, because a StreamColorSwatch isn't stateful and it's computed purely from a Subscription.color value.

When UI code is processing a subscription with .color 0xff76ce90, it can call a function that adds an entry for 0xff76ce90 if it's missing, or reads it if it's already present, and returns the corresponding StreamColorSwatch.

It's possible for that entry to fall into disuse at some point during the app session; for example, if

  • the stream's channel's color is changed
  • the user leaves the channel
  • the user logs out of the org with the channel

But I don't see really significant downsides to that.

@gnprice
Copy link
Member

gnprice commented May 3, 2024

Those leaks should be basically fine in practice, since those changes aren't common and since this cache wouldn't outlast the current process. Still, it'd be nicer to have something that doesn't leak.

How about just indexing on the stream ID? (As part of the PerAccountStore.) Then there'd be no leaks.

If the user has lots of subscriptions sharing the same color, then that'd give up opportunities to reuse the results. But that's probably not common, and in any event the worst case is just one StreamColorSwatch per subscription.

@chrisbobbe
Copy link
Collaborator Author

chrisbobbe commented May 3, 2024

It would be nice for UI elements to consume these color swatches via AnimatedTheme / ThemeExtension, so they would animate between their light- and dark-mode colors when you toggle dark mode, just like other style attributes that are consumed via AnimatedTheme. If the cache is global instead of per-account, the natural way to do that is through MaterialApp.theme, which configures an AnimatedTheme built by MaterialApp.

In principle we should be able to instantiate per-account AnimatedThemes in our own code. Maybe that could be a job of PerAccountStoreWidget? That widget is meant to be lightweight, though; would it be OK if it created an AnimatedTheme for its subtree? Its .data would have to come from Theme.of(context).copyWith(extensions: [/* … */]), I think. I recently used ThemeData.copyWith for an "inner ThemeData" in a non-final revision of #635 (see #635 (comment)). I remember it felt unsatisfying not to define all our theme data in one place, though.

…Hmm. I guess one concrete downside of putting an AnimatedTheme in every PerAccountStoreWidget is that, when dark mode is toggled, the animation will be accomplished by calling ThemeData.lerp redundantly for each page in the nav, when it could instead have been called only for the one AnimatedTheme near the root. That could get expensive, especially when the stream-color-swatch cache is large.

@chrisbobbe
Copy link
Collaborator Author

chrisbobbe commented May 3, 2024

If the cache were indexed by Subscription.color ints, I guess someone could artificially inflate the cache for people by using the Zulip API to rapidly change a single channel's color through some millions of colors. I don't know how realistically this could become a denial of service (probably they'd hit server rate limits first) but it would be easy to mitigate/discourage by limiting the cache size.

@gnprice
Copy link
Member

gnprice commented May 10, 2024

Yeah, I'd prefer to avoid having PerAccountStoreWidget itself cause any nontrivial computations.

The global cache indexed by Subscription.color seems fine, if that's the easiest solution.

chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Jun 18, 2024
This will help verify the upcoming refactor, zulip#393 (storing
stream-color variants in DesignVariables instead of Subscription
model objects).
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Jun 18, 2024
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
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Jun 18, 2024
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
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Jun 18, 2024
…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
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Jun 18, 2024
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 added a commit to chrisbobbe/zulip-flutter that referenced this issue Jun 18, 2024
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 added a commit to chrisbobbe/zulip-flutter that referenced this issue Jun 20, 2024
This will help verify the upcoming refactor, zulip#393 (storing
stream-color variants in DesignVariables instead of Subscription
model objects).
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Jun 20, 2024
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
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Jun 20, 2024
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
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Jun 20, 2024
…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
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Jun 20, 2024
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 added a commit to chrisbobbe/zulip-flutter that referenced this issue Jun 21, 2024
This will help verify the upcoming refactor, zulip#393 (storing
stream-color variants in DesignVariables instead of Subscription
model objects).
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Jun 21, 2024
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
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Jun 21, 2024
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
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Jun 21, 2024
…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
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Jun 21, 2024
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
gnprice pushed a commit to chrisbobbe/zulip-flutter that referenced this issue Jun 21, 2024
This will help verify the upcoming refactor, zulip#393 (storing
stream-color variants in DesignVariables instead of Subscription
model objects).
gnprice pushed a commit to chrisbobbe/zulip-flutter that referenced this issue Jun 21, 2024
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
gnprice pushed a commit to chrisbobbe/zulip-flutter that referenced this issue Jun 21, 2024
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
gnprice pushed a commit to chrisbobbe/zulip-flutter that referenced this issue Jun 21, 2024
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-model Implementing our data model (PerAccountStore, etc.)
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants