Skip to content

Conversation

@stefan-niedermann
Copy link
Member

@AndyScherzinger
Copy link
Member

fine by me, or we go straight for a89c4bd and the PR #208 while with the new Google implementation there is no fixed/stored values anymore but they are continuously calculated upon requesting them. However Scheme is deprecated with DynamicScheme. So we could either merge it and later add a "caching" object inbetween. OR are fine with the dynamic calculation.

What do you think @stefan-niedermann ?

@stefan-niedermann
Copy link
Member Author

I don't need it urgently, so I think I can rebase it after DynamicScheme has been merged 🙂

@AndyScherzinger
Copy link
Member

@stefan-niedermann what is your point of view on merging dynamicScheme? I will calculate any color on each access of the color, so far I didn't see any performance impact from an experience perspective on the talk client so I'd be fine with merging and release that version as-is and in case performance becomes an issue add a way to cache the calculated colors, not sure though that is possible/easy given the dynamicScheme implementation of Google.

@stefan-niedermann
Copy link
Member Author

I didn't have a look into DynamicScheme yet. It looks like the API surface of this library stays pretty much the same and the changes are under the hood? What's the deal with DynamicSchemes?

I thought that this is a color scheme dynamically generated depending on the wallpaper a user choses for his launcher homescreen?
So in which way does this affect us? Given we need to apply Nextcloud instance colors rather than wallpaper depending colors 😉 ?

@AndyScherzinger
Copy link
Member

Well, if you look at 1a1df3c#diff-f2305b95fc7795d7fc2ff8c309ead47432f28737b0fb5336b690876a48623572L86

You will see the API change on the outside, so things seems to be roughly the same but they are not.
While Scheme would have all colors calculated at instantiation time of the class DynamicScheme actually calculates them upon accessing the color, so whenever you do somethign like dynamicColor.onSecondaryContainer().getArgb(scheme) which you need to do to get a color and dive into getArgb(scheme) you will see that is actually calculates the color. So the new class doesn't contain any values to return but calculates them every time you access the color. So if you theme something and access the primary color 3 times it will get calculated 3 times while with the old implementation it'll just give you the value of the variable it already has stored the value in (call that a static value) while dynamic here means "always recalculated".

@github-actions
Copy link
Contributor

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

@AndyScherzinger AndyScherzinger force-pushed the theme-bottom-sheet-drag-handle-view branch from 676732f to 6a79d9e Compare February 15, 2024 11:19
@AndyScherzinger AndyScherzinger added this to the 0.15.0 milestone Feb 15, 2024
@AndyScherzinger AndyScherzinger merged commit f416a0d into main Feb 15, 2024
@AndyScherzinger AndyScherzinger deleted the theme-bottom-sheet-drag-handle-view branch February 15, 2024 11:36
@AndyScherzinger
Copy link
Member

Released with 0.15.0 @stefan-niedermann, have fun 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants