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

Support querying whether virtual modifiers are active (alternative 2) #512

Merged
merged 5 commits into from
Nov 29, 2024

Conversation

wismill
Copy link
Member

@wismill wismill commented Sep 23, 2024

Allow to query also virtual modifiers with the following functions, so they work with any modifiers (real and virtual):

  • xkb_state_mod_index_is_active
  • xkb_state_mod_indices_are_active
  • xkb_state_mod_name_is_active
  • xkb_state_mod_names_are_active
  • xkb_state_mod_index_is_consumed
  • xkb_state_mod_index_is_consumed2

Warning: they may overmatch in case there are overlappings virtual-to-real
modifiers mappings.

Also added the following modifiers names definitions in xkbcommon-names.h:

  • XKB_MOD_NAME_MOD1
  • XKB_MOD_NAME_MOD2
  • XKB_MOD_NAME_MOD3
  • XKB_MOD_NAME_MOD4
  • XKB_MOD_NAME_MOD5
  • XKB_VMOD_NAME_ALT
  • XKB_VMOD_NAME_META
  • XKB_VMOD_NAME_NUM
  • XKB_VMOD_NAME_SUPER
  • XKB_VMOD_NAME_HYPER
  • XKB_VMOD_NAME_LEVEL3
  • XKB_VMOD_NAME_LEVEL5
  • XKB_VMOD_NAME_SCROLL

This is a simplier version of #353, which is also an alternative to #36.

This is necessary to implement fully #450.

Fixes #88

@wismill wismill added the state Indicates a need for improvements or additions to the xkb_state API label Sep 23, 2024
@wismill wismill added this to the 1.8.0 milestone Sep 23, 2024
@wismill wismill force-pushed the modifiers/query-virtual-mods branch from 946ec04 to 06597bd Compare September 23, 2024 14:40
@wismill
Copy link
Member Author

wismill commented Sep 23, 2024

TODO: more tests, with all modifiers. We want this to be 100% reliable, with no breaking changes in the known and foreseen uses.

@wismill wismill force-pushed the modifiers/query-virtual-mods branch from 06597bd to 700d0a7 Compare September 27, 2024 10:45
@wismill wismill force-pushed the modifiers/query-virtual-mods branch 4 times, most recently from 9d79bb6 to 3d6eb0b Compare October 14, 2024 15:30
@wismill wismill force-pushed the modifiers/query-virtual-mods branch from 3d6eb0b to e278b15 Compare October 15, 2024 07:56
@wismill wismill marked this pull request as ready for review October 15, 2024 07:58
@wismill wismill requested review from bluetech and whot October 15, 2024 07:58
@wismill
Copy link
Member Author

wismill commented Oct 30, 2024

We should probably deprecate the following in favor of the new names:

  • XKB_MOD_NAME_ALTXKB_VMOD_NAME_ALT
  • XKB_MOD_NAME_LOGOXKB_VMOD_NAME_SUPER
  • XKB_MOD_NAME_NUMXKB_VMOD_NAME_NUM

Copy link
Contributor

@whot whot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this LGTM though admittedly I haven't managed to page everything back in in detail. Two nitpicks, otherwise 👍

src/state.c Outdated Show resolved Hide resolved
src/state.c Outdated Show resolved Hide resolved
src/state.c Outdated Show resolved Hide resolved
src/state.c Show resolved Hide resolved
@wismill wismill force-pushed the modifiers/query-virtual-mods branch from 172830b to 3646804 Compare November 4, 2024 16:22
Respect the claim that real modifiers names are built-in:
- Add `XKB_MOD_NAME_MOD[1-5]` constants.
- Replace modifiers names with their corresponding constants.
Added support for the following virtual modifiers:
- `Alt`
- `Meta`
- `NumLock`
- `Super`
- `Hyper`
- `LevelThree`
- `LevelFive`
- `ScrollLock`
@wismill
Copy link
Member Author

wismill commented Nov 18, 2024

Added support for xkb_state_mod_mask_remove_consumed as well. I am going reactivate the draft status, as this latter function failure was not caught by the test. Well, the function is deprecated, but it is also still supported!

@wismill wismill marked this pull request as draft November 18, 2024 19:12
@wismill
Copy link
Member Author

wismill commented Nov 19, 2024

