8308644: [Linux] Missing mappings for dead keys + Alt Gr by tsayao · Pull Request #1143 · openjdk/jfx

tsayao · 2023-05-22T00:19:45Z

This PR adds missing key mappings for dead keys and fixes "Alt Gr" for some systems.

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-8308644: [Linux] Missing mappings for dead keys + Alt Gr (Bug - P4)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1143

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2023-05-22T00:19:57Z

👋 Welcome back tsayao! 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.

tsayao · 2023-05-23T11:44:42Z

@beldenfox AKA master of keys :)

Could you take a look?

mlbridge · 2023-05-23T15:23:48Z

Webrevs

beldenfox · 2023-05-23T15:42:18Z

modules/javafx.graphics/src/main/native-glass/gtk/glass_key.cpp

+    { GDK_KEY_F21, com_sun_glass_events_KeyEvent_VK_F21 },
+    { GDK_KEY_F22, com_sun_glass_events_KeyEvent_VK_F22 },
+    { GDK_KEY_F23, com_sun_glass_events_KeyEvent_VK_F23 },
+    { GDK_KEY_F24, com_sun_glass_events_KeyEvent_VK_F24 },


F13 on up used to be in the mapping tables on both Mac and Windows but were commented out long ago. This happened before everything was migrated to git and there's no bug number or comment in the code that explains why this happened. I would just leave them out.

beldenfox · 2023-05-23T15:49:31Z

modules/javafx.graphics/src/main/native-glass/gtk/glass_key.cpp

+    { GDK_KEY_braceright, com_sun_glass_events_KeyEvent_VK_BRACERIGHT },
+    { GDK_KEY_exclamdown, com_sun_glass_events_KeyEvent_VK_INV_EXCLAMATION },
+    { GDK_KEY_EuroSign, com_sun_glass_events_KeyEvent_VK_EURO_SIGN },
+    { GDK_KEY_ISO_Level3_Shift, com_sun_glass_events_KeyEvent_VK_ALT_GRAPH },


The old mapping (which you kept) is to map GDK Alt_R to VK_ALT_GRAPH. That seemed to be working fine. Is this a different key? How would I go about generating this keysym?

It was not working for me, so I did a little search and sometimes it's mapped do ISO_Level3:

xmodmap: up to 4 keys per modifier, (keycodes in parentheses): shift Shift_L (0x32), Shift_R (0x3e) lock Caps_Lock (0x42) control Control_L (0x25), Control_R (0x69) mod1 Alt_L (0x40), Meta_L (0xcd) mod2 Num_Lock (0x4d) mod3 mod4 Super_L (0x85), Super_R (0x86), Super_L (0xce), Hyper_L (0xcf) mod5 ISO_Level3_Shift (0x5c), Mode_switch (0xcb)

beldenfox · 2023-05-23T16:08:01Z

modules/javafx.graphics/src/main/native-glass/gtk/glass_key.cpp

    glass_mask |= (mask & GDK_BUTTON5_MASK) ? com_sun_glass_events_KeyEvent_MODIFIER_BUTTON_FORWARD : 0;
-    glass_mask |= (mask & GDK_SUPER_MASK) ? com_sun_glass_events_KeyEvent_MODIFIER_WINDOWS : 0; // XXX: is this OK?
+    glass_mask |= (mask & GDK_SUPER_MASK) ? com_sun_glass_events_KeyEvent_MODIFIER_WINDOWS : 0;
+    glass_mask |= (mask & GDK_META_MASK) ? com_sun_glass_events_KeyEvent_MODIFIER_WINDOWS : 0;


Have you been able to generate the Super and Meta modifiers? Googling around it seems the original keys were on the 1978 Space-cadet keyboard and there seems to be some debate as to how these modifiers should be emulated on a modern keyboard. I wouldn't change this unless a user has entered a bug.

Yeah, they seem unused. This is mostly used on other events modifiers, like mouse dragging and probably will never touch GDK_META_MASK.

beldenfox · 2023-05-23T16:30:38Z

