starred/terminal
1
0
Fork 0
mirror of https://github.com/microsoft/terminal.git synced 2026-02-03 21:25:34 +00:00
Code Issues 19.0k Packages Projects Releases 20 Wiki Activity

OpenConsole not DpiAware, causes GetSystemMetrics to indicate poor icon sizes #22803

New Issue
Open
opened 2026-01-31 08:23:58 +00:00 by claunia · 5 comments
claunia commented 2026-01-31 08:23:58 +00:00
Owner
Copy Link

Originally created by @malxau-msft on GitHub (Jan 28, 2025).

Originally assigned to: @DHowett on GitHub.

Windows Terminal version

1.21.3231.0

Windows build number

10.0.22631

Other Software

(I was using Yori but I don't think you need to)

Steps to reproduce

  1. Set the display DPI to 125%
  2. Have a program that supports 20x20 and 40x40 icon sizes
  3. Run the program, and ConHost/OpenConsole will load the 16x16 icon and scale it up rather than load the 20x20 size that is native for this DPI

Expected Behavior

The optimal icon size for the DPI configuration.

Actual Behavior

ConExtractIconInBothSizesW calls GetSystemMetrics, and the debugger indicated that GetSystemMetrics returns the wrong size. Looking at the OpenConsole manifest showed no indication of DPI awareness, which explains its behavior. ConExtractIcons seems to have quite sophisticated logic to probe for these icon sizes and use them if present before falling back to more common sizes, but this is effectively disabled if GetSystemMetrics won't return the exotic sizes. Patching the return values from GetSystemMetrics was sufficient for the rest of OpenConsole to load and use the 20x20 icon.

Image

Originally created by @malxau-msft on GitHub (Jan 28, 2025). Originally assigned to: @DHowett on GitHub. ### Windows Terminal version 1.21.3231.0 ### Windows build number 10.0.22631 ### Other Software (I was using Yori but I don't think you need to) ### Steps to reproduce 1. Set the display DPI to 125% 2. Have a program that supports 20x20 and 40x40 icon sizes 3. Run the program, and ConHost/OpenConsole will load the 16x16 icon and scale it up rather than load the 20x20 size that is native for this DPI ### Expected Behavior The optimal icon size for the DPI configuration. ### Actual Behavior `ConExtractIconInBothSizesW` calls `GetSystemMetrics`, and the debugger indicated that `GetSystemMetrics` returns the wrong size. Looking at the OpenConsole manifest showed no indication of DPI awareness, which explains its behavior. `ConExtractIcons` seems to have quite sophisticated logic to probe for these icon sizes and use them if present before falling back to more common sizes, but this is effectively disabled if `GetSystemMetrics` won't return the exotic sizes. Patching the return values from `GetSystemMetrics` was sufficient for the rest of OpenConsole to load and use the 20x20 icon. ![Image](https://github.com/user-attachments/assets/1905a52b-33a9-4fb3-8eeb-524e7e85a464)
claunia commented 2026-01-31 08:24:00 +00:00
Author
Owner
Copy Link

@DHowett commented on GitHub (Jan 28, 2025):

Both conhost and OpenConsole absolutely should be DPI aware - that's how they are able to handle DPI transitions.

src\interactivity\win32\windowdpiapi.cpp
16:    return SetProcessDpiAwareness(PROCESS_PER_MONITOR_DPI_AWARE);
19:BOOL WindowDpiApi::SetProcessDpiAwarenessContext()
21:    return SetProcessDpiAwarenessContext(DPI_AWARENESS_CONTEXT_PER_MONITOR_AWARE_V2);
26:BOOL WindowDpiApi::SetProcessDpiAwarenessContext(_In_ DPI_AWARENESS_CONTEXT dpiContext)
31:        typedef int(WINAPI * PfnSetProcessDpiAwarenessContexts)(DPI_AWARENESS_CONTEXT dpiContext);
34:        static PfnSetProcessDpiAwarenessContexts pfn = nullptr;
38:            pfn = (PfnSetProcessDpiAwarenessContexts)GetProcAddress(_hUser32, "SetProcessDpiAwarenessContext");
51:    return SetProcessDpiAwarenessContext(dpiContext);

It's delay-loaded because it doesn't exist on non-user32 SKUs of Windows. I don't know why my forebears did not put it in the manifest. conhost has two manifests, which is an annoying accident of history that we should probably fix...

Though: Is there a chance this is because we extract icons before establishing per-process per-monitor DPI awareness/?

@DHowett commented on GitHub (Jan 28, 2025): Both conhost and OpenConsole absolutely should be DPI aware - that's how they are able to handle DPI transitions. ``` src\interactivity\win32\windowdpiapi.cpp 16: return SetProcessDpiAwareness(PROCESS_PER_MONITOR_DPI_AWARE); 19:BOOL WindowDpiApi::SetProcessDpiAwarenessContext() 21: return SetProcessDpiAwarenessContext(DPI_AWARENESS_CONTEXT_PER_MONITOR_AWARE_V2); 26:BOOL WindowDpiApi::SetProcessDpiAwarenessContext(_In_ DPI_AWARENESS_CONTEXT dpiContext) 31: typedef int(WINAPI * PfnSetProcessDpiAwarenessContexts)(DPI_AWARENESS_CONTEXT dpiContext); 34: static PfnSetProcessDpiAwarenessContexts pfn = nullptr; 38: pfn = (PfnSetProcessDpiAwarenessContexts)GetProcAddress(_hUser32, "SetProcessDpiAwarenessContext"); 51: return SetProcessDpiAwarenessContext(dpiContext); ``` It's delay-loaded because it doesn't exist on non-user32 SKUs of Windows. I don't know why my forebears did not put it in the manifest. conhost has two manifests, which is an annoying accident of history that we should probably fix... Though: Is there a chance this is because we extract icons before establishing per-process per-monitor DPI awareness/?
claunia commented 2026-01-31 08:24:00 +00:00
Author
Owner
Copy Link

@malxau-msft commented on GitHub (Jan 28, 2025):

Yes, that looks to be it.

Icons loaded at 1040035b55/src/host/srvinit.cpp (L168)

DPI awareness set at
1040035b55/src/host/srvinit.cpp (L230)

@malxau-msft commented on GitHub (Jan 28, 2025): Yes, that looks to be it. Icons loaded at https://github.com/microsoft/terminal/blob/1040035b5530ede83b4ac032a6c1edc86868a861/src/host/srvinit.cpp#L168 DPI awareness set at https://github.com/microsoft/terminal/blob/1040035b5530ede83b4ac032a6c1edc86868a861/src/host/srvinit.cpp#L230
claunia commented 2026-01-31 08:24:00 +00:00
Author
Owner
Copy Link

@DHowett commented on GitHub (Jan 28, 2025):

Honestly, we should probably dispense with this API and the delay load and all and just use the manifest. I can't foresee a reason that it won't work (though perhaps conhost having two manifests is the reason? maybe someone tried it and it didn't work because of that?)

@DHowett commented on GitHub (Jan 28, 2025): Honestly, we should probably dispense with this API and the delay load and all and just use the manifest. I can't foresee a reason that it won't work (though perhaps conhost having two manifests is the reason? maybe someone tried it and it didn't work because of that?)
claunia commented 2026-01-31 08:24:01 +00:00
Author
Owner
Copy Link

@DHowett commented on GitHub (Jan 28, 2025):

I wish we could get rid of all of our "High DPI" related manual delayloads, but...

  • GetSystemMetricsForDpi is in ext-ms-win-ntuser-private-l1 (private, despite being public) ✅
  • GetDpiForWindow is in ext-ms-win-ntuser-window-l1 ✅
  • SetProcessPerMonitorDpiAwareness and SetProcessDpiAwarenessContext are trivially replaceable with a manifest entry. ✅
  • AdjustWindowRectExForDpi is in user32 and cannot be delayloaded; we need to LL/GPA it ❌

Perfect won't be the enemy of good enough, but sheesh.
We did a bad job with apisets if an application like conhost which otherwise works on OneCore still needs to LL/GPA something from user32 because nobody put it in an ext- apiset... even though it's a sibling of other APIs which did get put into an ext- apiset.

@DHowett commented on GitHub (Jan 28, 2025): I wish we could get rid of all of our "High DPI" related manual delayloads, but... * `GetSystemMetricsForDpi` is in `ext-ms-win-ntuser-private-l1` (`private`, despite being public) ✅ * `GetDpiForWindow` is in `ext-ms-win-ntuser-window-l1` ✅ * `SetProcessPerMonitorDpiAwareness` and `SetProcessDpiAwarenessContext` are trivially replaceable with a manifest entry. ✅ * `AdjustWindowRectExForDpi` is in `user32` and cannot be delayloaded; we need to LL/GPA it :x: Perfect won't be the enemy of good enough, but _sheesh._ We did a bad job with apisets if an application like conhost _which otherwise works on OneCore_ still needs to LL/GPA something from user32 because nobody put it in an ext- apiset... even though it's a sibling of other APIs which _did_ get put into an ext- apiset.
claunia commented 2026-01-31 08:24:01 +00:00
Author
Owner
Copy Link

@DHowett commented on GitHub (Jan 30, 2025):

So! The problem is more ~ ~ faceted ~ ~ than that.

I did a bunch of digging and figured out why we have two manifests1 . That's not exactly germane here, so it's footnoted. Because of that, though, we set our manifest at runtime using CreateActCtxW.

8a806e0ac9/src/host/init.cpp (L31-L38)

I suspect that CreateActCtxW doesn't (or didn't?) propagate the DPI awareness bits to the right places, so we did it later. Or, the people who did the API calls didn't know about the manifest bits (and would have been hurt by them not working properly later.)

So, I fixed it.

It doesn't matter.

The code in ConExtractIcons is a direct copy of the code in shell32 for doing the same; we took this on in late 24H2 because ExtractIconW was moved from shell32 to Windows.Storage, and we got hit with a huge commit charge.

The code in shell32 clamps icon sizes to some arbitrary well-known four:

8a806e0ac9/src/interactivity/win32/icon.cpp (L22-L24)

That's fine - we can add 20 and 24 there, or add 64, or whatever.

But we're still using GetSystemMetrics before there is a Window, and before that Window is assigned to a Monitor:

8a806e0ac9/src/interactivity/win32/icon.cpp (L249-L250)

That always returns 16 and 32 because DPI-less system metrics are assessed against 96dpi2 .

The correct fix is fourfold...

  1. Move icon loading after we have a window.
  2. Use the DPI of that window to get the system metrics.
  3. Stop clamping icon sizes if we can help it, or clamp "better" and "smarter"
  4. Re-assess the icons when the window transitions to a new DPI!

Plus, I'm still gonna go ahead with removing the secondary manifest. It's got some wild stuff in it which I don't think we need anymore, but it's honestly difficult to tell.
 

  1. conhost is spawned with RtlCreateUserProcess sometimes, and by default that does not do any manifest loading(!) It can be made to. ↩︎

  2. I don't know if NTUSER has a metrics cache which changes based on the main window's monitor. IT MIGHT. ↩︎

@DHowett commented on GitHub (Jan 30, 2025): So! The problem is more ~ ~ faceted ~ ~ than that. I did a bunch of digging and figured out why we have two manifests[^1]. That's not _exactly_ germane here, so it's footnoted. Because of that, though, _we set our manifest at runtime_ using `CreateActCtxW`. https://github.com/microsoft/terminal/blob/8a806e0ac99003ef059641a3b503eefd969f6de7/src/host/init.cpp#L31-L38 I suspect that `CreateActCtxW` doesn't (or didn't?) propagate the DPI awareness bits to the right places, so we did it later. Or, the people who did the API calls didn't know about the manifest bits (and would have been hurt by them not working properly later.) So, I fixed it. It doesn't matter. The code in `ConExtractIcons` is a direct copy of the code in shell32 for doing the same; we took this on in late 24H2 because `ExtractIconW` was moved from shell32 to Windows.Storage, and we got hit with a _huge_ commit charge. The code in shell32 clamps icon sizes to some arbitrary well-known four: https://github.com/microsoft/terminal/blob/8a806e0ac99003ef059641a3b503eefd969f6de7/src/interactivity/win32/icon.cpp#L22-L24 That's fine - we can add 20 and 24 there, or add 64, or whatever. But we're still using `GetSystemMetrics` before there is a Window, and before that Window is assigned to a Monitor: https://github.com/microsoft/terminal/blob/8a806e0ac99003ef059641a3b503eefd969f6de7/src/interactivity/win32/icon.cpp#L249-L250 That always returns 16 and 32 because DPI-less system metrics are assessed against 96dpi[^2]. The correct fix is fourfold... 1. Move icon loading after we have a window. 2. Use the DPI of that window to get the system metrics. 3. Stop clamping icon sizes if we can help it, or clamp "better" and "smarter" 4. _Re-assess the icons when the window transitions to a new DPI!_ Plus, I'm still gonna go ahead with removing the secondary manifest. It's got some _wild_ stuff in it which I don't think we need anymore, but it's honestly difficult to tell.   [^1]: conhost is spawned with `RtlCreateUserProcess` sometimes, and by default that does not do any manifest loading(!) It can be made to. [^2]: I don't know if NTUSER has a metrics cache which changes based on the main window's monitor. IT MIGHT.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
⛺ Reserved
A11yCO
A11yMAS
A11ySev1
A11ySev2
A11ySev3
A11yTTValidated
A11yUsable
A11yVoiceAccess
A11yWCAG
Area-Accessibility
Area-AtlasEngine
Area-AzureShell
Area-Build
Area-Build
Area-Chat
Area-CmdPal
Area-CodeHealth
Area-Commandline
Area-CookedRead
Area-DefApp
Area-Extensibility
Area-Fonts
Area-GroupPolicy
Area-i18n
Area-Input
Area-Interaction
Area-Interop
Area-Localization
Area-Output
Area-Performance
Area-Portable
Area-Quality
Area-Remoting
Area-Rendering
Area-Schema
Area-Server
Area-Settings
Area-SettingsUI
Area-ShellExtension
Area-ShellExtension
Area-ShellExtension
Area-Suggestions
Area-Suggestions
Area-TerminalConnection
Area-TerminalControl
Area-Theming
Area-UserInterface
Area-VT
Area-Windowing
Area-WPFControl
AutoMerge
Blocking-Ingestion
Culprit-Centennial
Culprit-WinUI
Disability-All
Disability-Blind
Disability-LowVision
Disability-Mobility
External-Blocked-WinUI3
Fixed
Gathering-Data
good first issue
HCL-E+D
HCL-WindowsTerminal
Help Wanted
Impact-Compatibility
Impact-Compliance
Impact-Correctness
Impact-Visual
In-PR
InclusionBacklog
InclusionBacklog-Windows TerminalWin32
InclusionCommitted-202206
Issue-Bug
Issue-Docs
Issue-Feature
Issue-Feature
Issue-Question
Issue-Samples
Issue-Scenario
Issue-Task
Needs-Attention
Needs-Author-Feedback
Needs-Bisect
Needs-Discussion
Needs-Repro
Needs-Tag-Fix
Needs-Tag-Fix
Needs-Triage
No-Recent-Activity
Priority-0
Priority-1
Priority-2
Priority-3
Product-Cmd.exe
Product-Colortool
Product-Colortool
Product-Colortool
Product-Conhost
Product-Conpty
Product-Meta
Product-Powershell
Product-Terminal
Product-WSL
pull-request
Mirrored from GitHub Pull Request
 Resolution-Answered
Resolution-By-Design
Resolution-Duplicate
Resolution-External
Resolution-Fix-Available
Resolution-Fix-Committed
Resolution-No-Repro
Resolution-Won't-Fix
Severity-Blocking
Severity-Crash
Severity-DataLoss
spam
this-will-be-a-breaking-change
Tracking-External
WindowsTerminal_Win32
Work-Item
zAskModeBug
zInbox-Bug
No Label Area-Server Issue-Bug Issue-Task Priority-3 Product-Conhost zInbox-Bug
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
claunia
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: starred/terminal#22803
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?