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 per-monitor DPI for Win10 #8786

Merged
merged 1 commit into from Apr 12, 2017

Conversation

@zcbenz
Contributor

zcbenz commented Feb 27, 2017

Close #5429.

@zcbenz zcbenz removed the in progress label Feb 27, 2017

@groundwater groundwater self-requested a review Feb 28, 2017

@groundwater

This comment has been minimized.

Show comment
Hide comment
@groundwater

groundwater Feb 28, 2017

Member

I will test this with my Surface Pro 4 and non-high-DPI external monitor.

Member

groundwater commented Feb 28, 2017

I will test this with my Surface Pro 4 and non-high-DPI external monitor.

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Apr 3, 2017

Contributor

I will test this with my Surface Pro 4 and non-high-DPI external monitor.

@groundwater have you had a chance to test this out yet?

Contributor

kevinsawicki commented Apr 3, 2017

I will test this with my Surface Pro 4 and non-high-DPI external monitor.

@groundwater have you had a chance to test this out yet?

@miniak

Please consider keeping the manifest entry instead of adding back the code.

@@ -32,7 +32,6 @@
<asmv3:application xmlns:asmv3="urn:schemas-microsoft-com:asm.v3">
<asmv3:windowsSettings xmlns="http://schemas.microsoft.com/SMI/2005/WindowsSettings">
<dpiAware>true</dpiAware>

This comment has been minimized.

@miniak
@miniak

miniak Apr 4, 2017

Contributor

This comment has been minimized.

@alespergl

alespergl Apr 5, 2017

Contributor

I also think setting this via manifest is preferrable. You can also combine the <dpiAware> element with the new <dpiAwareness> element for Windows 10 build 1607 and higher to enable additional per-monitor DPI features.
https://msdn.microsoft.com/en-us/library/windows/desktop/aa374191.aspx

@alespergl

alespergl Apr 5, 2017

Contributor

I also think setting this via manifest is preferrable. You can also combine the <dpiAware> element with the new <dpiAwareness> element for Windows 10 build 1607 and higher to enable additional per-monitor DPI features.
https://msdn.microsoft.com/en-us/library/windows/desktop/aa374191.aspx

This comment has been minimized.

@clarkezone
@clarkezone

clarkezone Apr 8, 2017

You may also want to look at what we added in Windows 10 Creators update which is detailed here: https://blogs.windows.com/buildingapps/2017/04/04/high-dpi-scaling-improvements-desktop-applications-windows-10-creators-update/#HBK3Qfrm4qbzWAEV.97

This comment has been minimized.

@peterfelts

peterfelts Apr 10, 2017

Non-client area scaling is available via the following:

  1. PerMonitor tag + a call to EnableNonClinetDpiScaling() on 1607
  2. PerMonitorV2 on 1703 (no need for that additional API call)

You can also use a combination of tags that will be consumed differently on different versions of Windows:
true/pm (this will give you per-monitor (version 1) on >= 8.1 and system awareness on Vista to 8)
PerMonitorV2, PerMonitor will result in PerMonitorV2 on 1703 (Creators Update) and PerMonitor (version 1) on 1607 (Anniversary Update). When running on versions of Windows that consume this tag the tag will be ignored.

@peterfelts

peterfelts Apr 10, 2017

Non-client area scaling is available via the following:

  1. PerMonitor tag + a call to EnableNonClinetDpiScaling() on 1607
  2. PerMonitorV2 on 1703 (no need for that additional API call)

You can also use a combination of tags that will be consumed differently on different versions of Windows:
true/pm (this will give you per-monitor (version 1) on >= 8.1 and system awareness on Vista to 8)
PerMonitorV2, PerMonitor will result in PerMonitorV2 on 1703 (Creators Update) and PerMonitor (version 1) on 1607 (Anniversary Update). When running on versions of Windows that consume this tag the tag will be ignored.

This comment has been minimized.

@ouned

ouned Apr 10, 2017

I can probably work on this and I also have a combination of High/Low DPI screens running.
I think EnableNonClientDpiScaling is really important (or permonitorv2), i wouldn't go for true/pm only

@ouned

ouned Apr 10, 2017

I can probably work on this and I also have a combination of High/Low DPI screens running.
I think EnableNonClientDpiScaling is really important (or permonitorv2), i wouldn't go for true/pm only

This comment has been minimized.

