On Thu, 27 Jun 2024 17:17:12 GMT, Alexey Ivanov <aiva...@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
