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

[Sergey Bylokhov]

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.