Implement input-method keyboard grab

[xdavidwu]

I'm interested in implementing CJK input method in wayland so I try to implement this.
I've tested with my local client code but it's not suitable for a test example.
Should I extend examples/input-method.c for this or write a new one

[ddevault]

This looks like a great start. Can you rebase it against master? Whether or not to extend examples/input-method.c or write a new example is at your discretion.

[ddevault]

Is there a particular IME software you're looking to implement this for? ibus?

[xdavidwu]

I plan to implement a new IME with libchewing, not using existing frameworks like ibus or fcitx.

[ddevault]

Oh, nice. Hoping for anthy support too 😄

[xdavidwu]

I found that virtual-keyboard-unstable-v1 states that it can accompany the input method protocol, so that means in input-method keyboard grab, we should not intercept events generated by virtual-keyboard, right?

[ddevault]

You should intercept those events, I think. Virtual keyboards behave just like any other kind.

[xdavidwu]

Then I think there is no way for a input method to send unhandled key events back to the surface. Should we add an interface to input-method protocol for this?

[ddevault]

I'm not sure I fully understand the problem. Can you explain it in more detail?

[xdavidwu]

I have been writing a experimental input-method client that grabs, and I found that I need to send key events that are not handled by input-method back to the application for things like moving the cursor to already committed text or some keyboard shortcuts like ctrl+s. If I use virtual-keyboard for that, the events would be caught by the input-method again.

[ddevault]

Is the key forwarding request in input-method not sufficient?

[xdavidwu]

There is only event that send key to input method, but no request method to send it back (when unhandled) to the surface that requested text input.

[ddevault]

https://gitlab.freedesktop.org/wayland/wayland-protocols/blob/master/unstable/input-method/input-method-unstable-v1.xml#L145-196

[xdavidwu]

Oh I didn't know that v1 has it. In input-method-unstable-v2 bundled in protocols/ it is missing.

[ddevault]

Ah, I see. I think we did split that off, now that you mention it. Let's say that virtual keyboards should bypass input-method, then. Virtual keyboards which want to do i18n input should probably use input-method themselves, I think that was the idea.

[xhebox]

@xdavidwu Good work. I am trying to modify wlchewing, or almost rewrite a new one based libpinyin probably. But I found a bug with the current implementation. The workaround patch first:

--- wlroots-0.10.0/types/wlr_keyboard_group.c   2020-01-23 02:22:04.000000000 +0800
+++ wlroots-0.10.0/types/wlr_keyboard_group.c   2020-01-23 02:22:04.000000000 +0800
@@ -134,7 +134,13 @@
                wl_list_insert(&group->keys, &key->link);
        }
 
+       if (group_device->keyboard->virtual_keyboard != NULL) {
+               group_device->keyboard->group->keyboard.virtual_keyboard = group_device->keyboard->virtual_keyboard;
+       }
        wlr_keyboard_notify_key(&group_device->keyboard->group->keyboard, data);
+       if (group_device->keyboard->virtual_keyboard != NULL) {
+               group_device->keyboard->group->keyboard.virtual_keyboard = NULL;
+       }
 }
 
 static void handle_keyboard_modifiers(struct wl_listener *listener,
@@ -157,8 +163,14 @@
                }
        }
 
+       if (group_device->keyboard->virtual_keyboard != NULL) {
+               group_device->keyboard->group->keyboard.virtual_keyboard = group_device->keyboard->virtual_keyboard;
+       }
        wlr_keyboard_notify_modifiers(&group_device->keyboard->group->keyboard,
                        mods.depressed, mods.latched, mods.locked, mods.group);
+       if (group_device->keyboard->virtual_keyboard != NULL) {
+               group_device->keyboard->group->keyboard.virtual_keyboard = NULL;
+       }
 }
 
 static void handle_keyboard_keymap(struct wl_listener *listener, void *data) {
@@ -176,7 +188,13 @@
                }
        }
 
+       if (group_device->keyboard->virtual_keyboard != NULL) {
+               group_device->keyboard->group->keyboard.virtual_keyboard = group_device->keyboard->virtual_keyboard;
+       }
        wlr_keyboard_set_keymap(&keyboard->group->keyboard, keyboard->keymap);
+       if (group_device->keyboard->virtual_keyboard != NULL) {
+               group_device->keyboard->group->keyboard.virtual_keyboard = NULL;
+       }
 }
 
 static void handle_keyboard_repeat_info(struct wl_listener *listener,
@@ -196,8 +214,14 @@
                }
        }
 
+       if (group_device->keyboard->virtual_keyboard != NULL) {
+               group_device->keyboard->group->keyboard.virtual_keyboard = group_device->keyboard->virtual_keyboard;
+       }
        wlr_keyboard_set_repeat_info(&keyboard->group->keyboard,
                        keyboard->repeat_info.rate, keyboard->repeat_info.delay);
