RFR: 8307934 JRobot.moveMouseTo must access component on EDT [v2]
Alexey Ivanov
aivanov at openjdk.org
Mon Jun 26 14:28:11 UTC 2023
On Sat, 24 Jun 2023 03:20:45 GMT, Renjith Kannath Pariyangad <duke at openjdk.org> wrote:
>> Hi Reviewers,
>>
>> I have updated the JRobot.java file for enabling EDT support, along with I have removed warning BUTTON1_MASK with 'BUTTON1_DOWN_MASK'.
>>
>> Regards,
>> Renjith
>
> Renjith Kannath Pariyangad has updated the pull request incrementally with one additional commit since the last revision:
>
> Updated documentation of modified functions
Looks good to me except for the few comments I've left.
Did you run the tests?
test/jdk/javax/swing/regtesthelpers/JRobot.java line 42:
> 40: import javax.swing.SwingUtilities;
> 41: import java.util.concurrent.atomic.AtomicReference;
> 42: import java.lang.reflect.InvocationTargetException;
Could you sort the imports, please? You IDE should be able to handle it for you.
test/jdk/javax/swing/regtesthelpers/JRobot.java line 44:
> 42: import java.lang.reflect.InvocationTargetException;
> 43:
> 44: public class JRobot extends java.awt.Robot {
It would be good if you moved the javadoc for the `JRobot` class to where it belongs: to the class declaration. Now it precedes the import block.
test/jdk/javax/swing/regtesthelpers/JRobot.java line 45:
> 43:
> 44: public class JRobot extends java.awt.Robot {
> 45: private static int DEFAULT_DELAY = 550;
Can you add the `final` modifier to both `DEFAULT_DELAY` and `INTERNAL_DELAY`.
test/jdk/javax/swing/regtesthelpers/JRobot.java line 116:
> 114: * Move mouse cursor to the center of the Component.
> 115: * <p>
> 116: * <b>Note:</b> This method is executed on EDT
Suggestion:
* <b>Note:</b> This method is executed on EDT.
test/jdk/javax/swing/regtesthelpers/JRobot.java line 178:
> 176:
> 177: /**
> 178: * Click in the center of the given Component
Suggestion:
* Click in the center of the given Component.
Let's fix the javadoc even further and add the full stop.
test/jdk/javax/swing/regtesthelpers/JRobot.java line 180:
> 178: * Click in the center of the given Component
> 179: * <p>
> 180: * <b>Note:</b> This method uses EDT
Suggestion:
* <b>Note:</b> This method is executed on EDT.
Let's be consistent and use the same phrase.
test/jdk/javax/swing/regtesthelpers/JRobot.java line 183:
> 181: *
> 182: * @param c the Component to click on
> 183: * @param buttons mouse button(s).
Suggestion:
* @param buttons mouse button(s)
In description of parameters, there are usually no full stops.
test/jdk/javax/swing/regtesthelpers/JRobot.java line 326:
> 324: return (getPixelColor(x0, y0).equals(getPixelColor(x1, y1)));
> 325: }
> 326: }
Could you add a blank line to the end of the file, please?
-------------
Changes requested by aivanov (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/14354#pullrequestreview-1498648975
PR Review Comment: https://git.openjdk.org/jdk/pull/14354#discussion_r1242269274
PR Review Comment: https://git.openjdk.org/jdk/pull/14354#discussion_r1242285001
PR Review Comment: https://git.openjdk.org/jdk/pull/14354#discussion_r1242270965
PR Review Comment: https://git.openjdk.org/jdk/pull/14354#discussion_r1242273273
PR Review Comment: https://git.openjdk.org/jdk/pull/14354#discussion_r1242276455
PR Review Comment: https://git.openjdk.org/jdk/pull/14354#discussion_r1242274807
PR Review Comment: https://git.openjdk.org/jdk/pull/14354#discussion_r1242277111
PR Review Comment: https://git.openjdk.org/jdk/pull/14354#discussion_r1242281272
More information about the client-libs-dev
mailing list