RFR: 8326458: Menu mnemonics don't toggle in Windows LAF when F10 is pressed [v5]

[Alexey Ivanov]

On Fri, 1 Mar 2024 16:05:07 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:
> 
>   Review comment update

Changes requested by aivanov (Reviewer).

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

> 140:     @SuppressWarnings("serial") // Superclass is not serializable across versions
> 141:     private static class TakeFocus extends AbstractAction {
> 142:         static boolean mnemonicShowHideFlag = false;

Not used any more.

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

> 141:     private static class TakeFocus extends AbstractAction {
> 142:         static boolean mnemonicShowHideFlag = false;
> 143:         public void actionPerformed(ActionEvent e) {

I suggest adding `@Override` annotation.

Most methods in the class have the annotation.

test/jdk/javax/swing/JMenuBar/TestMenuMnemonic.java line 36:

> 34: import java.awt.event.KeyEvent;
> 35: import java.awt.Robot;
> 36: import java.util.concurrent.atomic.AtomicInteger;

Sort imports:
Suggestion:

import java.awt.Robot;
import java.awt.event.KeyEvent;
import java.util.concurrent.atomic.AtomicInteger;

test/jdk/javax/swing/JMenuBar/TestMenuMnemonic.java line 40:

> 38: import javax.swing.JMenu;
> 39: import javax.swing.JMenuItem;
> 40: import javax.swing.JMenuBar;

Sort imports:
Suggestion:

import javax.swing.JMenuBar;
import javax.swing.JMenuItem;

test/jdk/javax/swing/JMenuBar/TestMenuMnemonic.java line 45:

> 43: import javax.swing.SwingUtilities;
> 44: import javax.swing.UIManager;
> 45: import com.sun.java.swing.plaf.windows.WindowsLookAndFeel;

Suggestion:

import javax.swing.UIManager;

import com.sun.java.swing.plaf.windows.WindowsLookAndFeel;


I suggest adding a blank line to separate the standard imported packages from non-standard ones, in this case it's even an implementation-private package.

test/jdk/javax/swing/JMenuBar/TestMenuMnemonic.java line 55:

> 53:     public static void main(String[] args) throws Exception {
> 54:         UIManager.setLookAndFeel("com.sun.java.swing.plaf.windows.WindowsLookAndFeel");
> 55:         int expectedMnemonicShowHideCount = 5;

Make it a constant, for example `EXPECTED`.

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

> 64:             robot.delay(1000);
> 65: 
> 66:             for (int i = 0; i < 10; i++) {

Suggestion:

            for (int i = 0; i < EXPECTED * 2; i++) {

No magic numbers, after all the number of `EXPECTED` show/hide depends on how many iterations the loop does.

test/jdk/javax/swing/JMenuBar/TestMenuMnemonic.java line 87:

> 85:                         mnemonicShowCount.getAndIncrement();
> 86:                         if (selectedPath.length != 2 &&
> 87:                                 (selectedPath[0] != menuBar || selectedPath[1] != fileMenu)) {

Suggestion:

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

In all the other places you wrap before the operator; in fact it's what Java Coding Style recommends doing. And this style makes it clear that it's a continuation line, if it starts with an operator.

test/jdk/javax/swing/JMenuBar/TestMenuMnemonic.java line 92:

> 90:                         }
> 91:                     }
> 92:                 });

May I suggest creating a method for verifying the state of mnemonics? Then the `main` will be shorter:


                SwingUtilities.invokeAndWait(TestMenuMnemonic::verifyMnemonicsState);

You'll have to make `mnemonicHideCount` and `mnemonicShowCount` static fields (and `final` please).

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

PR Review: https://git.openjdk.org/jdk/pull/17961#pullrequestreview-1914242401
PR Review Comment: https://git.openjdk.org/jdk/pull/17961#discussion_r1511119367
PR Review Comment: https://git.openjdk.org/jdk/pull/17961#discussion_r1511127629
PR Review Comment: https://git.openjdk.org/jdk/pull/17961#discussion_r1511130722
PR Review Comment: https://git.openjdk.org/jdk/pull/17961#discussion_r1511131445
PR Review Comment: https://git.openjdk.org/jdk/pull/17961#discussion_r1511134440
PR Review Comment: https://git.openjdk.org/jdk/pull/17961#discussion_r1511145696
PR Review Comment: https://git.openjdk.org/jdk/pull/17961#discussion_r1511148255
PR Review Comment: https://git.openjdk.org/jdk/pull/17961#discussion_r1511154492
PR Review Comment: https://git.openjdk.org/jdk/pull/17961#discussion_r1511152467