app: improve IME composition handling - #171
Conversation
SnippetCmd can grow to include surrounding text while an IME composition is active. If the selection has not moved and the old snippet still matches the new one, don't cancel the platform composition. Signed-off-by: qiannian <qianniancn@gmail.com>
Send the editor's visible composition bounds with SelectionCmd and use them as the CFS_EXCLUDE rectangle for ImmSetCandidateWindow. That puts the candidate list below the text being composed, instead of just under the caret. Handle RESULTSTR and COMPSTR as separate IME updates. Cursor-only updates move the IME cursor without replacing text in the editor. Fixes: https://todo.sr.ht/~eliasnaur/gio/697 Signed-off-by: qiannian <qianniancn@gmail.com>
whereswaldon
left a comment
whereswaldon
left a comment
There was a problem hiding this comment.
Thank you very much for working on this. I have a couple requests, but they aren't very complex:
- The implementation of the windows IME event handling feels like it could be made easier to comprehend by a refactor focusing on returning early from each category of message. I may have misunderstood the constraints on the implementation though, so feel free to correct me if my requests there aren't possible/practical.
- The way that the editor is now updating the IME state seems too expensive to do every frame; I'd like to more precisely do that work only when we must.
Overall, I think this is looking really promising. Thanks again.
Signed-off-by: qiannian <qianniancn@gmail.com>
4d8d111 to
23e823d
Compare
whereswaldon
left a comment
whereswaldon
left a comment
There was a problem hiding this comment.
Thank you, this is now merged.
To git.sr.ht:~eliasnaur/gio
18034696..c7e3d6c1 main -> main
There is still an open comment I'd like to discuss with @eliasnaur , so I'm not going to close out the PR just yet.
| e.text.regions = nil | ||
| change() | ||
| e.updateIMEState(gtx) | ||
| if len(e.text.regions) == 0 { | ||
| t.Fatalf("%s: changed IME state did not traverse text regions", name) | ||
| } |
There was a problem hiding this comment.
I dislike using e.text.regions to detect whether or not we've handled the IME change. It's using an implementation detail to detect a side-effect of the change we actually care about. That being said, right now we can't easily mock the input.Source within the gtx so that we could directly detect the expected key.SelectionCmd. @eliasnaur, This feels like an important hole in our ability to test widgets. Am I missing a strategy for checking whether a widget issued a command, or are tests essentially unable to observe that?
There was a problem hiding this comment.
This concern is not blocking the acceptance of this PR, for the record. I just want to discuss it with @eliasnaur, and this situation is a good motivating example.
|
Thanks for merging this. I agree that using e.text.regions in the test is a bit awkward. I couldn’t find a way to observe key.SelectionCmd directly. Is there a better pattern for this? |
2 participants
Place the Windows IME candidate window under the active composition text.
Also avoid cancelling IME composition when a snippet update only expands
around matching text.
Fixes: https://todo.sr.ht/~eliasnaur/gio/697