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

[wip] Windows: system theme #143

Closed
wants to merge 19 commits into from
Closed

Conversation

i10416
Copy link
Contributor

@i10416 i10416 commented Aug 28, 2021

  • get system theme
  • disable when high contrast mode
  • set theme
  • expose isHighContrast

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

I implemented it by referring to winit and this.

See also:

@i10416 i10416 changed the title [wip] Feature system theme [wip] Feature: system theme Aug 28, 2021
@i10416 i10416 changed the title [wip] Feature: system theme [wip] Windows: system theme Aug 28, 2021
@tonsky
Copy link
Collaborator

tonsky commented Aug 30, 2021

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
Copy link
Contributor Author

i10416 commented Sep 1, 2021

@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.

- separate version check from function to check dark theme support
- simplify version check since JWM currently supports Windows 10+
- format code style
@i10416 i10416 force-pushed the feature-system-theme branch 2 times, most recently from 5e8dbce to 7f70a45 Compare September 1, 2021 14:39
@i10416
Copy link
Contributor Author

i10416 commented Sep 5, 2021

@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?

@i10416 i10416 changed the title [wip] Windows: system theme Windows: system theme Sep 5, 2021
@tonsky
Copy link
Collaborator

tonsky commented Sep 5, 2021

Thus, could I leave isInverted out for this PR?

Sure, we can come back to it later

@tonsky
Copy link
Collaborator

tonsky commented Sep 5, 2021

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
Copy link
Contributor Author

i10416 commented Sep 5, 2021

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
Copy link
Collaborator

tonsky commented Sep 8, 2021

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:

  • How do you see AppearancePreference might be used?
  • In which cases Theme::SYSTEM is used?

@i10416 i10416 changed the title Windows: system theme [wip] Windows: system theme Sep 11, 2021
@i10416
Copy link
Contributor Author

i10416 commented Sep 11, 2021

@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
Copy link
Contributor Author

i10416 commented Sep 19, 2021

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

@tonsky tonsky force-pushed the main branch 2 times, most recently from b4a6c73 to bada613 Compare October 4, 2021 17:27
@i10416 i10416 mentioned this pull request Oct 4, 2021
@i10416 i10416 closed this Oct 4, 2021
@i10416 i10416 deleted the feature-system-theme branch February 2, 2022 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants