more complete input method support #2053

lilydjwg · 2023-12-07T06:33:51Z

This series of patches add the following features:

keyboard grab: the compositor sends all key events to input method for process
IM popup: map and place the input method popup window at the expected place

There is an issue with the protocol that input method doesn't know about focus changing and we don't have a method to reset its state (v1 has which KDE uses, but v2 not), so a workaround (d60beee) is implemented.

This works for fcitx5 and alacritty, gtk3 and gtk4. Qt doesn't work because it doesn't support text input v3.

This fixes #1160.

fixes WayfireWM#1160.

We pass all key events to input method after plugins, but don't pass input method generated ones to plugins since we should have already done that. This conflicts with onscreen keyboards, but unfortunately no better way in sight.

lilydjwg · 2023-12-07T15:39:20Z

This won't work well for onscreen keyboards because we can't distinguish if a key event is sent due to input method not process it or the user presses an onscreen keyboard.

ammen99

Thanks for your work so far, especially for splitting into meaningful commits, this was of great help to read and understand the changes :)

I have left some comments where I have failed to follow the logic (it could well be because I am not very familiar with the protocols in question, so please correct me if some of my comments are wrong) or where I had some ideas for improvements.

src/core/seat/input-method-relay.cpp

src/core/seat/input-method-relay.hpp

src/core/seat/input-method-relay.cpp

ammen99 · 2023-12-07T15:39:59Z

src/core/seat/input-method-relay.cpp

+    // on focus change, we disable on one text input and enable on another
+    // however, the former may also send us a disable request AFTER we've done the above
+    // let's just ignore repeated disable reuqests.
+    if (input == already_disabled_input)


input has an input->active field which keeps track of whether the input was activated or not. Maybe we can use that instead of keeping track of this ourselves? Wlroots automatically updates input->active so we wouldn't need already_disabled_input at all if I understand correctly.

input->current_enabled is still true on destruction.

I am not not sure what input->current_enabled is, but I am pretty sure that input->active in wlroots is doing exactly what you need here: it is set only when deactive() is called (which is also when you will set already_disabled_input). Not to mention that the current approach seems a bit hacky, what if there are multiple text inputs or something of the sort?

Oh you mean the input method struct, not the text input one... Yes, it works! I'll update the code.

