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

Allow for merged KcCGST files and a system path #162

Merged
merged 6 commits into from Aug 30, 2020

Conversation

whot
Copy link
Contributor

@whot whot commented Jul 10, 2020

Three additions in this branch, it sits on top of #160 otherwise, starting at commit
"Add asprintf_safe helper function"

/etc/xkb as lookup path

This adds /etc/xkb as a lookup path for system-wide keyboard configurations. Fixes #40, see my long comment on why just a single path and not a conf.d style directory.

So our lookup path sequence becomes - in that order:

  • XDG_CONFIG_HOME/xkb
  • $HOME/.xkb
  • /etc/xkb
  • /usr/share/X11/xkb

Which gives us roughly the same behaviour as most other tools that have config files.

xkbcli scaffold tool

All directories still need to adhere to the same folder structure since the folder names and file names are part of the API. To make it simpler to get started with a custom XKB layout, there's a new tool called xkbcli scaffold. So a user would run, e.g.

$ xkbcli scaffold --user --layout "us(myvariant)" --option "custom:foo"

And that would create the required bits in $XDG_CONFIG_HOME/xkb. All they need now is learn how XKB files work and fill them in :) Passing --system does the same in the new /etc/xkb folder. Otherwise it's a fairly simple tool and it only writes to files that don't yet exist so it's safe to call twice (though ineffective).

Merge-include behaviour for KcCGST files

And since it's currently not possible to have us(myvariant) (see the same comment), change the KcCGST include behavior to run through all include paths.
So an include statement like include "us(basic)" will be attempted in in all symbols/us files in the include paths until the first one that works is found. It's thus possible to have a custom variant extending a system layout like this:

$ cat $XDG_CONFIG_HOME/xkb/symbols/us
partial alphanumeric_keys modifier_keys
xkb_symbols "myvariant" {
    name[Group1]= "myvariant (us)";
    include "us(basic)"
    key <CAPS> { [ Escape ] };
};

And this does the right thing.

cc @fooishbar and @svu because while I think this is save to do, I'm not sure if there are hidden side effects I haven't though of.

@bluetech
Copy link
Member

I'll review this when I get the next chance. In the mean time a rebase would be good.

Is this something you think 0.11.0 should include, or is it fine to release without?

@whot
Copy link
Contributor Author

whot commented Jul 27, 2020

Rebased on top of #173 now.

I renamed the tool to xkbcli-scaffold-new-layout but marked it install: false until we know it's useful. Rest is the same but has seen a few improvements in the code handling. Marking it as ready

Is this something you think 0.11.0 should include, or is it fine to release without?

I think we should include it. 0.10.0 added the user-specific files but they were only usable in a narrow context. With libxkbregistry and this the handling of custom files is complete (I think) and we have a good replacement for xmodmap (and of course custom/experimental keyboard layouts). If we get that in 0.11 in one go, life is a bit easier for everyone including writing documentation.

At least on my TODO list the only thing left now is to fix the xkeyboard-config format to be less awkward, but that's not going to happen anytime soon...

@whot whot marked this pull request as ready for review July 27, 2020 02:13
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.

Hi @whot, I looked at the changes and they mostly look good to me. Sorry for the delay.

Note that it needs a rebase.

One high level comment regarding the KcCGST merging behavior:

The strategy you've used for this is to reduce the granularity from the file level to the map level. This is good. The first file which has a matching map is used. Another alternative is to merge the maps of all files together. XKB has a bunch of merging machinery for this, with 3 different merge keywords augment, override, replace. Have you considered using that somehow instead of first-wins? I haven't considered it deeply myself, but wanted to mention it.