+       if (group_device->keyboard->virtual_keyboard != NULL) {
+               group_device->keyboard->group->keyboard.virtual_keyboard = NULL;
+       }
 }
 
 static void refresh_state(struct keyboard_group_device *device,

I am not familiar with the internal mechanics of wlroots, I can not tell the full picture. But, this is basically what happened: I have a native keyboard, and a virtual keyboard. So sway is using keyboard_group to multiplex events. Thus, what we grab is the fake wlr_keyboard in keybaord_group object. But that fake wlr_keyboard was not with virtual_keyboard pointer. So your sway grab code will not work at all.

BTW: I do not think the current implementation is "good" enough.

1. It seems that, when you grab the keyboard, keymap events are not ignored like key/modifiers. I still need a check to avoid infinite recursion. And the protocol xml file talks nothing about that. It will be good if all events avoid recursion automatically.
2. Grab implementation in sway is not consistent on the behavior of ignoring. Key events will check the resource while modifiers events are not. Check swaywm/sway@7471b43#diff-d7aa39a56641abe2c1362ca922ba299bR453-R455 and swaywm/sway@7471b43#diff-d7aa39a56641abe2c1362ca922ba299bR556.
3. I think grab implementation should be at the side of wlroots. It is weird that you expose the pointer of virtual_keyboard. Though it now works somehow, there must be a better way to implement, IMO.

EDIT: I can confirm that grab interface and ime interface are working in a pure wayland environment(no x11/GLX at all). I can type things in a patched pure-wayland(cairo-gtk3-wayland) firefox. I will continue my work this minimal IME and exam popup interface. Because I really can not find a working native IME.

[xdavidwu]

wlr_keyboard_group is a helper to reduce keyboard events. For example, assuming there is a keyboard group g1 consisting of two keyboards k1 and k2, and the original events sequence look like this: (with the same key) k1 pressed, k2 pressed, k1 released, k2 released.
Without helps from wlr_keyboard_group, compositor will send all events to client.
With wlr_keyboard_group, compositor would only send events at the first pressed and the last released.
When keyboard group is enabled, sway listens on both individual and grouped signals to handle bindings but only grouped signal will be sent to clients.

Keyboards with same keymap can be grouped.

In keyboard grab + virtual keyboard combo, both virtual and the original keyboard device will likely have the same keymap and currently will be grouped. But the events here should not be reduced. In the above example, replace k1 with original keyboard and k2 with virtual keyboard, k1 should be sent to input-method grab and k2 should be sent to text-input client.

I think the ideal solution is not to group the virtual one when it is a keyboard grab + virtual keyboard combo. I will find a way to do this.

A temporarily workaround would be turn off grouping in sway config, by seat * keyboard_grouping none.

As for the keymap issue, I think we should avoid sending identical keymaps to client in wlr_seat_set_keyboard. This can be a seperated PR.

Grab implementation in sway is not consistent on the behavior of ignoring. Key events will check the resource while modifiers events are not.

Nice catch, I will fix it soon.

I implemented the grab in wlroots initially but decided to move it to sway to make use of pressed event sent state tracking to avoid cut off a press-release pair.

Edit: I recalled it incorrectly about how keymaps are sent in my code. It is by a new function I wrote. I will update it to avoid sending identical keymaps soon.

[emersion]

Should we send a protocol error here? If it's missing, we should add it to the protocol.

[emersion]

Overall looks good.

The interactions between IME keyboard grabs and other grabs are complicated. I'll think a bit more about this.

[emersion]

Is this ready for another review round?

[emersion]

Let's say that virtual keyboards should bypass input-method, then.

That means a VNC session will bypass the input-method, too. I wonder if that's desirable.

[xdavidwu]

Let's say that virtual keyboards should bypass input-method, then.

That means a VNC session will bypass the input-method, too. I wonder if that's desirable.

I've make only virtual keyboard from the same client as grab bypass input-method grab on sway side.

[emersion]

I've make only virtual keyboard from the same client as grab bypass input-method grab on sway side.

This isn't going to work in all cases. For instance a user using Waypipe would have all Wayland objects belonging to the same client. On the long run I think we'll want to add a way for clients to tie a virtual-keyboard to an input-method.

[emersion]

With these remaining comments fixed, this LGTM!

[emersion]

This isn't going to work in all cases. For instance a user using Waypipe would have all Wayland objects belonging to the same client. On the long run I think we'll want to add a way for clients to tie a virtual-keyboard to an input-method.

Created a follow-up issue: #2322

[emersion]

I've squashed all of your commits into three logical commits. Also made a minor change to avoid breaking the API: added back wlr_input_method_v2.seat.

[emersion]

Code LGTM. Thanks for working on this!