From 131d9ca8d4de3288e0770d32651e605634f1f671 Mon Sep 17 00:00:00 2001 From: ksqsf Date: Wed, 22 Jan 2025 00:17:05 +0100 Subject: [PATCH 1/2] macOS: improve IME state management fixes #3925 on macOS, when IME is allowed, always send text to IME and use that result when possible. Even if the keyboard is a simple one, like US keyboard. Committed text is now inserted regardless of the presence of a preedit. --- src/changelog/unreleased.md | 1 + src/platform_impl/apple/appkit/view.rs | 65 +++++++++----------------- 2 files changed, 22 insertions(+), 44 deletions(-) diff --git a/src/changelog/unreleased.md b/src/changelog/unreleased.md index 6a9c4b08dc..a6022e7342 100644 --- a/src/changelog/unreleased.md +++ b/src/changelog/unreleased.md @@ -165,6 +165,7 @@ changelog entry. - Rename `VideoModeHandle` to `VideoMode`, now it only stores plain data. - Make `Fullscreen::Exclusive` contain `(MonitorHandle, VideoMode)`. - On Wayland, no longer send an explicit clearing `Ime::Preedit` just prior to a new `Ime::Preedit`. +- On macOS, IME management is improved. As a result, more key events will be forwarded to IME when IME is allowed. ### Removed diff --git a/src/platform_impl/apple/appkit/view.rs b/src/platform_impl/apple/appkit/view.rs index ddbf4dbe22..45c3bb6234 100644 --- a/src/platform_impl/apple/appkit/view.rs +++ b/src/platform_impl/apple/appkit/view.rs @@ -293,12 +293,6 @@ declare_class!( // Update marked text. *self.ivars().marked_text.borrow_mut() = marked_text; - // Notify IME is active if application still doesn't know it. - if self.ivars().ime_state.get() == ImeState::Disabled { - *self.ivars().input_source.borrow_mut() = self.current_input_source(); - self.queue_event(WindowEvent::Ime(Ime::Enabled)); - } - if unsafe { self.hasMarkedText() } { self.ivars().ime_state.set(ImeState::Preedit); } else { @@ -396,8 +390,7 @@ declare_class!( let is_control = string.chars().next().is_some_and(|c| c.is_control()); - // Commit only if we have marked text. - if unsafe { self.hasMarkedText() } && self.is_ime_enabled() && !is_control { + if self.is_ime_enabled() && !is_control { self.queue_event(WindowEvent::Ime(Ime::Preedit(String::new(), None))); self.queue_event(WindowEvent::Ime(Ime::Commit(string))); self.ivars().ime_state.set(ImeState::Committed); @@ -406,17 +399,11 @@ declare_class!( // Basically, we're sent this message whenever a keyboard event that doesn't generate a "human // readable" character happens, i.e. newlines, tabs, and Ctrl+C. + // In this case, forward the key event to the app. #[method(doCommandBySelector:)] fn do_command_by_selector(&self, command: Sel) { trace_scope!("doCommandBySelector:"); - // We shouldn't forward any character from just committed text, since we'll end up sending - // it twice with some IMEs like Korean one. We'll also always send `Enter` in that case, - // which is not desired given it was used to confirm IME input. - if self.ivars().ime_state.get() == ImeState::Committed { - return; - } - self.ivars().forward_key_to_app.set(true); if unsafe { self.hasMarkedText() } && self.ivars().ime_state.get() == ImeState::Preedit @@ -444,18 +431,11 @@ declare_class!( fn key_down(&self, event: &NSEvent) { trace_scope!("keyDown:"); { - let mut prev_input_source = self.ivars().input_source.borrow_mut(); - let current_input_source = self.current_input_source(); - if *prev_input_source != current_input_source && self.is_ime_enabled() { - *prev_input_source = current_input_source; - drop(prev_input_source); - self.ivars().ime_state.set(ImeState::Disabled); - self.queue_event(WindowEvent::Ime(Ime::Disabled)); - } + let mut input_source = self.ivars().input_source.borrow_mut(); + *input_source = self.current_input_source(); } // Get the characters from the event. - let old_ime_state = self.ivars().ime_state.get(); self.ivars().forward_key_to_app.set(false); let event = replace_event(event, self.option_as_alt()); @@ -478,18 +458,12 @@ declare_class!( self.update_modifiers(&event, false); - let had_ime_input = match self.ivars().ime_state.get() { - ImeState::Committed => { - // Allow normal input after the commit. - self.ivars().ime_state.set(ImeState::Ground); - true - } - ImeState::Preedit => true, - // `key_down` could result in preedit clear, so compare old and current state. - _ => old_ime_state != self.ivars().ime_state.get(), - }; + // Allow normal input after the commit. + if self.ivars().ime_state.get() == ImeState::Committed { + self.ivars().ime_state.set(ImeState::Ground); + } - if !had_ime_input || self.ivars().forward_key_to_app.get() { + if !self.is_ime_enabled() || self.ivars().forward_key_to_app.get() { let key_event = create_key_event(&event, true, unsafe { event.isARepeat() }); self.queue_event(WindowEvent::KeyboardInput { device_id: None, @@ -882,16 +856,19 @@ impl WinitView { return; } self.ivars().ime_allowed.set(ime_allowed); - if self.ivars().ime_allowed.get() { - return; - } - - // Clear markedText - *self.ivars().marked_text.borrow_mut() = NSMutableAttributedString::new(); + if ime_allowed { + if self.ivars().ime_state.get() == ImeState::Disabled { + self.ivars().ime_state.set(ImeState::Ground); + self.queue_event(WindowEvent::Ime(Ime::Enabled)); + } + } else { + // Clear markedText + *self.ivars().marked_text.borrow_mut() = NSMutableAttributedString::new(); - if self.ivars().ime_state.get() != ImeState::Disabled { - self.ivars().ime_state.set(ImeState::Disabled); - self.queue_event(WindowEvent::Ime(Ime::Disabled)); + if self.ivars().ime_state.get() != ImeState::Disabled { + self.ivars().ime_state.set(ImeState::Disabled); + self.queue_event(WindowEvent::Ime(Ime::Disabled)); + } } } From cc12ca0be429293ba7339bbcb3031981b538eaf9 Mon Sep 17 00:00:00 2001 From: ksqsf Date: Mon, 27 Jan 2025 20:31:54 +0100 Subject: [PATCH 2/2] update changelog; remove unused code --- src/changelog/unreleased.md | 3 ++- src/platform_impl/apple/appkit/view.rs | 16 ---------------- 2 files changed, 2 insertions(+), 17 deletions(-) diff --git a/src/changelog/unreleased.md b/src/changelog/unreleased.md index a6022e7342..4a013261a9 100644 --- a/src/changelog/unreleased.md +++ b/src/changelog/unreleased.md @@ -165,7 +165,7 @@ changelog entry. - Rename `VideoModeHandle` to `VideoMode`, now it only stores plain data. - Make `Fullscreen::Exclusive` contain `(MonitorHandle, VideoMode)`. - On Wayland, no longer send an explicit clearing `Ime::Preedit` just prior to a new `Ime::Preedit`. -- On macOS, IME management is improved. As a result, more key events will be forwarded to IME when IME is allowed. +- On macOS, always forwards keys to IME if IME is allowed via `set_ime_allowed`. ### Removed @@ -208,3 +208,4 @@ changelog entry. - On macOS, fixed redundant `SurfaceResized` event at window creation. - On Windows, fixed the event loop not waking on accessibility requests. - On X11, fixed cursor grab mode state tracking on error. +- On macOS, fixed handling of CJK full-width punctuation and SKK Japanese IME. diff --git a/src/platform_impl/apple/appkit/view.rs b/src/platform_impl/apple/appkit/view.rs index 45c3bb6234..3b8d6942a7 100644 --- a/src/platform_impl/apple/appkit/view.rs +++ b/src/platform_impl/apple/appkit/view.rs @@ -120,7 +120,6 @@ pub struct ViewState { phys_modifiers: RefCell>, tracking_rect: Cell>, ime_state: Cell, - input_source: RefCell, /// True iff the application wants IME events. /// @@ -430,10 +429,6 @@ declare_class!( #[method(keyDown:)] fn key_down(&self, event: &NSEvent) { trace_scope!("keyDown:"); - { - let mut input_source = self.ivars().input_source.borrow_mut(); - *input_source = self.current_input_source(); - } // Get the characters from the event. self.ivars().forward_key_to_app.set(false); @@ -788,7 +783,6 @@ impl WinitView { phys_modifiers: Default::default(), tracking_rect: Default::default(), ime_state: Default::default(), - input_source: Default::default(), ime_allowed: Default::default(), forward_key_to_app: Default::default(), marked_text: Default::default(), @@ -797,8 +791,6 @@ impl WinitView { }); let this: Retained = unsafe { msg_send_id![super(this), init] }; - *this.ivars().input_source.borrow_mut() = this.current_input_source(); - this } @@ -821,14 +813,6 @@ impl WinitView { !matches!(self.ivars().ime_state.get(), ImeState::Disabled) } - fn current_input_source(&self) -> String { - self.inputContext() - .expect("input context") - .selectedKeyboardInputSource() - .map(|input_source| input_source.to_string()) - .unwrap_or_default() - } - pub(super) fn cursor_icon(&self) -> Retained { self.ivars().cursor_state.borrow().cursor.clone() }