On Wed, 3 Jul 2024 11:17:56 GMT, Abhishek Kumar <abhis...@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:
>
> Test update to refer MnemonicHandler class
This changeset enables hiding / showing mnemonics on `JMenuBar` only. Do you
plan to update `ButtonUI` and `LabelUI` for GTK Look-and-Feel too?
src/java.desktop/macosx/classes/com/apple/laf/AquaLabelUI.java line 37:
> 35: import javax.swing.plaf.UIResource;
> 36:
> 37: import javax.swing.plaf.basic.BasicLabelUI;
I would prefer not to add blank lines between these imports.
Having a blank line between `java.*` and `javax.*` above is fine.
src/java.desktop/macosx/classes/com/apple/laf/AquaLabelUI.java line 43:
> 41:
> 42: import com.apple.laf.AquaUtils.RecyclableSingleton;
> 43: import com.apple.laf.AquaUtils.RecyclableSingletonFromDefaultConstructor;
I'd sort them alphabetically too: `sun.swing` follows `com.apple.laf`. I'd not
add a blank line between these imports.
src/java.desktop/macosx/classes/com/apple/laf/AquaLookAndFeel.java line 58:
> 56: import apple.laf.JRSUIControl;
> 57: import apple.laf.JRSUIUtils;
> 58:
Remove the blank line?
src/java.desktop/macosx/classes/com/apple/laf/AquaMenuPainter.java line 37:
> 35: import java.awt.Rectangle;
> 36:
> 37: import java.awt.event.InputEvent;
No blank line here.
src/java.desktop/macosx/classes/com/apple/laf/AquaMenuPainter.java line 52:
> 50: import javax.swing.UIManager;
> 51:
> 52: import javax.swing.border.Border;
No blank line here.
src/java.desktop/macosx/classes/com/apple/laf/AquaMenuPainter.java line 62:
> 60: import apple.laf.JRSUIConstants.Widget;
> 61:
> 62: import com.apple.laf.AquaIcon.InvertableIcon;
Maybe preserve the blank lines in this section, yet it could be better to
remove these too.
src/java.desktop/share/classes/com/sun/java/swing/plaf/gtk/GTKLookAndFeel.java
line 46:
> 44: import java.util.Map;
> 45: import sun.awt.SunToolkit;
> 46: import sun.awt.UNIXToolkit;
Optimize imports in this file too, to expand the wildcard imports.
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsGraphicsUtils.java
line 43:
> 41: import javax.swing.JMenu;
> 42: import javax.swing.JMenuItem;
> 43: import javax.swing.UIManager;
Suggestion:
import javax.swing.AbstractButton;
import javax.swing.ButtonModel;
import javax.swing.JButton;
import javax.swing.JCheckBox;
import javax.swing.JMenu;
import javax.swing.JMenuItem;
import javax.swing.JRadioButton;
import javax.swing.JToggleButton;
import javax.swing.UIManager;
Keep the imports sorted.
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsGraphicsUtils.java
line 49:
> 47: import sun.swing.SwingUtilities2;
> 48:
> 49: import com.sun.java.swing.plaf.windows.WindowsButtonUI;
The `WindowsButtonUI` import is unused.
test/jdk/javax/swing/JMenuBar/TestMenuMnemonicLinuxAndMac.java line 63:
> 61: System.out.println("Testing: "+laf.getName());
> 62: UIManager.setLookAndFeel(laf.getClassName());
> 63: }
Suggestion:
if (laf.getName().contains("GTK") ||
laf.getName().contains("Aqua")) {
System.out.println("Testing: " + laf.getName());
UIManager.setLookAndFeel(laf.getClassName());
break;
}
Since we expect one and only one L&F, break from the loop as soon as you found
a match.
-------------
Changes requested by aivanov (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18992#pullrequestreview-2159270954
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1665920495
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1665921960
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1665923019
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1665924234
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1665924574
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1665925751
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1665926565
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1665928024
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1665927518
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1665934285
