On Thu, 27 Jun 2024 10:06:49 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 two additional
> commits since the last revision:
>
> - Mnemonic handler class
> - Mnemonic handler added and previous implementation revert back
I think we're moving in the right direction.
My idea was to extract the common functionality as the methods
`setMnemonicHidden`, `isMnemonicHidden` and `repaintMnemonicsInWindow`,
`repaintMnemonicsInContainer` into a helper class.
`AltProcessor`… Your version for GTK and the version in Aqua look the same to
me. I think it makes sense to create a common `AltProcessor` class which would
be used by GTK and Aqua.
The Windows version is different, so it'll stay this way… It doesn't look worth
trying to fit that version into a shared class.
src/java.desktop/share/classes/com/sun/java/swing/plaf/gtk/GTKLookAndFeel.java
line 1461:
> 1459:
> 1460: KeyboardFocusManager.getCurrentKeyboardFocusManager().
> 1461: addKeyEventPostProcessor(MnemonicHandler.altProcessor);
Suggestion:
KeyboardFocusManager.getCurrentKeyboardFocusManager()
.addKeyEventPostProcessor(MnemonicHandler.altProcessor);
Better wrap the line before the dot operator — this conveys that it's a
continuation of a call sequence rather than a wrapped parameter to a method.
src/java.desktop/share/classes/com/sun/java/swing/plaf/gtk/GTKLookAndFeel.java
line 1469:
> 1467: /**
> 1468: * Called by UIManager when this look and feel is uninstalled.
> 1469: */
Does it repeat the javadoc of the super class? Likely it does, use
`{@inheritDoc}` instead.
src/java.desktop/share/classes/com/sun/java/swing/plaf/gtk/GTKLookAndFeel.java
line 1470:
> 1468: * Called by UIManager when this look and feel is uninstalled.
> 1469: */
> 1470: @Override
You may want to add the `@Override` annotation to `initialize` method too… for
consistency.
src/java.desktop/share/classes/javax/swing/plaf/synth/SynthGraphicsUtils.java
line 672:
> 670: int mnemIndex =
> lh.getMenuItem().getDisplayedMnemonicIndex();
> 671: // Check to see if the Mnemonic should be rendered in
> GTK.
> 672: if (mnemIndex >= 0 &&
> MnemonicHandler.isMnemonicHidden()) {
Is 0 a valid mnemonic? Probably not.
src/java.desktop/share/classes/sun/swing/MnemonicHandler.java line 40:
> 38: import javax.swing.UIManager;
> 39:
> 40: public class MnemonicHandler {
Suggestion:
public final class MnemonicHandler {
It's an utility class that should not be extended. In addition to that, it
should have a private constructor to prevent instantiating the class.
src/java.desktop/share/classes/sun/swing/MnemonicHandler.java line 41:
> 39:
> 40: public class MnemonicHandler {
> 41: public static final AltProcessor altProcessor = new AltProcessor();
I'd like to see `AltProcessor` here but it's different for Windows. Thus,
`AltProcessor` should be local to `*RootUI` as it is now.
If `AltProcessor` in GTK and Aqua are similar or the same, it makes sense to
extract them into a common helper class.
src/java.desktop/share/classes/sun/swing/MnemonicHandler.java line 43:
> 41: public static final AltProcessor altProcessor = new AltProcessor();
> 42:
> 43: protected static boolean isMnemonicHidden;
Suggestion:
private static boolean isMnemonicHidden;
It's accessed via `isMnemonicHidden` and `setMnemonicHidden`; since the class
cannot be extended, it should not have protected members.
src/java.desktop/share/classes/sun/swing/MnemonicHandler.java line 103:
> 101: * Repaints all the components with the mnemonics in the given
> window and all its owned windows.
> 102: */
> 103: static void repaintMnemonicsInWindow(final Window w) {
Suggestion:
public static void repaintMnemonicsInWindow(final Window w) {
This method would be called from `AltProcessor` class in the three different
L&Fs, it should be `public`.
src/java.desktop/share/classes/sun/swing/MnemonicHandler.java line 120:
> 118: * Recursively searches for all the subcomponents.
> 119: */
> 120: static void repaintMnemonicsInContainer(final Container cont) {
If `repaintMnemonicsInContainer` is never called directly but only from
`repaintMnemonicsInWindow` above, the method should be `private`.
src/java.desktop/share/classes/sun/swing/MnemonicHandler.java line 131:
> 129: c.repaint();
> 130: continue;
> 131: } else if (c instanceof Container) {
`continue` is redundant if you chain `if` block with `else if`.
If you remove `continue` from the first `if` block, it becomes empty, which
isn't good. I suggest moving the first block into its own `if` statement:
if (c == null || !c.isVisible()) {
continue;
}
if (/* other conditions */) {
You can use pattern matching to avoid explicit cast. Are we planning to
backporting this change to JDK 11? If not, I'd go for pattern matching:
if (c instanceof AbstractButton b && b.getMnemonic() != '\0') {
c.repaint();
}
Likely, the condition can be simplified further: the button and the label cases
have the same body, and thus the two conditions can be combined into one
condition.
-------------
Changes requested by aivanov (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18992#pullrequestreview-2145923809
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1657452729
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1657450301
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1657450931
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1657448791
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1657454306
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1657457592
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1657459171
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1657461663
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1657462542
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1657470679
