Revert "Add composition event on macOS (#1979)" by ArturKovacs · Pull Request #2119 · rust-windowing/winit

ArturKovacs · 2021-12-29T11:59:40Z

Fixes #2097

This reverts commit 8afeb91.

Reverting because this change made Pinyin input unusable
(only latin characters showed even after selecting the
desired Chinese character)

Furthermore the change itself was pretty much a hack. The change allowed an application to show the preedit text. However winit doesn't yet have an API for this so the implementation simply sent ReceivedCharacter events for each character of the preedit text and whenever the text changed it sent 'u{7f}' (aka DELETE) characters for each UTF32 code unit of the previous preedit text, then sent each character of the new preedit text.

For all these reasons, I believe a revert is warranted.

Tested on all platforms changed
Compilation warnings were addressed
cargo fmt has been run on this branch
cargo doc builds successfully
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

ArturKovacs · 2021-12-29T12:42:53Z

With all that said I believe there's a strong need for a proper IME API in winit.

ArturKovacs · 2021-12-31T07:00:51Z

@madsmtm @maroider Do you mind if I merge this?

maroider · 2021-12-31T07:11:25Z

So if I've read this correctly, IME will work fine, but will lack the "enhancement" added by #1979?

If so, then I have no issue with reverting it, as having all IMEs work at all is more important than hacking in pre-edit text.

ArturKovacs · 2021-12-31T07:13:40Z

So if I've read this correctly, IME will work fine, but will lack the "enhancement" added by #1979?

Yes.

madsmtm · 2021-12-31T09:47:10Z

Tagging @komi1230

madsmtm

I'm very inexperienced with IME, but I agree that it's more important to fix the regression than for the "smart" rendering to work (to be honest, I never liked that hack, so that may also influence my decision).

madsmtm · 2021-12-31T09:28:52Z

src/platform_impl/macos/util/mod.rs

