On Tue, 23 May 2023 16:59:10 GMT, Martin Fox <[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 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.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1143#discussion_r1203112225
