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

Enable visual styles for host error dialog via activation context #76762

Merged
merged 3 commits into from
Oct 14, 2022

Conversation

elinor-fung
Copy link
Member

@elinor-fung elinor-fung commented Oct 7, 2022

Make apphost on Windows enable visual styles using the WindowsShell manifest such that a modern-themed dialog can be shown for runtime/framework not found errors in GUI applications.

This is just creating an activation context using the manifest, not trying to show a win32 task dialog yet. This is a prerequisite for using the task dialog though.

Before:
image

After:
image

Contributes to #71288.

cc @richlander

@ghost
Copy link

ghost commented Oct 7, 2022

Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov
See info in area-owners.md if you want to be subscribed.

Issue Details

Embed a manifest into apphost on Windows such that it can be used to enabled a modern-themed dialog when we show runtime/framework not found errors in GUI applications.

This is just creating an activation context using the manifest, not trying to show a win32 task dialog yet. This is a prerequisite for using the task dialog though.

This is +1kB to apphost on Windows.

Before:
image

After:
image

Contributes to #71288.

cc @richlander

Author: elinor-fung
Assignees: -
Labels:

area-Host

Milestone: -

Copy link
Contributor

@deeprobin deeprobin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -0,0 +1,15 @@
<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
<assembly xmlns="urn:schemas-microsoft-com:asm.v1" manifestVersion="1.0">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it also be possible to set the default DPI awareness in the manifest, like this:

Suggested change
<assembly xmlns="urn:schemas-microsoft-com:asm.v1" manifestVersion="1.0">
<assembly xmlns="urn:schemas-microsoft-com:asm.v1" manifestVersion="1.0" xmlns:asmv3="urn:schemas-microsoft-com:asm.v3">
<asmv3:application>
<asmv3:windowsSettings>
<dpiAware xmlns="http://schemas.microsoft.com/SMI/2005/WindowsSettings">true/pm</dpiAware>
<dpiAwareness xmlns="http://schemas.microsoft.com/SMI/2016/WindowsSettings">PerMonitorV2,PerMonitor</dpiAwareness>
</asmv3:windowsSettings>
</asmv3:application>

It will prevent the dialog box from being blurry on high DPI monitors (like it is in the image in the PR description). There aren't any code changes required since MessageBox (and Task Dialog) knows how to scale itself when the proper DPI mode is selected. (The specific set of options here makes it compatible with all of the legacy high DPI modes in Windows 7 and 8.x.)

Copy link
Member Author

@elinor-fung elinor-fung Oct 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, that seems to be one of those settings that has to be in the process manifest and doesn't work with programmatically creating/activating a context.

It might be worth looking into what we can do and what it would cost though. I did see the APIs available starting with one of the Win10 updates that allow controlling DPI awareness for the thread, but I'd rather keep investigating that separate from getting a manifest / activation context in for modern theming.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh—that's too bad. I see now from the docs that the activation context only holds the SxS information. I am sorry, I should have tried with the activation context before suggesting this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's a great suggestion, and it's unfortunate it doesn't work the simple way. I agree that we should investigate the APIs a separate work item - if nothing else it would need some light-up code and so on.

src/native/corehost/apphost/resource.windows.h Outdated Show resolved Hide resolved
src/native/corehost/apphost/common_controls.manifest Outdated Show resolved Hide resolved
src/native/corehost/apphost/apphost.windows.cpp Outdated Show resolved Hide resolved
src/native/corehost/apphost/apphost.windows.cpp Outdated Show resolved Hide resolved
src/native/corehost/apphost/apphost.windows.cpp Outdated Show resolved Hide resolved
src/native/corehost/apphost/apphost.windows.cpp Outdated Show resolved Hide resolved
src/native/corehost/apphost/apphost.windows.cpp Outdated Show resolved Hide resolved
Copy link
Member

@vitek-karas vitek-karas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks so simple :-) (I know it's not)

@elinor-fung elinor-fung merged commit 8bc9a07 into dotnet:main Oct 14, 2022
@elinor-fung elinor-fung deleted the actctx branch October 14, 2022 06:08
@ghost ghost locked as resolved and limited conversation to collaborators Nov 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants