Re: RFR: 8089373: Translation from character to key code is not sufficient [v2]
On Fri, 9 Feb 2024 03:37:17 GMT, Martin Fox wrote: >> On Windows a common shortcut like Ctrl+'+' could only be invoked from the >> main keyboard and not the numeric keypad. Toolkit.getKeyCodeForChar did not >> have enough context to know whether it should return a result from the main >> keyboard or the keypad. >> >> This PR alters getKeyCodeForChar to pass in the code of the key the system >> is trying to match against. Only the Windows platform actually uses this >> additional information. >> >> On the Mac the numeric keypad has always worked due to the odd way >> getKeyCodeForChar is implemented (until PR #1209 the keypad worked more >> reliably than the main keyboard). On Linux getKeyCodeForChar is a mess; >> neither the main keyboard or the numeric keypad work reliably. I have an >> upcoming PR which should make both work correctly. > > Martin Fox has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains seven additional commits since > the last revision: > > - Consistent terminology and more details in comments. > - Merge remote-tracking branch 'upstream/master' into keypadcombo > - Added SEPARATOR to list of keypad keys > - CharacterCombinations now work on the numeric keypad > - Fixed Monocle > - Merge remote-tracking branch 'upstream/master' into keypadcombo > - Added hint to getKeyCodeForChar to enable numeric keypad Looks good to me. - Marked as reviewed by mstrauss (Committer). PR Review: https://git.openjdk.org/jfx/pull/1289#pullrequestreview-1888180397
Re: RFR: 8089373: Translation from character to key code is not sufficient [v2]
On Fri, 9 Feb 2024 03:37:17 GMT, Martin Fox wrote: >> On Windows a common shortcut like Ctrl+'+' could only be invoked from the >> main keyboard and not the numeric keypad. Toolkit.getKeyCodeForChar did not >> have enough context to know whether it should return a result from the main >> keyboard or the keypad. >> >> This PR alters getKeyCodeForChar to pass in the code of the key the system >> is trying to match against. Only the Windows platform actually uses this >> additional information. >> >> On the Mac the numeric keypad has always worked due to the odd way >> getKeyCodeForChar is implemented (until PR #1209 the keypad worked more >> reliably than the main keyboard). On Linux getKeyCodeForChar is a mess; >> neither the main keyboard or the numeric keypad work reliably. I have an >> upcoming PR which should make both work correctly. > > Martin Fox has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains seven additional commits since > the last revision: > > - Consistent terminology and more details in comments. > - Merge remote-tracking branch 'upstream/master' into keypadcombo > - Added SEPARATOR to list of keypad keys > - CharacterCombinations now work on the numeric keypad > - Fixed Monocle > - Merge remote-tracking branch 'upstream/master' into keypadcombo > - Added hint to getKeyCodeForChar to enable numeric keypad Thank you for clarification! LGTM(tm) - Marked as reviewed by angorya (Reviewer). PR Review: https://git.openjdk.org/jfx/pull/1289#pullrequestreview-1872883477
Re: RFR: 8089373: Translation from character to key code is not sufficient [v2]
On Thu, 8 Feb 2024 19:26:42 GMT, Andy Goryachev wrote: >> Martin Fox has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains seven additional >> commits since the last revision: >> >> - Consistent terminology and more details in comments. >> - Merge remote-tracking branch 'upstream/master' into keypadcombo >> - Added SEPARATOR to list of keypad keys >> - CharacterCombinations now work on the numeric keypad >> - Fixed Monocle >> - Merge remote-tracking branch 'upstream/master' into keypadcombo >> - Added hint to getKeyCodeForChar to enable numeric keypad > > tests/system/src/test/java/test/robot/com/sun/glass/ui/monocle/MonocleApplicationTest.java > line 143: > >> 141: char ch = (char) TEST_CASES[i][0]; >> 142: int expectedCode = TEST_CASES[i][1]; >> 143: int code = >> MonocleApplicationShim._getKeyCodeForChar(ch, KeyEvent.VK_UNDEFINED); > > Do you want to add a new test(s) for the cases when the hint is important? Looking through the Monocle code I see that it does support the numeric keypad and could benefit from the provided hint. I didn't find any bug in the database concerning KeyCharacterCombination support on Monocle so this may be a non-issue. If it is a problem I think it should be addressed in a separate PR at which point the test cases can be expanded. - PR Review Comment: https://git.openjdk.org/jfx/pull/1289#discussion_r1484552266
Re: RFR: 8089373: Translation from character to key code is not sufficient [v2]
On Thu, 8 Feb 2024 19:20:32 GMT, Andy Goryachev wrote: >> Martin Fox has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains seven additional >> commits since the last revision: >> >> - Consistent terminology and more details in comments. >> - Merge remote-tracking branch 'upstream/master' into keypadcombo >> - Added SEPARATOR to list of keypad keys >> - CharacterCombinations now work on the numeric keypad >> - Fixed Monocle >> - Merge remote-tracking branch 'upstream/master' into keypadcombo >> - Added hint to getKeyCodeForChar to enable numeric keypad > > modules/javafx.graphics/src/main/java/com/sun/glass/events/KeyEvent.java line > 255: > >> 253: * character with respect to the currently active keyboard layout or >> 254: * VK_UNDEFINED if the character isn't present in the current >> layout. >> 255: * The hint is the KeyCode of the key the system is attempting to >> match. > > is this clear enough or should we say that it's KeyCode.getCode()? > > (also Application:746) I updated the comment so the wording better matches the existing text. The new comment also adds a reminder that the hint could be VK_UNDEFINED. > modules/javafx.graphics/src/main/java/com/sun/glass/events/KeyEvent.java line > 257: > >> 255: * The hint is the KeyCode of the key the system is attempting to >> match. >> 256: * It can be used to optimize the search or to distinguish between >> the >> 257: * main keyboard and the numeric keypad. > > should we mention that it is supposed to be `KeyEvent.VK_UNDEFINED` when not > available? I updated the comment here as well. - PR Review Comment: https://git.openjdk.org/jfx/pull/1289#discussion_r1484541213 PR Review Comment: https://git.openjdk.org/jfx/pull/1289#discussion_r1484541702
Re: RFR: 8089373: Translation from character to key code is not sufficient [v2]
> On Windows a common shortcut like Ctrl+'+' could only be invoked from the > main keyboard and not the numeric keypad. Toolkit.getKeyCodeForChar did not > have enough context to know whether it should return a result from the main > keyboard or the keypad. > > This PR alters getKeyCodeForChar to pass in the code of the key the system is > trying to match against. Only the Windows platform actually uses this > additional information. > > On the Mac the numeric keypad has always worked due to the odd way > getKeyCodeForChar is implemented (until PR #1209 the keypad worked more > reliably than the main keyboard). On Linux getKeyCodeForChar is a mess; > neither the main keyboard or the numeric keypad work reliably. I have an > upcoming PR which should make both work correctly. Martin Fox has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains seven additional commits since the last revision: - Consistent terminology and more details in comments. - Merge remote-tracking branch 'upstream/master' into keypadcombo - Added SEPARATOR to list of keypad keys - CharacterCombinations now work on the numeric keypad - Fixed Monocle - Merge remote-tracking branch 'upstream/master' into keypadcombo - Added hint to getKeyCodeForChar to enable numeric keypad - Changes: - all: https://git.openjdk.org/jfx/pull/1289/files - new: https://git.openjdk.org/jfx/pull/1289/files/7a297aee..6400299e Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1289&range=01 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1289&range=00-01 Stats: 347637 lines in 7144 files changed: 198051 ins; 105131 del; 44455 mod Patch: https://git.openjdk.org/jfx/pull/1289.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1289/head:pull/1289 PR: https://git.openjdk.org/jfx/pull/1289
Re: RFR: 8089373: Translation from character to key code is not sufficient
On Fri, 17 Nov 2023 20:05:09 GMT, Martin Fox wrote: > On Windows a common shortcut like Ctrl+'+' could only be invoked from the > main keyboard and not the numeric keypad. Toolkit.getKeyCodeForChar did not > have enough context to know whether it should return a result from the main > keyboard or the keypad. > > This PR alters getKeyCodeForChar to pass in the code of the key the system is > trying to match against. Only the Windows platform actually uses this > additional information. > > On the Mac the numeric keypad has always worked due to the odd way > getKeyCodeForChar is implemented (until PR #1209 the keypad worked more > reliably than the main keyboard). On Linux getKeyCodeForChar is a mess; > neither the main keyboard or the numeric keypad work reliably. I have an > upcoming PR which should make both work correctly. sorry this PR was out waiting for too long. could you sync it up with the latest master please? modules/javafx.graphics/src/main/java/com/sun/glass/events/KeyEvent.java line 255: > 253: * character with respect to the currently active keyboard layout or > 254: * VK_UNDEFINED if the character isn't present in the current layout. > 255: * The hint is the KeyCode of the key the system is attempting to > match. is this clear enough or should we say that it's KeyCode.getCode()? (also Application:746) modules/javafx.graphics/src/main/java/com/sun/glass/events/KeyEvent.java line 257: > 255: * The hint is the KeyCode of the key the system is attempting to > match. > 256: * It can be used to optimize the search or to distinguish between > the > 257: * main keyboard and the numeric keypad. should we mention that it is supposed to be `KeyEvent.VK_UNDEFINED` when not available? tests/system/src/test/java/test/robot/com/sun/glass/ui/monocle/MonocleApplicationTest.java line 143: > 141: char ch = (char) TEST_CASES[i][0]; > 142: int expectedCode = TEST_CASES[i][1]; > 143: int code = MonocleApplicationShim._getKeyCodeForChar(ch, > KeyEvent.VK_UNDEFINED); Do you want to add a new test(s) for the cases when the hint is important? - PR Review: https://git.openjdk.org/jfx/pull/1289#pullrequestreview-1871084872 PR Review Comment: https://git.openjdk.org/jfx/pull/1289#discussion_r1483488977 PR Review Comment: https://git.openjdk.org/jfx/pull/1289#discussion_r1483493173 PR Review Comment: https://git.openjdk.org/jfx/pull/1289#discussion_r1483495224
Re: RFR: 8089373: Translation from character to key code is not sufficient
On Fri, 17 Nov 2023 20:05:09 GMT, Martin Fox wrote: > On Windows a common shortcut like Ctrl+'+' could only be invoked from the > main keyboard and not the numeric keypad. Toolkit.getKeyCodeForChar did not > have enough context to know whether it should return a result from the main > keyboard or the keypad. > > This PR alters getKeyCodeForChar to pass in the code of the key the system is > trying to match against. Only the Windows platform actually uses this > additional information. > > On the Mac the numeric keypad has always worked due to the odd way > getKeyCodeForChar is implemented (until PR #1209 the keypad worked more > reliably than the main keyboard). On Linux getKeyCodeForChar is a mess; > neither the main keyboard or the numeric keypad work reliably. I have an > upcoming PR which should make both work correctly. will try to review this this week... - PR Comment: https://git.openjdk.org/jfx/pull/1289#issuecomment-1915382478
Re: RFR: 8089373: Translation from character to key code is not sufficient
On Fri, 17 Nov 2023 20:05:09 GMT, Martin Fox wrote: > On Windows a common shortcut like Ctrl+'+' could only be invoked from the > main keyboard and not the numeric keypad. Toolkit.getKeyCodeForChar did not > have enough context to know whether it should return a result from the main > keyboard or the keypad. > > This PR alters getKeyCodeForChar to pass in the code of the key the system is > trying to match against. Only the Windows platform actually uses this > additional information. > > On the Mac the numeric keypad has always worked due to the odd way > getKeyCodeForChar is implemented (until PR #1209 the keypad worked more > reliably than the main keyboard). On Linux getKeyCodeForChar is a mess; > neither the main keyboard or the numeric keypad work reliably. I have an > upcoming PR which should make both work correctly. Comment to keep the bot at bay - PR Comment: https://git.openjdk.org/jfx/pull/1289#issuecomment-1897258715
Re: RFR: 8089373: Translation from character to key code is not sufficient
On Fri, 17 Nov 2023 20:05:09 GMT, Martin Fox wrote: > On Windows a common shortcut like Ctrl+'+' could only be invoked from the > main keyboard and not the numeric keypad. Toolkit.getKeyCodeForChar did not > have enough context to know whether it should return a result from the main > keyboard or the keypad. > > This PR alters getKeyCodeForChar to pass in the code of the key the system is > trying to match against. Only the Windows platform actually uses this > additional information. > > On the Mac the numeric keypad has always worked due to the odd way > getKeyCodeForChar is implemented (until PR #1209 the keypad worked more > reliably than the main keyboard). On Linux getKeyCodeForChar is a mess; > neither the main keyboard or the numeric keypad work reliably. I have an > upcoming PR which should make both work correctly. Commenting to keep the bot at bay. - PR Comment: https://git.openjdk.org/jfx/pull/1289#issuecomment-1864977054
Re: RFR: 8089373: Translation from character to key code is not sufficient [v3]
On Mon, 10 Jul 2023 16:41:47 GMT, Andy Goryachev wrote: >> At this point I don't have a strong opinion on whether this field is private >> or not. Originally I wanted it to be private because I thought of it as >> "whatever magic value makes KeyCharacterCombinations work" and was concerned >> we might want to tweak that over time. That's less of a concern now that >> I've prototyped the implementation on three different platforms. >> >> This is largely a policy issue that's above my pay grade. This is an >> intrinsically platform-specific bit of information we would be exposing to >> developers. > > Are we going to create side effects when KeyEvents are synthesized? See, for > instance, EmbeddedScene:366 or FXVKSkin:860 (and possibly other places)? A KeyEvent that doesn’t originate from Glass will not have a hardwareCode but will have a KeyCode. When matching it against a char combo the platform can just invoke the old getKeyCodeForChar logic and compare the result against the KeyEvent’s KeyCode. This is the same test that used to happen in the Java core so there should be no change in behavior. Unfortunately the current behavior is all over the map. On the Mac KeyCharacterCombinations don’t work when JavaFX is embedded in a JFXPanel (long story). On Windows and Linux they do work. I could make char combos work on the Mac but that would just lead to other bugs because AWT on the Mac does a poor job of assigning KeyCodes for punctuation and symbol keys. I will take a look at the VKFX skin but I also doubt it assigns KeyCodes “correctly” for char combos, it just doesn’t have enough information to do the job right. (In general we should only match KeyCharacterCombinations against KeyEvents that Glass generates. Otherwise we’re depending on the creator of the KeyEvent to assign the same code that Glass would assign to that key. That’s a tall order.) So there’s nothing in this PR that would make matters worse but given the current behavior I don’t know how to write tests to ensure that. - PR Review Comment: https://git.openjdk.org/jfx/pull/1126#discussion_r1261673784
Re: RFR: 8089373: Translation from character to key code is not sufficient [v3]
On Tue, 9 May 2023 17:34:23 GMT, Martin Fox wrote: >> 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. > > At this point I don't have a strong opinion on whether this field is private > or not. Originally I wanted it to be private because I thought of it as > "whatever magic value makes KeyCharacterCombinations work" and was concerned > we might want to tweak that over time. That's less of a concern now that I've > prototyped the implementation on three different platforms. > > This is largely a policy issue that's above my pay grade. This is an > intrinsically platform-specific bit of information we would be exposing to > developers. Are we going to create side effects when KeyEvents are synthesized? See, for instance, EmbeddedScene:366 or FXVKSkin:860 (and possibly other places)? - PR Review Comment: https://git.openjdk.org/jfx/pull/1126#discussion_r1258582546
Re: RFR: 8089373: Translation from character to key code is not sufficient [v3]
On Thu, 11 May 2023 02:46:47 GMT, Martin Fox 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. > > Martin Fox has updated the pull request incrementally with one additional > commit since the last revision: > > Renamed notifyKeyEx to notifyKey In the mailing list discussion the doc/ folder was intended for internal JavaFX developers and not for users of JavaFX. I was planning on updating the public JavaDocs but that means spreading the information out across the KeyCode, KeyCombination, KeyCharacterCombination, and KeyCodeCombination classes. A short overview document would be nice to have but would that be suitable for the proposed doc/ folder? I think what most developers need is a) for us to fix our bugs and b) to know that KeyCharacterCombinations should be used for accelerators involving symbols and punctuation and KeyCodeCombinations should be used for everything else. I should probably add that to the KeyCombination documentation sooner rather than later. - PR Comment: https://git.openjdk.org/jfx/pull/1126#issuecomment-1585067801
Re: RFR: 8089373: Translation from character to key code is not sufficient [v3]
On Fri, 9 Jun 2023 18:25:39 GMT, Martin Fox wrote: > App developers have a hard time sorting this out There was a discussion on the mailing list about adding a doc/ folder with markdown informational notes. (Build instructions and snapping rules were the prime candidates for such notes). If you could write such an explanatory note, we could put it there (there is no JBS issue for the doc/ folder, I'll create one). Another suggestion is that we could create an umbrella task linking all related issues, as we did, for example with skin changing [JDK-8241364](https://bugs.openjdk.org/browse/JDK-8241364). What do you think? - PR Comment: https://git.openjdk.org/jfx/pull/1126#issuecomment-1584996182
Re: RFR: 8089373: Translation from character to key code is not sufficient [v3]
On Thu, 11 May 2023 02:46:47 GMT, Martin Fox 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. > > Martin Fox has updated the pull request incrementally with one additional > commit since the last revision: > > Renamed notifyKeyEx to notifyKey I added JDK-8274967 as an issue this PR fixes. Support for `Ctrl+'+'` and `Ctrl+'-'` is erratic on Windows without this PR. Currently the platform code finds the correct key but fails to generate the correct Java KeyCode (it uses a hard-coded table that may not correspond to the current layout). With this PR it just compares the platform key code it found to the `hardwareCode` passed in. (BTW, bugs like this are under-represented in the Java bug tracking system. I was looking through the issues list for a JavaFX desktop and saw at least four bugs related to KeyCharacterCombinations spread across all three platforms. App developers have a hard time sorting this out since the behavior varies so much between platforms and layouts and the specific character being targeted.) - PR Comment: https://git.openjdk.org/jfx/pull/1126#issuecomment-1584979525
Re: RFR: 8089373: Translation from character to key code is not sufficient [v3]
> 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. Martin Fox has updated the pull request incrementally with one additional commit since the last revision: Renamed notifyKeyEx to notifyKey - Changes: - all: https://git.openjdk.org/jfx/pull/1126/files - new: https://git.openjdk.org/jfx/pull/1126/files/8c3a5c09..4afb44fe Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1126&range=02 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1126&range=01-02 Stats: 7 lines in 4 files changed: 0 ins; 0 del; 7 mod Patch: https://git.openjdk.org/jfx/pull/1126.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1126/head:pull/1126 PR: https://git.openjdk.org/jfx/pull/1126
Re: RFR: 8089373: Translation from character to key code is not sufficient
On Fri, 5 May 2023 20:20:02 GMT, John Hendrikx wrote: > > I'm not quite sold on having `notifyKey` and `notifyKeyEx` be two separate > > methods. Why not just have one? Not changing the existing call sites > > doesn't seem to be a sufficient reason to expand the toolkit API surface. > > I also think it isn't really needed; Mac and Linux can just ignore the extra > parameter for now? I now realize that there's no reason for `notifyKey` and `notifyKeyEx` to have unique names since `GetMethodID` has no problem distinguishing between Java overloads. I'll be tweaking this PR to rename `notifyKeyEx` to `notifyKey` so I won't have to get rid of the clunky name in a separate pass. Mac and Linux will migrate naturally to the new version of `notifyKey` as I fix bugs related to accelerator handling. The remaining clients would be iOS and Monocle. If we're still determined to stamp out the older version it would make sense to tweak iOS at the same time as the Mac since that's where the cross-compilation would happen. Both versions of `notifyKey` are one-liners that pass their parameters on to the same internal routine (the older version just defaults the `hardwareCode`). I'm still not convinced that it's worth the effort to mess with iOS and Monocle to make the older version go away. - PR Comment: https://git.openjdk.org/jfx/pull/1126#issuecomment-1542796058
Re: RFR: 8089373: Translation from character to key code is not sufficient [v2]
> 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. Martin Fox has updated the pull request incrementally with one additional commit since the last revision: Rename new routine to canKeyGenerateCharacter, other review cleanup - Changes: - all: https://git.openjdk.org/jfx/pull/1126/files - new: https://git.openjdk.org/jfx/pull/1126/files/a2fd0046..8c3a5c09 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1126&range=01 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1126&range=00-01 Stats: 18 lines in 9 files changed: 0 ins; 1 del; 17 mod Patch: https://git.openjdk.org/jfx/pull/1126.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1126/head:pull/1126 PR: https://git.openjdk.org/jfx/pull/1126
Re: RFR: 8089373: Translation from character to key code is not sufficient
On Tue, 9 May 2023 20:38:35 GMT, Martin Fox wrote: >> 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 & 0x8000) != 0; >> >> Wouldn't `hardwareCode >= 0` already filter out dead keys since when the >> highest bit is set the value is negative? > > I just realized that I never actually said what `hardwareCode` means on this > platform. > > A negative number means the `hardwareCode` isn't known (in other words, the > KeyEvent wasn't constructed by glass). If it's not negative it's a Windows VK > code that identifies the key the user pressed. That code doesn't reflect > whether the key is dead or not. Here I'm asking what character that key would > generate if pressed without any modifiers and ignoring it if the key is dead. I misread partially, somehow I read `keyChar` as the same as `hardwareCode`. LGTM then. - PR Review Comment: https://git.openjdk.org/jfx/pull/1126#discussion_r1189443592
Re: RFR: 8089373: Translation from character to key code is not sufficient
On Fri, 5 May 2023 20:04:12 GMT, John Hendrikx 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. > > 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 & 0x8000) != 0; > > Wouldn't `hardwareCode >= 0` already filter out dead keys since when the > highest bit is set the value is negative? I just realized that I never actually said what `hardwareCode` means on this platform. A negative number means the `hardwareCode` isn't known (in other words, the KeyEvent wasn't constructed by glass). If it's not negative it's a Windows VK code that identifies the key the user pressed. That code doesn't reflect whether the key is dead or not. Here I'm asking what character that key would generate if pressed without any modifiers and ignoring it if the key is dead. - PR Review Comment: https://git.openjdk.org/jfx/pull/1126#discussion_r1189109403
Re: RFR: 8089373: Translation from character to key code is not sufficient
On Fri, 5 May 2023 20:25:23 GMT, John Hendrikx wrote:
>> 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`.
>
> I forgot the `KeyEvent` is needed to extract the hardware code. Depending on
> whether this hardware code remains a hidden variable this may be for the best
> (or can pass both `KeyCode` and the hardware code).
I'll change the call over to `canKeyGenerateCharacter`, the longer form was
starting to annoy me too.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1126#discussion_r1188926005
Re: RFR: 8089373: Translation from character to key code is not sufficient
On Fri, 5 May 2023 20:00:55 GMT, John Hendrikx 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. > > 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. At this point I don't have a strong opinion on whether this field is private or not. Originally I wanted it to be private because I thought of it as "whatever magic value makes KeyCharacterCombinations work" and was concerned we might want to tweak that over time. That's less of a concern now that I've prototyped the implementation on three different platforms. This is largely a policy issue that's above my pay grade. This is an intrinsically platform-specific bit of information we would be exposing to developers. - PR Review Comment: https://git.openjdk.org/jfx/pull/1126#discussion_r1188927478
Re: RFR: 8089373: Translation from character to key code is not sufficient
On Fri, 5 May 2023 19:45:53 GMT, John Hendrikx 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.
>
> 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`.
I forgot the `KeyEvent` is needed to extract the hardware code. Depending on
whether this hardware code remains a hidden variable this may be for the best
(or can pass both `KeyCode` and the hardware code).
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1126#discussion_r1186474970
Re: RFR: 8089373: Translation from character to key code is not sufficient
On Fri, 5 May 2023 19:38:36 GMT, Martin Fox wrote: > I'm not quite sold on having `notifyKey` and `notifyKeyEx` be two separate > methods. Why not just have one? Not changing the existing call sites doesn't > seem to be a sufficient reason to expand the toolkit API surface. I also think it isn't really needed; Mac and Linux can just ignore the extra parameter for now? - PR Comment: https://git.openjdk.org/jfx/pull/1126#issuecomment-1536735587
Re: RFR: 8089373: Translation from character to key code is not sufficient
On Thu, 4 May 2023 17:18:21 GMT, Martin Fox 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
Re: RFR: 8089373: Translation from character to key code is not sufficient
On Fri, 5 May 2023 16:35:38 GMT, Michael Strauß wrote: > I'm not quite sold on having `notifyKey` and `notifyKeyEx` be two separate > methods. Why not just have one? Not changing the existing call sites doesn't > seem to be a sufficient reason to expand the toolkit API surface. The long term goal is to have one method. Eventually the Mac and Linux platforms will transition over, PR #694 has already been entered for Linux and bugs like [JDK-8087863](https://bugs.openjdk.org/browse/JDK-8087863) require the new method on the Mac. Once the transition is done I'll enter a bug to remove the old method and rename the new one. I thought it would be easiest to review and test one platform at a time so I kept `notifyView` around as a compatibility method. It just calls into `notifyViewEx` with a `hardwareCode` of -1. - PR Comment: https://git.openjdk.org/jfx/pull/1126#issuecomment-1536693719
Re: RFR: 8089373: Translation from character to key code is not sufficient
On Thu, 4 May 2023 17:18:21 GMT, Martin Fox 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. This will need careful review and lots of testing. - PR Comment: https://git.openjdk.org/jfx/pull/1126#issuecomment-1536515060
Re: RFR: 8089373: Translation from character to key code is not sufficient
On Thu, 4 May 2023 17:18:21 GMT, Martin Fox 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. I'm not quite sold on having `notifyKey` and `notifyKeyEx` be two separate methods. Why not just have one? Not changing the existing call sites doesn't seem to be a sufficient reason to expand the toolkit API surface. - PR Comment: https://git.openjdk.org/jfx/pull/1126#issuecomment-1536503360
