-
-
Notifications
You must be signed in to change notification settings - Fork 14.7k
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
keyd: init at 2.2.5-beta and Keyd module #158793
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for submitting this PR!
Have not built it myself but left some general comments. Also, I am not sure if this belongs to the hardware directory. I don't know have a strong opinion for any other directory though.
}; | ||
|
||
configuration = mkOption { | ||
type = types.lines; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to allow a config file path as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not exactly filepath since keyd
expects it's configs under /etc/keyd
but I've made it possible to have multiple config files in the latest commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it needs a directory containing files passed to it rather than a specific file, there still isn't anything wrong with setting this to types.path
.
maintainers = [ "cidkid" ]; | ||
}; | ||
|
||
options = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably worth adding options applicationSpecificMapping.enable
and applicationSpecificMapping.config
if you are building it with Python (which is not required for the base version). Also, I don't see keyd-application-mapper
being called anywhere in the systemd service.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keyd recommends to use your wm or an autostart to launch keyd-application-mapper, and we can't define it in a declarative way due to the config path being in the users $HOME
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could write a systemd user service (i.e. systemd.user.services.keyd-application-mapper
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are users created here and now talks of running it as a user service. If it can run as a user service, it definitely doesn't need a hardcoded user.
I've been using this for over a month and it works great. It would be awesome to see it merged. |
default = {}; | ||
|
||
description = '' | ||
What the filename should be for configuration files for keyd (Multiple are allowed in keyd) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The description is a little confusing. Also can you add an example
?
configuration = mkOption { | ||
type = types.attrsOf (types.submodule ({ config, options, ... }: { | ||
options = { | ||
text = mkOption { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you gain anything by making this a submodule instead of just making it a listOf
? The name of the config file is largely irrelevant, isn't it? Also I'd prefer making this a lisfOf path
, to have the config in a separate file, but that's just personal preference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any point to allowing multiple configuration files? That sounds like an implementation detail to me...
Additionally, is there any way we can accept a nix
attribute set for the actual configuration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dit7ya requested the ability to have multiple config files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But what is the reason for multiple files? @aanderse's point about passing an attrset is also valid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add changelog entry as contributing document states.
|
||
config = mkIf cfg.enable { | ||
users.groups.keyd = { | ||
gid = 994; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is seeing a gid
necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not certain, I believe there was a reason before although I can't find it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a community rfc stating not to use uid
and gid
unless explicitly necessary. So see if you can remember or not.
configuration = mkOption { | ||
type = types.attrsOf (types.submodule ({ config, options, ... }: { | ||
options = { | ||
text = mkOption { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any point to allowing multiple configuration files? That sounds like an implementation detail to me...
Additionally, is there any way we can accept a nix
attribute set for the actual configuration?
}; | ||
}; | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing final new line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good.
I tested with the changes below and it works for me.
I wish upstream didn't take a hard line about restricting config paths to /etc/keyd (rvaiya/keyd#70); that is rather idiosyncratic. Ah well.
Let's get this merged!
Sorry about the long-wait on this, I've been busy! |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/how-to-bring-karabiner-elements-to-nixos/21011/7 |
Thanks for the work! This PR was mentioned in this discussion on NixOS official Discourse. |
This isn't closed, just accidentally updated my local nixpkgs tree and forced pushed, oops |
https://github.com/cidkidnix/nixpkgs/tree/cidkidnix/keyd, |
You might need to open a new PR if my understanding of how github works is correct. |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/how-to-get-the-service-running-for-keyd/24991/2 |
Expands on #158489, and allows for the per-application keyd experiment to work
Also closes #132474.