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

stylix: remove cfg.enable from mkEnableTarget default #419

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

danth
Copy link
Owner

@danth danth commented Jun 10, 2024

The individual modules are already gated by stylix.enable, so there is no need for it to also affect the value of stylix.targets.«name».enable.

This should not produce any noticeable change in behavior.

Note: I have not tested this yet.

The individual modules are already gated by `stylix.enable`, so
there is no need for it to also affect the value of
`stylix.targets.«name».enable`.

This should not produce any noticeable change in behavior.
Copy link
Collaborator

@trueNAHO trueNAHO left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not produce any noticeable change in behavior.

Note: I have not tested this yet.

In my standalone Home Manager setup, I receive the following error:

error: A definition for option `stylix.image' is not of type `path or package convertible to it'. Definition values:
- In `/nix/store/d33h4m5arzybvnvl7ibmis8svpahsadm-source/modules/stylix': null

Note that this could be a problem with my setup.

Merging #407 would help.

@danth
Copy link
Owner Author

danth commented Jun 11, 2024

That's unusual. Could be caused by some of the recent changes, but I suspect it's not this one. Can you share your config?

@NovaViper
Copy link

Hey I would like to comment that removing the target toggles would cause regressions for edge-case uses such as those in my own NixOS configuration:

  • For Tmux, I use the dracula plugin because I like that it has powerline support and has preconfigured status line that I can control what status is displayed for Tmux. If I had the stylix tmux version enabled, it actually conflicts with the Dracula plugin (it completely overrides it actually!).
  • For KDE (Plasma 6 specifically), I still have the KDE target on but I actually manually configure KDE with plasma-manager, since Stylix hasn't been updated to be more compatible with Plasma 6 yet (i.e. the Look and Feel module is still broken).

Having these toggles make it easier for me to fine tune what exactly I want to use; especially since there's currently no way to change the smaller details of how stylix applies colors (basically what PR #270 proposes with having 'swatches' for different applications).

@danth
Copy link
Owner Author

danth commented Jun 11, 2024

I think you've misunderstood the change here - the individual target enable options won't be removed.

Instead, it's just removing the behaviour where the default for each individual enable option changes to false when stylix.enable is false. We already say that "stylix will do nothing unless stylix.enable is true", and that will still be the case, so the default swapping just adds extra confusion for no real benefit.

@NovaViper
Copy link

I think you've misunderstood the change here - the individual target enable options won't be removed.

Instead, it's just removing the behaviour where the default for each individual enable option changes to false when stylix.enable is false. We already say that "stylix will do nothing unless stylix.enable is true", and that will still be the case, so the default swapping just adds extra confusion for no real benefit.

Ah, you're right! I did completely misinterpret the change. Thank you for the clarification! 😅

@trueNAHO
Copy link
Collaborator

trueNAHO commented Jun 11, 2024

Could be caused by some of the recent changes, but I suspect it's not this one.

Can you share your config?

My standalone Home Manager setup 3 is fairly complex.

The relevant minimal working example is:

nix build .#checks.x86_64-linux.standalone-homeManager-programs-mpv-enable-false

The minimal working example (MWE) is generated based on my MPV module 1, which imports my Stylix module 2. In this MWE, all .enable options are set to false.

Upon applying the following patch, the previous MWE fails:

From e3b838a88fe463828de19c7a22eb1a7823a0d1ee Mon Sep 17 00:00:00 2001
From: NAHO <[email protected]>
Date: Tue, 11 Jun 2024 01:27:43 +0200
Subject: [PATCH] chore!: 2024-06-11 01:27:43 +0200

---
 flake.lock | 7 ++++---
 flake.nix  | 2 +-
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/flake.lock b/flake.lock
index ad971932..0ef77b0e 100644
--- a/flake.lock
+++ b/flake.lock
@@ -548,15 +548,16 @@
         ]
       },
       "locked": {
-        "lastModified": 1718013167,
-        "narHash": "sha256-L+IzjhovTTqOzqLXjrfGFsDPVuCLWZTah+rt7wRkGJ8=",
+        "lastModified": 1718047991,
+        "narHash": "sha256-S/6sxMs6aWlHqveNRTyQq4sQEMICtmmem61P0Vq2Lfo=",
         "owner": "danth",
         "repo": "stylix",
-        "rev": "7682713f6af1d32a33f8c4e3d3d141af5ad1761a",
+        "rev": "11166aa5a2b92437f03cf0b57fd51ef490dee635",
         "type": "github"
       },
       "original": {
         "owner": "danth",
+        "ref": "remove-default-cfg-enable",
         "repo": "stylix",
         "type": "github"
       }
diff --git a/flake.nix b/flake.nix
index c83e2c25..4a8f1519 100644
--- a/flake.nix
+++ b/flake.nix
@@ -89,7 +89,7 @@
         nixpkgs.follows = "nixpkgs";
       };

-      url = "github:danth/stylix";
+      url = "github:danth/stylix/remove-default-cfg-enable";
     };

     # This input ensures consistent versioning across inputs.
--
2.44.0

Note that the following complementary MWE, setting all .enable options to true, still succeeds before and after the previously mentioned patch:

nix build .#checks.x86_64-linux.standalone-homeManager-programs-mpv-enable-true

Unless you see what the problem is, I can try to build a simple static MWE that does not rely on my complex generation process.

Also, the GitHub Actions currently fail due to timeout. Everything works as expected.

That's unusual.

I suspect the problem stems from the fact that something is not properly guarded. Either on my end or in Stylix.

@trueNAHO
Copy link
Collaborator

Unless you see what the problem is, I can try to build a simple static MWE that does not rely on my complex generation process.

Potentially related: #421

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.

4 participants