HumbleUI / JWM

Cross-platform window management and OS integration library for Java
Apache License 2.0
552 stars 44 forks source link

[wip] Windows: system theme #143

Closed i10416 closed 2 years ago

i10416 commented 2 years ago

Add functions to get window theme and switch light/dark mode.

I implemented it by referring to winit and this.

See also:

tonsky commented 2 years ago

Hi!

Thanks again for taking time and effort to tackle this. This is an important issue we’d love to see it merged in JWM as soon as possible.

I know your branch is still WIP, but @EgorOrachyov and I took a quick look and have a bit of feedback to share so far. I hope you don’t mind.

  1. Early on, we decided to only target Windows 10+, mostly for implementation simplicity and ease of testing. If we only target Windows 10+, it feels that dynamic version check (user32.dll) and uxtheme.dll are excessive. @EgorOrachyov thinks linking uxtheme.dll in CMakeLists.txt is enough, what do you think?

  2. Dark/Light are not the only flags that modern OSes use to control apps’s look. Another two are High Contrast and Inverted colors. Do you think you can accomodate that in the API? One option might be to make Theme a data class with three booleans: isDark, isHighContrast, isInverted? This Electron seems to be doing the same

  3. This is minor, but we are using 4 spaces indentation in C++ and { on the same line. It would be great for consistency if you switched to that style also.

Thanks again and glad to see you among contributors!

i10416 commented 2 years ago

@tonsky Thank you for helpful feedback!

it feels that dynamic version check (user32.dll) and uxtheme.dll are excessive.

I suppose you mean ntdll.dll here; "dynamic version check (user32.dll)".

https://github.com/HumbleUI/JWM/pull/143/files#diff-11b9c69611650a205f189c75939df5d4d45959ff5b435a33adcc7ada86220778R90

I separated version check from set theme function respectively and now use only uxtheme.dll part.

Should I remove version check part or keep it?

Dark/Light are not the only flags that modern OSes use to control apps’s look.

I agree with you. It would be better. I checked Electron implementation.

I'm going to expose isHighContrast function and isInverted function.

i10416 commented 2 years ago
i10416 commented 2 years ago

@tonsky After investigating Windows API and Electron implementation, I found that Windows does not offer API to check whether or not inverted color mode is enabled. Besides, it seems electron uses chromium properties to determine InvertedColorScheme mode.

Perhaps I may miss something, but I think isInverted is not as much relevant as isHighContrast since Windows disables dark mode when it is in high contrast mode whereas it does not when in inverted color mode. Thus, could I leave isInverted out for this PR?

tonsky commented 2 years ago

Thus, could I leave isInverted out for this PR?

Sure, we can come back to it later

tonsky commented 2 years ago

From what I see on Electron, not all of these properties are supported on all platforms. It might be that Windows doesn’t even have inverted mode (does it?)

i10416 commented 2 years ago

As far as I konw, Windows does have invert color scheme(ctrl+alt+I), but I could not find a way to programatically detect/control it.(There is InvertRect api, but it does not suit for use case. ) https://docs.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-invertrect

I also have a quick look at chromium implementation.

https://chromium.googlesource.com/chromium/src/+/refs/heads/main/ui/native_theme/native_theme_win.cc

tonsky commented 2 years ago

I’m not sure how is Github supposed to communicate when this is ready to review, so please let me know!

So far, I took a quick look and have a few questions:

i10416 commented 2 years ago

@tonsky Sorry for late reply and forgetting notifying you of progress. And thank you for feedbacks.

How do you see AppearancePreference might be used?

I suppose the case where user wants to get Theme, contrast and other appearance properties at once rather than calling each getter (e.g. when initializing an application window).

I decide to have theme data class according to your advice and I prefer having Theme as enum rather to having theme as bool flag like isDark. I named the class as AppearancePreference since I could not come up with good name alternative to Theme but I think it is a bit redundant :(

In which cases Theme::SYSTEM is used?

I assumed such case as user wants to specify whether to use theme inherited from system or to override theme of a respective window by specifying theme. (As far as I know,) It is impossible to override theme of each window respectively in Windows, and I am not sure it is possible in Linux and macOS. If it is impossible for them too, I will remove Theme::SYSTEM.

i10416 commented 2 years ago

@tonsky I think this PR becomes a bit messy, so I will separate changes in other PRs.