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

Workaround for Windows DPI issues #10972

Merged
merged 4 commits into from Nov 2, 2017

Conversation

Projects
None yet
7 participants
@felixrieseberg
Member

felixrieseberg commented Oct 30, 2017

This isn't really a solution, this is more a workaround in case we can't find any real solution.

On Windows, multiple screens with different scaling factors result in the first call to setBounds being incorrect. Subsequent calls will place the window correctly. This dirty fix simply does that.

Ref #10862

@alespergl

This comment has been minimized.

Contributor

alespergl commented Oct 30, 2017

This definitely gets points for its embedded quality of invoking the spirits. :)
Yeah, why not. If it really helps... Doesn't seem like it could hurt anyway.

BTW I noticed today that some unit tests fail when executed in a high-DPI context. Fixing those tests might actually help us find the right solutions to these types of problems. I thought I would have a look at that at some point in near future after we're done with the current Chromium upgrade.

@@ -117,6 +117,14 @@ void NativeWindow::InitFromOptions(const mate::Dictionary& options) {
bool center;
if (options.Get(options::kX, &x) && options.Get(options::kY, &y)) {
SetPosition(gfx::Point(x, y));
#if DEFINEED(OS_WIN)

This comment has been minimized.

@mgc

mgc Oct 31, 2017

Contributor

defineEd?

@@ -117,6 +117,14 @@ void NativeWindow::InitFromOptions(const mate::Dictionary& options) {
bool center;
if (options.Get(options::kX, &x) && options.Get(options::kY, &y)) {
SetPosition(gfx::Point(x, y));
#if DEFINED(OS_WIN)

This comment has been minimized.

@poiru

poiru Oct 31, 2017

Member

I think this needs to be lowercase defined.

@felixrieseberg felixrieseberg changed the title from Discussion: Workaround for Windows DPI issues to Workaround for Windows DPI issues Oct 31, 2017

@@ -117,6 +117,14 @@ void NativeWindow::InitFromOptions(const mate::Dictionary& options) {
bool center;
if (options.Get(options::kX, &x) && options.Get(options::kY, &y)) {
SetPosition(gfx::Point(x, y));
#if defined(OS_WIN)
// Dirty, dirty workaround for

This comment has been minimized.

@zcbenz

zcbenz Nov 1, 2017

Contributor

Can you add FIXME(felixrieseberg): in the comment?

@bpasero

This comment has been minimized.

Contributor

bpasero commented Nov 1, 2017

To avoid calling SetPosition twice in cases where not needed, wouldn't it be better to immediately after SetPosition ask the window for its position and only call SetPosition again if the position is different? Also it seems that the code should not run when just one monitor is attached.

@zcbenz

zcbenz approved these changes Nov 2, 2017 edited

I'm good with this, and if no one opposes it I'm going to merge this one.

@zcbenz

This comment has been minimized.

Contributor

zcbenz commented Nov 2, 2017

To avoid calling SetPosition twice in cases where not needed, wouldn't it be better to immediately after SetPosition ask the window for its position and only call SetPosition again if the position is different? Also it seems that the code should not run when just one monitor is attached.

Setting window position is a rather cheap operation before window is shown, and it is only called twice when window is being created, so I think it is fine do it blindly here.

@bpasero

This comment has been minimized.

Contributor

bpasero commented Nov 2, 2017

@zcbenz ok fine 👍

@ckerr ckerr merged commit 4ceeddc into master Nov 2, 2017

8 of 9 checks passed

electron-mas-x64 Build #5618 failed in 19 min
Details
ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
electron-osx-x64 Build #5593 succeeded in 15 min
Details

@ckerr ckerr deleted the dpi-issues branch Nov 2, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment