jdk icon indicating copy to clipboard operation
jdk copied to clipboard

Published 20 hours ago • verified publisher icon openjdk

8155030: The Menu Mnemonics are always displayed for GTK LAF

Open kumarabhi006 opened this issue 1 year ago • 14 comments
trafficstars

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.

Progress

  • [ ] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • [x] Change must not contain extraneous whitespace
  • [x] Commit message must refer to an issue
  • [ ] Change requires a CSR request matching fixVersion 24 to be approved (needs to be created)

Issue

  • JDK-8155030: The Menu Mnemonics are always displayed for GTK LAF (Bug - P5)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18992/head:pull/18992
$ git checkout pull/18992

Update a local copy of the PR:
$ git checkout pull/18992
$ git pull https://git.openjdk.org/jdk.git pull/18992/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18992

View PR using the GUI difftool:
$ git pr show -t 18992

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18992.diff

Webrev

Link to Webrev Comment

kumarabhi006 avatar Apr 29 '24 09:04 kumarabhi006

:wave: Welcome back abhiscxk! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

bridgekeeper[bot] avatar Apr 29 '24 09:04 bridgekeeper[bot]

@kumarabhi006 This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8155030: The Menu Mnemonics are always displayed for GTK LAF

Hides mnemonics on menus, buttons, and labels for GTK L&F.

Moved shared code for hiding mnemonics into
sun/swing/MnemonicHandler and AltProcessor to avoid code duplication.

Reviewed-by: prr, tr, achung, dnguyen, aivanov

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 270 new commits pushed to the master branch:

  • 73e3e0edeb20c6f701b213423476f92fb05dd262: 8321509: False positive in get_trampoline fast path causes crash
  • 9eb611e7f07ebb6eb0cbcca32d644abf8352c991: 8334055: Unhelpful 'required: reference' diagnostics after JDK-8043226
  • 5100303c6c5e4224d2c41f90719139bb5f4e236e: 8335668: NumberFormat integer only parsing should throw exception for edge case
  • 58c98420b65bcea08f37982fdfba747005c03553: 8336021: Doccheck: valign not allowed for HTML5 in java.xml
  • d06d79c80980644df511cded0eb8bc0309d878d3: 8325369: @sealedGraph: Bad link to image for tag on nested classes
  • dea92742c2b5889717f2183dc29b5772daff5340: 8332125: [nmt] Totals in diff report should print out total malloc and mmap diffs
  • 5c612c230b0a852aed5fd36e58b82ebf2e1838af: 8332689: RISC-V: Use load instead of trampolines
  • 6fcd49f9431cc3507f96ef2acdca43fc6a394a14: 8336239: Fix javadoc markup in java.lang.Process
  • b32e4a68bca588d908bd81a398eb3171a6876dc5: 8335356: Shenandoah: Improve concurrent cleanup locking
  • 62cbf70346e78ca94ce6ea4ba5a308ea0a2bbfa8: 8336085: Fix simple -Wzero-as-null-pointer-constant warnings in CDS code
  • ... and 260 more: https://git.openjdk.org/jdk/compare/e227c7e37d4de0656f013f3a936b1acfa56cc2e0...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk[bot] avatar Apr 29 '24 09:04 openjdk[bot]

@kumarabhi006 The following label will be automatically applied to this pull request:

  • client

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

openjdk[bot] avatar Apr 29 '24 09:04 openjdk[bot]

Webrevs

mlbridge[bot] avatar Apr 29 '24 09:04 mlbridge[bot]

The fix applies to single/first menu, its doesn't apply to other Menu items when there are more than one. Since you have taken gedit as reference where one Menu item is present you might have followed according to it, but when you see other native applications like Libre, on press of Alt key all the Menu Mnemonics are toggled. It would be better to consider update the fix for all Menus.

Checked with the SwingSet2 demo with GTK Look and Feel where more than one menu or menuitem are available to test. It is working as expected on press of ALT key, the mnemonics are toggled between show/hide for all menus and the menu items if menu is expanded.

kumarabhi006 avatar May 07 '24 17:05 kumarabhi006

The fix applies to single/first menu, its doesn't apply to other Menu items when there are more than one. Since you have taken gedit as reference where one Menu item is present you might have followed according to it, but when you see other native applications like Libre, on press of Alt key all the Menu Mnemonics are toggled. It would be better to consider update the fix for all Menus.

