Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v6]
On Thu, 20 Jun 2024 20:55:11 GMT, Phil Race wrote: >> Infact `isMnemonicHidden` can also be changed to a `protected` member of the >> class. I will check and update. > > protected members of public classes are part of the API > Go look at javadoc - or generate javadoc for this change and see for > yourself. So this still requires a CSR as written. Yeah..I checked that and the protected members are a part of javadoc. One possible way to not have a CSR is to remove the access modifiers from the method that results in default access specifier. In that case, these methods no longer be a part of javadoc API. - PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1648565337
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v6]
On Tue, 18 Jun 2024 13:15:00 GMT, Abhishek Kumar wrote: >>> Hmm. So .. new public API ? Is this absolutely necessary ? >> I don't see why an app would need to call this directly. >> >> `setMnemonicHidden` can be changed to a `protected` member as you pointed >> out it may not be required by an app to call this method. >> But I guess the `isMnemonicHidden` should be public API. >> >>> And it would need a CSR, and it is too late for 23 anyway. >> >> Will update the `@since to 24` for `isMnemonicHidden` method if we are ok >> with `isMnemonicHidden` method's access modifier. > > Infact `isMnemonicHidden` can also be changed to a `protected` member of the > class. I will check and update. protected members of public classes are part of the API Go look at javadoc - or generate javadoc for this change and see for yourself. So this still requires a CSR as written. - PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1648120126
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v6]
On Fri, 14 Jun 2024 20:21:16 GMT, Phil Race wrote:
>> Abhishek Kumar has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> condition update
>
> src/java.desktop/share/classes/javax/swing/plaf/synth/SynthGraphicsUtils.java
> line 780:
>
>> 778: }
>> 779:
>> 780: if (c instanceof JLabel && ((JLabel)
>> c).getDisplayedMnemonic() != '\0') {
>
> you can use else if in all these cases
Updated.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1647252426
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v6]
On Tue, 18 Jun 2024 16:28:50 GMT, Alexey Ivanov wrote: >> Abhishek Kumar has updated the pull request incrementally with one >> additional commit since the last revision: >> >> condition update > > src/java.desktop/share/classes/javax/swing/plaf/synth/SynthRootPaneUI.java > line 115: > >> 113: KeyboardFocusManager.getCurrentKeyboardFocusManager(). >> 114: addKeyEventPostProcessor(altProcessor); >> 115: } > > Can this lead to installing `altProcessor` twice or more? Added flag variable to check of it is installed or not. - PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1647253418
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v6]
On Tue, 18 Jun 2024 16:41:12 GMT, Alexey Ivanov wrote:
>> Abhishek Kumar has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> condition update
>
> test/jdk/com/sun/java/swing/plaf/gtk/TestMenuMnemonicOnAltPress.java line 44:
>
>> 42: import javax.swing.plaf.synth.SynthLookAndFeel;
>> 43:
>> 44: public class TestMenuMnemonicOnAltPress {
>
> This test seems to repeat the
> [`JMenuBar/TestMenuMnemonic.java`](https://github.com/openjdk/jdk/blob/master/test/jdk/javax/swing/JMenuBar/TestMenuMnemonic.java)
> test that you created for Windows L&F when you worked on
> [JDK-8326458](https://bugs.openjdk.org/browse/JDK-8326458).
This test is almost similar to the
[JMenuBar/TestMenuMnemonic.java](https://github.com/openjdk/jdk/blob/master/test/jdk/javax/swing/JMenuBar/TestMenuMnemonic.java)
but is bit different for linux.
Here we are testing for Alt key press scenario whereas windows test was
checking for F10 key press. And secondly windows test checked for the menu
selection path also which is not the case in linux.
Initially I thought of extending the same test for linux fix but then decided
not to do it for not making it too much complex.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1645987858
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v6]
On Tue, 18 Jun 2024 16:26:49 GMT, Alexey Ivanov wrote: > Can we use or even re-use the logic in Windows L&F to display and hide menu > mnemonics? For windows behavior is different than linux. https://github.com/openjdk/jdk/pull/18992#issuecomment-2100033545 I don't think that the logic in Windows L&F can be reused here. > Is this fix limited to menu mnemonics? Does it apply to all mnemonics in > components? As of now verified with the menu elements. > I think you have to keep track of whether `altProcessor` was installed or > not. If the value of `"RootPane.altPress"` changes, the listener may be left > installed. Yeah, if the value of "RootPane.altPress" is set to false, the listener won't be removed. Will update. - PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1645974421 PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1645976963
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v6]
On Fri, 14 Jun 2024 10:07:39 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**), 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:
>
> condition update
Changes requested by aivanov (Reviewer).
src/java.desktop/share/classes/javax/swing/plaf/synth/SynthRootPaneUI.java line
82:
> 80: KeyboardFocusManager.getCurrentKeyboardFocusManager().
> 81: removeKeyEventPostProcessor(altProcessor);
> 82: }
I think you have to keep track of whether `altProcessor` was installed or not.
If the value of `"RootPane.altPress"` changes, the listener may be left
installed.
src/java.desktop/share/classes/javax/swing/plaf/synth/SynthRootPaneUI.java line
115:
> 113: KeyboardFocusManager.getCurrentKeyboardFocusManager().
> 114: addKeyEventPostProcessor(altProcessor);
> 115: }
Can this lead to installing `altProcessor` twice or more?
test/jdk/com/sun/java/swing/plaf/gtk/TestMenuMnemonicOnAltPress.java line 44:
> 42: import javax.swing.plaf.synth.SynthLookAndFeel;
> 43:
> 44: public class TestMenuMnemonicOnAltPress {
This test seems to repeat the
[`JMenuBar/TestMenuMnemonic.java`](https://github.com/openjdk/jdk/blob/master/test/jdk/javax/swing/JMenuBar/TestMenuMnemonic.java)
test that you created for Windows L&F when you worked on
[JDK-8326458](https://bugs.openjdk.org/browse/JDK-8326458).
-
PR Review: https://git.openjdk.org/jdk/pull/18992#pullrequestreview-2126001490
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1644763217
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1644755969
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1644770126
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v6]
On Fri, 14 Jun 2024 10:07:39 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**), 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:
>
> condition update
src/java.desktop/share/classes/javax/swing/plaf/synth/SynthLookAndFeel.java
line 1063:
> 1061: } else {
> 1062: isMnemonicHidden = hide;
> 1063: }
Can we use or even re-use the logic in Windows L&F to display and hide menu
mnemonics?
Is this fix limited to menu mnemonics? Does it apply to all mnemonics in
components?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1644753520
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v6]
On Tue, 18 Jun 2024 12:50:42 GMT, Abhishek Kumar wrote: >> src/java.desktop/share/classes/javax/swing/plaf/synth/SynthLookAndFeel.java >> line 1056: >> >>> 1054: * @param hide true if mnemonics should be hidden >>> 1055: * @since 23 >>> 1056: */ >> >> Hmm. So .. new public API ? Is this absolutely necessary ? >> I don't see why an app would need to call this directly. >> And it would need a CSR, and it is too late for 23 anyway. > >> Hmm. So .. new public API ? Is this absolutely necessary ? > I don't see why an app would need to call this directly. > > `setMnemonicHidden` can be changed to a `protected` member as you pointed out > it may not be required by an app to call this method. > But I guess the `isMnemonicHidden` should be public API. > >> And it would need a CSR, and it is too late for 23 anyway. > > Will update the `@since to 24` for `isMnemonicHidden` method if we are ok > with `isMnemonicHidden` method's access modifier. Infact `isMnemonicHidden` can also be changed to a `protected` member of the class. I will check and update. - PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r168223
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v6]
On Tue, 18 Jun 2024 12:51:34 GMT, Abhishek Kumar wrote: >> since this method should be recursing through all of a component's >> subcomponents, shouldn't there not even be an if check here? > >> JComponent extends Container .. so this will traverse everything in the >> Swing UI. > Is there any pattern elsewhere in Swing you can use to short-circuit this ? > > I am unable to recall any pattern as such. > since this method should be recursing through all of a component's > subcomponents, shouldn't there not even be an if check here? If I understand correctly, you mean to say we don't require an if condition here. But the if condition is to identify the components in container which needs to be repainted for mnemonic related changes. So if a component is an instance of container, all the sub-components are fetched and if any of sub-component is either AbstractButton or JLabel instance, it will be repainted based on mnemonic condition otherwise will be ignored (if not a container itself). - PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1644431689
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v6]
On Fri, 14 Jun 2024 20:28:11 GMT, Phil Race wrote: > Hmm. So .. new public API ? Is this absolutely necessary ? I don't see why an app would need to call this directly. `setMnemonicHidden` can be changed to a `protected` member as you pointed out it may not be required by an app to call this method. But I guess the `isMnemonicHidden` should be public API. > And it would need a CSR, and it is too late for 23 anyway. Will update the `@since to 24` for `isMnemonicHidden` method if we are ok with `isMnemonicHidden` method's access modifier. - PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1644412541
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v6]
On Sat, 15 Jun 2024 00:23:07 GMT, Alisen Chung wrote:
>> src/java.desktop/share/classes/javax/swing/plaf/synth/SynthGraphicsUtils.java
>> line 785:
>>
>>> 783: }
>>> 784:
>>> 785: if (c instanceof Container) {
>>
>> JComponent extends Container .. so this will traverse everything in the
>> Swing UI.
>> Is there any pattern elsewhere in Swing you can use to short-circuit this ?
>
> since this method should be recursing through all of a component's
> subcomponents, shouldn't there not even be an if check here?
> JComponent extends Container .. so this will traverse everything in the Swing
> UI.
Is there any pattern elsewhere in Swing you can use to short-circuit this ?
I am unable to recall any pattern as such.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1644413797
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v6]
On Fri, 14 Jun 2024 10:07:39 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**), 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: > > condition update > /csr > > I am adding this comment NOT because I want to see a CSR - I think we should > find a non-API fix, but because if I don't add it skara doesn't know that > this isn't ready to integrate as it currently is written Ok. - PR Comment: https://git.openjdk.org/jdk/pull/18992#issuecomment-2176034688
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v6]
On Fri, 14 Jun 2024 20:22:42 GMT, Phil Race wrote:
>> Abhishek Kumar has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> condition update
>
> src/java.desktop/share/classes/javax/swing/plaf/synth/SynthGraphicsUtils.java
> line 785:
>
>> 783: }
>> 784:
>> 785: if (c instanceof Container) {
>
> JComponent extends Container .. so this will traverse everything in the
> Swing UI.
> Is there any pattern elsewhere in Swing you can use to short-circuit this ?
since this method should be recursing through all of a component's
subcomponents, shouldn't there not even be an if check here?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1640558484
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v6]
On Fri, 14 Jun 2024 10:07:39 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**), 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: > > condition update I am adding this comment NOT because I want to see a CSR - I think we should find a non-API fix, but because if I don't add it skara doesn't know that this isn't ready to integrate as it currently is written - PR Comment: https://git.openjdk.org/jdk/pull/18992#issuecomment-2168757871
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v6]
On Fri, 14 Jun 2024 10:07:39 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**), 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:
>
> condition update
src/java.desktop/share/classes/javax/swing/plaf/synth/SynthGraphicsUtils.java
line 780:
> 778: }
> 779:
> 780: if (c instanceof JLabel && ((JLabel)
> c).getDisplayedMnemonic() != '\0') {
you can use else if in all these cases
src/java.desktop/share/classes/javax/swing/plaf/synth/SynthGraphicsUtils.java
line 785:
> 783: }
> 784:
> 785: if (c instanceof Container) {
JComponent extends Container .. so this will traverse everything in the Swing
UI.
Is there any pattern elsewhere in Swing you can use to short-circuit this ?
src/java.desktop/share/classes/javax/swing/plaf/synth/SynthLookAndFeel.java
line 1056:
> 1054: * @param hide true if mnemonics should be hidden
> 1055: * @since 23
> 1056: */
Hmm. So .. new public API ? Is this absolutely necessary ?
I don't see why an app would need to call this directly.
And it would need a CSR, and it is too late for 23 anyway.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1640319600
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1640320632
PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1640324672
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v6]
> In GTK LAF, the menu mnemonics are always displayed which is different from > the native behavior. In native application **(tested with gedit**), 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: condition update - Changes: - all: https://git.openjdk.org/jdk/pull/18992/files - new: https://git.openjdk.org/jdk/pull/18992/files/604cfe50..47d4c18c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18992&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18992&range=04-05 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 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
