Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

m17n:sa:itrans move the input cursor to the head #2536

Open
itt533 opened this issue Jul 16, 2023 · 51 comments
Open

m17n:sa:itrans move the input cursor to the head #2536

itt533 opened this issue Jul 16, 2023 · 51 comments

Comments

@itt533
Copy link

itt533 commented Jul 16, 2023

Please fill in the following items if you don't know the root cause.

Which distribution and version?:
Slackware 15.0, 64bit

Which desktop environment and version?:
Xfce 4.16

Which session type?:
X11

Which application and version?:
anything that take text input. Here Mousepad 0.5.8 is taken

IBus version?:
1.5.25

Issue description:
when issuing some text in a language script containing ligatures like Indic scripts with Ibus, Ibus displays the text gradually as it is typed, in a temporary field that the program in which it is typed (here Mousepad) does not see. That temporary text is then validated with either a key stroke that makes it complete with regards to the corresponding language grammar, or a space stroke or an arrow. By looking carefully at the text during the moment Ibus validates and passes that text to Mousepad, one can see that Ibus makes that text blinking once and the cursor looks like going backward by one text unit and move forward. When typing for just about 3~4 lines it can really become annoying. It's like giving a kick in the screen at every syllable. Off course it means that the code in Ibus is not proper.

Emacs has its own foreign input system. I have made a screencast comparing Ibus and Emacs. One could easily see that typing in Emacs looks perfectly smooth.

here is

Steps to reproduce:

  1. enable Ibus, and add 'sanskrit - itrans' or 'telugu - itrans' if not yet there
  2. open Mousepad, or Kate, or whatever GUI text editor (even Firefox is a good test bed)
  3. select that script mentioned in step 1
  4. type 'bhA space' it should output 'भा' if 'sanskrit - itrans', and 'భా' if 'telugu - itrans'
  5. repeat the step 4 many times by hitting space in between. It is clear that the text blinks once and the cursor disappears and reappears. Compare with Emacs foreign input system. (C-x C-m C-\ prompts for an input method, type 'deva' + tab tab and choose 'devanagari-itrans' or type 'te-itans' tab and Enter for telugu)

here is the screencast: http://0x0.st/Hjtu.mkv

Can you reproduce your problem when you restart ibus-daemon? (yes / no):
(Run ibus exit and ibus-daemon --xim &)
YES

Do you see any errors when you run ibus-daemon with the verbose option?:
(Run ibus-daemon --xim --verbose & and look at the output when you encounter your problem.)
NO

Can you reproduce your problem with a new user account instead of the current your account? (yes / no):
YES

@fujiwarat
Copy link
Member

I could find "te-itrans" IBus M17N engine and tried your step #5 but cannot reproduce your issue with mousepad and gedit.
I don't know which IBus component includes the "sanskrit - itrans" engine.
You could run ibus engine command to get the current engine name.

here is the screencast: http://0x0.st/Hjtu.mkv

I don't notice any issues from the screencast and you type several characters besides 'भा'.
I'd ask you to take the screenshot of your step #5 only to show your issue clearly,

@mike-fabian
Copy link

I don't know which IBus component includes the "sanskrit - itrans" engine.

This one:

$ ibus list-engine | grep sa-itrans
  m17n:sa:itrans - sa-itrans (m17n)

@mike-fabian
Copy link

mike-fabian commented Jul 20, 2023

Upload video converted to mp4 here for convenience:

(@itt533: github does not support .mkv but you can convert to .mp4 using ffmpeg -i Hjtu.mkv -codec copy Hjtu.mp4)

Hjtu.mp4

@mike-fabian
Copy link

@itt533: Are you doing this using ibus-m17n or ibus-typing-booster?

@mike-fabian
Copy link

@itt533 Apparently you are using ibus-m17n.

@fujiwarat
Copy link
Member

OK, I don't see any issue with m17n:sa:itrans.
I'd ask you to take the screenshot of your step #5 only to show your issue clearly,

@mike-fabian
Copy link

Maybe it is easier to see in this video (high framerate 60 fps):

Peek.2023-07-21.15-52.mp4

I am typing bhA using ibus-m17n with sa-itrans.
Each time the preedit gets an update, the cursor jumps to the left of the preedit, the updated preedit is displayed and the cursor jumps to the right again. This looks like a very quick flicker. Depending on which system I test on, this is sometimes very hard to see and on some systems I cannot see it at all. For example I cannot see the flicker in a Fedora 38 virtual machine but I can see the flicker on a real Fedora 38 machine.

I tried whether other input methods show such a flicker when the preedit is updated and could not find any. As far as I can tell,

  • ibus-typing-booster
  • ibus-hangul
  • ibus-anthy
  • ibus-libpinyin

never show any flicker when the preedit is updated.

Maybe ibus-m17n updates the preedit in an unusual way?

Anyway, it doesn't look like a very serious problem to me and apparently one can avoid it by using ibus-typing-booster instead of ibus-m17n.

@fujiwarat
Copy link
Member

Each time the preedit gets an update, the cursor jumps to the left of the preedit, the updated preedit is displayed and the cursor jumps to the right again. This looks like a very quick flicker

@mike-fabian Thank you for the explanation.
Seems you mean the the problem is the cursor position only but the output is no problem.

I guess it would be a small bug of the miss usage of IBus APIs in ibus-m17n.

@itt533
Copy link
Author

itt533 commented Jul 26, 2023

Maybe it is easier to see in this video (high framerate 60 fps):
Peek.2023-07-21.15-52.mp4

I am typing bhA using ibus-m17n with sa-itrans. Each time the preedit gets an update, the cursor jumps to the left of the preedit, the updated preedit is displayed and the cursor jumps to the right again.
[ ... ]
Maybe ibus-m17n updates the preedit in an unusual way?

Anyway, it doesn't look like a very serious problem to me and apparently one can avoid it by using ibus-typing-booster instead of ibus-m17n.

Yes it's as you explained it. I did my video with 60 fps, (i used simplescreenrecorder). With 30, the flicker was not noticeable.

Yes i just noticed that with ibus-typing-booster it's fine. With the latter only recently i have been able to shift the input method (previous_input_method and next_input_method). Ctrl+Up and Ctrl+Down respectively don't seem to work, but with custom ones (Ctrl+space and Shift+Ctrl+space) it seems to work. Still i need to see what is selected. Rocket not changing, and no contextual menu.

@mike-fabian
Copy link

Yes i just noticed that with ibus-typing-booster it's fine. With the latter only recently i have been able to shift the input method (previous_input_method and next_input_method). Ctrl+Up and Ctrl+Down respectively don't seem to work, but with custom ones (Ctrl+space and Shift+Ctrl+space) it seems to work. Still i need to see what is selected. Rocket not changing, and no contextual menu.

Very surprising that Ctrl+Up and Ctrl+Down don’t work for you. I am not sure whether it is worth investigating though as you have found keys which work for you.

For seeing what currently is selected:

This is easy to do unless you use Gnome. If you are using any other desktop than Gnome, you can use ibus-setup and choose the option

Show property panel: [Always]

You can move this property panel to a place you like with the mouse.

When ibus-typing-booster is active, the property panel shows which dictionary and which input method currently have highest priority. Like in this screenshot:

Screenshot

@fujiwarat
Copy link
Member

Since it's just a bug of ibus-m17n, it's good to fix it.

@fujiwarat fujiwarat changed the title ENHANCEMENT - ibus input is aggressive m17n:sa:itrans move the input cursor to the head Jul 31, 2023
@fujiwarat
Copy link
Member

The bug title is very bad to understand his issue, modified.

@fujiwarat fujiwarat changed the title m17n:sa:itrans move the input cursor to the head m17n:sa:itrans move the input cursor to the head Jul 31, 2023
@mike-fabian
Copy link

The bug title is very bad to understand his issue, modified.

Thank you!

Since it's just a bug of ibus-m17n, it's good to fix it.

Yes, I agree. I have spent a few hours on this today, but no success so far.

@mike-fabian
Copy link

mike-fabian commented Aug 1, 2023

While debugging, I noticed that ibus_m17n_engine_update_preedit() is almost always called twice for each update to the preedit, so I thought that maybe drawing the preedit twice causes the problem and I added code not to call ibus_engine_update_preedit_text_with_mode() again when the contents of the preedit didn’t change since the last call. Like this:

diff --git a/src/engine.c b/src/engine.c
index d02e566..b399280 100644
--- a/src/engine.c
+++ b/src/engine.c
@@ -546,27 +546,37 @@ ibus_m17n_engine_destroy (IBusM17NEngine *m17n)
 static void
 ibus_m17n_engine_update_preedit (IBusM17NEngine *m17n)
 {
+    static IBusText *last_text = NULL;
+    static int last_cursor_pos = 0;
     IBusText *text;
     gchar *buf;
     IBusM17NEngineClass *klass = (IBusM17NEngineClass *) G_OBJECT_GET_CLASS (m17n);
 
+    if (!m17n)
+        return;
     buf = ibus_m17n_mtext_to_utf8 (m17n->context->preedit);
-    if (buf) {
-        text = ibus_text_new_from_string (buf);
-        if (klass->preedit_foreground != INVALID_COLOR)
-            ibus_text_append_attribute (text, IBUS_ATTR_TYPE_FOREGROUND,
-                                        klass->preedit_foreground, 0, -1);
-        if (klass->preedit_background != INVALID_COLOR)
-            ibus_text_append_attribute (text, IBUS_ATTR_TYPE_BACKGROUND,
-                                        klass->preedit_background, 0, -1);
-        ibus_text_append_attribute (text, IBUS_ATTR_TYPE_UNDERLINE,
-                                    klass->preedit_underline, 0, -1);
-        ibus_engine_update_preedit_text_with_mode ((IBusEngine *) m17n,
-                                                   text,
-                                                   m17n->context->cursor_pos,
-                                                   mtext_len (m17n->context->preedit) > 0,
-                                                   klass->preedit_focus_mode);
+    if (!buf)
+        return;
+    if (last_text && ibus_text_get_text(last_text) && strcmp(buf, ibus_text_get_text(last_text)) == 0 && m17n->context->cursor_pos == last_cursor_pos) {
+        g_free (buf);
+        return;
     }
+    text = ibus_text_new_from_string (buf);
+    last_text = ibus_text_new_from_string (buf);
+    last_cursor_pos = m17n->context->cursor_pos;
+    if (klass->preedit_foreground != INVALID_COLOR)
+        ibus_text_append_attribute (text, IBUS_ATTR_TYPE_FOREGROUND,
+                                    klass->preedit_foreground, 0, -1);
+    if (klass->preedit_background != INVALID_COLOR)
+        ibus_text_append_attribute (text, IBUS_ATTR_TYPE_BACKGROUND,
+                                    klass->preedit_background, 0, -1);
+    ibus_text_append_attribute (text, IBUS_ATTR_TYPE_UNDERLINE,
+                                klass->preedit_underline, 0, -1);
+    ibus_engine_update_preedit_text_with_mode ((IBusEngine *) m17n,
+                                               text,
+                                               m17n->context->cursor_pos,
+                                               mtext_len (m17n->context->preedit) > 0,
+                                               klass->preedit_focus_mode);
     g_free (buf);
 }
 

Unfortunately that didn’t improve anything at all, the behaviour is still exactly the same as before.

Still searching for other possible reasons ...

@mike-fabian
Copy link

Also, not only did the above attempt to draw the preedit only once when it is not changed not help to reduce the flicker, it also interferes with the compose support.

That gave me the idea that the flicker appears because of the compose support. So I commented out these lines in ibus_m17n_engine_process_key_event() at:

https://github.com/ibus/ibus-m17n/blob/main/src/engine.c#L735

    if (IBUS_ENGINE_CLASS (parent_class)->process_key_event (engine, keyval, keycode, modifiers)) {
      if (mtext_len (m17n->context->preedit) > 0) {
	gchar *buf;
	buf = ibus_m17n_mtext_to_utf8 (m17n->context->preedit);
	if (buf) {
	  IBusText *text;
	  text = ibus_text_new_from_string (buf);
	  ibus_engine_commit_text (engine, text);
	  g_free (buf);
	}
	minput_reset_ic (m17n->context);
      }
      return TRUE;
    }

and indeed this removes the flicker. But it also removes the compose support.

@mike-fabian
Copy link

I think https://github.com/ibus/ibus-m17n/tree/release-candidate-1.4.20 mostly fixes the problem, it seems to reduce the flicker to almost nothing.

See this commit for the fix:

ibus/ibus-m17n@b74c6a6

copr test builds for Fedora 37, 38, and rawhide are here:

https://copr.fedorainfracloud.org/coprs/mfabian/ibus-m17n/builds/

@itt533 as you are using Slackware, you can try to build from source using the release candidate branch:

https://github.com/ibus/ibus-m17n/tree/release-candidate-1.4.20

@mike-fabian
Copy link

Here are two videos comparing the flicker with ibus-m17n-1.4.19 and ibus-m17n-1.4.20. To make the flicker even more obvious, I did set a background colour and foreground colour for the preedit in the setup tool.

In both videos I use sa-itrans and type bhhhhhhhhhh and then delete everything again by typing backspace many times.

Only ibus-m17n-1.4.19 produces flickering of the preedit, with ibus-m17n-1.4.20 the preedit does not flicker.

Video with ibus-m17n-1.4.19 (without the fix), considerable flicker:

Peek.2023-08-02.18-26.mp4

Video with ibus-m17n-1.4.20 (with the fix), no flicker:

Peek.2023-08-02.18-29.mp4

@mike-fabian
Copy link

Another test on Gnome Wayland. On Gnome Wayland, my change reduces the flicker less well then on Xorg, but it still reduces the flicker a lot. Weird things happen with the underlining in Gnome Wayland. The underlining is sometimes too short and sometimes too long when backspacing. But this weird problem with the underlining on Gnome Wayland happens both with and without my change, so it is probably unrelated to my change:

Peek.2023-08-03.16-26.mp4

@mike-fabian
Copy link

I tested on Plasma Wayland on Fedora 38 as well, it reduces the flicker there as well.

@mike-fabian
Copy link

Fix is included in the ibus-m17n-1.4.20 release:
https://github.com/ibus/ibus-m17n/releases/tag/1.4.20

@itt533
Copy link
Author

itt533 commented Aug 19, 2023

why did you close it? I just tested your new release and it does not seem that there is any improvement.
check these screencasts, comparing ibus m17n with emacs embedded input method:
http://0x0.st/HLW0.bin (tar.gz file)

@mike-fabian
Copy link

I cannot download that file. Are you sure that you tested the correct release? For me it made a significant difference as you can see in the videos above.

A perfect fix needs new API in ibus.

IBUS_ENGINE_CLASS (parent_class)->process_key_event (engine, keyval, keycode, modifiers)

hides the current preedit and then ibus-m17n shows it again.

For me adding a ibus_engine_show_preedit_text ((IBusEngine *)m17n); before and after calling the process_key_event() method of the parent class made the flicker mostly go away:

ibus/ibus-m17n@b74c6a6

A perfect fix would need a change in ibus to offer an extra parameter in the process_key_event() method of IbusEngineSimple so I could call it like this:

hide_preedit = mtext_len (m17n->context->preedit) == 0
IBUS_ENGINE_CLASS (parent_class)->process_key_event (engine, keyval, keycode, modifiers, hide_preedit)

or something similar.

@itt533
Copy link
Author

itt533 commented Aug 27, 2023

I cannot download that file. Are you sure that you tested the correct release? For me it made a significant difference as you can see in the videos above.

i upgraded to version 1.4.20
I re-uploaded the file, i got the same link and i can download:
http://0x0.st/HLW0.bin

rename extension to .tar

@mike-fabian mike-fabian changed the title m17n:sa:itrans move the input cursor to the head The preedit in all ibus-m17n input methods sometimes flickers Aug 28, 2023
@mike-fabian
Copy link

This time I can download it. But that site still gives me a warning in firefox "File not downloaded. Potential security risk.". So I have to download it on the commandline with wget, rename to .tar.gz, unpack. In future, to make it easier, can you please attach videos here directly using one of the supported formats? (MOV, MP4, WEBM should work).

@mike-fabian
Copy link

mike-fabian commented Aug 28, 2023

Here is your video as mp4s: ibus-1.4.20_flickering.mp4

ibus-1.4.20_flickering.mp4

@fujiwarat fujiwarat added this to the 1.5.30 milestone Sep 14, 2023
@fujiwarat fujiwarat reopened this Sep 14, 2023
fujiwarat added a commit to fujiwarat/ibus that referenced this issue Sep 14, 2023
Several engines can inherit IBusEngieSimple for the compose key support
likes Anthy, Hangul, M17n and it could have the duplicated preedit text
between the engine and the parent IBusEngineSimple and it could update
the preedit text mutually by mistake.

Then the preedit text should not be hidden for zero length at least.
This update might not be enough but hope to fix the cursor position
reset with hiding the preedit text against the reported issue with
`m17n:sa:itrans` engine.

BUG=ibus#2536
fujiwarat added a commit to fujiwarat/ibus-m17n that referenced this issue Sep 14, 2023
Hiding preedit text before committing text causes a reset of
the cursor position.
I think minput_filter() does not consider the order and
ibus-m17n should arrage the order.

ibus/ibus#2536
fujiwarat added a commit to fujiwarat/ibus that referenced this issue Sep 14, 2023
Several engines can inherit IBusEngieSimple for the compose key support
likes Anthy, Hangul, M17n and it could have the duplicated preedit text
between the engine and the parent IBusEngineSimple and it could update
the preedit text mutually by mistake.

Then the preedit text should not be hidden for zero length at least.
This update might not be enough but hope to fix the cursor position
reset with hiding the preedit text against the reported issue with
`m17n:sa:itrans` engine.

BUG=ibus#2536
mike-fabian pushed a commit to ibus/ibus-m17n that referenced this issue Oct 27, 2023
Hiding preedit text before committing text causes a reset of
the cursor position.

I think minput_filter() does not consider the order and
ibus-m17n should arrage the order.

See also: ibus/ibus#2536

Reverts: b74c6a6

This was a workaround to reduce the flicker which is not
needed anymore with ibus >= 1.5.29 and this patch here.
mike-fabian added a commit to ibus/ibus-m17n that referenced this issue Oct 28, 2023
Resolves: #64

Together with ibus >= 1.5.29 this solves the preedit flicker
problem reported in ibus/ibus#2536
perfectly.

Thank you very much to Takao Fujiwara for the pull request.

I rewrote it a bit to make it easier for me to understand.

Hiding preedit text before committing text causes a reset of the
cursor position.

minput_filter() sometimes causes two calls to
ibus_m17n_engine_update_preedit(), the first one with a non-empty
preedit and then a second one with an empty preedit if text to commit
was produced.  But the call to ibus_m17n_engine_commit_string ()
happens a bit later and updating the empty preedit before the commit
causes the cursor to jump left and then right again when the commit is
done (This cursor jump is not always reproducible, it depends on speed
and may be invisible).
mike-fabian added a commit to ibus/ibus-m17n that referenced this issue Oct 28, 2023
Resolves: #64

Together with ibus >= 1.5.29 this solves the preedit flicker
problem reported in ibus/ibus#2536
perfectly.

Thank you very much to Takao Fujiwara for the pull request.

I rewrote it a bit to make it easier for me to understand.

Hiding preedit text before committing text causes a reset of the
cursor position.

minput_filter() sometimes causes two calls to
ibus_m17n_engine_update_preedit(), the first one with a non-empty
preedit and then a second one with an empty preedit if text to commit
was produced.  But the call to ibus_m17n_engine_commit_string ()
happens a bit later and updating the empty preedit before the commit
causes the cursor to jump left and then right again when the commit is
done (This cursor jump is not always reproducible, it depends on speed
and may be invisible).

This also reverts the b74c6a6
(the workaround for the flicker).
mike-fabian added a commit to mike-fabian/ibus that referenced this issue Nov 6, 2023
…ate zero length preedit text"

This causes a crash when typing for example Backspace or Down with an empty preedit in ibus-table:

commit 7bfd2ad (HEAD -> simple-hide-0-preedit, fujiwarat/simple-hide-0-preedit)
Author: fujiwarat <takao.fujiwara1@gmail.com>
Date:   Thu Sep 14 17:44:26 2023 +0900

    src/ibusenginesimple: Do not update zero length preedit text

    Several engines can inherit IBusEngieSimple for the compose key support
    likes Anthy, Hangul, M17n and it could have the duplicated preedit text
    between the engine and the parent IBusEngineSimple and it could update
    the preedit text mutually by mistake.

    Then the preedit text should not be hidden for zero length at least.
    This update might not be enough but hope to fix the cursor position
    reset with hiding the preedit text against the reported issue with
    `m17n:sa:itrans` engine.

    BUG=ibus#2536
@mike-fabian
Copy link

https://github.com/ibus/ibus-m17n/releases/tag/1.4.24 includes a fix now which together with ibus >= 1.5.29 fixes the preedit flicker perfectly.

This is the fix on the ibus-m17n side: ibus/ibus-m17n@2331934

The previous workaround is not needed anymore and was removed.

@fujiwarat
Copy link
Member

Again, don't close this bug!

@fujiwarat fujiwarat reopened this Nov 13, 2023
@itt533
Copy link
Author

itt533 commented Nov 15, 2023

https://github.com/ibus/ibus-m17n/releases/tag/1.4.24 includes a fix now which together with ibus >= 1.5.29 fixes the preedit flicker perfectly.

only this package needs an update?
can you show a screencast?

@mike-fabian
Copy link

https://github.com/ibus/ibus-m17n/releases/tag/1.4.24 includes a fix now which together with ibus >= 1.5.29 fixes the preedit flicker perfectly.

only this package needs an update? can you show a screencast?

No, only an update of this package is not enough (I guess this is why Fujiwara San reopened the bug after I closed it).

You need that update and an update of ibus including this branch:

fujiwarat@7bfd2ad

and including this pull request for the branch:

fujiwarat#6

This is not yet in ibus-1.5.29~rc2-1.fc39.x86_64

So please wait a bit more.

fujiwarat added a commit to fujiwarat/ibus that referenced this issue Apr 11, 2024
Several engines can inherit IBusEngieSimple for the compose key support
likes Anthy, Hangul, M17n and it could have the duplicated preedit text
between the engine and the parent IBusEngineSimple and it could update
the preedit text mutually by mistake.

Then the preedit text should not be hidden for zero length at least.
This update might not be enough but hope to fix the cursor position
reset with hiding the preedit text against the reported issue with
`m17n:sa:itrans` engine.

BUG=ibus#2536
@fujiwarat
Copy link
Member

Sorry for the late response.
I evaluated this issue with #2566 again.
I will integrate the patch after Fedora 40 GA.

@fujiwarat fujiwarat modified the milestones: 1.5.30, 1.5.31 Apr 11, 2024
fujiwarat added a commit to fujiwarat/ibus that referenced this issue Apr 17, 2024
Several engines can inherit IBusEngieSimple for the compose key support
likes Anthy, Hangul, M17n and it could have the duplicated preedit text
between the engine and the parent IBusEngineSimple and it could update
the preedit text mutually by mistake.

Then the preedit text should not be hidden for zero length at least.
This update might not be enough but hope to fix the cursor position
reset with hiding the preedit text against the reported issue with
`m17n:sa:itrans` engine.

BUG=ibus#2536
fujiwarat added a commit that referenced this issue May 24, 2024
Several engines can inherit IBusEngieSimple for the compose key support
likes Anthy, Hangul, M17n and it could have the duplicated preedit text
between the engine and the parent IBusEngineSimple and it could update
the preedit text mutually by mistake.

Then the preedit text should not be hidden for zero length at least.
This update might not be enough but hope to fix the cursor position
reset with hiding the preedit text against the reported issue with
`m17n:sa:itrans` engine.

BUG=#2536
fujiwarat added a commit to fujiwarat/ibus that referenced this issue May 25, 2024
IBusEngineSimple no longer calls to hide the preedit with the zero
length to fix the slurring cursor position but the emoji preedit
becomes not to be hidden with the preedit commit or escape events
as a side effect.
To fix the issue, IBusEngine needs to decide to clear or keep
the preedit when the emoji mode is finished.

BUG=ibus#2536
Fixes: ibus@a3a5a20a
@fujiwarat
Copy link
Member

The main patch is integrated but I found a regression and the referred another patch is under the testing in Fedora 40.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants