ibus / ibus

Intelligent Input Bus for Linux/Unix
https://github.com/ibus/ibus/wiki
GNU Lesser General Public License v2.1
865 stars 179 forks source link

Hangul preedit is moved with mouse click #1980

Open iyagicom opened 6 years ago

iyagicom commented 6 years ago

End of Korean Hangul character error. I can not write documents with ibus. End character error in all programs. Please fix the end character error.

ubuntu 17.10 Xorg Linux iyagi-Inspiron-7559 4.13.0-32-generic #35-Ubuntu SMP Thu Jan 25 09:13:46 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux

vokoscreen-2018-02-03_19-39-13.zip

fujiwarat commented 6 years ago

I don't know what you were typing in Korean. My understanding is your issue is that preedit text is moved by the mouse click. I think if you don't click the text, you don't see your problem.

Currently ibus-hangul commits the preedit when the focus is changed but clicking the same input context does not change the focus. Probably I think ibus clients need to send the click event. I think either cursor change event or focus out even t is too late to commit the preedit in the current cursor position.

iyagicom commented 6 years ago

This is true. "My understanding is that the preedit text is moved by the mouse click."

When writing a document, When I need to fix multiple pages and lines ibus-hangul should give up writing documentation.

This is a very old bug. I do not know what the problem is. If you can fix it with a click event, please fix it.

iyagicom commented 6 years ago

Appears when the last character is inverted. It is normal if I can not see the reversal by moving the cursor. vokoscreen-2018-02-06_18-02-42.zip

changwoo commented 6 years ago

same with #1437

This is not exactly Korean specific. It happens in all languages if they have preedit. The only difference is that Chinese/Japanese input sequences usually have explicit commit key so Chinese/Japanese users don't move cursor and/or change focus when they think preedit is not finished.

changwoo commented 6 years ago

same with #1437

and #1282

changwoo commented 6 years ago

The fundamental problem of this is, that ibus messages are very asynchronous.

  1. User clicks.
  2. Gtk just resets the input context (by calling ibus method) and changes the cursor position. The commit text, if any, is expected to be inserted at the previous position.
  3. But the ibus signal to update commit is received ASYNCHRONOUSLY after changing the cursor position.
  4. Gtk inserts the committed text in the unexpected new cursor position.

IMO such this messages should be synchronous, replying all the necessary commits/preedit updates/etc, especially if the messages can change the input status.

fujiwarat commented 6 years ago

I think this issue should be fixed with async.

changwoo commented 6 years ago

I think this issue should be fixed with async.

I have no idea how.

IMO we can't make clients completely asynchronous. GTK+, Qt and other apps sometimes assume synchronous behaviors from input method server.

iyagicom commented 6 years ago

Is not it related to this issue?

https://github.com/choehwanjin/ibus-hangul/issues/52 https://codereview.qt-project.org/#/c/212179/

iyagicom commented 6 years ago

Synchronously there is https://github.com/cogniti/nimf.

changwoo commented 6 years ago

Is not it related to this issue?

The ForwardKeyEvent message is to keep the order of non-IM key event and commit text. But it doesn't fix this problem from synchronously-written input widgets.

Synchronously there is https://github.com/cogniti/nimf.

I know this project. And no. It's not an option to use this premature and unpredictably maintained alternative.

fujiwarat commented 6 years ago

Is not it related to this issue?

No, it's a different problem.

changwoo commented 6 years ago

For minimal and compatible changes, how about adding a new "Sync" message? Clients could call Sync after ProcessKeyEvent, Reset, FocusOut, ... and then wait for its reply. So it will ensure processing of all the pending CommitText, UpdatePreeditText and/or ForwardKeyEvent before the reply.

I think it should be only for IBUS_ENABLE_SYNC_MODE=1; in the current version on this mode, the clients wait for the ProcessKeyEvent reply so adding more waits shouldn't introduce big issues.

fujiwarat commented 6 years ago

how about adding a new "Sync" message?

I won't think this option. I noted my plan in #1980#issuecomment-363319847

changwoo commented 6 years ago

Just sending and receiving click event won't fix this issue. How could clients put the commit text in the old cursor position in asynchronous way? Even if it is possible (well, by sending or storing the cursor position in gtk and put the commit text in the previous cursor position instead of the new position), I think it is to ruin abstraction to keep asynchronism.

fujiwarat commented 6 years ago

Currently the preedit info is reserved in ibus-daemon but not ibus-hangul with ibus_engine_update_preedit_text_with_mode() and probably I think the mouse event won't be delayed.

changwoo commented 6 years ago

I'm not just talking about not-moving the preedit area. Instead of moving the preedit, the expected behavior of Hangul input in this click event case is to commit the current preedit text at the old cursor position.

