feat: add IME preedit support #2198
Conversation
There was a problem hiding this comment.
Once the issues are fixed, I think we can go with this, since it’s better than nothing.
But there will still be problems, like word wrapping, especially on the last line. The colors might also not be as expected in all places. So I think we eventually should change the implementation to something else.
When I was thinking about this previously, I had the idea using the regula neovim insert mode for displaying the text, with a lua side plugin helping to deal with that. For example to handle aborts, or selecting another entry. The cursor would also be native, and the selection could use a custom highlight group or perhaps a built in, again, using the lua plugin to mark it.
That way we would not need any special rendering in Neovide, and word wrapping/scrolling of the buffer would happen automatically. We could even integrate proper code completion that way.
| // } => { | ||
| // self.ime_preedit = (text.to_string(), *cursor_offset); | ||
| // log::trace!("Ime preedit {text}"); | ||
| // } |
There was a problem hiding this comment.
I would prefer to keep the ime state in the keyboard manager. And I also think that this breaks something, since we used to block keyboard inputs when the pre-edit was active, and that code is no longer effective.
| if position != self.ime_position { | ||
| self.ime_position = position; | ||
| if position != self.ime.position() { | ||
| // self.ime.position = position; |
There was a problem hiding this comment.
This commented code should be removed
| Event::WindowEvent { | ||
| event: WindowEvent::Ime(ime), | ||
| .. | ||
| } => match ime { |
There was a problem hiding this comment.
The clippy warnings should be fixed
| &self.text | ||
| } | ||
| pub fn cursor_offset(&self) -> Option<usize> { | ||
| if let Some(offset) = self.cursor_offset { |
There was a problem hiding this comment.
This clippy warning should be fixed
|
|
||
| #[inline] | ||
| pub fn position(&self) -> PhysicalPosition<i32> { | ||
| return self.position; |
There was a problem hiding this comment.
This clippy warning should be fixed
| // draw ime if possible under current cursor | ||
| if let Some(preedit) = preedit { | ||
| let factor = grid_renderer.font_dimensions.width as f32; | ||
| let y_adjustment = y_adjustment as f32 + factor / 5.0; |
There was a problem hiding this comment.
What does this factor of 5.0 do?
| start, | ||
| self.destination.y + y_adjustment, | ||
| end, | ||
| self.destination.y + y_adjustment + factor / 4.0, |
There was a problem hiding this comment.
What does this factor 4.0 do?
| canvas.draw_text_blob(blob, (start, self.destination.y + y_adjustment), &paint); | ||
| } | ||
|
|
||
| paint.set_color(skia_safe::colors::GREY.to_color()); |
There was a problem hiding this comment.
This should use the correct highlight group color, otherwise it might be invisible. I think this is the selection right?
Also, shouldn’t it be drawn before the text? So that the text remain invisible, with the text color also set apporopriately through the highlight groups.
I also think that the floating window transparency need to be taken into account.
| @@ -145,7 +147,7 @@ impl Renderer { | |||
| self.cursor_renderer.prepare_frame() | |||
| } | |||
|
|
|||
| pub fn draw_frame(&mut self, root_canvas: &Canvas, dt: f32) { | |||
| pub fn draw_frame(&mut self, root_canvas: &Canvas, dt: f32, ime: &Ime, w: f32) { | |||
There was a problem hiding this comment.
Can you give the the w parameter a more descriptive name? We don’t use the shorthand for width anywhere else?
|
|
||
| let mut start = self.destination.x; | ||
| let mut end = self.destination.x + off; | ||
| if end + factor > w { |
There was a problem hiding this comment.
I think ideally we should use the neovim grid window width here, not the global window width, otherwise it will render outside floating window and splits for example.
This als means that the code better belongs in rendered_window, where the width and tranaparency needed is available.
|
very sorry for the late reply. I am a lot bit busy currently. |
|
Don't worry, I think it's quite close, if we just want an initial version that is usable, but indeed there's a lot to think about and fix if we want it to work perfectly. You don't need to hurry though. And if you want to attempt the other way, using Neovim insert mode, I can give you a more detailed design of what I had planned. I think that's the best way of doing it, but the implementation is more complex, at least to get started. |
What kind of change does this PR introduce?
IME preedit is something lost in Neovide.
This pr support ime preedit.
Screen.Recording.2023-12-25.at.01.24.08.mp4
Did this PR introduce a breaking change?