RFR: 8307934 JRobot.moveMouseTo must access component on EDT [v2]

[Alexey Ivanov]

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