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

[beldenfox]

The algorithm in KeyCharacterCombination.match relies on the call Toolkit.getKeyCodeForChar which is difficult to implement correctly. It defies the way most keyboard API’s work and no platform has got it right yet. In particular the Mac and Linux implementations have to resort to a brute-force approach which monitors keystrokes to learn the relationship between keys and characters.

This PR introduces an alternative mechanism which directly asks the platform whether a given key can generate a specific character. It also allows the platform to attach identifying key information to each KeyEvent to make it easier to answer the question (much, much easier).

This is mostly dumb plumbing. On the front-end there’s a new call View.notifyKeyEx that takes an additional platform-specific hardwareCode parameter. It also returns a boolean indicating whether the event was consumed or not so I can fix JDK-8087863. If you want to follow the path visit the files in this order:

View.java
GlassViewEventHandler.java
TKSceneListener.java
Scene.java

The KeyEvent class has been expanded with an additional hardwareCode member that can only be accessed internally. See KeyEvent.java and KeyEventHelper.java.

On the back-end KeyCharacterCombination.match calls a new routine Toolkit.getKeyCanGenerateCharacter which unpacks the KeyEvent information and sends it on to the Application. The default implementation falls back to the old getKeyCodeForChar call but platform specific Applications can send it on to the native glass code.

KeyCharacterCombination.java
Toolkit.java
QuantumToolkit.java
Application.java
GtkApplication.java

The glass code can use the hardwareCode to answer the question directly. It also has enough information to fall back on the old getKeyCodeForChar logic while also enabling the keypad (a common complaint is that Ctrl+’+’ only works on the main keyboard and not the keypad, see JDK-8090275).

This PR improves the situation for key events generated by keystrokes. Manually constructed key events won’t work any better or worse than they already do. Based on the bug database I don't think this is an issue.

---

Progress

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

Issue

• JDK-8273743: KeyCharacterCombination for "+" does not work on US QWERTY keyboard layout

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 694

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jfx/pull/694.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

• 05: Full (dc5c6367)
• 04: Full - Incremental (ddaf808c)
• 03: Full - Incremental (5cd7e9a2)
• 02: Full (bd1e4a02)
• 01: Full - Incremental (7cb6c5bc)
• 00: Full (9c5dcbcb)

[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).

[kevinrushforth]

I see from your comment in PR #635 that you propose to withdraw that PR in favor of this one. Given that, why don't you change that one to "Draft" and we'll focus on this one. As a first pass, it does seem a reasonable approach.

I do have a few high level questions:

