search

libhangul / ibus-hangul

The hangul engine for IBus
GNU General Public License v2.0
66 stars 18 forks source link

Use synchornized process key event API for gtk4 #113

Closed epico closed 2 years ago

epico commented 2 years ago

For gtk4 immodule, use synchornized process key event API for gtk4, and avoid to use forward key event API.

fujiwarat commented 2 years ago

lgtm

choehwanjin commented 2 years ago

Please let me know why this patch is required. A specific explanation would be appreciated.

epico commented 2 years ago

Actually there is some issue with forward key event in gtk4 input method module.

For now, we used synchornized process key event API in gtk4. With this synchornized API, we don't use the forward key event API in gtk4.

The IBUS_CAP_SYNC_PROCESS_KEY enum means the gtk4 client will use synchornized process key event API, and please don't use the forward key event API in gtk4.

Maybe @fujiwarat can give more details about this patch.

choehwanjin commented 2 years ago

If use_event_forwarding option is disabled, wrong input order problem(#42) will occur again. Please test 'rk1', this should be '가1' not '1가'

ibus has a problem with synchronous mode. See the comment in https://github.com/libhangul/ibus-hangul/blob/66b5ec4a0973347f3528370806b16c1bfaf56cdd/src/engine.c#L1537

epico commented 2 years ago

This patch only affects gtk4 client, only ibus-gtk4 will use the IBUS_CAP_SYNC_PROCESS_KEY enum value currently.

I think Takao Fujiwara uses synced process key event for ibus-gtk4. With synced process key event and without forward key event, it works fine with the gtk4 client like gnome-text-editor.

Maybe you can try Fedora 36 with the patch.

URL: https://copr.fedorainfracloud.org/coprs/fujiwara/ibus

epico commented 2 years ago

We plan to include this ibus-hangul patch in Fedora 37 for broader testing.

bsjeon commented 2 years ago

For the run-time ibus version check, it seems good to use ibus_hangul_check_ibus_version(1, 5, 27) instead of IBUS_CHECK_VERSION(1, 5, 27).

fujiwarat commented 2 years ago

GTK4 no longer supports to forward non-ASCII keys. If you type "a" + Enter key, the Enter key won't be committed with GTK_IM_MODULE=ibus and GTK4 applications.

epico commented 2 years ago

For the run-time ibus version check, it seems good to use ibus_hangul_check_ibus_version(1, 5, 27) instead of IBUS_CHECK_VERSION(1, 5, 27).

Sorry, the run-time check is handled by IBUS_CAP_SYNC_PROCESS_KEY.

With ibus 1.5.27, we can still use forward key event if the IBUS_CAP_SYNC_PROCESS_KEY enum is not set. For example, gtk3 client will not set the IBUS_CAP_SYNC_PROCESS_KEY enum.

The IBUS_CHECK_VERSION(1, 5, 27) macro plans to fix compile issue with ibus.

bsjeon commented 2 years ago

For the run-time ibus version check, it seems good to use ibus_hangul_check_ibus_version(1, 5, 27) instead of IBUS_CHECK_VERSION(1, 5, 27).

Sorry, the run-time check is handled by IBUS_CAP_SYNC_PROCESS_KEY.

The IBUS_CHECK_VERSION(1, 5, 27) macro plans to fix compile issue with ibus.

I agree :)

Nevertheless, I prefer to use IBUS_CHECK_VERSION() only for C preprocessor. Wouldn't it be better to have explicit code than to be implicitly correct? By the way, with the previous version of ibus 1.5.27, it seems that ibus-hangul build issue will occur. What about the combination of IBUS_CHECK_VERSION(), ibus_hangul_check_ibus_version(), and g_warning()?

In fact, I don't have any qualifications for this PR. I just hope this PR will be more discussed because it is a very useful approach.

With ibus 1.5.27, we can still use forward key event if the IBUS_CAP_SYNC_PROCESS_KEY enum is not set. For example, gtk3 client will not set the IBUS_CAP_SYNC_PROCESS_KEY enum.

Because the '_use_sync_mode' default value is zero in the gtk3 ibus im module, it seems to set the IBUS_CAP_SYNC_PROCESS_KEY enum.

https://github.com/ibus/ibus/blob/1.5.27/client/gtk2/ibusimcontext.c#L120 https://github.com/ibus/ibus/blob/1.5.27/client/gtk2/ibusimcontext.c#L987

