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

[Andy Goryachev]

On Wed, 15 Dec 2021 21:30:41 GMT, Martin Fox <duke at openjdk.org> wrote:

>> 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.
>
> 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](https://bugs.openjdk.java.net/browse/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.

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

-------------

PR Comment: https://git.openjdk.org/jfx/pull/694#issuecomment-1518206571