Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v8]
On Tue, 25 Jun 2024 15:47:49 GMT, Alexey Ivanov wrote: >> Abhishek Kumar has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Remove access modifier from method declaration > > Changes requested by aivanov (Reviewer). @aivanov-jdk Updated the implementation for mnemonic handler separately inside `sun.swing` package and installed the listener specific to GTK LookAndFeel only. Mnemonic handler class is responsible for tracking the mnemonics show/hide status and repainting the components. Changes are reverted back for `SynthLookAndFeel` and `SynthRootPaneUI` files. - PR Comment: https://git.openjdk.org/jdk/pull/18992#issuecomment-2194302062
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v8]
On Tue, 25 Jun 2024 15:35:47 GMT, Alexey Ivanov wrote: >> Abhishek Kumar has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Remove access modifier from method declaration > > src/java.desktop/share/classes/com/sun/java/swing/plaf/gtk/GTKLookAndFeel.java > line 868: > >> 866: "ctrl released ENTER", "release" >> 867: }, >> 868: "RootPane.altPress", true, > > `RootPane.hideMnemonics` or `RootPane.showMnemonicsOnAltPress` could be a > more descriptive property name. Removed this property now. > You may add another condition to the if statement, mnemIndex < 0 — if there's > no mnemonic defined, there's no need to query the properties from UIManager > at all. Updated. > src/java.desktop/share/classes/javax/swing/plaf/synth/SynthLookAndFeel.java > line 1046: > >> 1044: >> 1045: // Toggle flag for drawing the mnemonic state >> 1046: private static boolean isMnemonicHidden = true; > > I wonder why it defaults to `true` where only one L derived from Synth sets > it to true. That's correct. Modified it to be `false` by default and based on L it can be set to `true`. For GTK initialization, it is set to `true` to hide mnemonics by default. > test/jdk/com/sun/java/swing/plaf/gtk/TestMenuMnemonicOnAltPress.java line 72: > >> 70: pt = fileMenu.getLocationOnScreen(); >> 71: fileMenuWidth = fileMenu.getWidth(); >> 72: fileMenuHeight = fileMenu.getHeight(); > > If you save width and height in a `Dimension` object, `fileMenuSize`, you'll > be able to use `new Rectangle(pt, fileMenuSize)` for capturing the screen. > > Since you need that rectangle only, you can create the rectangle here: > > Suggestion: > > fileMenuRect = new Rectangle(fileMenu.getLocationOnScreen(), > fileMenu.getSize()); Updated. - PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1656861152 PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1656861413 PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1656860692 PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1656858840
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v8]
On Wed, 26 Jun 2024 08:40:24 GMT, Abhishek Kumar wrote: >> Thank you for looking into it. A `MnemonicHandler` class in `sun.swing` or >> `sun.swing.plaf` package could be a good candidate. The `sun.swing` package >> contains a lot of support classes for Swing, including `SwingUtilities2` and >> `UIAction`; the `sun.swing.plaf` may be better as the mnemonic handler is >> part of PLAF. >> >> Another option is the `com.sun.java.swing` package which currently contains >> `SwingUtilities3` and `plaf` subpackage with `gtk` and `motif` related >> classes. > > I tried moving the `repaintMnemonicsInWindow` and > `repaintMnemonicsInContainer` under `SwingUtilities` class but faced build > issue while accessing them in `WindowsPopupMenuUI` file. > > I will try with the suggested packages as you mentioned. The `javax.swing.SwingUtilities` is a public class, which means adding public methods to it requires a CSR; package-private methods aren't visible from other packages. The `sun.swing` and `com.sun.java.swing` packages aren't public, you can add public classes with public methods easily; since the classes are public and the methods are public, they're accessible from any other package in Swing. - PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1654417685
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v8]
On Tue, 25 Jun 2024 15:11:31 GMT, Alexey Ivanov wrote: >> I will check and update if it is possible. > > Thank you for looking into it. A `MnemonicHandler` class in `sun.swing` or > `sun.swing.plaf` package could be a good candidate. The `sun.swing` package > contains a lot of support classes for Swing, including `SwingUtilities2` and > `UIAction`; the `sun.swing.plaf` may be better as the mnemonic handler is > part of PLAF. > > Another option is the `com.sun.java.swing` package which currently contains > `SwingUtilities3` and `plaf` subpackage with `gtk` and `motif` related > classes. I tried moving the `repaintMnemonicsInWindow` and `repaintMnemonicsInContainer` under `SwingUtilities` class but faced build issue while accessing them in `WindowsPopupMenuUI` file. I will try with the suggested packages as you mentioned. - PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1654388577
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v8]
On Fri, 21 Jun 2024 07:55:24 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 one additional
> commit since the last revision:
>
> Remove access modifier from method declaration
Changes requested by aivanov (Reviewer).
src/java.desktop/share/classes/com/sun/java/swing/plaf/gtk/GTKLookAndFeel.java
line 868:
> 866: "ctrl released ENTER", "release"
> 867: },
> 868: "RootPane.altPress", true,
`RootPane.hideMnemonics` or `RootPane.showMnemonicsOnAltPress` could be a more
descriptive property name.
src/java.desktop/share/classes/javax/swing/plaf/synth/SynthGraphicsUtils.java
line 676:
> 674: int mnemIndex =
> lh.getMenuItem().getDisplayedMnemonicIndex();
> 675: // Check to see if the Mnemonic should be rendered in
> GTK.
> 676: if (UIManager.getBoolean("RootPane.altPress")
You may add another condition to the `if` statement, `mnemIndex < 0` — if
there's no mnemonic defined, there's no need to query the properties from
`UIManager` at all.
Is the new property even needed? Is `Button.showMnemonics` not enough? This
property controls mnemonics on all the components, including menu bar, in
Windows and Aqua. Can't Synth and GTK follow the same pattern?
src/java.desktop/share/classes/javax/swing/plaf/synth/SynthGraphicsUtils.java
line 679:
> 677: && SynthLookAndFeel.isMnemonicHidden()) {
> 678: mnemIndex = -1;
> 679: }
Perhaps, verifying the value of the `RootPane.altPress` property should also be
moved into `SynthLookAndFeel.isMnemonicHidden()` which already queries the
value of `Button.showMnemonics`. Keeping all the properties that affect hiding
or showing the mnemonics in one place is easier to reason about if anything
doesn't work as expected.
src/java.desktop/share/classes/javax/swing/plaf/synth/SynthLookAndFeel.java
line 1046:
> 1044:
> 1045: // Toggle flag for drawing the mnemonic state
> 1046: private static boolean isMnemonicHidden = true;
I wonder why it defaults to `true` where only one L derived from Synth sets
it to true.
test/jdk/com/sun/java/swing/plaf/gtk/TestMenuMnemonicOnAltPress.java line 72:
> 70: pt = fileMenu.getLocationOnScreen();
> 71: fileMenuWidth = fileMenu.getWidth();
> 72: fileMenuHeight = fileMenu.getHeight();
If you save width and height in a `Dimension` object, `fileMenuSize`, you'll be
able to use `new Rectangle(pt, fileMenuSize)` for capturing the screen.
Since you need that rectangle only, you can create the rectangle here:
Suggestion:
fileMenuRect = new Rectangle(fileMenu.getLocationOnScreen(),
fileMenu.getSize());
-
PR Review: https://git.openjdk.org/jdk/pull/18992#pullrequestreview-2139047741
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1653058424
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1653054277
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1653051513
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1653063685
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1653089074
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v8]
On Tue, 25 Jun 2024 15:18:29 GMT, Alexey Ivanov wrote: >>> In my initial fix, I added the `altProcessor` handler in >>> `SynthLookAndFeel.initialize` with condition check for GTK L Phil has >>> suggested not to check for GTK L instead look for some alternate way like >>> mentioned >>> [here](https://github.com/openjdk/jdk/pull/18992#discussion_r1595782003). >>> >>> So, the handler implementation is moved to `SynthRootPaneUI`. >> >> Thus, out of Synth-derived Look and Feels, only GTK needs the feature. Can't >> the listener be installed in `GTKLookAndFeel.initialize`? >> >> At the same time, if you install the listener in `SynthLookAndFeel`, other >> L based on Synth could also use this feature, thus your way is more >> flexible. >> >> However, `SynthRootPaneUI` is still part of the base class… I still don't >> understand how it's different from what I'm proposing. >> >> I'd like to avoid keeping a flag whether the listener is installed or not… >> But there could be no other way since not all Synth-based L enable hiding >> mnemonics. > >> > Requesting the value of RootPane.altPress from UIManager each time >> > postProcessKeyEvent is called is inefficient, so you can store the value >> > in altProcessor when look and feel is installed. >> >> I guess you are pointing out the code in SynthGraphicsUtils.paintText method >> where RootPane.altPress value is retrieved from UIManager each time. Storing >> the value in it's constructor doesn't reflect the correct value and is >> always `false`. > > No, I didn't refer to this particular case. I referred to my suggestion where > you always add `altProcessor` as the keyboard listener and keep track of the > value of the `RootPane.altPress` property. > > Anyway this approach seems to be rejected. > > If RootPane.altPress can be changed dynamically, you can install a > > PropertyChangeListener to UIManager. > > I think it can be changed but is it really required to handle it ? I mean why > does a user change it dynamically ? Not impossible. On Windows, there used to be an option to hide or display mnemonics and focus indicators, and this option could be changed while the app is running. I think there's no option in GNOME, so we shouldn't go out of the way to support dynamic change of the `RootPane.altPress` property. - PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1653037803
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v8]
On Tue, 25 Jun 2024 14:58:53 GMT, Alexey Ivanov wrote: >>> If RootPane.altPress can be changed dynamically, you can install a >>> PropertyChangeListener to UIManager. >> >> I think it can be changed but is it really required to handle it ? >> I mean why does a user change it dynamically ? > >> In my initial fix, I added the `altProcessor` handler in >> `SynthLookAndFeel.initialize` with condition check for GTK L Phil has >> suggested not to check for GTK L instead look for some alternate way like >> mentioned >> [here](https://github.com/openjdk/jdk/pull/18992#discussion_r1595782003). >> >> So, the handler implementation is moved to `SynthRootPaneUI`. > > Thus, out of Synth-derived Look and Feels, only GTK needs the feature. Can't > the listener be installed in `GTKLookAndFeel.initialize`? > > At the same time, if you install the listener in `SynthLookAndFeel`, other > L based on Synth could also use this feature, thus your way is more > flexible. > > However, `SynthRootPaneUI` is still part of the base class… I still don't > understand how it's different from what I'm proposing. > > I'd like to avoid keeping a flag whether the listener is installed or not… > But there could be no other way since not all Synth-based L enable hiding > mnemonics. > > Requesting the value of RootPane.altPress from UIManager each time > > postProcessKeyEvent is called is inefficient, so you can store the value in > > altProcessor when look and feel is installed. > > I guess you are pointing out the code in SynthGraphicsUtils.paintText method > where RootPane.altPress value is retrieved from UIManager each time. Storing > the value in it's constructor doesn't reflect the correct value and is always > `false`. No, I didn't refer to this particular case. I referred to my suggestion where you always add `altProcessor` as the keyboard listener and keep track of the value of the `RootPane.altPress` property. Anyway this approach seems to be rejected. - PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1653029069
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v8]
On Mon, 24 Jun 2024 07:20:24 GMT, Abhishek Kumar wrote:
>> src/java.desktop/share/classes/javax/swing/plaf/synth/SynthGraphicsUtils.java
>> line 751:
>>
>>> 749: * Repaints all the components with the mnemonics in the given
>>> window and all its owned windows.
>>> 750: */
>>> 751: static void repaintMnemonicsInWindow(final Window w) {
>>
>> The `repaintMnemonicsInWindow` and `repaintMnemonicsInContainer` are exactly
>> the same as methods in `WindowsGraphicsUtils`. And `AquaMnemonicHandler` has
>> yet another copy of the same code.
>>
>> Is it possible to move these methods to a utility class that's available for
>> all Look-and-Feels to avoid duplicating code?
>
> I will check and update if it is possible.
Thank you for looking into it. A `MnemonicHandler` class in `sun.swing` or
`sun.swing.plaf` package could be a good candidate. The `sun.swing` package
contains a lot of support classes for Swing, including `SwingUtilities2` and
`UIAction`; the `sun.swing.plaf` may be better as the mnemonic handler is part
of PLAF.
Another option is the `com.sun.java.swing` package which currently contains
`SwingUtilities3` and `plaf` subpackage with `gtk` and `motif` related classes.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1653017112
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v8]
On Mon, 24 Jun 2024 07:21:49 GMT, Abhishek Kumar wrote: >>>Requesting the value of RootPane.altPress from UIManager each time >>>postProcessKeyEvent is called is inefficient, so you can store the value in >>>altProcessor when look and feel is installed. >> >> I guess you are pointing out the code in SynthGraphicsUtils.paintText method >> where RootPane.altPress value is retrieved from UIManager each time. Storing >> the value in it's constructor doesn't reflect the correct value and is >> always `false`. > >> If RootPane.altPress can be changed dynamically, you can install a >> PropertyChangeListener to UIManager. > > I think it can be changed but is it really required to handle it ? > I mean why does a user change it dynamically ? > In my initial fix, I added the `altProcessor` handler in > `SynthLookAndFeel.initialize` with condition check for GTK L Phil has > suggested not to check for GTK L instead look for some alternate way like > mentioned > [here](https://github.com/openjdk/jdk/pull/18992#discussion_r1595782003). > > So, the handler implementation is moved to `SynthRootPaneUI`. Thus, out of Synth-derived Look and Feels, only GTK needs the feature. Can't the listener be installed in `GTKLookAndFeel.initialize`? At the same time, if you install the listener in `SynthLookAndFeel`, other L based on Synth could also use this feature, thus your way is more flexible. However, `SynthRootPaneUI` is still part of the base class… I still don't understand how it's different from what I'm proposing. I'd like to avoid keeping a flag whether the listener is installed or not… But there could be no other way since not all Synth-based L enable hiding mnemonics. - PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1652996654
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v8]
On Mon, 24 Jun 2024 07:19:49 GMT, Abhishek Kumar wrote: > Should I revert it back to javadoc style comment ? Absolutely! - PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1652814116
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v8]
On Mon, 24 Jun 2024 07:17:19 GMT, Abhishek Kumar wrote: >>> Wouldn't it be easier to install altProcessor always in >>> SynthLookAndFeel.initialize and to uninstall it in >>> SynthLookAndFeel.uninitialize like it's done in WindowsLookAndFeel: >> https://github.com/openjdk/jdk/blob/master/src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsLookAndFeel.java#L197-L198 >> Then altProcessor would do nothing if the RootPane.altPress property isn't >> set or is false. >> >> In my initial fix, I added the `altProcessor` handler in >> `SynthLookAndFeel.initialize` with condition check for GTK L Phil has >> suggested not to check for GTK L instead look for some alternate way like >> mentioned >> [here](https://github.com/openjdk/jdk/pull/18992#discussion_r1595782003). >> >> Now I left with few options: >> 1. Install `altProcessor` handler similar to `WindowsLookAndFeel` >> implementation but that results in unnecessary installation of `alt handler >> for Nimbus L (derived from Synth L)` as well. >> 2. Add the `RootPane.altPress` condition check before installing the >> `altProcessor` handler but the value returned for `RootPane.altPress` is >> always **false** and that left with `altProcessor` handler uninstalled. >> >> So, the handler implementation is moved to `SynthRootPaneUI`. > >>Requesting the value of RootPane.altPress from UIManager each time >>postProcessKeyEvent is called is inefficient, so you can store the value in >>altProcessor when look and feel is installed. > > I guess you are pointing out the code in SynthGraphicsUtils.paintText method > where RootPane.altPress value is retrieved from UIManager each time. Storing > the value in it's constructor doesn't reflect the correct value and is always > `false`. > If RootPane.altPress can be changed dynamically, you can install a > PropertyChangeListener to UIManager. I think it can be changed but is it really required to handle it ? I mean why does a user change it dynamically ? - PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1650477692
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v8]
On Mon, 24 Jun 2024 06:21:51 GMT, Abhishek Kumar wrote: >> src/java.desktop/share/classes/javax/swing/plaf/synth/SynthRootPaneUI.java >> line 45: >> >>> 43: private SynthStyle style; >>> 44: static final AltProcessor altProcessor = new AltProcessor(); >>> 45: static boolean altProcessorInstalledFlag; >> >> Wouldn't it be easier to install `altProcessor` always in >> `SynthLookAndFeel.initialize` and to uninstall it in >> `SynthLookAndFeel.uninitialize` like it's done in `WindowsLookAndFeel`: >> >> https://github.com/openjdk/jdk/blob/master/src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsLookAndFeel.java#L197-L198 >> >> Then `altProcessor` would do nothing if the `RootPane.altPress` property >> isn't set or is `false`. >> >> Requesting the value of `RootPane.altPress` from `UIManager` each time >> `postProcessKeyEvent` is called is inefficient, so you can store the value >> in `altProcessor` when look and feel is installed. >> >> If `RootPane.altPress` can be changed dynamically, you can install a >> `PropertyChangeListener` to `UIManager`. > >> Wouldn't it be easier to install altProcessor always in >> SynthLookAndFeel.initialize and to uninstall it in >> SynthLookAndFeel.uninitialize like it's done in WindowsLookAndFeel: > https://github.com/openjdk/jdk/blob/master/src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsLookAndFeel.java#L197-L198 > Then altProcessor would do nothing if the RootPane.altPress property isn't > set or is false. > > In my initial fix, I added the `altProcessor` handler in > `SynthLookAndFeel.initialize` with condition check for GTK L Phil has > suggested not to check for GTK L instead look for some alternate way like > mentioned > [here](https://github.com/openjdk/jdk/pull/18992#discussion_r1595782003). > > Now I left with few options: > 1. Install `altProcessor` handler similar to `WindowsLookAndFeel` > implementation but that results in unnecessary installation of `alt handler > for Nimbus L (derived from Synth L)` as well. > 2. Add the `RootPane.altPress` condition check before installing the > `altProcessor` handler but the value returned for `RootPane.altPress` is > always **false** and that left with `altProcessor` handler uninstalled. > > So, the handler implementation is moved to `SynthRootPaneUI`. >Requesting the value of RootPane.altPress from UIManager each time >postProcessKeyEvent is called is inefficient, so you can store the value in >altProcessor when look and feel is installed. I guess you are pointing out the code in SynthGraphicsUtils.paintText method where RootPane.altPress value is retrieved from UIManager each time. Storing the value in it's constructor doesn't reflect the correct value and is always `false`. - PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1650472661
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v8]
On Fri, 21 Jun 2024 20:14:28 GMT, Alexey Ivanov wrote:
>> Abhishek Kumar has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Remove access modifier from method declaration
>
> src/java.desktop/share/classes/javax/swing/plaf/synth/SynthGraphicsUtils.java
> line 751:
>
>> 749: * Repaints all the components with the mnemonics in the given
>> window and all its owned windows.
>> 750: */
>> 751: static void repaintMnemonicsInWindow(final Window w) {
>
> The `repaintMnemonicsInWindow` and `repaintMnemonicsInContainer` are exactly
> the same as methods in `WindowsGraphicsUtils`. And `AquaMnemonicHandler` has
> yet another copy of the same code.
>
> Is it possible to move these methods to a utility class that's available for
> all Look-and-Feels to avoid duplicating code?
I will check and update if it is possible.
>You can still write proper javadocs for members with any visibility, and IDE
>will show you the description of a method or a field when you hover over its
>usage anywhere in the code.
You already wrote the javadoc, leave them.
A regular comment won't be shown this way.
Should I revert it back to javadoc style comment ?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1650476132
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1650475455
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v8]
On Fri, 21 Jun 2024 20:08:58 GMT, Alexey Ivanov wrote: > Wouldn't it be easier to install altProcessor always in > SynthLookAndFeel.initialize and to uninstall it in > SynthLookAndFeel.uninitialize like it's done in WindowsLookAndFeel: https://github.com/openjdk/jdk/blob/master/src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsLookAndFeel.java#L197-L198 Then altProcessor would do nothing if the RootPane.altPress property isn't set or is false. In my initial fix, I added the `altProcessor` handler in `SynthLookAndFeel.initialize` with condition check for GTK L Phil has suggested not to check for GTK L instead look for some alternate way like mentioned [here](https://github.com/openjdk/jdk/pull/18992#discussion_r1595782003). Now I left with few options: 1. Install `altProcessor` handler similar to `WindowsLookAndFeel` implementation but that results in unnecessary installation of `alt handler for Nimbus L (derived from Synth L)` as well. 2. Add the `RootPane.altPress` condition check before installing the `altProcessor` handler but the value returned for `RootPane.altPress` is always **false** and that left with `altProcessor` handler uninstalled. So, the handler implementation is moved to `SynthRootPaneUI`. - PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1650410849
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v8]
On Fri, 21 Jun 2024 07:55:24 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 one additional
> commit since the last revision:
>
> Remove access modifier from method declaration
Changes requested by aivanov (Reviewer).
src/java.desktop/share/classes/javax/swing/plaf/synth/SynthGraphicsUtils.java
line 751:
> 749: * Repaints all the components with the mnemonics in the given
> window and all its owned windows.
> 750: */
> 751: static void repaintMnemonicsInWindow(final Window w) {
The `repaintMnemonicsInWindow` and `repaintMnemonicsInContainer` are exactly
the same as methods in `WindowsGraphicsUtils`. And `AquaMnemonicHandler` has
yet another copy of the same code.
Is it possible to move these methods to a utility class that's available for
all Look-and-Feels to avoid duplicating code?
src/java.desktop/share/classes/javax/swing/plaf/synth/SynthGraphicsUtils.java
line 779:
> 777: c.repaint();
> 778: continue;
> 779: } else if (c instanceof Container){
You said you were handling menu mnemonics only, yet this is more than menus.
However, this would make the UI look consistent.
src/java.desktop/share/classes/javax/swing/plaf/synth/SynthLookAndFeel.java
line 1053:
> 1051: * This method is a non operation if the underlying operating system
> 1052: * does not support the mnemonic hiding feature.
> 1053: */
You can still write proper javadocs for members with any visibility, and IDE
will show you the description of a method or a field when you hover over its
usage anywhere in the code.
You already wrote the javadoc, leave them.
A regular comment won't be shown this way.
src/java.desktop/share/classes/javax/swing/plaf/synth/SynthRootPaneUI.java line
45:
> 43: private SynthStyle style;
> 44: static final AltProcessor altProcessor = new AltProcessor();
> 45: static boolean altProcessorInstalledFlag;
Wouldn't it be easier to install `altProcessor` always in
`SynthLookAndFeel.initialize` and to uninstall it in
`SynthLookAndFeel.uninitialize` like it's done in `WindowsLookAndFeel`:
https://github.com/openjdk/jdk/blob/master/src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsLookAndFeel.java#L197-L198
Then `altProcessor` would do nothing if the `RootPane.altPress` property isn't
set or is `false`.
Requesting the value of `RootPane.altPress` from `UIManager` each time
`postProcessKeyEvent` is called is inefficient, so you can store the value in
`altProcessor` when look and feel is installed.
If `RootPane.altPress` can be changed dynamically, you can install a
`PropertyChangeListener` to `UIManager`.
-
PR Review: https://git.openjdk.org/jdk/pull/18992#pullrequestreview-2133329175
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1649390460
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1649377154
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1649372792
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1649386106
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v8]
> 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: Remove access modifier from method declaration - Changes: - all: https://git.openjdk.org/jdk/pull/18992/files - new: https://git.openjdk.org/jdk/pull/18992/files/dc9465ac..b6cfd95c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18992=07 - incr: https://webrevs.openjdk.org/?repo=jdk=18992=06-07 Stats: 8 lines in 1 file changed: 0 ins; 4 del; 4 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
