stylix: improve `stylix.mkEnableTarget` behaviour #400

trueNAHO · 2024-05-27T12:27:17Z

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:

stylix/stylix/target.nix

Lines 17 to 24 in 00a11ba

    
           # If the module only touches options under its target (programs.target.*) 
        
           # then this can simply be `true`, as those options are already gated by the 
        
           # upstream enable option. 
        
           # 
        
           # Otherwise, use `config` to check whether the target is enabled. 
        
           # 
        
           # If some manual setup is required, or the module leads to the target 
        
           # being installed if it wasn't already, set this to `false`.

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:

stylix/modules/feh/hm.nix

Lines 4 to 11 in 00a11ba

    
           options.stylix.targets.feh.enable = 
        
             config.lib.stylix.mkEnableTarget 
        
             "the desktop background using Feh" 
        
             (with config.xsession.windowManager; bspwm.enable 
        
                                               || herbstluftwm.enable 
        
                                               || i3.enable 
        
                                               || spectrwm.enable 
        
                                               || xmonad.enable);

Due to some targets not being enabled by default with stylix.mkEnableTarget, the documentation incorrectly generates Default: false in some cases with #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
stylix: simplify mkEnableTarget documentation #399

The text was updated successfully, but these errors were encountered:

danth · 2024-05-28T11:30:13Z

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?

trueNAHO · 2024-05-28T12:10:25Z

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.

danth · 2024-05-31T18:20:11Z

See this comment for my suggestions.

Simplify the 'stylix.mkEnableTarget' documentation. The following issue is to be resolved in danth#400: > Due to some targets not being enabled by default with > 'stylix.mkEnableTarget', the documentation incorrectly generates > 'Default: false' in some cases

Simplify the 'stylix.mkEnableTarget' documentation. The following issue is to be resolved in #400: > Due to some targets not being enabled by default with > 'stylix.mkEnableTarget', the documentation incorrectly generates > 'Default: false' in some cases

danth · 2024-06-12T23:32:48Z

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

trueNAHO · 2024-06-14T14:47:16Z

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:

stylix/modules/feh/hm.nix

Lines 4 to 11 in 00a11ba

options.stylix.targets.feh.enable =

config.lib.stylix.mkEnableTarget

"the desktop background using Feh"

(with config.xsession.windowManager; bspwm.enable

|| herbstluftwm.enable

|| i3.enable

|| spectrwm.enable

|| xmonad.enable);

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.

danth · 2024-06-14T22:05:19Z

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

trueNAHO · 2024-06-16T13:19:02Z

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 a7fbda1:

