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

Add inline input method support #5790

Closed
wants to merge 2 commits into from

Conversation

kchibisov
Copy link
Member

This commit adds support for inline IME handling. It also makes search
bar draw proper Underline cursor instead of using '_' character.

Fixes #1613.

@kchibisov kchibisov force-pushed the winit-new-ime-event branch 2 times, most recently from d07935f to 00bd046 Compare January 18, 2022 01:05
@kchibisov
Copy link
Member Author

@chrisduerr Would like to hear your input on this one, since alacritty part is done. However maybe you suggest something wrt the winit API we're using, so we can tune it.

cursor_start: Option<usize>,

/// Byte offset for cursor end in text.
_cursor_end: Option<usize>,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is basically to have a highlight in preedit string while you're at it. We could probably do that by drawing hollow block cursor from cursor_start up to cursor_end, what do you think? For now I don't handle end.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is start/end here? In which scenario can the cursor be more than a fullwidth character (I'm not familiar with IME)?

Copy link

@rano-oss rano-oss Jan 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Cursor covers the whole sentence, often one writes whole sentences:
E.G:
Start: 0, end: 0 = |您好
Start: 0, end: 6* = |您好 (or the whole text is marked or something, I haven't seen it yet)
Start: 6, end: 6* = 您好|

*(these are usually 3 bytes each, could also be 4 or 2, it is Unicode characters encoded as utf-8)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it could be treated as selection from start to end in preedit, I also haven't seen something like this, all IMEs I've used just report the same start and end.

WindowEvent::IME(ime) => match ime {
// TODO decide what to do with it, but in general we should stop update of
// IME input methods.
IME::Enabled => {},
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will it make sense to check whether ime is present based on this enabled info? We can't check based on preedit string since you don't have it right after inserting text with commit.

Copy link

@pickfire pickfire Jan 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by ime is present? Like when user toggled ime? IIRC there isn't an event for that until user enter some text, maybe I am wrong.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does this Enabled info come from? Would likely be helpful to read the platform-specific documentation on this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Wayland it comes when you enable input method. So you can clearly skip all input method handling until you've get this. For example you don't have to do anything about IME popup position, which we're currently doing in display, since there's no IME at all.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by ime is present? Like when user toggled ime? IIRC there isn't an event for that until user enter some text, maybe I am wrong.

No, On Wayland/macOS you known when you're using IME. So if you don't have something like fcitx running you won't ever get Enabled.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah that is true, just got confused by the naming. Then I agree with your statement, it makes sense to check that IME is present based on this.
Also thanks, that clarified something for my compositor module.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this might not apply to X11, so not sure.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, but those platforms in winit will just send this event on window creation and emit disabled on window death, so it doesn't matter.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example you don't have to do anything about IME popup position, which we're currently doing in display, since there's no IME at all.

Probably easier for us to just update IME position than to check whether we have to or not.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably easier for us to just update IME position than to check whether we have to or not.

I also feel like it, so I don't want to do anything with enabled event.

@crocket
Copy link

crocket commented Jan 18, 2022

Does this support fcitx5?

@kchibisov
Copy link
Member Author

@crocket it's using text-input-v3, so everything that is using wayland protocol for IME and not dbus should work. So if fcitx is using input-method-v2 it should work. I don't have ability to test fcitx, but it doesn't matter, since they talk to compositor and compositor back to us.

For example the following https://github.com/tadeokondrak/anthywl and https://github.com/emersion/wlhangul works just fine.

@rano-oss
Copy link

It works great on wayland in my tests, but it fails to start on x11 with this error:
"error: XMisc("IME Context creation failed")"

@pickfire
Copy link

I tested it out on x11 with fcitx5, didn't seemed to work, no inline preedit and preedit position is incorrect. Do I have to do something special for the settings? Below I have screenshots with fcitx5 mozc and pinyin.

Screenshot_20220118_180751
Screenshot_20220118_180811

For a comparison, this is how konsole looks like.

Screenshot_20220118_180928

@komi1230
Copy link

@rano-oss This issue will be fixed in rust-windowing/winit#2147. Currently, the implementation for X11 is still not done. We are working on that, so wait for a second.

cursor_start: Option<usize>,

/// Byte offset for cursor end in text.
_cursor_end: Option<usize>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is start/end here? In which scenario can the cursor be more than a fullwidth character (I'm not familiar with IME)?

alacritty/src/display/mod.rs Show resolved Hide resolved
alacritty/src/display/mod.rs Show resolved Hide resolved
WindowEvent::IME(ime) => match ime {
// TODO decide what to do with it, but in general we should stop update of
// IME input methods.
IME::Enabled => {},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does this Enabled info come from? Would likely be helpful to read the platform-specific documentation on this.

alacritty/src/event.rs Outdated Show resolved Hide resolved
alacritty/src/event.rs Outdated Show resolved Hide resolved
@rano-oss
Copy link

@rano-oss This issue will be fixed in rust-windowing/winit#2147. Currently, the implementation for X11 is still not done. We are working on that, so wait for a second.

I will, I hope I didn't sound impatient, I was just pointing it out. Maybe this is behind a feature flag and I should have kept my mouth shut, if so I apologize.

@kchibisov kchibisov force-pushed the winit-new-ime-event branch 2 times, most recently from 1aae1f8 to 1ccdf9e Compare January 27, 2022 11:04
@kchibisov kchibisov added this to the Version 0.11.0 milestone Jan 28, 2022
@kchibisov
Copy link
Member Author

For anyone who is using ime on daily basis, do you need a beam(or your config) cursor in the end of preedit of IME input? I think vte/gtk doesn't draw it, but some terminals on macOS do draw it.

@rano-oss
Copy link

For anyone who is using ime on daily basis, do you need a beam(or your config) cursor in the end of preedit of IME input? I think vte/gtk doesn't draw it, but some terminals on macOS do draw it.

Yes, the cursor at the end is important to give an indication of where one is when writing. It is typical to move that cursor backwards to E.G change a character somewhere in a sentence before committing the sentence.

@kchibisov
Copy link
Member Author

It is typical to move that cursor backwards to E.G change a character somewhere in a sentence before committing the sentence.

Yeah, I guess I've just haven't seen it in GTK, they only draw cursor when you start moving it backwards, but not when it's in the end. I'm also not quite sure why you'd need a cursor in the end tbqh, but sure, could add it I guess.

@rano-oss
Copy link

It is typical to move that cursor backwards to E.G change a character somewhere in a sentence before committing the sentence.

Yeah, I guess I've just haven't seen it in GTK, they only draw cursor when you start moving it backwards, but not when it's in the end. I'm also not quite sure why you'd need a cursor in the end tbqh, but sure, could add it I guess.

I think as long as you just accommodate for a cursor at the position of "cursor_end"? And then just let the IME handle cursor placement? And I guess if "cursor_begin != cursor_end" you make the text between marked?

@kchibisov
Copy link
Member Author

The X11 IME should work, at least in a way xlib allows it to work. The preedit window at the bottom of the window is xlib feature...

@kchibisov kchibisov force-pushed the winit-new-ime-event branch 2 times, most recently from d2ab7e1 to f58a6fe Compare March 23, 2022 23:10
@pickfire
Copy link

pickfire commented Mar 24, 2022

The X11 IME should work, at least in a way xlib allows it to work. The preedit window at the bottom of the window is xlib feature...

I think that might be the default. But for it to appear under the cursor like other applications we need to set the preedit style to either over-the-spot or on-the-spot (IIRC on-the-spot requires more stuff) and update the preedit position every time the cursor position changed. That was what I learned when I was adding IME support for suckless's st (simple terminal) back then.

Now it does work on x11 but the experience is not good since the preedit window is not below the cursor, over-the-spot (like the one in st) would be at least better than this but on-the-spot (like the one in konsole) would have been the best experience.

@kchibisov
Copy link
Member Author

@pickfire no, xlib basically doesn't support it with on-the-spot. You'd need xcb or patch xlib directly, there's no way around it. I'd try sending patch to xlib, since right now when preedit callbacks are used the cursor spot setting requests are discarded by xlib for some 'legacy' reasons.

@pickfire
Copy link

Ah, xlib vs xcb. Yeah, I think maybe need xcb. Is it a dependency that we shouldn't take in?

@kchibisov
Copy link
Member Author

@pickfire yes, because winit is using xlib, so unless we rewrite winit with xcb, we can't use really use it. But we can try patching xlib to allow that, since winit has spot setting in place.

@@ -19,11 +19,13 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
- Support for OpenGL ES 2.0
- Escape sequence to set underline color (`CSI 58 : 2 : Ps : Ps : Ps m`/`CSI 58 : 5 : Ps m`)
- Escape sequence to reset underline color (`CSI 59 m`)
- Support for inline input method
Copy link

@pickfire pickfire Apr 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to mention that the caveat is that on x11 only off-the-spot preedit area (no over-the-spot or on-the-spot) is supported due to library limitations (xcb not available)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entirely sure. It's kind of common and I've seen other clients with such issue.

@chrisduerr what do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have absolutely no clue what these different IME modes are, but chances are if we just say "support XYZ" and Toby's version of XYZ doesn't work he'll come crying on the issue tracker. So we should set the expectations so people are aware of which setups should and shouldn't work now?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like IME is now stuck at the bottom of the window due to libX11 limitations in CHANGED?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IME is only available at the bottom of the window (off-the-spot preedit area) due to libX11 limitations for x11

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know, I kind of like the one I have in CHANGELOG right now, since it's more about changed.

Copy link

@pickfire pickfire Apr 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's also about changed, but at least it needs to mention the unexpected, better than letting users dig in to figure out why it didn't work as they expected.

@kchibisov kchibisov force-pushed the winit-new-ime-event branch 2 times, most recently from 936c010 to 48cde15 Compare April 6, 2022 22:08
CHANGELOG.md Outdated Show resolved Hide resolved
@kchibisov kchibisov force-pushed the winit-new-ime-event branch 2 times, most recently from bad1cff to 8f04982 Compare April 13, 2022 18:41
@kchibisov
Copy link
Member Author

Windows IME inline bits were also added.

This commit adds support for inline IME handling. It also makes search
bar draw proper Underline cursor instead of using '_' character.

Fixes alacritty#1613.
@kchibisov
Copy link
Member Author

Closing in favor of #6079.

@kchibisov kchibisov closed this Jun 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Support inline "input method" input
6 participants