There are full of hacks in the implementation of Linux input method unfortunately. It never gets better. We thank god when it works (compared to those doesn't work, e.g. Steam).

src/core/seat/input-method-relay.cpp

src/core/seat/keyboard.cpp

ammen99 · 2023-12-07T15:54:50Z

src/core/seat/input-method-relay.cpp

@@ -100,6 +100,7 @@ wf::input_method_relay::input_method_relay()
        on_new_popup_surface.disconnect();
        input_method  = nullptr;
        keyboard_grab = nullptr;
+        last_keyboard_resource = nullptr;


Unfortunately, I am not quite sure why last_keyboard_resource is needed as well, can you elaborate?

Also, you wrote that this interferes with OSKs, in what ways exactly?

keyboard_grab will be null on unfocus, so I keep keyboard_grab->resource for comparison.

When the input method also provide an OSK, we can't distinguish if the input method is passing a user-pressed key back, or the user presses the OSK provided by the input method.

Sorry for pestering you with questions, but I would like to understand the code before merging ..

So could you please give me a concrete example of the events which go between the compositor and clients, where we will get the wrong result if we did not save the last_keyboard_resource. Something like: "first user focuses a window, opens virtual keyboard which provides text input, user presses XYZ, user switches away to another surface, in the code foo() is triggered, we get Z in the other window".

create a window and activate IM

open another window

close the window with keyboard (e.g. super+c from wayfire, or ctrl+d for a terminal)

Some key release events will be received by the first window twice (if we return false in is_im_sent in case input_method->keyboard_grab is null). It doesn't seem to cause problems though.

Where do the releasing the key twice come from? Once from the IM and once from the physical release, or am I missing something?

By the way, https://github.com/WayfireWM/wayfire/blob/master/src/core/seat/keyboard.cpp#L40 - is this maybe the reason why it works in practice?

Once from the IM and once from the physical release

Yes.

https://github.com/WayfireWM/wayfire/blob/master/src/core/seat/keyboard.cpp#L40 - is this maybe the reason why it works in practice?

It seems so. If the events aren't sent to client twice, it shouldn't matter. But I prefer to keep the processing in the same place and make the is_im_sent method accurate.

Alright, I see your point. Now on to the technical realization: I see that you are storing keyboard_grab->resource. Is it not technically possible that the resource is destroyed but we still keep holding a reference to it? last_keyboard_resource seems to be reset on input-method-destroy, which to my understanding can be also happening at a much later point than grab destroy.

Another thing: when you call wl_resource_get_client() in is_im_sent, if the IM has not used a grab, the chosen resource will be NULL, right? Wouldn't that lead to a crash when you do a wl_resource_get_client()?

Last but not least, in the current implementation, a grab is grabbed forever (in the sense that the keys will never reach the clients, because is_im_sent returns true) - because last_keyboard_resource is cleared only when the whole IM is destroyed, right? This is a protocol violation to my understanding.

I think in general it might make much more sense to just filter out the few release events in the end (like we already do in core, if that is not enough, feel free to do something similar in the input method relay).

Is it not technically possible that the resource is destroyed but we still keep holding a reference to it?

I think so, but it should be fine as long as it isn't dereferenced.

when you call wl_resource_get_client() in is_im_sent, if the IM has not used a grab, the chosen resource will be NULL, right?

virtual_keyboard should also be null in this case, so we've already returned early. I can add a null check just in case.

a grab is grabbed forever (in the sense that the keys will never reach the clients, because is_im_sent returns true) - because last_keyboard_resource is cleared only when the whole IM is destroyed, right?

is_im_sent only returns true when the key events are from the IM. These keys will never reach the IM or the compositor's shortcut keys logic, but they will reach the clients using text inputs.

The IM needs to bypass key events, and it does so by sending it via a virtual keyboard. We need to ensure that these events aren't sent to the IM again and get through to the text input clients. This roundabout is caused by the deficiencies of the input method v2 protocol.

virtual_keyboard should also be null in this case, so we've already returned early. I can add a null check just in case.

This is not necessary. You can have a standalone virtual keyboard which is not connected to an IM or whatever. see for example wf-osk, or wayvnc.

I think so, but it should be fine as long as it isn't dereferenced.

Sure, but it is dereferenced when we try to get its client.

To check both cases, simply start a virtual-keyboard-only client like wf-osk and you will certainly get a crash (at least if you run with address sanitizer). I just checked :)

is_im_sent only returns true when the key events are from the IM. These keys will never reach the IM or the compositor's shortcut keys logic, but they will reach the clients using text inputs.

But these keys will not reach applications if they are coming from a virtual keyboard, but not from the IM after the grab has ended.

Yes, you're right. I'm switching to use pid to identify the input method.

lilydjwg · 2023-12-07T16:38:24Z

Thanks for your review. I'll look into other issues tomorrow.

…o onscreen ones

src/api/wayfire/view.hpp

ammen99 · 2023-12-25T07:33:37Z

src/core/seat/input-method-relay.cpp

+    // on focus change, we disable on one text input and enable on another
+    // however, the former may also send us a disable request AFTER we've done the above
+    // let's just ignore repeated disable reuqests.
+    if (input == already_disabled_input)


I am not not sure what input->current_enabled is, but I am pretty sure that input->active in wlroots is doing exactly what you need here: it is set only when deactive() is called (which is also when you will set already_disabled_input). Not to mention that the current approach seems a bit hacky, what if there are multiple text inputs or something of the sort?

src/core/seat/input-method-relay.cpp

ammen99 · 2023-12-25T09:31:15Z

src/core/seat/input-method-relay.cpp

@@ -100,6 +100,7 @@ wf::input_method_relay::input_method_relay()
        on_new_popup_surface.disconnect();
        input_method  = nullptr;
        keyboard_grab = nullptr;
+        last_keyboard_resource = nullptr;


Sorry for pestering you with questions, but I would like to understand the code before merging ..

So could you please give me a concrete example of the events which go between the compositor and clients, where we will get the wrong result if we did not save the last_keyboard_resource. Something like: "first user focuses a window, opens virtual keyboard which provides text input, user presses XYZ, user switches away to another surface, in the code foo() is triggered, we get Z in the other window".

ammen99 · 2023-12-26T16:43:31Z

By the way, there is something weird in the compilation, the CI does not catch it because we use unity pch builds which obviously changes some stuff .. Here is a patch to make everything compile:

