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

neo291
Copy link
Contributor

@neo291 neo291 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 a team November 20, 2018 12:46
Copy link
Contributor

@brenca brenca left a comment

Choose a reason for hiding this comment

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

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

Copy link
Member

@nornagon nornagon left a comment

Choose a reason for hiding this comment

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

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 fix-set-parent-window-for-windows branch from bcf7d8c to 16a7b13 Compare November 20, 2018 23:35
@neo291
Copy link
Contributor Author

neo291 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 fix-set-parent-window-for-windows branch from 16a7b13 to 42a9b57 Compare November 21, 2018 07:48
Copy link
Contributor

@brenca brenca left a comment

Choose a reason for hiding this comment

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

Nice, LGTM then! 👍

@neo291
Copy link
Contributor Author

neo291 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
Copy link
Contributor

brenca 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
Copy link
Contributor Author

neo291 commented Nov 21, 2018

@brenca that's perfect thank you!

@neo291 neo291 force-pushed the fix-set-parent-window-for-windows branch from 42a9b57 to 3ba7489 Compare November 21, 2018 21:25
@neo291 neo291 changed the title [WIP] fix: make 'setParentWindow' compatible under Windows fix: make 'setParentWindow' compatible under Windows Nov 21, 2018
@neo291
Copy link
Contributor Author

neo291 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 fix-set-parent-window-for-windows branch 2 times, most recently from 15936ad to 3710d14 Compare November 29, 2018 14:04
@neo291 neo291 force-pushed the fix-set-parent-window-for-windows branch from 3710d14 to b73a163 Compare December 6, 2018 11:18
@neo291
Copy link
Contributor Author

neo291 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.

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.

approving on behalf of docs

@neo291 neo291 force-pushed the fix-set-parent-window-for-windows branch from b73a163 to 195d9b2 Compare December 12, 2018 07:57
@neo291
Copy link
Contributor Author

neo291 commented Dec 12, 2018

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

@codebytere
Copy link
Member

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

@neo291
Copy link
Contributor Author

neo291 commented Dec 12, 2018

@codebytere ok, thanks :)

@codebytere codebytere merged commit 649633b into electron:master Dec 13, 2018
@release-clerk
Copy link

release-clerk bot commented Dec 13, 2018

Release Notes Persisted

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

@neo291 neo291 deleted the fix-set-parent-window-for-windows branch December 13, 2018 11:12
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.

None yet

4 participants