slint-ui / slint

Slint is a declarative GUI toolkit to build native user interfaces for Rust, C++, or JavaScript apps.
https://slint.dev
Other
17.54k stars 600 forks source link

Bug: Input method , if press esc in the lineedit, the preview edit will not be clean #1925

Closed Decodetalkers closed 1 year ago

Decodetalkers commented 1 year ago

Use input method , and then press esc, the preview text will not be cleaned image

else: the line edit seems always remember the first option of my input method, and set it , it will not remember the method used previous.. at least, it should be always set as not selected..

tronical commented 1 year ago

Thanks for your report! Which operating system are you using, and what's the input method?

Decodetalkers commented 1 year ago

Thanks for your report! Which operating system are you using, and what's the input method?

Archlinux, with sway im patch, input method is fcitx5

tronical commented 1 year ago

I think this is legit. The esc works with the input method on macOS, for example with winit we receive an IME event with an empty pre-edit and it is cleared. But I would not be surprised if that's missing with ibus for example. Commit 8cba0622f58890bc36d9d00da3a6c46e5edce166 mentions a similar caveat: Since mouse events aren't forwarded by winit, clicking during pre-edit gives funny results.

wengxt commented 1 year ago

Hi. fcitx5 dev here. I think this is definitely a bug in slint.

In most case, the "esc" is not thing special comparing to any other key event. In most case, when fcitx receives a esc, the engine logic will send a request to clear the preedit text. I'm testing the examples/gallery here and it seems to run with my system Qt library. This works with regular Qt app, but not in slint.

It seems that certain QInputMethodEvent is not handled correctly.

Preedit string can be empty, and if it's empty, it means hide the preedit.

I believe the bug in the qt_window.rs and below is a naive fix to demonstrate the idea.

diff --git a/internal/backends/qt/qt_window.rs b/internal/backends/qt/qt_window.rs
index e2f226890..2c0489500 100644
--- a/internal/backends/qt/qt_window.rs
+++ b/internal/backends/qt/qt_window.rs
@@ -278,16 +278,14 @@ struct SlintWidget : QWidget {
                 preedit_cursor: i32 as "int"] {
                     let runtime_window = WindowInner::from_pub(&rust_window.window);

-                    if !preedit_string.is_empty() {
-                        let event = KeyInputEvent {
-                            event_type: KeyEventType::UpdateComposition,
-                            text: preedit_string.to_string().into(),
-                            preedit_selection_start: replacement_start as usize,
-                            preedit_selection_end: replacement_start as usize + replacement_length as usize,
-                            ..Default::default()
-                        };
-                        runtime_window.process_key_input(event);
-                    }
+                    let event = KeyInputEvent {
+                        event_type: KeyEventType::UpdateComposition,
+                        text: preedit_string.to_string().into(),
+                        preedit_selection_start: replacement_start as usize,
+                        preedit_selection_end: replacement_start as usize + replacement_length as usize,
+                        ..Default::default()
+                    };
+                    runtime_window.process_key_input(event);

                     if !commit_string.is_empty() {
                         let event = KeyInputEvent {
tronical commented 1 year ago

Thank you @wengxt for chiming in and the elaboration - this is much appreciated. I think your patch makes sense, I've applied it.

I did manage now to get fcixt5 working. I'm now using our Qt backend for testing this. What I observe is that when pressing escape, the preedit string is indeed empty, but the commit string is set. Consequently we end up clearing the preedit string anyway and we commit what the IME supplied.

I've made a quick clip to show what I'm seeing:

https://user-images.githubusercontent.com/1486/209104174-6c7fb6fe-589c-4b12-b36b-9847a38500a4.mov

I added debug output with the preedit and commit strings:

IME preedit string a commit string 
IME preedit string as commit string 
IME preedit string asd commit string 
IME preedit string asdf commit string 
IME preedit string  commit string asdf <-- Received when pressing escape
IME preedit string  commit string 

Does this also look like correct behavior to you?

Since you're also seeing the wrong behavior, would you be able to describe to me what I need to set up in my fcixt5 config panel to reproduce this?

Decodetalkers commented 1 year ago

Thank you @wengxt for chiming in and the elaboration - this is much appreciated. I think your patch makes sense, I've applied it.

I did manage now to get fcixt5 working. I'm now using our Qt backend for testing this. What I observe is that when pressing escape, the preedit string is indeed empty, but the commit string is set. Consequently we end up clearing the preedit string anyway and we commit what the IME supplied.

I've made a quick clip to show what I'm seeing: Screen.Recording.2022-12-22.at.10.33.46.mov

I added debug output with the preedit and commit strings:

IME preedit string a commit string 
IME preedit string as commit string 
IME preedit string asd commit string 
IME preedit string asdf commit string 
IME preedit string  commit string asdf <-- Received when pressing escape
IME preedit string  commit string 

Does this also look like correct behavior to you?

Since you're also seeing the wrong behavior, would you be able to describe to me what I need to set up in my fcixt5 config panel to reproduce this?

I have try this commit, and it seems to have solve the problem of preview text. another problem is that the text area always remember the first option of input method,

About how to setup fcitx5, you can take the page as a reference

wengxt commented 1 year ago

Thank you @wengxt for chiming in and the elaboration - this is much appreciated. I think your patch makes sense, I've applied it.

I did manage now to get fcixt5 working. I'm now using our Qt backend for testing this. What I observe is that when pressing escape, the preedit string is indeed empty, but the commit string is set. Consequently we end up clearing the preedit string anyway and we commit what the IME supplied.

I've made a quick clip to show what I'm seeing: Screen.Recording.2022-12-22.at.10.33.46.mov

I added debug output with the preedit and commit strings:

IME preedit string a commit string 
IME preedit string as commit string 
IME preedit string asd commit string 
IME preedit string asdf commit string 
IME preedit string  commit string asdf <-- Received when pressing escape
IME preedit string  commit string 

Does this also look like correct behavior to you?

Since you're also seeing the wrong behavior, would you be able to describe to me what I need to set up in my fcixt5 config panel to reproduce this?

As long as you follow that QInputMethodEvent says, it should be fine. The engine itself may choose whatever it think the right thing to do upon Escape. Pinyin may choose to clear preedit, while others may commit the preedit.

The issue described in https://github.com/slint-ui/slint/issues/1925#issuecomment-1362831652 , is more about missing the required interaction between input method.

QInputMethod provides a set of API that allow application to notify input method to do something, including: QInputMethod::reset (reset the state) , QInputMethod::commit (commit current preedit), QInputMethod::update ( notify input method the return value of inputMethodQuery changes).

See Qt own code about how those are being used by Qt native widget: e.g. https://codebrowser.dev/qt5/qtbase/src/widgets/widgets/qwidgetlinecontrol_p.h.html#249

E.g. when a widget about lost focus, you may consider call QInputMethod::commit to commit any pending preedit string. When a line edit is being set with a new string progmatically, consider call QInputMethod::reset to reset the input method state (commit may also works, depending on how you think of it).

ogoffart commented 1 year ago

Now with https://github.com/slint-ui/slint/pull/3000 , i do think this should be fixed.

Decodetalkers commented 1 year ago

Now with #3000 , i do think this should be fixed.

oh, yes, this is fixed, I will close this issue, but I have found another issue about input method, I will open a new one