<AWT Dev> [8] Review request for 8027628: JWindow jumps to (0, 0) after mouse clicked

Oleg Pekhovskiy oleg.pekhovskiy at oracle.com
Fri Nov 15 07:35:26 PST 2013


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



More information about the awt-dev mailing list