RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v11]
Alexey Ivanov
aivanov at openjdk.org
Fri Jun 28 20:10:27 UTC 2024
On Fri, 28 Jun 2024 11:32:49 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 AquaMnemonicHandler class and unused APIs from WinDowsLookAndFeel, copyright year update
Overall, the changes look good to me.
There are several minor comments though.
Now we have one common mnemonic handler instead of two… short of having three of those.
src/java.desktop/macosx/classes/com/apple/laf/AquaButtonUI.java line 79:
> 77: import com.apple.laf.AquaUtils.RecyclableSingletonFromDefaultConstructor;
> 78: import sun.swing.SwingUtilities2;
> 79: import sun.swing.MnemonicHandler;
Suggestion:
import sun.swing.MnemonicHandler;
import sun.swing.SwingUtilities2;
Allow you IDE handle imports for you, we tend to keep the imports sorted alphabetically.
src/java.desktop/macosx/classes/com/apple/laf/AquaButtonUI.java line 493:
> 491: final Graphics2D g2d = g instanceof Graphics2D ? (Graphics2D)g : null;
> 492:
> 493: final AbstractButton b = (AbstractButton)c;
Perhaps, it makes sense to remove `g2d` local variable — it's unused.
src/java.desktop/macosx/classes/com/apple/laf/AquaLabelUI.java line 32:
> 30: import javax.swing.*;
> 31: import javax.swing.plaf.*;
> 32: import javax.swing.plaf.basic.*;
Could you expand all the wildcard imports?
src/java.desktop/macosx/classes/com/apple/laf/AquaLookAndFeel.java line 61:
> 59: import sun.swing.SwingUtilities2;
> 60: import sun.swing.MnemonicHandler;
> 61: import sun.swing.AltProcessor;
Suggestion:
import sun.swing.AltProcessor;
import sun.swing.MnemonicHandler;
import sun.swing.SwingAccessor;
import sun.swing.SwingUtilities2;
src/java.desktop/macosx/classes/com/apple/laf/AquaMenuPainter.java line 34:
> 32: import javax.swing.border.Border;
> 33: import javax.swing.plaf.basic.BasicHTML;
> 34: import javax.swing.text.View;
Could you expand all the wildcard imports?
src/java.desktop/share/classes/com/sun/java/swing/plaf/gtk/GTKLookAndFeel.java line 53:
> 51: import sun.swing.SwingUtilities2;
> 52: import sun.swing.MnemonicHandler;
> 53: import sun.swing.AltProcessor;
Could you expand all the wildcard imports?
Allow your IDE sort the imports; `AltProcessor` should go before `DefaultLayoutStyle` and `MnemonicHandler` should go before `SwingAccessor`.
src/java.desktop/share/classes/sun/swing/AltProcessor.java line 35:
> 33: import javax.swing.SwingUtilities;
> 34:
> 35: import sun.swing.MnemonicHandler;
My IDE highlights `MnemonicHandler` import as redundant because it comes from the same package…
src/java.desktop/share/classes/sun/swing/AltProcessor.java line 47:
> 45: }
> 46:
> 47: public boolean postProcessKeyEvent(final KeyEvent ev) {
I'd add `@Override` annotation to highlight it implements the `KeyEventPostProcessor` interface.
src/java.desktop/share/classes/sun/swing/AltProcessor.java line 62:
> 60: MnemonicHandler.setMnemonicHidden(true);
> 61: break;
> 62: }
Just an idea…
Suggestion:
// Show mnemonics when Alt is pressed and hide them when released
MnemonicHandler.setMnemonicHidden(ev.getID() == KeyEvent.KEY_RELEASED);
The `switch` statement is more explicit, so easier to understand.
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsGraphicsUtils.java line 31:
> 29: import sun.swing.MnemonicHandler;
> 30:
> 31: import java.awt.*;
Could you expand all the wildcard imports?
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsLabelUI.java line 39:
> 37: import sun.awt.AppContext;
> 38: import sun.swing.SwingUtilities2;
> 39: import sun.swing.MnemonicHandler;
Suggestion:
import sun.swing.MnemonicHandler;
import sun.swing.SwingUtilities2;
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsLabelUI.java line 82:
> 80: mnemonicIndex = -1;
> 81: }
> 82: if ( UIManager.getColor("Label.disabledForeground") instanceof Color &&
Perhaps, we could resolve the warning: `UIManager.getColor` returns `Color` object, so what `instanceof Color` really verifies here is whether the returned object is `null` or not.
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsLookAndFeel.java line 101:
> 99: import sun.swing.icon.SortArrowIcon;
> 100: import sun.swing.plaf.windows.ClassicSortArrowIcon;
> 101: import sun.swing.MnemonicHandler;
`MnemonicHandler` should go between `sun.swing.ImageIconUIResource` and `sun.swing.StringUIClientPropertyKey`.
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsMenuItemUI.java line 50:
> 48: import sun.swing.MenuItemLayoutHelper;
> 49: import sun.swing.SwingUtilities2;
> 50: import sun.swing.MnemonicHandler;
Suggestion:
import sun.swing.MenuItemLayoutHelper;
import sun.swing.MnemonicHandler;
import sun.swing.SwingUtilities2;
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsPopupMenuUI.java line 51:
> 49: import com.sun.java.swing.plaf.windows.XPStyle.Skin;
> 50: import sun.swing.StringUIClientPropertyKey;
> 51: import sun.swing.MnemonicHandler;
Suggestion:
import sun.swing.MnemonicHandler;
import sun.swing.StringUIClientPropertyKey;
test/jdk/com/sun/java/swing/plaf/gtk/TestMenuMnemonicOnAltPress.java line 1:
> 1: /*
You extended the test to Aqua, so the test doesn't belong in `swing.plaf.gtk` any more.
I recommend moving it closer to the test you wrote for Windows: [`JMenuBar/TestMenuMnemonic.java`](https://github.com/openjdk/jdk/blob/master/test/jdk/javax/swing/JMenuBar/TestMenuMnemonic.java).
The new test could be `JMenuBar/TestMenuMnemonicLinuxAndMac.java`.
The old test could be renamed to `TestMenuMnemonicWindows.java`; or leave it unchanged.
test/jdk/com/sun/java/swing/plaf/gtk/TestMenuMnemonicOnAltPress.java line 63:
> 61: UIManager.setLookAndFeel("com.sun.java.swing.plaf.gtk.GTKLookAndFeel");
> 62: } else if (laf.getName().contains("Aqua")) {
> 63: UIManager.setLookAndFeel("com.apple.laf.AquaLookAndFeel");
Suggestion:
if (laf.getName().contains("GTK")) {
UIManager.setLookAndFeel(laf.getClassName());
} else if (laf.getName().contains("Aqua")) {
UIManager.setLookAndFeel(laf.getClassName());
Now that the body of both `if` statements is the same, you can combine the conditions into one `if` statement.
-------------
Changes requested by aivanov (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18992#pullrequestreview-2148771462
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1659199665
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1659204014
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1659200851
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1659206189
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1659208779
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1659211912
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1659221421
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1659223035
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1659218539
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1659228792
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1659230759
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1659233121
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1659238602
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1659244418
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1659246138
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1659259110
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1659254091
More information about the client-libs-dev
mailing list