suggestion: allow xkb_state_update_mask() to accept negative groups to unlock group wrapping functionality

[bam80]

libxkbcommon/src/state.c

Lines 792 to 798 in f60bdb1

	xkb_state_update_mask(struct xkb_state *state,
	xkb_mod_mask_t base_mods,
	xkb_mod_mask_t latched_mods,
	xkb_mod_mask_t locked_mods,
	xkb_layout_index_t base_group,
	xkb_layout_index_t latched_group,
	xkb_layout_index_t locked_group)

xkb_layout_index_t is unsigned int which prevents us from passing a negative group to utilize internal group wrapping:

libxkbcommon/src/state.c

Lines 192 to 195 in f60bdb1

	if (group < 0)
	return ((int) num_groups + (group % (int) num_groups));
	else
	return group % num_groups;

This forces the client to re-implement that wrapping on it's side, which is unnecessary.

Could we just declare the *_group arguments as int32_t?

[bluetech]

I'm curious where negative values come from exactly?

[bam80]

The calling code could pass just current_group - 1 to switch to previous layout.

[bam80]

Right now, we have to do the wrapping ourselves here in KWin, which duplicates the code above:
https://lxr.kde.org/source/plasma/kwin/src/xkb.cpp#0606

[bluetech]

I see, thanks.

As you can see from the XkbWrapGroupIntoRange code you linked to, XKB has a concept of "Out of range group action", which determines what happens when the group index goes of range. It can be configured per-key using the following values in the XKB symbols file:

• groupswrap - wrap
• groupsclamp - saturate, i.e. stop at zero if underflow and at last group if overflow
• groupsredirect=<groupnumber> - set to <groupnumber>

Like many things in XKB, I'm pretty sure this functionality is entirely unused, but it's supported by libxkbcommon and you can never know what configs people have.

So question is, when switching to prev/next in KWin UI, do you want the "Out of range group action" to be consulted, or always wrap?

[bam80]

libxkbcommon/src/state.c

Lines 690 to 694 in f60bdb1

	/* TODO: Use groups_wrap control instead of always RANGE_WRAP. */
	wrapped = XkbWrapGroupIntoRange(state->components.locked_group,
	state->keymap->num_groups,
	RANGE_WRAP, 0);

I see right now it always wraps, which corresponds to our current behavior.
If it ever change, I think we could iterate from that.

[bluetech]

Right, I was confusing the "global" active layout (which always wraps as we don't support the global "XKB Controls" like X11 does), and the per-key derived layout. So that's fine.

The next problem is that xkb_state_update_mask is not really meant to be used in the way KWin uses it in XKB::switchToLayout. As the xkb_state_update_mask docstring says:

 * This entry point is intended for window systems and the like, where a
 * master process holds an xkb_state, then serializes it over a wire
 * protocol, and clients then use the serialization to feed in to their own
 * xkb_state.
 *
 * All parameters must always be passed, or the resulting state may be
 * incoherent.
 *
 * The serialization is lossy and will not survive round trips; it must only
 * be used to feed slave state objects, and must not be used to update the
 * master state.
 *
 * If you do not fit the description above, you should use
 * xkb_state_update_key() instead.  The two functions should not generally be
 * used together.

that is, it is only meant to be used as a counterpart to xkb_state_serialize_mods and xkb_state_serialize_layout.

Of course, KWin is entirely justified in using it this way, as xkbcommon doesn't provide any other way for a UI toggle to switch the layout in the master state.

From this lens, xkb_state_update_mask is good as it is, and what we really need is a new function to update the active layout in the master state (like xkb_state_update_key for updating key state in the master state).

If we design such a function, would you be able to use it?

[bam80]

Yes I think we could use it, just thought it maybe feasible to change the type for xkb_state_update_mask, too - because right now it already allows wrapping "forward", but doesn't allow "backward" because of that unsigned type. So that looks like a symmetrical and at the same time less invasive change, backward-compatible withe the current API.

[bam80]

If we design such a function, would you be able to use it?

On a second thought, do we really need a new function here if the same can be achieved with the existing one?

[bluetech]

I've created a proposal for a new function for doing this - #314. Not implemented yet, just the API, but let me know if you have any thoughts on it.