keyboard: add the ability to load layout from file #870

isti115 · 2023-08-12T19:40:31Z

As per the discussion on IRC that followed my issue (#869) I have added a separate riverctl keyboard-layout-file command with the file path as its single parameter.

The loading mechanism reads the file and passes the buffer to xkbcommon instead of the descriptor / handle to avoid dealing with the problem mentioned in #869.

Leon-Plickat · 2023-08-12T20:52:32Z

I am still curious what issue this solves, since custom xkbcommon layouts are already possible, provided you use the canonical path to put your layout file ($HOME/.xkb/symbols/).

isti115 · 2023-08-12T21:09:15Z

@Leon-Plickat Oh, I just replied to you in the issue.

I couldn't get that to work (quite possibly due to something I'm missing because of the lacking documentation, as my layout file doesn't seem to be completely valid, for example it does not work under XWayland, probably because I'm not using four character long key identifiers or something like that), but also, even if I could've, I prefer keeping my home directory as clean as possible and store these kinds of things somewhere else. (In this case, under $XDG_CONFIG_HOME/xkb, which is supposedly also a valid location [with even higher precedence], but doesn't work with keyboard-layout either.)

Anyway, having the flexibility to load a file from anywhere doesn't hurt in my opinion.

To stop the bifurcation of this conversation, I'll reply to your last comment on the issue here:

From what I can tell the problem is that your layout file doesn't give a name to the layout you define.
I recommend you look for a layout that is close to your custom one, then defining a variant of it. This you can then load.

I had issues with overriding certain parts of built-in layouts when I tried extending those, so that's not really a direction that I'd like to invest more time into, but I might try finding what's missing from my layout file.

Leon-Plickat · 2023-08-12T21:15:18Z

The reason you could not get it to work is because your custom layout does not have a name.

If you add your layout file to one of the canonical locations, xkb-common adds it to its database of layouts. Yes, even if you have a local config file, all global configuration is still loaded. Since it has no name, you can't access it. The name is not automatically inferred from the filename.

The name is a quoted string between xkb_keymap and {.

isti115 · 2023-08-12T21:18:18Z

I just tried adding a name there, but I'm still getting error: invalid value.
Do I need to flush any kind of magic cache of compiled xkm files or something?

Leon-Plickat · 2023-08-12T21:28:33Z

Probably. I suspect xkbcommon compiles its layout list on library init.

isti115 · 2023-08-21T07:19:31Z

Sorry for the delay, I have been away from home last week, but so far I still haven't managed to get my layout loaded through the currently available command.
@Leon-Plickat are you strongly opposed to merging this? I think that it would be useful even if we figure out the issue with the layout file on my end.
@ifreund What is your opinion on the matter?

Leon-Plickat · 2023-08-21T14:40:39Z

I am not opposed to this, I merely remain unconvinced this is necessary. My custom layout works find as is and I don't see why yours shouldn't.

Leon-Plickat · 2023-08-21T16:16:02Z

I have converted your layout file into the canonical form. You can find it here. Place those directories into ~/.xkb and make sure all the files you include are also in the right place (I was too lazy to hunt those down, but I am sure you have them somewhere). Use --rules vortex.

You can check if it compiles with

xkbcli compile-keymap --layout vortex --rules vortex

For riverctl keyboard-layout you'll also need to provide the rules.

river/command.zig

river/command/keyboard.zig

Leon-Plickat · 2023-08-21T16:24:20Z

river/command/keyboard.zig

+    var gpa = std.heap.GeneralPurposeAllocator(.{}){};
+    defer _ = gpa.deinit();
+
+    var file = std.fs.cwd().openFile(args[1], .{}) catch return error.CannotOpenFile;


Does it really make sense to have two different errors for being unable to open the file and being unable to read the file? I'd combine them.

Leon-Plickat · 2023-08-21T16:24:53Z

river/command/keyboard.zig

+    var file = std.fs.cwd().openFile(args[1], .{}) catch return error.CannotOpenFile;
+    defer file.close();
+
+    var file_bytes = file.readToEndAlloc(gpa.allocator(), 512 * 4096) catch return error.CannotReadFile;


Also I think you are leaking this buffer.

ifreund · 2023-08-21T16:46:08Z

@ifreund What is your opinion on the matter?

I would like to be convinced that this feature is necessary before reviewing/merging it.

Leon-Plickat · 2023-08-21T17:16:33Z

I would like to be convinced that this feature is necessary before reviewing/merging it.

I don't think it's strictly necessary, however it is arguably more convenient than splitting combined keymap files into separate files to make xkbcommon happy.

So I think it might be interesting to know how common it is to have these combined keymap files. Are they typical for weird / unusual keyboards? If yes, this feature might be nice sugar. Otherwise, if this is not common and just used by people DIYing their own layouts, I think it's fine to ask them to do it the Right Way™ instead.

isti115 · 2023-08-21T18:53:56Z

@Leon-Plickat RE: #870 (comment)

Thank you once again for taking your time with this!
I have followed your instructions by cloning the contents of the linked repository to ~/.xkb, but I still get error: invalid value when I try to load the layout.

xkbcli runs successfully (as it also does on my own layouts in .config/xkb/) and xkbcomp fails as expected (due to the custom keycode mapping names, as it also does during my previous experiments), which I am able to fix by converting all the identifiers to be similar to those used in the commonly distributed layouts. I have uploaded a version here that has this modification and all the includes inlined as well.
Vortex-Core-merged-compat.txt (Renamed to txt to fit GitHub's rules.)

I would think that this issue is orthogonal to the current one though, since as far as I know xkbcomp would only play a role if I wanted to use xwayland.

P.S. Thank you for reviewing the code as well, I'll go through your comments tomorrow and make adjustments accordingly. This was my first time writing zig, so any feedback is appreciated!

Leon-Plickat · 2023-08-21T19:53:47Z

On Mon Aug 21, 2023 at 8:54 PM CEST, István Donkó wrote: Thank you once again for taking your time with this! I have followed your instructions by cloning the contents of the linked repository to `~/.xkb`, but I still get `error: invalid value` when I try to load the layout.

I believe xkbcommon caches layouts, so river needs to be restarted. If you can compile the vortex layout with xkbcli, it should work in river. Otherwise track down the errors xkcli shows you.

ifreund · 2023-08-22T11:43:19Z

I believe xkbcommon caches layouts, so river needs to be restarted.

Would the dedicated keymap-file command work around this caching? That would be another point in favor of adding it.

Restarting river to tweak configuration isn't fun.

isti115 · 2023-08-22T13:21:03Z

Ah, I forgot about the caching since I brought it up last time (#870 (comment)), and voilà, after restarting river and having the layout definitions in their "canonical form" as provided by @Leon-Plickat I finally managed to load the layout without getting an invalid value error, but strangely only some of my keys work. 😮

For example none of the alphanumeric keys got properly assigned, they just show up as NoSymbol when checked through wev. At first I thought that this was due to those having my custom five level type, but I couldn't manage to get them working in any other way either, and I feel like I cannot reasonably allocate any more time to debugging this issue after already having an alternative solution that works reliably.

Also, iterating on this was pretty painful due to having to restart river each time (which didn't seem to reliably clear the cache either 🤨), so I also wanted to mention the faster (and less drastic) feedback loop as an advantage to the ability to directly load from the contents of a file, but it seems like @ifreund got ahead of me with that one. 😉

Another point could be the fact that this feature is present in other major window managers (such as Sway and Hyprland), so if it's missing from river, it could be an annoying issue for those who try to migrate. (The number of people using completely custom layouts being pretty low is a fair counterpoint though.)

Leon-Plickat · 2023-08-22T15:57:49Z

Would the dedicated keymap-file command work around this caching?

I think it would.

IIRC the part of xkbcommon that applies a keymap is separate from the part that compiles a keymap from the various files and caches them.

isti115 · 2023-08-22T16:23:07Z

I guess that this wasn't entirely clear from my previous message, but as far as I understand, there could technically be no caching issue with my approach, since xkbcommon doesn't even get the file path, I just pass the contents through a buffer, which it could cache as much as it wanted, that couldn't cause any issues, since the same contents should compile to the same layout. 😀

Anyway, I did test it before I wrote my previous reply, and keyboard-layout-file does properly apply the changes each time. I'll get to the requested code changes now.

isti115 · 2023-08-23T14:56:47Z

@Leon-Plickat I have pushed a fixup! commit with the requested changes, does it look good to you now?

(I had to explicitly return error.OutOfMemory as Zig didn't narrow the type of the error based on the matched switch case. 🤨)

novakne · 2023-08-23T18:13:11Z

river/command/keyboard.zig

+    var gpa = std.heap.GeneralPurposeAllocator(.{}){};
+    const allocator = gpa.allocator();
+    defer _ = gpa.deinit();
+


Why don't you use gpa from util.zig?

The answer if very simple, because I didn't know that it existed. 😃
Thanks for the review, I'll include this into the fixup!

Leon-Plickat · 2023-08-23T22:06:33Z

river/command/keyboard.zig

+    var file = std.fs.cwd().openFile(args[1], .{}) catch return error.CannotReadFile;
+    defer file.close();
+
+    var file_bytes = file.readToEndAlloc(util.gpa, 512 * 4096) catch |err| {


Not a fan of uncommented magic numbers in code, TBH.

Well, I stole that one from here:
https://github.com/ifreund/zig-wayland/blob/7f528883e057b282b1cb51db2899f9183b2e5a3b/src/scanner.zig#L191

It's a huge overkill for this purpose, but I don't know what's the best way to come up with a reasonable estimated upper limit for the size of XKB files. 🤔

The size isn't the issue, just that it is arbitrary. readToEndAlloc() will only allocate as much as is needed to store the contents of the file. As such, I'd use math.maxInt(usize).

And one more thing I forgot on the first review round: readToEndAlloc() can return error.FileTooBig, which should make your function return error.OutOfMemory.

Leon-Plickat · 2023-08-23T22:09:21Z

river/command.zig

@@ -112,6 +113,8 @@ pub const Error = error{
    InvalidOrientation,
    InvalidRgba,
    InvalidValue,
+    CannotReadFile,
+    CannotParseFile,


Suggested change

CannotParseFile,

Leon-Plickat · 2023-08-23T22:09:36Z

river/command.zig

@@ -155,6 +158,8 @@ pub fn errToMsg(err: Error) [:0]const u8 {
        Error.InvalidOrientation => "invalid orientation. Must be 'horizontal', or 'vertical'",
        Error.InvalidRgba => "invalid color format, must be hexadecimal 0xRRGGBB or 0xRRGGBBAA",
        Error.InvalidValue => "invalid value",
+        Error.CannotReadFile => "cannot read file",
+        Error.CannotParseFile => "cannot parse file",


Suggested change

Error.CannotParseFile => "cannot parse file",

I think that providing a separate read error and parse error has meaningful difference, for example, providing a wrong path leads to a read error, while providing a path to a file with contents that XKB cannot compile leads to a parse error.

river/command/keyboard.zig

isti115 · 2023-08-29T10:21:31Z

I think that with b41f1f8 I have now addressed all the reviews except for removing CannotParseFile, which I don't agree with currently. What are other's opinion on that? #870 (comment)

river/command/keyboard.zig

isti115 · 2023-09-05T11:53:34Z

@Leon-Plickat @ifreund Sorry for the ping, but it has now been a week since the last response. Do you think that there is anything else to be done before this can be merged?

ifreund

Hello, thanks for your patience!

Here's my proper in-depth review. When these comments are addressed this should be good to go.

river/command/keyboard.zig

doc/riverctl.1.scd

isti115 · 2023-10-18T16:08:20Z

@ifreund Thanks for the review, I'm a bit overwhelmed by work right now, but I'll make sure to finish this properly when I have the capacity to do so!

ifreund · 2023-10-18T17:08:55Z

@ifreund Thanks for the review, I'm a bit overwhelmed by work right now, but I'll make sure to finish this properly when I have the capacity to do so!

No worries at all, take your time!

isti115 · 2023-11-08T23:21:39Z

Finally getting around to cleaning this up! I have rebased my previous changes and updated the fixup commit with the requested changes from the review. Thanks again for the great piece of software, thorough review and patience! I have had some more ideas in the last few weeks of using River, so I will try to find more time for contributions either here, or in the form of smaller utilities or layout generators! :)

ifreund

Thanks for the patch! I did a bit more cleanup myself and added a link to the best documentation I could find on the XKB keymap file format.

isti115 force-pushed the master branch from a7e5437 to 9eae525 Compare August 12, 2023 19:46

Leon-Plickat requested changes Aug 21, 2023

View reviewed changes

isti115 force-pushed the master branch from 70a1584 to b7ee4e8 Compare August 23, 2023 14:55

isti115 force-pushed the master branch from b7ee4e8 to 7bdb87e Compare August 23, 2023 15:01

novakne reviewed Aug 23, 2023

View reviewed changes

isti115 force-pushed the master branch from 7bdb87e to a0ebfcd Compare August 23, 2023 18:23

Leon-Plickat reviewed Aug 23, 2023

View reviewed changes

Leon-Plickat requested changes Aug 23, 2023

View reviewed changes

river/command/keyboard.zig Outdated Show resolved Hide resolved

isti115 force-pushed the master branch from a0ebfcd to b41f1f8 Compare August 29, 2023 10:19

Leon-Plickat reviewed Aug 29, 2023

View reviewed changes

river/command/keyboard.zig Outdated Show resolved Hide resolved

isti115 force-pushed the master branch from b41f1f8 to 0659c19 Compare August 29, 2023 21:02

ifreund requested changes Oct 18, 2023

View reviewed changes

isti115 force-pushed the master branch 4 times, most recently from 7d044c7 to 34089d2 Compare November 8, 2023 23:16

keyboard: add the ability to load layout from file

927dceb

ifreund force-pushed the master branch from 34089d2 to 927dceb Compare November 9, 2023 12:24

ifreund approved these changes Nov 9, 2023

View reviewed changes

ifreund merged commit 927dceb into riverwm:master Nov 9, 2023
0 of 3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

keyboard: add the ability to load layout from file #870

keyboard: add the ability to load layout from file #870

isti115 commented Aug 12, 2023

Leon-Plickat commented Aug 12, 2023

isti115 commented Aug 12, 2023 •

edited

Leon-Plickat commented Aug 12, 2023 •

edited

isti115 commented Aug 12, 2023 •

edited

Leon-Plickat commented Aug 12, 2023

isti115 commented Aug 21, 2023 •

edited

Leon-Plickat commented Aug 21, 2023 via email

Leon-Plickat commented Aug 21, 2023

Leon-Plickat Aug 21, 2023

Leon-Plickat Aug 21, 2023

ifreund commented Aug 21, 2023

Leon-Plickat commented Aug 21, 2023

isti115 commented Aug 21, 2023 •

edited

Leon-Plickat commented Aug 21, 2023 via email

ifreund commented Aug 22, 2023 •

edited

isti115 commented Aug 22, 2023 •

edited

Leon-Plickat commented Aug 22, 2023

isti115 commented Aug 22, 2023 •

edited

isti115 commented Aug 23, 2023 •

edited

novakne Aug 23, 2023

isti115 Aug 23, 2023

Leon-Plickat Aug 23, 2023

isti115 Aug 23, 2023

Leon-Plickat Aug 24, 2023

Leon-Plickat Aug 23, 2023

Leon-Plickat Aug 23, 2023

isti115 Aug 29, 2023 •

edited

isti115 commented Aug 29, 2023

isti115 commented Sep 5, 2023

ifreund left a comment

isti115 commented Oct 18, 2023 •

edited

ifreund commented Oct 18, 2023

isti115 commented Nov 8, 2023 •

edited

ifreund left a comment

keyboard: add the ability to load layout from file #870

keyboard: add the ability to load layout from file #870

Conversation

isti115 commented Aug 12, 2023

Leon-Plickat commented Aug 12, 2023

isti115 commented Aug 12, 2023 • edited

Leon-Plickat commented Aug 12, 2023 • edited

isti115 commented Aug 12, 2023 • edited

Leon-Plickat commented Aug 12, 2023

isti115 commented Aug 21, 2023 • edited

Leon-Plickat commented Aug 21, 2023 via email

Leon-Plickat commented Aug 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ifreund commented Aug 21, 2023

Leon-Plickat commented Aug 21, 2023

isti115 commented Aug 21, 2023 • edited

Leon-Plickat commented Aug 21, 2023 via email

ifreund commented Aug 22, 2023 • edited

isti115 commented Aug 22, 2023 • edited

Leon-Plickat commented Aug 22, 2023

isti115 commented Aug 22, 2023 • edited

isti115 commented Aug 23, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isti115 Aug 29, 2023 • edited

Choose a reason for hiding this comment

isti115 commented Aug 29, 2023

isti115 commented Sep 5, 2023

ifreund left a comment

Choose a reason for hiding this comment

isti115 commented Oct 18, 2023 • edited

ifreund commented Oct 18, 2023

isti115 commented Nov 8, 2023 • edited

ifreund left a comment

Choose a reason for hiding this comment

isti115 commented Aug 12, 2023 •

edited

Leon-Plickat commented Aug 12, 2023 •

edited

isti115 commented Aug 12, 2023 •

edited

isti115 commented Aug 21, 2023 •

edited

isti115 commented Aug 21, 2023 •

edited

ifreund commented Aug 22, 2023 •

edited

isti115 commented Aug 22, 2023 •

edited

isti115 commented Aug 22, 2023 •

edited

isti115 commented Aug 23, 2023 •

edited

isti115 Aug 29, 2023 •

edited

isti115 commented Oct 18, 2023 •

edited

isti115 commented Nov 8, 2023 •

edited