Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
bam80 opened this issue Dec 10, 2022 · 10 comments · May be fixed by #314
Labels
enhancement Indicates new feature requests help wanted Indicates that a maintainer wants help on an issue or pull request state Indicates a need for improvements or additions to the xkb_state API

Comments

@bam80
Copy link

bam80 commented Dec 10, 2022

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
Copy link
Member

I'm curious where negative values come from exactly?

@bam80
Copy link
Author

bam80 commented Dec 14, 2022

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

@bam80
Copy link
Author

bam80 commented Dec 14, 2022

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
Copy link
Member

bluetech commented Dec 14, 2022

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
Copy link
Author

bam80 commented Dec 14, 2022

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
Copy link
Member

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
Copy link
Author

bam80 commented Dec 14, 2022

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
Copy link
Author

bam80 commented Dec 15, 2022

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 added a commit that referenced this issue Dec 17, 2022
…server state

Up to now, the "server state" xkb_state API only offered one entry point
to update the server state - `xkb_state_update_key`, which reflects the
direct keyboard keys state. But some updates come out-of-band from
keyboard input events stream, for example, a GUI layout switcher.

The X11 XKB protocol has a request which allows for such updates,
`XkbLatchLockState`[0], but xkbcommon does not have similar
functionality. So server applications ended up using
`xkb_state_update_state` for this, but that's a function intended for
client applications, not servers.

Add support for updating the latched & locked state of the mods and
layout. Note that the depressed states cannot be updated in this way --
XKB does not expect them to be updated out of band.

[0] https://www.x.org/releases/X11R7.7/doc/kbproto/xkbproto.html#Querying_and_Changing_Keyboard_State

Fixes: #310
Signed-off-by: Ran Benita <ran@unusedvar.com>
@bluetech
Copy link
Member

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.

bluetech added a commit that referenced this issue Dec 17, 2022
…server state

Up to now, the "server state" xkb_state API only offered one entry point
to update the server state - `xkb_state_update_key`, which reflects the
direct keyboard keys state. But some updates come out-of-band from
keyboard input events stream, for example, a GUI layout switcher.

The X11 XKB protocol has a request which allows for such updates,
`XkbLatchLockState`[0], but xkbcommon does not have similar
functionality. So server applications ended up using
`xkb_state_update_state` for this, but that's a function intended for
client applications, not servers.

Add support for updating the latched & locked state of the mods and
layout. Note that the depressed states cannot be updated in this way --
XKB does not expect them to be updated out of band.

[0] https://www.x.org/releases/X11R7.7/doc/kbproto/xkbproto.html#Querying_and_Changing_Keyboard_State

Fixes: #310
Signed-off-by: Ran Benita <ran@unusedvar.com>
@wismill wismill added enhancement Indicates new feature requests help wanted Indicates that a maintainer wants help on an issue or pull request state Indicates a need for improvements or additions to the xkb_state API labels May 15, 2023
@wismill wismill added this to the 1.7.0 milestone Sep 20, 2023
@wismill wismill removed this from the 1.7.0 milestone Feb 8, 2024
@wismill
Copy link
Member

wismill commented Feb 20, 2024

Related: #72

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Indicates new feature requests help wanted Indicates that a maintainer wants help on an issue or pull request state Indicates a need for improvements or additions to the xkb_state API
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants