treewide: make stylix.image optional

[Flameopathic]

Closes: #200
Closes: #442

will make stylix.image optional, as long as stylix.base16Scheme is set

tasks

• alter most modules using config.stylix.image
• alter the option definition or set up a check to ensure that one of stylix.image or stylix.base16Scheme is set
• add option to each module
• alter the KDE module; would appreciate help from @danth who authored most of it, though if you don't have the time, i'm sure that i could figure it out eventually
• use cfg where necessary
• update docs
• test

i wonder if, in some places, pixel.nix would be useful or better than setting nothing at all, though i'm not sure

[trueNAHO]

• altered the KDE module; would appreciate help from danth who authored most of it, though if you don't have the time, i'm sure that i could figure it out eventually

This PR could be mostly generated with:

fd -e nix -X sed -i 's/config\.stylix\.image/lib.mkIf (\0 != null) \0'

• altered the option definition or set up a check to ensure that one of stylix.image or stylix.base16Scheme is set

I suggest adding an assertions where the config.stylix.image type is defined.

i wonder if, in some places, pixel.nix would be useful or better than setting nothing at all, though i'm not sure

Consistently wrapping with lib.mkIf (config.stylix.image != null) config.stylix.image might be the best solution.

For reference, #442 might contain more practical information.

[Flameopathic]

• altered the KDE module; would appreciate help from danth who authored most of it, though if you don't have the time, i'm sure that i could figure it out eventually

This PR could be mostly generated with:

fd -e nix -X sed -i 's/config\.stylix\.image/lib.mkIf (\0 != null) \0'

i'll test it out doing just that, though it seems more complicated than the rest

• altered the option definition or set up a check to ensure that one of stylix.image or stylix.base16Scheme is set

I suggest adding an assertions where the config.stylix.image type is defined.

good idea, done

i wonder if, in some places, pixel.nix would be useful or better than setting nothing at all, though i'm not sure

Consistently wrapping with lib.mkIf (config.stylix.image != null) config.stylix.image might be the best solution.

sounds good and makes sense; why do anything more complicated

For reference, #442 might contain more practical information.

ah, hadn't noticed it. are you interested in me adding the proposed wallpaper option for every module that uses config.stylix.image?

[trueNAHO]

For reference, #442 might contain more practical information.

[...] are you interested in me adding the proposed wallpaper option for every module that uses config.stylix.image?

Yes.

[Flameopathic]

can't seem to get KDE themed, even with an image using the official release of stylix. is it known to be broken?

either way it may be best to, for the moment, disable KDE theming when there is no image, as #708 looks like it's about to change the module entirely

[Flameopathic]

set up individual toggles. failing to set stylix.image and intentionally enabling one of the wallpapers (stylix.targets.<name>.wallpaper = true) will cause an error; i'd like to fix that with an assertion, but I don't see a way to do that without setting up an assertion for every one individually.

i still need help with KDE for reasons explained previously

testing of all other modules in progress

[lordkekz]

can't seem to get KDE themed, even with an image using the official release of stylix. is it known to be broken?

either way it may be best to, for the moment, disable KDE theming when there is no image, as #708 looks like it's about to change the module entirely

Yeah, KDE theme activation is currently broken for any setup where the activation isn't run inside a Plasma session.
I hope that #708 can get merged soon and you can rebase then to make your changes.

I think it'll be sufficient to just guard the if-block in the activator script and the magick-related commands in the themePackage builder with checks on whether the wallpaper is enabled.

[Flameopathic]

Yeah, KDE theme activation is currently broken for any setup where the activation isn't run inside a Plasma session. I hope that #708 can get merged soon and you can rebase then to make your changes.

got it, thanks

I think it'll be sufficient to just guard the if-block in the activator script and the magick-related commands in the themePackage builder with checks on whether the wallpaper is enabled.

awesome, double thanks

[trueNAHO]

set up individual toggles. failing to set stylix.image and intentionally enabling one of the wallpapers (stylix.targets.<name>.wallpaper = true) will cause an error; i'd like to fix that with an assertion, but I don't see a way to do that without setting up an assertion for every one individually.

Can you elaborate? I assume the following should be the pattern:

{
  config,
  lib,
  ...
}: let
  target = "<TARGET>";
in {
  options.stylix.targets.${target} = {
    enable = config.lib.stylix.mkEnableTarget target true;

    # TODO: Define config.lib.stylix.mkEnableWallpaper with an appropriate
    # description.
    wallpaper = config.lib.stylix.mkEnableWallpaper target true;
  };

  config = let
    cfg = config.stylix.targets.${target};
  in
    lib.mkIf (config.stylix.enable && cfg.enable) {
      programs.${target} = {
        enable = true;

        wallpaper =
          lib.mkIf
          (cfg.wallpaper && config.stylix.image != null)
          config.stylix.image;
      };
    };
}

