Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v9]
On Thu, 27 Jun 2024 17:17:12 GMT, Alexey Ivanov wrote: >> 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. @aivanov-jdk I have updated the PR to have a common AltProcessor class to handle Alt key press for GTK and Aqua. Helper methods are now part of MnemonicHandler class and due to that there are number of files related to Windows and Aqua implementation got changed. Test program is extended to verify the behavior for Aqua L&F as well. Currently WIndowsLookAndFeel class still contains the `setMnemonicHidden` and `isMnemonicHidden` APIs which should be removed but I am not sure if they can directly be removed or first it should be marked as deprecated ? same confusion for AquaMnemonicHandler class as well which is unused now. CI testing is in progress to confirm if there is any regression. - PR Comment: https://git.openjdk.org/jdk/pull/18992#issuecomment-2196623255
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v9]
On Thu, 27 Jun 2024 17:17:12 GMT, Alexey Ivanov 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
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v9]
On Thu, 27 Jun 2024 10:06:49 GMT, Abhishek Kumar 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
`repaintMnemonicsI
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v9]
> 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 - Changes: - all: https://git.openjdk.org/jdk/pull/18992/files - new: https://git.openjdk.org/jdk/pull/18992/files/b6cfd95c..e1aae36e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18992&range=08 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18992&range=07-08 Stats: 302 lines in 6 files changed: 166 ins; 126 del; 10 mod Patch: https://git.openjdk.org/jdk/pull/18992.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18992/head:pull/18992 PR: https://git.openjdk.org/jdk/pull/18992