@zcbenz

zcbenz Apr 11, 2017

Contributor

you can use True/PM instead of adding back the code

I didn't know this, it is much better!

PerMonitor tag + a call to EnableNonClinetDpiScaling() on 1607
PerMonitorV2 on 1703 (no need for that additional API call)

I agree it is important to have these, but I'm not sure whether they work well with Chromium's own UI framework. Can someone who has Windows running on multiple monitors test it?

@zcbenz

zcbenz Apr 11, 2017

Contributor

you can use True/PM instead of adding back the code

I didn't know this, it is much better!

PerMonitor tag + a call to EnableNonClinetDpiScaling() on 1607
PerMonitorV2 on 1703 (no need for that additional API call)

I agree it is important to have these, but I'm not sure whether they work well with Chromium's own UI framework. Can someone who has Windows running on multiple monitors test it?

This comment has been minimized.

@alespergl

alespergl Apr 11, 2017

Contributor

I actually plan to have a look at this. Leave it to me then, I'll make it work across all the different scenarios.

@alespergl

alespergl Apr 11, 2017

Contributor

I actually plan to have a look at this. Leave it to me then, I'll make it work across all the different scenarios.

This comment has been minimized.

@poiru
@poiru

poiru Apr 11, 2017

Member
@alespergl

Let's keep using the manifest and adjust the value to true/pm

@poiru

This comment has been minimized.

Show comment
Hide comment
@poiru

poiru Apr 12, 2017

Member

What do we think of merging this? I agree that we should ideally also have

<dpiAwareness xmlns="http://schemas.microsoft.com/SMI/2016/WindowsSettings">permonitorv2, permonitor</dpiAwareness>

... but it would be nice to have at least <dpiAware>true/pm</dpiAware> until we can confirm that permonitorv2 works properly.

Member

poiru commented Apr 12, 2017

What do we think of merging this? I agree that we should ideally also have

<dpiAwareness xmlns="http://schemas.microsoft.com/SMI/2016/WindowsSettings">permonitorv2, permonitor</dpiAwareness>

... but it would be nice to have at least <dpiAware>true/pm</dpiAware> until we can confirm that permonitorv2 works properly.

@kevinsawicki kevinsawicki merged commit 1c44bcf into master Apr 12, 2017

9 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
electron-linux-arm Build #6190783 succeeded in 73s
Details
electron-linux-ia32 Build #6190784 succeeded in 65s
Details
electron-linux-x64 Build #6190785 succeeded in 152s
Details
electron-mas-x64 Build #3873 succeeded in 8 min 40 sec
Details
electron-osx-x64 Build #3865 succeeded in 8 min 44 sec
Details
electron-win-ia32 Build #2855 succeeded in 8 min 17 sec
Details
electron-win-x64 Build #2828 succeeded in 8 min 28 sec
Details

@kevinsawicki kevinsawicki deleted the per-monitor-dpi-aware branch Apr 12, 2017

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Apr 12, 2017

Contributor

What do we think of merging this? I agree that we should ideally also have

Yeah, sounds good 🚢

We can follow this up with more changes as well as pulling in anything via the Chrome 58 upgrade.

Contributor

kevinsawicki commented Apr 12, 2017

What do we think of merging this? I agree that we should ideally also have

Yeah, sounds good 🚢

We can follow this up with more changes as well as pulling in anything via the Chrome 58 upgrade.

@alespergl

This comment has been minimized.

Show comment
Hide comment
@alespergl

alespergl Apr 12, 2017

Contributor

When is Chrome 58 coming in? The change @poiru mentioned would likely need to be backported if it's delayed.

Contributor

alespergl commented Apr 12, 2017

When is Chrome 58 coming in? The change @poiru mentioned would likely need to be backported if it's delayed.

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Apr 12, 2017

Contributor

When is Chrome 58 coming in?

I believe @zcbenz is working through some offscreen window compilation issues right now that are being updated in #9116, no definite timeline at this time.

The change @poiru mentioned would likely need to be backported if it's delayed.

Understood, maybe we wait until the end of next week to see where Chrome 58 is then?

Contributor

kevinsawicki commented Apr 12, 2017

When is Chrome 58 coming in?

I believe @zcbenz is working through some offscreen window compilation issues right now that are being updated in #9116, no definite timeline at this time.

The change @poiru mentioned would likely need to be backported if it's delayed.