lib.mkIf config.stylix.enable
- stylix/modules/foot/hm.nix
  
  Line 14 in a7fbda1
  
  config.programs.foot.settings = lib.mkIf cfg.enable {
- stylix/modules/gtk/hm.nix
  
  Line 34 in a7fbda1
  
  config = lib.mkIf cfg.enable {
- stylix/modules/kitty/hm.nix
  
  Line 23 in a7fbda1
  
  config = lib.mkIf cfg.enable {
- stylix/modules/plymouth/nixos.nix
  
  Line 84 in a7fbda1
  
  config.boot.plymouth = lib.mkIf cfg.enable {
lib.mkIf config.stylix.targets.<TARGET>.enable
- stylix/modules/k9s/hm.nix
  
  Line 9 in a7fbda1
  
  config = lib.mkIf config.stylix.targets.k9s.enable {

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

stylix/modules/feh/hm.nix

Lines 9 to 21 in a7fbda1

    
           config.xsession.initExtra = 
        
             lib.mkIf ( 
        
               config.stylix.enable 
        
               && config.stylix.targets.feh.enable 
        
               && ( 
        
                 with config.xsession.windowManager; 
        
                 bspwm.enable 
        
                 || herbstluftwm.enable 
        
                 || i3.enable 
        
                 || spectrwm.enable 
        
                 || xmonad.enable 
        
               ) 
        
             )

stylix/modules/feh/nixos.nix

Lines 9 to 18 in a7fbda1

    
           config.services.xserver.displayManager.sessionCommands = 
        
             lib.mkIf ( 
        
               config.stylix.enable 
        
               && config.stylix.targets.feh.enable 
        
               && ( 
        
                 with config.services.xserver.windowManager; 
        
                 xmonad.enable 
        
                 || i3.enable 
        
               ) 
        
             )

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

stylix/modules/btop/hm.nix

Line 8 in a7fbda1

config = lib.mkIf (config.stylix.enable && config.stylix.targets.btop.enable && config.programs.btop.enable) {

stylix/modules/gnome/nixos.nix

Lines 10 to 14 in a7fbda1

    
           config = lib.mkIf ( 
        
             config.stylix.enable 
        
             && config.stylix.targets.gnome.enable 
        
             && config.services.xserver.desktopManager.gnome.enable 
        
           ) {

stylix/modules/helix/hm.nix

Line 19 in a7fbda1

config = lib.mkIf (config.stylix.enable && config.stylix.targets.helix.enable && config.programs.helix.enable) {
stylix/modules/wezterm/hm.nix

Line 8 in a7fbda1

config = lib.mkIf (config.stylix.enable && config.stylix.targets.wezterm.enable && config.programs.wezterm.enable) {

lib.mkIf (config.stylix.enable && config.stylix.targets.<TARGET>.enable && pkgs.stdenv.hostPlatform.isLinux)
- stylix/modules/kde/hm.nix
  
  Line 227 in a7fbda1
  
  config = lib.mkIf (config.stylix.enable && config.stylix.targets.kde.enable && pkgs.stdenv.hostPlatform.isLinux) {
- stylix/modules/swaylock/hm.nix
  
  Line 26 in a7fbda1
  
  config = lib.mkIf (config.stylix.enable && config.stylix.targets.swaylock.enable && pkgs.stdenv.hostPlatform.isLinux) {
External Input Support: lib.optionalAttrs (options.services ? <TARGET>) (lib.mkIf (config.stylix.enable && config.stylix.targets.<TARGET>.enable)
- stylix/modules/avizo/hm.nix
  
  Line 13 in a7fbda1
  
  config = lib.optionalAttrs (options.services ? avizo) (lib.mkIf (config.stylix.enable && config.stylix.targets.avizo.enable) {
- stylix/modules/mako/hm.nix
  
  Line 12 in a7fbda1
  
  config = lib.optionalAttrs (options.services ? mako) (lib.mkIf (config.stylix.enable && config.stylix.targets.mako.enable) {
- stylix/modules/nixvim/nixvim.nix
  
  Line 16 in a7fbda1
  
  config = lib.mkIf (config.stylix.enable && config.stylix.targets.nixvim.enable && (config.programs ? nixvim)) (

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 #419, this change first uniforms module guards across all Stylix modules. IMHO, #419 should be applied only after this change.

danth · 2024-06-16T14:13:27Z

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.

trueNAHO added technical debt Things which should be cleaned up or improved C-tracking-issue Category: A tracking issue labels May 27, 2024

trueNAHO mentioned this issue May 27, 2024

stylix: simplify mkEnableTarget documentation #399

Merged

trueNAHO mentioned this issue Jun 23, 2024

waybar support broken(?) #445

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stylix: improve `stylix.mkEnableTarget` behaviour #400

stylix: improve `stylix.mkEnableTarget` behaviour #400

trueNAHO commented May 27, 2024 •

edited by danth

Loading

danth commented May 28, 2024

trueNAHO commented May 28, 2024

danth commented May 31, 2024

danth commented Jun 12, 2024

trueNAHO commented Jun 14, 2024

danth commented Jun 14, 2024

trueNAHO commented Jun 16, 2024 •

edited

Loading

danth commented Jun 16, 2024 •

edited

Loading

stylix: improve stylix.mkEnableTarget behaviour #400

stylix: improve stylix.mkEnableTarget behaviour #400

Comments

trueNAHO commented May 27, 2024 • edited by danth Loading

About

Steps

danth commented May 28, 2024

trueNAHO commented May 28, 2024

danth commented May 31, 2024

danth commented Jun 12, 2024

trueNAHO commented Jun 14, 2024

danth commented Jun 14, 2024

trueNAHO commented Jun 16, 2024 • edited Loading

danth commented Jun 16, 2024 • edited Loading

stylix: improve `stylix.mkEnableTarget` behaviour #400

stylix: improve `stylix.mkEnableTarget` behaviour #400

trueNAHO commented May 27, 2024 •

edited by danth

Loading

trueNAHO commented Jun 16, 2024 •

edited

Loading

danth commented Jun 16, 2024 •

edited

Loading