DRAFT 8273743: KeyCharacterCombination for "+" does not work on US QWERTY keyboard layout

[beldenfox]

There was a mismatch between the way get_glass_key generated the Java key code for a given key press and how getKeyCodeForChar determined the key code for the matching character. For example, when pressing the plus sign on a US keyboard get_glass_key correctly generated KeyCode.EQUALS but getKeyCodeForChar("+") generated KeyCode.PLUS.

In this PR getKeyCodeForChar mirrors the behavior of get_glass_key; it determines which key the character lies on and generates a key code based on the unshifted character on the same key.

I'm working on a more comprehensive test case that allows you to press any key on the keyboard and test whether a KeyCharacterCombination for that character will succeed or not. I've attached it to this thread. It might be worth submitting as a manual test case CharComboTest.txt .

---

Progress

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

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 635

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

Using diff file

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

[bridgekeeper[bot]]

:wave: Welcome back beldenfox! 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.

[mlbridge[bot]]

Webrevs

• 03: Full - Incremental (80606863)
• 02: Full - Incremental (69d46c2e)
• 01: Full - Incremental (2e8510bf)
• 00: Full (cce46016)

[kevinrushforth]

/reviewers 2

[openjdk[bot]]

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

[pankaj-bansal]

As you recommended in the description, could you please add the attached test as part of PR as manual test which fails before the fix and passes with the fix?

[pankaj-bansal]

The changes look fine. I ran the full system test and I do not see any regression being introduced because of this change. The manual test attached fails for all keys before the change. With the change, the test passes for most of the keys, but fails for following 3 keys

• Failed: key code Comma did not match Shift+<
• Failed: key code Comma did not match Shift+<
• Failed: key code Comma did not match Shift+<
• Failed: key code Comma did not match Shift+<
• Failed: key code 0 did not match Shift+)
• Failed: key code 0 did not match Shift+)
• Failed: key code 0 did not match Shift+)
• Failed: key code 9 did not match Shift+(
• Failed: key code 9 did not match Shift+(
• Failed: key code 9 did not match Shift+(

[beldenfox]

Gdk is finding these three characters (comma, left paren, right paren) on the main keyboard using the Shift modifier and on the keypad without Shift. Presumably there are keyboards out there which have some or all of these characters on the keypad and those keys were incorporated into the US keymap (but not French, Russian, or German). Most keys on the keypad have distinct keyvals but not these, they share the same keyvals as the keys on the main keyboard.

I'll continue to investigate but this just reinforces my conviction that KeyCharacterCombination should be re-engineered. The current approach requires mapping backward from a character to a key which is difficult to do on every platform except Windows (where it's still tricky, see #672) and the requirement that each character maps to just one key is inherently limiting.

[beldenfox]

I can map from a character to a key but I haven’t found any way of verifying that the key is actually on the user’s keyboard. I can’t even filter them out as keypad keys, their XkbKeyTypeIndex puts them in the same class as the Space bar.

I’ve amended the PR to approach this the same way the Mac does. As the user types we query the key to determine which characters it can generate and stash them in a map. In getKeyCodeForChar we consult that map to answer the question.

This means both the Mac and Linux would be using the same brute force hack to get around the limitations of getKeyCodeForChar. I think it’s time for it to go. I’ve entered an alternate PR #694 that paves the way for retiring that call and replacing it with one that’s much easier to get right on all platforms. I would prefer to close this PR and focus on that one. Life is short.

[beldenfox]

I marked this as draft in hopes that #694 goes through instead. The review cycles were useful as very similar code is in #694 and will be needed to address JDK-8278924.

[bridgekeeper[bot]]

@beldenfox This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[bridgekeeper[bot]]

@beldenfox This pull request has been inactive for more than 16 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.