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

Modern rules #362

Merged
merged 4 commits into from
Sep 27, 2024
Merged

Modern rules #362

merged 4 commits into from
Sep 27, 2024

Conversation

wismill
Copy link
Member

@wismill wismill commented Jul 13, 2023

This is a replacement of #334.

There is a lot of repetition in the current rules files, because the MLVO mappings need to match on the exact layout/variant index. This also prevents to increase the number of maximum groups.

We introduces new special layout/variant indexes:

  • “single”: matches a single layout; same as with no index.
  • “first”: matches the first layout/variant, no matter how many layouts are in the RMLVO configuration.
  • “later”: matches all but the first layout. This is an index range.
  • “any”: matches layout at any position. This is an index range.

We also introduces the new %i expansion, which correspond to the index of the matched layout in a mapping with an index range. Example:

layout[later] = symbols
my_layout     = +my_symbols:%i
*             = +%l[%i]:%i

Let’s have a look at concrete examples from xkeyboard-config:

! model     layout     = symbols
  *         *          = pc+%l%(v)
! model     layout[1]  = symbols
  *         *          = pc+%l[1]%(v[1])
! model     layout[2]  = symbols
  *         *          = +%l[2]%(v[2])
! model     layout[3]  = symbols
  *         *          = +%l[3]%(v[3])
! model     layout[4]  = symbols
  *         *          = +%l[4]%(v[4])

! layout    option     = symbols
  *         grp:toggle = +group(toggle)
! layout[1] option     = symbols
  *         grp:toggle = +group(toggle):1
! layout[2] option     = symbols
  *         grp:toggle = +group(toggle):2
! layout[3] option     = symbols
  *         grp:toggle = +group(toggle):3
! layout[4] option     = symbols
  *         grp:toggle = +group(toggle):4

With this commit we can now simplify it into:

! model    layout[first] = symbols
  *        *             = pc+%l[%i]%(v[%i])
! model    layout[later] = symbols
  *        *             = +%l[%i]%(v[%i]):%i

! layout[any] option     = symbols
  *           grp:toggle = +group(toggle):%i

The latter rule will work even if we increase XKB_MAX_GROUPS, whereas
the former would require to add the missing entries for the new groups.

In order to maintain consistent rules, we now disallow layout and variant to have different indexes. For example, the following mapping are now invalid:

  • layout variant[1]
  • layout[1] variant[2]
  • layout[1] variant
  • layout[first] variant[1]
  • layout[first] variant
  • layout variant[any]
  • etc.

Some layout options require to be applied to every group to maintain consistency (e.g. a group switcher). Currently this must be done manually for all layout indexes. This is error prone and prevents the increase of the maximum group count.

We introduces the :all qualifier for kccgst values. When a rule with this qualifier applies, it will expands for every layout, e.g. +group(toggle):all would expand to +group(toggle):1+group(toggle):2 if there are 2 layouts, etc.

TODO:

  • Update docs/rules-format.md
  • Add log entry
  • Propose changes to xkeyboard-config

Related: #37 and #311

@wismill wismill mentioned this pull request Jul 14, 2023
@wismill wismill added enhancement Indicates new feature requests rules X11 legacy: compatibility Indicate a need to ensure compatibility with X11 labels Jul 24, 2023
@wismill
Copy link
Member Author

wismill commented Nov 17, 2023

@whot @bluetech Rebased and improved the code. There are still redundancies, but for now I would like some feedback before to do further optimization.

Once the POC is validated, I can start to implement it into libxkbfile.

TODO: documentation

@wismill wismill requested review from bluetech and whot November 17, 2023 17:42
@wismill wismill marked this pull request as ready for review November 17, 2023 17:42
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.

Awesome work @wismill. Now with X starting to go the way of the dodo, the fact that we're still stuck on max 4 layouts is a bit embarrassing. And the repetition in the rules format is even more embarrassing :)

I've read through the code, it looks good (I will write some nits later), but let's finalize the design first.


Regarding first, later, any.

I already forgot why the format insists on distinguishing foo (matches first only if single layout given) and foo[1] (matches first only if multiple layouts given). Is there an honest semantic reason for this, or just some historic weirdness?

