<AWT Dev> [9] Review request for 8046495: KeyEvent can not be accepted in quick mouse clicking
anton nashatyrev
anton.nashatyrev at oracle.com
Fri Jul 18 10:28:02 UTC 2014
Hello,
in offline discussion with Artem and Petr we decided to further
clean up the code and completely remove TimeHelper functions from
awt_Component.cpp in JDK9.
Could you please review the updated fix:
http://cr.openjdk.java.net/~anashaty/8046495/9/webrev.01/
<http://cr.openjdk.java.net/%7Eanashaty/8046495/9/webrev.01/>
bug: https://bugs.openjdk.java.net/browse/JDK-8046495
Thank you!
Anton.
On 17.07.2014 13:14, anton nashatyrev wrote:
> Hello All,
>
> could please anyone else take a look at the fix?
>
> Thanks!
> Anton.
>
> On 02.07.2014 19:37, Petr Pchelko wrote:
>> Hello, Anton.
>>
>> Thanks for clarifications and additional testing.
>> The fix looks good to me too.
>>
>> With best regards. Petr.
>>
>> On 02 июля 2014 г., at 19:34, Anton V. Tarasov
>> <anton.tarasov at oracle.com <mailto:anton.tarasov at oracle.com>> wrote:
>>
>>> On 02.07.2014 19:28, anton nashatyrev wrote:
>>>> Hello, Anton
>>>>
>>>> On 02.07.2014 18:13, Anton V. Tarasov wrote:
>>>>> On 02.07.2014 11:44, Petr Pchelko wrote:
>>>>>> Hello, Anton.
>>>>>>
>>>>>> I'm not sure I have a detailed understanding of what's happening.
>>>>>>
>>>>>> Before your fix the timestamp of the event represented the time
>>>>>> when the event was created, and now it's the time when it's sent
>>>>>> to java.
>>>>>> This might be important if some events cause other events to be
>>>>>> issued on the java side.
>>>>>>
>>>>>> So suppose the following:
>>>>>> Toolkit thread: InputEvent#1 created - timestamp 0
>>>>>> Toolkit thread: InputEvent#2 created - timestamp 1
>>>>>> Toolkit thread: InputEvent#1 sent - timestamp 2
>>>>>> EDT: InputEvent#1 dispatched - timestamp 3
>>>>>> EDT: FocusEvent created - timestamp 4
>>>>>> Toolkit thread: InputEvent#2 sent - timestamp 5
>>>>>>
>>>>>> Before you fix we had the following event order:
>>>>>> InputEvent#1(ts=0), InputEvent#2(ts=1), FocusEvent(ts=4).
>>>>>> But after your fix we will have a different order:
>>>>>> InputEvent#1(ts=2), FocusEvent(ts=4), InputEvent#2(ts=5).
>>>>>> So now we would have troubles, because the Input Event will go to
>>>>>> the new focused component instead of the old one.
>>>>>> Do you think that my arguments are correct? I understand that the
>>>>>> likelihood of such a situation is very low, but for me it looks
>>>>>> possible? Am I missing something?
>>>>>
>>>>> A timestamp for a FocusEvent is taken from
>>>>> the_most_recent_KeyEvent_time which is set on EDT when the event
>>>>> is dispatched. So the fix shouldn't affect it.
>>>>>
>>>>> Also, in awt_Window.cpp, when a TimedWindowEvent is created it is
>>>>> passed a timestamp got with TimeHelper::getMessageTimeUTC(). It
>>>>> appears, that the fix makes key events times even more consistent
>>>>> with it. So, from the kbd focus perspective, it shouldn't make any
>>>>> harm.
>>>>>
>>>>> Anton, could you just please run the following tests, at least:
>>>>>
>>>>> test/java/awt/Focus/6981400/*
>>>>> test/java/awt/KeyboardFocusManager/TypeAhead/*
>>>>
>>>> I've tested with the following set:
>>>> [closed]/java/awt/event/*
>>>> [closed]/java/awt/Focus/*
>>>> [closed]java/awt/KeyboardFocusManager/*
>>>>
>>>> : no unexpected failures here.
>>>>
>>>> I've also verified that my regression test which comes with the fix
>>>> works fine on Linux and Mac (despite the fix is Win specific).
>>>
>>> Great, thanks!
>>>
>>> Anton.
>>>
>>>>
>>>> Thanks for review!
>>>> Anton.
>>>>
>>>>>
>>>>> Thanks,
>>>>> Anton.
>>>>>
>>>>>>
>>>>>> Another thing I do not understand is why we were used to use the
>>>>>> complicated formula instead of initializing the msg.time field
>>>>>> with the JVM current time and using it when sending the event?
>>>>>> Wouldn't that resolve both your issue and the issue the original
>>>>>> fix was made for?
>>>>>>
>>>>>> I have a couple of comments about the code, but let's postpone
>>>>>> that until we decide on the approach.
>>>>>>
>>>>>> Thank you.
>>>>>> With best regards. Petr.
>>>>>>
>>>>>> On 01 июля 2014 г., at 21:20, anton nashatyrev
>>>>>> <anton.nashatyrev at oracle.com
>>>>>> <mailto:anton.nashatyrev at oracle.com>> wrote:
>>>>>>
>>>>>>> Hello,
>>>>>>> could you please review the following fix:
>>>>>>>
>>>>>>> fix: http://cr.openjdk.java.net/~anashaty/8046495/9/webrev.00/
>>>>>>> <http://cr.openjdk.java.net/%7Eanashaty/8046495/9/webrev.00/>
>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8046495
>>>>>>>
>>>>>>> *Problem:*
>>>>>>> On Windows the later InputEvent may have earlier timestamp
>>>>>>> (getWhen()), which results in incorrect processing of enqueued
>>>>>>> input events and may also potentially be the reason of other
>>>>>>> artifacts
>>>>>>>
>>>>>>> *Evaluation*:
>>>>>>> On Windows we have some algorithm for calculating input event
>>>>>>> timestamp: jdk/src/windows/native/sun/windows/awt_Component.cpp:2100
>>>>>>> Shortly the timestamp is calculated by the following formula:
>>>>>>> when = JVM_CurrentTimeMillis() - (GetTickCount() -
>>>>>>> GetMessageTime())
>>>>>>>
>>>>>>> Where:
>>>>>>> JVM_CurrentTimeMillis() - the same as System.currentTimeMillis()
>>>>>>> GetTickCount() - Win32 function, current millis from boot time
>>>>>>> GetMessageTime() - Win32 function, millis from boot time of
>>>>>>> the latest native Message
>>>>>>>
>>>>>>> In theory the formula looks good: we are trying our best to
>>>>>>> calculate the actual time of the OS message by taking the
>>>>>>> current JVM time (JVM_CurrentTimeMillis) and adjusting it with
>>>>>>> the offset (GetTickCount - GetMessageTime) which should indicate
>>>>>>> how many milliseconds ago from the current moment (GetTickCount)
>>>>>>> the message has been actually issued (GetMessageTime).
>>>>>>> In practice due to usage of different system timers by the
>>>>>>> JVM_CurrentTimeMillis and GetTickCount their values are not
>>>>>>> updated synchronously and we may get an earlier timestamp for
>>>>>>> the later event.
>>>>>>>
>>>>>>> *Fix*:
>>>>>>> Just to use JVM_CurrentTimeMillis only as events timestamp. On
>>>>>>> Mac this is done in exactly the same way:
>>>>>>> CPlatformResponder.handleMouse/KeyEvent()
>>>>>>>
>>>>>>> The message time offset calculation has been introduced with the
>>>>>>> fix for JDK-4434193
>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-4434193> and it seems
>>>>>>> that the issue has addressed very similar problem (At times
>>>>>>> getWhen()in ActionEvent returns higher value than the
>>>>>>> CurrentSystemTime) which has not been completely resolved in fact.
>>>>>>> I also didn't find any benefits in using the existing approach:
>>>>>>> - all the usages of the TimerHelper are in fact reduced to the
>>>>>>> mentioned formula: when = JVM_CurrentTimeMillis() -
>>>>>>> (GetTickCount() - GetMessageTime())
>>>>>>> - GetMessageTime() always increases (except of the int overflow
>>>>>>> moments), thus we couldn't get earlier OS messages after later ones
>>>>>>> - TimerHelper::windowsToUTC(DWORD windowsTime) doesn't guarantee
>>>>>>> returning the same timestamp across different calls for the same
>>>>>>> message time
>>>>>>>
>>>>>>> Thanks!
>>>>>>> Anton.
>>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/awt-dev/attachments/20140718/99a97626/attachment.html>
More information about the awt-dev
mailing list