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

actions: Add support for multiple actions per level #487

Merged
merged 6 commits into from
Oct 11, 2024

Conversation

wismill
Copy link
Member

@wismill wismill commented Jul 12, 2024

This makes 1 keysym == 1 action holds also for multiple keysyms per level.

The motivation of this new feature are:

  • Make multiple keysyms per level more intuitive.

  • Explore how to fix the issue with shortcuts in multi-layout settings (see the xkeyboard-config issue).

    The idea is to use e.g.:

    key <LCTL> {
          symbols[1] = [ {Control_L,                  ISO_First_Group    } ],
          actions[1] = [ {SetMods(modifiers=Control), SetGroup(group=-4) } ]
    };

    in order to switch temporarily to a reference layout in order to get the same shortcuts on every layout.

    See the xkeyboard-config MR “Add option shortcuts:qwerty”.

This is quite a big change, because it breaks the design “1 action per level”.

This is WIP because there might be lots of corner cases.

I will try to break this into smaller commits, although the nature of the change makes it difficult to have smaller commits that compile.

Fixes #486


TODO:

  • Fix remaining TODO/FIXME in the code
  • Log entry

@wismill wismill added enhancement Indicates new feature requests state Indicates a need for improvements or additions to the xkb_state API labels Jul 12, 2024
@wismill wismill force-pushed the actions/multiple-per-level branch from 80c9844 to 34b9145 Compare September 23, 2024 09:36
@wismill
Copy link
Member Author

wismill commented Sep 23, 2024

Rebased and split commits (a bit). The first commit should be dropped when #507 #516 is merged.

@wismill
Copy link
Member Author

wismill commented Oct 2, 2024

This new feature can be of help for shortcuts-related issues, such as: “Switching layout temporarily while modifier is held”.

For example, using:

    key <LCTL>               {
        symbols[1] = [       {Control_L,                             ISO_First_Group } ],
        actions[1] = [       {SetMods(modifiers=Control,clearLocks), SetGroup(group=-4)}]
    };

and setting keymap RANGE_REDIRECT to 0, every shortcut involving <LCTL> will use the first layout. The RANGE_REDIRECT and -4 are necessary because of how effective group computation works: it’s an addition of the various components (base, latched, locked), so SetGroup(group=1) does not set the effective group to 1.

That said, modifying actions in symbols is not really practical. But interpret statements only support one action and it is probably better so: we want to keep 1 keysym = 1 action.

We could, however, introduce “composite” actions, e.g. RunSequence(action1(…), actions2(…), …) (probably better with an array syntax). It would still count as a single action while parsing.

But in the end, I think we need a dedicated feature that:

  • Add additional action to modifiers automatically;
  • Handle the effective group computation properly;
  • Maybe act differently if the non-modifier keysym of the shortcut is a (Latin) letter or not.

This would only internal with no special syntax, but a specialized API.

@wismill wismill force-pushed the actions/multiple-per-level branch 2 times, most recently from 5d7c75d to 4075c7a Compare October 7, 2024 17:19
@wismill
Copy link
Member Author

wismill commented Oct 7, 2024

Rebased and added exhaustive tests, that enabled to discover some memory issues. We now disallow mixing multiple modifiers or group actions, as it requires to update state handling. I may add it in this PR if it is easy enough, else will do in a further MR/release.

macOS CI is becoming very slow and fails. Looks like some issues with packaging. I do not think we had a full xorg server installed there, but now it does.

@wismill wismill force-pushed the actions/multiple-per-level branch from 4075c7a to 685138c Compare October 8, 2024 05:22
@wismill
Copy link
Member Author

wismill commented Oct 8, 2024

Rebased to fix macOS CI.

@wismill wismill force-pushed the actions/multiple-per-level branch 2 times, most recently from 49eb8f5 to b1a4c7d Compare October 8, 2024 11:41
@wismill
Copy link
Member Author

wismill commented Oct 8, 2024

Rebased after merging #526.

@wismill wismill force-pushed the actions/multiple-per-level branch from b1a4c7d to d0c49c8 Compare October 8, 2024 15:51
@wismill wismill marked this pull request as ready for review October 8, 2024 15:52
@wismill wismill requested review from whot and bluetech October 8, 2024 15:52
@wismill wismill force-pushed the actions/multiple-per-level branch from d0c49c8 to 58c4149 Compare October 8, 2024 17:46
@wismill
Copy link
Member Author

wismill commented Oct 8, 2024

Note to self: we may want to merge #518 and #519 before this one. Or the contrary, whatever is easier.

@wismill wismill force-pushed the actions/multiple-per-level branch from 58c4149 to 3877267 Compare October 11, 2024 14:07
@wismill
Copy link
Member Author

wismill commented Oct 11, 2024

Rebased after merging #519.

Do not allow `{ a }` when a single `a` suffices.
The current field `u` (short for “union”) is not very descriptive.
Next commit will add multiple actions per level, so let’s rename the
keysym field to `s` (short for “symmbols”).
This makes 1 keysym == 1 action holds also for multiple keysyms per level.

The motivation of this new feature are:

- Make multiple keysyms per level more intuitive.
- Explore how to fix the issue with shortcuts in multi-layout settings
  (see the xkeyboard-config issue[^1]). The idea is to use e.g.:

  ```c
  key <LCTL> {
      symbols[1] = [ {Control_L,                  ISO_First_Group    } ],
      actions[1] = [ {SetMods(modifiers=Control), SetGroup(group=-4) } ]
  };
  ```

  in order to switch temporarily to a reference layout in order to get
  the same shortcuts on every layout.

