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.

Sign up for GitHub

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

Jump to bottom

xkbcomp: Fix key merge mode (keysyms, actions) #568

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

xkbcomp: Fix key merge mode (keysyms, actions) #568

wants to merge 4 commits into from

Conversation

wismill
Copy link
Member

@wismill wismill commented Dec 19, 2024 •
edited
Loading

This also adds exhaustive tests for merge modes.

Fixes #564

TODO

TODO in follow-up MRs:

  • Merge mode propagation
  • Other fields/objects

@mahkoh

@wismill wismill added bug Indicates an unexpected problem or unintended behavior compile-keymap Indicates a need for improvements or additions to keymap compilation labels Dec 19, 2024
@wismill wismill added this to the 1.8.0 milestone Dec 19, 2024
@wismill wismill force-pushed the symbols/fix-key-merge-mode-symbols branch from b946df1 to da3ee82 Compare December 19, 2024 18:16
@wismill
Copy link
Member Author

wismill commented Dec 19, 2024 •
edited
Loading

Windows Compiler Error C2026. Wonderful.

Fixed by using assert_printf instead of assert. We could also use a temp var.

@wismill
Copy link
Member Author

wismill commented Dec 19, 2024

Note to self: in order to check other key properties, we should probably just compare resulting keymap strings with a ref.

Also let's add the same tests for xkbcomp (at least where the syntax is supported: e.g. no multi keysym per level).

@wismill wismill force-pushed the symbols/fix-key-merge-mode-symbols branch 13 times, most recently from c5549d4 to 7ff4c3d Compare December 27, 2024 00:45
@bam80
Copy link

bam80 commented Dec 27, 2024

May be related: #573

@wismill wismill force-pushed the symbols/fix-key-merge-mode-symbols branch 3 times, most recently from c78c27c to 84d1cdc Compare December 27, 2024 01:09
@wismill
Copy link
Member Author

wismill commented Dec 27, 2024

Trying to fix the failing macOS pipeline. I suspect an out-of-bound in xkbcomp (see this MR) to be the origin of:

Warning:          Multiple actions for level 3/group 1 on key <AD03>
                  Using Private, ignoring SetGroup

The Private action bit above is suspicious.

The Linux pipeline is fragile too: it works without Valgrind, so I suspect race conditions with the various threads.

@wismill wismill force-pushed the symbols/fix-key-merge-mode-symbols branch 6 times, most recently from e7c94f6 to 3097e91 Compare December 28, 2024 17:57
@wismill wismill force-pushed the symbols/fix-key-merge-mode-symbols branch 4 times, most recently from 6ec280e to 6d70fda Compare December 28, 2024 18:37
@wismill wismill mentioned this pull request Dec 28, 2024
Merged
@wismill
symbols: Add tests for key merge modes (keysyms/actions)
e6f4f88 
This commit adds tests for merging various key configurations:
- With/without keysyms/actions
- Single/multiple keysyms/actions per level

We test all the merge modes for including a map:
- default (include)
- augment
- override
- replace

We also test against xkbcomp so we can compare its handling of merge
modes with xkbcommon. Unfortunately, there are currently some
differences due to xkbcomp discarding trailing `NoSymbol`. Thus the
tests are split into: common, X11-only and xkbcommon-only.
@wismill
symbols: Fix key symbols/actions merge
4eb1430
@wismill
x11: Keep level when the keysym is undefined but not the action
466e38b 
When getting the keymap from X11, the following:

```
key <AD01> { actions=[SetGroup(2)] };
```

is currently converted to:

```
key <AD01> { };
```

This commit fixes dropping a defined action when the keysym is undefined
@wismill
ci: Use patched xkbcomp for macOS
5107d69 
It seems that the X11 tests trigger an out-of-bounds bug in xkbcomp.
See: https://gitlab.freedesktop.org/xorg/app/xkbcomp/-/merge_requests/26
@wismill wismill force-pushed the symbols/fix-key-merge-mode-symbols branch from 6d70fda to 5107d69 Compare December 28, 2024 19:09
@wismill
Copy link
Member Author

wismill commented Dec 28, 2024

I finally managed to install the patched xkbcomp on the macOS CI. Linux CI seems robust now.

@wismill wismill requested review from bluetech and whot December 28, 2024 19:14
@wismill wismill marked this pull request as ready for review December 28, 2024 19:14
@wismill
Copy link
Member Author

wismill commented Dec 28, 2024

@mahkoh With this MR we will have a first round of tests for the common uses of merge modes multiple fixes.

TODO:

  • Merge mode propagation
  • Other fields/objects

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bluetech bluetech Awaiting requested review from bluetech

@whot whot Awaiting requested review from whot

Assignees
No one assigned
Labels
bug Indicates an unexpected problem or unintended behavior compile-keymap Indicates a need for improvements or additions to keymap compilation
Projects
None yet
Milestone
1.8.0
Development

Successfully merging this pull request may close these issues.

Abort when augmenting key definition
2 participants
@wismill @bam80