-
Notifications
You must be signed in to change notification settings - Fork 59
fix(ThemeService): preserve 'System' theme preference on OS theme changes #2937
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
base: main
Are you sure you want to change the base?
Conversation
|
Hi @kazo0 , When you have a moment, could you please take a look at this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a bug where the ThemeService would incorrectly overwrite a user's AppTheme.System preference when the operating system's theme changed, effectively "locking" the application to whichever theme it first switched to.
Key Changes:
- Refactored
ElementThemeChangedto check the saved theme preference before reacting to OS theme changes - The handler now only fires a notification event when the user preference is
AppTheme.System, avoiding unintended theme persistence - Removed the call to
InternalSetThemeAsyncfrom the event handler to preserve the user's original preference
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Tested again on the stable branch with Uno.SDK 6.3.28 + Uno.Extensions 6.2.3, and it works correctly. |
|
@kazo0 Thanks for the approval! Could you please merge this? |
Closes #2914
PR Type
What kind of change does this PR introduce?
What is the current behavior?
When the user set the application theme to
AppTheme.System, the service would correctly applyElementTheme.Defaultto follow the operating system's theme.However, the first time the system theme changed (e.g., from light to dark), the
ElementThemeChangedevent handler would re-apply the new theme (AppTheme.Dark) as a permanent setting. This had two negative effects:AppTheme.SystemtoAppTheme.Dark.RequestedThemeproperty was updated fromElementTheme.Defaultto the fixedElementTheme.Dark.As a result, the application would no longer respond to any future system theme changes, effectively "locking" the theme to whatever it first switched to.
What is the new behavior?
With the corrected logic, the ElementThemeChanged event handler behaves differently:
This ensures that the RequestedTheme property remains ElementTheme.Default and the user's AppTheme.System preference is preserved. The application now correctly follows all subsequent system theme changes without getting locked.
PR Checklist
Please check if your PR fulfills the following requirements:
Screenshots Compare Test Runresults.Other information
Tested on the
stablebranch with Uno.SDK 6.3.28 + Uno.Extensions 6.2.3, and it works correctly.