On Fri, 7 Apr 2023 18:28:28 GMT, Martin Fox <d...@openjdk.org> wrote:
>> PR looks good and works fine on Windows 11. >> >> I've tested the test attached to the JBS issue with a Spanish keyboard. It >> fails without the patch for `+`, `'`, `ยก`, and passes with it. >> >> There is a NPE if you press AltGr (with or without the patch): >> >> Exception in thread "JavaFX Application Thread" >> java.lang.NullPointerException: keyCode must not be null >> at java.base/java.util.Objects.requireNonNull(Objects.java:233) >> at >> javafx.graphics@21-internal/javafx.scene.robot.Robot.keyPress(Robot.java:92) >> at >> RobotKeySanityTest.lambda$userReleasedEvent$0(RobotKeySanityTest.java:121) >> ``` >> >> I wonder if RobotKeySanityTest could be part of the PR as a manual test? >> Same as in #694 > >> I wonder if RobotKeySanityTest could be part of the PR as a manual test? >> Same as in #694 > > The RobotKeySanityTest is tricky, it requires the user to press a lot of keys > to get good coverage but can produce confusing results if the user presses > certain keys, like AltGr. Windows treats AltGr as if you pressed Ctrl and Alt > at the same time. This means the OS sends two events when you press the key > and two more when you release it. This is confusing the RobotKeySanityTest > which can only handle one press/release pair at a time. I'm not sure I can > fix the test but will think about it. > > The KeyboardTest app that I recently added to PR #425 covers most of the same > ground and avoids the problematic keys. The downside of KeyboardTest is that > it only provides good coverage for specific layouts (I'll try to add Spanish) > and provides the best coverage when run against multiple layouts which takes > effort to set up. > > I've written an app that just logs key events as the user types and that > would probably be a better candidate to attach to some PR. When something > confusing happens with RobotKeySanityTest or KeyboardTest that's the app I > pull up to diagnose what's going on (like I just did with AltGr). @beldenfox One more comment: since your branch is quite out of date, can you please merge in the latest upstream master? ------------- PR Comment: https://git.openjdk.org/jfx/pull/702#issuecomment-1502426741
