search

danth / stylix

System-wide colorscheming and typography for NixOS
https://stylix.danth.me/
MIT License
1.23k stars 157 forks source link

stylix: improve `stylix.mkEnableTarget` behaviour #400

Open trueNAHO opened 6 months ago

trueNAHO commented 6 months ago

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

danth commented 6 months ago

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 commented 6 months ago

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 commented 5 months ago

See this comment for my suggestions.

danth commented 5 months ago

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

trueNAHO commented 5 months ago

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.

danth commented 5 months ago

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 commented 5 months ago

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:

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/danth/stylix/pull/419, this change first uniforms module guards across all Stylix modules. IMHO, https://github.com/danth/stylix/pull/419 should be applied only after this change.

danth commented 5 months ago

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

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.