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
fix: make 'setParentWindow' compatible under Windows #15775
Conversation
There was a problem hiding this 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*Style
s 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
There was a problem hiding this 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 😀
bcf7d8c
to
16a7b13
Compare
About the test: About the constants: |
16a7b13
to
42a9b57
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, LGTM then! 👍
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. |
@neo291 sure, you could add [WIP] to the beginning of the title of the PR temporarily to make sure it's not merged |
@brenca that's perfect thank you! |
42a9b57
to
3ba7489
Compare
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 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. |
15936ad
to
3710d14
Compare
3710d14
to
b73a163
Compare
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. |
There was a problem hiding this 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
b73a163
to
195d9b2
Compare
@nornagon is possible to merge this pull request or there is something blocking? |
@neo291 i'm going to rekick CI and then i'll get this merged :) |
@codebytere ok, thanks :) |
Release Notes Persisted
|
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
npm test
passesRelease Notes
Notes: 'win.setParentWindow(parent)' available also under Windows