Checked with the SwingSet2 demo with GTK Look and Feel where more than one menu or menuitem are available to test. It is working as expected on press of ALT key, the mnemonics are toggled between show/hide for all menus and the menu items if menu is expanded.

Yeah, its working fine, issue with my test program. Tested with SwingSet2 and its working fine.

TejeshR13 avatar May 08 '24 03:05 TejeshR13

postProcessKeyEvent(KeyEvent ev) method from AquaMnemonicHandler class is reused instead of WindowsRootPaneUI class, any reason for that? (Since I see some differences in the way they are processing the event).

In Windows, the behavior is different on press of ALT key to toggle the mnemonics. Please refer this https://github.com/openjdk/jdk/pull/17961#issuecomment-1965074756 where it is mentioned that the first press of Alt activates the menu bar and displays the mnemonics, the second press of Alt key returns the focus to the component which had the focus before Alt was pressed for the first time and removes the mnemonics from the menu bar. Screenshot on Win10 can be refer here https://github.com/openjdk/jdk/pull/17961#pullrequestreview-1906613974.

But in Linux I can't see that the "first press of ALT activates the menu bar and the second press of Alt key returns the focus to the component which had the focus before Alt key was pressed for first time".

So, the method postProcessKeyEvent(KeyEvent ev) is different in linux compare to windows machine.

kumarabhi006 avatar May 08 '24 08:05 kumarabhi006

/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

prrace avatar Jun 14 '24 21:06 prrace

@prrace has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@kumarabhi006 please create a CSR request for issue JDK-8155030 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

openjdk[bot] avatar Jun 14 '24 21:06 openjdk[bot]

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

kumarabhi006 avatar Jun 18 '24 12:06 kumarabhi006

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

kumarabhi006 avatar Jun 27 '24 10:06 kumarabhi006

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.

kumarabhi006 avatar Jun 28 '24 10:06 kumarabhi006

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

kumarabhi006 avatar Jun 28 '24 10:06 kumarabhi006

@aivanov-jdk TestMenuMnemonic.java, bug4736093.java and bug6921687.java tests failed after the helper methods are moved from WindowsLookAndFeel.java file to MnemonicHandler.java. Updated the test to invoke helper methods from MnemonicHandler class. CI testing is all ok after these tests update.

kumarabhi006 avatar Jul 03 '24 11:07 kumarabhi006

This changeset enables hiding / showing mnemonics on JMenuBar only. Do you plan to update ButtonUI and LabelUI for GTK Look-and-Feel too?

Not as a part of this PR. It can be taken up as other bug.

kumarabhi006 avatar Jul 04 '24 18:07 kumarabhi006

@aivanov-jdk Imports are expanded and sorted as per recent review comments. I guess everything is fine now.

kumarabhi006 avatar Jul 04 '24 18:07 kumarabhi006

This changeset enables hiding / showing mnemonics on JMenuBar only. Do you plan to update ButtonUI and LabelUI for GTK Look-and-Feel too?

Not as a part of this PR. It can be taken up as other bug.

Does it make sense to handle mnemonics in buttons and labels as a separate bug? I doesn't seem worth it.

You've done 98% of the job in this PR. Refactoring touched all the classes which hide mnemonics.

The only thing left is to add handling into SynthButtonUI and SynthLabelUI where you need to replace b.getDisplayedMnemonicIndex() and label.getDisplayedMnemonicIndex() with a conditional display based on MnemonicHandler.isMnemonicHidden().

I still think it is better to include mnemonics for buttons and labels in this PR. It could be done easily by the following diff:

diff --git a/src/java.desktop/share/classes/javax/swing/plaf/synth/SynthGraphicsUtils.java b/src/java.desktop/share/classes/javax/swing/plaf/synth/SynthGraphicsUtils.java
index befa65d095e..514bfe400a5 100644
--- a/src/java.desktop/share/classes/javax/swing/plaf/synth/SynthGraphicsUtils.java
+++ b/src/java.desktop/share/classes/javax/swing/plaf/synth/SynthGraphicsUtils.java
@@ -361,7 +361,9 @@ public void paintText(SynthContext ss, Graphics g, String text,
             FontMetrics fm = SwingUtilities2.getFontMetrics(c, g);
             y += fm.getAscent();
             SwingUtilities2.drawStringUnderlineCharAt(c, g, text,
-                                                      mnemonicIndex, x, y);
+                                                      MnemonicHandler.isMnemonicHidden()
+                                                      ? -1 : mnemonicIndex,
+                                                      x, y);
         }
     }

I committed it as 7c70b20 on top of the latest version in this PR.

