On Mon, 22 May 2023 00:19:45 GMT, Thiago Milczarek Sayao <[email protected]>
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
