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

Enhanced dark mode. #984

Merged
merged 7 commits into from Apr 10, 2020
Merged

Enhanced dark mode. #984

merged 7 commits into from Apr 10, 2020

Conversation

Berrysoft
Copy link
Contributor

Fixes #983

  • Use undocumented api RtlGetNtVersionNumbers to get build number.
  • Determine if it is 1903+, and use different apis.

Before (current one shipped with msys2):
Before

After:
After

@Berrysoft
Copy link
Contributor Author

I confirmed again and found that we should use ShouldAppsUseDarkMode. The reason why it doesn't work on Windows 10 1903+ is because that the value of DWMWA_USE_IMMERSIVE_DARK_MODE changed. Corrected.

@mintty
Copy link
Owner

mintty commented Apr 10, 2020

Thanks. 3 comments:

  1. I'd prefer to retrieve the build number with HIWORD(GetVersion()), easier call and one less library to inspect. I know GetVersion is marked deprecated but I doubt they'll drop it and mintty uses it anyway.
  2. I saw on https://stackoverflow.com/questions/53501268/win10-dark-theme-how-to-use-in-winapi that SetPreferredAppMode should be called prior to any window creation. Should we move it a few lines up or does it work there alike?
  3. As SetPreferredAppMode is an upward-compatible repleacement of AllowDarkModeForApp (same "ordinal"), could we simply got with the latter unconditionally and save the reproduction of undocumented typedef enum PREFERRED_APP_MODE?

@mintty
Copy link
Owner

mintty commented Apr 10, 2020

I've tried it (simplified as suggested), works. FlushMenuThemes does not seem to be needed.
dark.patch.txt

@Berrysoft
Copy link
Contributor Author

  1. HIWORD(GetVersion()) returns 9200 after Windows 8.1, which is not the right build version.
  2. It works fine there, but also OK to move a few lines up. I'll do this.
  3. It's OK to remove PREFERRED_APP_MODE and replace it with int. But I think they are not fully compatible because sizeof(BOOLEAN)==1 and sizeof(PREFERRED_APP_MODE)==4.

@Berrysoft
Copy link
Contributor Author

Yes, it seems that FlushMenuThemes isn't needed... I'll remove it then.

@mintty
Copy link
Owner

mintty commented Apr 10, 2020

GetVersion is sensitive to "manifestation". Mintty includes "manifest" entries up to Windows 10 (res.mft), so HIWORD(GetVersion()) reports the proper build number.
The build is not needed for this patch anyway if we continue with the SetPreferredAppMode signature only (which catches the previous case).

@mintty
Copy link
Owner

mintty commented Apr 10, 2020

To explain my hesitation about GetVersion: I don't like the idea to replace a function that is - even if marked deprecated - still available and documented by some undocumented function, somehow further supporting MS's policy of hidden APIs. If you don't follow here, I'll merge your pull request and tweak back on it, no big issue.

@Berrysoft
Copy link
Contributor Author

Well, I don't have a machine with Windows 10 1809, so I can't confirm that the signature of SetPreferredAppMode is valid on that. As the size of the parameters of the function may be not exactly the same, and __stdcall functions on 32-bit machine clears the calling stack itself, I'm not sure it will work properly.

I'll change RtlGetNtVersionNumbers to GetVersion.

@mintty mintty merged commit b888672 into mintty:master Apr 10, 2020
mintty added a commit that referenced this pull request Apr 10, 2020
@mintty
Copy link
Owner

mintty commented Apr 10, 2020

Tweaked / simplified, not using build dependency anymore.
Tested on older Windows build.
Thanks a lot.

@mintty
Copy link
Owner

mintty commented Apr 13, 2020

I've tried to extend the effect of dark mode to the Options dialog and search bar (Alt+F3), with unconvincing results. Could you have a look, please? Should something be done about RegisterClass?

@Berrysoft
Copy link
Contributor Author

I'm afraid that it coundn't be done without effort of Microsoft. I'll show what I've found, and it depends on you whether to darken the dialog and search bar:

  • Button: set theme to "DarkMode_Explorer"
  • Edit: set theme to "DarkMode_Explorer" and deal with "WM_CTLCOLOREDIT" message of the parent window.
  • Dialog background: deal with "WM_ERASEBKGND" message

There's a repo attempting to darken some common controls, but it uses a lot of hacking and even so, it cannot darken the menu bar...

Since the dialog of File Explorer is in light theme, I don't suggest make dialog in dark mode.

@mintty
Copy link
Owner

mintty commented Apr 13, 2020

Thanks a lot. I've added some comments to the code, documenting what's darkened by the respective calls. And I've limited the darkening to the title bar of the Options dialog, maybe that should be done.

mintty added a commit that referenced this pull request Apr 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhanced dark mode in Windows 10
2 participants