search

Tyrrrz / LightBulb

Reduces eye strain by adjusting screen gamma based on the current time
MIT License
2.23k stars 141 forks source link

Force Light Mode permanently on #297

Closed LilithSilver closed 4 months ago

LilithSilver commented 4 months ago

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

Tyrrrz commented 4 months ago

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

LilithSilver commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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.

Tyrrrz commented 4 months ago

Thanks!