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

[Sergey Bylokhov]

Hi, Dmitriy.
Do I understand correctly that these methods will be added to the Robot 
class in jdk9?
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.

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
>>>
>


-- 
Best regards, Sergey.