RFR: 8308644: [Linux] Simplify and fix small bugs on glass key related events

Tue May 23 17:10:18 UTC 2023

On Mon, 22 May 2023 00:19:45 GMT, Thiago Milczarek Sayao <tsayao at openjdk.org> wrote:

> While working on IME I noticed this code could be simplified, so I removed the `g_hash_table` usage for a simple struct ordered for binary search (I'm sure it's ordered because I did a simple app to generate it ordered).
> 
> This also fixes:
> 
> - UNKNOWN key codes being sent on KEY_PRESS;
> - Alt Gr is now working;
> - Replaced X calls with GDK calls for key locked check for as a small step towards supporting Wayland in the future
> - Added missing mappings (for F13 - F24 and dead keys) - Linux does not send dead keys as keypress but sends as key release, and they might be used on a robot test, so it's now covered.

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

> 131:     { GDK_KEY_exclamdown, com_sun_glass_events_KeyEvent_VK_INV_EXCLAMATION },
> 132:     { GDK_KEY_EuroSign, com_sun_glass_events_KeyEvent_VK_EURO_SIGN },
> 133:     { 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?

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

> 223:     { GDK_KEY_F22, com_sun_glass_events_KeyEvent_VK_F22 },
> 224:     { GDK_KEY_F23, com_sun_glass_events_KeyEvent_VK_F23 },
> 225:     { 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.

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

> 238: };
> 239: 
> 240: 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.

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

> 308:     glass_mask |= (mask & GDK_BUTTON5_MASK) ? com_sun_glass_events_KeyEvent_MODIFIER_BUTTON_FORWARD : 0;
> 309:     glass_mask |= (mask & GDK_SUPER_MASK) ? com_sun_glass_events_KeyEvent_MODIFIER_WINDOWS : 0;
> 310:     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.

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

> 394: 
> 395:         case com_sun_glass_events_KeyEvent_VK_NUM_LOCK:
> 396:             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.

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

> 477: 
> 478:     // do not send undefined keys
> 479:     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.

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/1143#discussion_r1202594787
PR Review Comment: https://git.openjdk.org/jfx/pull/1143#discussion_r1202571877
PR Review Comment: https://git.openjdk.org/jfx/pull/1143#discussion_r1202717688
PR Review Comment: https://git.openjdk.org/jfx/pull/1143#discussion_r1202632753
PR Review Comment: https://git.openjdk.org/jfx/pull/1143#discussion_r1202725569
PR Review Comment: https://git.openjdk.org/jfx/pull/1143#discussion_r1202678777