<AWT Dev> Review request for JDK-8038631: Create wrapper for awt.Robot with additional functionality

[Dmitriy Ermashov]

Hi, Sergey

On 31.03.2014 17:49, Sergey Bylokhov wrote:
> Hi, Dmitriy.
> Do I understand correctly that these methods will be added to the 
> Robot class in jdk9?
Yes, it is a current plan.
> A few notes:
>  - Different javadoc formatting for different methods.
>  - There is no information about exceptions. At least waitForIdle will 
> throw an exception if will be called on edt.
2All: please, review the latest version of changeset:
http://cr.openjdk.java.net/~yan/8038631/webrev.02/
>
> On 31.03.2014 15:51, Dmitriy Ermashov wrote:
>> Hi,
>>
>> Please, review the latest version of changeset for:
>> https://bugs.openjdk.java.net/browse/JDK-8038631
>>
>> Webrev is here:
>> http://cr.openjdk.java.net/~yan/8038631/webrev.01/
>>
>>
>> On 31.03.2014 14:15, Petr Pchelko wrote:
>>>>> 4. The default value for realSync additional delay doesn't seem 
>>>>> enough. We normally use 500 ms as the frame creation and first 
>>>>> draw could take more time.
>>>> We have to use small delay between mousePress and mouseRelease 
>>>> events to emulate real mouse click event.  And larger delay can be 
>>>> put after mouseRelease call, for ex. 500 ms, as you wrote.
>>>> Don't you mind of this solution?
>>> And couldn't you reuse DEFAULT_SPEED for sleeping in mouseClick? The 
>>> comment states it's used there but it's not..
>>>
>>>>> 5. The click method could be more consistent with others if called 
>>>>> "mouseClick". No strong opinion on this.
>>>> For now we have several hundreds of functional tests, most of them 
>>>> use method named click(). It seems more reasonable to create method 
>>>> click() and use it than search the usage of it in all tests and fix 
>>>> to mouseClick() call.
>>> Ok, no problem. I'm fine either way.
>>>
>>> With best regards. Petr.
>>>
>>> On 31.03.2014, at 14:11, Dmitriy Ermashov 
>>> <dmitriy.ermashov at oracle.com> wrote:
>>>
>>>> On 31.03.2014 13:12, Petr Pchelko wrote:
>>>>> Hello, Dmitry.
>>>>>
>>>>> Some comments:
>>>>>
>>>>> 1. The name RobotWrapper does not fit in my opinion. The "Wrapper" 
>>>>> is more useful for delegation not extension and it feels like you 
>>>>> should pass the normal Robot to the wrapper constructor.. I think 
>>>>> something like "ExtendedRobot" would feel better. This is my 
>>>>> personal preference so it's arguable.
>>>> No objections. Will rename it to ExtendedRobot.
>>>>> 2. I'm not sure I understand why is this location of the util 
>>>>> works? in JavaDoc you state that it should be included from 
>>>>> regtesthelpers but you put it to the test lib. Is it some feature 
>>>>> of jtreg I'm not aware of?
>>>> Will fix in next version of webrev.
>>>>> 3. Could you please use @code instead of <code>
>>>> Will fix in next version of webrev.
>>>>> 4. The default value for realSync additional delay doesn't seem 
>>>>> enough. We normally use 500 ms as the frame creation and first 
>>>>> draw could take more time.
>>>> We have to use small delay between mousePress and mouseRelease 
>>>> events to emulate real mouse click event.  And larger delay can be 
>>>> put after mouseRelease call, for ex. 500 ms, as you wrote.
>>>> Don't you mind of this solution?
>>>>> 5. The click method could be more consistent with others if called 
>>>>> "mouseClick". No strong opinion on this.
>>>> For now we have several hundreds of functional tests, most of them 
>>>> use method named click(). It seems more reasonable to create method 
>>>> click() and use it than search the usage of it in all tests and fix 
>>>> to mouseClick() call.
>>>>> 6. "Clicks mouse button 1 with the default pause between Press and 
>>>>> Release." this statement does not seem clear. How to override the 
>>>>> default value? Why does this method click with the default pause 
>>>>> and the previous one does not? or does it? It's not clear from the 
>>>>> JavaDoc
>>>> I believe, javadoc should be fixed.
>>>> Will fix in next version of webrev.
>>>>> 7. Could you please add @Override annotations where appropriate 
>>>>> (waitForIdle for example)
>>>> Will fix in next version of webrev.
>>>>> 8. "public void type" may be keyType would be better? For 
>>>>> consistency with other methods.
>>>> Same as 5.
>>>>> 9. Could the method you are using for converting KeyChar to 
>>>>> KeyCode be replaces with KeyEvent.getExtendedKeyCodeForChar? Or 
>>>>> KeyStroke API? This switch is ugly and it would be awesome to 
>>>>> replace it.
>>>> Will fix in next version of webrev.
>>>>> Thank you.
>>>>> With best regards. Petr.
>>>>>
>>>>> On 31.03.2014, at 12:40, Dmitriy Ermashov 
>>>>> <dmitriy.ermashov at oracle.com> wrote:
>>>>>
>>>>>> Hi,
>>>>>> Please, review the changeset for:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8038631
>>>>>>
>>>>>> Webrev is here:
>>>>>> http://cr.openjdk.java.net/~yan/8038631/webrev.00
>>>>>>
>>>>>> Presently for testing of JDK client we use test suites of two 
>>>>>> kinds, historically called regression and functional (internal). 
>>>>>> In JDK9 we plan an attempt to unify them and ultimately get rid 
>>>>>> of the functional suites.
>>>>>>
>>>>>> One of the first technical problems in this refactoring attempt 
>>>>>> is a multitude of the java.awt.Robot wrappers. There are some 
>>>>>> really elaborate libraries enhansing Robot which all we cannot 
>>>>>> and should not port to jtreg.
>>>>>>
>>>>>> Fortunately, test writers almost never use complex features of 
>>>>>> these wrappers. So here's our plan:
>>>>>> (1) describe the real practice of the robot use in the functional 
>>>>>> tests (please don't worry, that's out of scope of this request);
>>>>>> (2) write a minimal useful RobotWrapper extending java.awt.Robot;
>>>>>> (3) start functional tests refactoring;
>>>>>> (4) cautiously move enhancements to the java.awt.Robot trying not 
>>>>>> to break backward compatibility of thousands existing tests. For 
>>>>>> instance, waitForIdle should use realSync() but there definitely
>>>>>> are plenty of tests using it on EDT: or maybe not -- we should 
>>>>>> spent some time for background research in parallel with (3).
>>>>>>
>>>>>> -- 
>>>>>> Thanks,
>>>>>> Dima
>>>>>>
>>>> -- 
>>>> Thanks,
>>>> Dima
>>>>
>>
>
>

-- 
Thanks,
Dima