-
Notifications
You must be signed in to change notification settings - Fork 891
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
Revert "Add composition event on macOS (#1979)" #2119
Conversation
With all that said I believe there's a strong need for a proper IME API in winit. |
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. |
Yes. |
Tagging @komi1230 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
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() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was also UB btw, the block must take the four arguments as described in the docs (even if unused)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing this out, I'll try to remember this for the future.
CHANGELOG.md
Outdated
@@ -1,6 +1,7 @@ | |||
# 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inteded
is misspelled, and the wording is a bit convoluted. Maybe something along the lines of "fixes Pinyin IME input by reverting ..."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be resolved now
let mutable_string = marked_text.mutableString(); | ||
let _: () = msg_send![mutable_string, setString:""]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure whether this or the other method (release + alloc) is better here, but let's just leave it be for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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)
a222fff
to
f4f6aab
Compare
@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. |
@madsmtm did I address all your concerns and would you say that this can be merged? |
Yup, go ahead! |
As the current status, Japanese input method still doesn't work by merging this change and Chinese input method accidentally works fine. I am Alacritty user with Japanese language, so I have big motivation to fix this issue. In the below code, there is Lines 251 to 264 in 1b3b82a
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 |
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.
cargo fmt
has been run on this branchcargo doc
builds successfullyCHANGELOG.md
if knowledge of this change could be valuable to users