stylix/testbeds/themes: overhaul testbed themes for broader coverage #1857
Conversation
stylix-automation
bot
added
topic: documentation
| icons = { | ||
| dark = "Adwaita"; | ||
| enable = true; | ||
| light = "Adwaita"; | ||
| package = pkgs.adwaita-icon-theme; | ||
| }; |
There was a problem hiding this comment.
Could someone verify whether this works?
| icons = { | ||
| dark = "Adwaita"; | ||
| enable = true; | ||
| package = pkgs.adwaita-icon-theme; | ||
| }; |
There was a problem hiding this comment.
Could someone verify whether this works?
trueNAHO
added
the
backport: release-25.05
| }; | ||
| }; | ||
|
|
||
| config = lib.mkIf config.stylix.enable ( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
| dark = "Adwaita"; | ||
| enable = true; | ||
| light = "Adwaita"; |
There was a problem hiding this comment.
why are we using the light icons for dark?
| dark = "Adwaita"; | |
| enable = true; | |
| light = "Adwaita"; | |
| dark = "Adwaita Dark"; | |
| enable = true; | |
| light = "Adwaita"; |
There was a problem hiding this comment.
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
force-pushed
the
stylix-testbed-themes-overhaul-testbed-themes-for-broader-coverage
branch
from
fcde0a7 to
3c286df
Compare
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