stylix icon indicating copy to clipboard operation
stylix copied to clipboard
verified publisher icon danth

stylix: improve `stylix.mkEnableTarget` behaviour

Open trueNAHO opened this issue 1 year ago • 8 comments

About

This is a tracking issue for enabling all Stylix targets by default with stylix.autoEnable, and simplifying the stylix.mkEnableTarget documentation. This behaviour is desired based on the following reasoning:

https://github.com/danth/stylix/blob/00a11ba2f0b52f761c0bc77daebb00cb4d44ba09/stylix/target.nix#L17-L24

Currently, some targets are enabled based on unscalable checks. For example, feh manually tries a non-exhaustive list of window managers, although the window manager dependency seems wrong in this case:

https://github.com/danth/stylix/blob/00a11ba2f0b52f761c0bc77daebb00cb4d44ba09/modules/feh/hm.nix#L4-L11

Due to some targets not being enabled by default with stylix.mkEnableTarget, the documentation incorrectly generates Default: false in some cases with https://github.com/danth/stylix/pull/399. Resolving this involves manually extending docs/settings.nix or enabling all targets by default with stylix.mkEnableTarget.

Steps

  • [ ] Enable all targets by default with stylix.autoEnable
  • [x] https://github.com/danth/stylix/pull/399

trueNAHO avatar May 27 '24 12:05 trueNAHO

The feh module checks for certain window managers since these are known to not come with their own wallpaper program, and are based on X rather than Wayland, so feh is a suitable all-round solution to install for these cases.

I'm not completely sure what the goal of this issue is. Are you suggesting the argument to mkEnableTarget should always be a constant true or false?

danth avatar May 28 '24 11:05 danth

The feh module checks for certain window managers since these are known to not come with their own wallpaper program, and are based on X rather than Wayland, so feh is a suitable all-round solution to install for these cases.

Makes sense. However, with XWayland being a thing, it would make sense to enable it also on Wayland. For example, I use feh on Hyprland, which runs on Wayland.

I'm not completely sure what the goal of this issue is.

Essentially, the goal is to resolve the following issue:

Due to some targets not being enabled by default with stylix.mkEnableTarget, the documentation incorrectly generates Default: false in some cases with #399.

Are you suggesting the argument to mkEnableTarget should always be a constant true or false?

Actually, I don't have a concrete suggestion yet. I only noticed the inconsistency, but have not investigated it any further yet.

trueNAHO avatar May 28 '24 12:05 trueNAHO

See this comment for my suggestions.

danth avatar May 31 '24 18:05 danth

What's the status of this now that #244 has been merged?

danth avatar Jun 12 '24 23:06 danth

I'm not completely sure what the goal of this issue is.

Essentially, the goal is to resolve the following issue:

Due to some targets not being enabled by default with stylix.mkEnableTarget, the documentation incorrectly generates Default: false in some cases with #399.

What's the status of this now that #244 has been merged?

For example, #244 changes the stylix.targets.feh.enable documentation from

Default: false

to

Default: same as stylix.autoEnable

which resolves the initial main issue.

However, what should we do about the following related issue:

Currently, some targets are enabled based on unscalable checks. For example, feh manually tries a non-exhaustive list of window managers, although the window manager dependency seems wrong in this case:

https://github.com/danth/stylix/blob/00a11ba2f0b52f761c0bc77daebb00cb4d44ba09/modules/feh/hm.nix#L4-L11

The feh module checks for certain window managers since these are known to not come with their own wallpaper program, and are based on X rather than Wayland, so feh is a suitable all-round solution to install for these cases.

Makes sense. However, with XWayland being a thing, it would make sense to enable it also on Wayland. For example, I use feh on Hyprland, which runs on Wayland.

trueNAHO avatar Jun 14 '24 14:06 trueNAHO

Perhaps we should split the feh module into individual targets for each of the window managers involved?

Similar to how the GNOME module uses GNOME's built in wallpaper setting, Hyprland uses/will use hyprpaper, etc

danth avatar Jun 14 '24 22:06 danth

Perhaps we should split the feh module into individual targets for each of the window managers involved?

Similar to how the GNOME module uses GNOME's built in wallpaper setting, Hyprland uses/will use hyprpaper, etc

Yes, inverting the dependency chain sounds like a good idea, as it leverages Locality of Behaviour (LoB).

Additionally, it simplifies theming feh itself in the future, as its Window Manager functionality is unnecessary for that.

Consequently, I suggest adapting all config guards not adhering to the lib.mkIf (config.stylix.enable && config.stylix.targets.<TARGET>.enable) pattern.