When no action is specified, `interpret` statements are used to find
an action corresponding for *each* keysym, as expected.

For an interpretation matching Any keysym, we may get the same
interpretation for multiple keysyms. This may result in unwanted
duplicate actions. So set this interpretation only if no previous
keysym was matched with this interpret at this level, else set the
default interpretation.

For now, at most one action of each following categories is allowed
per level:
- modifier actions: `SetMods`, `LatchMods`, `LockMods`;
- group actions: `SetGroup`, `LatchGroup`, `LockGroup`.

Some examples:
- `SetMods` + `SetGroup`: ok
- `SetMods` + `SetMods`: error
- `SetMods` + `LockMods`: error
- `SetMods` + `LockGroup`: ok

[^1]: https://gitlab.freedesktop.org/xkeyboard-config/xkeyboard-config/-/issues/416
@wismill wismill force-pushed the actions/multiple-per-level branch from 3877267 to 35aa91c Compare October 11, 2024 14:19
@wismill
Copy link
Member Author

wismill commented Oct 11, 2024

Rebased after merging #518.

@wismill wismill merged commit 71d64df into xkbcommon:master Oct 11, 2024
4 checks passed
@wismill wismill deleted the actions/multiple-per-level branch October 11, 2024 14:26
@wismill wismill mentioned this pull request Oct 11, 2024
4 tasks
@wismill wismill added this to the 1.8.0 milestone Oct 12, 2024
@mahkoh
Copy link

mahkoh commented Jan 23, 2025

There is an issue with this. Xwayland and older xkbcommon will fail to parse keymaps with multiple actions, making this feature impossible to use in practice. I've worked around this in my implementation by not formatting such actions by default: mahkoh/kbvm#11

@wismill
Copy link
Member Author

wismill commented Jan 23, 2025

@mahkoh This is a known issue. Proper handling requires fallback and format version management, see #484. Contrary to kbvm, we have always the same number of keysyms and actions, so we are out of luck for now. #528 may help a bit.

I can see how having different number of keysyms and actions can be useful. The case 1 keysym/multiple actions would then be possible to handle as you have done. But it is still not clear to me what the merge behavior should be. Could you point me to your test cases?

Anyway keep in mind this is all very recent; multiple keysyms per level is not even very well supported.

I preliminary added multiple actions per level as a possible solution to the shortcut hell when using multiple layouts.

I aim to work on stability in the next release.

@mahkoh
Copy link

mahkoh commented Jan 23, 2025

Contrary to kbvm, we have always the same number of keysyms and actions, so we are out of luck for now.

In that case a solution could be to send NO explicit actions for that key. The only good reason I know to have them in client-side keymaps in the first place is to allow clients to figure out how to type keysyms.

But it is still not clear to me what the merge behavior should be. Could you point me to your test cases?

It's difficult since I wrote the testcases via mutation testing. First I modify a line in the source code in an incompatible way, then I run the tests. If no test fails, I add a new test for that line. But I don't keep track of which lines are tested by which testcases.

But here is the logic that merges the actions from the same level:

                        if new_level.actions.is_not_empty() {
                            if augment && existing_group.levels[idx].actions.is_not_empty() {
                                // emit diagnostic message
                            } else {
                                existing_group.levels[idx].actions = new_level.actions;
                                existing_group.has_explicit_actions |= new_group.has_explicit_actions;
                            }
                        }

@wismill
Copy link
Member Author

wismill commented Jan 23, 2025

In that case a solution could be to send NO explicit actions for that key. The only good reason I know to have them in client-side keymaps in the first place is to allow clients to figure out how to type keysyms.

We still would have the issue with the multiple keysyms (remember, n keysyms = n actions), thus the keymap would still be invalid for XWayland. Thus my reference to #528.

About the test cases: do you have realistic examples of count “conflicts” of keysyms/action when merging?

Most of the time the actions are implicit via interprets anyway, so I am maybe just too conservative by keeping the count sync between actions and keysyms.

@mahkoh
Copy link

mahkoh commented Jan 23, 2025

We still would have the issue with the multiple keysyms (remember, n keysyms = n actions), thus the keymap would still be invalid for XWayland.

Consider

xkb_keymap {
    xkb_keycodes {
        <a> = 9;
    };
    xkb_types { };
    xkb_compat {
        interpret VoidSymbol {
            repeat = false;
        };
    };
    xkb_symbols {
        key <a> {
            [ { a, b } ],
            [ { SetMods(mods = Mod1), SetGroup(group = +1) } ]
        };
    };
};

xkbcomp does not parse this map at all. However, if instead

    xkb_symbols {
        key <a> {
            [ { a, b } ]
        };
    };

then xkbcomp parses the map and produces

xkb_symbols {

    key    <a> {         [        NoSymbol ] };
};

So the key is useless, but at least all other keys work.

About the test cases: do you have realistic examples of count “conflicts” of keysyms/action when merging?

I don't know about realistic examples. But there is no place in kbvm that ever compares the number of keysyms and the number of actions. The only place where the are related is when no explicit actions are defined and actions are determined via interpret sections. In that case kbvm should behave just like xkbcommon but that happens long after merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Indicates new feature requests 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.

support multiple actions for a key per level
2 participants