Even when the mouse event won't be delayed, the resulting (in case of ibus-hangul) CommitText message will be delivered to clients after cursor position change. Then clients have no choice but to insert the commit text in the unexpected new cursor position.

fujiwarat commented 6 years ago

OK, I got your point. However I think it cannot be fixed in ibus side. I.e.

Mouse click event -> application change the cursor posistion -> application sends the cursor position to IM module. Even if IM module have the old position, IM module cannot send the preedit or commit the preedit text to the old position because the application already changed the cursor position. I think the Sync and Async process does not effect this problem.

changwoo commented 6 years ago

This is exactly the issue which shouldn't occur if IM is synchronous. AFAICS in the current version of gtk3, on mouse click, GtkEntry and GtkTextView (1) reset the input context and (2) change the cursor position. So they could work in the expected way if the reset is implemented to finish all the pending commit/preedit before (2) the cursor position change, not in a fire-and-forget way.

fujiwarat commented 6 years ago

if the reset is implemented to finish all the pending commit/preedit before (2) the cursor position change

It should work with async too. Anyway I think the first implementation should be done in GTK.

changwoo commented 6 years ago

if the reset is implemented to finish all the pending commit/preedit before (2) the cursor position change

It should work with async too. Anyway I think the first implementation should be done in GTK.

It is possible, but much more difficult in an asynchronous way. The GTK input widgets' "click" handler should be splitted as two; one before reset and the other after the finish of pending commit/preedit. And even if GTK maintainers agree to make such changes for ibus, ibus should be changed first to provide a way to tell "the finish" of pending commit/preedit processing.

This is why I propose to fix this in easier and more intuitive synchronous way.

fujiwarat commented 6 years ago

I don't think to add any handlers in the ibus side. Do you have a tentative patch in the gtk3 or gtk4 side to add the reset signal? and then I can explain how to fix the ibus and ibus-hangul.

changwoo commented 6 years ago

Do you have a tentative patch in the gtk3 or gtk4 side to add the reset signal? and then I can explain how to fix the ibus and ibus-hangul.

In gtk 3.22.30, GtkTextView semms to be buggy and often crashes by repeated preedit+click. But in the first click, it always resets the GtkIMContext first and change the cursor position, and the preedit text is commited at the unexpected new cursor position.

I mean GtkTextView is already written in that way, though it is currently buggy.

changwoo commented 6 years ago

I mean GtkTextView is already written in that way, though it is currently buggy.

I stand corrected. Reset seems to be done only in some condition.

iyagicom commented 6 years ago

There is no ending character error in uim-byeoru. uim-byeoru supports Hangul like ibus-hangul. I hope ibus will solve this problem.

https://github.com/uim/uim

changwoo commented 6 years ago

There is no ending character error in uim-byeoru. uim-byeoru supports Hangul like ibus-hangul. I hope ibus will solve this problem. https://github.com/uim/uim

UIM did nothing special to resolve the so-called last-character-bug. It's because UIM is written in synchronous way. I respect the efforts to have made ibus asynchronous but I think the ibus async does little benefit compared to many usage issues.

fujiwarat commented 6 years ago

There is no ending character error in uim-byeoru.

And then my original idea to use mouse click works fine.

fujiwarat commented 6 years ago

The two patches of ibus and ibus-hangul fixes this problem.

changwoo commented 6 years ago

So the strategy is to pre-configure what to do on reset and to commit-or-clear on client side. Clever!

fujiwarat commented 6 years ago

So the strategy is to pre-configure what to do on reset and to commit-or-clear on client side. Clever!

Not sure what you mean. Anyway my patch resolve this issue without GTK changes.

changwoo commented 6 years ago

Does your patch fix this issue on gnome-shell/wayland input ("gtk_text_input")?

fujiwarat commented 6 years ago

Does your patch fix this issue on gnome-shell/wayland input ("gtk_text_input")?

No, not yet. I guess the similar patch is needed for gnome-shell/mutter.

fujiwarat commented 6 years ago

Since the current patches fix Xorg desktops only but I think this needs more testings, I postpone the fix.

choehwanjin commented 6 years ago

This problem is quite hard to solve in asynchronous mode. So I think a new approach.

Providing a method to transfer engine logic to client side.

With this feature, engine can register a rule that a client should follow. Then client can handle specific case without communicating to the engine.

More specifically, if we have something like

typedef enum {
    IBUS_CLIENT_LOGIC_COMMIT_ON_FOCUS_OUT = 1 << 0,
    IBUS_CLIENT_LOGIC_COMMIT_ON_RESET = 1 << 1,
} IBusClientLogic;

options, then client can determine what to do with preedit string synchronously.

We may add some new API:

On engine side:

ibus_engine_set_client_logic(engine,
    IBUS_CLIENT_LOGIC_COMMIT_ON_FOCUS_OUT | IBUS_CLIENT_LOGIC_COMMIT_ON_RESET);

