-
-
Notifications
You must be signed in to change notification settings - Fork 323
stylix/testbeds/themes: overhaul testbed themes for broader coverage #1857
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
This file was deleted.
This file was deleted.
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| # Test the default Stylix configuration. | ||
| { pkgs, ... }: | ||
| { | ||
| stylix = { | ||
| base16Scheme = "${pkgs.base16-schemes}/share/themes/catppuccin-macchiato.yaml"; | ||
| enable = true; | ||
| }; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| # Test Stylix being disabled. | ||
| { stylix.enable = false; } | ||
|
trueNAHO marked this conversation as resolved.
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| # Test Stylix being enabled with all its features disabled. | ||
| { | ||
| config, | ||
| lib, | ||
| pkgs, | ||
| ... | ||
| }: | ||
| { | ||
| stylix = { | ||
| enable = true; | ||
| icons.enable = false; | ||
| overlays.enable = false; | ||
|
|
||
| # TODO: Disable the color functionality by replacing the mandatory | ||
| # stylix.base16Scheme declaration with 'stylix.colors.enable = false;' once | ||
| # this option is implemented. | ||
| base16Scheme = "${pkgs.base16-schemes}/share/themes/mellow-purple.yaml"; | ||
|
|
||
| # TODO: Properly disable the cursor functionality by replacing the | ||
| # stylix.cursor declaration with 'stylix.cursor.enable = false;' once this | ||
| # option is implemented. | ||
| cursor = null; | ||
|
|
||
| # TODO: Disable the font functionality with 'stylix.fonts.enable = false;' | ||
| # once this option is implemented. | ||
|
|
||
| # TODO: Properly disable the image functionality by replacing the | ||
| # stylix.image declaration with 'stylix.image.enable = false;' once this | ||
| # option is implemented. The stylix.image.enable namespace is subject to | ||
| # change. | ||
| image = null; | ||
|
|
||
| # TODO: Properly disable the opacity functionality by replacing the | ||
| # stylix.opacity declaration with 'stylix.opacity.enable = false;' once this | ||
| # option is implemented. | ||
| opacity = lib.genAttrs (builtins.attrNames config.stylix.opacity) (_: 1.0); | ||
| }; | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,113 @@ | ||||||||||||||||||||||||||
| # Test a complete Stylix configuration with a dark theme, image, and all options | ||||||||||||||||||||||||||
| # set to non-default values. | ||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||
| config, | ||||||||||||||||||||||||||
| lib, | ||||||||||||||||||||||||||
| pkgs, | ||||||||||||||||||||||||||
| ... | ||||||||||||||||||||||||||
| }: | ||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||
| stylix = { | ||||||||||||||||||||||||||
| enable = true; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| # TODO: Declare different dark and light themes after resolving [1]. | ||||||||||||||||||||||||||
| # | ||||||||||||||||||||||||||
| # [1]: https://github.com/nix-community/stylix/issues/1855 | ||||||||||||||||||||||||||
| icons = { | ||||||||||||||||||||||||||
|
0xda157 marked this conversation as resolved.
|
||||||||||||||||||||||||||
| dark = "Adwaita"; | ||||||||||||||||||||||||||
| enable = true; | ||||||||||||||||||||||||||
| light = "Adwaita"; | ||||||||||||||||||||||||||
|
Comment on lines
+17
to
+19
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why are we using the light icons for dark?
Suggested change
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
As implied by the theme name and its file header,
Does $ 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 |
||||||||||||||||||||||||||
| package = pkgs.adwaita-icon-theme; | ||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||
|
Comment on lines
+16
to
+21
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could someone verify whether this works? |
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| overlays.enable = true; | ||||||||||||||||||||||||||
| polarity = "dark"; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| override = | ||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||
| author = "Stylix"; | ||||||||||||||||||||||||||
| scheme = "Catppuccin Macchiato and Mocha"; | ||||||||||||||||||||||||||
| slug = "catppuccin-macchiato-and-mocha"; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| // lib.importJSON ( | ||||||||||||||||||||||||||
| pkgs.runCommand "catppuccin-macchiato-and-mocha.json" | ||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||
| nativeBuildInputs = [ pkgs.yq-go ]; | ||||||||||||||||||||||||||
| src = "${pkgs.base16-schemes}/share/themes/catppuccin-mocha.yaml"; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| '' | ||||||||||||||||||||||||||
| yq \ | ||||||||||||||||||||||||||
| --output-format json \ | ||||||||||||||||||||||||||
| ' | ||||||||||||||||||||||||||
| .palette | | ||||||||||||||||||||||||||
| with_entries(select(.key | test("^base0(1|3|5|7|9|B|D|F)$"))) | ||||||||||||||||||||||||||
| ' \ | ||||||||||||||||||||||||||
| <"$src" \ | ||||||||||||||||||||||||||
| >"$out" | ||||||||||||||||||||||||||
| '' | ||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| # TODO: Explicitly enable the color functionality with 'stylix.colors.enable | ||||||||||||||||||||||||||
| # = true;' once this option is implemented. | ||||||||||||||||||||||||||
| base16Scheme = "${pkgs.base16-schemes}/share/themes/catppuccin-macchiato.yaml"; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| cursor = { | ||||||||||||||||||||||||||
| name = "Bibata-Modern-Amber"; | ||||||||||||||||||||||||||
| package = pkgs.bibata-cursors; | ||||||||||||||||||||||||||
| size = 25; | ||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| fonts = { | ||||||||||||||||||||||||||
| # TODO: Since this declaration is the default stylix.fonts.emoji value, | ||||||||||||||||||||||||||
| # the default value or this declaration should be changed. | ||||||||||||||||||||||||||
| emoji = { | ||||||||||||||||||||||||||
| name = "Noto Color Emoji"; | ||||||||||||||||||||||||||
| package = pkgs.noto-fonts-color-emoji; | ||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| monospace = { | ||||||||||||||||||||||||||
| name = "Fira Code"; | ||||||||||||||||||||||||||
| package = pkgs.fira-code; | ||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| sansSerif = { | ||||||||||||||||||||||||||
| name = "Noto Sans"; | ||||||||||||||||||||||||||
| package = pkgs.noto-fonts; | ||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| serif = { | ||||||||||||||||||||||||||
| name = "Noto Serif"; | ||||||||||||||||||||||||||
| package = pkgs.noto-fonts; | ||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| sizes = lib.genAttrs (builtins.attrNames config.stylix.fonts.sizes) (_: 14); | ||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| # Test path containing special characters. | ||||||||||||||||||||||||||
| image = | ||||||||||||||||||||||||||
| let | ||||||||||||||||||||||||||
| file = "${ | ||||||||||||||||||||||||||
| lib.pipe "-0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ_abcdefghijklmnopqrstuvwxyz" [ | ||||||||||||||||||||||||||
| lib.stringToCharacters | ||||||||||||||||||||||||||
| (builtins.concatStringsSep " ") | ||||||||||||||||||||||||||
| ] | ||||||||||||||||||||||||||
| }.jpg"; | ||||||||||||||||||||||||||
| in | ||||||||||||||||||||||||||
| "${ | ||||||||||||||||||||||||||
| pkgs.runCommandLocal "image-path-containing-special-characters" | ||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||
| src = pkgs.fetchurl { | ||||||||||||||||||||||||||
| hash = "sha256-Dm/0nKiTFOzNtSiARnVg7zM0J1o+EuIdUQ3OAuasM58="; | ||||||||||||||||||||||||||
| name = "image.jpg"; | ||||||||||||||||||||||||||
| url = "https://unsplash.com/photos/ZqLeQDjY6fY/download"; | ||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| '' | ||||||||||||||||||||||||||
| mkdir "$out" | ||||||||||||||||||||||||||
| cp "$src" "$out"/${lib.escapeShellArg file} | ||||||||||||||||||||||||||
| '' | ||||||||||||||||||||||||||
| }/${file}"; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| opacity = lib.genAttrs (builtins.attrNames config.stylix.opacity) (_: 0.8); | ||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
This file was deleted.
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are we reverting 73b7d0f? this pretty clear goes against the spirit of 7682713
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new
disabledtestcase is failing. The following commit fixes that: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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
I am optimistically marking this PR as ready for review again.
It seems whatever was the issue in that discussion, seems to have resolved itself in the meantime, as indicated by the new theme coverage:
imagebase16Schemedisabledfalsefalsedefaultfalsetruepartial-lighttruefalsefull-darktruetrueUh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weighing in here as a random stylix user: in general
enableoptions 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
enablegoes against that since it makes the HM options existence conditional onenable, which forces users to hide all their HM config behindstylix.enable. That shouldn't be needed and is bad/unconventional UX. AndmkIfis not even enough since the options don't exist, you needoptionalAttrsor similar, to completely hide theconfigvalues 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I'd recommend adding a comment in the code to explain this, cause it might seem surprising at first compared to other modules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you mean whether
optionsshould always be available, this is not generally the case and is AFAIK an undocumented convention: #2078.As previously mentioned, the final
[PATCH 3/3] stylix/testbeds/themes: overhaul testbed themes for broader coveragepatch is exactly what increases correctness confidence:Without it, we merely assume this works. I am in favor of merging this only with the new test coverage.
I assume you are comparing the
/stylix/home-manager-integration.nixcore code to/modules/<MODULE>/{hm,nixos}.nixmodules. 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:I think adding such a comment would be actively confusing because it re-states the norm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it's an undocumented convention, but it holds true across the entire Nix ecosystem AFAIK.
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) :)