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

[Settings]Takes some time to adapt to theme change #26791

Closed
jaimecbernardo opened this issue Jun 11, 2023 · 6 comments
Closed

[Settings]Takes some time to adapt to theme change #26791

jaimecbernardo opened this issue Jun 11, 2023 · 6 comments
Assignees
Labels
Area-User Interface things that regard UX for PowerToys Issue-Bug Something isn't working Priority-3 Bug that is low priority Product-Settings The standalone PowerToys Settings application Resolution-Fix Committed Fix is checked in, but it might be 3-4 weeks until a release. Status-Reproducible This issue was reproduced by a maintainer

Comments

@jaimecbernardo
Copy link
Collaborator

Microsoft PowerToys version

0.71.0-pre

Installation method

Dev build in Visual Studio

Running as admin

None

Area(s) with issue?

Settings

Steps to reproduce

After #25936 , when changing the windows theme with the Settings page opened, it takes a bit to adapt to the theme change.

  1. Open the Settings window and make sure that its theme is set to use the system's theme.
  2. Change the theme in Settings. (Either light or dark mode, the one you're not currently using.

✔️ Expected Behavior

The Settings window reacts to the theme change.

❌ Actual Behavior

The Settings window won't react to the theme change unless you use its UI for a bit.

Other Software

Tested on Windows 11 only, but I assume Windows 10 might have issues too.

@jaimecbernardo jaimecbernardo added Issue-Bug Something isn't working Product-Settings The standalone PowerToys Settings application Area-User Interface things that regard UX for PowerToys Priority-3 Bug that is low priority Status-Reproducible This issue was reproduced by a maintainer labels Jun 11, 2023
@Jay-o-Way
Copy link
Collaborator

Jay-o-Way commented Jul 23, 2023

I notice that only the General page triggers the switch, so...
@niels9001 Cause: the following code is only in GeneralPage.xaml.cs

public static int UpdateUIThemeMethod(string themeName)
{
switch (themeName?.ToUpperInvariant())
{
case "LIGHT":
// OobeShellPage.OobeShellHandler.RequestedTheme = ElementTheme.Light;
ShellPage.ShellHandler.RequestedTheme = ElementTheme.Light;
break;
case "DARK":
// OobeShellPage.OobeShellHandler.RequestedTheme = ElementTheme.Dark;
ShellPage.ShellHandler.RequestedTheme = ElementTheme.Dark;
break;
case "SYSTEM":
// OobeShellPage.OobeShellHandler.RequestedTheme = ElementTheme.Default;
ShellPage.ShellHandler.RequestedTheme = ElementTheme.Default;
break;
default:
Logger.LogError($"Unexpected theme name: {themeName}");
break;
}
App.HandleThemeChange();
return 0;
}

This code must be "app-scope"

@Jay-o-Way
Copy link
Collaborator

@niels9001 it really is best practice to leave absolutely as much as possible to the framework/SDK. In other words: remove as much "manual" code as possible.

@niels9001
Copy link
Contributor

@niels9001 it really is best practice to leave absolutely as much as possible to the framework/SDK. In other words: remove as much "manual" code as possible.

It should work by default.. but theme changing at runtime has been a pain point for a long, long time. The theming stuff we are doing turned out to be a spaghetti - and we are making registry edits to make things work. It's not the way, yet there isn't a great one :-).

Let's leave it at prio3 though - it only happens on theme change within the app which is not a common thing and navigating to a different page solves it.

@niels9001 niels9001 added Priority-3 Bug that is low priority and removed Priority-2 Bug that is medium priority labels Aug 18, 2023
@Jay-o-Way
Copy link
Collaborator

Jay-o-Way commented Aug 18, 2023

It should work by default

And it does - if only we stop making so much spaghetti! (I don't even like spaghetti)

...yet there isn't a great one :-).

There is - like I said: Let the framework/sdk/winui/whateverthenameis do its thing. #LessIsMore!

@niels9001 I am literally showing you what/where the problem is. Why can't you accept that?

@niels9001
Copy link
Contributor

It should work by default

And it does - if only we stop making so much spaghetti! (I don't even like spaghetti)

...yet there isn't a great one :-).

There is - like I said: Let the framework/sdk/winui/whateverthenameis do its thing. #LessIsMore!

As far as I am aware, there's is no out-of-the-box way to set the ApplicationTheme outside of the constructor of App.xaml.cs. This means that changing the entire app window theme at runtime is not (fully) supported within the app - only when triggered by W11 Settings (this dates back to the Windows 8 days where this wasn't added - see microsoft/microsoft-ui-xaml#4474).
The only thing we can update is the theme of the root UI element (e.g. the page that sits within the Window). But things like Mica, titlebar (caption buttons), control specific acrylic effects (e.g. flyout backgrounds) are controlled by the Window. That's why we need to do a bunch of custom code (call it hacky if you will 😁) to make it (somewhat) work. (See also microsoft/microsoft-ui-xaml#8249)

So.. it's not an easy thing that we somehow decided to make more complicated 😊.

@niels9001 I am literally showing you what/where the problem is.

I think the code you mentioned gets triggered whenever the ViewModel on the General page gets/sets the theme (e.g. on page load or when the user selects the theme option).. That's the reason why navigating from and back to the General page triggers that method and applies the correct theme.
What we need is to catch the OS trigger that the theme changes and then apply the theme logic. We try to do that with the theme listener:

themeListener = new ThemeListener();
.
If I recall correctly, I think that works but we get the wrong theme passed - not sure though.. I know I struggled a lot to get #25936 in a workable state due to this (although I'm pretty sure I tested this scenario as well so this is surprising to see).

If you have any code suggestions on how to fix this, please let it be known in this thread! (Or feel free to PR the solution 😁)

Why can't you accept that?

In my earlier comment, I tried to explain what's the root cause of why these theme related bugs pop-up across various WinUI3 modules. Not sure how that is "not accepting" things? I just mentioned that this is not a 'medium' bug but a Prio3 as @jaimecbernardo tagged it to be (it's not a common scenario an average user would run into nor is it a blocking/crashing bug). I just wanted to point out that there's a more complex root issue causing this and that this is not a straight-forward thing to fix - hopefully my explanation in this comment is more clear, but it takes a lot of time to write these things down.

Point of note: at times you come off as lecturing people on why things/code aren't the way they should be, or making snarky remarks. Yeah, people and the code they write ain't perfect (definitely this codebase isn't :-)). I'm sure you don't mean it that way, but please take that in mind and check out our Code of Conduct. We are all friends here!

@Jay-o-Way Jay-o-Way added the Status-In progress This issue or work-item is under development label Nov 21, 2023
@Jay-o-Way Jay-o-Way moved this to In Progress ⚒️ in Test for PowerToys Nov 21, 2023
@jaimecbernardo jaimecbernardo added Resolution-Fix Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed Status-In progress This issue or work-item is under development labels Nov 23, 2023
@stefansjfw
Copy link
Collaborator

The work associated with this issue has been released as part of the 0.76 sprint. Please update PowerToys to the latest. https://github.com/microsoft/PowerToys/releases

@github-project-automation github-project-automation bot moved this from In Progress ⚒️ to Done ✔ in Test for PowerToys Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-User Interface things that regard UX for PowerToys Issue-Bug Something isn't working Priority-3 Bug that is low priority Product-Settings The standalone PowerToys Settings application Resolution-Fix Committed Fix is checked in, but it might be 3-4 weeks until a release. Status-Reproducible This issue was reproduced by a maintainer
Projects
Status: Done
Development

No branches or pull requests

4 participants