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

Keysyms: fix xkb_keysym_is_modifier #415

Merged
merged 2 commits into from
Jan 10, 2024

Conversation

wismill
Copy link
Member

@wismill wismill commented Dec 5, 2023

Fixes #413 and add corresponding tests. Also add some tests for keypad keysyms.

This is based on #414; we should wait for it to be merged.

@wismill wismill added keysyms bug Indicates an unexpected problem or unintended behavior labels Dec 5, 2023
@wismill wismill force-pushed the keysyms/is-modifier-fix branch from b8f86b1 to 31431f2 Compare December 6, 2023 12:01
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.

Only looked at the top two commits (and only skim-reviewed those) but they LGTM, thanks!

test/keysym.c Outdated

#include "test.h"
#include "keysym.h" /* For unexported is_lower/upper/keypad() */

static const xkb_keysym_t modifier_keysyms[] = {

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

@bluetech
Copy link
Member

Thanks @wismill, the modifier fixes LGTM. Apologies for getting it wrong 10 years ago :)

Can you explain why this depends on #414? I see assert(!xkb_keysym_is_assigned(0xfe10)); but I don't think all of the complexity of #414 is worth it just for that...

@wismill
Copy link
Member Author

wismill commented Dec 30, 2023

@bluetech the fix itself does not depend on #414, but the tests do: they require the keysym iterator. I did not expect #414 to be problematic. I should probably reverse the PR dependency then.

Currently `xkb_keysym_is_modifier` does not detect the following keysyms:
- `XKB_KEY_ISO_Level5_Shift`
- `XKB_KEY_ISO_Level5_Latch`
- `XKB_KEY_ISO_Level5_Lock`

Indeed, there is a mistake in the keysym interval that the code checks.
The reason seems a confusing order of the keysyms in
`xkbcommon-keysyms.h`: the current code has a comment “libX11 only goes
up to XKB_KEY_ISO_Level5_Lock”, but in fact the modifiers keysyms are
listed in a _semantic_ order in `xkbcommon-keysyms.h`, not in the
increasing keysym _value_ order.

Fixed by using the same (correct) code as libX11 and added some tests.
@wismill wismill force-pushed the keysyms/is-modifier-fix branch from 31431f2 to 2cb9217 Compare January 10, 2024 13:40
@wismill wismill marked this pull request as ready for review January 10, 2024 13:42
@wismill
Copy link
Member Author

wismill commented Jan 10, 2024

Rebased.

@wismill wismill merged commit 82305ad into xkbcommon:master Jan 10, 2024
4 checks passed
@wismill wismill deleted the keysyms/is-modifier-fix branch January 10, 2024 13:45
@wismill wismill mentioned this pull request Feb 8, 2024
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior keysyms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compose sequence terminates after pressing ISO_LEVEL5_SHIFT
3 participants