fix(linux): fix context after typing Bksp with Wayland#15820
fix(linux): fix context after typing Bksp with Wayland#15820ermshiperete wants to merge 8 commits intomasterfrom
Conversation
This fixes a problem with the context after typing Backspace when using Wayland. Wayland uses double-buffering for the text, so we always have to commit after making changes. Fixes: #15676
keymanapp-test-bot
bot
added
the
user-test-missing
User Test ResultsTest specification and instructions User tests are not required |
| g_autofree gchar *debug = NULL; | ||
| g_message("DAR: %s - %s", __FUNCTION__, debug = debug_utf8_with_codepoints(string)); | ||
| text = ibus_text_new_from_static_string (string); | ||
| text = ibus_text_new_from_string(string); |
There was a problem hiding this comment.
Devin.ai:
Change from ibus_text_new_from_static_string to ibus_text_new_from_string is a correct fix
The old code on line 639 used ibus_text_new_from_static_string(string), which assumes the passed string has static lifetime (it does NOT copy the string). However, all three callers of commit_string pass dynamically-allocated strings: ibus_keyman_set_text passes D-Bus text, process_output_action passes g_ucs4_to_utf8 output, and commit_current_queue_item passes char_buffer. The change to ibus_text_new_from_string (which copies the string) is strictly safer. The old code likely worked in practice because ibus_engine_commit_text serializes the text over D-Bus before returning, but it was technically incorrect.
mcdurdin
left a comment
mcdurdin
left a comment
There was a problem hiding this comment.
I think this LGTM. Might be good to add a comment on the purpose of is_dirty
| // This is not always reliable: IBus detects whether the client supports | ||
| // surrounding text by emitting the retrieve-surrounding signal. As part | ||
| // of that signal handler, the client is expected to call | ||
| // gtk_im_context_set_surrounding_with_selection which ends calling | ||
| // set_context_if_needed at a time when IBus is still in the middle of | ||
| // determining whether the client supports surrounding text. |
There was a problem hiding this comment.
This feels a little bit like a TODO item. Is there some way we can make this reliable?
There was a problem hiding this comment.
I've no idea how we could improve this on the keyman side.
There was a problem hiding this comment.
Seems like re-entrancy is the main problem? I am not 100% clear if you are saying that when we call this, that process may not have completed, or something else?
There was a problem hiding this comment.
Things are complicated. I tried to add a document that shows the sequence of events, but that doesn't give the full picture either. I thought we could use engine->enabled to know if IBus has the correct values, but that's still too early. When we get a keydown we can rely on the value from client_supports_surrounding_text , but before that we might get a wrong value.
Anyway, what I'm trying to say with this code comment is that client_supports_surrounding_text is not always 100% reliable and getting a wrong value is expected and hasn't caused problems IIRC.
There was a problem hiding this comment.
(see also the sequence diagram which I added to this PR.)
Co-authored-by: Marc Durdin <marc@durdin.net>
Addresses code review comment.
keymanapp-test-bot
bot
removed
the
user-test-missing
3 participants
This fixes a problem with the context after typing Backspace when using Wayland. Wayland uses double-buffering for the text, so we always have to commit after making changes.
This is only a partial fix, so we can't really test this.
Part-of: #15676
Test-bot: skip