stylix: add `stylix.enable` option

[jalil-salame]

About

Add a stylix.enable option to enable or disable all Stylix modules in order to resolve issues similar to https://github.com/danth/stylix/issues/216.

To align with the default lib.mkEnableOption behavior, stylix.enable defaults to false.

Breaking Change

Stylix is disabled by default. To enable it, use:

stylix.enable = true;

Closes

• https://github.com/danth/stylix/issues/216

TODO

• [X] Manually integration test in Home Manager setup 1
• [ ] Manually integration Test in NixOS setup

Commit Message

stylix: add 'stylix.enable' option

Add a 'stylix.enable' option to enable or disable all Stylix modules in
order to resolve issues similar to [2].

To align with the default 'lib.mkEnableOption' [1] behavior,
'stylix.enable' defaults to 'false'.

BREAKING CHANGE: Stylix is disabled by default. To enable it, use:

    stylix.enable = true;

[1]: https://github.com/NixOS/nixpkgs/blob/23.11/lib/options.nix#L91-L105
[2]: https://github.com/danth/stylix/issues/216

[jalil-salame]

I don't have the bandwidth to test this PR, seeing that it has been stale for ~3 months I'll mark it as ready, and if someone tests it and finds some problems I'll happily fix them.

[jalil-salame]

Ci is failing, I think we need to set sylix.enable = true in CI.

[trueNAHO]

Ci is failing, I think we need to set sylix.enable = true in CI.

CI is successful now: https://github.com/danth/stylix/actions/runs/8958380642.

I tested stylix.enable = false; and stylix.enable = true; in my Home Manger configuration, and so far everything works as expected.

@jalil-salame, could you maybe double check this PR on your setup?

[jalil-salame]

My setup relies on #201 so it would be a mess to use this PR sadly.

[jalil-salame]

I was nerd sniped into trying it out anyways, both true and false work (with some minor modifications). By work I mean compile, my styling relies on #201 so it is broken in this PR.

[trueNAHO]

I tested stylix.enable = false; and stylix.enable = true; in my Home Manger configuration, and so far everything works as expected.

[jalil-salame]

I'm adding this PR to my config (here if you are curious), so I'll be able to test the Linux modules a bit more thoroughly soon c:

[trueNAHO]

For reference, I would use the following commit message when merging this PR:

stylix: add 'stylix.enable' option

Add a 'stylix.enable' option to enable or disable all Stylix modules in
order to resolve issues similar to [2].

To align with the default 'lib.mkEnableOption' [1] behavior,
'stylix.enable' defaults to 'false'.

BREAKING CHANGE: Stylix is disabled by default. To enable it, use:

    stylix.enable = true;

[1]: https://github.com/NixOS/nixpkgs/blob/23.11/lib/options.nix#L91-L105
[2]: https://github.com/danth/stylix/issues/216

[jalil-salame]

Considering that stylix.enable defaults to false, why does https://github.com/danth/stylix/pull/244/commits/f957c280ec73e9bfda49f3a78b7aebc742e61612 cause the following error:

