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

fix: handle WM_GETMINMAXINFO instead of letting chromium do it #19928

Merged
merged 3 commits into from
Aug 28, 2019

Conversation

brenca
Copy link
Contributor

@brenca brenca commented Aug 24, 2019

Description of Change

There is a workaround I introduced that's no longer needed and is causing bugs, so this PR removes that. We had that workaround because during the WM_GETMINMAXINFO message Windows reports a position that's incorrect (somewhere around -32000 for both coordinates), and the handler in chromium calculates the scale factor for the window based on that position, which will be incorrect if you have more than one monitor (the leftmost will be used).

To work around this, we instead handle the WM_GETMINMAXINFO message ourselves, similarly to how chromium does it, but without a size compensation that's not required for us (it compensates for the window frame size, but we expect the minimum and maximum sizes to apply to the window anyway by default, and that compensation code is from 2011, doesn't care about scale factors, and probably should be removed/corrected in chromium anyway).

I'm going to look into upstreaming this new workaround/fix so that we can later remove it altogether.

Depends on #19886.

Fixes #19423.
Fixes #19816.

Checklist

Release Notes

Notes: Fixed a bug where windows would sometimes shrink to 0 size after being restored on Windows.

@whyboris
Copy link

Thank you @brenca for the PR -- it will likely fix my issue: whyboris/Video-Hub-App#260 🙇

Copy link
Member

@ckerr ckerr left a comment

Choose a reason for hiding this comment

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

@brenca the diff itself looks fine, but could you quickly summarize why this workaround is no longer needed?

@brenca
Copy link
Contributor Author

brenca commented Aug 25, 2019

@ckerr thanks for the quick review! I realized that removing the workaround does re-introduce a bug, so I changed the code to address that bug and still fix the issues mentioned above.

I'll do some additional testing to make sure things work alright.

@brenca brenca changed the title fix: remove WM_GETMINMAXINFO workaround since it's no longer needed [wip] fix: handle WM_GETMINMAXINFO instead of letting chromium do it Aug 25, 2019
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Aug 25, 2019
@brenca brenca force-pushed the brenca/restore-after-minimize branch from 032073f to 256da70 Compare August 25, 2019 21:07
@brenca brenca requested a review from ckerr August 25, 2019 21:25
@brenca
Copy link
Contributor Author

brenca commented Aug 25, 2019

@ckerr I added some explanatory comments and made some changes, can I get another review from you?

@brenca brenca changed the title [wip] fix: handle WM_GETMINMAXINFO instead of letting chromium do it fix: handle WM_GETMINMAXINFO instead of letting chromium do it Aug 25, 2019
Copy link
Member

@codebytere codebytere left a comment

Choose a reason for hiding this comment

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

lgtm given added context

@zcbenz zcbenz merged commit 27ce6a9 into master Aug 28, 2019
@release-clerk
Copy link

release-clerk bot commented Aug 28, 2019

Release Notes Persisted

Fixed a bug where windows would sometimes shrink to 0 size after being restored on Windows.

@zcbenz zcbenz deleted the brenca/restore-after-minimize branch August 28, 2019 00:34
@trop
Copy link
Contributor

trop bot commented Aug 28, 2019

I was unable to backport this PR to "6-0-x" cleanly;
you will need to perform this backport manually.

@trop
Copy link
Contributor

trop bot commented Aug 28, 2019

I was unable to backport this PR to "7-0-x" cleanly;
you will need to perform this backport manually.

brenca added a commit that referenced this pull request Aug 28, 2019
* fix: remove WM_GETMINMAXINFO workaround since it's no longer needed

* fix: handle WM_GETMINMAXINFO ourselves

* fix: remove part of the chromium WM_GETMINMAXINFO handler
@trop trop bot added the in-flight/7-0-x label Aug 28, 2019
brenca added a commit that referenced this pull request Aug 28, 2019
* fix: remove WM_GETMINMAXINFO workaround since it's no longer needed

* fix: handle WM_GETMINMAXINFO ourselves

* fix: remove part of the chromium WM_GETMINMAXINFO handler
@trop
Copy link
Contributor

trop bot commented Aug 28, 2019

A maintainer has manually backported this PR to "6-0-x", please check out #20001

ckerr pushed a commit that referenced this pull request Aug 28, 2019
… (#20000)

* fix: remove WM_GETMINMAXINFO workaround since it's no longer needed

* fix: handle WM_GETMINMAXINFO ourselves

* fix: remove part of the chromium WM_GETMINMAXINFO handler
ckerr pushed a commit that referenced this pull request Aug 28, 2019
… (#20001)

* fix: remove WM_GETMINMAXINFO workaround since it's no longer needed

* fix: handle WM_GETMINMAXINFO ourselves

* fix: remove part of the chromium WM_GETMINMAXINFO handler
@whyboris
Copy link

whyboris commented Aug 29, 2019

Thank you @brenca for working on this bugfix 🙇

Your PR probably closed the other bug, but mine persists as of 6.0.5: #19816 (comment)

@whyboris
Copy link

whyboris commented Aug 30, 2019

False alarm -- seems like this PR has not been merged into 6.0.5 but instead was released with 6.0.6 https://github.com/electron/electron/releases/tag/v6.0.6 (via #20001 as a backport of this code change) 🙇
I pulled in the code and everything works beautifully 😍

❤️ Thank you @brenca, the Electron team, and all those who help with it 🙇

@carterbs
Copy link
Contributor

@codebytere this PR is causing issues for us on Electron 6 and 7. When we call setBounds on a hidden window, and then show that window on a monitor with a different display scale than the primary....it's the wrong size. But Electron thinks it's the correct size. We've narrowed it down to this PR causing the bad behavior.

The reason we do this is that we have menus that are hidden, and change size based on the contents within the menus. So this bug makes our app look awful in the latest electron versions.

I've reached out to @brenca on slack to see if he has any ideas of how to fix this. It's presently preventing us from keeping up with the latest electron release.

@sgd2z
Copy link
Contributor

sgd2z commented Oct 22, 2019

@codebytere this PR is causing issues for us on Electron 6 and 7. When we call setBounds on a hidden window, and then show that window on a monitor with a different display scale than the primary....it's the wrong size. But Electron thinks it's the correct size. We've narrowed it down to this PR causing the bad behavior.

The reason we do this is that we have menus that are hidden, and change size based on the contents within the menus. So this bug makes our app look awful in the latest electron versions.

I've reached out to @brenca on slack to see if he has any ideas of how to fix this. It's presently preventing us from keeping up with the latest electron release.

After some testing, I found that I can only reproduce when resizable is false. I'm working on a reproducer.

deepak1556 added a commit that referenced this pull request Nov 13, 2019
deepak1556 added a commit that referenced this pull request Nov 15, 2019
deepak1556 added a commit that referenced this pull request Nov 15, 2019
deepak1556 added a commit that referenced this pull request Nov 15, 2019
deepak1556 added a commit that referenced this pull request Nov 15, 2019
* Revert "fix: handle WM_GETMINMAXINFO instead of letting chromium do it (#19928)"

This reverts commit 27ce6a9.

* fix: don't reset the width and height when correcting window placement
deepak1556 added a commit that referenced this pull request Nov 15, 2019
* Revert "fix: handle WM_GETMINMAXINFO instead of letting chromium do it (#19928)"

This reverts commit 27ce6a9.

* fix: don't reset the width and height when correcting window placement
codebytere pushed a commit that referenced this pull request Nov 18, 2019
* Revert "fix: handle WM_GETMINMAXINFO instead of letting chromium do it (#19928) (#20000)"

This reverts commit 182f63d.

* fix: don't reset the width and height when correcting window placement
codebytere pushed a commit that referenced this pull request Nov 18, 2019
* Revert "fix: handle WM_GETMINMAXINFO instead of letting chromium do it (#19928) (#20001)"

This reverts commit fb82bfa.

* fix: don't reset the width and height when correcting window placement
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Electron 6 - restore after minimize has wrong window size Electron 6, maximize/restore not working correctly
7 participants