Some input method fixes (take 2) #2113
Conversation
|
Continuing our discussion from the previous PR, I was thinking of using the serials like this: diff --git a/src/core/seat/input-method-relay.cpp b/src/core/seat/input-method-relay.cpp
index 9689ba9d..e7386534 100644
--- a/src/core/seat/input-method-relay.cpp
+++ b/src/core/seat/input-method-relay.cpp
@@ -50,30 +50,16 @@ wf::input_method_relay::input_method_relay()
auto evt_input_method = static_cast<wlr_input_method_v2*>(data);
assert(evt_input_method == input_method);
- // FIXME: workaround focus change while preediting
+ // When we switch focus, we send a done event to the IM.
+ // The IM may need time to process further events and may send additional commits after switching
+ // focus, which belong to the old text input.
//
- // With input method v2, we have no way to notify the input method that
- // input focus has changed. The input method maintains its state, and
- // will bring it to the new window, i.e. a half-finished preedit string
- // from the old window will be brought to the new one. This is undesired.
- //
- // We ignore such commit requests so it doesn't have any affect on the
- // new window. Even when the previous window isn't preediting when
- // switching focus, it doesn't have any bad effect to the new window anyway.
- //
- // Preediting can be disabled (selectively or globally) because
- // application bugs were observed. In this case, we see only commits
- // but no preedit strings, so we need care the timing.
- static std::chrono::milliseconds focus_change_duration{100};
- if (last_focus_changed.has_value())
+ // To prevent this, we simply ignore commits which do not acknowledge the newest done event from the
+ // compositor.
+ if (input_method->current_serial < last_done_serial.value_or(0))
{
- auto elapsed = std::chrono::steady_clock::now() - *last_focus_changed;
- last_focus_changed.reset();
- if (elapsed < focus_change_duration)
- {
- LOGI("focus just changed, ignore input method commit");
- return;
- }
+ LOGD("focus just changed, ignore input method commit");
+ return;
}
auto *text_input = find_focused_text_input();
@@ -173,7 +159,13 @@ void wf::input_method_relay::send_im_state(wlr_text_input_v3 *input)
wlr_input_method_v2_send_content_type(input_method,
input->current.content_type.hint,
input->current.content_type.purpose);
- wlr_input_method_v2_send_done(input_method);
+ send_im_done();
+}
+
+void wf::input_method_relay::send_im_done()
+{
+ last_done_serial = next_done_serial;
+ next_done_serial++;
}
void wf::input_method_relay::disable_text_input(wlr_text_input_v3 *input)
diff --git a/src/core/seat/input-method-relay.hpp b/src/core/seat/input-method-relay.hpp
index 036b730d..eca8cf55 100644
--- a/src/core/seat/input-method-relay.hpp
+++ b/src/core/seat/input-method-relay.hpp
@@ -8,7 +8,6 @@
#include <vector>
#include <memory>
-#include <chrono>
namespace wf
{
@@ -23,7 +22,11 @@ class input_method_relay
on_input_method_new, on_input_method_commit, on_input_method_destroy,
on_grab_keyboard, on_grab_keyboard_destroy, on_new_popup_surface;
wlr_input_method_keyboard_grab_v2 *keyboard_grab = nullptr;
- std::optional<std::chrono::time_point<std::chrono::steady_clock>> last_focus_changed;
+
+ std::optional<uint32_t> last_done_serial;
+ uint32_t next_done_serial = 0;
+ void send_im_done();
+
text_input *find_focusable_text_input();
void set_focus(wlr_surface*);
@@ -37,8 +40,6 @@ class input_method_relay
{
set_focus(nullptr);
}
-
- last_focus_changed = std::chrono::steady_clock::now();
};
bool should_grab(wlr_keyboard*);Does this not work to prevent the old state from interfering with the new focus when changing focus? |
…perfluous release events These events may be sent by the IM, and meant to reach the client. check: * in firefox, no text input, press ctrl-l, there should not be continuous l being inserted into the urlbar * in firefox, with text input, press super+some key to switch workspace and back (but don't release super inbetween). click in firefox, super should not be consider pressed * with text input, does the switcher plugin activate and deactivate correctly
IM sends release key events when deactivated, which caused e.g. the switcher plugin to deactivate early. The plugin has a higher priority when activating, but lower for subsequent key events.
fcitx5 no longer commit_string on deactivating.
|
We can get rid of the workaround now. fcitx5 no longer commits that string. |
I think we still should do the thing I linked with the serial check. Namely, imagine the following scenario:
Of course this is a pretty rare race condition but it still might happen. So it would be great if you could test it and see whether anything breaks, if not, I think it should stay in that reduced form. |
|
Tried with the patch. It didn't work. I mean, it does no good nor bad (I used a previous commit of fcitx5 to test the issue). Here's the debug log from fcitx5, grepping (In the log, I only typed Btw, there was a missing |
Ok yeah .. with the new investigation you did on fcitx5 it turns out that this workaround couldn't have fixed the earlier problem because they were sending the old state even after deactivation. If the workaround (with your fix, thanks for it) doesn't harm anything, I'd like to include it as well simply because it might prevent a rare race-condition (which is apparently different from the bug you were experiencing before). Also I see that you still do the thing with grabs, is this still really needed when fcitx5 doesn't send fake release events? |
Yes I can include it, as long as I don't find issues in the following days.
It wouldn't need but fcitx5 does send them. From the developer of fcitx5, sway seems to no longer need these release events but hyprland definitely needs them. Also I got two times that an |
Maybe not. There was an issue that a backspace delete to the start in DDNet (a sdl2 game). I didn't investigate before it was gone due to some changes I'm not sure. |
Or we'll alternate between two keyboards, sending a lot of keymap and repeat_info events to all clients.
|
@lilydjwg I see that fcitx5 got a new commit which disables release events for non-repeat keys like the alt modifier. So probably we don't have to differentiate between grabs and non-grabs anymore. |
|
Thanks for letting me know. I'll try a few days without that. |
|
switcher still behaves unexpected: |
Tab should never be sent to the IM in the first place. |
|
Oh, I realized we get race conditions between the IM and the compositor. Thanks for testing this, I'll consider a bit more. |
I realized that an alt release can also cause switcher to finish, so I'm not sure what happened. I'll do logging later. |
|
I don't understand, but it is still the alt release event: |
|
@ammen99 Hello, could you review this pr ? It works and solve some issues for me. |
…t method reconnects
|
@lilydjwg I am looking at this again and I do not see why diff --git a/src/core/seat/keyboard.cpp b/src/core/seat/keyboard.cpp
index 026b4a43..e95ccc0a 100644
--- a/src/core/seat/keyboard.cpp
+++ b/src/core/seat/keyboard.cpp
@@ -65,8 +65,7 @@ void wf::keyboard_t::setup_listeners()
}
}
- // don't send IM sent keys to plugin grabs
- if (seat->priv->keyboard_focus && !(seat->priv->is_grab && is_im_sent))
+ if (seat->priv->keyboard_focus)
{
seat->priv->keyboard_focus->keyboard_interaction()
.handle_keyboard_key(wf::get_core().seat.get(), *ev);
diff --git a/src/core/seat/seat-impl.hpp b/src/core/seat/seat-impl.hpp
index f668bb31..2c00c250 100644
--- a/src/core/seat/seat-impl.hpp
+++ b/src/core/seat/seat-impl.hpp
@@ -57,7 +57,6 @@ struct seat_t::impl
void set_keyboard_focus(wf::scene::node_ptr keyboard_focus);
wf::scene::node_ptr keyboard_focus;
- bool is_grab = false;
// Keys sent to the current keyboard focus
std::multiset<uint32_t> pressed_keys;
void transfer_grab(wf::scene::node_ptr new_focus);
diff --git a/src/core/seat/seat.cpp b/src/core/seat/seat.cpp
index b47b32be..40ee3ad6 100644
--- a/src/core/seat/seat.cpp
+++ b/src/core/seat/seat.cpp
@@ -500,7 +500,6 @@ void wf::seat_t::impl::transfer_grab(wf::scene::node_ptr grab_node)
}
this->keyboard_focus = grab_node;
- this->is_grab = true;
grab_node->keyboard_interaction().handle_keyboard_enter(wf::get_core().seat.get());
wf::keyboard_focus_changed_signal data;
@@ -522,7 +521,6 @@ void wf::seat_t::impl::set_keyboard_focus(wf::scene::node_ptr new_focus)
}
this->keyboard_focus = new_focus;
- this->is_grab = false;
if (new_focus)
{
new_focus->keyboard_interaction().handle_keyboard_enter(wf::get_core().seat.get());Also for comparison, here is how I can reproduce the alt-tab bug on Wayfire master: https://github.com/ammen99/wayfire-tests/blob/im-tests/tests/im/fcitx5-switcher/main.py The test runs successfully if I use this PR, but also if I apply the patch. |
|
Can the test assert that the plugin is active before releasing the alt key? |
Oh, I see, yes. Somehow this slipped my radar .. I'll think whether there is a better way to fix the problem or not. Thanks :) |
This is continuation for #2111.
This is a draft as I haven't tested it for long. Also, it needs a fcitx5 patch (to not send superfluous release events).