diff --git a/src/core/seat/input-method-relay.cpp b/src/core/seat/input-method-relay.cpp
index 3d804496..972a87db 100644
--- a/src/core/seat/input-method-relay.cpp
+++ b/src/core/seat/input-method-relay.cpp
@@ -1,7 +1,7 @@
 #include <wayfire/util/log.hpp>
 #include "input-method-relay.hpp"
 #include "../core-impl.hpp"
-#include "../../output/output-impl.hpp"
+#include "../../view/view-impl.hpp"
 #include "wayfire/scene-operations.hpp"
 
 #include <algorithm>
diff --git a/src/core/seat/input-method-relay.hpp b/src/core/seat/input-method-relay.hpp
index 3f012bf7..d951460c 100644
--- a/src/core/seat/input-method-relay.hpp
+++ b/src/core/seat/input-method-relay.hpp
@@ -1,10 +1,10 @@
 #pragma once
-#include "seat-impl.hpp"
 #include "wayfire/util.hpp"
 #include "wayfire/signal-definitions.hpp"
 #include "wayfire/view.hpp"
 #include <wayfire/nonstd/wlroots-full.hpp>
 #include <wayfire/unstable/translation-node.hpp>
+#include <wayfire/unstable/wlr-surface-node.hpp>
 
 #include <vector>
 #include <memory>

because last_keyboard_resource may be invalid when we need to use it.

lilydjwg · 2023-12-27T08:11:41Z

All previous issues should have been addressed.

ammen99 · 2023-12-27T09:18:05Z

@lilydjwg Most changes look fine, however, PIDs are often times unreliable and up to recently weren't even available on all OSes. What do you think about the following patch (I haven't tested but I hope it works):

diff --git a/src/core/seat/input-method-relay.cpp b/src/core/seat/input-method-relay.cpp
index b1a7d445..bd55e3c1 100644
--- a/src/core/seat/input-method-relay.cpp
+++ b/src/core/seat/input-method-relay.cpp
@@ -2,9 +2,11 @@
 #include "input-method-relay.hpp"
 #include "../core-impl.hpp"
 #include "../../view/view-impl.hpp"
+#include "core/seat/seat-impl.hpp"
 #include "wayfire/scene-operations.hpp"
 
 #include <algorithm>
