-
Notifications
You must be signed in to change notification settings - Fork 437
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
base: master
Are you sure you want to change the base?
Conversation
👋 Welcome back mfox! A progress list of the required criteria for merging this PR into |
Webrevs
|
/reviewers 2 |
@kevinrushforth |
❗ This change is not yet ready to be integrated. |
@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! |
Comment added to keep the bots at bay. |
@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! |
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. |
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? |
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.
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. |
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
Issue
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