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 composition event on macOS #1979

Merged
merged 14 commits into from Aug 9, 2021
Merged

Conversation

komi1230
Copy link
Contributor

@komi1230 komi1230 commented Jul 27, 2021

  • 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

This change is to enable to use IME.

Before:

default.mov

After:

default.mov

@komi1230
Copy link
Contributor Author

hmm....
This change has no effect on Android.
I'm not sure why CI fails.

@maroider maroider added DS - macos C - waiting on maintainer A maintainer must review this code labels Jul 29, 2021
@maroider
Copy link
Member

I think we might need to update our Android and WASM CI, so you don't need to do anything yourself (unless you'd like to fix our CI for us :D).

@madsmtm
Copy link
Member

madsmtm commented Aug 3, 2021

Is it possible for you to provide a minimal example, so that we can reproduce without having to build Alacritty?

Additionally, maybe you could also provide an explanation of why this is necessary / why it works, because that is not immediately obvious to me 😉

@komi1230
Copy link
Contributor Author

komi1230 commented Aug 4, 2021

Thank you for fixing CI !

And sorry for less explanation.
This PR is related to #1497.

It seems Winit has backend and frontend.
Backend catches events from OS (mouse or keyboard, for example) and passes over them to frontend, and also frontend catches events from backend and arranges them into common structs or types.

Backend for each OS corresponds to platform_impl dir.

So far, in macOS, keyboard event didn't work fine for Japanese language.
macOS has three types of keyboard event: insertText, setMarkedText, doCommandBySelector.
insertText is for English alphabet, this is easy, and doCommandBySelector is for keyboard shortcuts, not inserting some character (Ctrl-c, for example).
And setMarkedText is for other languages.
This is really tough point of IME...

This is a topic of linguistics.
For example, Japanese language has some formats: Kanji, Hiragana and Katakana, (maybe you know 😋 )
When we input Kanji, we have to start from Hiragana by typing English alphabet at first and convert it into Kanji, like sakura into さくら into .
In the step of converting Hiragana into Kanji, there is a challenge.
Hiragana can be pronounced in only one way, but a couple of Kanji characters have the same pronounce, as さくら can be or 佐倉.
So when we input Kanji, we have to select which Kanji to input.

Let's get back to a topic of IME API.
In selecting Kanji, characters to insert are still not decided. In macOS, this undecided character is called markedText.
When we have decided which Kanji to input, the selected character is handled in insertText from setMarkedText.
So inputting Kanji is inserted by insertText.
But this is a problem. We don't know which Kanji we are selecting.
So far, insertText was the only function to insert characters but setMarkedText should be another function to display undecided character.

In this PR, I implemented this architecture by adding event-push part to setMarkedText.
So you can find which Kanji we are selecting is visible and decided Kanji is properly inserted and displayed.

Thank you for reading long description.

Ref:

@komi1230
Copy link
Contributor Author

komi1230 commented Aug 4, 2021

I'm sorry I don't have a minimal example for this.
But you can reproduce this fixed Alacritty in this repo.
https://github.com/komi1230/tmp

@ArturKovacs
Copy link
Contributor

Correct me if I'm wrong but it seems that this implementation does the following:

  • Send the currently selected IME text to the application by sending a ReceivedCharacter for each character in the text.
  • When the selecteed IME text changes, winit sends as many DELETE characters as long the previously selected text was, expecting the application to remove all those characters from the screen. Then winit sends the characters of the newly selected text again.

I have to admit, this is a kinda smart way of dealing with IME without changing winit's API. I would actually be willing to merge this, considering that the discussion at #1497 got stuck almost a year ago. We can add a nice IME API later.

With that said, I haven't tested this PR so I'm going to give a proper review later, I think, but in the meantime @madsmtm feel free to review this.

@ArturKovacs
Copy link
Contributor

ArturKovacs commented Aug 4, 2021

Here's a small test application:

Click for the code
use log::LevelFilter;
use simple_logger::SimpleLogger;
use winit::{
    event::{Event, WindowEvent},
    event_loop::{ControlFlow, EventLoop},
    window::WindowBuilder,
};

