Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8278938: [Win] Robot can target wrong key for punctuation and symbols #702

Closed
wants to merge 5 commits into from

Conversation

beldenfox
Copy link
Contributor

@beldenfox beldenfox commented Dec 23, 2021

When processing a WM_CHAR event on an OEM key (punctuation, symbol, dead key) the glass code will dynamically query the key's unshifted character to determine the Java code to assign to it. This is necessary since the relationship between OEM key codes and the characters they generate varies from layout to layout.

The Robot implementation was consulting a table which assumed a fixed relationship between Java codes and Windows key codes even for the OEM keys. The table was also missing entries for any Java code not on a US QWERTY layout, like PLUS.

In this PR if we don't find the Java code in the table or if it maps to an OEM key (which may be wrong) we sweep through all the OEM keys looking for the matching Java code.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

  • JDK-8278938: [Win] Robot can target wrong key for punctuation and symbols

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/702/head:pull/702
$ git checkout pull/702

Update a local copy of the PR:
$ git checkout pull/702
$ git pull https://git.openjdk.org/jfx.git pull/702/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 702

View PR using the GUI difftool:
$ git pr show -t 702

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/702.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Dec 23, 2021

👋 Welcome back beldenfox! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Ready for review label Dec 23, 2021
@mlbridge
Copy link

mlbridge bot commented Dec 23, 2021

Webrevs

@kevinrushforth
Copy link
Member

/reviewers 2

@kevinrushforth kevinrushforth self-requested a review January 4, 2022 14:37
@openjdk
Copy link

openjdk bot commented Jan 4, 2022

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 31, 2023

@beldenfox This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@kevinrushforth
Copy link
Member

@andy-goryachev-oracle or @jperedadnr would one of you also be able to review this?

Copy link
Collaborator

@jperedadnr jperedadnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -185,18 +185,134 @@ jint WindowsKeyToJavaKey(UINT wKey)
return com_sun_glass_events_KeyEvent_VK_UNDEFINED;
}

void JavaKeyToWindowsKey(jint jkey, UINT &vkey, UINT &modifiers)
static UINT const oemKeys[] = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update license header copyright

@@ -29,5 +29,5 @@
jint WindowsKeyToJavaKey(UINT wKey);
void JavaKeyToWindowsKey(jint jkey, UINT &vkey, UINT &modifiers);
BOOL IsExtendedKey(UINT vkey);

jint OEMCharToJavaKey(UINT ch, bool deadKey);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update license header copyright

@@ -406,65 +406,9 @@ void ViewContainer::HandleViewKeyEvent(HWND hwnd, UINT msg, WPARAM wParam, LPARA
case 0x00E2:// VK_OEM_102
if (unicodeConverted < 0) {
// Dead key
switch (wChar[0]) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update license header copyright

@beldenfox
Copy link
Contributor Author

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).

@jperedadnr
Copy link
Collaborator

Okay, makes sense. I'll approve this PR as is then, if you modify it for any reason, I'll review again.

Copy link
Collaborator

@jperedadnr jperedadnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code changes look fine to me. I do have two minor points about code style.

  • Please enclose all single-line targets of if, etc., with curly braces.
  • I also recommend (but do not insist), that the opening curly brace the target block of if, etc., be on the same line as the if.

We aren't quite as rigorous on code style in native code, but for code that isn't third-party, consistency is a good thing.

Comment on lines 209 to 210
if (oemKeys[i] == vkey)
return true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please enclose this in curly braces (or move the return true; to be on the same line as the if).

Comment on lines 286 to 287
if (jkey == com_sun_glass_events_KeyEvent_VK_UNDEFINED)
return;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same.

@kevinrushforth
Copy link
Member

kevinrushforth commented Apr 10, 2023

@beldenfox One more comment: since your branch is quite out of date, can you please merge in the latest upstream master?

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@openjdk
Copy link

openjdk bot commented Apr 11, 2023

@beldenfox This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8278938: [Win] Robot can target wrong key for punctuation and symbols

Reviewed-by: jpereda, kcr

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 2 new commits pushed to the master branch:

  • fb63b26: 8224260: ChangeListener not triggered when adding a new listener in invalidated method
  • 5395942: 8304933: BitSet (used for CSS pseudo class states) listener management is incorrect

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@jperedadnr, @kevinrushforth) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready Ready to be integrated label Apr 11, 2023
@beldenfox
Copy link
Contributor Author

/integrate

@openjdk openjdk bot added the sponsor Ready to sponsor label Apr 11, 2023
@openjdk
Copy link

openjdk bot commented Apr 11, 2023

@beldenfox
Your change (at version d606359) is now ready to be sponsored by a Committer.

@kevinrushforth
Copy link
Member

/sponsor

@openjdk
Copy link

openjdk bot commented Apr 11, 2023

Going to push as commit 3d6f328.
Since your change was applied there have been 2 commits pushed to the master branch:

  • fb63b26: 8224260: ChangeListener not triggered when adding a new listener in invalidated method
  • 5395942: 8304933: BitSet (used for CSS pseudo class states) listener management is incorrect

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Apr 11, 2023
@openjdk openjdk bot closed this Apr 11, 2023
@openjdk openjdk bot removed ready Ready to be integrated rfr Ready for review sponsor Ready to sponsor labels Apr 11, 2023
@openjdk
Copy link

openjdk bot commented Apr 11, 2023

@kevinrushforth @beldenfox Pushed as commit 3d6f328.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@beldenfox beldenfox deleted the winrobot branch April 18, 2023 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated
Development

Successfully merging this pull request may close these issues.

3 participants