search

danth / stylix

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

treewide: declare options with `lib.mkDefault` #388

Closed dafitt closed 3 weeks ago

dafitt commented 1 month ago

This is a suggestion to make use of lib.mkDefault to decrease the priority of the stylix options.

I ran into the problem that I wanted to keep the Stylix target and only change a single option. One common example is wayland.windowManager.hyprland.settings.decoration."col.shadow" = "rgba(${config.lib.stylix.colors.base01}E5)"; which currently requires the use of lib.mkForce and looks ugly. So if we lower the priority of the Stylix options, the end user of stylix can more easily customize their theme without having to think of lib.mkForce (and build twice :P) to override the stylix options.

Is it ready for merging, or does it need work?

Yes, i have tested the first three commits, but if this gets accepted i would like to look over the other moules stylix currently has and apply lib.mkDefault where it makes sense.

trueNAHO commented 1 month ago

This is a suggestion to make use of lib.mkDefault to decrease the priority of the stylix options.

I ran into the problem that I wanted to keep the Stylix target and only change a single option. One common example is wayland.windowManager.hyprland.settings.decoration."col.shadow" = "rgba(${config.lib.stylix.colors.base01}E5)"; which currently requires the use of lib.mkForce and looks ugly. So if we lower the priority of the Stylix options, the end user of stylix can more easily customize their theme without having to think of lib.mkForce (and build twice :P) to override the stylix options.

Thanks for the proposal! However, although convenient, this proposal might cause a lot of confusion as end-users may unwillingly override Stylix options. Additionally, end-users options declared with lib.mkDefault would still collide with lib.mkDefault Stylix options.

Instead, Stylix should aim to declare only necessary options. For example, https://github.com/danth/stylix/pull/174 removed unnecessary Stylix declarations.

IMHO, end-users should explicitly override upstream (Stylix) options with lib.mkForce, following the declarative and immutable nature of Nix.

Is it ready for merging, or does it need work?

Yes, i have tested the first three commits, but if this gets accepted i would like to look over the other moules stylix currently has and apply lib.mkDefault where it makes sense.

Since end-users still require lib.mkForce if they already declare their options with lib.mkDefault, Stylix could use an arbitrary priority value below lib.mkDefault, which is currently set to 1000. However, this arguably does not scale well, results in hard to debug conflicts, and is somewhat tedious.

@danth, what do you think?

danth commented 4 weeks ago

Personally, I prefer having the options set as they are now, so I'm aware of any conflicts.

This is most relevant when Stylix needs to set more than one option for a theme to work correctly - with mkDefault you could unknowingly override only part of the options and cause an error.

For example, the current KDE module needs to both install a Look-and-Feel package, and also run a script to activate it at runtime.