RFR: 8326458: Menu mnemonic doesn't toggle between show and hide in Windows LAF when F10 is pressed. [v4]

[Alexey Ivanov]

On Fri, 1 Mar 2024 11:39:09 GMT, Abhishek Kumar <abhiscxk at openjdk.org> wrote:

>> Menu mnemonic doesn't toggle between show and hide state when F10 is pressed. Behavior is not similar to windows native application. Fix is to ensure that menu mnemonic state toggles between show and hide.
>> 
>> Can be verified with SwingSet2 application. 
>> CI tests are green with the fix. Link posted in JBS.
>
> Abhishek Kumar has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Test case added

Changes requested by aivanov (Reviewer).

src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsMenuBarUI.java line 1:

> 1: /*

Please, update the copyright year.

test/jdk/javax/swing/JMenu/TestMenuMnemonic.java line 1:

> 1: /*

You're modifying `WindowsMenuBarUI`, so the test belongs under `JMenuBar`.

test/jdk/javax/swing/JMenu/TestMenuMnemonic.java line 31:

> 29:  * @modules java.desktop/com.sun.java.swing.plaf.windows
> 30:  * @summary Verifies if menu mnemonics toggle between show or hide
> 31:  *          on F10 press in windows LAF.

“Between” implies “and” (instead of ”or”).

However, we can drop that part completely as “toggle” implies it changes from on to off and so on.
Suggestion:

 * @summary Verifies if menu mnemonics toggle on F10 press in Windows LAF

test/jdk/javax/swing/JMenu/TestMenuMnemonic.java line 66:

> 64:             SwingUtilities.invokeAndWait(() -> {
> 65:                 createAndShowUI();
> 66:             });

I'm for using method references:
Suggestion:

            SwingUtilities.invokeAndWait(TestMenuMnemonic::createAndShowUI);

test/jdk/javax/swing/JMenu/TestMenuMnemonic.java line 75:

> 73:                 robot.waitForIdle();
> 74:                 robot.delay(50);
> 75:                 robot.keyRelease(KeyEvent.VK_F10);

You should add `robot.waitForIdle()` to allow the test to process the key release event.

test/jdk/javax/swing/JMenu/TestMenuMnemonic.java line 84:

> 82:                     if (selectedPath.length != 0) {
> 83:                         throw new RuntimeException("Menubar is active even after" +
> 84:                                 " mnemonics are hidden");

Suggestion:

                        throw new RuntimeException("Menubar is active after" +
                                " mnemonics are hidden");


Is the line too long without wrapping?

test/jdk/javax/swing/JMenu/TestMenuMnemonic.java line 89:

> 87:                     mnemonicShowCount++;
> 88:                     if (selectedPath.length == 0 &&
> 89:                         (selectedPath[0] != menuBar || selectedPath[1] != fileMenu)) {

Suggestion:

                    if (selectedPath.length != 2
                        || selectedPath[0] != menuBar
                        || selectedPath[1] != fileMenu) {

You expect the selected path length to be 2 — if it's not, it's a failure.

test/jdk/javax/swing/JMenu/TestMenuMnemonic.java line 94:

> 92:                     }
> 93:                 }
> 94:             }

Swing is not thread-safe therefore `MenuSelectionManager` as well as `WindowsLookAndFeel.isMnemonicHidden()` are to be called on EDT.

Because of that, you should use `AtomicInteger` and its `getAndIncrement()` method to increase the counter; then use `get()` to get the accumulated value on the main thread to verify.

test/jdk/javax/swing/JMenu/TestMenuMnemonic.java line 96:

> 94:             }
> 95:             robot.waitForIdle();
> 96:             robot.delay(1000);

There's no need for any delays after the for-loop.

test/jdk/javax/swing/JMenu/TestMenuMnemonic.java line 120:

> 118:         editMenu.setMnemonic(KeyEvent.VK_E);
> 119:         item1 = new JMenuItem("Item 1");
> 120:         item2 = new JMenuItem("Item 2");

`editMenu` as well as `item1` and `item2` aren't used outside of this method — convert them to local variables.

test/jdk/javax/swing/JMenu/TestMenuMnemonic.java line 127:

> 125:         frame.setJMenuBar(menuBar);
> 126:         frame.pack();
> 127:         frame.setSize(250, 200);

Suggestion:

        frame.setSize(250, 200);

You set the size, `pack` is redundant.

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

PR Review: https://git.openjdk.org/jdk/pull/17961#pullrequestreview-1911373693
PR Review Comment: https://git.openjdk.org/jdk/pull/17961#discussion_r1509147894
PR Review Comment: https://git.openjdk.org/jdk/pull/17961#discussion_r1509147704
PR Review Comment: https://git.openjdk.org/jdk/pull/17961#discussion_r1509139166
PR Review Comment: https://git.openjdk.org/jdk/pull/17961#discussion_r1509140683
PR Review Comment: https://git.openjdk.org/jdk/pull/17961#discussion_r1509132518
PR Review Comment: https://git.openjdk.org/jdk/pull/17961#discussion_r1509134125
PR Review Comment: https://git.openjdk.org/jdk/pull/17961#discussion_r1509136261
PR Review Comment: https://git.openjdk.org/jdk/pull/17961#discussion_r1509131418
PR Review Comment: https://git.openjdk.org/jdk/pull/17961#discussion_r1509133750
PR Review Comment: https://git.openjdk.org/jdk/pull/17961#discussion_r1509143946
PR Review Comment: https://git.openjdk.org/jdk/pull/17961#discussion_r1509142739