<AWT Dev> [9] Review request for 8046495: KeyEvent can not be accepted in quick mouse clicking

[anton nashatyrev]

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/20140717/0dca3676/attachment-0001.html>