nixos/dwm: init module#470575
Conversation
|
|
please format your PR title and commit message to meet the commit conventions, e.g.: nixos/dwm: add more module |
e55ffcd to
ed912e8
Compare
maevii
left a comment
There was a problem hiding this comment.
Very non-exhaustive first pass, this is a lot to review...
You should also add yourself into the meta.maintainers of the module, I'm not sure this would get maintained particularly well otherwise. The dwm package itself also currently has no maintainers, so you could consider becoming one as well.
| pkgs, | ||
| ... | ||
| }: | ||
| with lib; |
There was a problem hiding this comment.
| with lib; |
This shouldn't be used (esp. in new modules), see #208242
There was a problem hiding this comment.
Should it be a inherit (lib) ...; instead or just use full names such as lib.mkOption?
There was a problem hiding this comment.
Either one is fine, I personally prefer specifying full names in my modules but it's up so you.
| options = { | ||
| flag = mkOption { | ||
| type = types.str; | ||
| description = "The flag or argument name"; |
There was a problem hiding this comment.
| description = "The flag or argument name"; | |
| description = "The flag or argument name."; |
Nitpick, but unlike meta.description these should be full sentences. Same with all the other descriptions.
| dwm = pkgs.dwm.overrideAttrs (oldAttrs: { | ||
| # if package source defined use it else use normal source | ||
| src = if cfg.package.src != null then cfg.package.src else oldAttrs.src; | ||
| /* | ||
| if you wish to add your own patch to the module then use the following format to do so. | ||
| make sure to remove anything editing the `config.def.h` to ensure that no errors occur | ||
| ++ (if <enable-patch> then [ <patch-dir> ] else []) | ||
| */ | ||
| patches = | ||
| (oldAttrs.patches or [ ]) | ||
| ++ cfg.package.patches | ||
| ++ (if cfg.patches.gaps.enable then [ ./gaps.diff ] else [ ]); |
There was a problem hiding this comment.
Never seen something like this in a module (and a quick search doesn't bring up anything), I'm not sure this should be included.
A package = lib.mkPackageOption ... option that would then be overridden with the postPatch would probably be much better (and much more maintainable, just the gaps patch is pretty big and could break with updates).
There was a problem hiding this comment.
Would it be better to do that and turn it into an overlay due to the fact if a person references the package it wouldn't use this one and instead use the default dwm package?
There was a problem hiding this comment.
Things like this are usually done by adding a readonly finalPackage option, see e.g. the postgresql module:
nixpkgs/nixos/modules/services/databases/postgresql.nix
Lines 131 to 151 in 3ceaaa8
There was a problem hiding this comment.
Ah so that the orignal pkgs are left to be, and the user can use config.programs.dwm.finalPackage instead. Correct how I think it works is wrong
| type = types.int; | ||
| default = 10; | ||
| example = 12; | ||
| description = ''The font size''; |
There was a problem hiding this comment.
| description = ''The font size''; | |
| description = "The font size."; |
Another nitpick, using '' on a single line looks weird. Same with the file/package descriptions and a few other things.
| }; | ||
| appCmd = mkOption { | ||
| type = types.str; | ||
| default = "dmenu_run"; |
There was a problem hiding this comment.
Is this actually installed anywhere in the module? It might be better to omit the default so users set it manually.
There was a problem hiding this comment.
Dmenu requires a flag inside it's args to know which monitor is open. The reason I made it an option is because inside dwm there is a variable that is for dmenu to know which monitor to spawn on.
Also I noticed that other wm included opts like these and I thought it'd be best to make sure that there is a applaucher as an option to make sure the user defines one.
There was a problem hiding this comment.
I'm not very familiar with dwm, is dmenu_run included in its package? That's what I was getting at, if it isn't then I don't think it should be made a default like this.
There was a problem hiding this comment.
dmenu_run is included as dmenu_run uses the path to find all binaries and runs the one you chose dmenu takes inputs and lets you chose which input to chose typically used for shell script.
Sorry reread your message no dwm doesn't include dmenu but it does have it as the default and as I am trying to keep all dwm defaults as the defaults inside the module.
However st and dmenu are a part of the suckless software which is why I chose to keep it, but yes with you saying this i will turn it into an example instead of a default.
|
|
||
| appCmd = mkOption { | ||
| type = types.str; | ||
| default = "st"; |
There was a problem hiding this comment.
Again, doesn't this need to be installed separately?
| argument = ''"#eeeeee"''; | ||
| } | ||
| ]; | ||
| example = '' |
There was a problem hiding this comment.
| example = '' | |
| example = lib.literalExpression '' |
Same with all the others.
There was a problem hiding this comment.
how would you like me to commit these changes?
There was a problem hiding this comment.
Check the nixpkgs commit conventions:
https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#commit-conventions
https://github.com/NixOS/nixpkgs/blob/master/nixos/README.md#commit-conventions
But TLDR, git commit --amend and git push --force usually works.
|
The package is set up this way because I wanted to create an overlay, so
that when a user installs it, their configuration takes precedence over the
default.
also how would you like me to commit these changes. Such as ammending the current commit or making new ones?
Thanks for all the suggestions — I’ll start working on them now.
…On Sun, 18 Jan 2026 at 06:07, Maevi ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Very non-exhaustive first pass, this is a lot to review...
You should also add yourself into the meta.maintainers of the module, I'm
not sure this would get maintained particularly well otherwise. The dwm
package itself also currently has no maintainers, so you could consider
becoming one as well.
------------------------------
In nixos/modules/programs/dwm/module.nix
<#470575 (comment)>:
> @@ -0,0 +1,619 @@
+{
+ lib,
+ config,
+ pkgs,
+ ...
+}:
+with lib;
⬇️ Suggested change
-with lib;
This shouldn't be used (esp. in new modules), see #208242
<#208242>
------------------------------
In nixos/modules/programs/dwm/module.nix
<#470575 (comment)>:
> +let
+ cfg = config.programs.dwm;
+
+ file = pkgs.writeText "config.c" (
+ (import ./file.nix) {
+ inherit lib;
+ inherit config;
+ }
+ );
+
+ appsSubmoduleType = types.listOf (
+ types.submodule {
+ options = {
+ flag = mkOption {
+ type = types.str;
+ description = "The flag or argument name";
⬇️ Suggested change
- description = "The flag or argument name";
+ description = "The flag or argument name.";
Nitpick, but unlike meta.description these should be full sentences. Same
all the other descriptions.
------------------------------
In nixos/modules/programs/dwm/module.nix
<#470575 (comment)>:
> + dwm = pkgs.dwm.overrideAttrs (oldAttrs: {
+ # if package source defined use it else use normal source
+ src = if cfg.package.src != null then cfg.package.src else oldAttrs.src;
+ /*
+ if you wish to add your own patch to the module then use the following format to do so.
+ make sure to remove anything editing the `config.def.h` to ensure that no errors occur
+ ++ (if <enable-patch> then [ <patch-dir> ] else [])
+ */
+ patches =
+ (oldAttrs.patches or [ ])
+ ++ cfg.package.patches
+ ++ (if cfg.patches.gaps.enable then [ ./gaps.diff ] else [ ]);
Never seen something like this in a module (and a quick search
<https://search.nixos.org/options?channel=25.11&query=patches+%7C%7C+src>
doesn't bring up anything), I'm not sure this should be included.
A package = lib.mkPackageOption ... option that would then be overridden
with the postPatch would probably be much better (and much more
maintainable, just the gaps patch is pretty big and could break with
updates).
------------------------------
In nixos/modules/programs/dwm/module.nix
<#470575 (comment)>:
> + argument = "{0}";
+ }
+ ];
+ };
+
+ font = {
+ name = mkOption {
+ type = types.str;
+ default = "monospace";
+ example = "JetbrainsMono NF";
+ };
+ size = mkOption {
+ type = types.int;
+ default = 10;
+ example = 12;
+ description = ''The font size'';
⬇️ Suggested change
- description = ''The font size'';
+ description = "The font size.";
Another nitpick, using '' on a single line looks weird. Same with the file
/package descriptions and a few other things.
------------------------------
In nixos/modules/programs/dwm/module.nix
<#470575 (comment)>:
> + example = 16;
+ description = "snap pixel";
+ };
+
+ appLauncher = {
+ modifier = mkOption {
+ type = types.str;
+ default = "MODKEY";
+ };
+ launchKey = mkOption {
+ type = types.str;
+ default = "XK_p";
+ };
+ appCmd = mkOption {
+ type = types.str;
+ default = "dmenu_run";
Is this actually installed anywhere in the module? It might be better to
omit the default so users set it manually.
------------------------------
In nixos/modules/programs/dwm/module.nix
<#470575 (comment)>:
> + };
+ };
+
+ terminal = {
+ modifier = mkOption {
+ type = types.str;
+ default = "MODKEY|ShiftMask";
+ };
+ launchKey = mkOption {
+ type = types.str;
+ default = "XK_Return";
+ };
+
+ appCmd = mkOption {
+ type = types.str;
+ default = "st";
Again, doesn't this need to be installed separately?
------------------------------
In nixos/modules/programs/dwm/module.nix
<#470575 (comment)>:
> + resizehints = mkOption {
+ type = types.bool;
+ default = true;
+ example = false;
+ description = "1 means respect size hints in tiled resizals";
+ };
+ lockfullscreen = mkOption {
+ type = types.bool;
+ default = true;
+ example = false;
+ description = "1 will force focus on the fullscreen window";
+ };
Maybe these could be mkEnableOption instead?
------------------------------
In nixos/modules/programs/dwm/module.nix
<#470575 (comment)>:
> + argument = ''"#222222"'';
+ }
+ {
+ flag = "-nf";
+ argument = ''"#bbbbbb"'';
+ }
+ {
+ flag = "-sb";
+ argument = ''"#5577"'';
+ }
+ {
+ flag = "-sf";
+ argument = ''"#eeeeee"'';
+ }
+ ];
+ example = ''
⬇️ Suggested change
- example = ''
+ example = lib.literalExpression ''
Same with all the others.
—
Reply to this email directly, view it on GitHub
<#470575 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BLDABXAWO27IU2IVWU7MEBD4HMPKFAVCNFSM6AAAAACO62YKWKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTMNZVGIZTQOBXGM>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
ed912e8 to
4e9f383
Compare
maevii
left a comment
There was a problem hiding this comment.
Still non-exhaustive second pass, you can/should also resolve review comments to keep track of them better (non-committer reviewers like me can't).
The actual module commit should also be named nixos/dwm: ... , see conventions
| windowManager.dwm = { | ||
| enable = true; | ||
| package = dwm; | ||
| }; |
There was a problem hiding this comment.
I didn't realize this was a thing, the functionality from this module should probably be integrated into that one rather than being separate like this.
There was a problem hiding this comment.
Sorry not sure what you mean with "the module should be integrated into that one rather than being seperate like this"?
There was a problem hiding this comment.
Means instead of having a programs.dwm and windowManager.dwm, we should implement the features of this PR into the existing windowManager.dwm module to avoid duplication, confusion, fragmentation, and other potential issues.
| /* c */ '' | ||
| ${cfg.file.prepend} | ||
| #define MODKEY ${cfg.modifier} | ||
| #define TAGKEYS(KEY, TAG) \ | ||
| {${cfg.tagKeys.modifiers.viewOnlyThisTag}, KEY, view, {.ui = 1 << TAG} }, \ | ||
| {${cfg.tagKeys.modifiers.toggleThisTagInView}, KEY, toggleview, {.ui = 1 << TAG} }, \ | ||
| {${cfg.tagKeys.modifiers.moveWindowToThisTag}, KEY, tag, {.ui = 1 << TAG} }, \ | ||
| {${cfg.tagKeys.modifiers.toggleWindowOnThisTag}, KEY, toggletag, {.ui = 1 << TAG} }, | ||
| #define SHCMD(cmd) { .v = (const char*[]){ "/bin/sh", "-c", cmd, NULL } } | ||
| static const unsigned int borderpx = ${toString cfg.borderpx}; | ||
| ${lib.optional (cfg.patches.gaps.enable) ''static const unsigned int gappx = ${toString cfg.patches.gaps.width};''} | ||
| static const unsigned int snap = ${toString cfg.snap}; | ||
| static const int showbar = ${boolToInt cfg.showBar}; | ||
| static const int topbar = ${boolToInt cfg.topBar}; | ||
| static char dmenumon[2] = "0"; /* component of dmenucmd, manipulated in spawn() */ | ||
| static const char *fonts[] = { "${cfg.font.name}:size=${toString cfg.font.size}" }; | ||
|
|
||
| /* layout(s) */ | ||
| static const float mfact = ${toString cfg.layout.mfact}; /* factor of master area size [0.05..0.95] */ | ||
| static const int nmaster = ${toString cfg.layout.nmaster}; /* number of clients in master area */ | ||
| static const int resizehints = ${toString cfg.layout.resizehints}; /* 1 means respect size hints in tiled resizals */ | ||
| static const int lockfullscreen = ${toString cfg.layout.lockfullscreen}; /* 1 will force focus on the fullscreen window */ | ||
|
|
||
| static const char *colors[][3] = { ${ | ||
| concatMapStringsSep ",\n" (name: pair: '' | ||
| [ ${name} ] = { "${pair.value.fg}", "${pair.value.bg}", "${pair.value.border}" } | ||
| '') (cfg.colors) | ||
| } }; | ||
|
|
||
| static const char *dmenucmd[] = { "${cfg.appLauncher.appCmd}", | ||
| ${lib.optionals (cfg.appLauncher.appArgs != [ ]) concatMapStringsSep " " ( | ||
| arg: ''"${toString arg.flag}", ${toString arg.argument},'' | ||
| ) cfg.appLauncher.appArgs} | ||
| NULL }; | ||
| static const char *termcmd[] = { "${cfg.terminal.appCmd}", | ||
| ${lib.optionals (cfg.terminal.appArgs != [ ]) concatMapStringsSep " " ( | ||
| arg: ''"${toString arg.flag}", ${toString arg.argument},'' | ||
| ) cfg.terminal.appArgs} | ||
| NULL }; | ||
|
|
||
| static const char *tags[] = { ${concatMapStringsSep ", " (tag: ''"${toString tag}"'') cfg.tags} }; | ||
|
|
||
| static const Layout layouts[] = { | ||
| ${concatMapStringsSep ",\n " ( | ||
| layout: ''{"${layout.symbol}", ${layout.arrangeFunction}}'' | ||
| ) cfg.layout.layouts} | ||
| }; | ||
|
|
||
| static const Rule rules[] = { | ||
| ${concatMapStringsSep ",\n " (rule: '' | ||
| { | ||
| "${rule.class}", ${rule.instance}, ${rule.title}, ${toString rule.tagsMask}, ${boolToInt rule.isFloating}, ${toString rule.monitor} | ||
| } | ||
| '') cfg.rules} | ||
| }; | ||
|
|
||
| static const Key keys[] = { | ||
| {${cfg.terminal.modifier}, ${cfg.terminal.launchKey}, spawn, {.v=termcmd}}, | ||
| {${cfg.appLauncher.modifier}, ${cfg.appLauncher.launchKey}, spawn, {.v=dmenucmd}}, | ||
|
|
||
| ${ | ||
| # create default key bindings before user defined bindings | ||
| concatMapStringsSep ",\n " ( | ||
| key: ''{${toString key.modifier}, ${key.key}, ${key.function}, ${key.argument} }'' | ||
| ) (if cfg.key.useDefault then defaultKeys ++ cfg.key.keys else cfg.key.keys) | ||
| }, | ||
|
|
||
| ${ | ||
| # create tage keys bindings | ||
| concatMapStringsSep "\n " ( | ||
| tag: ''TAGKEYS(${tag.key}, ${toString tag.tag})'' | ||
| ) cfg.tagKeys.definitions | ||
| } | ||
| }; | ||
|
|
||
| static const Button buttons[] = { | ||
| ${ | ||
| concatMapStringsSep ",\n " ( | ||
| button: ''{${button.click},${button.mask},${button.button},${button.function},${button.argument}}'' | ||
| ) cfg.buttons | ||
| }, | ||
| }; | ||
|
|
||
| ${cfg.file.append} | ||
| '' |
There was a problem hiding this comment.
Just for the sake of maintainability, the layout of this should probably be based on the upstream example
| (oldAttrs.patches or [ ]) | ||
| ++ cfg.package.patches | ||
| ++ (lib.optionals (cfg.patches.gaps.enable) [ ./gaps.diff ]); | ||
| postPatch = "cp ${file} config.h; cp ${file} config.def.h"; |
There was a problem hiding this comment.
| postPatch = "cp ${file} config.h; cp ${file} config.def.h"; | |
| postPatch = "cp ${file} config.h"; |
In addition to the thing from the first review pass, isn't config.def.h just a fallback for if config.h doesn't exist?
There was a problem hiding this comment.
While I was creating this module i had the issue where it wouldn't use the config.h config but i'll see if it's user error
1a34d4a to
79db688
Compare
|
Had to remove the patch and options that came with the module due to the gh bot failing it because treefmt doesn't format |
Be aware that the formatter was recently updated on Once you have the up-to-date formatter, |
c2b720e to
79db688
Compare
2a06c7d to
6737a05
Compare
2951c81 to
4558542
Compare
800f69e to
ddc4d9e
Compare
| tagsMask = 0; | ||
| isFloating = true; | ||
| monitor = -1; | ||
| }; |
There was a problem hiding this comment.
The type is listOf (submodule ...), this example is incorrect. I'd probably just delete this and make the default an example
| useDefault = mkOption { | ||
| type = types.bool; | ||
| default = true; | ||
| example = lib.literalExpresion false; |
There was a problem hiding this comment.
| example = lib.literalExpresion false; | |
| example = false; |
Same as in the previous review, literalExpression is only for strings when you want special formatting/comments in the example. This also still isn't the only place where it's still used incorrectly
|
Ok after talking with others about this module, people have sugested another way which seems more common within nix. Which is to include the option of a config file instead. I have no idea on whether I should continue using the current method, or use a "customRC" config option? |
Updating 5 files Co-authored-by: Maevi <me@maevi.net>
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.