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

8273743: KeyCharacterCombination for "+" does not work on US QWERTY keyboard layout #1373

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

beldenfox
Copy link
Contributor

@beldenfox beldenfox commented Feb 20, 2024

On Linux getKeyCodeForChar does not consult the current keyboard layout. For example, it assumes the “+” character is on KeyCode.PLUS even on layouts which don’t generate KeyCode.PLUS. The result is that most KeyCharacterCombinations don’t work.

This PR fixes this using the same approach that Mac glass uses. When the user types a key we lookup all the characters that key might generate and put them in a map. getKeyCodeForChar then consults the map to find the Java key code. This ensures that we only pay attention to keys that are on the user’s physical keyboard.

(Some Linux layout maps are expansive and include entries for keys that may be on the user’s keyboard but probably aren’t, like “(“ and “)” on the numeric keypad. If we simply ask for all entries that generate a given character we can get multiple hits with no good way to determine which ones are physically present.)

This PR also contains fixes to the Robot to enable testing. When Glass consults the GdkKeymap to determine which keys might generate a keyval GDK returns a list covering every installed layout and shift level. The old Glass code simply picked the first entry in the list. This PR iterates through the list looking for one that works for the current layout and correct shift level.


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-8273743: KeyCharacterCombination for "+" does not work on US QWERTY keyboard layout (Bug - P3)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1373

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

Using diff file

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

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Feb 20, 2024

👋 Welcome back mfox! 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 Feb 20, 2024
@mlbridge
Copy link

mlbridge bot commented Feb 20, 2024

Webrevs

@kevinrushforth
Copy link
Member

/reviewers 2

@openjdk
Copy link

openjdk bot commented Feb 21, 2024

@kevinrushforth
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

@openjdk
Copy link

openjdk bot commented Mar 13, 2024

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

@bridgekeeper
Copy link

bridgekeeper bot commented Apr 10, 2024

@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!

@beldenfox
Copy link
Contributor Author

Comment added to keep the bots at bay.

@bridgekeeper
Copy link

bridgekeeper bot commented May 9, 2024

@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!

@beldenfox
Copy link
Contributor Author

I've updated to the latest master. This will probably be the last time I refresh this pull request; I can only hold off the bots for so long.

@andy-goryachev-oracle
Copy link
Contributor

In the context of the recent discussion on the mailing list, would it be possible to actually detect the keyboard layout? There is a limited number of layouts, so in theory that should be possible.

Another question - I recall on Windows there is a way to enter certain key using Alt codes. Does this method exist on Linux?

@beldenfox
Copy link
Contributor Author

In the context of the recent discussion on the mailing list, would it be possible to actually detect the keyboard layout?

I answered this question in full on the mailing list but will provide a summary here. There's no documented way of doing this on Windows. The Mac organizes layouts based on language (for example, English), Linux organizes them based on country (so Australia, Great Britain, and the U.S. are all distinct). On all platforms there are more layouts than you think (the Mac has 5 German layouts and Linux has 20 Germany layouts) and there's no standard on how the variants are designated.

Another question - I recall on Windows there is a way to enter certain key using Alt codes. Does this method exist on Linux?

I'm not that familiar with this feature but here's my understanding. The feature isn't universal but tied to the input method system you're using. With IBus you can press Shift+Ctrl+U followed by the Unicode value followed by Enter. But if you want to use an IME with JavaFX you have to use the obsolete XIM framework instead which doesn't support this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rfr Ready for review
3 participants