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

[Petr Pchelko]

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