@@ -11,6 +11,8 @@ libxkbcommon searches the following paths for XKB configuration files:
- `$XDG_CONFIG_HOME/xkb/`, or `$HOME/.config/xkb/` if the `$XDG_CONFIG_HOME`
environment variable is not defined
- `$HOME/.xkb/`
- `$XKB_CONFIG_EXTRA_PATH` if set, otherswise `<sysconfdir>/xkb` (on most
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we can skip this envvar, unless it's really needed?

But if we keep it, xkbcommon.h has a doc section that needs to mention it (search for envvars).

There's also a include-path section there that needs to be updated for /etc/xkb in general.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we sort-of need it for the tests. By overriding that variable we can test default include path handling without having to actually rely on correct configuration in /etc/

test/tool-option-parsing.py Outdated Show resolved Hide resolved
tools/xkbcli-scaffold-new-layout.py Outdated Show resolved Hide resolved
tools/xkbcli-scaffold-new-layout.py Outdated Show resolved Hide resolved
if not xdgdir:
home = os.getenv('HOME')
if not home:
logger.error('Unable to resolve base directory from $XDG_CONFIG_HOME or $HOME')
Copy link
Member

Choose a reason for hiding this comment

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

This I think will show up with timestamps and stuff like that.

I think a print to stderr would be best, but can set a custom formatter otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Default output is this one which I thought was good enough for a niche tool like this:

ERROR:xkbcli:Unable to resolve base directory from $XDG_CONFIG_HOME or $HOME

tools/xkbcli-scaffold-new-layout.py Outdated Show resolved Hide resolved
tools/xkbcli-scaffold-new-layout.py Outdated Show resolved Hide resolved
tools/xkbcli-scaffold-new-layout.py Show resolved Hide resolved
fd.write(option_template)

footer = dedent(f'''\
// Include the system '{ruleset}' file
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if --user is used we should also somehow hook up /etc/xkb here? Might be a little difficult as it might (probably) not exist...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, and we'd need to run as root. And it's a niche case anyway, so overall I think it's not worth the bother. Or at least not until we see that users actually need this, right now it's all a bit of stab in the dark.

tools/xkbcli-scaffold-new-layout.py Show resolved Hide resolved
@whot
Copy link
Contributor Author

whot commented Aug 9, 2020

The strategy you've used for this is to reduce the granularity from the file level to the map level. This is good. The first file which
has a matching map is used. Another alternative is to merge the maps of all files together. XKB has a bunch of merging
machinery for this, with 3 different merge keywords augment, override, replace. Have you considered using that somehow
instead of first-wins? I haven't considered it deeply myself, but wanted to mention it.

My understanding is that those keywords apply at the map level, not at the file level. Technically, the "layout is the filename" is an implementation detail of xkeyboard-config and our parsers, there is nothing that requires this explicitly. We could switch to sqlite with the layout as index and it'd be correct too. So augment is really just include-with-custom-merge-behaviour but like the pure include it doesn't explicitly specify where the data should come from.

Merging the files together thus wouldn't change much I think - if anything it makes our behaviour parser-dependent. If you have a (virtual combined) file with two identically-named sections, is the parser going to pick the first one? or the last one? or will it throw an error in that case? The current approach is a bit more expressive, you know where things should be picked up from.

We could possibly design some sort of custom-tailored include instruction set but I think it'd be overkill for what we need.

@bluetech
Copy link
Member

bluetech commented Aug 9, 2020

My understanding is that those keywords apply at the map level, not at the file level.

They apply at a bunch of places -- search for MergeMode and OptMergeMode in src/xkbcomp/parser.y.

If you have a (virtual combined) file with two identically-named sections, is the parser going to pick the first one? or the last one? or will it throw an error in that case? The current approach is a bit more expressive, you know where things should be picked up from.

I'm not sure I understand what is parser-dependent. xkbcomp only ever looks at the XKB root, and the only other parser is us :) And this PR is changing the behavior.

Also not sure I follow about the expressiveness. The first-wins strategy is replace (in reverse?), so it can't be more expressive than something that offers more choices.

To be more concrete, let's say I want to modify something about the us layout. With the first-wins/replace strategy, the procedure is:

  • Add a variant, us(myvariant) to us file
  • Make myvariant include us(basic)
  • Add the desired modifications to myvariant
  • Add myvariant to the rules
  • Configure my system to use myvariant

With a map-merging augment strategy it will be

  • Add a us(basic) map to us file
  • Add the desired modifications to it

The trouble with this are:

  1. It changes the meaning of the us(basic) layout itself - maybe we do want to force a different name, but I think it's fine.
  2. I would require changing the parsing order such that system files are processed before the user files (so the user files can augment them). I think -- maybe not, I need to remind myself of the code here...

To be clear, I'm not advocating for this change, I'm pretty much offloading all of the thinking on this to you 😁 But I want to make sure we discuss all options. I'm also having a deja vu that we discussed this before already but maybe it's just a glitch in the Matrix.

@whot
Copy link
Contributor Author

whot commented Aug 10, 2020

Add myvariant to the rules

just ftr, this is only needed for options, not for variants due to how the rules catchall commands work. but yeah, that's just a lucky accident.

My argument of the parser dependency was: if you merge all files into a virtual combined file, you then need to also specify how the parser will treat sections that are doubly defined and ensure that all parsers (well, "both") parse correctly. By making it a lookup-path dependency, this behaviour is implied. It's a slightly wobbly argument but it's not a fully theoretical case - we recently had a bug where xkbcomp and libxkbcommon's parsing behaviour differed, see #153.

It changes the meaning of the us(basic) layout itself - maybe we do want to force a different name, but I think it's fine.

I disagree here, primarily for our benefit. Being able to modify the us layout and have it still be named us makes bug reports less reliable, let alone the various forums where custom modifications will spread like wildfire and make it more confusing for everyone. It's also a (subjective) interpretation of what layouts are and how people want to modify them. The naming of "it's a US layout but it's the foo variant" is useful and explicit enough, "it's a modified US layout" is harder. This isn't a technical argument, it's mostly one of long-term convenience for us as maintainers. And in terms of use-case: I'm aiming for the top-down configuration only. We assume the system files are constant and custom configuration can modify them or hide them altogether, but there's no bidirectional flow - you can't (easily) create a custom variant that includes a system file that then includes your custom other variant. It's probably technically possible but it's not really a use-case we should be worrying about. Where something like that is needed, it's cheaper to just modify the whole layout and effectively copy the system one into the custom configuration.

Ok, so with all that said, let's think about the technical implementation. I simply don't know where we'd put those instructions. Our components are spread across up to three files, $XDG_CONFIG_HOME, /etc/xkb and /usr/share/X11/xkb. The lookup behaviour is merely defined in the lookup paths [1] but otherwise it's not specified and it has no effect on the actual behaviour. Right now, augment should work just as well as include (not tested though).

IOW, right now there's nothing explicit about where us(basic) will come from. Picking different merge behaviours would require a place where to set those and right now, we don't really have one. I guess we could expand to allow things like include %S/us(basic) and so on but I'm not sure we really want to.

Maybe I'm missing something obvious here but right now I just don't know where we'd even configure different merge behaviours across files (and whether this would really have an effect).

[1] lookup paths are controlled by the parser, not the config. That's a potential issue but, well, uhm... don't know what to do about that either.

No functional changes, prep work for some other refacturing.

Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
Streamline the code a bit - instead of handling all the if (!file) conditions
handle the case of where we have a file and jump to the end.

Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
Previously, a 'symbols/us' file in path A would shadow the same file in path B.
This is suboptimal, we rarely need to hide the system files - we care mostly
about *extending* them. By continuing to check other lookup paths, we make it
possible for a XDG_CONFIG_HOME/xkb/symbols/us file to have sections including
those from /usr/share/X11/xkb/symbols/us.

Note that this is not possible for rules files which need to be manually
controlled to get the right bits resolved.

Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
This completes the usual triplet of configuration locations available for most
processes:
- vendor-provided data files in /usr/share/X11/xkb
- system-specific data files in /etc/xkb
- user-specific data files in $XDG_CONFIG_HOME/xkb

The default lookup order user, system, vendor, just like everything else that
uses these conventions.

For include directives in rules files, the '%E' resolves to that path.

Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
This tool set ups the required directory structure and template files to add new
keyboard layouts or options. For example, run like this:

    xkbcli-scaffold-new-layout  --layout 'us(myvariant)' --option 'custom:foo'

This will up the evdev rules file, the evdev.xml file, the symbols/us file and
symbols/custom file in $XDG_CONFIG_HOME so that the user has everything in place
and can start filling in the actual key mappings.

This tool is currently uninstalled until we figure out whether it's useful.

Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
… entries

It's a niche use-case but basically the same as adding symbols, so let's go with
a general handwavy explanation.

Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
@whot
Copy link
Contributor Author

whot commented Aug 26, 2020

Force-pushed with the requested fixed, the ones I've not marked as resolved haven't been changed - please see my comments there.

@bluetech bluetech merged commit ae90a6a into xkbcommon:master Aug 30, 2020
@bluetech
Copy link
Member

Thanks for tackling this @whot! I think we're about ready for release. I'll aim for this weekend. And let's call it 1.0.0 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pluggable layout support
2 participants