Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

treewide: make stylix.image optional #717

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Flameopathic
Copy link
Contributor

@Flameopathic Flameopathic commented Jan 1, 2025

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
  • 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
Copy link
Collaborator

trueNAHO commented Jan 1, 2025

  • 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
Copy link
Contributor Author

  • 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?

@Flameopathic Flameopathic force-pushed the optional-image branch 2 times, most recently from aa17ed3 to f7e7cbc Compare January 2, 2025 17:01
@trueNAHO
Copy link
Collaborator

trueNAHO commented Jan 2, 2025

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.

stylix/palette.nix Outdated Show resolved Hide resolved
@Flameopathic
Copy link
Contributor Author

Flameopathic commented Jan 2, 2025

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 Flameopathic force-pushed the optional-image branch 2 times, most recently from 50cce00 to ce128e9 Compare January 3, 2025 00:12
@Flameopathic
Copy link
Contributor Author

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
Copy link
Contributor

lordkekz commented Jan 3, 2025

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
Copy link
Contributor Author

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
Copy link
Collaborator

trueNAHO commented Jan 3, 2025

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
Copy link
Contributor Author

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.

modules/grub/nixos.nix Outdated Show resolved Hide resolved
stylix/testbed.nix Outdated Show resolved Hide resolved
@trueNAHO
Copy link
Collaborator

trueNAHO commented Jan 4, 2025

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.

stylix/target.nix Outdated Show resolved Hide resolved
@trueNAHO
Copy link
Collaborator

trueNAHO commented Jan 4, 2025

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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

treewide: optionalize wallpaper functionality stylix: wallpaper should be optional
4 participants