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

[Andy Goryachev]

On Tue, 21 Feb 2023 17:09:32 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.
>
> Martin Fox has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits:
> 
>  - Merge remote-tracking branch 'upstream/master' into scancode
>  - Added manual test for KeyCharacterCombination matching
>  - New KeyCharacterCombination implementation

Changes requested by angorya (Committer).

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?

modules/javafx.graphics/src/main/java/com/sun/javafx/tk/TKSceneListener.java line 67:

> 65:      * Pass a key event to the scene to handle
> 66:      */
> 67:     public boolean keyEvent(KeyEvent keyEvent);

could we update javadoc to explain when it returns true?

modules/javafx.graphics/src/main/java/javafx/scene/Scene.java line 2178:

> 2176:     }
> 2177: 
> 2178:     boolean processKeyEvent(KeyEvent e) {

would be nice to have a one-line javadoc explaining when it returns true

modules/javafx.graphics/src/main/native-glass/gtk/glass_key.cpp line 433:

> 431: 
> 432:     if (hardwareCode < 0) {
> 433:         if (vkCode != com_sun_glass_events_KeyEvent_VK_UNDEFINED)

please add curly braces

modules/javafx.graphics/src/main/native-glass/gtk/glass_key.cpp line 462:

> 460:             }
> 461:             if (allAreZero)
> 462:                 searchGroup = 0;

{

tests/manual/events/KeyCharacterCombinationTest.java line 86:

> 84:     {
> 85:         if (down)
> 86:             return KeyCombination.ModifierValue.DOWN;

{

tests/manual/events/KeyCharacterCombinationTest.java line 151:

> 149:             // Replace 'invisible' characters with their names.
> 150:             if (!combinationDescription.isEmpty())
> 151:                 combinationDescription = combinationDescription.substring(0, combinationDescription.length() - 1);

{

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

PR Review: https://git.openjdk.org/jfx/pull/694#pullrequestreview-1396267822
PR Comment: https://git.openjdk.org/jfx/pull/694#issuecomment-1518242039
PR Review Comment: https://git.openjdk.org/jfx/pull/694#discussion_r1174082802
PR Review Comment: https://git.openjdk.org/jfx/pull/694#discussion_r1174078238
PR Review Comment: https://git.openjdk.org/jfx/pull/694#discussion_r1174080814
PR Review Comment: https://git.openjdk.org/jfx/pull/694#discussion_r1174080930
PR Review Comment: https://git.openjdk.org/jfx/pull/694#discussion_r1174081157
PR Review Comment: https://git.openjdk.org/jfx/pull/694#discussion_r1174081471