Hi, Oleg.
The fix looks good.
On 15.11.2013 19:35, Oleg Pekhovskiy wrote:
Hi Anthony,
thank you for the review,
the second version of fix is here:
http://cr.openjdk.java.net/~bagiras/8027628.2/
Brief:
1. XBaseWindow changes were reverted.
2. Shared code moved from XDecoratedPeer to XWindowPeer
3. XWindowPeer.handleConfigureNotifyEvent() method improved.
please see my comments below...
Thanks,
Oleg
On 11/08/2013 06:18 PM, Anthony Petrov wrote:
One more point is about the performance. It could be degraded
significantly if we call toGlobal() all the time unconditionally.
So I suggest to test this using SwingMark to ensure the effect is not
very severe.
Does it make sense to perform such testing if I made functionality
changes in XWindowPeer.java only?
--
best regards,
Anthony
On 11/08/2013 06:11 PM, Anthony Petrov wrote:
Hi Oleg,
The XBaseWindow class is not the best place for this code. Firstly, the
XDecoratedPeer.handleConfigureNotifyEvent() never calls its super.*
counterpart. Secondly, all top-level windows use XWindowPeer as an
instance for its peer, so this code actually belongs there (e.g. you
can
update x, y after calling the super method.)
I analyzed the code in XDecoratedPeer.handleConfigureNotifyEvent()
method and found the part that could be shared with
XWindowPeer. I left XDecoratedPeer.handleConfigureNotifyEvent()
functioanlity intact (just moved the code).
I reverted my changes in XBaseWindow.handleConfigureNotifyEvent()
method and implemented everything in XWindowPeer.
However, I'd still suggest to analyze why this isn't a problem for
decorated peers in the first place, and consider using the same
mechanism for undecorated peers as well (perhaps we could share some
code in this area and move some common parts to XWindowPeer, for
example).
It's not a problem for decorated windows because location is corrected
there too using the way similar to the proposed way
in the first version of my fix. So the code was shared.
Also, the handleConfigureNotifyEvent code is *VERY* fragile. After we
settle on some more-or-less final version of the fix, you should run
most of Window/Frame/Dialog/Layout regression tests (both automatic and
manual, from open and closed repos) to make sure we don't regress. This
needs to be tested on OEL 6 (or which is the minimum supported version
for JDK 8?) and Solaris boxes as well, because the code in this event
handler covers many rare cases only reproducible with specific window
managers found on old systems.
I ran the following tests just on Ubuntu:
test/java/awt/Dialog
test/java/awt/Frame
test/java/awt/Window
closed/test/java/awt/Dialog
closed/ test/java/awt/Frame
closed/ test/java/awt/Layout
closed/ test/java/awt/Window
There were no regressions with my fix even one more previously failing
test passed.
I also ran SQE functional tests mentioned in JIRA issue description.
Windows stopped jumping.
Thanks,
Oleg
--
best regards,
Anthony
On 11/08/2013 12:00 PM, Oleg Pekhovskiy wrote:
Hi all,
please review the fix
http://cr.openjdk.java.net/~bagiras/8027628.1/
for
https://bugs.openjdk.java.net/browse/JDK-8027628
The issue affects all top-level windows. XConfigureEvent.x and
XConfigureEvent.y fields
from ConfigureNotify event could have wrong values, e.g. when the
window
is resized by not moved.
That's why manual calculation was added.
This is explained in ICCCM, in "4.1.5. Configuring the Window"
section:
"Advice to Implementors
Clients cannot distinguish between the case where a top-level
window is
resized and moved from the case where the window is resized but not
moved, since a real ConfigureNotify event will be received in both
cases. Clients that are concerned with keeping track of the absolute
position of a top-level window should keep a piece of state indicating
whether they are certain of its position. Upon receipt of a real
ConfigureNotify event on the top-level window, the client should note
that the position is unknown. Upon receipt of a synthetic
ConfigureNotify event, the client should note the position as known,
using the position in this event. If the client receives a KeyPress ,
KeyRelease , ButtonPress , ButtonRelease , MotionNotify ,
EnterNotify ,
or LeaveNotify event on the window (or on any descendant), the client
can deduce the top-level window's position from the difference between
the (event-x, event-y) and (root-x, root-y) coordinates in these
events.
Only when the position is unknown does the client need to use the
TranslateCoordinates request to find the position of a top-level
window."
Thanks,
Oleg
--
Best regards, Sergey.
