-
Notifications
You must be signed in to change notification settings - Fork 631
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
Fix for bug when user changes Windows System theme when winui gallery is up and running, caption buttons don't pick the correct theme color. #1239
Conversation
…s app properties are changed.
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.
Thanks for submitting a PR for this! There are a few things I noticed:
- We should reuse the existing mechanism we already have, namely update the
TitleBarHelper.ApplySystemThemeToCaptionButtons();
to properly update the color of the buttons. This function is already being triggered on theme change by a colors changed listener in the NavigationRootPage. - I was able to get dark caption buttons in dark theme, is that intentional?
Thanks for the suggestion. Yeah,I could definitely refactor more so that Titlebarhelper functions are used more effectively and functionality is not repeated. However, a lot of this code will go away soon once I provide appwindow + winui 3 custom titlebar merger, hence, i am not spending more effort in refactoring this code. For 2. Can you provide more details ? did you visit Titlebar page ? It sets the foreground color to something custom. The default custom color is black. I could set it that default is different for light and dark colors but that's for another PR 🙂 |
Regarding 1, I'm strongly against pulling in a dependency for a functionality/pattern we already have. If you plan on refactoring AppWindow and custom titlebar anyway, I think this can also wait. Regarding 2, I opened the app on the settings page. I switched the app theme to dark and then changed the system theme to light. |
@chingucoding thanks for letting me know of that bug issue. It should be fixed now. I am using For the first part, the merger code will take months to come. Given winui gallery is a demo app for WinUI functionality, I would prefer it to have correct functionality, even if it comes with cost of pulling a new dependency. |
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.
approved with one suggestion.
internal class TitleBarHelper | ||
{ | ||
|
||
public static void triggerTitleBarRepaint(Window window) |
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.
I would make this private. The other methods should give all required functionality and this is more of an implementation detail.
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.
LGTM
When you open winui gallery and then goto Windows System setting and change its theme, winui gallery dynamically updates to new theme but not caption buttons.
This happens because we are using WinUI 3 custom titlebar for titlebar and caption controls. The caption buttons need to be colored by our code. In order for this code to dynamically update the caption button foreground color, it needs to know when the system setttings have changed. Windows.UI.ViewManagement.Settings provide that solution. Though for reasons unknown to me, its lambda is called off-thread so a dispatcher queue is needed to call caption controls update function in app's UI thread.
(I referenced the implementation Windows Community Toolkit's ThemeListener helper class)
Line endings have also been updated for some files to CRLF which Windows uses.
Types of changes