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

dark theme scrollbars #277

Closed
musm opened this issue Oct 10, 2018 · 13 comments
Closed

dark theme scrollbars #277

musm opened this issue Oct 10, 2018 · 13 comments
Assignees
Labels
Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Conhost For issues in the Console codebase Resolution-Fix-Available It's available in an Insiders build or a release Work-Item It's being tracked by an actual work item internally. (to be removed soon)
Milestone

Comments

@musm
Copy link

musm commented Oct 10, 2018

can we add the option that if you are using the dark theme in windows that the scrollbars in the prompt
image

match the dark scrollbars in e.g. explorer

image

@ysc3839
Copy link
Contributor

ysc3839 commented Oct 11, 2018

Try:

SetWindowTheme(GetConsoleWindow(), L"DarkMode_Explorer", nullptr);

In your console application.

Correct way is using internal API AllowDarkModeForWindow.
I think console team have known such feature, they just too busy to support it. So just wait.

@miniksa miniksa self-assigned this Oct 11, 2018
@miniksa miniksa added Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Conhost For issues in the Console codebase Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. labels Oct 11, 2018
@miniksa
Copy link
Member

miniksa commented Oct 11, 2018

Made a change today to try this out for Insiders builds.

@musm
Copy link
Author

musm commented Oct 11, 2018

@miniksa awesome, which build number? I'm on the slow ring fyi.

@zadjii-msft
Copy link
Member

@musm It'll be out in a few weeks. It usually takes just about 3 weeks for checkins we make to get up to master, and usually a week after that for the Insider's build to get spun with the change.

So, soontm

@zadjii-msft zadjii-msft added this to the 19H1 milestone Oct 11, 2018
@oising
Copy link
Collaborator

oising commented Oct 11, 2018

Hmm, I tried what @ysc3839 said in powershell (on 1809, of course) and it had no effect on the scrollbars:

PS> add-type -Name NativeMethods -Namespace Temp -MemberDefinition @"
[DllImport("uxtheme.dll", SetLastError=true, ExactSpelling=true, CharSet=CharSet.Unicode)]
public static extern int SetWindowTheme(IntPtr hWnd, string pszSubAppName, string pszSubIdList);
[DllImport("kernel32.dll", SetLastError=true)]
public static extern IntPtr GetConsoleWindow();
"@
PS> [Temp.NativeMethods]::SetWindowTheme([Temp.NativeMethods]::GetConsoleWindow(),
               "DarkMode_Explorer", $null)
0
PS> # scrollbars still light theme

Suggestions on how to correct this, @zadjii-msft or @miniksa ?

@miniksa
Copy link
Member

miniksa commented Oct 11, 2018

I'm really not sure why it doesn't work from the outside. Could be that it doesn't like being done after ShowWindow has been called and the WM_SETTINGCHANGED handler isn't ready for it. Could be that SetWindowTheme generates a window message that UIPI is blocking from your Powershell binary. Could be something completely different. I'm sorry. I tried it inside the console code and it works, so I'm not intending to spend anytime debugging why it doesn't work from the outside.

@musm
Copy link
Author

musm commented Oct 11, 2018

(unrelated) @ysc3839 can you provide me with some simple minimal example with that in use? Trying to dive in so I can create PRs for some apps I use, but the win32 API world is a little dizzying for first timers.

@ysc3839
Copy link
Contributor

ysc3839 commented Oct 12, 2018

@musm
It works on my GUI application. Currently I have not tested on console Windows.
Maybe SetWindowTheme for external process is not allowed.

https://github.com/ysc3839/VCMPBrowser/blob/darkmode/DarkMode.h
This code uses internal APIs and may not work on a newer Windows version.

PS: Hope Microsoft makes these APIs public for all Win32 apps.

@be5invis
Copy link

be5invis commented Nov 3, 2018

Does the dark mode cover settings dialog?

@DHowett-MSFT DHowett-MSFT added Resolution-Fix-Available It's available in an Insiders build or a release and removed Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. labels Nov 21, 2018
@miniksa miniksa added the Work-Item It's being tracked by an actual work item internally. (to be removed soon) label Jan 18, 2019
@miniksa
Copy link
Member

miniksa commented Jan 18, 2019

MSFT: 19271078 reached Insiders for 18272+.

The follow on fix for a mistake (MSFT: 19548142) reached Insiders for 18309+.

@miniksa miniksa closed this as completed Jan 18, 2019
@ysc3839
Copy link
Contributor

ysc3839 commented Feb 16, 2019

@miniksa I reverse engineered and found you use GetProcAddress with a magic number. Why not static link to that function?
I'm afraid that the console will broken when the ordinal of that function changed.

image

@DHowett-MSFT
Copy link
Contributor

@ysc3839 There's a couple reasons for that:

  1. The function may not be exported by name.
  2. We can't statically link it, as the library that provides it is a dynamic library.
  3. We all use the console internally every day, and are very likely to be the first to know if this ordinal changes, probably through a change in its observable behavior. Since we're developing on builds of windows that are very close to mainline, we'll almost certainly figure out changes like this before any breakage makes it out to Insiders.
    1. (additionally, any given ordinal is not at risk of changing during the lifetime of a single version of Windows)

@ysc3839
Copy link
Contributor

ysc3839 commented Feb 16, 2019

@DHowett-MSFT
Sorry, the "static link" means link to that DLL using import address table.
Some other DLL (explorerframe.dll) link to that function using IAT. It's better than using GetProcAddress.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Conhost For issues in the Console codebase Resolution-Fix-Available It's available in an Insiders build or a release Work-Item It's being tracked by an actual work item internally. (to be removed soon)
Projects
None yet
Development

No branches or pull requests

7 participants