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

Add support for conditional comments #432

Open
wismill opened this issue Jan 13, 2024 · 5 comments
Open

Add support for conditional comments #432

wismill opened this issue Jan 13, 2024 · 5 comments
Labels
compile-keymap Indicates a need for improvements or additions to keymap compilation discussion: backward compatibility

Comments

@wismill
Copy link
Member

wismill commented Jan 13, 2024

(EDIT: This need reformulation. See my comment below.

TLDR;

I propose to add support for conditional comments in the keymap text format, in order to introduce non-backward compatible features, in files parsable by both legacy XOrg tools and xkbcommon.

In a gist (syntax subject to change):

xkb_symbols "example-1" {
    // Comments starting with “@” are treated as special comments.
    // They are ignored by XOrg tools and previous versions of xkbcommon.
    
    // The following conditional comment starts a conditional block.
    // It guards XKB code for xkbcommon with support for multiple keysyms.
    
    //@if support-multiple-keysyms
    
    // Next comment is commented normal XKB code, parsed and interpreted only by 
    // xkbcommon versions that support both conditional comments and multiple keysyms output.
    // This allow us to support both legacy and modern XKB next to each other.
    
    //@  key <AD01> { [ {a, b}, Q ] };
    
    // Next comment is a conditional comment to guard the fallback code.
    
    //@else
    
    // The following line is normal code, always parsed by XOrg tools and old xkbcommon versions,
    // but parsed by newer version of xkbcommon only without support for multiple keysyms.
         key <AD01> { [ q, Q ] };
        
    // Next comment ends the conditional block.
        
    //@endif
}

Context

I started to look at pretty old issues such as:

I am not going to discuss them in details here. What they have in common is that they need to act against the XKB protocol. Or rather, they need to extend the protocol. Also: they are not going to be fixed in XOrg1.

XKB devs were pretty clear that going against the XKB specification is risky in general. Hard-coding ad-hoc solutions for the issues mentioned before does not seem a good idea either.

Instead, I think we should extend the protocol and the keymap text syntax. Again, I am not going to share a solution here, but rather raise a question in the following section.

Note that we already introduced a backward-incompatible syntax for multiple keysyms:

xkb_symbols "example-2" {
    key <AD01> { [ {a, b}, Q ] };
}

Problem

Extending the syntax is viable only if we can maintain backward-compatible code in xkeyboard-config. I am convinced it should be answered before elaborating any solution that extends the syntax.

Possible solutions

Keep backward-incompatible code in separate files

Keep backward-incompatible code in separate files and use different rules files depending on the supported features.

Cons: related code (e.g. new feature & its fallback) are in separate files. Also, it may duplicate rules files and requires introduce a new logic to select the proper rules file.

Create the keymap text format 2

Same issue as previously: we will have duplication of the code. But above all, this solution should be probably kept for the wonderfull XKB V2 protocol.

(Ab)use existing syntax

The action Private is currently not supported in xkbcommon and is ignored. We could start parsing it and take advantage of the unused bits in the XKB protocol.

Cons: Counter-intuitive ugly hack. Also, this would not allow to support the multiple keysyms feature.

Use a preprocessor with conditional macros, a la C

Big hammer solution.

Cons: What about XOrg tools? There is no plan to modify them. And even if it were the case, updating xkeyboard-config would require to wait that the new XOrg tools are used in most distributions, … which may never happen.

Proposed solution: use conditional special comments

The idea is similar to the C preprocessor2. See the section TLDR above for the commented syntax.

Pros:

  • Fully backward-compatible, both is XOrg and previous xkbcommon version.
  • Future-proof.
  • No need for separate files; related codes are located next to each other.
  • Kind of intuitive for people familiar with C macros.
  • Allow to have fine-grained control over features support.

Cons:

  • Not a native feature

Footnotes

  1. Well, they could, in the unlikely case somebody stands up.

  2. Also, some of you may remember the infamous Internet Explorer conditional comments. While ugly, they did their job.

@wismill wismill added the compile-keymap Indicates a need for improvements or additions to keymap compilation label Jan 13, 2024
@wismill
Copy link
Member Author

wismill commented Jan 14, 2024

So, I slept on it and now I see a major use case I did not develop sufficiently: how to gracefully handle server → client keymap transfer, in case we have a different version of xkbcommon in both.

I am going to close this issue and reformulate it properly, without proposing a specific solution.

@wismill wismill closed this as not planned Won't fix, can't repro, duplicate, stale Jan 14, 2024
@whot
Copy link
Contributor

whot commented Jan 14, 2024

how to gracefully handle server → client keymap transfer, in case we have a different version of xkbcommon in both

very much a quickfire comment only here: the wayland protocol specifies the format as xkb_v1 so I think this use-case could be covered by adding a new version.

Alas, there is no version negotiation, so we'd have to add something like "I support keymap versions blah" to this protocol, or alternatively move into a protocol extension that supplies other protocol formats.

@wismill
Copy link
Member Author

wismill commented Jan 15, 2024

@whot Yes, I think this is the proper way. I really wanted to avoid this and do minimal effort to fix the aforementioned bugs. In my mind, xkb_v2 would be the glorious XKB 2, with probably a simpler text format. Maybe xkb_v1_1 would better suit. But I have no strong opinion on this.

This does not solves the issue in xkeyboard-config though. The current solution I propose here helps for the step where the compositor parses the keymap. But in order to be a proper solution, it should parse the guarded feature and its fallback, and then provide one or the other depending of the version negotiation aforementioned.

My current solution is more a hack and now I do not feel comfortable with it.

@wismill wismill reopened this Jan 15, 2024
@wismill
Copy link
Member Author

wismill commented Jan 15, 2024

So, the gist of my approach to solve the issues above12 in xkbcommon is to add a new boolean field “onRelease” (subject to bikeshedding) to LockMods and LockGroup actions. I also withdraw from the idea of conditional comments.

As this is a backward-incompatible syntax, I opened the current ticket in order to assess the handling in xkeyboard-config of such change. It would be silly to dedicate time to develop a solution that we cannot deploy!

Let’s put the pieces together. I understand that the feasibility of extending the syntax could be achieved:

  • In xkbcommon:
    • Parsing & handling of the new feature.
    • When dumping the keymap: if the request format does not support the feature, then the following trivial fallback mechanism is used: ignore the new feature altogether and use the historic behavior.
    • A mechanism that allow the registry to include a XML file in another (see hereinafter).
  • In Wayland:
    • Add a new xkb_v1_1 format.
    • Add a new Wayland extension to handle keymap text format version negociation.
  • In xkeyboard-config:
    • Separate files for the new format. That would be e.g. symbols/groups-v1.1, symbols/caps-v1.1.
    • A new evdev-v1.1 rules set, that include the previous evdev.
    • evdev-v1.1.xml would import evdev.xml and add specific v1.1 entries. I think this requires a new feature in xkbcommon though.
  • In OS distributions: set the default rules set depending of the installed versions of xkbcommon and xkeyboard-config.
    Distros will certainly be careful with such a change, but users are free to experiment once they get the proper xkbcommon version installed.

That is certainly a good amount of work!

Some further comments:

  • All names/versions are subject to bikeshedding.
  • I do not plan to add more features to XKB 1, but only fix some limitations of the current protocol. So no v1.2 planned.
  • Having version negotiation would open the door for an hypothetical XKB 2.

So, to me it sounds feasible this way, although this can be long way, especially in Wayland.

@whot @bluetech I would like your feedback:

  • What is your opinion on extending the syntax, following my proposal above?
  • Do you think the bugs I mentioned above would be better fixed another way? E.g. with a special ad-hoc fix only in xkbcommon, without modifying the xkb files in xkeyboard-config?

Footnotes

  1. Need to kick hotkeys on release, not press, etc.

  2. Option to apparently unlock the Lock modifier on press of Caps Lock

@whot
Copy link
Contributor

whot commented Jan 29, 2024

Recommended addition: adding parsing (but not necessarily handling) to xkbcomp. This should be simple enough once the libxkbcommon part is done and will effectively give you full coverage on anything that updates, so you effectively punt much of the backwards compatibility trickiness to distributions that need to support old combinations.

For xkeyboard-config: I would definitely advise against separate files within the installation. I think a easier-to-manage approach would be something like duplicating the whole tree via e.g. /usr/share/X11/xkb/v1.1/. Using hard/symlinks this can be reduced to a negligible size.

For Wayland: I'm not a 100% sure if it needs to be a new extension or can be added to a new version of wl_keyboard, this is something best discussed at in the wayland gitlab repo. (as an extension it'd be possible to even add a generic version negotiation extension that can be re-used by other protocols but that probably goes too far).

I was hoping we might be able to add some new action like LatchModsOnRelease but xkbcomp doesn't like that. Anyway, leaving this idea here in case it sparks something else in your head :)

Other than my hope for a new action the only ad-hoc fixes I can think of are using the Private action (we use that for XF86LogGrabInfo). The feature is very contained so going with a private action here may be a, haha, option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compile-keymap Indicates a need for improvements or additions to keymap compilation discussion: backward compatibility
Projects
None yet
Development

No branches or pull requests

2 participants