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: declare options with lib.mkDefault #388

Closed
wants to merge 3 commits into from
Closed

treewide: declare options with lib.mkDefault #388

wants to merge 3 commits into from

Conversation

dafitt
Copy link
Contributor

@dafitt dafitt commented May 23, 2024

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

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, #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?

@trueNAHO trueNAHO changed the title Make use of lib.mkDefault treewide: declare options with lib.mkDefault May 23, 2024
@danth
Copy link
Owner

danth commented Jun 4, 2024

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.

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.

3 participants