Hi Artem,
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
check_if_event_predicate().
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
'isPacked'.
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
the future.
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
XBaseWindow?
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
XrootWindow.getRootWindow(). :)
Will take a look at XDecoratedPeer and XWM shortly.
Thanks! Looking forward to seeing the comments.
--
best regards,
Anthony
Thanks,
Artem
Anthony Petrov wrote:
Hello.
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
4.1.3/3.0
solaris-sparc: Gnome/Metacity 2.8, CDE/DTWM Solaris 10
Please review the code at:
http://cr.openjdk.java.net/~anthony/7-16-insets-6689983.0/
Suggestions are welcome. Thanks!
--
best regards,
Anthony