I do not see other functions requiring changes.

@wismill wismill marked this pull request as ready for review November 19, 2024 18:45
Copy link
Member

@bluetech bluetech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will be very cool if we can make this work.

I only did a very shallow skim of the code. I too have mostly forgotten the details unfortunately. Some questions:

  1. Is the overmatching possibility fundamental to the way virtual modifiers work, or is it possible to rework the internals such that they track particular vmods?

  2. Does this work for xkbcommon-x11?

  3. In practice, I think most users ignore the is_active functions and just use xkb_state_serialize_mods, despite it being documented as for serialization only. Unfortunate, and maybe it has changed by now, but probably not, since people always take the path of least resistance. The question is -- is there anything we can do about it?

test/state.c Outdated Show resolved Hide resolved
test/state.c Outdated Show resolved Hide resolved
test/state.c Outdated Show resolved Hide resolved
@wismill
Copy link
Member Author

wismill commented Nov 20, 2024

Some questions:

  1. Is the overmatching possibility fundamental to the way virtual modifiers work, or is it possible to rework the internals such that they track particular vmods?

@bluetech Unfortunately it’s fundamental, at least until #450 is merged. Here some examples:

  1. Alt and Meta both map to Mod1 by default, so activating any of the 3 modifiers activates the two others.
  2. If a virtual modifier is mapped to 2 real modifiers, activating these 2 modifiers independently will also activate this vmod.

In practice, 1) is already an issue so DE and apps handle it already. 2) is probably extremely rare and more due to an accident. The only use I know is to differentiate left and right variant of a modifier.

  1. Does this work for xkbcommon-x11?

Yes, AFAICT: in the end we rely on the real modifiers. It will start to change when #450 is merged.

  1. In practice, I think most users ignore the is_active functions and just use xkb_state_serialize_mods, despite it being documented as for serialization only. Unfortunate, and maybe it has changed by now, but probably not, since people always take the path of least resistance. The question is -- is there anything we can do about it?

That is a good question! State and its value are supposed to be opaque, as stated by Daniel in this Wayland issue. This may change, but apps working on states values directly expose themselves to bugs. Do you have some examples in mind?

More probably they replicate the logic, like KDE KWin. Anyway, if they work directly with the state mod masks they must already work with real modifiers and I did not change the state representation/serialization with the current MR. Again, what is a real modifier will change when #450 get merged: we will have to distinguish X11 and non-X11 real modifiers, because “pure” virtual modifiers as stated in #450 are in fact non-X11 real modifiers.

So, until this Wayland issue declare the contrary, the only thing we can do about it is to document that the state values should only be processed by the xkbcommon API.

Previously it was not possible to query the status of virtual modifiers
with the following functions:
- `xkb_state_mod_index_is_active`
- `xkb_state_mod_indices_are_active`
- `xkb_state_mod_name_is_active`
- `xkb_state_mod_names_are_active`

Note that it may *overmatch* if some modifier mappings overlap. For
example, the default “us” PC layout maps both “Alt” and “Meta” to the
real modifier “Mod1”; thus “Mod1”, “Alt” and “Meta” modifiers will
return the same result with these functions.
Previously it was not possible to query the status of virtual modifiers
with the following functions:
- `xkb_state_mod_index_is_consumed`
- `xkb_state_mod_index_is_consumed2`

Note that it may *overmatch* if some modifier mappings overlap. For
example, the default “us” PC layout maps both “Alt” and “Meta” to the
real modifier “Mod1”; thus “Mod1”, “Alt” and “Meta” modifiers will
return the same result with these functions.
Copy link
Member Author

@wismill wismill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated according to @bluetech review.

test/state.c Outdated Show resolved Hide resolved
test/state.c Outdated Show resolved Hide resolved
test/state.c Outdated Show resolved Hide resolved
@wismill wismill force-pushed the modifiers/query-virtual-mods branch from ad0b6cb to fe78832 Compare November 20, 2024 08:22
@wismill wismill merged commit e0130e3 into xkbcommon:master Nov 29, 2024
4 checks passed
@wismill wismill deleted the modifiers/query-virtual-mods branch November 29, 2024 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state Indicates a need for improvements or additions to the xkb_state API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

API for reading xkb_mod_index_t to keysym mapping
3 participants