nixos/picom: refactor #391600
nixos/picom: refactor #391600
Conversation
re-submitting NixOS#379151
|
|
nixos/modules/services/x11/picom.nix
Outdated
| in | ||
| "${escape [ sep ] k}${sep}${mkValueString v};" | ||
| "${escape [ sep ] k}${sep}${mkValueString toList v};" | ||
| ); | ||
|
|
||
| # This serializes a Nix expression to the libconfig format. | ||
| mkValueString = |
There was a problem hiding this comment.
A libconfig generator has been added to Nixpkgs somewhat recently. This should all be replaced with pkgs.formats.libconfig.
There was a problem hiding this comment.
Hi, please review my recent changes and let me know what you think :D
There was a problem hiding this comment.
let me know if i made any mistakes im still very new to nix :(
There was a problem hiding this comment.
Why did you remove all the options under services.picom? If you really want to, you could move them under services.picom.settings with the original names and keep the documentation/type checking, but removing the entirely is a net negative in term of usability.
There was a problem hiding this comment.
I thought it would've been nicer to let the user take care of it, but I'll add those again, gimme a few mins
There was a problem hiding this comment.
Why did you remove all the options under
services.picom? If you really want to, you could move them underservices.picom.settingswith the original names and keep the documentation/type checking, but removing the entirely is a net negative in term of usability.
i added back pls review and let me know :))
removed existing options and implemented a 'settings' option to make the code cleaner, easy to maintain and to keep things in one place
There was a problem hiding this comment.
I'm having a hard time reviewing these changes. Can you please start again
-
without removing or changing the implementation (
picom.settings = mkDefaultAttrs { ... }) of the top-level options, so we don't have any breaking changes -
using
nix-shell --run treefmtto format the file, so the diff is readable
| "detect-transient" = cfg.detectTransient; | ||
| "detect-rounded-corners" = cfg.detectRoundedCorners; | ||
| "mark-wmwin-focused" = cfg.markWmwinFocused; | ||
| } // cfg.settings; # Allows overrides via `settings` |
There was a problem hiding this comment.
Why don't you let the module system do the merge? It's much more powerful than the // operator.
| }; | ||
|
|
||
| fadeExclude = mkOption { | ||
| type = types.listOf types.str; | ||
| default = [ ]; |
There was a problem hiding this comment.
Why did you remove the documentation?
| }; | ||
|
|
||
| package = mkPackageOption pkgs "picom" { }; | ||
| shadowOffsetX = mkOption { |
There was a problem hiding this comment.
I asked you to not remove these options to avoid unnecessary breackages, but it's pointless if you rename them.
|
Any update regarding this PR? Is there some help needed? The ricing community would very much appreciate to access to animations settings :D |
|
Hello sorry for the late response, I've been busy with exams. I'm still up to contribute and help with this. I'm not sure what needs to be done next so assist and let me know and I'll do it asap, thanks :)) |
re-submitting #379151 because I had accidentally deleted the previous branch from my nixpkgs fork :(
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.