Thanks for clarification. Looks fine.
Thanks,
Artem
On 4/29/2013 5:19 PM, Sergey Bylokhov wrote:
On 29.04.2013 16:50, Artem Ananiev wrote:
Looks fine. Please, add more comments to the bug and describe the
difference between OS X and Windows platform wrt how parents,
containers, and owners are treated.
Bug was updated.
What about X11? Since we change semantics of Component.getContainer(),
which is shared code, all the platforms should be checked.
Actually the bug starts from the X11. But I agree with Antony that the
code in X11 is correct, and is necessary to fix methods which it uses.
Thanks,
Artem
On 4/29/2013 4:31 PM, Sergey Bylokhov wrote:
Hi, Anthony, Artem.
Can you take a look to the new version of the fix:
http://cr.openjdk.java.net/~serb/7166296/webrev.02
On 24.04.2013 13:53, Anthony Petrov wrote:
Hi Sergey,
This second version of the fix looks much better. A couple of comments:
src/macosx/classes/sun/lwawt/LWComponentPeer.java
189 final Container parent =
SunToolkit.getNativeContainer(target);
I still suggest to avoid using the word "parent" wherever possible.
And in this particular case we know exactly that we're requesting a
container here. Please rename this variable.
src/windows/classes/sun/awt/windows/WComponentPeer.java
773 WComponentPeer getNativeParent(){
I suggest to add a comment above this method stating that we use the
term "parent" explicitly because we override the method in top-level
window peer implementations.
Otherwise the fix looks fine to me. Thank you.
--
best regards,
Anthony
On 04/23/2013 08:51 PM, Sergey Bylokhov wrote:
On 23.04.2013 17:23, Anthony Petrov wrote:
Indeed, it does. But in e.g. AwtButton::Create() it uses the same
thing as a container. This code is ugly, and the
Toolkit/Component.getNativeContainer() is named incorrectly.
I see that this erroneous pattern comes from the Windows
implementation, but it doesn't make any sense on any of other
platforms because they do make a clear distinction between owners and
containers and use completely different APIs/concepts to set them. So
I'd try and get rid of the pattern. I think we should investigate all
use cases (~15 in JDK) and see whether we can rework them to
something
meaningful.
I believe that in most cases a get*Container() method must really
return a container belonging to the same top-level window only.
And in
cases where we do want to get an owner of a window, we must use the
Window.getParent() explicitly. I think the latter is only really
needed when we create a native window, actually.
But somewhere it is necessary to do check of the type.
- Like in the first version of the fix:
XComponentPeer/XWindowPeer.getContainerPeer()
- Or something like this:
http://cr.openjdk.java.net/~serb/7166296/webrev.01/
WComponentPeer/WWindowPeer.getNativeParent
- Or I can rename this method from getNativeContainer to
getNativeParent, and add new getNativeContainer method with correct
implementation.
Note that in xawt/lwawt getNativeContainer is used only once.