modules/javafx.graphics/src/main/native-glass/gtk/glass_window.cpp

+    }
+
+    // do not send undefined keys
+    if (glassKey > 0) {


I haven't run this code but this looks wrong. There are many keys which don't have key codes including most characters with accents or other diacritic marks (like ñ on a Spanish layout). These have always generated PRESSED and RELEASED events with undefined key codes.

It was wrong, fixed.

beldenfox · 2023-05-23T16:59:10Z

modules/javafx.graphics/src/main/native-glass/gtk/glass_key.cpp

+    { GDK_KEY_Delete, com_sun_glass_events_KeyEvent_VK_DELETE }
+};
+
+jint gdk_keyval_to_glass(guint keyval) {


It's almost always best to leave working code as-is. The Contributing document contains this line:

Note that it is unlikely the project will merge refactors for the sake of refactoring.

I don't find a hard-coded table and custom binary search algorithm to be simpler than a hash map, particularly if contributors are responsible for keeping the table in correct order. In any case the old code was working fine and I would leave it alone.

I'm still glad you did this work, it's good to revisit detail-oriented code like this every now and then looking for things that were overlooked.

I was not sure about that, It does generate the hash table on the first key press, but performance-wise I don't think there are a significant difference. I like the binary search better, but without further arguments/reasons I think it's hard to convince the reviewer.

Reverted back to hash table and added the missing dead keys mapping.

beldenfox · 2023-05-23T17:05:13Z

modules/javafx.graphics/src/main/native-glass/gtk/glass_key.cpp

-        } else {
-            return com_sun_glass_events_KeyEvent_KEY_LOCK_OFF;
-        }
+            return (gdk_keymap_get_num_lock_state(keymap))


Have you tried testing this with a Wayland backend? I was doing some testing and it seemed that unless JavaFX opened a window we didn't connect with the Wayland server so we couldn't get accurate keyboard layout information. It wouldn't surprise me if this also prevented us from getting accurate keyboard state so the manual test for this code (which does not create a window) might fail. Beyond that I agree we should be migrating away from X11 calls if possible.

I have attached a key_release.c on the JBS bug, it works better on Wayland. On Xorg when you release the CAPS LOCK it will report as still ON until you press another key.

Compile with gcc -o key_release key_release.c pkg-config --cflags --libs gtk+-3.0

I reverted back for now since the CAPS LOCK key release behaviour was not ok.

tsayao · 2023-05-24T13:31:56Z

Before (when pressing dead keys):

After:

tsayao · 2023-06-22T11:45:29Z

Changed the scope of the PR to a minimum more "mergeable" approach.

bridgekeeper · 2023-07-20T17:11:06Z

@tsayao 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 · 2023-08-03T19:02:07Z

I'm surprised no customer ever noticed this. On Linux the current JavaFX behavior is that the real AltGr key (the one that appears on European and UK keyboards) is assigned KeyCode.UNDEFINED. On layouts like US English the right Alt key is just an Alt key but we sill assign it KeyCode.ALT_GRAPH. So, yeah, we should be adding ISO_Level3_Shift to the map.

The Robot digs through the hash map to do a reverse lookup from KeyCode to keysym. I'll add AltGr to my keyboard test app and sort out how this affects the Robot. I'm not too worried about making matters worse, as it stands the Robot can't send an ALT_GRAPH key event on layouts that actually use AltGr.

The dead key additions are harmless and make the system look more professional but those codes only show up on RELEASED KeyEvents so I suspect they will be ignored. Windows doesn't even generate RELEASED events for dead keys. The Mac does but the codes are wrong. But, again, I don't see any downside to adding them.

bridgekeeper · 2023-09-01T02:55:12Z

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

tsayao · 2023-09-03T18:10:16Z

It's a simple addition, with no hurries, just commenting to keep it on the list.

bridgekeeper · 2023-10-01T18:19:29Z

@tsayao 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 · 2023-10-27T16:17:57Z

/reviewers 2

openjdk · 2023-10-27T16:20:02Z

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