-    let char_count = Rc::new(Cell::new(0));
-    let block = {
-        let char_count = char_count.clone();
-        ConcreteBlock::new(move || char_count.set(char_count.get() + 1)).copy()


This was also UB btw, the block must take the four arguments as described in the docs (even if unused)

Thanks for pointing this out, I'll try to remember this for the future.

madsmtm · 2021-12-31T09:34:44Z

CHANGELOG.md

 # Unreleased

 - On X11, add mappings for numpad comma, numpad enter, numlock and pause.
+- On macOS, revert a change that inteded to improve IME but caused Pinyin input to stop working. (In other words, this change fixes Pinyin input)


inteded is misspelled, and the wording is a bit convoluted. Maybe something along the lines of "fixes Pinyin IME input by reverting ..."

Should be resolved now

madsmtm · 2021-12-31T09:45:44Z

src/platform_impl/macos/view.rs

+        let mutable_string = marked_text.mutableString();
+        let _: () = msg_send![mutable_string, setString:""];


Not sure whether this or the other method (release + alloc) is better here, but let's just leave it be for now

I agree that releasing and allocating a new one would be safer, but the reference to markedText seems to be constrainted to the View, so it seems safe enough.

komi1230 · 2021-12-31T10:33:29Z

I was also inexperienced in IME and #1979 was just a workaround to make Japanese input work fine.

I'm sorry for taking time from you and I appreciate you to spending much cost to fix this.

This reverts commit 8afeb91. Reverting because this change made pinyin input unusable (only latin characters showed even after selecting the desired Chinese character)

ArturKovacs · 2021-12-31T16:49:40Z

@komi1230 I'm glad you contributed, and your original change added a feature that I also wanted have in winit. Actually I still want this feature but we will need a separate API for IME to do it right.

ArturKovacs · 2022-01-02T18:59:52Z

@madsmtm did I address all your concerns and would you say that this can be merged?

madsmtm · 2022-01-02T19:21:13Z

Yup, go ahead!

komi1230 · 2022-01-06T02:59:09Z

As the current status, Japanese input method still doesn't work by merging this change and Chinese input method accidentally works fine.
We should add new API to handle pre-edit text as @ArturKovacs says.

I am Alacritty user with Japanese language, so I have big motivation to fix this issue.

In the below code, there is is_synthetic field in struct KeyboardInput in enum WindowEvent.
I looked up how to use this field but I'm not sure.

winit/src/event.rs

Lines 251 to 264 in 1b3b82a

    
               KeyboardInput { 
        
                   device_id: DeviceId, 
        
                   input: KeyboardInput, 
        
                   /// If `true`, the event was generated synthetically by winit 
        
                   /// in one of the following circumstances: 
        
                   /// 
        
                   /// * Synthetic key press events are generated for all keys pressed 
        
                   ///   when a window gains focus. Likewise, synthetic key release events 
        
                   ///   are generated for all keys pressed when a window goes out of focus. 
        
                   ///   ***Currently, this is only functional on X11 and Windows*** 
        
                   /// 
        
                   /// Otherwise, this value is always `false`. 
        
                   is_synthetic: bool, 
        
               },

To implement new API to handle pre-edit text, should we add new field for this ?

ArturKovacs · 2022-01-07T07:56:31Z

To implement new API to handle pre-edit text, should we add new field for this ?

I don't think that's a good approach. I think we will need a new WindowEvent kind. See the following thread for discussion about this: #1497

ArturKovacs added the DS - appkit Affects the AppKit/macOS backend label Dec 29, 2021

ArturKovacs mentioned this pull request Dec 29, 2021

0.10.0 dev (master branch) can not input chinese by apple input method alacritty/alacritty#5669

Closed

madsmtm approved these changes Dec 31, 2021

View reviewed changes

Revert "Add composition event on macOS (rust-windowing#1979)"

f4f6aab

This reverts commit 8afeb91. Reverting because this change made pinyin input unusable (only latin characters showed even after selecting the desired Chinese character)

ArturKovacs force-pushed the revert-1979 branch from a222fff to f4f6aab Compare December 31, 2021 16:40

ArturKovacs merged commit 6b250a7 into rust-windowing:master Jan 2, 2022

ArturKovacs deleted the revert-1979 branch January 2, 2022 21:01

This was referenced Jan 2, 2022

Release v0.26.1 #2124

Closed

Release 0.26.1 #2125

Merged

madsmtm mentioned this pull request Jan 6, 2022

expected 'bool', found integer - Mac M1 Chip #2139

Closed

madsmtm mentioned this pull request Jan 7, 2022

Fix some invalid msg_send! usage #2138

Merged

5 tasks

komi1230 mentioned this pull request Jan 11, 2022

macOS: Composition Event #2147

Closed

5 tasks

fredizzimo mentioned this pull request Jun 15, 2023

Non Qwerty\En US Input neovide/neovide#445

Closed

		let mutable_string = marked_text.mutableString();
		let _: () = msg_send![mutable_string, setString:""];

Conversation

ArturKovacs commented Dec 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ArturKovacs commented Dec 29, 2021

Uh oh!

ArturKovacs commented Dec 31, 2021

Uh oh!

maroider commented Dec 31, 2021

Uh oh!

ArturKovacs commented Dec 31, 2021

Uh oh!

madsmtm commented Dec 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

madsmtm left a comment

Choose a reason for hiding this comment

Uh oh!

madsmtm Dec 31, 2021

Choose a reason for hiding this comment

Uh oh!

ArturKovacs Dec 31, 2021

Choose a reason for hiding this comment

Uh oh!

madsmtm Dec 31, 2021

Choose a reason for hiding this comment

Uh oh!

ArturKovacs Dec 31, 2021

Choose a reason for hiding this comment

Uh oh!

madsmtm Dec 31, 2021

Choose a reason for hiding this comment

Uh oh!

ArturKovacs Dec 31, 2021

Choose a reason for hiding this comment

Uh oh!

komi1230 commented Dec 31, 2021

Uh oh!

ArturKovacs commented Dec 31, 2021

Uh oh!

ArturKovacs commented Jan 2, 2022

Uh oh!

madsmtm commented Jan 2, 2022

Uh oh!

komi1230 commented Jan 6, 2022

Uh oh!

ArturKovacs commented Jan 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

ArturKovacs commented Dec 29, 2021 •

edited

Loading

madsmtm commented Dec 31, 2021 •

edited

Loading