I think what would help me quite a bit is to see rules/evdev before/after (if it's not too much effort). Then I can see more directly why each new addition is needed.


Idle thought: maybe instead of first, later, any we can use python slice syntax, i.e. layout[2:], layout[:]. I realize the first case is complicated (see previous question).


%i SGTM, I think we can agree on this design.


:all SGTM, I think we can agree on this design. I left some nits on the commit.


docs/rules-format.md will need an update, can do it later though.


In order to maintain consistent rules, we now disallow layout and
variant to have different indexes.

This part LGTM, you can split & cherry-pick it to master already if you'd like unless you want to wait for libxkbfile to agree to this as well.

src/xkbcomp/rules.c Outdated Show resolved Hide resolved
src/xkbcomp/rules.c Outdated Show resolved Hide resolved
src/xkbcomp/rules.c Show resolved Hide resolved
@wismill
Copy link
Member Author

wismill commented Dec 4, 2023

@bluetech Thanks! I refactored based on your review. I also introduced MAX_LAYOUT_INDEX_STR_LENGTH to be more explicit.

Regarding first, later, any.

I already forgot why the format insists on distinguishing foo (matches first only if single layout given) and foo[1] (matches first only if multiple layouts given). Is there an honest semantic reason for this, or just some historic weirdness?

I think this is due to some config model/layouts adding a second group, e.g:

! model		layout			=	symbols
  ataritt	$nonlatin		=	xfree68_vndr/ataritt(us)+%l%(v):2

Now I think it makes sense to add e.g. layout[single] to make it more explicit. What do you think?

I think what would help me quite a bit is to see rules/evdev before/after (if it's not too much effort). Then I can see more directly why each new addition is needed.

I think this transformation would make sense even without this implementation, in order to simplify the rules. Let me take time to propose a MR in xkeyboard-config. Meanwhile, please have a look at the examples above.

Idle thought: maybe instead of first, later, any we can use python slice syntax, i.e. layout[2:], layout[:]. I realize the first case is complicated (see previous question).

I thought exactly about that! But then: what does it bring to us? Do we need e.g. [3:] or [1:3] or [:2]? If not, then I think this is overkill.

docs/rules-format.md will need an update, can do it later though.

Yes, it should be done in this MR. Let’s add a TODO.

In order to maintain consistent rules, we now disallow layout and
variant to have different indexes.

This part LGTM, you can split & cherry-pick it to master already if you'd like unless you want to wait for libxkbfile to agree to this as well.

I do not think we need to wait for libxkbfile.

@wismill
Copy link
Member Author

wismill commented Sep 25, 2024

Rebased, added [single] special index and refactored a bit.

This will make their semantics explicit.
@wismill
Copy link
Member Author

wismill commented Sep 25, 2024

Fixed some edge cases and ensure we are prepared to raise the group limit.

I am now fairly confident in this PR. I will give another review tomorrow and probably merge it soon after, so we can start to work on a modern set of rules in xkeyboard-config and investigate the impact of an increase of the group limit.


To myself: do not forget the TODO list 😄

@wismill wismill requested a review from bluetech September 25, 2024 14:49
@wismill wismill force-pushed the wip/modern-rules branch 2 times, most recently from 91da8b1 to 7cdfe09 Compare September 26, 2024 19:23
There is a lot of repetition in the current rules files provided by
xkeyboard-config, because the MLVO mappings need to match on the exact
layout/variant index. This also prevents to increase the number of
maximum groups, because it does not scale.

We introduces the following new special layout/variant indexes:
- “single”: matches a single layout; same as with no index.
- “first”: matches the first layout/variant, no matter how many layouts
  are in the RMLVO configuration. It allows to merge `layout` and
  `layout[1]` patterns.
- “later”: matches all but the first layout. This is an index range.
- “any”: matches layout at any position. This is an index range.

We also introduces the new `%i` expansion, which correspond to the index
of the matched layout in a mapping with an index range. Example:

    layout[later] = symbols
    my_layout     = +my_symbols:%i
    *             = +%l[%i]:%i

Let’s have a look at concrete examples from xkeyboard-config:

    ! model     layout     = symbols
      *         *          = pc+%l%(v)
    ! model     layout[1]  = symbols
      *         *          = pc+%l[1]%(v[1])
    ! model     layout[2]  = symbols
      *         *          = +%l[2]%(v[2])
    ! model     layout[3]  = symbols
      *         *          = +%l[3]%(v[3])
    ! model     layout[4]  = symbols
      *         *          = +%l[4]%(v[4])

    ! layout    option     = symbols
      *         grp:toggle = +group(toggle)
    ! layout[1] option     = symbols
      *         grp:toggle = +group(toggle):1
    ! layout[2] option     = symbols
      *         grp:toggle = +group(toggle):2
    ! layout[3] option     = symbols
      *         grp:toggle = +group(toggle):3
    ! layout[4] option     = symbols
      *         grp:toggle = +group(toggle):4

With this commit we can now simplify it into:

    ! model    layout[first] = symbols
      *        *             = pc+%l[%i]%(v[%i])
    ! model    layout[later] = symbols
      *        *             = +%l[%i]%(v[%i]):%i

    ! layout[any] option     = symbols
      *           grp:toggle = +group(toggle):%i

The latter rule will work even if we increase XKB_MAX_GROUPS, whereas
the former would require to add the missing entries for the new groups.

In order to maintain consistent rules, we now disallow layout and
variant to have different indexes. For example, the following mapping
are now invalid:
- layout variant[1]
- layout[1] variant[2]
- layout[1] variant
- layout[first] variant[1]
- layout[first] variant
- layout variant[any]
- etc.
Some layout options require to be applied to every group to maintain
consistency (e.g. a group switcher). Currently this must be done manually
for all layout indexes. This is error prone and prevents the increase of
the maximum group count.

This commit introduces the `:all` qualifier for KcCGST values. When a
rule with this qualifier is matched, it will expands the qualified
value (and its optional merge mode) for every layout, e.g.
`+group(toggle):all` (respectively `|group(toggle)`) would expand to
`+group(toggle):1+group(toggle):2` (respectively
`|group(toggle):1|group(toggle):2`) if there are 2 layouts, etc.

If there is no merge mode, it defaults to *override* `+`, e.g. `x:all`
expands to `x:1+x:2+x:3` for 3 layouts.

Note that only the qualified *value* is expanded, e.g. `x+y:all` expands
to `x+y:1+y:2` for 2 layouts.

`:all` can be used in combination with special layout indexes. Since
this can lead to an unexpected behaviour, a warning will be raised.
@wismill wismill merged commit 4ea9d43 into xkbcommon:master Sep 27, 2024
4 checks passed
@wismill wismill deleted the wip/modern-rules branch September 27, 2024 06:26
@wismill wismill mentioned this pull request Sep 27, 2024
3 tasks
@wismill wismill added this to the 1.8.0 milestone Oct 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion: backward compatibility enhancement Indicates new feature requests rules X11 legacy: compatibility Indicate a need to ensure compatibility with X11
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants