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

[Anthony Petrov]

Hi Oleg,

Thank you for the additional investigation. The updated fix looks fine 
to me.

To answer your question:
> Does it make sense to perform such testing if I made functionality changes in XWindowPeer.java only?

Well, the fix could still hurt the performance for heavyweight popup 
menus. However, there's no other ways to fix the bug. Provided that 
currently the updated fix only affects top-level windows and doesn't 
affect regular components, I think we're good to go.

--
best regards,
Anthony

On 11/15/2013 07:35 PM, 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
>