bridgekeeper · 2023-11-24T16:23:49Z

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

bridgekeeper · 2023-12-22T19:36:58Z

@tsayao This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

beldenfox · 2023-12-22T20:51:13Z

My apologies, I have been reviewing this but didn't get around to writing up my comments.

Currently JavaFX assumes that AltGr will be reported as Alt_R. On ISO keyboards (including U.K. English) AltGr is reported as ISO_Level3_Shift. On these keyboards ISO_Level3_Shift is also the only keyval that works if we want to target AltGr using a Robot. Just adding a second entry to the table isn't sufficient to fix that, currently the Robot will only attempt one code and at the moment it's Alt_R. I'll add AltGr to the KeyboardTest app, make sure ISO_Level3_Shift is in the table, and update the Robot code as part of fixing JDK-8278924.

I do want the dead key changes in at some point. There's no reason for us to misidentify keys when the changes are this easy and adding them to the table will enable testing of dead key handling using a Robot.

tsayao added 3 commits May 21, 2023 21:05

Replace g_hash_map for a binary search

e00a720

Add note

e2f2b43

Remove unused preprocessor

1b7a318

tsayao changed the title ~~Simplify GDK key to Glass key in glass-gtk code~~ Simplify GDK key to Glass key lookup in glass-gtk code May 22, 2023

tsayao added 7 commits May 21, 2023 21:53

Add missing mappings

2cce988

Remove duplicated entry

a3a8af1

Map Alt Gr key which is named GDK_KEY_ISO_Level3_Shift

f569c32

GDK_META_MASK is not ALT

78e129c

Avoid sending unknown key event

0e26b3a

Use GDK instead of X calls (to ease in a future wayland implementation)

46f64be

Remove print statement

f1bf9f4

tsayao changed the title ~~Simplify GDK key to Glass key lookup in glass-gtk code~~ 8308644: [Linux] Simplify and fix small bugs on glass key related events May 23, 2023

tsayao marked this pull request as ready for review May 23, 2023 11:41

openjdk bot added the rfr Ready for review label May 23, 2023

beldenfox reviewed May 23, 2023

View reviewed changes

tsayao added 4 commits May 23, 2023 19:51

Revert modifier change + Remove F13 -> F24

f7a9f6f

Revert key lock change because it has different behaviour

49165bf

Back to hash

7213644

Forgot test code

0696816

tsayao added 2 commits June 22, 2023 08:36

Merge branch 'master' into gdk_to_glass_key_map

2ffe6fe

Minimize changes

aad021a

tsayao changed the title ~~8308644: [Linux] Simplify and fix small bugs on glass key related events~~ 8308644: [Linux] Missing mappings for dead keys + Alt Gr Jun 22, 2023

bridgekeeper bot closed this Dec 22, 2023

tsayao deleted the gdk_to_glass_key_map branch December 22, 2023 23:22

Conversation

tsayao commented May 22, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented May 22, 2023

Uh oh!

tsayao commented May 23, 2023

Uh oh!

mlbridge bot commented May 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsayao May 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsayao May 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsayao commented May 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tsayao commented Jun 22, 2023

Uh oh!

bridgekeeper bot commented Jul 20, 2023

Uh oh!

beldenfox commented Aug 3, 2023

Uh oh!

bridgekeeper bot commented Sep 1, 2023

Uh oh!

tsayao commented Sep 3, 2023

Uh oh!

bridgekeeper bot commented Oct 1, 2023

Uh oh!

kevinrushforth commented Oct 27, 2023

Uh oh!

openjdk bot commented Oct 27, 2023

Uh oh!

bridgekeeper bot commented Nov 24, 2023

Uh oh!

bridgekeeper bot commented Dec 22, 2023

Uh oh!

beldenfox commented Dec 22, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

tsayao commented May 22, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented May 23, 2023 •

edited

Loading

tsayao May 24, 2023 •

edited

Loading

tsayao May 23, 2023 •

edited

Loading

tsayao commented May 24, 2023 •

edited

Loading