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?
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 <[email protected]> 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