1. What are the drawbacks of this approach?
2. I see that the new mechanism has a Linux implementation. Do you plan to propose this for the other platforms (particularly macOS)? It seems like you would need to in order to solve JDK-8090275
3. Would this PR help address any of the other in-flight PRs? (e.g., PR #672) presuming we eventually do this on all platforms)?
4. Can you add the manual test from PR #635 ?

[beldenfox]

1. No drawbacks that I can think of. With this approach the platform glass code will have enough information to recreate the original getKeyCodeForChar algorithm so it can at least maintain the existing behavior. This PR just gives it enough information to do better. For example, even in the absence of a hardwareCode the platform will be able to use the event's KeyCode to allow matching on keypad keys.
2. I've written and tested Windows and Mac implementations. They both address JDK-8090275. The Mac implementation requires more testing and uses some code from PR #425. The Windows implementation is just a tweak on the existing code to enable the keypad.
3. I'm not sure what to say about #672. It boils down to how we want to deal with KeyEvents that are created using the API vs. a physical keystroke. I'll address that in a follow-up comment.
4. I'll add the test case modified to enable testing on the keypad. It might take a day or two, there's a few other tweaks that would be useful to have.

[beldenfox]

I've decided that if this PR goes forward we should abandon #672. The changes in #672 would be bypassed for KeyEvents generated by physical keystrokes. For a while I thought it would be useful for handling KeyEvents created via the API but have decided against it.

I've been going back and forth on how to handle synthetic events created using the KeyEvent constructor i.e. events that don't have a hardwareCode attached. One option is to just route them through the old getKeyCodeForChar machinery to maintain the current level of support. Unfortunately this varies widely, from mostly working on Windows to almost never working on Mac. If we just maintain what we've got I would ditch #672 since it would be fine-tuning a feature on Windows that's largely busted on other platforms.

The other option is to dive in and make synthetic events work as well as possible on every platform. In this PR getKeyCanGenerateCharacter has a KeyCode in hand and can use the existing Robot code to map that to a physical key. It wouldn't be entirely reliable (particularly on Mac and Linux) but it would be easy and improve support for synthetic events enough to forestall bugs like JDK-8087486. And it would be behavior I could write automated and manual tests for.

I don't think synthetic events are all that useful for triggering KeyCharacterCombinations (you have to know the current keyboard layout to accurately target punctuation or symbols) but they exist and it's probably best to get them to a testable state. This means fixing the Robot code which has bugs on every platform but that was already on my to-do list and I've prototyped most of the code. I'll get the Robot bugs entered in the next few days.

[kevinrushforth]

I've decided that if this PR goes forward we should abandon #672. The changes in #672 would be bypassed for KeyEvents generated by physical keystrokes. For a while I thought it would be useful for handling KeyEvents created via the API but have decided against it.

OK, I'll focus on this one then, along with #425.

[openjdk[bot]]

@beldenfox this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout scancode
git fetch https://git.openjdk.org/jfx master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

[bridgekeeper[bot]]

@beldenfox This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 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!

[kevinrushforth]

@andy-goryachev-oracle or @jperedadnr would one of you also be able to review this?

[andy-goryachev-oracle]

@beldenfox would it be possible to make the same change to the .classpath file (though it might create a merge conflict later)?

<?xml version="1.0" encoding="UTF-8"?>
<classpath>
	<classpathentry combineaccessrules="false" kind="src" path="/base">
		<attributes>
			<attribute name="module" value="true"/>
		</attributes>
	</classpathentry>
	<classpathentry combineaccessrules="false" kind="src" path="/controls">
		<attributes>
			<attribute name="module" value="true"/>
		</attributes>
	</classpathentry>
	<classpathentry combineaccessrules="false" kind="src" path="/graphics">
		<attributes>
			<attribute name="module" value="true"/>
		</attributes>
	</classpathentry>
	<classpathentry kind="con" path="org.eclipse.jdt.launching.JRE_CONTAINER">
		<attributes>
			<attribute name="module" value="true"/>
		</attributes>
	</classpathentry>
	<classpathentry excluding=".classpath|.project|.settings" kind="src" output="bin" path=""/>
	<classpathentry kind="output" path="bin"/>
</classpath>

[andy-goryachev-oracle]

The ticket is a bit unclear when it talks about "US Layout" and Ctrl-+.

For example, on a Mac with an attached USB 101(?) key IBM keyboard, one can find two + keys. One +/=, and one on the key pad (the latter generates "Ignored: keypad code" - is this expected?)

[andy-goryachev-oracle]

On Mac, Ctrl-_/- generates "Ignored: control key" - is this expected?

[andy-goryachev-oracle]

For some reason, Command-+ on Mac did not generate console output in KeyCharComboTest (in the ticket). Using Windows+L (on the attached IBM keyboard) did generate stdout, but not Windows-+.

Am I doing something wrong?

[beldenfox]

@andy-goryachev-oracle This bug is Linux-specific and this PR only fixes Linux. It's still worth testing on Mac and Windows just to verify that they're not affected. Those platforms also have problems with KeyCharacterCombinations but I entered separate bugs, see JDK-8274967 for Windows and JDK-8087700 for Mac. I updated my comment in the Mac bug to better explain the behavior you're seeing.

Since this PR was submitted I have written the KeyboardTest app submitted as part of PR #425 which includes tests for KeyCharacterCombinations and covers a lot of keys in one go. Unfortunately that test relies on robust Robot code and right now only Windows has that (Linux is waiting on #718 and Mac on #425). It's also good to have the final Robot code in place since it's reasonable for an implementation to re-use that code when transitioning to Toolkit.getKeyCanGenerateCharacter.

The core Java code in the PR is mostly plumbing and is still worth reviewing. I needed to transition at least one platform to the new machinery to validate it but in retrospect Linux was probably not the right choice. Eventually I should close out this PR and re-submit the code with the Windows front- and back-ends in place so we can use the semi-automated KeyboardTest app instead.

BTW, when I originally started working on this I wasn't planning on getting KeyCharacterCombinations working with the numeric keypad. That's why the original bug report assumed all testing would be restricted to the main keyboard and the test app in this PR prevents testing on the keypad. But the new Toolkit call makes it (relatively) easy to fix JDK-8090275 so all that's obsolete. Sorry for the confusion.

[andy-goryachev-oracle]

@beldenfox thank you for doing this work! Should we be reviewing/integrating PRs in a specific sequence?

[beldenfox]

@andy-goryachev-oracle My highest priorities are #425 (Mac) and the Java code in this PR. I'll get it switched over to Windows as soon as possible so we have a complete implementation of the new key event pipeline to review. Third on the list is the Linux Robot code (#718) but the documentation of that API is, um, scattered so I don't expect the review to go all that quickly.

[andy-goryachev-oracle]

Thank you for clarifications, @beldenfox .

[bridgekeeper[bot]]

@beldenfox This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 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!

[beldenfox]

@andy-goryachev-oracle My apologies, I really should have cleaned this PR up weeks ago when the bridgekeeper bot flagged it.

The portion of this PR that is platform-agnostic was also submitted in PR #1126. In that PR I incorporated all of the feedback you already provided (and thank you for that). I re-submitted all of this with a Windows front- and back-end because at the time Windows was the only platform with working Robot code so the changes could be tested (the manual app is included in #1126 as well as #425). I was also hoping that the review would go faster on Windows since no one seems to have much familiarity with the Linux API's except me and @tsayao (and the documentation is wretched).

I've already done a bit of cleanup over in #1126 that conflict with the changes here (I just renamed some routines). I've kept this PR around so the Linux-specific code can be reviewed but I should probably switch it over to Draft. And the Linux Robot code needs to be reviewed and integrated first to enable testing (#718).

[tsayao]

@andy-goryachev-oracle Linux code looks correct to me.

[andy-goryachev-oracle]

The portion of this PR that is platform-agnostic was also submitted in PR #1126.

will take a look, thanks!

[bridgekeeper[bot]]

@beldenfox This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 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 8 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.