On Tue, 23 May 2023 22:43:11 GMT, Thiago Milczarek Sayao <[email protected]>
wrote:
>> 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.
>
> 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 bash to hash table and added the missing dead keys mapping.
>> 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.
>
> 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.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1143#discussion_r1204130650
PR Review Comment: https://git.openjdk.org/jfx/pull/1143#discussion_r1204129794
