Wlroots: wlr_keyboard_set_keymap releases all modifiers

[acrisci]

If the keymap is set in the modifiers handler, the xkb keyboard state will not reflect the state of the keys physically pressed on the keyboard. Upon the next keypress when modifiers are updated with wlr_keyboard.c::keyboard_modifier_update(), xkb always reports the modifiers are not pressed.

Setting the modifiers while keys are pressed is necessary to support proposed features in sway. A keyboard may be attached to different seats at the same time with different layouts per seat. For instance, attaching a keyboard to seat0 with us layout and attaching a keyboard to seat1 with a de layout will create two seat events for the same keyboard keypress event with each different layouts being reported to seat clients.

Modifiers are broken in rootston with this diff:

diff --git a/rootston/keyboard.c b/rootston/keyboard.c
index 4aaf2d4..b35f2fb 100644
--- a/rootston/keyboard.c
+++ b/rootston/keyboard.c
@@ -276,6 +276,7 @@ void roots_keyboard_handle_key(struct roots_keyboard *keyboard,

 void roots_keyboard_handle_modifiers(struct roots_keyboard *r_keyboard) {
    struct wlr_seat *seat = r_keyboard->seat->seat;
+   wlr_keyboard_set_keymap(r_keyboard->device->keyboard, r_keyboard->device->keyboard->keymap);
    wlr_seat_set_keyboard(seat, r_keyboard->device);
    wlr_seat_keyboard_notify_modifiers(seat);
 }

[acrisci]

If keyboard layout is now a per-seat-keyboard concept rather than a per-keyboard concept, then maybe all layout handling and xkb logic should be moved to the seat. To support this feature, there is no longer one xkb state per keyboard, but rather a number of xkb states, one for each seat.

Replace wlr_keyboard_set_keymap() with wlr_seat_keyboard_set_keymap() and move all state associated with xkb to the seat. This will allow the seat keyboard to keep a persistent keyboard layout rather than reporting extra layout switches to every seat client.

This has the effect of making wlr-keyboard extremely thin and almost stateless. It will become a simple router from input device events to keyboard events. We might want to consider removing this interface from wlroots and simply listening to keypress events on the input device itself since almost all of the responsibilities will be moved to the seat.

[ddevault]

That seems like a reasonable design to me, but I don't think we should remove wlr-keyboard entirely.

[acrisci]

A better way to do it might be to separate the wlr-keyboard from the device so that one device can power multiple wlr-keyboards and then just create multiple wlr-keyboards per layout per seat for one device.

[ddevault]

I'm not sure how that'd look in practice, but we can see how it pans out if you like that approach.

[emersion]

Can't we just fix this bug by calling xkb_state_update_key with the currently pressed keys (wlr_keyboard::keycodes) when the keyboard state is reset when changing the layout? (We'd need to release all keyboard keys before destroying the old state)