<AWT Dev> Review request: 6689983 (reevaluate our inset-related code in XAWT)
Anthony.Petrov at Sun.COM
Tue Jul 7 07:44:26 PDT 2009
On 06/30/2009 07:03 PM, Artem Ananiev wrote:
> 1. Should XlibWrapper.CheckIfEvent be XCheckIfEvent?
Which one of them? :) Or both?
Actually the signatures of these methods do not strictly correspond to
the prototype of the Xlib XCehckIfEvent() function. That's why I avoid
adding the prefix letter 'X'.
> 2. XlibWrapper.CheckIfEvent: I might be wrong, but it seems 'event'
> parameter can be eliminated. It's enough to make use of 'event' param in
Please suggest a way to construct an XEvent object out of some unsafe
memory block. If that is possible, I would love to get rid of the tricks
with passing the event and the event.pData. If that isn't possible, I
have to construct the object and then pass it to the native code.
On the other hand we could retrieve the pData value in the native
function Java_sun_awt_X11_XlibWrapper_CheckIfEvent() via JNI, but I
decided to somewhat optimize it and pass both the event and the
event.pData arguments. Given the native method is private in the
XlibWrapper, I don't see a problem with that.
> 3. I'd vote for removing 'isPacked' field from Component class if it's
> possible. I'm not an expert in serialization, though, to help you with
> this :)
As Oleg points out this is impossible.
> 4. Window.reshape(): is there any help from having the tree lock here?
> Access to 'width' and 'height' is not synchronized, the same is true for
You might notice that the Component.reshape() acquires the lock
immediately and then proceeds handling the bounds of the component.
Since Window.reshape() now makes a decision based on the current size of
the window and then calls super.reshape(), I decided to extend the
synchronized block in order to make the code consistent.
The access to the fields is indeed being done asynchronously now.
However the bounds, as well as the isPacked attribute, all represent a
layout-related information, modifications to which (in an ideal world)
should be synchronized with the TreeLock. I strongly believe that this
will become true someday. For now, I just save little bits of work for
> 5. XWindow.getRootWindow(): the code is pretty ugly, although I
> understand it saves another call to Xlib comparing to what we have now
> (XRootWindow + XGetWindowAttributes). Is it worth moving this method to
I don't know. Do we need it? There's also the XRootWindow class - a
direct descendant of the XBaseWindow. It looks a little funny to call
> Will take a look at XDecoratedPeer and XWM shortly.
Thanks! Looking forward to seeing the comments.
> Anthony Petrov wrote:
>> The insets-related code in the XToolkit has been a nightmare to
>> maintain for quite a long time. This fix is a try to rewrite the code
>> making it more understandable and maintainable.
>> Testing: all more-or-less related automatic regression tests have been
>> run. Categories include but not limited to: top-level tests, layouts
>> tests, embedded frame tests, mouse events tests, focus tests,
>> menu/popup menu tests, and some other. Nearly 60 tests were found
>> failing with a clean build, so I filed the corresponding CRs. The rest
>> pass with this fix on:
>> linux-i586: Gnome/Metacity 2.24, Gnome/Compiz 2.24/0.7.8, KDE/KWin
>> solaris-sparc: Gnome/Metacity 2.8, CDE/DTWM Solaris 10
>> Please review the code at:
>> Suggestions are welcome. Thanks!
>> best regards,
More information about the awt-dev