$ nix build .#docs
error:
       … while calling the 'derivationStrict' builtin

         at /builtin/derivation.nix:9:12: (source not available)

       … while evaluating derivation 'stylix-book'
         whose name attribute is located at /nix/store/3lllcglr2ip90mkxs5q4asl98r305mia-source/pkgs/stdenv/generic/make-derivation.nix:331:7

       … while evaluating attribute 'patchPhase' of derivation 'stylix-book'

         at /nix/store/blw4ppvzwz08gw7dxbc41pxv83qgr6pi-source/docs/default.nix:44:3:

           43|
           44|   patchPhase = ''
             |   ^
           45|     cp ${../README.md} src/README.md

       (stack trace truncated; use '--show-trace' to show the full trace)

       error: A definition for option `stylix.image' is not of type `path or package convertible to it'. Definition values:
       - In `/nix/store/3lllcglr2ip90mkxs5q4asl98r305mia-source/flake.nix': null

Ideally, it should be possible to revert https://github.com/danth/stylix/pull/244/commits/e8e3304c2f8cf2ca60dcfc736a7422af2f24b8a8.

I'd need to look at the code carefully, but if a lib.mkIf removes imports or options then you'd expect that behavior.

[trueNAHO]

Considering that stylix.enable defaults to false, why does f957c28 cause the following error:

$ nix build .#docs
error:
       … while calling the 'derivationStrict' builtin

         at /builtin/derivation.nix:9:12: (source not available)

       … while evaluating derivation 'stylix-book'
         whose name attribute is located at /nix/store/3lllcglr2ip90mkxs5q4asl98r305mia-source/pkgs/stdenv/generic/make-derivation.nix:331:7

       … while evaluating attribute 'patchPhase' of derivation 'stylix-book'

         at /nix/store/blw4ppvzwz08gw7dxbc41pxv83qgr6pi-source/docs/default.nix:44:3:

           43|
           44|   patchPhase = ''
             |   ^
           45|     cp ${../README.md} src/README.md

       (stack trace truncated; use '--show-trace' to show the full trace)

       error: A definition for option `stylix.image' is not of type `path or package convertible to it'. Definition values:
       - In `/nix/store/3lllcglr2ip90mkxs5q4asl98r305mia-source/flake.nix': null

Ideally, it should be possible to revert e8e3304.

I'd need to look at the code carefully, but if a lib.mkIf removes imports or options then you'd expect that behavior.

Considering this merely cleans up the code, this should probably be addressed in a followup-PR.

NOTE: I added a list of things that should be verified right before merging, to the first PR message.

@danth, could you review this PR?

[jalil-salame]

~I am testing in in my configuration, but setting stylix.enable = false; on my nixos configuration does nothing, the styling is still applied.~

Edit: I had autoEnable = true set. The behavior of autoEnable is similar to enable, we might want to consider changing this.

[jalil-salame]

Works on my configuration now that I fixed the autoEnable thing c:

[jalil-salame]

The changes to autoEnable look good! I haven't tested it, but it should work with my previously broken config c:

broken is a bit harsh, but yeah, it was mostly a me problem c:

[danth]

After the changes in #398 and #399, would it make more sense to do the following?

1. For modules which pass something other than a constant true or false to mkEnableTarget, move this to a lib.mkIf condition within the module itself. Then, rather than "auto-enable", we would simply have some modules which, when enabled, don't produce an effect unless the related program is installed.
2. Rename stylix.autoEnable to stylix.enable and change its default value to false.

After this, the boolean argument to mkEnableTarget would always be a constant, so the function would behave as follows:

• When true, the default value follows stylix.enable and the documentation explains this.
• When false, it's just a wrapper around lib.mkEnableOption which inserts the words "styling for" within the description.

We could make this behaviour even more clear by splitting it into two different functions.

I think removing the autoEnable option entirely is a much clearer way to go about this.

[trueNAHO]

After the changes in #398 and #399, would it make more sense to do the following?

1. For modules which pass something other than a constant true or false to mkEnableTarget, move this to a lib.mkIf condition within the module itself. Then, rather than "auto-enable", we would simply have some modules which, when enabled, don't produce an effect unless the related program is installed.

Do you mean replacing

https://github.com/danth/stylix/blob/ebaed9d4bf258f4eda7d0690c4092fadcbeefa9d/modules/lightdm/nixos.nix#L3-L9

with

{
  options.stylix.targets.lightdm.enable =
    config.lib.stylix.mkEnableTarget "LightDM" true;

  config.services.xserver.displayManager.lightdm.background =
    lib.mkIf
    (config.stylix.enable && config.stylix.targets.lightdm.enable)
    config.stylix.image;
}

2. Rename stylix.autoEnable to stylix.enable and change its default value to false.

Considering stylix.enable defaults to false, this simply removes the stylix.autoEnable option.

After this, the boolean argument to mkEnableTarget would always be a constant, so the function would behave as follows:

• When true, the default value follows stylix.enable and the documentation explains this.
• When false, it's just a wrapper around lib.mkEnableOption which inserts the words "styling for" within the description.

We could make this behaviour even more clear by splitting it into two different functions.

What about stylix.mkEnableTarget defaulting to true in order to allow end-users to opt-out of certain targets:

mkEnableTarget = name: lib.mkEnableOption "styling for ${name}" // {
  default = true;
  example = false;
};

In this case, we could wrap lib.mkIf by prefixing the stylix.enable option:

mkIf = condition: lib.mkIf (config.stylix.enable && condition);

This would simplify the module pattern to:

{
  options.stylix.targets.<TARGET>.enable = mkEnableTarget "<NAME>";
  config = mkIf config.stylix.targets.<TARGET>.enable <CONFIG>;
}

Consequently, all targets are disabled by default, as stylix.enable defaults to false. Additionally, end-users can opt-out of a target with

config.stylix.targets.<TARGET>.enable = false;

if stylix.enable is true.

Assuming the underlying upstream (NixOS or Home Manager) modules are guarded with their own enable options, setting their options should have no effect, unless end-users explicitly opt-in.

I think removing the autoEnable option entirely is a much clearer way to go about this.

Indeed. stylix.enable and stylix.autoEnable are rather confusing together.

[danth]

Do you mean replacing

No, I mean cases where the argument to mkEnableTarget takes the value of some other option, for example

https://github.com/danth/stylix/blob/99bcaa552096ccff21658a9eb6a1e8397b10eb78/modules/gnome/nixos.nix#L8-L10

rather than a simple true or false.

In this case we would replace config.services.xserver.desktopManager.gnome.enable with a constant true, and then add lib.mkIf config.services.xserver.desktopManager.gnome.enable later in the module.

This will prevent the documentation changing depending on the value of services.xserver.desktopManager.gnome.enable in the configuration where it's generated.

What about stylix.mkEnableTarget defaulting to true

If we make the default value follow stylix.enable, then the user can either:

• Set stylix.enable to false, then opt in to individual modules by setting stylix.targets.«name».enable to true
• Set stylix.enable to true, then opt out from individual modules by setting stylix.targets.«name».enable to false

[trueNAHO]

I noticed that my previously proposed module pattern could be simplified by prefixing the stylix.enable option in mkEnableTarget:

let
  mkEnableTarget = name: condition:
    lib.mkEnableOption "styling gggfor ${name}" // {
      default = config.stylix.enable && condition;
      example = false;
    };
in {
  options.stylix.targets.<TARGET>.enable = mkEnableTarget "<NAME>";
  config = lib.mkIf config.stylix.targets.<TARGET>.enable <CONFIG>;
}

What about stylix.mkEnableTarget defaulting to true

If we make the default value follow stylix.enable, then the user can either:

• Set stylix.enable to false, then opt in to individual modules by setting stylix.targets.«name».enable to true
• Set stylix.enable to true, then opt out from individual modules by setting stylix.targets.«name».enable to false

Where exactly should this value be set? Setting it in the default value in the mkEnableTarget option results in the following truth table (bold values are default values), which is surprisingly independant of config.stylix.enable:

config.stylix.enable	condition	default
0	0	0
0	1	1
1	0	0
1	1	1

I assume I did something wrong.

Actually, on second thought, it would be counterintuitive if Stylix modules could be enabled despite Stylix itself being disabled with stylix.enable = false;. This would go against the fundamental purpose of the .enable options. To keep the functionality of enabling or disabling all Stylix styling, it seems like a autoEnable option is good after all. Maybe we should rename it to something more explicit, like enableAll?

[jalil-salame]

Apparently the home-manager module is not inheriting the enable option properly: https://github.com/jalil-salame/configuration.nix/pull/77#discussion_r1623241507

To test this you can clone my configuration:

$ git clone https://github.com/jalil-salame/configuration.nix
$ cd configuration.nix
$ nix run nixpkgs#just run-vm

[!Note] You can use just build-vm to ensure the configuration builds. Also, because I compile lix it might take a while to run (~6m the first time on my laptop).

This builds and runs a qemu vm with my configuration, the user and passwords are jdoe and example respectively.

As you can see, the TTY has the gruvbox colorscheme applied (switch to a different TTY to verify). But if you remove the mentioned line, the home-manager styling is missing (no background, wrong colorscheme in the terminal/dmenu (Super+Return/Super+D)).

I'm guessing this is not intended, but it's an easy fix from my side (I have a styling.enable option that I can query to turn stylix on/off and I can repurpose it to also turn on the home-manager stylix module.

[danth]

To keep the functionality of enabling or disabling all Stylix styling, it seems like a autoEnable option is good after all. Maybe we should rename it to something more explicit, like enableAll?

I agree, it seems like both options have a purpose.

A name change may be appropriate, but let's leave that for a follow-up PR to avoid delaying this one much longer. Options can be renamed in a backwards compatible way using mkRenamedOptionModule, so it won't be another breaking change.

Apparently the home-manager module is not inheriting the enable option properly

The new option needs to use fromOs to pull the default from NixOS. I'll commit the edit myself to save time.

Once that's done, I'll test the following:

• [ ] Stylix should only take effect when stylix.enable is true
• [x] stylix.enable carries over to Home Manager automatically (when configured together)
• [x] stylix.image should not be required when Stylix is disabled
• [x] Setting stylix.autoEnable to false should mean targets need to be selected manually, as before
• [ ] The reference documentation is understandable
  • [x] For the new stylix.enable option
  • [ ] For stylix.autoEnable
  • [ ] For stylix.targets.*.enable
• [x] The breaking change is displayed throughout the website and README

All being well this should be ready to merge soonish.

[danth]

So, I've found a couple of problems which still need to be fixed:

Issue 1

When stylix.enable is false, you can still enable a target by explicitly setting stylix.targets.«name».enable to true. This goes against the idea of a global on/off switch.

I think the behaviour we settled on was this:

• When stylix.enable is false, disregard all other settings and do nothing.
• Otherwise, enable each target based on stylix.targets.«name».enable.
• The default for stylix.targets.«name».enable should be as follows:
  • Argument to mkEnableTarget is true: the default is whatever stylix.autoEnable is set to.
  • Argument to mkEnableTarget is false: the default is false.

To fix this, each module should include config.stylix.enable as part of its mkIf condition.

Issue 2

When the argument to mkEnableTarget is not a constant, the documentation shows the default value as being false unless the target happens to be installed in this NixOS system.

This can be fixed by replacing any non-constant arguments to mkEnableTarget with a constant true, and moving the previous argument into the mkIf condition.

Simplification

Since every module will now include at least lib.mkIf (config.stylix.enable && config.stylix.targets.«name».enable), and possibly lib.mkIf (config.stylix.enable && config.stylix.targets.«name».enable && «check that the target is installed»), we may want to introduce a function to reduce repetition.

Since this will likely involve some discussion, I suggest just using the long conditions for now so we can get this merged. This part of the code is not user-facing, so cleaning it up won't be a breaking change.

@jalil-salame I don't want to ask you to do the tedious work of editing the mkIf in every single module, so I'll do that myself and commit it here. I hope that's okay with you.

[jalil-salame]

@jalil-salame I don't want to ask you to do the tedious work of editing the mkIf in every single module, so I'll do that myself and commit it here. I hope that's okay with you.

I find this kind of repetitive work therapeutic c: but I don't have the availability to spend much time working on it (although I could maybe spare some time tomorrow 🤔).

Feel free to take over this PR c: I'm not over protective of my code/PRs

[danth]

I managed to do the first part with some sed magic, just need to go through and fix the second issue by hand now.

[danth]

Can docs/settings.nix be removed due to the new stylix.enable option?

Not yet: although the option is not required any more, the default value for stylix.image is still of an incorrect type, so it needs to be overridden. This will be fixed in #407, after which it should be possible to remove the file.

[trueNAHO]

Is the following commit message good:

stylix: add 'stylix.enable' option

Add a 'stylix.enable' option to enable or disable all Stylix modules in
order to resolve issues similar to [2].

To align with the default 'lib.mkEnableOption' [1] behavior,
'stylix.enable' defaults to 'false'.

BREAKING CHANGE: Stylix is disabled by default. To enable it, use:

    stylix.enable = true;

[1]: https://github.com/NixOS/nixpkgs/blob/23.11/lib/options.nix#L91-L105
[2]: https://github.com/danth/stylix/issues/216

Co-authored-by: Daniel Thwaites <[email protected]>
Co-authored-by: Jalil David Salamé Messina <[email protected]>
Co-authored-by: NAHO <[email protected]>

[danth]

Yep, I had already set that for auto-merge, just using ` rather than '