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

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

Merged
merged 3 commits into from
Apr 3, 2023

Conversation

pratikone
Copy link
Contributor

@pratikone pratikone commented Mar 30, 2023

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

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Copy link
Contributor

@marcelwgn marcelwgn left a 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:

  1. 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.
  2. I was able to get dark caption buttons in dark theme, is that intentional?

@pratikone
Copy link
Contributor Author

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 🙂

@marcelwgn
Copy link
Contributor

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.

@pratikone
Copy link
Contributor Author

pratikone commented Mar 31, 2023

@chingucoding thanks for letting me know of that bug issue. It should be fixed now. I am using FrameworkElement.ActualTheme instead of Application.RequestedTheme. I have also done the refactoring of coloring caption buttons.

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.

Copy link
Contributor

@bpulliam bpulliam left a 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)
Copy link
Contributor

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.

Copy link
Contributor

@marcelwgn marcelwgn left a comment

Choose a reason for hiding this comment

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

LGTM

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