macOS: Composition Event #2147

komi1230 · 2022-01-11T12:59:36Z

This is related to #1497.

To make IME work fine in all languages, we should add new event for composition as discussed in #1497 and #2119.
This PR defines enum variant Composition and implement it in macOS.

I found some effort in #1404. (Thanks, @garasubo)
But this branch is very old and it's hard to sync with current master branch.

Thanks,

Tested on all platforms changed
Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
Created or updated an example program if it would help users understand this functionality
Updated feature matrix, if new features were added or implemented

kchibisov · 2022-01-15T03:47:30Z

I'm not familiar with macOS, but I can help bringing and finishing IME handling (by resurrecting old branch and as well as pushing Wayland bits) and providing help downstream(adding inline ime into alacritty).

komi1230 · 2022-01-15T14:49:55Z

Thanks, @kchibisov. According to the discussion in #1497 (comment), the API design for IME seems to be fixed like this commit.

I know this change is destructive, and this change should be done very carefully. I already found master version of Alacritty
would be broken if this was merged. The maintainer of Alacritty already know this. (ref alacritty/alacritty#1101 (comment))

If you agree with this design and you are willing to implement it for Wayland, you may have to cherry-pick one of the commits in this PR. Should I split this PR into defining CompositionEvent and its implementation for macOS ?

kchibisov · 2022-01-15T16:02:34Z

If you agree with this design and you are willing to implement it for Wayland, you may have to cherry-pick one of the commits in this PR. Should I split this PR into defining CompositionEvent and its implementation for macOS ?

Just wait until tomorrow so I rebase composition-event and as well alacritty support. You'll just open PR against that branch, and then we rollup all of the platform changes in one patch.

garasubo · 2022-01-16T03:48:31Z

@komi1230 Thank you for working on MacOS side.
@kchibisov I can help the implementation and verification for x11. Let me know if you need support.

And I recently got a windows machine, so I might support windows implementation in near future.

kchibisov · 2022-01-16T10:57:15Z

@komi1230 I've update composition-event branch, so please update your branch to make use of API the composition-event branch is using right now.

If you have any questions wrt that API or you're thinking that you need to expose something else to make things work properly on macOS feel free share.

komi1230 · 2022-01-16T12:26:51Z

Thanks. I updated this PR to correspond to composition-event branch.
And I agree with shortening the name of enum variant Composition into IME. This is more understandable and clear.

I have confirmed this works fine in macOS. I think this PR is ready to merge.

kchibisov

I'll leave the rest to @madsmtm , since I'm not a macOS expert(I don't use macOS).

src/platform_impl/macos/view.rs

kchibisov · 2022-01-18T01:13:20Z

I've added support for new IME event handling into alacritty alacritty/alacritty#5790 . Though, I'm using my Wayland branch, but just cloning and pointing to your branch in alacritty's Cargo.toml should just work.

komi1230 · 2022-01-18T09:04:59Z

Thanks, @kchibisov. I appreciate you for your great work!! Now I can find IME is properly working in my Alacritty.

2022-01-18.17.56.41.mov

kchibisov · 2022-01-19T10:24:42Z

Thanks, @kchibisov. I appreciate you for your great work!! Now I can find IME is properly working in my Alacritty.

It seems like cursor position is off on your gif. When the cursor is in the end of string(so while you're typing) it shouldn't be visible, however when you start moving with left and right arrow keys it should draw hollow block cursor. Also, make sure that those offsets are in bytes and not in chars.

komi1230 · 2022-01-19T14:20:45Z

@kchibisov When I set cursor_start as byte-wise length of string, the cursor has been in the end of string and now invisible. In addition to this, I fixed some behavior of IME when repeating key without making side effect on window.
Could you check again ?

src/platform_impl/macos/view.rs

komi1230 · 2022-01-21T07:25:53Z

@ArturKovacs @kchibisov I fixed some points, and I want your advice for this. If you feel OK about these changes, please merge.

ArturKovacs · 2022-01-21T10:18:40Z

I hope you don't mind that I added some changes. We apparently don't need NSTextInputContextKeyboardSelectionDidChangeNotification, so I removed it.

ArturKovacs · 2022-01-21T10:21:02Z

@kchibisov note that I added some important information to the documentation of IME::Enabled

src/event.rs

ArturKovacs · 2022-01-21T12:28:39Z

@madsmtm I reviewed the IME part of the PR, and now it seems done now. If it's up to me, you can merge it.

@kchibisov is there anything else that needs to be done with this?

@komi1230 As you can see I pushed some commits to this branch. If you don't mind I'll take over this and address the remaining feedback.

komi1230 · 2022-01-21T12:39:21Z

I completely agree with your changes and have found this works good in macOS.

kchibisov

I've tested the changes on my macOS machine.

The backspace key is triggered twice. always. even on us layout.
I think we shouldn't send backspace and other keys downstream when we're doing composing. If we really want to, we can mark them as synthetic, so the users will still get keys, but will know that those weren't for them.

komi1230 · 2022-01-22T07:38:40Z

This is a topic about responsibility boundary between winit and application. I think all keyboard event should be sent to application, and let application decide how to handle all keyboard event. This is possible via IME enum because application know when IME has been enabled.
Winit is low-level GUI crate so that this should be used as interface to GUI.

ArturKovacs · 2022-01-31T21:03:23Z

Do you mean testing with IBus or something else?

I mean to remove the existing implementation, and create a new implementation that uses IBus to get IME events. I have no idea how to make the existing implementation work, but I have a decent idea of how to use IBus.

kchibisov · 2022-01-31T21:09:29Z

Won't it make us to depend on ibus for X11 IME? Can't IME be done just with what X11 provides?

ArturKovacs · 2022-01-31T21:18:28Z

Why is that a concern? Is there any system currently that uses X11 and doesn't have IBus?

ArturKovacs · 2022-01-31T21:20:25Z

On a different note: what is stopping this from getting merged?

kchibisov · 2022-01-31T21:29:47Z

Why is that a concern? Is there any system currently that uses X11 and doesn't have IBus?

I never had ibus installed and some use fctix, etc, not ibus.

kchibisov · 2022-01-31T21:31:05Z

On a different note: what is stopping this from getting merged?

I should test it, which I'll do in an hour.

ArturKovacs · 2022-01-31T21:45:38Z

I never had ibus installed and some use fctix, etc, not ibus.

In that case, I think I'll try to get the current implementation to work.

madsmtm

I'll try to allocate time for reviewing it this Friday (I'm not well-versed in IME, so doing a bit of research and testing it on my local machine will take some time)

If I haven't responded by then, feel free to merge

src/platform_impl/macos/util/mod.rs

kchibisov

I've tested it on my macOS machine with alacritty and it fails in the following ways:

The state of IME isn't persistent. If I call set_ime_allowed(true) and change input source ime will stop working.
The ime gets randomly locked up. The event loop is continuing to run, but I have neither Keyboard nor IME events. I've tested it with hiragana and you likely can press arrow keys to make it trigger.
When IME is disabled I still have IME window, but it's in the left bottom corner of the screen. I'd expect that it doesn't exist.

I think the locking up is the major pita now.

src/window.rs

ArturKovacs · 2022-02-01T09:40:26Z

I could reproduce the first two with alacritty, but I haven't found a reliable way to reproduce them. Although I think did find how to reproduce getting double characters:

The double character appearance can be reproduced by having the US layout active, switch to another desktop (with CTRL + Arrow), and switch back. Pressing keys like A, S, D will produce two characters in alacritty. This can then be reverted by having alacritty in focus, switching to Hiragana, pressing A, then switching back to US.
HOWEVER after this, if one presses Backspace, then every keypress will again produce two characters.

My previous commit should fix this, and hopefully some of the other issues as well.

kchibisov · 2022-02-02T00:45:01Z

@ArturKovacs it seems like I can't get alacritty locked anymore, which is good.

However if I enable ime on creation, but start with ABC layout. Toggling layout to hiragana won't make IME work, I'll just continue to receive input as if there's no IME running at all. There's no way for downstream to enable IME in that case, since it doesn't even known that anything changed. So winit should track end enabled IME handling routines for a particular window based on recent call to set_ime_allowed. So when user switches layout from ABC to hiragana or something like that IME will work automatically.

ArturKovacs · 2022-02-03T11:18:05Z

However if I enable ime on creation, but start with ABC layout. Toggling layout to hiragana won't make IME work, I'll just continue to receive input as if there's no IME running at all.

I tried to reproduce this, but I couldn't. I modified the ime_example so that ime_set_enabled is called immediately after the window is created (before run). I had the US layout active when starting the application, then I switched to Hiragana and I received IME events as I should, and the candidate selection box appeared.　Does this do the same for you?

ArturKovacs · 2022-02-04T11:23:56Z

@madsmtm suggested that we might not need the subview if we instead implmenet the NSTextInputClient with the original winit view, and then simply only call interpretKeyEvents when IME is allowed. This yields a simpler implementation and the behaviour seems to be identical.

Note that both of the solutions require IME to be allowed for dead-key combinations to be forwarded to the application.

kchibisov

It seems like everything is working for me now. Just some minor commends and should be good from my side.

src/platform_impl/macos/view.rs

madsmtm

So I think I've laid my eyes on most of it, in general it looks really good, nice work all of you!

A bit of terminology: The official term for the popup / overlay that allows you to select the desired characters is called the "candidate window", see the chinese guide and the japanese guide - as @ArturKovacs pointed out to me, the type of candidate window differs between input methods. I think it would be nice to have this term documented somewhere (along with all the other names that IME has).

Another important thing: Creating a new NSTextInputContext in util::create_input_context is probably wrong, we should just use NSView -inputContext (which means view::set_ime_position is likely working on the wrong NSTextInputContext).

src/platform_impl/macos/view.rs

madsmtm · 2022-02-04T15:31:12Z

src/window.rs

+    /// [`KeyboardInput`]: crate::event::WindowEvent::KeyboardInput
+    /// [`ReceivedCharacter`]: crate::event::WindowEvent::ReceivedCharacter
+    #[inline]
+    pub fn set_ime_allowed(&self, allowed: bool) {


We should add a similar method WindowBuilder::with_ime_allowed

Nice catch, I forgot that we should likely add it on a builder.

src/platform_impl/macos/view.rs

examples/ime_input.rs

madsmtm · 2022-02-04T17:10:28Z

src/platform_impl/macos/view.rs

+        let input_source = current_input_source();
+        if state.input_source != input_source && state.is_ime_enabled() {
+            state.ime_state = ImeState::Disabled;
+            state.input_source = input_source;
+            AppState::queue_event(EventWrapper::StaticEvent(Event::WindowEvent {
+                window_id: WindowId(get_window_id(state.ns_window)),
+                event: WindowEvent::IME(IME::Disabled),
+            }));
+        }


It seems unnecessary to me that we signal to the user that IME is "disabled" when the input source changes, since AppKit already clears the markedText buffer, but maybe I just don't really understand how the Enabled and Disabled events would be used?

@kchibisov could you elaborate on the requirements on these?

The purpose is that after you've got disabled you don't need to handle set_ime_position at all and if you had something in preedit you could safely discard it.

Hmm, okay, but a Preedit("", None, None) could do the same, right?

It means that you could have IME running, but just preedit is empty, so you still call set_ime_postion and potentially will call set_surrounding_text(if we add this one).

You could send empty preedit if you want though, but your preedit is empty anyway after the last commit, since this is how it usually works.

madsmtm · 2022-02-04T17:34:30Z

src/platform_impl/macos/view.rs

+                Some(cursor_start),
+                Some(cursor_end),


I don't really understand the purpose of the cursor positions, but they are either redundant on macOS, or we're handling them wrong here? Perhaps @kchibisov could elaborate?

The purpose is that the position of your cursor is defined by your IME. For example on Wayland you can move your cursor around in preedit text, however it's done entirely by IME. So cursor position is to indicate where your cursor is currently is and where its end. Since if start != end you likely have a so called marked text(selection). The position of the cursor in preedit determines how it should look in the end.

I still have a question who is responsible to update it on macOS, since I can move cursor on preedit string in terminal.app, but not with winit, since when I press left/rigth nothing changes, however in terminal app it seems like moving.

Maybe the user should set cursor position itself on macOS, but at this information is essential for Wayland IME and is the only way to obtain current cursor position in preedit string, so you can draw cursor in the right place.

Does it clarify it for you?

Hmm, I think downstream (e.g. alacritty/winit, not AppKit) is expected to render things differently, including the cursor/selected text. At first I thought firstRectForCharacterRange:actualRange: could be used to help tracking the currently focused characters, but then again, its value is cached, so probably not.

I actually think the selectedRange parameter in setMarkedText:selectedRange:replacementRange: could do the trick though. Thanks for the explanation!

I mean, the current information this API exposes is already enough for alacritty to draw everything. At least on Wayland it looks like in any other app from my testing.

If it works out that will be nice, meaning that macOS actually handles it itself.

src/platform_impl/macos/view.rs

examples/ime_input.rs

kchibisov · 2022-02-24T08:36:02Z

what's the status on this @madsmtm @ArturKovacs ?

ArturKovacs · 2022-02-24T08:50:45Z

Sorry I'm super busy these days, I'll probably be able to work on it between the 4th and 6th but not sure.

ArturKovacs · 2022-03-08T22:19:52Z

If you ask me I think this is pretty much mergeable as it is. The conflicts will have to be resolved of course, but the other stuff isn't that important in my opinion. It's more important to have this feature and we can tweak it later.

I don't really have enough time to continue working on this these days. Although I can resolve the merge conflicts if you are willing to merge this afterwards, @kchibisov.

kchibisov · 2022-03-08T22:35:16Z

If @madsmtm is fine with that or could resolve raised issues, I could merge it.

madsmtm

Go ahead, I'll give another review when we're ready with the final PR

ArturKovacs · 2022-03-27T09:04:10Z

I merged the composition-event branch and tested on macos and it seems to work correctly. However the merge introduced a few compile errors for the Linux implementation. Could you please fix that and push the fix to this PR @kchibisov?

kchibisov · 2022-03-27T10:38:51Z

@ArturKovacs it seems you've merged incorrectly, could you just discard the changes you've made to x11/ime/context.rs? After that it should be fine.

I can reset file later if you can't do that now.

ArturKovacs · 2022-03-27T11:04:38Z

I think it's best if you do it.

kchibisov · 2022-04-03T13:39:53Z

Ok, I've rebased and applied the patch on composition-event branch. Thanks.

ArturKovacs added DS - macos C - needs discussion Direction must be ironed out labels Jan 11, 2022

komi1230 mentioned this pull request Jan 14, 2022

Cannot input japanese characters alacritty/alacritty#1101

Closed

kchibisov self-requested a review January 15, 2022 03:44

kchibisov changed the base branch from master to composition-event January 16, 2022 10:55

madsmtm self-requested a review January 16, 2022 15:09

kchibisov requested changes Jan 17, 2022

View reviewed changes

src/platform_impl/macos/view.rs Outdated Show resolved Hide resolved

src/platform_impl/macos/view.rs Outdated Show resolved Hide resolved

src/platform_impl/macos/view.rs Outdated Show resolved Hide resolved

komi1230 mentioned this pull request Jan 18, 2022

Add inline input method support alacritty/alacritty#5790

Closed

ArturKovacs suggested changes Jan 19, 2022

View reviewed changes

komi1230 requested review from ArturKovacs and kchibisov January 19, 2022 16:33

kchibisov reviewed Jan 21, 2022

View reviewed changes

src/event.rs Outdated Show resolved Hide resolved

ArturKovacs approved these changes Jan 21, 2022

View reviewed changes

kchibisov requested changes Jan 21, 2022

View reviewed changes

madsmtm requested changes Jan 31, 2022

View reviewed changes

src/platform_impl/macos/util/mod.rs Outdated Show resolved Hide resolved

kchibisov requested changes Jan 31, 2022

View reviewed changes

src/window.rs Outdated Show resolved Hide resolved

kchibisov requested changes Feb 4, 2022

View reviewed changes

src/platform_impl/macos/view.rs Outdated Show resolved Hide resolved

src/platform_impl/macos/view.rs Outdated Show resolved Hide resolved

src/platform_impl/macos/view.rs Outdated Show resolved Hide resolved

madsmtm mentioned this pull request Feb 4, 2022

add composition event support for Mac #1669

Closed

9 tasks

madsmtm requested changes Feb 4, 2022

View reviewed changes

madsmtm mentioned this pull request Feb 4, 2022

macOS set_ime_position fixes #2180

Merged

5 tasks

kchibisov force-pushed the composition-event branch from 5042d9a to c152698 Compare February 28, 2022 08:50

madsmtm approved these changes Mar 8, 2022

View reviewed changes

kchibisov force-pushed the composition-event branch from c152698 to 6a27687 Compare March 23, 2022 21:19

kchibisov closed this Apr 3, 2022

kchibisov force-pushed the master branch from 3d5a70e to 78e5a39 Compare April 3, 2022 13:21

macOS: Composition Event #2147

macOS: Composition Event #2147

Conversation

komi1230 commented Jan 11, 2022

kchibisov commented Jan 15, 2022

komi1230 commented Jan 15, 2022 • edited

kchibisov commented Jan 15, 2022

garasubo commented Jan 16, 2022

kchibisov commented Jan 16, 2022

komi1230 commented Jan 16, 2022

kchibisov left a comment

Choose a reason for hiding this comment

kchibisov commented Jan 18, 2022

komi1230 commented Jan 18, 2022

kchibisov commented Jan 19, 2022

komi1230 commented Jan 19, 2022

komi1230 commented Jan 21, 2022

ArturKovacs commented Jan 21, 2022

ArturKovacs commented Jan 21, 2022

ArturKovacs commented Jan 21, 2022

komi1230 commented Jan 21, 2022

kchibisov left a comment • edited

Choose a reason for hiding this comment

komi1230 commented Jan 22, 2022

ArturKovacs commented Jan 31, 2022

kchibisov commented Jan 31, 2022

ArturKovacs commented Jan 31, 2022

ArturKovacs commented Jan 31, 2022

kchibisov commented Jan 31, 2022

kchibisov commented Jan 31, 2022

ArturKovacs commented Jan 31, 2022

madsmtm left a comment

Choose a reason for hiding this comment

kchibisov left a comment

Choose a reason for hiding this comment

ArturKovacs commented Feb 1, 2022

kchibisov commented Feb 2, 2022

ArturKovacs commented Feb 3, 2022

ArturKovacs commented Feb 4, 2022

kchibisov left a comment

Choose a reason for hiding this comment

madsmtm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madsmtm Feb 5, 2022 • edited

Choose a reason for hiding this comment

kchibisov Feb 5, 2022 • edited

Choose a reason for hiding this comment

kchibisov commented Feb 24, 2022

ArturKovacs commented Feb 24, 2022

ArturKovacs commented Mar 8, 2022

kchibisov commented Mar 8, 2022

madsmtm left a comment

Choose a reason for hiding this comment

ArturKovacs commented Mar 27, 2022 • edited

kchibisov commented Mar 27, 2022

ArturKovacs commented Mar 27, 2022

kchibisov commented Apr 3, 2022

komi1230 commented Jan 15, 2022 •

edited

kchibisov left a comment •

edited

madsmtm Feb 5, 2022 •

edited

kchibisov Feb 5, 2022 •

edited

ArturKovacs commented Mar 27, 2022 •

edited