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

Force Light Mode permanently on #297

Merged
merged 6 commits into from
May 6, 2024

Conversation

LilithSilver
Copy link
Contributor

Closes #296 by forcing light mode on via the theme, and specifying theme colors in Application.Resources instead of by manually building a theme.

<Application.Styles>
<materialStyles:MaterialTheme />
<!-- Dummy values for foreground colors to initialize properly. All associated brushes are overridden in Application.Resources. -->
<materialStyles:MaterialTheme BaseTheme="Light" PrimaryColor="Purple" SecondaryColor="Green"/>
Copy link
Owner

Choose a reason for hiding this comment

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

Are the dummy values actually necessary here? If so, what do they achieve?

Comment on lines 24 to 29
<SolidColorBrush x:Key="MaterialSecondaryLightBrush" Color="#F9A825"></SolidColorBrush>
<SolidColorBrush x:Key="MaterialSecondaryLightForegroundBrush" Color="#F9A825"></SolidColorBrush>
<SolidColorBrush x:Key="MaterialSecondaryMidBrush" Color="#F9A825"></SolidColorBrush>
<SolidColorBrush x:Key="MaterialSecondaryMidForegroundBrush" Color="#F9A825"></SolidColorBrush>
<SolidColorBrush x:Key="MaterialSecondaryDarkBrush" Color="#F9A825"></SolidColorBrush>
<SolidColorBrush x:Key="MaterialSecondaryDarkForegroundBrush" Color="#F9A825"></SolidColorBrush>
Copy link
Owner

Choose a reason for hiding this comment

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

I may be wrong, but shouldn't all these colors not be the same?

@Tyrrrz
Copy link
Owner

Tyrrrz commented May 6, 2024

I think a simpler fix would be to set RequestedThemeVariant="Light" on the <Application> element, and leave the rest of the code as is

@Tyrrrz Tyrrrz added the bug label May 6, 2024
@LilithSilver
Copy link
Contributor Author

I think a simpler fix would be to set RequestedThemeVariant="Light" on the <Application> element, and leave the rest of the code as is

I'd push back against this -- the current approach is actually incorrect, which is the source of this bug. Read this documentation; since we always apply the theme manually, we should be using MaterialThemeBase instead of MaterialTheme.

However, MaterialThemeBase seems to not be working with DialogHost (or something) due to a missing brush at startup. There's a hack fix by overriding the missing brush before the theme is applied, but that's a more complex fix than this one. This lets us just use MaterialTheme.

@Tyrrrz
Copy link
Owner

Tyrrrz commented May 6, 2024

Ok fair enough. I'm just wary of setting all color brushes by hand like that, for two reasons:

  1. My understanding is that, when using the current approach, the tertiary colors (such as MaterialSecondaryDarkBrush) are automatically derived from primary/secondary colors by way of adjusting HSL. Correct me if I'm wrong, I haven't checked how it works since ~2017.
  2. If the Material theming library adds new colors in a new version, we'd have to update the resources to support them. Which could lead to some unexpected issues.

It looks like the doc you linked doesn't necessarily specify why the current approach is incorrect though, at least I don't see where it does that.

@LilithSilver
Copy link
Contributor Author

LilithSilver commented May 6, 2024

Those points are good ones -- I played around with it a bit more and got MaterialThemeBase to work with an initial override that later gets set by the theme for the missing brush. (Before I was doing it via code, and it was ugly, but this axaml version looks OK to me).

Let me know what you think.

By the way, the part in the doc that specifies MaterialThemeBase:

NOTE: If you always set theme from code use MaterialThemeBase in your Application.Styles. Because MaterialTheme will create an additional overhead to apply the original styles (written in xaml), which you will override anyway.

@Tyrrrz
Copy link
Owner

Tyrrrz commented May 6, 2024

Looks good. Can you also please explain why RequestedThemeVariant="Light" doesn't work, regardless of MaterialTheme/MaterialThemeBase approach? My understanding is that it would hard-code the theme so even if the OS-wide requested theme changes, it won't affect the app.

@LilithSilver
Copy link
Contributor Author

Looks good. Can you also please explain why RequestedThemeVariant="Light" doesn't work, regardless of MaterialTheme/MaterialThemeBase approach? My understanding is that it would hard-code the theme so even if the OS-wide requested theme changes, it won't affect the app.

It does work -- I'll make that change as well. As long as we're correctly using MaterialThemeBase I've got no issues with it.

LightBulb/App.axaml Outdated Show resolved Hide resolved
@Tyrrrz
Copy link
Owner

Tyrrrz commented May 6, 2024

Thanks!

@Tyrrrz Tyrrrz merged commit 9e8b788 into Tyrrrz:master May 6, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UI breaks if Windows switches dark/light themes
2 participants