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

Do not use : to separate linux devices #121

Closed
oberblastmeister opened this issue Aug 31, 2022 · 8 comments · Fixed by #647
Closed

Do not use : to separate linux devices #121

oberblastmeister opened this issue Aug 31, 2022 · 8 comments · Fixed by #647
Labels
enhancement New feature or request linux Issue pertains to Linux only PRs welcome jtroo has no plans to work on this at present, but PRs are welcome

Comments

@oberblastmeister
Copy link
Contributor

Interpreting : as separating devices is a bad idea. We should just allow a list to be passed to linux-dev. For example

(defcfg
  linux-dev (dev1 dev2 dev3)
)
@jtroo jtroo added enhancement New feature or request PRs welcome jtroo has no plans to work on this at present, but PRs are welcome labels Aug 31, 2022
@jtroo
Copy link
Owner

jtroo commented Aug 31, 2022

The idea of using : comes from the Linux $PATH variable. Parsing a list is fine, but it should be added as an alternative and not a replacement, since it doesn't seem like a useful enough change to break existing configurations.

@jtroo
Copy link
Owner

jtroo commented Aug 31, 2022

As an aside, some communication advice: avoid unsubstantiated statements like "X is a bad idea". A better way to phrase this type of issue is something like:

The current approach has X, Y, Z weaknesses. The new approach A fixes these weaknesses and also has improvements B, C. Approach A has downsides D, E, but to me, these are worth the advantages.

@oberblastmeister
Copy link
Contributor Author

The idea of using : comes from the Linux $PATH variable.

Yes, but linux environment variables have no "types", everything is a string. By contrast, we are using a structured configuration file in the form of S-expressions. We should take advantage of that structure! It doesn't make sense to have one special case here when we use lists everywhere else.

Most paths in /dev/input/by-path/ have multiple colons in them. For example, pci-0000:00:14.0-usb-0:1:1.0-event. Why force the user to escape all those colons just to use a path? It is also not intuitive to treat colons as something special. If a new user puts a path into their configuration and it doesn't work because of the colon, it is probably very confusing.

To me, there are no disadvantages to using lists except breaking some existing configurations. We can always document this somewhere so I don't see this as a big issue.

@jtroo
Copy link
Owner

jtroo commented Sep 1, 2022

Sounds sensible. We can bump the major version to 2 to signal a breaking change if/when this is implemented.

@maueroats
Copy link

I suggest just adding an alternative option linux-dev-list. Then you won't have an incompatible change.

Since my dev list has 5 long ugly things in it (because so many keyboards get connected to my laptop at various times), I like the sound of having a list that I can break up into multiple lines.

@jtroo
Copy link
Owner

jtroo commented Dec 8, 2022

That sounds like a decent idea. For anyone that may want to implement this (including my future self), do be aware that currently, the entirety of defcfg is parsed as a HashMap<String, String>. It may be easier to have a different top-layer configuration item, e.g.

(defcfg)

(deflinux-devices
  /dev/input/1
  /dev/input/2
  ...
)

A consideration: should the two methods merge their items together, or should one take priority over the other?

@jtroo jtroo added the linux Issue pertains to Linux only label Aug 19, 2023
jtroo pushed a commit that referenced this issue Nov 22, 2023
1. Move validation of cfg values to `parse_defcfg` instead of validating
them all over the codebase. Reason: before kanata didn't show location
when parsing of a cfg value failed, now it does.

2. Replace `HashMap<String, String>` with `CfgOptions`, which holds only
validated cfg values. Reason: Allows to easily parse lists as values,
since now the actual value parsing is no longer delayed after parser
function exited. This was done with #121 in mind.

3. Move cfg options defaults to a `CfgOptions::default`. Reason: it's
good to have similar code in one place.
@rszyma
Copy link
Contributor

rszyma commented Nov 23, 2023

For anyone that may want to implement this (including my future self), do be aware that currently, the entirety of defcfg is parsed as a HashMap<String, String>

#624 have been merged, now we got the tech to easily parse lists in defcfg

@rszyma
Copy link
Contributor

rszyma commented Nov 23, 2023

One thing to note, that we now have not only linux-dev, but also linux-dev-names-include and linux-dev-names-exclude, so to keep consistency allowing lists as values should be implemented for all 3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request linux Issue pertains to Linux only PRs welcome jtroo has no plans to work on this at present, but PRs are welcome
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants