stylix/testbeds/themes: overhaul testbed themes for broader coverage by trueNAHO · Pull Request #1857 · nix-community/stylix

trueNAHO · 2025-08-19T20:32:22Z

commit 5a75d352c1dc8f4a3fa4b018d59589fbecb0533c
Author: NAHO <90870942+trueNAHO@users.noreply.github.com>
Date:   2025-08-19 22:21:23 +0200

    flake: remove tinted-schemes input in favor of pkgs.base16-scheme

 flake.lock                           | 17 -----------------
 flake.nix                            |  5 -----
 flake/dev/flake.nix                  |  1 -
 stylix/testbed/themes/cursorless.nix |  5 ++---
 stylix/testbed/themes/dark.nix       |  5 ++---
 stylix/testbed/themes/imageless.nix  |  7 ++-----
 stylix/testbed/themes/light.nix      |  5 ++---
 7 files changed, 8 insertions(+), 37 deletions(-)

commit e698150f68a2e443c9c01133ee072abaab3de47b
Author: NAHO <90870942+trueNAHO@users.noreply.github.com>
Date:   2025-08-19 23:36:32 +0200

    stylix/home-manager-integration: do not guard on stylix.enable

    Do not guard on stylix.enable because the original issue from commit
    73b7d0f30039 ("stylix: guard home-manager-integration config (#1494)")
    seems to have resolved itself in the meantime.

    This allows testbeds to declare Stylix target options when Stylix is
    disabled.

    Reverts: 73b7d0f30039 ("stylix: guard home-manager-integration config (#1494)")

 stylix/home-manager-integration.nix | 32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

commit 3c286df0a2b13d3e4c707e51ef5083f7ba124c35
Author: NAHO <90870942+trueNAHO@users.noreply.github.com>
Date:   2025-08-19 22:21:23 +0200

    stylix/testbeds/themes: overhaul testbed themes for broader coverage

 README.md                                          |   2 +-
 doc/src/testbeds.md                                |  20 ++--
 stylix/testbed/images.nix                          |  30 ------
 stylix/testbed/themes/dark.nix                     |  17 ----
 .../testbed/themes/{cursorless.nix => default.nix} |   8 +-
 stylix/testbed/themes/disabled.nix                 |   2 +
 stylix/testbed/themes/empty.nix                    |  38 +++++++
 stylix/testbed/themes/full-dark.nix                | 113 +++++++++++++++++++++
 stylix/testbed/themes/imageless.nix                |  13 ---
 stylix/testbed/themes/light.nix                    |  17 ----
 stylix/testbed/themes/partial-light.nix            |  41 ++++++++
 stylix/testbed/themes/schemeless.nix               |  16 ---
 12 files changed, 207 insertions(+), 110 deletions(-)

Submission Checklist

I certify that I have the right to submit this contribution under the MIT license
Commit messages adhere to Stylix commit conventions
Theming changes adhere to the Stylix style guide
Changes have been tested locally

Changes have been tested in testbeds

To test the Kitty module with the new testbed themes, run:

nix build --no-link \
  github:trueNAHO/stylix/stylix-testbed-themes-overhaul-testbed-themes-for-broader-coverage#testbed:kitty:{default,disabled,empty,full-dark,partial-light}

nix run nixpkgs#parallel -- \
  nix run '{}' ::: \
  github:trueNAHO/stylix/stylix-testbed-themes-overhaul-testbed-themes-for-broader-coverage#testbed:kitty:{default,disabled,empty,full-dark,partial-light}

Each commit in this PR is suitable for backport to the current stable branch

Notify Maintainers

@awwpotato @danth

trueNAHO · 2025-08-19T20:33:56Z

+    icons = {
+      dark = "Adwaita";
+      enable = true;
+      light = "Adwaita";
+      package = pkgs.adwaita-icon-theme;
+    };


Could someone verify whether this works?

trueNAHO · 2025-08-19T20:34:02Z

+    icons = {
+      dark = "Adwaita";
+      enable = true;
+      package = pkgs.adwaita-icon-theme;
+    };


Could someone verify whether this works?

0xda157 · 2025-08-20T06:24:01Z

    };
  };