But, there are no #42, #109 issues! And it looks more good. In use_event_forwarding == true, hangul_keyboard '2' and hangul input state, the following warning message is printed in the gtk3 client "rk"+Enter key or "rkrk"+three Backspace keys input.

(gedit:386275): Gdk-WARNING **: 21:43:02.746: Event with type 8 not holding a GdkDevice. It is most likely synthesized outside Gdk/GTK+

By this PR, these bunch warning message disappears.

Another test case:

In fact, at this point, my perception of the situation has been confused.

Do we really need the ibus-hangul "use-event-forwarding" workaround for the gtk client (with GTK_IM_MODULE=ibus)? Do we really need the "forward-key-event" processing patch on the GUI toolkit?

https://github.com/libhangul/ibus-hangul/blob/master/src/engine.c#L1537 This comment seems reasonable.

However, client/x11/main.c and client/gtk2/ibusimcontext.c seems to be preparing for this problem by using ibus_input_context_process_key_event_async(), ibus_input_context_process_key_event_async_finish().

If it's working properly, is there a problem with the x11 side? Otherwise, is the GTK something smartly re-arranging?

fujiwarat commented 2 years ago

Probably we won't change the GTK3 async mode for a while to keep the back compatibility because Java + GTK applications have a problem with the async mode. Please focus on the GTK4 issue at the moment. We have implemented the new hybrid async mode for GTK4 since IBus 1.5.27 and I'm not sure if the new async mode can fix the Java issue.

Right. the warning of GTK3 forwarding keys is a known issue but the functionality is no problem.

bsjeon commented 2 years ago

Probably we won't change the GTK3 async mode for a while to keep the back compatibility because Java + GTK applications have a problem with the async mode. Please focus on the GTK4 issue at the moment. We have implemented the new hybrid async mode for GTK4 since IBus 1.5.27 and I'm not sure if the new async mode can fix the Java issue.

Right. the warning of GTK3 forwarding keys is a known issue but the functionality is no problem.

https://github.com/ibus/ibus/blob/1.5.27/client/x11/main.c#L490 If this works properly, ibus-hangul does not need 'use-event-forwarding' workaround.

IMHO, maybe this is an event loop problem. On x11 side, ibus_input_context_process_key_event_async(), ibus_input_context_process_key_event_async_finish() are not working as intended.

I have applied the ibus commit c957c5f approach to ibus client/x11 side. There are no #42 issue without ibus-hangul 'use-event-forwarding' workaround.

fujiwarat commented 2 years ago

I think use-forward-event-forwarding is needed for GTK3 applications in ibus-hangul with the async mode in GNOME Wayland/Xorg and it's not #42.

epico commented 2 years ago

Sorry, this patch is intended to fix issue in gtk4 client.

The x11 and gtk3 clients will work as before.

bsjeon commented 2 years ago

Unfortunately, there was something ridiculous misunderstood, so the previous post are withdrawn. Sorry for making noise.

If the ibus-hangul 'use-event-forwarding == false' condition is satisfied, the #109 issue does not occur regardless of each "IBUS_ENABLE_SYNC_MODE" value (0, 1, 2). It was tested on the Gtk4 4.6.7, Wayland, Gnome 42.4, Archlinux.

Is this intended?

fujiwarat commented 2 years ago

If the ibus-hangul 'use-event-forwarding == false' condition is satisfied,

As I explained in #113#issuecomment-1236070059 , 'use-event-forwarding == false' is not enough. Are you the ibus-hangul maintainer? You have missed the point of this PR.

bsjeon commented 2 years ago

Sorry. I tried to focus on gtk4 (sync_mode == 2), but I was forced to go off-topic. Because it is related to ibus-hangul use-event-forwarding, I think it is not a complete off-topic.

I think use-forward-event-forwarding is needed for GTK3 applications in ibus-hangul with the async mode in GNOME Wayland/Xorg and it's not #42.

Are there any examples? In my analysis, use_event_forwarding is only needed for ibus-xim sync_mode==1 .

Probably we won't change the GTK3 async mode for a while to keep the back compatibility because Java + GTK applications have a problem with the async mode. Please focus on the GTK4 issue at the moment. We have implemented the new hybrid async mode for GTK4 since IBus 1.5.27 and I'm not sure if the new async mode can fix the Java issue.

