On Thu, 4 May 2023 17:18:21 GMT, Martin Fox <d...@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
