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

Update NixOS Module evremap #362661

Merged
merged 7 commits into from
Dec 22, 2024
Merged

Update NixOS Module evremap #362661

merged 7 commits into from
Dec 22, 2024

Conversation

pixL404
Copy link
Contributor

@pixL404 pixL404 commented Dec 7, 2024

I have updated the NixOS Module evremap.
I expanded the type for remappable keys to include BTN.
I added the optional phys attribute, which can be used if multiple devices share the same name. As toml doesn't support null values, I added a filter function to the toml generation.

When testing the phys attribute, an error occurs that prevents the service to start if you try to switch to the new generation without a reboot. The software wants to create a uinput device but some permissions were not set immediately. After rebooting the same config worked. I'm unsure if this is something to include in the description of the attribute.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

allow the key to be remapped start with BTN as well as KEY,
to enable remapping of mouse buttons
adds the optional attribute `phys` to uniquely identify a single device, if multiple
devices share the same name
filter out attrsets with a `null` value as toml doesn't support it
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Dec 7, 2024
@NixOSInfra NixOSInfra added the 12. first-time contribution This PR is the author's first one; please be gentle! label Dec 7, 2024
@github-actions github-actions bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Dec 7, 2024
@Scrumplex Scrumplex requested a review from pluiedev December 7, 2024 23:11
Copy link
Contributor

@pluiedev pluiedev left a comment

Choose a reason for hiding this comment

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

Why not

nixos/modules/services/misc/evremap.nix Outdated Show resolved Hide resolved
nixos/modules/services/misc/evremap.nix Outdated Show resolved Hide resolved
nixos/modules/services/misc/evremap.nix Outdated Show resolved Hide resolved
@pixL404
Copy link
Contributor Author

pixL404 commented Dec 9, 2024

@pluiedev done!

Copy link
Contributor

@pluiedev pluiedev left a comment

Choose a reason for hiding this comment

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

LGTM except I think you should probably squash the fixup commits and split changes based on areas you've changed (e.g. add phys option and allow BTN_* keys)

@pixL404
Copy link
Contributor Author

pixL404 commented Dec 9, 2024

Sorry if this is getting tedious but you mean in to squash in 3 commits right?

  1. update type
  2. add phys option (including change to toml generation)
  3. everything else

or do you mean squash into a single commit to have the PR as a single change?

@pluiedev
Copy link
Contributor

pluiedev commented Dec 9, 2024

No, just the first two commits (I believe the third commit, d22eeb2, is only needed because of the phys option, so it should be squashed into that commit)

adds the optional attribute `phys` to uniquely identify a single device, if multiple
devices share the same name

nixos/evremap: fix toml config generation

filter out attrsets with a `null` value as toml doesn't support it

nixos/evremap: use nixfmt

nixos/evremap: incorporate changes from review
@pixL404
Copy link
Contributor Author

pixL404 commented Dec 10, 2024

I think I borked it

@pixL404
Copy link
Contributor Author

pixL404 commented Dec 15, 2024

What steps are necessary to get this merged?

@pluiedev
Copy link
Contributor

pluiedev commented Dec 15, 2024

Get the attention of a committer and/or linking this PR to the "PRs already reviewed" thread on the NixOS Discourse

@pluiedev pluiedev added the awaiting_merger (old Marvin label, do not use) label Dec 15, 2024
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/2151

@Scrumplex Scrumplex merged commit 29b57ea into NixOS:master Dec 22, 2024
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 12. first-time contribution This PR is the author's first one; please be gentle! awaiting_merger (old Marvin label, do not use)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants