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: make 'setParentWindow' compatible under Windows #15775

Merged

Conversation

Projects
None yet
4 participants
@neo291
Copy link
Contributor

commented Nov 20, 2018

Description of Change

Make 'win.setParentWindow(parent)' available also under Windows.
To set parentship between windows into Windows is better to play with the owner instead of the parent, as Windows natively seems to do if a parent is specified at window creation time.
This change also include a fix for a bad behaviour when one parent is changed with another, previously the child were not removed from the old parent.

Checklist

  • PR description included and stakeholders cc'd
  • npm test passes
  • tests are changed or added
  • relevant documentation is changed or added
  • PR title follows semantic commit guidelines

Release Notes

Notes: 'win.setParentWindow(parent)' available also under Windows

@neo291 neo291 requested review from as code owners Nov 20, 2018

@brenca
Copy link
Member

left a comment

I'm fine with the Winapi parts, but it would be nice to get some tests for this if we can, and if possible, we should get those kWindowDefault*Styles from chromium directly, but my guess is that that would involve a small patch. Not sure about the tradeoff here between patch and code duplication, so /cc @nornagon

@nornagon
Copy link
Contributor

left a comment

I think duplicating those constants is OK, they're unlikely to change. But the kWindowDefaultChildStyle defined here is missing WS_VISIBLE compared to //ui/gfx/win/window_impl.cc.

I'm not familiar enough with the windows API to say whether this is correct, but it certainly looks complicated 😀

@neo291 neo291 force-pushed the neo291:fix-set-parent-window-for-windows branch from bcf7d8c to 16a7b13 Nov 20, 2018

@neo291

This comment has been minimized.

Copy link
Contributor Author

commented Nov 21, 2018

About the test:
The test already exists, I'm sorry, I've forgot to remove the piece of code that prevent it from running on Windows.
Now I've removed the preventing part, it should check also for Windows.

About the constants:
Yes they are unlikely to change for me too.
I've duplicated it maintaining the name that is used in chrome to make easy to search in the code in case of future comparison, the constants hasn't to be necessary the same, the missing WS_VISIBLE is an example of this.
The missing WS_VISIBLE is intentional, because if I use it for change the style in that phase when the window is already shown, I'll systematically hide that window in some cases, causing a bad behavior, instead use it in the creation phase, as chromium does, doesn't cause any problem.
I've just added a small comment before the constants to remember this thing about the WS_VISIBLE for the future.

@neo291 neo291 force-pushed the neo291:fix-set-parent-window-for-windows branch from 16a7b13 to 42a9b57 Nov 21, 2018

@brenca

brenca approved these changes Nov 21, 2018

Copy link
Member

left a comment

Nice, LGTM then! 👍

@neo291

This comment has been minimized.

Copy link
Contributor Author

commented Nov 21, 2018

But for the final merge I ask to wait some more time, I'll want to check one more strange thing I've noticed just now, that I hadn't noticed before.
I'll investigate more and apply other fixes if are required, I think that is required to change some more style flags.
I'll let you know ASAP.

@brenca

This comment has been minimized.

Copy link
Member

commented Nov 21, 2018

@neo291 sure, you could add [WIP] to the beginning of the title of the PR temporarily to make sure it's not merged

@neo291 neo291 changed the title fix: make 'setParentWindow' compatible under Windows [WIP] fix: make 'setParentWindow' compatible under Windows Nov 21, 2018

@neo291

This comment has been minimized.

Copy link
Contributor Author

commented Nov 21, 2018

@brenca that's perfect thank you!

@neo291 neo291 force-pushed the neo291:fix-set-parent-window-for-windows branch from 42a9b57 to 3ba7489 Nov 21, 2018

@neo291 neo291 changed the title [WIP] fix: make 'setParentWindow' compatible under Windows fix: make 'setParentWindow' compatible under Windows Nov 21, 2018

@neo291

This comment has been minimized.

Copy link
Contributor Author

commented Nov 21, 2018

Now I've finished!

At the end, by analyzing one more time and more deeper with the Spy++ the window native creation as a non-child compared to a child, it resulted that the window styles are the same for a child and a non-child, without need to change them, the only thing that need to be change is the owner.

Seems that in some point of the chromium code the WS_CHILD and other styles are changed and despite it is a child window and is owned by another window the style is set as the same of a normal non-child window, that have a sense for a child that can have other more child in turn.

Moreover, being the style the same for a non-child and a child window is removed the need of the constants derived from chromium and is automatically solved also the possible constants duplication doubt.

Thanks for your patience and sorry for the delay.

@neo291 neo291 force-pushed the neo291:fix-set-parent-window-for-windows branch 2 times, most recently from 15936ad to 3710d14 Nov 27, 2018

@neo291 neo291 force-pushed the neo291:fix-set-parent-window-for-windows branch from 3710d14 to b73a163 Dec 6, 2018

@neo291

This comment has been minimized.

Copy link
Contributor Author

commented Dec 6, 2018

UPDATE: I've removed a line (that I've found for a fluke) into the documentation where is stated that the change of parent is not supported in Windows, that is no more true with this change.
Also I've align sources with master.

@codebytere
Copy link
Member

left a comment

approving on behalf of docs

@neo291 neo291 force-pushed the neo291:fix-set-parent-window-for-windows branch from b73a163 to 195d9b2 Dec 12, 2018

@neo291

This comment has been minimized.

Copy link
Contributor Author

commented Dec 12, 2018

@nornagon is possible to merge this pull request or there is something blocking?

@codebytere

This comment has been minimized.

Copy link
Member

commented Dec 12, 2018

@neo291 i'm going to rekick CI and then i'll get this merged :)

@neo291

This comment has been minimized.

Copy link
Contributor Author

commented Dec 12, 2018

@codebytere ok, thanks :)

@codebytere codebytere merged commit 649633b into electron:master Dec 13, 2018

22 of 23 checks passed

ci/circleci: mas-testing-tests Your tests failed on CircleCI
Details
Absolute Zero
Artifact Comparison No Changes
Details
Semantic Pull Request ready to be squashed
Details
WIP ready for review
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
ci/circleci: linux-arm-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-arm-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-arm64-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-arm64-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-checkout Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-testing-tests Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-testing-tests Your tests passed on CircleCI!
Details
ci/circleci: mas-testing Your tests passed on CircleCI!
Details
ci/circleci: osx-testing Your tests passed on CircleCI!
Details
ci/circleci: osx-testing-tests Your tests passed on CircleCI!
Details
electron-lint Build #20181212.3 succeeded
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

commented Dec 13, 2018

Release Notes Persisted

'win.setParentWindow(parent)' available also under Windows

@neo291 neo291 deleted the neo291:fix-set-parent-window-for-windows branch Dec 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.