fn main() {
    SimpleLogger::new().with_level(LevelFilter::Debug).init().unwrap();
    let event_loop = EventLoop::new();

    let _window = WindowBuilder::new()
        .with_inner_size(winit::dpi::LogicalSize::new(128.0, 128.0))
        .build(&event_loop)
        .unwrap();
    
    event_loop.run(move |event, _, control_flow| {
        *control_flow = ControlFlow::Wait;
        match event {
            Event::WindowEvent {
                event: WindowEvent::CloseRequested,
                ..
            } => *control_flow = ControlFlow::Exit,
            Event::WindowEvent {
                event: WindowEvent::ReceivedCharacter(ch),
                ..
            } => {
                println!("recv ch: {:?}", ch);
            }
            _ => (),
        }
    });
}

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
src/platform_impl/macos/view.rs Outdated Show resolved Hide resolved
src/platform_impl/macos/view.rs Outdated Show resolved Hide resolved
@komi1230
Copy link
Contributor Author

komi1230 commented Aug 5, 2021

@ArturKovacs Thank you for reviewing ! I fixed parts you pointed out. Can you check again ?

src/platform_impl/macos/view.rs Outdated Show resolved Hide resolved
);
let composed_string = str::from_utf8_unchecked(slice);

state.marked_text = composed_string.to_string();
Copy link
Contributor

Choose a reason for hiding this comment

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

Now there's a state.marked_text as well as a markedText field. They seem to store the same text. I have a specific idea of resolving this. Would you mind if I added changes to this PR?

If you would not like me to add commits to this PR, that's completely okay, but then please remove either state.marked_text or markedText. Preferrably we would only have the NSMutableAttributedString stored within the ViewState.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Welcome your patch !
I'm also confused there are duplicated information for markedText and state.marked_text.
markedText is referred in setMarkedText but I'm not sure how to remove this because I'm not familiar with Obj-c and type system between Obj-c and Rust.

I appreciate your help.

@ArturKovacs
Copy link
Contributor

It should be fine, but please test as thoroughly as you can, because I may have broken something.

@komi1230
Copy link
Contributor Author

komi1230 commented Aug 6, 2021

This works fine and looks really simple !
Thanks !
This seems ready to merge !

@komi1230
Copy link
Contributor Author

komi1230 commented Aug 6, 2021

#1669 is duplicated against this PR.
It may be no problem to close.

@ArturKovacs ArturKovacs merged commit 8afeb91 into rust-windowing:master Aug 9, 2021
@ArturKovacs
Copy link
Contributor

Oh no. I forgot about the changelog. Maybe I'll make another PR just for updating the changelog

@garasubo
Copy link
Contributor

@komi1230 If you are interested in #1497, please help implementing the APIs for composition events. The APIs itself are defined and implemented for X11 and Wayland, but no implementation for other platforms.

@komi1230
Copy link
Contributor Author

Sure! When I tackle this issue, I refered your implementation for X11 and Wayland. I'd like to return the favor. I'll also try.

ArturKovacs added a commit to ArturKovacs/winit that referenced this pull request Dec 29, 2021
This reverts commit 8afeb91.

Reverting because this change made pinyin input unusable
(only latin characters showed even after selecting the
desired Chinese character)
ArturKovacs added a commit to ArturKovacs/winit that referenced this pull request Dec 29, 2021
This reverts commit 8afeb91.

Reverting because this change made pinyin input unusable
(only latin characters showed even after selecting the
desired Chinese character)
ArturKovacs added a commit to ArturKovacs/winit that referenced this pull request Dec 31, 2021
This reverts commit 8afeb91.

Reverting because this change made pinyin input unusable
(only latin characters showed even after selecting the
desired Chinese character)
ArturKovacs added a commit that referenced this pull request Jan 2, 2022
This reverts commit 8afeb91.

Reverting because this change made Pinyin input unusable
(only latin characters showed even after selecting the
desired Chinese character)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C - waiting on maintainer A maintainer must review this code DS - macos
Development

Successfully merging this pull request may close these issues.

None yet

5 participants