On client side:

connect to signal IBusInputContext::set-client-logic.

If it has an option IBUS_CLIENT_LOGIC_COMMIT_ON_RESET, then client will commit preedit string immediately on reset method, i.e. synchronously.

This may break ibus design consistency, but I think it is a practical solution.

iyagicom commented 6 years ago

I am glad that it is possible asynchronously. There is a lot of hardship in hot weather. I sincerely hope that this issue will be completed successfully.

fujiwarat commented 6 years ago

We may add some new API:

I don't think a new API and ibus_engine_update_preedit_text_with_mode() would be enough to distinguish ibus-kkc.

I think my suggestion works fine and I won't think reset signal which confuses engines but mouse click events can resolve this issue.

fujiwarat commented 6 years ago

Now I succeeded to prepare all the patches for this issue:

https://github.com/fujiwarat/ibus/commits/focus-out-gtk-client2 https://github.com/fujiwarat/ibus-hangul/commits/reset-commit2 https://gitlab.gnome.org/fujiwarat/mutter/tree/ibus-korean-click https://gitlab.gnome.org/fujiwarat/gnome-shell/tree/ibus-korean-click

changwoo commented 6 years ago

Since the current patches fix Xorg desktops only but I think this needs more testings, I postpone the fix.

I think you can just go and apply the fix, starting with ibus. It shouldn't break things.

fujiwarat commented 6 years ago

@changwoo Probably I will integrate the part of ibus next week. If you will try Fedora 29 beta, I can apply the ibus patch in both upstream and Fedora 29.

fujiwarat commented 5 years ago

@changwoo I updated https://github.com/libhangul/ibus-hangul/pull/73

fujiwarat commented 5 years ago

@choehwanjin Thinking about reset signal with mouse click again, now I think probably always emmitting reset would be fine with all IMEs and IMEs could handle the signal if the preedit is committed or cleared.

I created a patch of GTK2: https://gitlab.gnome.org/fujiwarat/gtk/commits/reset-with-click

Once the patch is upstreamed, I think ibus can delete to connect the button-press-event. https://github.com/fujiwarat/ibus/commits/focus-out-gtk-client2

Note: libhangul/ibus-hangul#73 is still needed.

fujiwarat commented 5 years ago

I submitted the patch to gtk; https://gitlab.gnome.org/GNOME/gtk/issues/1534

iyagicom commented 5 years ago

gtk에 패치를 제출했다. https://gitlab.gnome.org/GNOME/gtk/issues/1534

Is it a GTK bug moving with a mouse click? Numbers and some keystrokes can not be input in the Hangul mode number pad now. I will put this issue on a new issue.

fujiwarat commented 5 years ago

@iyagicom

Is it a GTK bug moving with a mouse click?

The GTK patch is not integrated yet but the mouse click issue can be fixed when my patch of ibus-hangul is integrated.

Numbers and some keystrokes can not be input in the Hangul mode number pad now.

Wayland uses keycode instead of keysym and you need to send the correct keycode with ibus_engine_forward_key_event() and also need the release event too.

iyagicom commented 5 years ago

@iyagicom

마우스 클릭으로 움직이는 GTK 버그입니까?

GTK 패치는 아직 통합되지 않았지만 ibus-hangul 패치가 통합되면 마우스 클릭 문제가 해결 될 수 있습니다.

한글 모드 숫자 패드에는 숫자 및 일부 키 입력을 입력 할 수 없습니다.

Wayland는 keysym 대신 keycode를 사용하고 ibus_engine_forward_key_event ()를 사용하여 올바른 키 코드를 보내야하며 release 이벤트도 필요합니다.

It was confirmed that the problem of #2068 is in iBUS. I use UIM and the problem is gone.

fujiwarat commented 5 years ago

I use UIM and the problem is gone.

Probably I think you don't use Wayland and ibus-hangul works fine in Xorg.

iyagicom commented 5 years ago

I use UIM and the problem is gone.

Probably I think you don't use Wayland and ibus-hangul works fine in Xorg.

I used Wayland very briefly. I use 99.99% Xorg. The reason is that I have to play games and Nvidia supports Xorg. iBus has a lot of bugs in Xorg Now I can not use iBus because of a lot of bugs.

fujiwarat commented 5 years ago

@iyagicom OK, if you're talking about the numpad issue with QT module, #2068 has been fixed when you update in the latest qtbase.

iyagicom commented 5 years ago

iBus still has a mouse click bug. Tested at 1.5.20. vokoscreen-2019-05-28_19-18-37.zip

fujiwarat commented 5 years ago

@iyagicom Currently I fixed GTK applications but seems your application is a QT. Also you need to integrate the fix of https://github.com/libhangul/ibus-hangul/pull/68