RFR: 8089373: Translation from character to key code is not sufficient

Fri May 5 20:18:25 UTC 2023

On Thu, 4 May 2023 17:18:21 GMT, Martin Fox <duke at openjdk.org> wrote:

> Note: the Java-side changes in this PR are also in #694 which fixes the same issue (and more) on Linux. Unfortunately the Linux Robot code is not working making it difficult to test on that platform (see #718).
> 
> KeyCharacterCombinations allow the specification of accelerators based on characters whose KeyCodes vary across keyboard layouts. For example, the + character is on KeyCode.EQUALS on a U.S. English layout, KeyCode.PLUS on a German layout, and KeyCode.DIGIT1 on a Mac Swiss German layout. KeyCharacterCombinations finds the correct KeyCode by calling `Toolkit.getKeyCodeForChar`.
> 
> `getKeyCodeForChar` can only return one KeyCode for a given character so it can't easily handle characters which appear in more than one location, like + which is on the main keyboard and the numeric keypad. It's also reliant on KeyCodes which prevents KeyCharacterCombinations from working on keys with no codes (e.g. the base character contains a diacritic). It also relies on the platform to map from a character to a key which is the reverse of how key mapping normally works making it slow and/or imprecise to implement on Mac and Linux (Windows is the only platform with a system call to do this).
> 
> This PR introduces a new way for a platform to pass key information to the Java core. `View.notifyKeyEx` takes an additional platform-specific `hardwareCode` which identifies the key and is tracked in a private field in the KeyEvent. This is opt-in; a platform can continue to call the old `View.notifyKey` method and allow the `hardwareCode` to default to -1.
> 
> On the back-end `KeyCharacterCombination.match` calls the new routine `Toolkit.getKeyCanGenerateCharacter` which unpacks the KeyEvent information and sends it on to the Application. This is also opt-in; the default implementation falls back to the Application's `getKeyCodeForChar` call. Platforms which call `View.notifyKeyEx` will be handed the `hardwareCode` for the key in addition to the Java KeyCode.
> 
> The new `View.notifyKeyEx` returns a boolean indicating whether the event was consumed or not. This plays no role here but will be used later to fix [JDK-8087863](https://bugs.openjdk.org/browse/JDK-8087863).
> 
> For testing I've included the manual KeyboardTest app that also appears in PR #425. Tests with keypad combinations should now work.
> 
> Note: this PR only fixes Windows. Fixes for Mac and Linux but can't be submitted until #425 and #718 are integrated.

Not entirely sure it is good to have a hidden variable in `KeyEvent`, but looks good other than that.

modules/javafx.graphics/src/main/java/com/sun/glass/ui/win/WinApplication.java line 382:

> 380:         // This platform has migrated to getKeyCanGenerateCharacter
> 381:         // so getKeyCodeForChar will no longer be called.
> 382:         return 0;

If you are sure it is never called, then how about going all in and throwing an exception here? Currently it just returns an incorrect result.

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

> 66:      *
> 67:      * @param keyEvent The key event
> 68:      * @return true iff the event was consumed

Suggestion:

     * @return {@code true} if the event was consumed

modules/javafx.graphics/src/main/java/com/sun/javafx/tk/Toolkit.java line 709:

> 707:      * The default implementation bridges into the existing getKeyCodeForChar call.
> 708:      */
> 709:     public boolean getKeyCanGenerateCharacter(KeyEvent event, String character) {

I think this method can be narrowed a bit to accept a `KeyCode` instead of `KeyEvent`, making it bit more generally useful (and easier to test).  Also I think perhaps it can be named a bit more direct, like `canKeyGenerateCharacter`.

modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/GlassViewEventHandler.java line 169:

> 167:             WindowStage stage = scene.getWindowStage();
> 168:             Boolean consumed = Boolean.FALSE;
> 169: 

Consider not defining this variable, and just returning directly when a result has been determined.

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

> 2181: 
> 2182:     /**
> 2183:      * @return true iff the event was consumed

Suggestion:

     * @return {@code true} if the event was consumed

modules/javafx.graphics/src/main/java/javafx/scene/input/KeyEvent.java line 388:

> 386:      * The hardware key code which is private to the implementation.
> 387:      */
> 388:     private int hardwareCode;

Does it need to be private? Events are public, and I think it should be possible to make your own events that act exactly like an event created by the system, which would preclude hidden variables.

modules/javafx.graphics/src/main/native-glass/win/KeyTable.cpp line 366:

> 364:         UINT keyChar = ::MapVirtualKeyEx(UINT(hardwareCode), 2, layout);
> 365:         // Filter out dead keys
> 366:         BOOL isDead = (keyChar & 0x80000000) != 0;

Wouldn't `hardwareCode >= 0` already filter out dead keys since when the highest bit is set the value is negative?

tests/manual/events/KeyboardTest.java line 134:

> 132:     static private class KeyList extends ArrayList<KeyData> {}
> 133: 
> 134:     static private class KeyListBuilder {

minor: standard keyword order is `private static class`

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

PR Review: https://git.openjdk.org/jfx/pull/1126#pullrequestreview-1415366666
PR Review Comment: https://git.openjdk.org/jfx/pull/1126#discussion_r1186465635
PR Review Comment: https://git.openjdk.org/jfx/pull/1126#discussion_r1186464101
PR Review Comment: https://git.openjdk.org/jfx/pull/1126#discussion_r1186448517
PR Review Comment: https://git.openjdk.org/jfx/pull/1126#discussion_r1186454982
PR Review Comment: https://git.openjdk.org/jfx/pull/1126#discussion_r1186456764
PR Review Comment: https://git.openjdk.org/jfx/pull/1126#discussion_r1186458329
PR Review Comment: https://git.openjdk.org/jfx/pull/1126#discussion_r1186460449
PR Review Comment: https://git.openjdk.org/jfx/pull/1126#discussion_r1186461172