-  config = lib.mkIf config.stylix.enable (


why are we reverting 73b7d0f? this pretty clear goes against the spirit of 7682713

why are we reverting 73b7d0f? this pretty clear goes against the spirit of 7682713

The new disabled testcase is failing. The following commit fixes that:

stylix/home-manager-integration: unconditionally enable Unconditionally enable the Home Manager integration to avoid error: The option `virtualisation.vmVariant.home-manager.users.guest.stylix' does not exist. Definition values: errors inside 'stylix.enable = false;' testbeds containing the following declaration: home-manager.sharedModules = lib.singleton { stylix.targets.${target}.${option} = value; }; Ideally, the Home Manager integration merely provides the options, and the actual modules should do the guarding.

-- fcde0a7 ("stylix/home-manager-integration: unconditionally enable")

When I implemented this fix and quickly glanced over the discussion from commit 73b7d0f, it seems the new test cases cover all problems that were originally mentioned.

I will test each of these cases locally in my standalone Home Manager setup to see if this also works in end-user setups. This is the main reason this PR is currently in draft mode. Also, the commit message from fcde0a7 ("stylix/home-manager-integration: unconditionally enable") is currently just for debugging purposes, and will be overhauled if its diff is good.

I will test each of these cases locally in my standalone Home Manager setup to see if this also works in end-user setups.

AFAICT, this does not affect Stylix integration with standalone Home Manager. The NixOS and Home Manager integration is covered by the new test cases, while the standalone NixOS integration seems untested.

This is the main reason this PR is currently in draft mode.

I am optimistically marking this PR as ready for review again.

quickly glanced over the discussion from commit 73b7d0f, it seems the new test cases cover all problems that were originally mentioned.

It seems whatever was the issue in that discussion, seems to have resolved itself in the meantime, as indicated by the new theme coverage:

Theme image base16Scheme

disabled false false

default false true

partial-light true false

full-dark true true

Weighing in here as a random stylix user: in general enable options don't change what you're allowed to write in the config, they only change whether that configuration applies to the resulting system.
Here, guarding the import behind an enable goes against that since it makes the HM options existence conditional on enable, which forces users to hide all their HM config behind stylix.enable. That shouldn't be needed and is bad/unconventional UX. And mkIf is not even enough since the options don't exist, you need optionalAttrs or similar, to completely hide the config values from the module system.
So I also believe reverting 73b7d0f is the right choice, and I arrived to it independently in #2089.

I'd ideally like to see this unblocked and cherry-picked before the rest of the MR cause it's the only custom change I have for Stylix, and I'd rather just use upstream cause it's easier to update compared to rebasing my fork.

Also I'd recommend adding a comment in the code to explain this, cause it might seem surprising at first compared to other modules.

in general enable options don't change what you're allowed to write in the config, they only change whether that configuration applies to the resulting system.

If you mean whether options should always be available, this is not generally the case and is AFAIK an undocumented convention: #2078.

I'd ideally like to see this unblocked and cherry-picked before the rest of the MR

As previously mentioned, the final [PATCH 3/3] stylix/testbeds/themes: overhaul testbed themes for broader coverage patch is exactly what increases correctness confidence:

quickly glanced over the discussion from commit 73b7d0f, it seems the new test cases cover all problems that were originally mentioned.

It seems whatever was the issue in that discussion, seems to have resolved itself in the meantime, as indicated by the new theme coverage:

Theme image base16Scheme

disabled false false

default false true

partial-light true false

full-dark true true

Without it, we merely assume this works. I am in favor of merging this only with the new test coverage.

Also I'd recommend adding a comment in the code to explain this, cause it might seem surprising at first compared to other modules.

I assume you are comparing the /stylix/home-manager-integration.nix core code to /modules/<MODULE>/{hm,nixos}.nix modules. IMHO, the core code should always just work, meaning the previous workaround commit 73b7d0f ("stylix: guard home-manager-integration config (#1494)") was the confusing exception and the new behaviour is the expected one:

Ideally, the Home Manager integration merely provides the options, and the actual modules should do the guarding.

-- fcde0a7 ("stylix/home-manager-integration: unconditionally enable")

I think adding such a comment would be actively confusing because it re-states the norm.

If you mean whether options should always be available, this is not generally the case and is AFAIK an undocumented convention: #2078.

Yes it's an undocumented convention, but it holds true across the entire Nix ecosystem AFAIK.

I think adding such a comment would be actively confusing because it re-states the norm.

It's about communicating with others since this has apparently caused enough confusion with multiple iterations.
I'm happy with whatever you prefer.

Anyways I agree with you in general, I just wanted to give additional input on the original comment, confirming the revert is the right move (IMO ofc) :)

0xda157 · 2025-08-20T06:26:21Z

+      dark = "Adwaita";
+      enable = true;
+      light = "Adwaita";


why are we using the light icons for dark?

Suggested change

dark = "Adwaita";

enable = true;

light = "Adwaita";

dark = "Adwaita Dark";

enable = true;

light = "Adwaita";

why are we using the light icons for dark?

As implied by the theme name and its file header, full-dark declares all available Stylix options to reasonable non-default values, while partial-light intentionally does not declare all available Stylix options in order to test modularity. The dark and light polarity tests are merged with the "full" and "partial" test cases to reduce the total number of themes. In other words, full-dark declares stylix.icons.light despite being stylix.polarity = "dark"; to test whether this redundant declared value causes any problems.

Suggested change

dark = "Adwaita";

enable = true;

light = "Adwaita";

dark = "Adwaita Dark";

enable = true;

light = "Adwaita";

Does "Adwaita Dark" work? I see no such reference:

$ nix build --no-link --print-out-paths nixpkgs#adwaita-icon-theme | xargs rg --ignore-case 'Adwaita|Dark|Light' /nix/store/phcncb0xzj0y2bcx7zd171b32cfqblz2-adwaita-icon-theme-47.0/share/icons/Adwaita/index.theme 2:Name=Adwaita /nix/store/phcncb0xzj0y2bcx7zd171b32cfqblz2-adwaita-icon-theme-47.0/share/pkgconfig/adwaita-icon-theme.pc 2:Name: adwaita-icon-theme

Do not guard on stylix.enable because the original issue from commit 73b7d0f ("stylix: guard home-manager-integration config (nix-community#1494)") seems to have resolved itself in the meantime. This allows testbeds to declare Stylix target options when Stylix is disabled. Reverts: 73b7d0f ("stylix: guard home-manager-integration config (nix-community#1494)")

trueNAHO requested review from 0xda157 and danth August 19, 2025 20:32

stylix-automation bot added topic: documentation Documentation additions or improvements topic: dependencies Dependency updates topic: testbed Testbed changes topic: flake /flake.nix, /flake.lock, and /flake/ subsystems topic: stylix /stylix/ subsystem labels Aug 19, 2025

trueNAHO commented Aug 19, 2025

View reviewed changes

trueNAHO added the backport: release-25.05 label Aug 19, 2025

trueNAHO marked this pull request as draft August 19, 2025 21:39

trueNAHO commented Aug 19, 2025

View reviewed changes

Comment thread stylix/testbed/themes/partial-light.nix Outdated

0xda157 reviewed Aug 20, 2025

View reviewed changes

trueNAHO added 3 commits August 20, 2025 23:30

flake: remove tinted-schemes input in favor of pkgs.base16-scheme

5a75d35

stylix/testbeds/themes: overhaul testbed themes for broader coverage

3c286df

trueNAHO force-pushed the stylix-testbed-themes-overhaul-testbed-themes-for-broader-coverage branch from fcde0a7 to 3c286df Compare August 21, 2025 00:10

trueNAHO marked this pull request as ready for review August 21, 2025 00:13

trueNAHO mentioned this pull request Aug 26, 2025

stylix: refactor stylix.icons to use { name, package } type #1855

Open

1 task

trueNAHO requested a review from 0xda157 August 26, 2025 18:14

stylix-automation bot added the status: merge conflict Merge conflict label Sep 5, 2025

This was referenced Nov 21, 2025

ci: add buildbot support #1985

Merged

stylix/testbeds: don't unnecessarily use an IFD #2019

Closed

ThinkChaos mentioned this pull request Dec 15, 2025

stylix/home-manager-integration: enable even when stylix.enable = false #2089

Closed

6 tasks

0xda157 removed the backport: release-25.05 label Dec 23, 2025

trueNAHO mentioned this pull request Dec 23, 2025

opencode: brighten default text colors #2100

Merged

6 tasks

trueNAHO mentioned this pull request Jan 6, 2026

firefox: add reader mode support and improve firefoxGnomeTheme #2126

Merged

6 tasks

Theme	`image`	`base16Scheme`
`disabled`	`false`	`false`
`default`	`false`	`true`
`partial-light`	`true`	`false`
`full-dark`	`true`	`true`

Uh oh!

Conversation

trueNAHO commented Aug 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Submission Checklist

Notify Maintainers

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trueNAHO Aug 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trueNAHO Aug 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ThinkChaos Dec 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

trueNAHO commented Aug 19, 2025 •

edited

Loading

trueNAHO Aug 20, 2025 •

edited

Loading

trueNAHO Aug 20, 2025 •

edited

Loading

ThinkChaos Dec 17, 2025 •

edited

Loading