+#include <wayland-server-core.h>
 
 wf::input_method_relay::input_method_relay()
 {
@@ -18,9 +20,6 @@ wf::input_method_relay::input_method_relay()
     on_input_method_new.set_callback([&] (void *data)
     {
         auto new_input_method = static_cast<wlr_input_method_v2*>(data);
-        auto client = wl_resource_get_client(new_input_method->resource);
-        wl_client_get_credentials(client, &pid, NULL, NULL);
-
         if (input_method != nullptr)
         {
             LOGI("Attempted to connect second input method");
@@ -111,7 +110,6 @@ wf::input_method_relay::input_method_relay()
         on_new_popup_surface.disconnect();
         input_method  = nullptr;
         keyboard_grab = nullptr;
-        pid = 0;
 
         auto *text_input = find_focused_text_input();
         if (text_input != nullptr)
@@ -230,10 +228,28 @@ bool wf::input_method_relay::is_im_sent(wlr_keyboard *kbd)
         return false;
     }
 
-    auto client = wl_resource_get_client(virtual_keyboard->resource);
-    pid_t vkbd_pid;
-    wl_client_get_credentials(client, &vkbd_pid, NULL, NULL);
-    return vkbd_pid == pid;
+    // We have already identified the device as IM-based device
+    auto device_impl = (wf::input_device_impl_t*)kbd->base.data;
+    if (device_impl->is_im_keyboard)
+    {
+        return true;
+    }
+
+    if (this->input_method)
+    {
+        // This is a workaround because we do not have sufficient information to know which virtual keyboards
+        // are connected to IMs
+        auto im_client = wl_resource_get_client(input_method->resource);
+        auto vkbd_client = wl_resource_get_client(virtual_keyboard->resource);
+
+        if (im_client == vkbd_client)
+        {
+            device_impl->is_im_keyboard = true;
+            return true;
+        }
+    }
+
+    return false;
 }
 
 bool wf::input_method_relay::handle_key(struct wlr_keyboard *kbd, uint32_t time, uint32_t key,
diff --git a/src/core/seat/input-method-relay.hpp b/src/core/seat/input-method-relay.hpp
index af6b6fb5..1d686475 100644
--- a/src/core/seat/input-method-relay.hpp
+++ b/src/core/seat/input-method-relay.hpp
@@ -23,7 +23,6 @@ class input_method_relay
         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;
-    pid_t pid = 0;
     text_input *find_focusable_text_input();
     void set_focus(wlr_surface*);
 
diff --git a/src/core/seat/seat-impl.hpp b/src/core/seat/seat-impl.hpp
index ad19ba1f..2c00c250 100644
--- a/src/core/seat/seat-impl.hpp
+++ b/src/core/seat/seat-impl.hpp
@@ -5,7 +5,6 @@
 #include <wayfire/seat.hpp>
 #include <set>
 
-#include "../../view/surface-impl.hpp"
 #include "wayfire/output.hpp"
 #include "wayfire/input-device.hpp"
 #include "wayfire/scene-input.hpp"
@@ -23,11 +22,13 @@ class input_device_impl_t : public wf::input_device_t
 {
   public:
     input_device_impl_t(wlr_input_device *dev);
-    virtual ~input_device_impl_t() = default;
+    virtual ~input_device_impl_t();
 
     wf::wl_listener_wrapper on_destroy;
     virtual void update_options()
     {}
+
+    bool is_im_keyboard = false;
 };
 
 class pointer_t;
diff --git a/src/core/seat/seat.cpp b/src/core/seat/seat.cpp
index c04f448d..40ee3ad6 100644
--- a/src/core/seat/seat.cpp
+++ b/src/core/seat/seat.cpp
@@ -1,15 +1,12 @@
 #include "seat-impl.hpp"
 #include "cursor.hpp"
-#include "wayfire/compositor-view.hpp"
 #include "wayfire/geometry.hpp"
-#include "wayfire/opengl.hpp"
 #include "../core-impl.hpp"
 #include "../view/view-impl.hpp"
 #include "keyboard.hpp"
 #include "pointer.hpp"
 #include "touch.hpp"
 #include "input-manager.hpp"
-#include "wayfire/render-manager.hpp"
 #include "wayfire/output-layout.hpp"
 #include <wayfire/util/log.hpp>
 #include "wayfire/scene-input.hpp"
@@ -593,6 +590,12 @@ wf::input_device_impl_t::input_device_impl_t(wlr_input_device *dev) :
         wf::get_core_impl().input->handle_input_destroyed(this->get_wlr_handle());
     });
     on_destroy.connect(&dev->events.destroy);
+    this->handle->data = this;
+}
+
+wf::input_device_impl_t::~input_device_impl_t()
+{
+    this->handle->data = NULL;
 }
 
 static wf::pointf_t to_local_recursive(wf::scene::node_t *node, wf::pointf_t point)

…abled_input" This reverts commit 1682ced. input_method->active doesn't work for this purpose (see comments)

lilydjwg · 2023-12-30T08:54:08Z

Patch applied. Also I reverted the input->active change because it doesn't work in some cases, see comments.

ammen99 · 2023-12-30T10:53:35Z

Also I reverted the input->active change because it doesn't work in some cases, see comments.

Sounds like we don't need to keep track of this manually (because it is just a hack, what would happen if you have 3 windows and very quickly switch input and destroy some of them? I am almost certain you will trigger the problem condition again). Instead, I would assume that we should return early in disable_text_input() if the input is not the currently focused input. That sounds like it would fix the problematic case you discovered, would it not?

…urrently focused input

lilydjwg · 2023-12-30T12:07:05Z

Yes, it sounds good and works.

ammen99

Looks great now, thanks a lot!

* implement input method keyboard grab fixes #1160. * support input method popups * input method: avoid disabling a just-enabled text input on focus change * workaround focus change while preediting * input method: don't grab keyboard if no text input is focused * input method: fix conflicts with compositor shortcut keys We pass all key events to input method after plugins, but don't pass input method generated ones to plugins since we should have already done that. This conflicts with onscreen keyboards, but unfortunately no better way in sight. * input method: address reviews * use at place_popup_at function to convert from relative coordinates to onscreen ones * input method: simplify key handling code * fix style * convert coordinates type * input method: use input_method->active instead of already_disabled_input * move place_popup_at to view-impl * input method: explain the focus change workaround in detail * input method: use pid to identify the input method sent key events because last_keyboard_resource may be invalid when we need to use it. * fix headers * input method: better ways to identify IM sent keys * Revert "input method: use input_method->active instead of already_disabled_input" This reverts commit 1682ced. input_method->active doesn't work for this purpose (see comments) * input method: don't send deactivate command if the input is not the currently focused input * fix code style

