Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8274854: Mnemonics for menu containing numeric text not working #647

Closed
wants to merge 8 commits into from

Conversation

mstr2
Copy link
Collaborator

@mstr2 mstr2 commented Oct 20, 2021

This PR fixes an issue with mnemonic parsing by removing the restriction that a mnemonic symbol must be a letter. Now, it can be any character except whitespace.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8274854: Mnemonics for menu containing numeric text not working

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jfx pull/647/head:pull/647
$ git checkout pull/647

Update a local copy of the PR:
$ git checkout pull/647
$ git pull https://git.openjdk.java.net/jfx pull/647/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 647

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jfx/pull/647.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Oct 20, 2021

👋 Welcome back mstrauss! 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.

@openjdk openjdk bot added the rfr label Oct 20, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 20, 2021

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Oct 20, 2021

/reviewers 2

@openjdk
Copy link

@openjdk openjdk bot commented Oct 20, 2021

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

label.setText("_foo _bar _qux");
label.autosize();
skin.updateDisplayedText();
assertEquals("foo _bar _qux", LabelSkinBaseShim.getText(label).getText());
Copy link
Collaborator

@hjohn hjohn Oct 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you verify here which letter / index is picked as mnemonic? (Also in the other tests).

Copy link
Collaborator Author

@mstr2 mstr2 Oct 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see an easy way to do that, and I'm not in favor of making private implementation details package-public just to test some internal state. Of course, mnemonic support should have been designed in a way that is more easily testable, but this PR is not the place to do that.

Copy link
Collaborator

@kleopatra kleopatra Oct 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the skin test, it could be tested indirectly, though not in isolation:

  • access the actual mnenomic via accessibleAttribute
  • test whether labelFor/action is working as expected when firing an alt-mnemonic onto the scene

Just noticed that there is no test of TextBinding .. that's where the correct working of the basics should be tested, shouldn't it?

Copy link
Collaborator

@hjohn hjohn Oct 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about adding a JUnit test for TextBinding instead? The class is sufficiently complicated to warrant one, and it has quite a few branches to cover. I think testing it through a Control is a bit too high level.

Something like:

TextBinding tb = new TextBinding("complicated_mnemonic__example_(s)__");

assertEquals("m", tb.getMnemonic());
assertEquals(KeyCombination.M, tb.getMnemonicKeyCombination());
assertEquals(12, tb.getMnemonicIndex());

Copy link
Collaborator

@hjohn hjohn Oct 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noticed @kleopatra that you suggested the same thing, I definitely agree that the place to test this is in a test for TextBinding.


for (int i = 0, length = s.length(); i < length; ++i) {
// Parse the input string and stop after the first mnemonic.
Copy link
Collaborator

@hjohn hjohn Oct 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to do much more than just fixing numeric mnemonics. Now it stops after the first mnemonic, whereas it picked the last mnemonic before this change (which seems to be a regression from what happened in jfx16).

I think the regression should be fixed first, then additional features can be added?

Copy link
Collaborator Author

@mstr2 mstr2 Oct 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR does not add any additional feature. It restores the behavior of jfx16, where any character is acceptable as a mnemonic, and the first mnemonic is selected.

Copy link
Collaborator

@hjohn hjohn Oct 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, the wording of the JBS ticket suggested something else, but I see it is just a regression fix.

Copy link
Member

@kevinrushforth kevinrushforth Oct 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, exactly. There were two bugs introduced with the previous fix: First, digits or other symbols are no longer recognized as mnemonics. Second, the last _ is used rather than the first. It looks like this fixes both problems.

@kleopatra
Copy link
Collaborator

@kleopatra kleopatra commented Oct 30, 2021

there is no need for the class rename, is there?

Even though it's formally internal api, I think we shouldn't do code breaking changes except if there's a very compelling reason - there are too many apps out in the wild that use internal api.

@mstr2
Copy link
Collaborator Author

@mstr2 mstr2 commented Oct 30, 2021

there is no need for the class rename, is there?

Even though it's formally internal api, I think we shouldn't do code breaking changes except if there's a very compelling reason - there are too many apps out in the wild that use internal api.

I don't know how TextBinding describes in any way what this class is doing. Naming is important, and I think MnemonicParser, on the other hand, describes exactly what is going on. I would disagree that treating internal APIs as if they were public APIs is a good place to be. Cleaning up and refactoring internal APIs is important for the long-term health of a codebase.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Oct 30, 2021

there is no need for the class rename, is there?

Even though it's formally internal api, I think we shouldn't do code breaking changes except if there's a very compelling reason - there are too many apps out in the wild that use internal api.

Well, internal interfaces aren't API. With JDK 9 - 16 it requires relaxing encapsulation to get at it. As of JDK 17, it requires an explicit export of the package to access it. Having said that, since this is a fix we are likely to want to backport to at least JavaFX 17.0.X, I think we should minimize the scope of the fix.

So I agree with @kleopatra (albeit for a different reason) that we shouldn't make this change.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Oct 30, 2021

I don't know how TextBinding describes in any way what this class is doing. Naming is important, and I think MnemonicParser, on the other hand, describes exactly what is going on.

Agreed.

I would disagree that treating internal APIs as if they were public APIs is a good place to be.

Completely agree.

Cleaning up and refactoring internal APIs is important for the long-term health of a codebase.

Yes, but I would prefer to see it as a separate fix, rather than as part of a (critical) regression bug fix.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

The latest changes look good (aside from the renaming of the class, which could be done separately). I'll look at the refactored tests and do some more testing next week.


for (int i = 0, length = s.length(); i < length; ++i) {
// Parse the input string and stop after the first mnemonic.
Copy link
Member

@kevinrushforth kevinrushforth Oct 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, exactly. There were two bugs introduced with the previous fix: First, digits or other symbols are no longer recognized as mnemonics. Second, the last _ is used rather than the first. It looks like this fixes both problems.

@@ -191,6 +191,7 @@ private void parseAndSplit(String s) {
builder.append(s.charAt(i++));
} else if (isExtendedMnemonic(s, i)) {
mnemonic = String.valueOf(s.charAt(i + 2));
mnemonicIndex = i;
Copy link
Member

@kevinrushforth kevinrushforth Oct 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. This is needed to make extended mnemonics work.

@kevinrushforth kevinrushforth self-requested a review Oct 30, 2021
Copy link
Member

@kevinrushforth kevinrushforth left a comment

The latest version looks good.

@mstr2 mstr2 requested a review from aghaisas Nov 2, 2021
@aghaisas
Copy link
Collaborator

@aghaisas aghaisas commented Nov 8, 2021

The fix and test is fine.

@openjdk
Copy link

@openjdk openjdk bot commented Nov 8, 2021

@mstr2 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:

8274854: Mnemonics for menu containing numeric text not working

Reviewed-by: aghaisas, kcr

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 23 new commits pushed to the master branch:

  • 4d8e12d: 8275723: Crash on macOS 12 in GlassRunnable::dealloc
  • 5fc047b: 8272808: Update constant collections to use the new immutable collections - leftovers
  • 7d6493b: 8275911: Keyboard doesn't show when tapping inside an iOS text input control
  • cde72c8: 8276179: PrismFontFile.isInstalledFont is dead code and should be removed
  • 6c88106: 8222455: JavaFX error loading glass.dll from cache
  • c6f4ff0: 8274699: Certain blend modes cannot be set from CSS
  • d9e1ad9: 8275848: Deprecate for removal mistakenly exposed field from class javafx.scene.shape.Box
  • 161e434: 8255015: Inconsistent illumination of 3D shape by PointLight
  • adcc40d: 8187474: Tree-/TableCell, TreeCell: editingCell/Item not updated in cell.startEdit
  • a947405: 8271091: Missing API docs in UI controls classes
  • ... and 13 more: https://git.openjdk.java.net/jfx/compare/55faac49082ee8c92893dac15d2777011ec7dca0...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@aghaisas, @kevinrushforth) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready label Nov 8, 2021
@mstr2
Copy link
Collaborator Author

@mstr2 mstr2 commented Nov 8, 2021

/integrate

@openjdk openjdk bot added the sponsor label Nov 8, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Nov 8, 2021

@mstr2
Your change (at version 5b5d720) is now ready to be sponsored by a Committer.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Nov 8, 2021

/sponsor

@openjdk
Copy link

@openjdk openjdk bot commented Nov 8, 2021

Going to push as commit 6749ab6.
Since your change was applied there have been 23 commits pushed to the master branch:

  • 4d8e12d: 8275723: Crash on macOS 12 in GlassRunnable::dealloc
  • 5fc047b: 8272808: Update constant collections to use the new immutable collections - leftovers
  • 7d6493b: 8275911: Keyboard doesn't show when tapping inside an iOS text input control
  • cde72c8: 8276179: PrismFontFile.isInstalledFont is dead code and should be removed
  • 6c88106: 8222455: JavaFX error loading glass.dll from cache
  • c6f4ff0: 8274699: Certain blend modes cannot be set from CSS
  • d9e1ad9: 8275848: Deprecate for removal mistakenly exposed field from class javafx.scene.shape.Box
  • 161e434: 8255015: Inconsistent illumination of 3D shape by PointLight
  • adcc40d: 8187474: Tree-/TableCell, TreeCell: editingCell/Item not updated in cell.startEdit
  • a947405: 8271091: Missing API docs in UI controls classes
  • ... and 13 more: https://git.openjdk.java.net/jfx/compare/55faac49082ee8c92893dac15d2777011ec7dca0...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Nov 8, 2021
@openjdk openjdk bot added integrated and removed ready rfr sponsor labels Nov 8, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Nov 8, 2021

@kevinrushforth @mstr2 Pushed as commit 6749ab6.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated
5 participants