Here is a complete list of all such guards on a7fbda1fd965cc22e62463896a4af0342cb00e6a:

  • lib.mkIf config.stylix.enable

    • https://github.com/nix-community/stylix/blob/a7fbda1fd965cc22e62463896a4af0342cb00e6a/modules/foot/hm.nix#L14
    • https://github.com/nix-community/stylix/blob/a7fbda1fd965cc22e62463896a4af0342cb00e6a/modules/gtk/hm.nix#L34
    • https://github.com/nix-community/stylix/blob/a7fbda1fd965cc22e62463896a4af0342cb00e6a/modules/kitty/hm.nix#L23
    • https://github.com/nix-community/stylix/blob/a7fbda1fd965cc22e62463896a4af0342cb00e6a/modules/plymouth/nixos.nix#L84

  • lib.mkIf config.stylix.targets.<TARGET>.enable

    • https://github.com/nix-community/stylix/blob/a7fbda1fd965cc22e62463896a4af0342cb00e6a/modules/k9s/hm.nix#L9

  • External Module Dependency: lib.mkIf (config.stylix.enable && config.stylix.targets.<TARGET>.enable && ...)

    • https://github.com/nix-community/stylix/blob/a7fbda1fd965cc22e62463896a4af0342cb00e6a/modules/feh/hm.nix#L9-L21
    • https://github.com/nix-community/stylix/blob/a7fbda1fd965cc22e62463896a4af0342cb00e6a/modules/feh/nixos.nix#L9-L18

  • lib.mkIf (config.stylix.enable && config.stylix.targets.<TARGET>.enable && config.programs.<TARGET>.enable)

    • https://github.com/nix-community/stylix/blob/a7fbda1fd965cc22e62463896a4af0342cb00e6a/modules/btop/hm.nix#L8
    • https://github.com/nix-community/stylix/blob/a7fbda1fd965cc22e62463896a4af0342cb00e6a/modules/gnome/nixos.nix#L10-L14
    • https://github.com/nix-community/stylix/blob/a7fbda1fd965cc22e62463896a4af0342cb00e6a/modules/helix/hm.nix#L19
    • https://github.com/nix-community/stylix/blob/a7fbda1fd965cc22e62463896a4af0342cb00e6a/modules/wezterm/hm.nix#L8

  • lib.mkIf (config.stylix.enable && config.stylix.targets.<TARGET>.enable && pkgs.stdenv.hostPlatform.isLinux)

    • https://github.com/nix-community/stylix/blob/a7fbda1fd965cc22e62463896a4af0342cb00e6a/modules/kde/hm.nix#L227
    • https://github.com/nix-community/stylix/blob/a7fbda1fd965cc22e62463896a4af0342cb00e6a/modules/swaylock/hm.nix#L26

  • External Input Support: lib.optionalAttrs (options.services ? <TARGET>) (lib.mkIf (config.stylix.enable && config.stylix.targets.<TARGET>.enable)

    • https://github.com/nix-community/stylix/blob/a7fbda1fd965cc22e62463896a4af0342cb00e6a/modules/avizo/hm.nix#L13
    • https://github.com/nix-community/stylix/blob/a7fbda1fd965cc22e62463896a4af0342cb00e6a/modules/mako/hm.nix#L12
    • https://github.com/nix-community/stylix/blob/a7fbda1fd965cc22e62463896a4af0342cb00e6a/modules/nixvim/nixvim.nix#L16

The External Input Support pattern is required to support external modules and is only included for completeness reasons.

Is it possible to extend the lib.mkIf (config.stylix.enable && config.stylix.targets.<TARGET>.enable && pkgs.stdenv.hostPlatform.isLinux) pattern to support non-Linux systems?

The remaining listed patterns should be adapted to the lib.mkIf (config.stylix.enable && config.stylix.targets.<TARGET>.enable) pattern to avoid inconsistencies with modules being unable to be toggled. Unlike https://github.com/nix-community/stylix/pull/419, this change first uniforms module guards across all Stylix modules. IMHO, https://github.com/nix-community/stylix/pull/419 should be applied only after this change.

trueNAHO avatar Jun 16 '24 13:06 trueNAHO

The first two categories you listed look like they were just missed when I used sed to apply the change across all modules:

  • k9s doesn't match the pattern [a-z\-]+
  • The others use a cfg alias hence don't match

Is it possible to extend the lib.mkIf (config.stylix.enable && config.stylix.targets.<TARGET>.enable && pkgs.stdenv.hostPlatform.isLinux) pattern to support non-Linux systems?

For KDE, this exists since we check whether KDE is installed at runtime rather than using its enable option (KDE would be enabled in a NixOS config, but we're using a Home Manager module). When running Home Manager on MacOS, it's impossible for KDE to be installed hence we skip the check. I believe there may also be an error involved due to some of the dependencies not being available on MacOS.

For Swaylock, it may be possible to remove the guard since Home Manager should only use the settings when Swaylock is enabled. I haven't tested this.

danth avatar Jun 16 '24 14:06 danth