Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some input method fixes #2111

Closed
wants to merge 3 commits into from
Closed

Some input method fixes #2111

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dbd98d3
input method: fix interaction with plugin grabs
lilydjwg Jan 10, 2024
49d1218
input method: don't send a release event without a corresponding pres…
lilydjwg Jan 19, 2024
52eb742
input method: fix first commit for non-preediting applications
lilydjwg Jan 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 35 additions & 4 deletions src/core/seat/input-method-relay.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,11 +60,20 @@ wf::input_method_relay::input_method_relay()
// 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.
if (focus_just_changed)
//
// 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())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at the protocol and I believe what we are doing here is not a real fix for the issue. The protocol includes serials which is what we really need here. So here's the deal as far as I understand it:

  • When we switch focus or do something of the sort, we can send a done event. This generates a new serial number, serial numbers are counted from 0 to infinity and are incremented for each done event. I briefly looked through wlroots implementation but it might be wrong though (because they reset input_method->current_serial on each commit, which might actually have outdated information!). So we might need to keep track of it ourselves.

  • For each commit from the text input, we receive the last serial that the input method has received so far (wlroots stores that in input_method->current_serial before emitting the commit event). So this means: until the input method has actually received our new configuration, it will send a commit with an outdated serial.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I tried to add a done event after set_focus. It sends a commit with a correct serial. It doesn't make any difference whether we've changed focus. The input method receives a sequence of done events and counts it correctly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I tried to add a done event after set_focus. It sends a commit with a correct serial. It doesn't make any difference whether we've changed focus. The input method receives a sequence of done events and counts it correctly.

Did you also try to check whether the serial belongs to the current focus or is an older event in the commit handler?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually there is already a done event when switching focus, and so by adding another done event, the input method replies with two commits, with the same serial.

What do you mean by the serial belongs to the current focus? The serial is just a counter of done events. (I'm reading WAYLAND_DEBUG here.)

{
LOGI("focus_just_changed, ignore input method commit");
focus_just_changed = false;
return;
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;
}
}

auto *text_input = find_focused_text_input();
Expand Down Expand Up @@ -136,6 +145,7 @@ wf::input_method_relay::input_method_relay()

on_grab_keyboard_destroy.set_callback([&] (void *data)
{
this->pressed_keys.clear();
on_grab_keyboard_destroy.disconnect();
keyboard_grab = nullptr;
});
Expand Down Expand Up @@ -221,6 +231,22 @@ bool wf::input_method_relay::should_grab(wlr_keyboard *kbd)
return !is_im_sent(kbd);
}

bool wf::input_method_relay::check_superfluous_release(uint32_t key, uint32_t state)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, now that you moved the handle_key() call to seat.cpp, doesn't the seat already do this filtering of keys for you automatically? Isn't dbd98d3#diff-206c23fa3adf45f1a58ee9a5865dc0dec21bd2ba72a2441fb41d3fa88794cb6aR45-R59 executed for the keys here, so why need to check here again?