lilydjwg added 5 commits December 7, 2023 23:31

implement input method keyboard grab

8f53847

fixes WayfireWM#1160.

support input method popups

e5699cb

input method: avoid disabling a just-enabled text input on focus change

eb4760a

workaround focus change while preediting

316955b

input method: don't grab keyboard if no text input is focused

74443a5

lilydjwg force-pushed the im branch from cae1bf6 to 91bdd93 Compare December 7, 2023 15:32

lilydjwg force-pushed the im branch from 91bdd93 to 371600a Compare December 7, 2023 15:35

ammen99 requested changes Dec 7, 2023

View reviewed changes

lilydjwg added 5 commits December 8, 2023 15:47

input method: address reviews

e0fce8a

use at place_popup_at function to convert from relative coordinates t…

880bb88

…o onscreen ones

input method: simplify key handling code

d60b7bb

fix style

a09a560

convert coordinates type

6ce370d

ammen99 requested changes Dec 25, 2023

View reviewed changes

input method: use input_method->active instead of already_disabled_input

1682ced

ammen99 mentioned this pull request Dec 26, 2023

disable unity builds in one of the CIs #2081

Merged

lilydjwg added 4 commits December 27, 2023 16:04

move place_popup_at to view-impl

302838f

input method: explain the focus change workaround in detail

cda1913

input method: use pid to identify the input method sent key events

7940111

because last_keyboard_resource may be invalid when we need to use it.

fix headers

c302c35

lilydjwg added 2 commits December 30, 2023 15:44

input method: better ways to identify IM sent keys

5f0ce1f

Revert "input method: use input_method->active instead of already_dis…

8a9cf96

…abled_input" This reverts commit 1682ced. input_method->active doesn't work for this purpose (see comments)

input method: don't send deactivate command if the input is not the c…

91e8f06

…urrently focused input

fix code style

bfaef7a

ammen99 approved these changes Dec 30, 2023

View reviewed changes

ammen99 merged commit 78b560b into WayfireWM:master Dec 30, 2023
4 checks passed

ammen99 mentioned this pull request Feb 26, 2024

Add support for input_method_v2 popups #1299

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

more complete input method support #2053

more complete input method support #2053

lilydjwg commented Dec 7, 2023 •

edited

Loading

lilydjwg commented Dec 7, 2023

ammen99 left a comment

ammen99 Dec 7, 2023

lilydjwg Dec 8, 2023

ammen99 Dec 25, 2023

lilydjwg Dec 26, 2023 •

edited

Loading

ammen99 Dec 7, 2023

lilydjwg Dec 7, 2023

ammen99 Dec 25, 2023

lilydjwg Dec 26, 2023

ammen99 Dec 26, 2023

lilydjwg Dec 26, 2023

ammen99 Dec 26, 2023

lilydjwg Dec 26, 2023

ammen99 Dec 26, 2023

lilydjwg Dec 27, 2023

lilydjwg commented Dec 7, 2023

ammen99 Dec 25, 2023

ammen99 Dec 25, 2023

ammen99 commented Dec 26, 2023

lilydjwg commented Dec 27, 2023

ammen99 commented Dec 27, 2023

lilydjwg commented Dec 30, 2023

ammen99 commented Dec 30, 2023

lilydjwg commented Dec 30, 2023

ammen99 left a comment

more complete input method support #2053

more complete input method support #2053

Conversation

lilydjwg commented Dec 7, 2023 • edited Loading

lilydjwg commented Dec 7, 2023

ammen99 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilydjwg Dec 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilydjwg commented Dec 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ammen99 commented Dec 26, 2023

lilydjwg commented Dec 27, 2023

ammen99 commented Dec 27, 2023

lilydjwg commented Dec 30, 2023

ammen99 commented Dec 30, 2023

lilydjwg commented Dec 30, 2023

ammen99 left a comment

Choose a reason for hiding this comment

lilydjwg commented Dec 7, 2023 •

edited

Loading

lilydjwg Dec 26, 2023 •

edited

Loading