RFR: 8305418: [Linux] Replace obsolete XIM as Input Method Editor [v22]

[Martin Fox]

On Sun, 9 Jun 2024 12:56:32 GMT, Thiago Milczarek Sayao <tsayao at openjdk.org> wrote:

>> This replaces obsolete XIM and uses gtk api for IME.
>> Gtk uses [ibus](https://github.com/ibus/ibus)
>> 
>> Gtk3+ uses relative positioning (as Wayland does), so I've added a Relative positioning on `InputMethodRequest`.
>> 
>> [Screencast from 17-09-2023 21:59:04.webm](https://github.com/openjdk/jfx/assets/30704286/6c398e39-55a3-4420-86a2-beff07b549d3)
>
> Thiago Milczarek Sayao has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 96 commits:
> 
>  - Merge branch 'refs/heads/master' into new_ime
>  - Merge branch 'refs/heads/master' into new_ime
>  - Merge branch 'master' into new_ime
>  - Add signals to avoid warnings
>  - Merge branch 'master' into new_ime
>    
>    # Conflicts:
>    #	modules/javafx.graphics/src/main/native-glass/gtk/GlassApplication.cpp
>  - Add check for jview
>  - - Fix comment path
>  - - Fix double keyrelease
>  - - Use a work-around to relative positioning (until wayland is not officially supported)
>    - Unref pango attr list
>  - Merge branch 'master' into new_ime
>  - ... and 86 more: https://git.openjdk.org/jfx/compare/c8b96e4e...d6230dec

I took another look and left some inline comments.

Overall looks good to me. I've been doing some testing looking at the stream of KeyEvents and InputMethod events and it all seems to be working as expected.

modules/javafx.graphics/src/main/java/com/sun/glass/ui/gtk/GtkView.java line 123:

> 121:         if (w != null) {
> 122:             pos[0] -= (pos[0] > 0) ? ((w.getX() + getX())) : 0;
> 123:             pos[1] -= (pos[1] > 0) ? ((w.getY() + getY())) : 0;

This code will only adjust a position coordinate if it's positive. I can't easily test a dual-display Linux system but I'm pretty sure X or Y can be negative on a secondary display to the left or above the primary display. I think the conditional checks should be removed (but would appreciate a second opinion on this).

By the way, I test on a Retina display at 200% scaling and the IM positioning is working as expected.

modules/javafx.graphics/src/main/native-glass/gtk/glass_window_ime.cpp line 110:

> 108:                 slen,
> 109:                 slen,
> 110:                 NULL);

This call is expecting a byte integer so you should be passing in 0 instead of NULL.

modules/javafx.graphics/src/main/native-glass/gtk/glass_window_ime.cpp line 186:

> 184: void WindowContextBase::disableIME() {
> 185:     if (im_ctx.ctx != NULL) {
> 186:         g_signal_handlers_disconnect_matched(im_ctx.ctx, G_SIGNAL_MATCH_DATA, 0, 0, NULL, NULL, NULL);

Since you're not passing in any data this disconnect call isn't disconnecting anything. Either pass `this` in the data argument or use `g_signal_handlers_disconnect_by_data` or just remove this line completely. According to the documentation the signal handlers will be disconnected when the IM context object is destroyed (but double-check that, I'm not a GObject expert).

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

Changes requested by mfox (Committer).

PR Review: https://git.openjdk.org/jfx/pull/1080#pullrequestreview-2243000603
PR Review Comment: https://git.openjdk.org/jfx/pull/1080#discussion_r1720014105
PR Review Comment: https://git.openjdk.org/jfx/pull/1080#discussion_r1720027640
PR Review Comment: https://git.openjdk.org/jfx/pull/1080#discussion_r1720020746