Note I find the whole IM stuff quite confusing so far because it seems everything is a mess (not on Wayfire's side but the protocols & clients & how they work together), so maybe my intuition is wrong ..

Also, do you have any references as to how gnome implements this or kwin? Do they also have so many checks and workarounds for the various brokenness in the protocols? I am kinda hoping there is an 'easy' solution to all the troubles that you've tried to fix here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so why need to check here again?

That's exactly why again. Take the Firefox case as example:

  1. No text input in focus. The user sends keydown ctrl and keydown l.
  2. Firefox focuses the address as requested. Text input in focus now.
  3. The user sends keyup l.
  4. Input method passes through the keyup l, and sends via virtual keyboard to the compositor.
  5. In the code you linked, this second keyup l is ignored because from the compositor's view it's already been released.
  6. Firefox never receives the keyup l event, and so auto repeat kicks in and a lot of l is inserted.

Also, do you have any references as to how gnome implements this or kwin?

The answer is super simple: they don't. GNOME uses D-Bus, and kwin uses input method v1 (ref).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The answer is super simple: they don't. GNOME uses D-Bus, and kwin uses input method v1 (ref).

Wayland at its finest :((

5. In the code you linked, this second keyup l is ignored because from the compositor's view it's already been released.

Ok, so repeating the flow of events as to how I see it to avoid misunderstandings:

  1. Keydown ctrl, keydown L
  2. Focus text input
  3. IM grabs keyboard
  4. keyup L -> we go to the code I linked -> L is erased from the set -> L is sent to the IM, not to the client
  5. IM sends virtual keyup L to compensate -> we go to the linked code -> L is not in the set, so it is dropped

I would say that the actual problem is that we erased L from the set in step 4. The set is supposed to contain all pressed, but not released keys for the current focus. Since the key was forwarded to the IM and not to the client, then L should have never been removed from the set.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the key was forwarded to the IM and not to the client, then L should have never been removed from the set.

Yes, it works nicely!

Copy link
Contributor Author

@lilydjwg lilydjwg Jan 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh no. With text input focused, super+some key to switch focus will leave super pressed...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to be some bad interactions with the IM. I did some change to fcitx5 and this seemed to no longer happen.

{
if (state == WL_KEYBOARD_KEY_STATE_PRESSED)
{
this->pressed_keys.insert(key);
return false;
} else if (auto it = this->pressed_keys.find(key);it != this->pressed_keys.end())
{
this->pressed_keys.erase(it);
return false;
} else
{
return true;
}
}

bool wf::input_method_relay::is_im_sent(wlr_keyboard *kbd)
{
struct wlr_virtual_keyboard_v1 *virtual_keyboard = wlr_input_device_get_virtual_keyboard(&kbd->base);
Expand Down Expand Up @@ -261,6 +287,11 @@ bool wf::input_method_relay::handle_key(struct wlr_keyboard *kbd, uint32_t time,
return false;
}

if (check_superfluous_release(key, state))
{
return false;
}

wlr_input_method_keyboard_grab_v2_set_keyboard(keyboard_grab, kbd);
wlr_input_method_keyboard_grab_v2_send_key(keyboard_grab, time, key, state);
return true;
Expand Down
7 changes: 5 additions & 2 deletions src/core/seat/input-method-relay.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

#include <vector>
#include <memory>
#include <chrono>

namespace wf
{
Expand All @@ -22,7 +23,7 @@ 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;
bool focus_just_changed = false;
std::optional<std::chrono::time_point<std::chrono::steady_clock>> last_focus_changed;
text_input *find_focusable_text_input();
void set_focus(wlr_surface*);

Expand All @@ -37,10 +38,12 @@ class input_method_relay
set_focus(nullptr);
}

focus_just_changed = true;
last_focus_changed = std::chrono::steady_clock::now();
};

bool should_grab(wlr_keyboard*);
std::multiset<uint32_t> pressed_keys;
bool check_superfluous_release(uint32_t key, uint32_t state);

public:

Expand Down
34 changes: 14 additions & 20 deletions src/core/seat/keyboard.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,9 @@ void wf::keyboard_t::setup_listeners()
}

seat->priv->set_keyboard(this);
if (!handle_keyboard_key(ev->time_msec, ev->keycode,
ev->state) && (mode == input_event_processing_mode_t::FULL))
auto is_im_sent = wf::get_core_impl().im_relay->is_im_sent(handle);
if ((is_im_sent || !handle_keyboard_key(ev->keycode, ev->state)) &&
(mode == input_event_processing_mode_t::FULL))
{
if (ev->state == WL_KEYBOARD_KEY_STATE_PRESSED)
{
Expand All @@ -57,7 +58,16 @@ void wf::keyboard_t::setup_listeners()
}
}

if (seat->priv->keyboard_focus)
// don't send IM sent keys to plugin grabs
if (!seat->priv->is_grab)
{
if (wf::get_core_impl().im_relay->handle_key(handle, ev->time_msec, ev->keycode, ev->state))
{
return;
}
}

if (seat->priv->keyboard_focus && !(seat->priv->is_grab && is_im_sent))
{
seat->priv->keyboard_focus->keyboard_interaction()
.handle_keyboard_key(wf::get_core().seat.get(), *ev);
Expand Down Expand Up @@ -287,19 +297,13 @@ bool wf::keyboard_t::has_only_modifiers()
return true;
}

bool wf::keyboard_t::handle_keyboard_key(uint32_t time, uint32_t key, uint32_t state)
bool wf::keyboard_t::handle_keyboard_key(uint32_t key, uint32_t state)
{
using namespace std::chrono;

auto& input = wf::get_core_impl().input;
auto& seat = wf::get_core_impl().seat;

if (wf::get_core_impl().im_relay->is_im_sent(handle))
{
mod_binding_key = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am confused, isn't mod_binding_key supposed to be reset on im keys? With this change, it no longer is reset.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm restoring the whole wf::keyboard_t::handle_keyboard_key function to previous state and moving the IM code to the on_key callback.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok but the modifier binding key should be reset even for grabbed events. It is used to detect when a modifier binding is pressed (e.g super pressed and released shortly after). Any keys in between reset the modifier binding, grabbed or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be reset later. It was here just because we returned early here.

return false;
}

bool handled_in_plugin = false;
auto mod = mod_from_key(key);
input->locked_mods = this->get_locked_mods();
Expand Down Expand Up @@ -328,11 +332,6 @@ bool wf::keyboard_t::handle_keyboard_key(uint32_t time, uint32_t key, uint32_t s

handled_in_plugin |= wf::get_core().bindings->handle_key(
wf::keybinding_t{get_modifiers(), key}, mod_binding_key);

if (!handled_in_plugin)
{
handled_in_plugin |= wf::get_core_impl().im_relay->handle_key(handle, time, key, state);
}
} else
{
if (mod_binding_key != 0)
Expand All @@ -349,11 +348,6 @@ bool wf::keyboard_t::handle_keyboard_key(uint32_t time, uint32_t key, uint32_t s
}
}

if (!handled_in_plugin)
{
handled_in_plugin |= wf::get_core_impl().im_relay->handle_key(handle, time, key, state);
}

mod_binding_key = 0;
}

Expand Down
2 changes: 1 addition & 1 deletion src/core/seat/keyboard.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ class keyboard_t

std::chrono::steady_clock::time_point mod_binding_start;

bool handle_keyboard_key(uint32_t time, uint32_t key, uint32_t state);
bool handle_keyboard_key(uint32_t key, uint32_t state);

/** Get the current locked mods */
uint32_t get_locked_mods();
Expand Down
1 change: 1 addition & 0 deletions src/core/seat/seat-impl.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ struct seat_t::impl

void set_keyboard_focus(wf::scene::node_ptr keyboard_focus);
wf::scene::node_ptr keyboard_focus;
bool is_grab = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am highly sceptical of such approaches as you might have noticed .. what makes grab nodes special here? Why is this not a problem for other nodes (normal client surfaces)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it intercepts the client's processing. Consider the following user events:

  1. keydown alt
  2. keydown tab
  3. keyup tab

The switcher plugin activates at event 2. Now the text input is unfocused, so the input releases all pressed keys, sending keyup tab and keyup alt. The latter causes switcher to think the user has released the alt key.

With 49d1218 the switcher case seems to be fixed, but there may be other complex interactions with other plugins.

Copy link
Member

@ammen99 ammen99 Jan 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so the input releases all pressed keys, sending keyup tab and keyup alt. The latter causes switcher to think the user has released the alt key.

I think I am starting to see what is going on, but why on earth does fcitx5 think it needs to generate these key releases? It is not supposed to do so, at least I don't think why it would need that. I'm suspecting that here we're really adding a workaround for an fcitx5 bug.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could just ignore all IM virtual keyboard events (whether to grab or not) until we switch focus? Just like we ignore commits until the focused text input switches properly. Because like now imagine alt-tab was not a shortcut which opens a grab, but immediately would switch and give focus to the next window. During normal interaction, that would be (this is what an imaginary plugin MIGHT do, I'm not saying we have a plugin which does this atm :)):

  1. press alt, press tab
  2. old window loses keyboard focus (no need for release events)
  3. new window get keyboard focus and is told that alt/tab are pressed on enter
  4. user releases alt, tab -> new window gets the release events

So what fcitx5 does would be wrong in this case, because fcitx5 generates additional release events between 2 and 3 (according to your description, if I understood it correctly)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why on earth does fcitx5 think it needs to generate these key releases?

It seems to workaround a sway bug....fcitx/fcitx5@6f92a0c

// Keys sent to the current keyboard focus
std::multiset<uint32_t> pressed_keys;
void transfer_grab(wf::scene::node_ptr new_focus);
Expand Down
2 changes: 2 additions & 0 deletions src/core/seat/seat.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -500,6 +500,7 @@ 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;
Expand All @@ -521,6 +522,7 @@ 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());
Expand Down
Loading