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

Jump to bottom

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 1 commit
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
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