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