Add XkbContext::update_mask and XkbContext::set_update_key

[ids1024]

For a nested compositor, it's useful to be able to pass through the modifiers state from the parent compositor unchanged. set_mask provides a way to do that. Meanwhile set_update_key() provides a way to disable the current calls to xkb_context_updatE_key.

Alternately to set_update_key(), a boolean argument could be added to both input() and input_intercept(). Not sure which is better. Either is a bit ugly.

update_mask uses seperate arguments for depressed/latched/locked instead of SerializedMods, since effective isn't applicable here. It uses u32 for those, like SerializedMods, but Layout for the layout since it exists. Xkbcommon has a separate depressed/latched/locked layout, but we only use one since that's what the set_layout function and wl_keyboard::modifiers already assumes. (I guess X11 differentiated these, but depressed/latched didn't end up being used in practice?)

This could also be used on the X11 backend. I'm not sure if winit exposes the necessary information to call set_mask.

[sodiboo]

For a nested compositor, it's useful to be able to pass through the modifiers state from the parent compositor unchanged.

This isn't just useful, it's required for a correctly behaving implementation. It's also worth noting that this isn't just required for nesting compositors, but also for implementing zwp_virtual_keyboard_v1::modifiers (yeah, there's an impl in smithay but it depends on internals, this PR would allow a non-internal impl to exist), as well as ei_keyboard::modifiers

---

I think the set_update_key() function is poorly named. I guess it's supposed to mean "whether or not to call xkb_state_update_key", but i think it could have a better name (especially given that it does more than just control the call to xkb_state_update_key). i think something like freeze_state() or freeze_mods_state() might be more descriptive (of course with an inverted meaning of the parameter in that case). that is if you will keep this as a function call on the KeyboardHandle. i have no strong opinion on whether it should have an entirely differently shaped API, but i'm content with the current state of the PR, since it works fine for my implementation

[ids1024]

Yeah, this is necessary for correct behavior. At least if you want the nested compositor to behave as a native part of the parent environment with the same latched/locked/group. I guess depressed state could also be addressed by treating keys as released on wl_keyboard::leave and handling the keys passed to wl_keyboard::enter. Which might make sense when running a full compositor as window for testing.

Yeah, the name is weird. I'm not sure about something like freeze_mods_state either though. To me that also seems confusing, and doesn't express that this disables the automatic updating of the xkb state on key presses (but it should still be updated by some other method for correct behavior).

[Drakulix]

Maybe instead of the ackward set_update_key could we have two variants of key_input? One that does update the xkb state and one that doesn't? I don't have particularly good ideas for names, but even just key_input and key_input_no_xkb_update feel better than this weird boolean.

[ids1024]

KbdInternal::key_input is a private function, called by KeyboadHandle::input_intercept. Which is also called by KeyboardHandle::input.

I was also thinking of a second function variant, but that would require a separate version of both input_intercept and input. Adding a bool argument to both would probably be better than ending up with 4 functions. Though input() already has a somewhat complicated function signature without adding an extra bool to it.

So probably either something like set_update_key, or a new argument to those functions, unless anyone has a better idea about the input API generally.

[sodiboo]

thought for a different API shape:

rename the mods function to something like set_forced_xkb_state() and it will automatically disable the internal calculation; to the consumer they're adding an override value that is always returned instead. and then a separate function like clear_forced_xkb_state() can unlock it for interpretation again.

is this necessarily better? no, you may discuss it. however, I thought of it and I think it's at least worthy of consideration.

this would work well for a nested compositor (since we might accidentally implement it correctly and not even realize that the xkb state would've been recomputed normally), but not so much if we mix and match emulated and libinput events to the same keyboard handle. but then again, the existing API is also not mixable like that. they should be separate keyboards.

having two separate input functions or a param there would make it easier to mix real and fake input to the same keyboard, but then race conditions can and will cause incorrect modifier states if the virtual keyboard sends a modifier state and then a keypress. they really should be separate even in this case.

[Drakulix]

KbdInternal::key_input is a private function, called by KeyboadHandle::input_intercept. Which is also called by KeyboardHandle::input.

Looking at those docs, these aren't very clear to begin with, we have

• KeyboardHandle::input with:

Handle a keystroke

All keystrokes from the input backend should be fed in order to this method of the keyboard handler. It will internally track the state of the keymap.

• and KeyboardHandle::input_intercept with

Update the state of the keyboard without forwarding the event to the focused client

Between "update the state of the keyboard" and "track the state of the keymap", it is difficult to figure out what exactly is happening in there.

Additionally your code with update_key seems to still track pressed_keys just not the xkb-state? So clearly we do update state.

Maybe we should split the input forwarding updating the xkb-state into two completely separate user-facing functions?

So no combining the two and no variants, just one function to update the xkb-state with a key-event (e.g. update_key) and alternatively update_mask plus what is currently input_forward. (Which begs the question, can update_mask trigger a shortcut?)