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

Adapt caching behaviour for dark/light theme switching #463

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

khmyznikov
Copy link

Trying to address this bug

Trying to address this bug
@khmyznikov
Copy link
Author

@uwolfer is it looks like a fix?

@khmyznikov
Copy link
Author

@PEConn @andreban how to check build?

Copy link
Member

@andreban andreban left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Gleb, thank you for the PR!

Would you mind adding how this change is solving #461 to the PR description? I also dropped a comment with regards moving magic values to static variables.

@khmyznikov
Copy link
Author

@andreban from my understanding, it solves the issue of wrong caching of different icons for light/dark splash screen. #51 (comment)

@andreban
Copy link
Member

Thanks for the update! What I'm making sure I understand is how this PR solves the issue - my understanding is that the bug is caused by only re-transferring the splash image file when the application is updated. But it also needs to be transferred when the theme changes. To solve that, you're adding the theme to the default preferences object, retrieving the value on launch, and ensuring the existing file is only re-used if all the following are true: the file exists, the app hasn't been updated, and the theme wasn't change. Does this sound right?

@khmyznikov
Copy link
Author

@andreban yeah that's sounds right 👍

Copy link
Member

@andreban andreban left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - Will wait for @PEConn to take a look before merging.

@uwolfer
Copy link

uwolfer commented Mar 12, 2024

@uwolfer is it looks like a fix?

Thanks for working on this, @khmyznikov. Code makes sense to me, but I have not tested it. I would suggest that you update the PR title to reflect what we actually have fixed with this change.

@khmyznikov khmyznikov changed the title BugFix #461 Adapt caching behaviour for dark/light theme switching Mar 12, 2024
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.

3 participants