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 [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 [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&pr=647&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=647&range=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
