Package: emacs;
Reported by: Yikai Zhao <yikai <at> z1k.dev>
Date: Thu, 28 Nov 2024 13:20:02 UTC
Severity: normal
Found in version 31.0.50
View this message in rfc822 format
From: Yikai Zhao <yikai <at> z1k.dev> To: Pip Cet <pipcet <at> protonmail.com> Cc: Po Lu <luangruo <at> yahoo.com>, Gerd Möllmann <gerd.moellmann <at> gmail.com>, Helmut Eller <eller.helmut <at> gmail.com>, 74590 <at> debbugs.gnu.org Subject: bug#74590: 31.0.50 [scratch/igc branch]; key input sometimes skip fcitx input method preedit box Date: Fri, 28 Feb 2025 23:55:36 +0800
On Tue, Feb 25, 2025 at 11:19 PM Yikai Zhao <yikai <at> z1k.dev> wrote:
>
> Hi,
>
> On Sat, Feb 22, 2025 at 5:25 AM Pip Cet <pipcet <at> protonmail.com> wrote:
> >
> > Pip Cet <pipcet <at> protonmail.com> writes:
> >
> > > "Yikai Zhao" <yikai <at> z1k.dev> 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
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.