https://github.com/ibus/ibus/issues/1713#issuecomment-104882187

Are there other issues that require this fix? I'm sure you've changed it after a lot of review, but I'm not sure this was the right solution. Even if it is difficult, I think it should have been verified with a small test code. Also I think the ecere-sdk developer could have found another solution by modifying their code.

After this fix, the ibus-hangul use-event-forwarding workaround continues to be an issue. It is enough to finish processing the input event according to the return value of something_process_key_event(). Additional forwarding-key-event handling is another workaround code. I think stubborn developers will reject this. They would think this is an ibus-hangul bug.

If you decide to keep this fix, how about introducing something like IBUS_CAP_SOMETHING only when ibus-xim sync-mode==1 condition?

bsjeon commented 2 years ago

If the ibus-hangul 'use-event-forwarding == false' condition is satisfied,

As I explained in #113#issuecomment-1236070059 , 'use-event-forwarding == false' is not enough. Are you the ibus-hangul maintainer? You have missed the point of this PR.

GTK4 no longer supports to forward non-ASCII keys. If you type "a" + Enter key, the Enter key won't be committed with GTK_IM_MODULE=ibus and GTK4 applications.

Sorry. I was writing a post, so I couldn't check new posts. This is the result of testing it. The test results seem different from mine, so I'll check more.

bsjeon commented 2 years ago

It's a test screen recording on "Wayland, GTK_IM_MODULE=ibus". Same results in Xorg.

https://user-images.githubusercontent.com/765207/189009698-bd7c42fa-4669-4052-a393-360865176869.mp4

I'm not a member of the ibus-hangul; I just posted my thoughts because I want the ibus to have good improvements.

fujiwarat commented 2 years ago

It's a test screen recording on "Wayland, GTK_IM_MODULE=ibus". Same results in Xorg.

IBUS_ENABLE_SYNC_MODE should be set to the client applications but not ibus-daemon and also GTK_IM_MODULE should be "ibus".

OK, probably I need to explain the implementation with detail of this PR.

  1. the current sync(IBUS_ENABLE_SYNC_MODE=1) is needed in IBus X11 IM module(XMODIFIERS=@im=ibus) because of Java + GTK application back compatibility - https://github.com/ibus/ibus/issues/1713
  2. forward-key-event is needed in ibus-hangul for the current GTK2/GTK3 async(IBUS_ENABLE_SYNC_MODE=0) . The test case is the output order of "a " in GNOME Wayland/Xorg
  3. the current async(IBUS_ENABLE_SYNC_MODE=0) does not work in ibus-hangul for IBus GTK4 IM module(GTK_IM_MODULE=ibus) because forward-key-event no longer supports non-ASCII keys. The test case is "a" + Enter in GNOME Xorg(GTK_IM_MODULE=ibus). And GNOME upstream also expects not to use the current async from GTK4.
  4. the current sync(IBUS_ENABLE_SYNC_MODE=1) also does not work in ibus-hangul. The test case is the output order of "a " in GNOME Wayland/Xorg.
  5. the new hybrid async(IBUS_ENABLE_SYNC_MODE=2) works in ibus-hangul with IBus GTK4 IM module(GTK_IM_MODULE=ibus). The test case is the output order of "a " in GNOME Wayland/Xorg
  6. forward-key-event needs to be disabled with the new async(IBUS_ENABLE_SYNC_MODE=2) in ibus-hangul for IBus GTK4 IM module(GTK_IM_MODULE=ibus) because non ASCII keys are no longer supported. The test case is "a" + Enter in GNOME Xorg(GTK_IM_MODULE=ibus)
  7. ibus-hangul needs to know which IBUS_ENABLE_SYNC_MODE=2 or IBUS_ENABLE_SYNC_MODE=0 is used and the ibus capability flag is added.
fujiwarat commented 2 years ago

Sorry I mistook 1 above and fix it now.

bsjeon commented 2 years ago

Thank you for the kind explanation. I feel as if I was hit by my head! Embarrassed, but I'll keep my wrong posts.

Sorry for wasting your time.

Updated: Hiding my wrong comments. I hope it doesn't interfere with the PR process.

bsjeon commented 2 years ago

I made two table of test results. The output order issue of "a"+Enter is included in #42. ibus/ibus#1713 has not been tested by me yet.

2. forward-key-event is needed in ibus-hangul for the current GTK2/GTK3 async(IBUS_ENABLE_SYNC_MODE=0) . The test case is the output order of "a " in GNOME Wayland/Xorg

The sync-mode=0 test failed to reproduce this issue. #42 seems to be a characteristic of sync-mode=1 only. I've checked it several times, but I may have made a mistake again.

im-ibus.so: sync-mode forward-key-event GTK2 GTK3 GTK4
0 0 #109
0 1 #109
1 0 #42 #42 #42
1 1 #109
2 0
2 1 #109

ibus-xim:

sync-mode forward-key-event GTK2 GTK3
0 0 ibus/ibus#1713 ibus/ibus#1713
0 1 ibus/ibus#1713 ibus/ibus#1713
1 0 #42 #42
1 1
choehwanjin commented 2 years ago

If the wrong function return order problem of ibus is solved(ibus c957c5f), then there is no need to use 'use-event-forwarding' workaround. I will merge this.

We plan to include this ibus-hangul patch in Fedora 37 for broader testing.

@epico Please notify me about the testing result, when it is done.

epico commented 2 years ago

Thanks for the review! We already included this patch in Fedora 37.

I will tell you the test result when we get the feed back.

fujiwarat commented 2 years ago
  1. forward-key-event is needed in ibus-hangul for the current GTK2/GTK3 async(IBUS_ENABLE_SYNC_MODE=0) . The test case is the output order of "a " in GNOME Wayland/Xorg

The sync-mode=0 test failed to reproduce this issue. https://github.com/libhangul/ibus-hangul/issues/42 seems to be a characteristic of sync-mode=1 only.

Hmm.., IBUS_ENABLE_SYNC_MODE=0 forward-key-event=0 IBUS_GTK_IM_MODULE=ibus in GTK2/3 had a problem AFAIR. I'm not sure the current status. If you believe there is no problem in case of IBUS_ENABLE_SYNC_MODE=0 , probably ibus capability IBUS_CAP_SYNC_PROCESS_KEY could be set for IBUS_ENABLE_SYNC_MODE=1 since ibus-xim must be IBUS_ENABLE_SYNC_MODE=1.

bsjeon commented 2 years ago

Hmm.., IBUS_ENABLE_SYNC_MODE=0 forward-key-event=0 IBUS_GTK_IM_MODULE=ibus in GTK2/3 had a problem AFAIR. I'm not sure the current status.

Thanks for the comment. There may be other rare issue, or #42 issue in unusual situations.

If you believe there is no problem in case of IBUS_ENABLE_SYNC_MODE=0 , probably ibus capability IBUS_CAP_SYNC_PROCESS_KEY could be set for IBUS_ENABLE_SYNC_MODE=1 since ibus-xim must be IBUS_ENABLE_SYNC_MODE=1.

I thought wrong. ibus/ibus#1713 cannot be ignored.

I hope to introduce hybrid-async mode to ibus-xim. ibus_input_context_process_key_event() behaves differently than I expected. I applied ibus/ibus@c957c5f6 to ibus-xim and it works fine without #42 issue. I believe ibus-xim hybrid-async mode will also solve the 1713.

If hybrid-async mode becomes the default for ibus-xim( or ibus), use-event-forwarding remains only a backwards compatibility feature in ibus-hangul.

fujiwarat commented 2 years ago

I hope to introduce hybrid-async mode to ibus-xim.

I'm fine with it. It would be good to report the issue or PR in ibus/ibus not to forget the request for ibus 1.5.28. The current phase is the bug fix and we will update ibus after Fedora 37 GA.

Currently IBUS_CAP_SYNC_PROCESS_KEY is set if the sync mode is not 1 in https://github.com/ibus/ibus/blob/main/client/gtk2/ibusimcontext.c#L987 And ibus-hangul uses forward-key-event if IBUS_CAP_SYNC_PROCESS_KEY is not set.

From your test results table, I think the correct condition is:

IBUS_CAP_SYNC_PROCESS_KEY is set if the sync mode is 1 in ibus-gtk and ibus-xim And ibus-hangul uses forward-key-event if IBUS_CAP_SYNC_PROCESS_KEY is set.

But I'm not sure if we could correct the condition because we export GTK_IM_MODULE=wayland in GNOME Wayland and it also uses ibus-xim and the conditions are more complicated.