By analyzing the blame information on this pull request, we identified @abbradar, @edwtjo and @edolstra to be potential reviewers

+1. Nitpick: why is the option called "enabled" with a 'd'? Most NixOS options are called "enable" (no 'd').

Right, enable in Nix is mostly used as a boolean option so foo.enable meaning is enable foo or not.
foo.enabled stand for "which one of foo options do I enable", but english is not my native language so I might make wrong assumptions.

So in my understanding:

• desktopManager.enable -> enable or not desktop manager
• desktopManager.enabled -> which desktop manager is enabled

My interpretation is that enable vs enabled is only about time: present/future vs past. IOW, having some "enable" and some "enabled" options would just make it inconsistent. (English isn't my native language either.)

@ericsagnes I agree with your interpretation: enable is a verb, enabled is a state, to my mind anyway. Not a big fan of "enabled", though, "manager" would be better, IMO, but still not perfect ...
EDIT: "enabled" implies a binary state space, so it really is semantically inappropriate.

More importantly, I wonder whether itemizing the possibilities in the docstring is required, won't that duplicate information already present in the enum?

I just picked a slightly different name to differentiate the option from the usual boolean enable but I am fine with any name.

More importantly, I wonder whether itemizing the possibilities in the docstring is required, won't that duplicate information already present in the enum?

I agree, this is just noise and detailed information should be in meta.doc. I added a commit to fix that.

I guess we can quibble over naming forever, so would be too bad if that were to hold this up, looks like an upgrade otherwise. "enabled" really doesn't make that much sense to me, but the docstring makes it clear enough, IMO.

👍 on the change to displayManager.

👎 on the change to desktopManager. It is possible now to enable multiple desktops and allow the user to choose one at login. Removing this feature is a pretty serious regression.

@ttuegel Thanks for the feedback, that was something I was unaware of.
As there seem to be some interest in this, I will work on a real implementation.

For desktop managers, making the option type types.nullOr (types.listOf (types.enum [ "enlightenment" "gnome3" "kde4" "kde5" "kodi" "xfce" "xterm" ])) could solve the problem.

So the configuration would become something like:

services.xserver.desktopManager.enable = [ "kde4" "xfce" ];

I'd drop the nullOr if there is no difference between null and [ ].

I updated the PR with a rough implementation, it still need some polishing and test but the few display/desktop/window manager related tests in release.nix I tested passed.

There might need assertions to make the system work nicely for edge cases like when incompatible window managers and desktop managers are enabled together.

This remove a fair amount of options and might break some configurations so it should be tested with nixos-rebuild build-vm and a custom nixos-config to avoid trouble.

So, the detail:

• there is no need to set services.xserver.enable = true; anymore, it will be set automatically if a wm or dm is enabled.
• services.xserver.desktopManager.enable is a list of enums, the first item in the list is the default desktop manager
• services.xserver.windowManager.enable is a list of enums, the first item in the list is the default window manager
• the auto display manager is disabled, just use slim.autoLogin and slim.defaultUser instead.
• the display manager is set depending the used desktop/window manager (just kdm or lightdm at the moment), but it can also be set manually with services.xserver.displayManager.enable.
• all the services.xserver.[window|desktop]Manager.*.enableoptions are gone in favor of services.xserver.[window|desktop]Manager.enable.

So for example to use xfce and kde and use xfce as the default session:

services.xserver.desktopManager.enable = [ "xfce" "kde4" ];

To use i3 only:

services.xserver.windowManager.enable = [ "i3" ];

To use xmonad and slim:

services.xserver.windowManager.enable = [ "xmonad" ];
services.xserver.displayManager.enable = "slim";

Nice!

On attribute names, I'd prefer something other than enable, since enable values are nearly always boolean. I could go with @joachifm's suggestion of manager, with managers for lists. I would have been okay with enabled (interpreted as an adjective, short for enabledManager/enabledManagers), but it's a little confusing.

I'm ok with either attribute name and don't think this discussion should block the pr getting merged. Maybe select is appropriate.

I think "enabled" is a better choice because then we get consistency with i18n.inputMethod.enabled.

Naming is always complicated, that is too bad we don't have maintainers for each module yet, it would have be best to let them decide.

Does @edolstra and @nbp have any preference about how to name the kind of option that enable one or multiple choices in a list like this PR windowManager.enable, displayManager.enable or desktopManager.enable does?

So far the propositions are:

• enable
• select
• enabled
• manager(s)

I'm a bit "meh" about this PR, which introduces a fair bit of incompatibility for little gain (i.e. saving one line in configuration.nix). And arguably the choice of whether to enable the X server and which desktop managers to enable should be independent options.

Another problem is that enums are not modular. For example, how can I extend the enumeration in nixos/modules/services/x11/window-managers/default.nix from an external module to add a new desktop manager?

An alternative is to have the desktop managers enable X11 and preferred display managers by default, e.g. KDE could do:

  config = mkIf config.services.xserver.desktopManager.kde4.enable {
    services.xserver.enable = mkDefault true;
    services.xserver.displayManager.kdm.enable = mkDefault true;
  };

Thanks for the feedback, I totally agree with the large incompatibilities and risks this PR brings, and at the moment I personally see it more as an experiment which the main point is to look for ways to be more user friendly.

Regarding the little gain, it depends on the standing point I guess, for a normal xmonad setup the following is currently needed:

services.xserver.enable = true;
services.xserver.windowManager.xmonad.enable = true;

services.xserver.windowManager.default = "xmonad";
# or
services.xserver.windowManager.xterm.enable = false;

The default = "monad"; or xterm.enable = false; requirement is in my humble opinion not user very friendly, and might require some search to find.

That is because the way windowManager.default and desktopManager.default logic is managed when none is set is quite arbitrary as it depends of the order of the imports in the module file.

Another problem is that enums are not modular. For example, how can I extend the enumeration in nixos/modules/services/x11/window-managers/default.nix from an external module to add a new desktop manager?

I think that at the moment this is not possible, but in the current implementation we also have non modular things like setting services.xserver.displayManager.slim.enable = false; in every display manager module that is not slim.
And to be fair, adding a new desktop manager also requires to modify nixos/modules/services/x11/desktop-managers/default.nix to put the import in the "right" position for the default option to work nicely.

An alternative is to have the desktop managers enable X11 and preferred display managers by default, e.g. KDE could do:

Please correct me if wrong, but I think that this alternative will not work well if, for example, the user enable gnome and kde4 and each enable a different display manager as there can be only one display manager enabled.
Unless of course, the user explicitly set the display managers he doesn't want to use to false, but then again he has to find which display managers are set by the desktop managers he enabled.

Anyway, I agree these are fair concerns that we should think when the PR is more mature to decide whether it is a real improvement and worth merging or not.

By the way, do you have any preference on the option name mentioned in my previous comment?

@ericsagnes

The reason I made the module system is because I had to modify NixOS sources to support my system. As a goal, I wanted to avoid modification of the NixOS sources to add any new displayManager, windowManager, desktopManager.

Thus, I absolutely wanted to avoid having an enumeration of names, as having such enumeration is likely a hint that imply that one is not able to extend NixOS with a new displayManager, windowManager, or desktopManager.

The way the list of window managers is made in the current set of patches ( https://github.com/ericsagnes/nixpkgs/blob/3d6c83aab2c11f5c2877b517b2a528e865d37c93/nixos/modules/services/x11/window-managers/default.nix#L43 ) is a limitation to the modularity.

The reason why modularity is so important is that one can use and extend NixOS, without having to give away the ownership of the code they used to build their system, such as password, …, while having the simplicity of updating their systems, without rebasing patches.

@nbp

Thus, I absolutely wanted to avoid having an enumeration of names, as having such enumeration is likely a hint that imply that one is not able to extend NixOS with a new displayManager, windowManager, or desktopManager.
The way the list of window managers is made in the current set of patches ( https://github.com/ericsagnes/nixpkgs/blob/3d6c83aab2c11f5c2877b517b2a528e865d37c93/nixos/modules/services/x11/window-managers/default.nix#L43 ) is a limitation to the modularity.

It totally make sense, but the current module system have also some un-modular parts.

• services.xserver.displayManager.slim.enable = false; in every display manager
• default desktopManager that depends on the order of imports
• possibility to set multiple contradictory enable options to true at the same time.

But yes, the approach of this PR is not the right one.

I wonder if something like extended option types could be implemented in the module system for such cases.

For example, some pseudo-code in the window manager module:

services.xserver.windowManager.enabled = mkOption {
  type = with types; listOf WM;
  default = [];
  description = "Enabled window manager";
}

...

types.WM = mkExtendableType {
  baseType = with types; sum;
  values   = [ ];
}

And in other module we extend the type, eg awesome module definition:

types.WM.values = [ "awesome" ];

And the values get merged according to the baseType definition and become the type of services.xserver.windowManager.enabled (a sum of all the wm names).

This way we could have modularity without dozens of possibly conflicting enable options.

Do you think this could be added to the current module system?

@ericsagnes

default desktopManager that depends on the order of imports

The order of imports is undefined, in the module system specification, as it can be cheated by importing the same module from else-where.

If the order is important, then the mkOrder properties should be used, such as mkBefore / mkAfter properties. https://github.com/NixOS/nixpkgs/blob/master/lib/modules.nix#L464-L470

possibility to set multiple contradictory enable options to true at the same time.

This should be guarded by assertions, which are suggesting how to fix things. Note that the module system cannot add modularity if the underlying program that it maps to does not support it. In such cases, assertions should prevent us from even generating configurations in such cases. The error would be to detect the error after a valid evaluation.

[…] And in other module we extend the type […]

Note that today we can already extend option declaration with non-conflicting option declarations. The enum type is not modular, as it only allow a finite set of options. Maybe we should add a typeMerge function to types which would give us the ability to merge types from multiple option declarations, Thus making enum modular.

There is already a special case implemented for submodules modules.

@nbp

Thanks for the pointers!
The typeMerge approach seems very interesting, will look into that.