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

[Abhishek Kumar]

On Thu, 27 Jun 2024 17:17:12 GMT, Alexey Ivanov <aivanov at openjdk.org> wrote:

> 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.

Common `ALtProcessor` class implemented for GTK and Aqua and `MnemonicHandler` class simplified to keep only helper methods.

> 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.

Updated.

> 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.

Ok.. updated.

> 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.

0 is a valid mnemonic.

> 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.

New class added for GTK and Aqua.

> 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.

Updated.

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

PR Comment: https://git.openjdk.org/jdk/pull/18992#issuecomment-2196608935
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1658496209
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1658496399
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1658496093
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1658496843
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1658497151