The diff modifies the generic SynthGraphicsUtils.paintText so that no mnemonic is passed to SwingUtilities2.drawStringUnderlineCharAt if isMnemonicHidden returns true.

This approach has a performance impact on all UI text painting. The condition can be moved into GTKGraphicsUtils so that only GTK L&F will call isMnemonicHidden.

aivanov-jdk avatar Jul 05 '24 18:07 aivanov-jdk

The diff modifies the generic SynthGraphicsUtils.paintText so that no mnemonic is passed to SwingUtilities2.drawStringUnderlineCharAt if isMnemonicHidden returns true.

This approach has a performance impact on all UI text painting. The condition can be moved into GTKGraphicsUtils so that only GTK L&F will call isMnemonicHidden.

Updated the code to handle the mnemonic for buttons as well as labels. Mnemonic hide condition check moved to GTKGraphicsUtils to avoid the performance issue.

kumarabhi006 avatar Jul 08 '24 09:07 kumarabhi006

Adding a brief details for the changes in PR as it got multiple commits and will be hard to track those changes.

Initial proposed fix was to handle the mnemonic show/hide on Alt press for GTK in Synth based classes where

  • SynthLookAndFeel class installed the AltProcessor handler after the conditional check of current L&F is GTK. public APIs setMnemonicHidden and isMnemonicHidden were added as part of fix.
  • SynthRootPaneUI class is having the AltProcessor class to handle the press and release of ALT key press.
  • SynthGraphicsUtils class is responsible for rendering the mnemonics based on the show/hide status of isMnemonicHidden flag for GTK L&F.

@prrace suggested that checking for GTK L&F is not an ideal way of implementation and we should look for other alternative mentioned here.

Based on the suggestion, AltPressProperty was added in GTKLookAndFeel to ensure the ALT key press event is handled only for GTK and thus moving the installation / uninstallation of ALTProcessor handler code in SynthRootPaneUI class. This fix required a CSR because of few public APIs which were added as a part of fix.

Later on @aivanov-jdk suggested to move the public APIs in a separate class MnemonicHandler (under sun.swing package) as the code was duplicated among Windows/Mac/Linux OSes and a separate AltProcessor class to handle the Alt key press/release for GTK and Aqua L&F (same logic was implemented for them), Windows is having it's own version of AltProcessor.

Current implementation details are:

  • Duplicate code removed, helper methods are moved to one common file (MnemonicHandler.java)
  • Common AltProcessor handler class for GTK and Aqua (AltProcessor.java)
  • Refactoring code for Windows and Aqua to access the helper methods from MnemonicHandler class and remove unused code.
  • Mnemonic hidden condition check moved to GTKGraphicsUtils so that only GTK L&F will call isMnemonicHidden to avoid the performance issue.
  • No need of CSR as sun.swing package is not public

The fix is extended for Labels and Buttons as well along with Menus for GTK L&F. Further the wild imports were expanded wherever applicable.

Automated test enhanced to test menu mnemonic behavior for Aqua L&F also. Few existing regression tests also modified to access the helper methods from MnemonicHandler class.

kumarabhi006 avatar Jul 08 '24 13:07 kumarabhi006

@kumarabhi006 Setting summary to:

Adding a brief summary for the changes in PR as it got multiple commits and will be hard to track those changes.

Initial proposed fix was to handle the **mnemonic show/hide on Alt press for GTK in Synth based classes** where

- `SynthLookAndFeel` class installed the AltProcessor handler after the conditional check of current L&F is GTK. public APIs `setMnemonicHidden and isMnemonicHidden` were added as part of fix.
- `SynthRootPaneUI` class is having the AltProcessor class to handle the press and release of ALT key press.
- `SynthGraphicsUtils` class is responsible for rendering the mnemonics based on the `show/hide status of isMnemonicHidden flag` for GTK L&F.

