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

[Anton V. Tarasov]

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/20140702/fb1abc78/attachment.html>