Don't initialize IME again while it's already in progress

[pjm0616]

I've recently tried wez/wezterm@30345b3 on Ubuntu 22.04 + X11 + ibus + ibus-hangul, and I've found that the IME isn't working most of the time.

After some debugging, I've discovered that:

1. In the release build the IME always doesn't work, and
2. In the debug build the IME works about 10% of the time, and
3. the IME initialization step(from try_open_ic to create_ic_callback) takes some time, up to 100ms, and
4. starting the IME initialization step again while it's already in progress ruins the internal state.

I've added a guard(with a 5-second timeout, so that it can deal with ime disconnects just in case) to try_init_ic(), and now the IME always works, even in the release build.

I believe updating wezterm with this change fixes the issue wez/wezterm#5125

[yshui]

i think i am hitting this problem too. see yshui/picom#1289.

also reported to upstream xcb-imdkit: fcitx/xcb-imdkit#16.

Apparently the correctly solution is to only call xcb_xim_open once. Quoting @wengxt:

I feel it is a bug about how they use the api.

xcb_xim_open should only be called once through the app life time, until a xcb_xim_close is called.

[pjm0616]

@wez Hi!

This fixes an issue where IME doesn't work on wezterm + X11 + tiling window manager, and what I've changed is making sure xcb_xim_open() doesn't get retried in less than 100ms, which fixed the issue.

Per @wengxt's comment above, it seems like xcb_xim_open() should be called only once, but seeing through the git history it seemed like there are some corner cases regarding ime disconnects, so I've only added the 100ms timer to the code(plus, calling xcb_xim_open() on init, not on first ime event), keeping the change minimal.

It would be great if you could update wezterm with this pull request!

[yshui]

eh?

it seems like xcb_xim_open() should be called only once
there are some corner cases regarding ime disconnects,

i think you missed the "until a xcb_xim_close is called" part. calling xcb_xim_open multiple times is fine, as long as you call close before calling it again. and i think that's what should be done here. see yshui@19140b6

[pjm0616]

@yshui Hi! I haven't had the time to try your changes yet, but I think that may not work(at least for my case) because the primary issue for me was that:

1. the whole process of ime initialization(from try_open_ic() to create_ic_callback()) takes some time, up to 100ms, and,
2. receiving events during ime init process makes try_open_ic() called again, interrupting the initialization, and
3. tiling windows managers(I'm using awesome wm) tends to emit several events in a short time on window init. (probably due to automatic resizing of windows?)

I think calling xcb_ime_close() during IME initialization will also interrupt the IME init process(don't know for sure), and if it's true, the changes at yshui@19140b6 may not fix this issue alone. If xcb_ime_close() properly cancels the ongoing IME init process, then it may work, but it still has some inefficiencies, because the ime init process will be canceled&restarted multiple times before it will finally succeed.

[pjm0616]

I just added a line that calls xcb_xim_close() before retrying xcb_ime_open(). That code path is only reached when IME init doesn't complete within 5 seconds, so it's very unlikely.
I hasn't checked how xcb_xim_close() exactly works and what is the ultimately correct fix yet 😢

[wengxt]

I'm not exactly sure why it has to be this complex.

The procedure should be simply just:

Init:
xcb_xim_create // Only memory allocation and there's no XCB request being sent
xcb_xim_set_im_callback
xcb_xim_open(..., open_callback, /auto_connect=/true ...); <--- no matter whether input method is running at this point or not, it can pick one up later.

Shutdown:
xcb_xim_close(im);
xcb_xim_destroy(im);

Once open_callback is called, you can start to send any input method related request. Until disconnected callback that you set via xcb_xim_set_im_callback is called, then you can assume all the resource you created are invalid (e.g. ic), just discard everything and wait for next open_callback (since auto_connect is true).

The auto reconnect is triggered by monitoring the XIM_SERVERS property on root window. Input method restart should initiate a re-connection if previous attempt fails.

You shouldn't need to do things like "try_open_ic" when filtering event, or during update_pos in lib.rs.

update_pos should be like:

if Some(ic):
   send_pos
else:
   cache_pos = pos

And you can then send cache_pos upon create_ic_callback.

Alternatively, you can set auto_connect=false. under current API it is harder to use, the reason is that

1. There's no timeout, so you would need to manage your own time out, e.g. 5secs after xcb_im_open open_callback is still not called, then we retry.
2. if any error happens during xcb_im_open, there's no callback to notify that such a failure really happens. Right now there's no way to allow you to check any internal state value for it, so you can only rely on (1) to retry.

If auto_connect=false sounds a better option to you, I can add some API to expose to the internal state to allow you to check the progress of xcb_im_open.

[wengxt]

I'm not sure why you choose to call try_open_ic in the current position in lib.rs, are you trying to resolve some racing issue?

If not, you can simply move xcb_im_open to initialization code and remove the two call site in lib.rs