[jfx17u] RFR: 8274854: Mnemonics for menu containing numeric text not working
Clean backport to `jfx17u`. - Commit messages: - 8274854: Mnemonics for menu containing numeric text not working Changes: https://git.openjdk.java.net/jfx17u/pull/19/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx17u=19=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8274854 Stats: 194 lines in 3 files changed: 162 ins; 22 del; 10 mod Patch: https://git.openjdk.java.net/jfx17u/pull/19.diff Fetch: git fetch https://git.openjdk.java.net/jfx17u pull/19/head:pull/19 PR: https://git.openjdk.java.net/jfx17u/pull/19
Re: RFR: 8274854: Mnemonics for menu containing numeric text not working [v4]
On Sat, 30 Oct 2021 20:40:12 GMT, Michael Strauß wrote: >> 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. > > Michael Strauß has updated the pull request incrementally with four > additional commits since the last revision: > > - revert rename > - revert rename > - test for KeyCombination > - renamed TextBinding to MnemonicParser, refactored tests Marked as reviewed by aghaisas (Reviewer). The fix and test is fine. - PR: https://git.openjdk.java.net/jfx/pull/647
Re: RFR: 8274854: Mnemonics for menu containing numeric text not working [v4]
On Sat, 30 Oct 2021 20:40:12 GMT, Michael Strauß wrote: >> 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. > > Michael Strauß has updated the pull request incrementally with four > additional commits since the last revision: > > - revert rename > - revert rename > - test for KeyCombination > - renamed TextBinding to MnemonicParser, refactored tests The latest version looks good. - Marked as reviewed by kcr (Lead). PR: https://git.openjdk.java.net/jfx/pull/647
Re: RFR: 8274854: Mnemonics for menu containing numeric text not working [v3]
On Fri, 29 Oct 2021 22:18:58 GMT, John Hendrikx wrote: >> 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. > > Okay, the wording of the JBS ticket suggested something else, but I see it is > just a regression fix. 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. - PR: https://git.openjdk.java.net/jfx/pull/647
Re: RFR: 8274854: Mnemonics for menu containing numeric text not working [v3]
On Sat, 30 Oct 2021 14:18:42 GMT, Michael Strauß wrote: > 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. > 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. - PR: https://git.openjdk.java.net/jfx/pull/647
Re: RFR: 8274854: Mnemonics for menu containing numeric text not working [v3]
On Fri, 29 Oct 2021 13:58:35 GMT, Michael Strauß wrote: >> 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. > > Michael Strauß has updated the pull request incrementally with one additional > commit since the last revision: > > changed javadoc 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. - PR: https://git.openjdk.java.net/jfx/pull/647
Re: RFR: 8274854: Mnemonics for menu containing numeric text not working [v4]
On Sat, 30 Oct 2021 14:28:09 GMT, Kevin Rushforth wrote:
>> Michael Strauß has updated the pull request incrementally with four
>> additional commits since the last revision:
>>
>> - revert rename
>> - revert rename
>> - test for KeyCombination
>> - renamed TextBinding to MnemonicParser, refactored tests
>
> modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/behavior/MnemonicParser.java
> line 63:
>
>> 61: *
>> 62: */
>> 63: public class MnemonicParser {
>
> As mentioned in the comments, I prefer this rename to be reverted. It could
> be done separately as cleanup.
I created [JDK-8276206](https://bugs.openjdk.java.net/browse/JDK-8276206) to
track this.
-
PR: https://git.openjdk.java.net/jfx/pull/647
Re: RFR: 8274854: Mnemonics for menu containing numeric text not working [v4]
On Sat, 30 Oct 2021 14:49:24 GMT, Michael Strauß wrote:
>> 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.
>
> Michael Strauß has updated the pull request incrementally with four
> additional commits since the last revision:
>
> - revert rename
> - revert rename
> - test for KeyCombination
> - renamed TextBinding to MnemonicParser, refactored tests
modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/behavior/MnemonicParser.java
line 63:
> 61: *
> 62: */
> 63: public class MnemonicParser {
As mentioned in the comments, I prefer this rename to be reverted. It could be
done separately as cleanup.
modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/behavior/MnemonicParser.java
line 194:
> 192: } else if (isExtendedMnemonic(s, i)) {
> 193: mnemonic = String.valueOf(s.charAt(i + 2));
> 194: mnemonicIndex = i;
Good catch. This is needed to make extended mnemonics work.
-
PR: https://git.openjdk.java.net/jfx/pull/647
Re: RFR: 8274854: Mnemonics for menu containing numeric text not working [v3]
On Fri, 29 Oct 2021 13:58:35 GMT, Michael Strauß wrote: >> 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. > > Michael Strauß has updated the pull request incrementally with one additional > commit since the last revision: > > changed javadoc 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. - PR: https://git.openjdk.java.net/jfx/pull/647
Re: RFR: 8274854: Mnemonics for menu containing numeric text not working [v4]
> 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. Michael Strauß has updated the pull request incrementally with four additional commits since the last revision: - revert rename - revert rename - test for KeyCombination - renamed TextBinding to MnemonicParser, refactored tests - Changes: - all: https://git.openjdk.java.net/jfx/pull/647/files - new: https://git.openjdk.java.net/jfx/pull/647/files/c4012056..5b5d720d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=647=03 - incr: https://webrevs.openjdk.java.net/?repo=jfx=647=02-03 Stats: 224 lines in 3 files changed: 146 ins; 72 del; 6 mod Patch: https://git.openjdk.java.net/jfx/pull/647.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/647/head:pull/647 PR: https://git.openjdk.java.net/jfx/pull/647
Re: RFR: 8274854: Mnemonics for menu containing numeric text not working [v3]
On Sat, 30 Oct 2021 14:12:17 GMT, Jeanette Winzenburg wrote: > 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. - PR: https://git.openjdk.java.net/jfx/pull/647
Re: RFR: 8274854: Mnemonics for menu containing numeric text not working [v3]
On Sat, 30 Oct 2021 09:46:13 GMT, Jeanette Winzenburg wrote: >> 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. > > 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? Just noticed @kleopatra that you suggested the same thing, I definitely agree that the place to test this is in a test for `TextBinding`. - PR: https://git.openjdk.java.net/jfx/pull/647
Re: RFR: 8274854: Mnemonics for menu containing numeric text not working [v3]
On Sat, 30 Oct 2021 09:46:13 GMT, Jeanette Winzenburg
wrote:
>> 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.
>
> 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?
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());
-
PR: https://git.openjdk.java.net/jfx/pull/647
Re: RFR: 8274854: Mnemonics for menu containing numeric text not working [v3]
On Fri, 29 Oct 2021 23:14:07 GMT, Michael Strauß wrote:
>> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/LabelSkinTest.java
>> line 2162:
>>
>>> 2160: label.autosize();
>>> 2161: skin.updateDisplayedText();
>>> 2162: assertEquals("foo _bar _qux",
>>> LabelSkinBaseShim.getText(label).getText());
>>
>> Can you verify here which letter / index is picked as mnemonic? (Also in the
>> other tests).
>
> 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.
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
-
PR: https://git.openjdk.java.net/jfx/pull/647
Re: RFR: 8274854: Mnemonics for menu containing numeric text not working [v3]
On Fri, 29 Oct 2021 20:19:02 GMT, John Hendrikx wrote:
>> Michael Strauß has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> changed javadoc
>
> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/LabelSkinTest.java
> line 2162:
>
>> 2160: label.autosize();
>> 2161: skin.updateDisplayedText();
>> 2162: assertEquals("foo _bar _qux",
>> LabelSkinBaseShim.getText(label).getText());
>
> Can you verify here which letter / index is picked as mnemonic? (Also in the
> other tests).
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.
-
PR: https://git.openjdk.java.net/jfx/pull/647
Re: RFR: 8274854: Mnemonics for menu containing numeric text not working [v3]
On Fri, 29 Oct 2021 20:54:04 GMT, Michael Strauß wrote: >> modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/behavior/TextBinding.java >> line 188: >> >>> 186: int i = 0; >>> 187: >>> 188: // Parse the input string and stop after the first mnemonic. >> >> 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? > > 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. Okay, the wording of the JBS ticket suggested something else, but I see it is just a regression fix. - PR: https://git.openjdk.java.net/jfx/pull/647
Re: RFR: 8274854: Mnemonics for menu containing numeric text not working [v3]
On Fri, 29 Oct 2021 20:26:56 GMT, John Hendrikx wrote: >> Michael Strauß has updated the pull request incrementally with one >> additional commit since the last revision: >> >> changed javadoc > > modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/behavior/TextBinding.java > line 188: > >> 186: int i = 0; >> 187: >> 188: // Parse the input string and stop after the first mnemonic. > > 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? 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. - PR: https://git.openjdk.java.net/jfx/pull/647
Re: RFR: 8274854: Mnemonics for menu containing numeric text not working [v3]
On Fri, 29 Oct 2021 13:58:35 GMT, Michael Strauß wrote: >> 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. > > Michael Strauß has updated the pull request incrementally with one additional > commit since the last revision: > > changed javadoc modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/behavior/TextBinding.java line 188: > 186: int i = 0; > 187: > 188: // Parse the input string and stop after the first mnemonic. 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? - PR: https://git.openjdk.java.net/jfx/pull/647
Re: RFR: 8274854: Mnemonics for menu containing numeric text not working [v3]
On Fri, 29 Oct 2021 13:58:35 GMT, Michael Strauß wrote:
>> 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.
>
> Michael Strauß has updated the pull request incrementally with one additional
> commit since the last revision:
>
> changed javadoc
modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/LabelSkinTest.java
line 2162:
> 2160: label.autosize();
> 2161: skin.updateDisplayedText();
> 2162: assertEquals("foo _bar _qux",
> LabelSkinBaseShim.getText(label).getText());
Can you verify here which letter / index is picked as mnemonic? (Also in the
other tests).
-
PR: https://git.openjdk.java.net/jfx/pull/647
Re: RFR: 8274854: Mnemonics for menu containing numeric text not working [v3]
On Fri, 29 Oct 2021 13:58:35 GMT, Michael Strauß wrote: >> 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. > > Michael Strauß has updated the pull request incrementally with one additional > commit since the last revision: > > changed javadoc Marked as reviewed by aghaisas (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/647
Re: RFR: 8274854: Mnemonics for menu containing numeric text not working [v3]
On Fri, 29 Oct 2021 13:45:24 GMT, Ajit Ghaisas wrote: >> Done. > > The corresponding comment for the method isSimpleMnemonic() also needs > correction. > You can limit it to - > > /** > * Determines whether the string contains a simple mnemonic at the > specified position. > * A simple mnemonic is any two-character string similar to "_x", where x > is not an > * underscore or a whitespace character. > */ Done. - PR: https://git.openjdk.java.net/jfx/pull/647
Re: RFR: 8274854: Mnemonics for menu containing numeric text not working [v3]
> 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. Michael Strauß has updated the pull request incrementally with one additional commit since the last revision: changed javadoc - Changes: - all: https://git.openjdk.java.net/jfx/pull/647/files - new: https://git.openjdk.java.net/jfx/pull/647/files/af670b07..c4012056 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=647=02 - incr: https://webrevs.openjdk.java.net/?repo=jfx=647=01-02 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/647.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/647/head:pull/647 PR: https://git.openjdk.java.net/jfx/pull/647
Re: RFR: 8274854: Mnemonics for menu containing numeric text not working [v2]
On Fri, 29 Oct 2021 12:34:50 GMT, Michael Strauß wrote: >> modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/behavior/TextBinding.java >> line 238: >> >>> 236: } >>> 237: >>> 238: return !isExtendedMnemonic(s, position); >> >> I am not sure why do we need to check for isExtendedMnemonic() inside >> isSimpleMnemonic() implementation. >> These two methods should be separate and it is up to the callers to check >> and take appropriate action. > > Done. The corresponding comment for the method isSimpleMnemonic() also needs correction. You can limit it to - /** * Determines whether the string contains a simple mnemonic at the specified position. * A simple mnemonic is any two-character string similar to "_x", where x is not an * underscore or a whitespace character. */ - PR: https://git.openjdk.java.net/jfx/pull/647
Re: RFR: 8274854: Mnemonics for menu containing numeric text not working [v2]
On Fri, 29 Oct 2021 11:23:50 GMT, Ajit Ghaisas wrote: >> Michael Strauß has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Removed isExtendedMnemonic check from isSimpleMnemonic > > modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/behavior/TextBinding.java > line 238: > >> 236: } >> 237: >> 238: return !isExtendedMnemonic(s, position); > > I am not sure why do we need to check for isExtendedMnemonic() inside > isSimpleMnemonic() implementation. > These two methods should be separate and it is up to the callers to check and > take appropriate action. Done. - PR: https://git.openjdk.java.net/jfx/pull/647
Re: RFR: 8274854: Mnemonics for menu containing numeric text not working [v2]
> 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. Michael Strauß has updated the pull request incrementally with one additional commit since the last revision: Removed isExtendedMnemonic check from isSimpleMnemonic - Changes: - all: https://git.openjdk.java.net/jfx/pull/647/files - new: https://git.openjdk.java.net/jfx/pull/647/files/7cc78d4a..af670b07 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=647=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=647=00-01 Stats: 18 lines in 1 file changed: 5 ins; 9 del; 4 mod Patch: https://git.openjdk.java.net/jfx/pull/647.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/647/head:pull/647 PR: https://git.openjdk.java.net/jfx/pull/647
Re: RFR: 8274854: Mnemonics for menu containing numeric text not working
On Wed, 20 Oct 2021 16:54:35 GMT, Michael Strauß wrote: > 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. modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/behavior/TextBinding.java line 238: > 236: } > 237: > 238: return !isExtendedMnemonic(s, position); I am not sure why do we need to check for isExtendedMnemonic() inside isSimpleMnemonic() implementation. These two methods should be separate and it is up to the callers to check and take appropriate action. - PR: https://git.openjdk.java.net/jfx/pull/647
RFR: 8274854: Mnemonics for menu containing numeric text not working
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. - Commit messages: - fixed mnemonic parsing - failing test Changes: https://git.openjdk.java.net/jfx/pull/647/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx=647=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8274854 Stats: 80 lines in 2 files changed: 72 ins; 1 del; 7 mod Patch: https://git.openjdk.java.net/jfx/pull/647.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/647/head:pull/647 PR: https://git.openjdk.java.net/jfx/pull/647
