On Fri, 18 Nov 2022 19:21:52 GMT, Naveen Narayanan <[email protected]> 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