@prrace suggested that checking for GTK L&F is not an ideal way of implementation and we should look for other alternative mentioned [here](https://github.com/openjdk/jdk/pull/18992#discussion_r1595782003).

Based on the suggestion, `AltPressProperty` was added in `GTKLookAndFeel` to ensure the ALT key press event is handled only for GTK and thus moving the installation / uninstallation of ALTProcessor handler code in SynthRootPaneUI class.
**This fix required a CSR because of few public APIs which were added as a part of fix.**

Later on @aivanov-jdk suggested to move the **public APIs in a separate class MnemonicHandler (under sun.swing package) as the code was duplicated among Windows/Mac/Linux OSes** and a separate `AltProcessor class to handle the Alt key press/release for GTK and Aqua L&F` (same logic was implemented for them), Windows is having it's own version of AltProcessor.

Current implementation details are:

- Duplicate code removed, helper methods are moved to one common file (MnemonicHandler.java)
- Common AltProcessor handler class for GTK and Aqua (AltProcessor.java)
- Refactoring code for Windows and Aqua to access the helper methods from MnemonicHandler class and remove unused code.
- Mnemonic hidden condition check moved to `GTKGraphicsUtils` so that only GTK L&F will call `isMnemonicHidden` to avoid the performance issue.
- No need of CSR as [sun.swing package is not public](https://github.com/openjdk/jdk/pull/18992#discussion_r1654417685)

**The fix is extended for Labels and Buttons as well along with Menus for GTK L&F.**
Further the wild imports were expanded wherever applicable.

Automated test enhanced to test menu mnemonic behavior for Aqua L&F also. Few existing regression tests also modified to access the helper methods from MnemonicHandler class.

openjdk[bot] avatar Jul 08 '24 13:07 openjdk[bot]

@kumarabhi006 Setting summary to:

It's not what I meant. This summary will be included in the commit message, its purpose is to provide additional details of the changeset.

Something like this should be enough:

Hides mnemonics on menus, buttons, and labels for GTK L&F.

Moved shared code for hiding mnemonics into
sun/swing/MnemonicHandler and AltProcessor to avoid code duplication.

aivanov-jdk avatar Jul 08 '24 13:07 aivanov-jdk

/summary Hides mnemonics on menus, buttons, and labels for GTK L&F.

Moved shared code for hiding mnemonics into sun/swing/MnemonicHandler and AltProcessor to avoid code duplication.

kumarabhi006 avatar Jul 08 '24 14:07 kumarabhi006

@kumarabhi006 Setting summary to:

It's not what I meant. This summary will be included in the commit message, its purpose is to provide additional details of the changeset.

Something like this should be enough:

Hides mnemonics on menus, buttons, and labels for GTK L&F.

Moved shared code for hiding mnemonics into
sun/swing/MnemonicHandler and AltProcessor to avoid code duplication.

oh ok.. I misunderstood that. Updated now.

kumarabhi006 avatar Jul 08 '24 14:07 kumarabhi006

@kumarabhi006 Updating existing summary to:

Hides mnemonics on menus, buttons, and labels for GTK L&F.

Moved shared code for hiding mnemonics into `sun/swing/MnemonicHandler` and `AltProcessor` to avoid code duplication.

openjdk[bot] avatar Jul 08 '24 14:07 openjdk[bot]

/summary Hides mnemonics on menus, buttons, and labels for GTK L&F.

Moved shared code for hiding mnemonics into sun/swing/MnemonicHandler and AltProcessor to avoid code duplication.

The second line is too long, is it possible to break the second line as I initially suggested?

aivanov-jdk avatar Jul 08 '24 14:07 aivanov-jdk

/summary Hides mnemonics on menus, buttons, and labels for GTK L&F.

Moved shared code for hiding mnemonics into sun/swing/MnemonicHandler and AltProcessor to avoid code duplication.

kumarabhi006 avatar Jul 08 '24 14:07 kumarabhi006

@kumarabhi006 Updating existing summary to:

Hides mnemonics on menus, buttons, and labels for GTK L&F.

Moved shared code for hiding mnemonics into
sun/swing/MnemonicHandler and AltProcessor to avoid code duplication.

openjdk[bot] avatar Jul 08 '24 14:07 openjdk[bot]

/csr unneeded

The updated fix does not introduce new public API, so the CSR is no longer needed.

aivanov-jdk avatar Jul 08 '24 15:07 aivanov-jdk

@aivanov-jdk determined that a CSR request is not needed for this pull request.

openjdk[bot] avatar Jul 08 '24 15:07 openjdk[bot]

This looks like it is much better than the first iteration and being able to unify some code is good. The change looks "big" but seems to be mostly because of refactoring. I suppose you tracked down all tests that need updating ? Meaning manual as well as automated.

Yeah, many files got changed due to code refactoring . Tests which failed in CI testing due to changes are updated and working fine now. Searched for the helper method reference in other tests (manual or automated) but seems there are no more tests that refer to old implementation.

CI link is posted in JBS.

kumarabhi006 avatar Jul 11 '24 05:07 kumarabhi006