On Fri, 1 Mar 2024 16:05:07 GMT, Abhishek Kumar <abhis...@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
