RFR: 8296275: Write a test to verify setAccelerator method of JMenuItem [v5]

[Alexey Ivanov]

On Fri, 18 Nov 2022 19:21:52 GMT, Naveen Narayanan <duke at openjdk.org> wrote:

>> This testcase will
>> 1) Verify setAccelerator method of JMenuitem.
>> 2) Check that the selection of a menu item in the menu bar will generate action by a key combination of META+M.
>> 
>> Testing:
>> Tested using Mach5(20 times per platform) in Mac OS, Linux and Windows and got all pass.
>
> Naveen Narayanan has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8296275: Review comments fixed.

Since the test has nothing to do with `Desktop` class it should be moved to `JMenuItem` folder because it tests functionality of `JMenuItem`.

test/jdk/java/awt/Desktop/JMenuItemSetAcceleratorTest.java line 24:

> 22: 
> 23: import java.awt.Desktop;
> 24: import java.awt.Desktop.Action;

The unused imports are to be removed.

test/jdk/java/awt/Desktop/JMenuItemSetAcceleratorTest.java line 51:

> 49:     private static JFrame frame;
> 50:     private final static CountDownLatch actionPerformLatch =
> 51:         new CountDownLatch(1);

Suggestion:

    private static final CountDownLatch actionPerformLatch =
        new CountDownLatch(1);

Please used “blessed” modifier order.

I see no reason to wrap the line, it's 84 chars long, which is acceptable to me. If you like, you can rename it to `actionLatch` so that it fits into 80 column limit.

test/jdk/java/awt/Desktop/JMenuItemSetAcceleratorTest.java line 63:

> 61: 
> 62:         menuItem.setAccelerator(
> 63:             KeyStroke.getKeyStroke(KeyEvent.VK_M, ActionEvent.META_MASK));

Suggestion:

        menuItem.setAccelerator(
            KeyStroke.getKeyStroke(KeyEvent.VK_M, InputEvent.META_DOWN_MASK));


The modifiers come from `InputEvent`, not `ActionEvent`.

test/jdk/java/awt/Desktop/JMenuItemSetAcceleratorTest.java line 78:

> 76:     }
> 77: 
> 78:     public static void main(String args[]) throws Exception {

Suggestion:

    public static void main(String[] args) throws Exception {


Please use Java-style array declaration.

test/jdk/java/awt/Desktop/JMenuItemSetAcceleratorTest.java line 81:

> 79:         try {
> 80:             SwingUtilities
> 81:                 .invokeAndWait(JMenuItemSetAcceleratorTest::createAndShow);

I suggest not to wrap the line.

No, it doesn't fit 80 column, yet wrapping creates more visual noise.

If you like, you can statically import `invokeAndWait`. I'm everyone would understand where it comes from without `SwingUtilities.` prefix. However, keeping the class name makes it clear.

test/jdk/java/awt/Desktop/JMenuItemSetAcceleratorTest.java line 98:

> 96:             }
> 97:             System.out
> 98:                 .println("Test passed, received action event on menu item.");

I'd rather wrap the string argument than the call. In this case, I wouldn't wrap it: the line takes 84 columns.

test/jdk/java/awt/Desktop/JMenuItemSetAcceleratorTest.java line 100:

> 98:                 .println("Test passed, received action event on menu item.");
> 99:         } finally {
> 100:             EventQueue.invokeAndWait(JMenuItemSetAcceleratorTest::disposeFrame);

Why not `SwingUtilities`?

-------------

Changes requested by aivanov (Reviewer).

PR: https://git.openjdk.org/jdk/pull/11035