<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