Understood, maybe we wait until the end of next week to see where Chrome 58 is then?

@alespergl

This comment has been minimized.

Show comment
Hide comment
@alespergl

alespergl Apr 13, 2017

Contributor

@kevinsawicki I created a PR for the backport, just in case.

Contributor

alespergl commented Apr 13, 2017

@kevinsawicki I created a PR for the backport, just in case.

@poiru

This comment has been minimized.

Show comment
Hide comment
@poiru

poiru Apr 17, 2017

Member

On Win10 version 1607, this patch made the titlebar, scrollbars, etc. huge:

titlebar

Maybe EnableNonClinetDpiScaling() will fix this, but that's not available on all Windows 10 versions. Should we revert this patch? cc @alespergl

Member

poiru commented Apr 17, 2017

On Win10 version 1607, this patch made the titlebar, scrollbars, etc. huge:

titlebar

Maybe EnableNonClinetDpiScaling() will fix this, but that's not available on all Windows 10 versions. Should we revert this patch? cc @alespergl

@peterfelts

This comment has been minimized.

Show comment
Hide comment
@peterfelts

peterfelts Apr 17, 2017

So it looks like before this change the process ran as system aware <dpiAware>true</dpiAware> and the change made it run under the original per-monitor mode <dpiAware>true/pm</dpiAware>. This would result in what @poiru is seeing: non-client area won't DPI scale on a DPI change. Getting the non-client area to scale (if Windows is drawing the non-client area, that is) wasn't supported until 1607 (via a call to EnableNonClinetDpiSacling(HWND)). What I would recommend is:

<dpiAware>true</dpiAware>
<dpiAwareness>PerMonitor</dpiAwareness>

  • call EnableNonClientDpiScaling()

This will result in the following behavior:

  1. Vista -> 1607 -> System Awareness (what I think was used before this change)
  2. >= 1607 -> per-monitor awareness and the non-client area will scale

Again, the non-client area will only scale if Windows is drawing it. I don't know how Electron draws its non-client area though. If its custom drawn, then Windows won't DPI scale it automatically.

peterfelts commented Apr 17, 2017

So it looks like before this change the process ran as system aware <dpiAware>true</dpiAware> and the change made it run under the original per-monitor mode <dpiAware>true/pm</dpiAware>. This would result in what @poiru is seeing: non-client area won't DPI scale on a DPI change. Getting the non-client area to scale (if Windows is drawing the non-client area, that is) wasn't supported until 1607 (via a call to EnableNonClinetDpiSacling(HWND)). What I would recommend is:

<dpiAware>true</dpiAware>
<dpiAwareness>PerMonitor</dpiAwareness>

  • call EnableNonClientDpiScaling()

This will result in the following behavior:

  1. Vista -> 1607 -> System Awareness (what I think was used before this change)
  2. >= 1607 -> per-monitor awareness and the non-client area will scale

Again, the non-client area will only scale if Windows is drawing it. I don't know how Electron draws its non-client area though. If its custom drawn, then Windows won't DPI scale it automatically.

@alespergl

This comment has been minimized.

Show comment
Hide comment
@alespergl

alespergl Apr 17, 2017

Contributor

@poiru Yes, EnableNonClientDpiScaling fixes it. There's no way to make this work universally, unless custom non-client drawing is implemented, or we don't do per-monitor DPI pre Win10 1607. That would result in blurred client area. Other per-monitor apps work the same. I think it's reasonable to expect most users upgrading their Win10 systems to the latest version, so I wouldn't optimize too much for earlier versions.
Btw dpiAwareness makes no difference here, it's simply a shortcoming of Windows.

Contributor

alespergl commented Apr 17, 2017

@poiru Yes, EnableNonClientDpiScaling fixes it. There's no way to make this work universally, unless custom non-client drawing is implemented, or we don't do per-monitor DPI pre Win10 1607. That would result in blurred client area. Other per-monitor apps work the same. I think it's reasonable to expect most users upgrading their Win10 systems to the latest version, so I wouldn't optimize too much for earlier versions.
Btw dpiAwareness makes no difference here, it's simply a shortcoming of Windows.

poiru added a commit that referenced this pull request Apr 18, 2017

Update libcc for electron/libchromiumcontent#285
This fixes non-client area DPI scaling on recent Windows 10 versions.
See discussion in #8786.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment