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

New XIM composition support doesn't not correctly negotiate with input method #2448

Closed
wengxt opened this issue Aug 30, 2022 · 6 comments · Fixed by #2491
Closed

New XIM composition support doesn't not correctly negotiate with input method #2448

wengxt opened this issue Aug 30, 2022 · 6 comments · Fixed by #2491
Labels
B - bug Dang, that shouldn't have happened DS - x11

Comments

@wengxt
Copy link
Contributor

wengxt commented Aug 30, 2022

X input method may choose not to support all the input styles.

ffi::XIMPreeditCallbacks | ffi::XIMStatusNothing,

Basically this line will make client always create an input context with preedit callback, which is not supported by input method.

The correct way to do this is to do GetIMValues first, to list all the supported style
https://www.x.org/releases/X11R7.6/doc/libX11/specs/XIM/xim.html#Getting_IM_Values

Then send over a style in XCreateIC that is supported by input method.

@kchibisov kchibisov added B - bug Dang, that shouldn't have happened DS - x11 labels Aug 30, 2022
@kchibisov
Copy link
Member

We try to pick method and fallback to other variant on failure if you look closely on the code. Using GetIMValues should help us here, but I'm not sure that it'll be any different in the end? It's hard to read through the issue in your tracker since I don't know chinise, but basically what issue we had that preedit was created, but wasn't sent at all. I had expectation that I'd get NULL, like with some other IMEs, hence the fallback we have

ffi::XIMPreeditNothing | ffi::XIMStatusNothing,
.

@wengxt
Copy link
Contributor Author

wengxt commented Aug 30, 2022

@kchibisov there are 3 different issues. I'm just trying point one of them in this report.

The actual issue

  1. winit doesn't use get im values, so always send the create ic with a wrong style (This is what this issue is about)
  2. fcitx5 doesn't really check the ic style matches the one it supports and always accept it. (this is what need to be fixed on fcitx5 side and I submit a fix fcitx/fcitx5@1d69497)
  3. alacritty+winit still somehow doesn't display preedit <- This is what still need to be investigated and probably belongs to a different issue report. I'm still investigating this.

@kchibisov
Copy link
Member

Yeah, I haven't tested fcitx at all during development and mostly used ibus, since that is what gnome uses, and I'm on Wayland and here we have a bit different IMEs and a bit better docs for all of that.

I'll look into fixing this on X11 when I have time, thx for you feedback.

@wengxt
Copy link
Contributor Author

wengxt commented Aug 30, 2022

Also, I don't think if there's any XIM server implementation send over a XIMError to make XCreateIC fail. The two state of art implementation (IMdkit which is used by xlib based one, used by fcitx4/ibus), xcb based on I created for fcitx5 https://github.com/fcitx/xcb-imdkit/, doesn't reject the style if it doesn't match the announced ones, which means GetIMValues should always be used.

The else branch "create_none_ic" will never be executed in real world.

@kchibisov
Copy link
Member

The else branch "create_none_ic" will never be executed in real world.

It actually was executed, since adding it was a bug fix, see:

#2402

@wengxt
Copy link
Contributor Author

wengxt commented Aug 30, 2022

The else branch "create_none_ic" will never be executed in real world.

It actually was executed, since adding it was a bug fix, see:

#2402

ah, uim.. never know they're using their own xim implementation.. nice to know :)

kchibisov added a commit to kchibisov/winit that referenced this issue Sep 11, 2022
kchibisov added a commit to kchibisov/winit that referenced this issue Sep 11, 2022
kchibisov added a commit to kchibisov/winit that referenced this issue Sep 11, 2022
kchibisov added a commit to kchibisov/winit that referenced this issue Sep 11, 2022
kchibisov added a commit to kchibisov/winit that referenced this issue Sep 11, 2022
kchibisov added a commit to kchibisov/winit that referenced this issue Sep 11, 2022
kchibisov added a commit that referenced this issue Sep 11, 2022
kchibisov added a commit to kchibisov/winit that referenced this issue Sep 11, 2022
kchibisov added a commit that referenced this issue Sep 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B - bug Dang, that shouldn't have happened DS - x11
Development

Successfully merging a pull request may close this issue.

2 participants