Extracting the hard-coded target = "<TARGET>"; variable in each module could be done in a seperate initial commit.

I could quickly do this in a separate PR, but getting my PRs merged takes quiet long since danth is rather slow to respond. However, keeping that commit in this PR will be rather annoying, since we need to resolve breaking changes with new modules introduced in master...

[Flameopathic]

Can you elaborate? I assume the following should be the pattern:
...

I must have misunderstood what you wanted in #442, i just added an option to every module that uses the wallpaper.

[trueNAHO]

Can you elaborate? I assume the following should be the pattern:
...

I must have misunderstood what you wanted in #442, i just added an option to every module that uses the wallpaper.

Your approach looks good. Sorry for the confusion.

[trueNAHO]

Cross-post:

the stylix.targets.hyprland.hyprpaper.enable option should probably be renamed to stylix.targets.hyprland.wallpaper.enable in #717.

NAHO, "hyprland: avoid trivial stylix.targets.hyprpaper.enable collision"

[Flameopathic]

Cross-post:

the stylix.targets.hyprland.hyprpaper.enable option should probably be renamed to stylix.targets.hyprland.wallpaper.enable in #717.
NAHO, "hyprland: avoid trivial stylix.targets.hyprpaper.enable collision"

implemented

[Flameopathic]

[...] the recent /modules/wayfire/hm.nix module is not utilizing the useWallpaper option.

should be fixed

Optionalizing the wallpaper functionality was surprisingly more involving than expected. Thanks a lot @Flameopathic for your continuous efforts in implementing the most requested Stylix feature.

I appreciate your active reviewing. I made many mistakes, so thank you for your patience!

[trueNAHO]

Optionalizing the wallpaper functionality was surprisingly more involving than expected. Thanks a lot [Flameopathic] for your continuous efforts in implementing the most requested Stylix feature.

I appreciate your active reviewing. I made many mistakes, so thank you for your patience!

Glad I could help :)

[danth]

Seems working based on the testbeds - is there anything specific you would like me to check?

Only things I noticed are:

• The testbed names are quite long - perhaps we could simplify this by giving each configuration an arbitrary name rather than detailing all of the settings?
• We are very close to the limit on the number of matrix jobs for GitHub Actions

However I think these could both be worked on after merging rather than delaying this PR.

[trueNAHO]

Seems working based on the testbeds - is there anything specific you would like me to check?

Nothing specific. An additional review on this major PR would not be bad.

Only things I noticed are:

• The testbed names are quite long - perhaps we could simplify this by giving each configuration an arbitrary name rather than detailing all of the settings?
• We are very close to the limit on the number of matrix jobs for GitHub Actions

However I think these could both be worked on after merging rather than delaying this PR.

Agreed.

[danth]

I've read through the code changes in detail: assume anything not commented on LGTM.

[trueNAHO]

I've read through the code changes in detail: assume anything not commented on LGTM.

Should we merge this PR as-is then?

[Flameopathic]

i'm a little unclear on what @danth meant by

But I think for this PR it should just be left as is.

do you mean that no more changes are necessary and your conversations are moot or that we just shouldn't mess with adding useWallpaper for specific non-wallpaper targets in this PR

[Flameopathic]

either way, i removed useWallpaper options from wallpaper-only modules and instead have the target enable options base on whether stylix.image is set

I'm not sure what's going on with the checks, though.

[danth]

i removed useWallpaper options from wallpaper-only modules and instead have the target enable options base on whether stylix.image is set

This is what I meant :)

I'm not sure what's going on with the checks, though.

This seems to be related to #861:

https://github.com/danth/stylix/actions/runs/13435736838/job/37537466901#step:3:279

Although I'm not sure why this is failing (or why the get-derivations job shows as success despite the error)

[trueNAHO]

Although I'm not sure why this is failing (or why the get-derivations job shows as success despite the error)

This should be resolved in #888.

[trueNAHO]

LGTM.

I would squash merge this PR with the following commit message:

treewide: optionalize stylix.image option

Optionalize the stylix.image option when stylix.base16Scheme is set,
making the following Stylix configurations valid:

    [
      // Now it possible to set 'stylix.image = null', if
      // stylix.base16Scheme is set.
      {
        base16Scheme = /* ... */;
      }

      // This configuration was already possible.
      {
        image = /* ... */;
      }

      // This configuration was already possible.
      {
        base16Scheme = /* ... */;
        image = /* ... */;
      }
    ]

Closes: https://github.com/danth/stylix/issues/200
Closes: https://github.com/danth/stylix/issues/442
Link: https://github.com/danth/stylix/pull/717

Co-authored-by: NAHO <[email protected]>
Reviewed-by: NAHO <[email protected]>
Tested-by: NAHO <[email protected]>
Reviewed-by: Daniel Thwaites <[email protected]>