RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v8]

[Alexey Ivanov]

On Fri, 21 Jun 2024 07:55:24 GMT, Abhishek Kumar <abhiscxk at openjdk.org> wrote:

>> In GTK LAF, the menu mnemonics are always displayed which is different from the native behavior. In native application **(tested with gedit for normal buttons and tested with libreoffice for menu**), the menu mnemonics toggle on press of `ALT` key. Menu mnemonics are hidden initially and then toggles between show/hide on `ALT` press. 
>> Proposed fix is to handle the `ALT` key press for GTK LAF and mimic the native behavior. Fix is similar to the `ALT` key processing in  Windows LAF. Automated test case is added to verify the fix and tested in Ubuntu and Oracle linux.
>> 
>> CI testing is green and link attached in JBS.
>
> Abhishek Kumar has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Remove access modifier from method declaration

Changes requested by aivanov (Reviewer).

src/java.desktop/share/classes/com/sun/java/swing/plaf/gtk/GTKLookAndFeel.java line 868:

> 866:                  "ctrl released ENTER", "release"
> 867:             },
> 868:             "RootPane.altPress", true,

`RootPane.hideMnemonics` or `RootPane.showMnemonicsOnAltPress` could be a more descriptive property name.

src/java.desktop/share/classes/javax/swing/plaf/synth/SynthGraphicsUtils.java line 676:

> 674:                 int mnemIndex = lh.getMenuItem().getDisplayedMnemonicIndex();
> 675:                 // Check to see if the Mnemonic should be rendered in GTK.
> 676:                 if (UIManager.getBoolean("RootPane.altPress")

You may add another condition to the `if` statement, `mnemIndex < 0` — if there's no mnemonic defined, there's no need to query the properties from `UIManager` at all.

Is the new property even needed? Is `Button.showMnemonics` not enough? This property controls mnemonics on all the components, including menu bar, in Windows and Aqua. Can't Synth and GTK follow the same pattern?

src/java.desktop/share/classes/javax/swing/plaf/synth/SynthGraphicsUtils.java line 679:

> 677:                     && SynthLookAndFeel.isMnemonicHidden()) {
> 678:                     mnemIndex = -1;
> 679:                 }

Perhaps, verifying the value of the `RootPane.altPress` property should also be moved into `SynthLookAndFeel.isMnemonicHidden()` which already queries the value of `Button.showMnemonics`. Keeping all the properties that affect hiding or showing the mnemonics in one place is easier to reason about if anything doesn't work as expected.

src/java.desktop/share/classes/javax/swing/plaf/synth/SynthLookAndFeel.java line 1046:

> 1044: 
> 1045:     // Toggle flag for drawing the mnemonic state
> 1046:     private static boolean isMnemonicHidden = true;

I wonder why it defaults to `true` where only one L&F derived from Synth sets it to true.

test/jdk/com/sun/java/swing/plaf/gtk/TestMenuMnemonicOnAltPress.java line 72:

> 70:                 pt = fileMenu.getLocationOnScreen();
> 71:                 fileMenuWidth = fileMenu.getWidth();
> 72:                 fileMenuHeight = fileMenu.getHeight();

If you save width and height in a `Dimension` object, `fileMenuSize`, you'll be able to use `new Rectangle(pt, fileMenuSize)` for capturing the screen.

Since you need that rectangle only, you can create the rectangle here:

Suggestion:

                fileMenuRect = new Rectangle(fileMenu.getLocationOnScreen(),
                                             fileMenu.getSize());

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

PR Review: https://git.openjdk.org/jdk/pull/18992#pullrequestreview-2139047741
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1653058424
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1653054277
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1653051513
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1653063685
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1653089074