Subject: Re: bug#74590: 31.0.50 [scratch/igc branch]; key input sometimes skip fcitx input method preedit box

From: Yikai Zhao <[email-masked]>
Date: Fri, 28 Feb 2025 23:55:36 +0800

On Tue, Feb 25, 2025 at 11:19 PM Yikai Zhao <[email-masked]> wrote:
>
> Hi,
>
> On Sat, Feb 22, 2025 at 5:25 AM Pip Cet <[email-masked]> wrote:
> >
> > Pip Cet <[email-masked]> writes:
> >
> > > "Yikai Zhao" <[email-masked]> writes:
> > >
> > >> Hi,
> > >>
> > >> I have finished bisecting between the two versions, I have identified
> > >> that it's this commit that lowers the frequency of the issue:
> > >>
> > >>   commit c425f10a30c96f45fe1ccce423851bf32b8cb8df: Make balancing
> > >> buffer intervals optional>
> > >
> > >> Based on the change of this commit, I can also verify that, with the
> > >> newest version of feature/igc, when setting igc--balance-intervals to
> > >> t, the issue can be reproduced with similar high probability as
> > >> before.
> > >
> > > So it is a timing issue, most likely, we just don't know what changed
> > > precisely.  My next suggestion would be to add random delays near the
> > > buffer_step function, in order to make the bug more likely, but it's
> > > hard to do and I still cannot reproduce the issue you are seeing here...
> > >
> > >> I also tried your newest patch, but the issue still happens... I
> > >> modified your patch for a little bit to add some more debug info
> > >> (timestamps etc.) and recorded a video reproducing the issue. I hope
> > >> it helps. See attachments for the modified patch, the screencast video
> > >> and the log.
> > >
> > > Thank you!  That points to this commit by Po Lu in 2022:
> > >
> > > 8d0a2e4dce41fd811319e94e5f01e5fcf8b3c62a Fix build without X11 I18N
> > >
> > > * src/xterm.c (event_handler_gdk):
> > > (handle_one_xevent):
> > > (x_draw_window_cursor):
> > > (x_term_init): Fix build without HAVE_X_I18N.
> > >
> > > That commit adds a call to xg_filter_key to event_handler_gdk, but only
> > > if "current_count" is >= 0.  I have tried to read the code, but I don't
> > > know what "current_count" is, why it's sometimes negative, and why in
> > > the case that it is negative, we call handle_one_xevent without ever
> > > calling xg_filter_key (x_filter_key now).  That would lead to the key
> > > being handled, right?
> >
> > I've stared at that code for a while, and I don't think it's intentional
> > that we're occasionally failing to call x_filter_event on key release
> > events.
> >
> > I understand you've tried a lot of things, and you're probably running
> > out of patience, but if you could try this patch, in *addition* to the
> > one you already have, that might provide further valuable input.  Again,
> > I understand if you don't want to do that right now :-)
>
> No problem at all :) I should thank *you* all for trying to help.
>
> I just tried this patch, it seems to fix the issue for me!
> I also tried the version without the original "xev" and "CurrentTime"
> hack (aka the patch below) and it still seems to work.
> Just to be sure, I will keep using it for some time and let you know the result.

Update: after some days of use, I can confirm that this patch fixes
the issue for me.

>
> diff --git a/src/xterm.c b/src/xterm.c
> index c3137945ac5..f34e84c52e4 100644
> --- a/src/xterm.c
> +++ b/src/xterm.c
> @@ -18027,7 +18027,63 @@ event_handler_gdk (GdkXEvent *gxev, GdkEvent
> *ev, gpointer data)
>                                 current_hold_quit);
>      }
>    else
> -    current_finish = x_dispatch_event (xev, xev->xany.display);
> +    {
> +      struct x_display_info *dpyinfo;
> +
> +      dpyinfo = x_display_info_for_display (xev->xany.display);
> +
> +#ifdef HAVE_X_I18N
> +      /* Filter events for the current X input method.
> +         GTK calls XFilterEvent but not for key press and release,
> +         so we do it here.  */
> +      if ((xev->type == KeyPress || xev->type == KeyRelease)
> +         && dpyinfo
> +         && x_filter_event (dpyinfo, xev))
> +       {
> +         unblock_input ();
> +         return GDK_FILTER_REMOVE;
> +       }
> +#elif USE_GTK
> +      if (dpyinfo && (dpyinfo->prefer_native_input
> +                     || x_gtk_use_native_input)
> +         && (xev->type == KeyPress
> +#ifdef HAVE_XINPUT2
> +             /* GTK claims cookies for us, so we don't have to claim
> +                them here.  */
> +             || (dpyinfo->supports_xi2
> +                 && xev->type == GenericEvent
> +                 && (xev->xgeneric.extension
> +                     == dpyinfo->xi2_opcode)
> +                 && ((xev->xgeneric.evtype
> +                      == XI_KeyPress)
> +                     || (xev->xgeneric.evtype
> +                         == XI_KeyRelease)))
> +#endif
> +             ))
> +       {
> +         struct frame *f;
> +
> +#ifdef HAVE_XINPUT2
> +         if (xev->type == GenericEvent)
> +           f = x_any_window_to_frame (dpyinfo,
> +                                      ((XIDeviceEvent *)
> xev->xcookie.data)->event);
> +         else
> +#endif
> +           f = x_any_window_to_frame (dpyinfo, xev->xany.window);
> +
> +         if (f && xg_filter_key (f, xev))
> +           {
> +             unblock_input ();
> +             return GDK_FILTER_REMOVE;
> +           }
> +       }
> +#endif
> +
> +      if (! dpyinfo)
> +        current_finish = X_EVENT_NORMAL;
> +      else
> +       current_finish = x_dispatch_event (xev, xev->xany.display);
> +    }
>
>    unblock_input ();
>
>
> > >> In the video, I typed: c e u i <space> c e u i <space> . Each "c e u
> > >> i" should translates to "测试" by fcitx and "<space>" should confirm it.
> > >> But note that the second "c" went straight to the emacs buffer. The
> >
> > The second 'c' was received by event_handler_gdk while current_count was
> > -1, so we never called x_filter_event for it.  I think that confuses
> > fcitx, which assumes all key presses and releases get filtered through
> > XFilterEvent.
> >
> > So the patch above might fix that issue, but it doesn't explain (to me)
> > what current_count == -1 might mean and why it appears to happen more
> > often with MPS.
> >
> > >> video also shows the live log on the left side, maybe it will be
> > >> helpful.
> > >
> > > Thanks!  The other thing I notice is that you're using GenericEvent
> > > events, while my setup (where I haven't been able to reproduce the bug
> > > after my changes) doesn't seem to produce those.  That might also be
> > > part of the issue.
> >
> >
> > FWIW, I've set up a qemu virtual machine to reproduce the bug, but I
> > haven't been successful so far.
>
> Regarding the test environment, there's one thing I maybe should have
> mentioned earlier:
>
> During all my testing, I'm running emacs packaged as AppImage
> (https://github.com/blahgeek/emacs-appimage) because it's easier for
> me to build libraries like MPS in the docker environment.
>
> I didn't think that's relevant because AppImage is just plain emacs
> executable with some extra dependencies. There's no namespaces or
> other magic stuff like flatpak. Among the packaged dynamic libraries,
> there's no X11 or gtk related ones either (the list is here:
> https://github.com/blahgeek/emacs-appimage/blob/7d357757d90a23e42d0b5976c8ace33721915e04/scripts/postprocess.py#L60),
> so there should not be some library version mismatch issue. Although,
> the build environment is different from the execution environment, so
> the x11 or gtk related libraries used during building are different
> from the ones used during execution. But to my understanding that's
> common and ok.
>
> I'm not sure if it will help you reproduce or analyze the issue. If
> so, I apologize for not mentioning it sooner.
>
> >
> > Thanks for your patience so far, I'll continue trying to trigger this on
> > a virtual machine.
> >
> > Pip
> >
>
> Thanks
> Yikai