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

Sergey Bylokhov Sergey.Bylokhov at oracle.com
Wed Apr 2 15:07:08 UTC 2014


On 4/1/14 1:06 PM, Dmitriy Ermashov wrote:
> 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.
Then why you did not apply these changes to the Robot class itself?
>> 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/
  - javadoc formatting still different.
  - you cannot use non public realSync method name in the public 
documentation.
  - Probably it will make sense to set syncDelay via system property and 
get rid setters? In this case delay can be controlled outside of the 
test itself.
  - It would be good to make some research in internet, what methods can 
be implemented as well.(like drag, etc).
>>
>> 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.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/awt-dev/attachments/20140402/7a7970d9/attachment.html>


More information about the awt-dev mailing list