vivid: replace theme content from yaml with json #7996
Conversation
modules/programs/vivid.nix
Outdated
|
|
||
| home.sessionVariables = mkIf (cfg.activeTheme != null) { VIVID_THEME = cfg.activeTheme; }; | ||
| config = mkMerge [ | ||
| (mkIf (cfg.enable || cfg.themes != { }) { |
There was a problem hiding this comment.
Why the OR operator? Shouldn't it be &&? We don't want to generate files in user's home unless they explicitly enable programs.vivid
There was a problem hiding this comment.
Why the OR operator? Shouldn't it be
&&? We don't want to generate files in user's home unless they explicitly enableprograms.vivid
No:
this Stylix module has no effect unless end-users happen to enable the underlying
programs.vivid.enablemodule. Although this is how all Stylix modules are supposed to work, it might be worth considering to make an exception for this module because it does not theme itself, but rather declares theming for an unknown amount of unrelated applications.In other words, Stylix only cares about the
home.sessionVariables.LS_COLORSstring, and nothing else. Technically, this extends to thexdg.configFile."vivid/themes/${cfg.activeTheme}.yml".sourcefile.
The cfg.enable || didWeDeclareTheYamlAttributeSet condition, where didWeDeclareTheYamlAttributeSet == cfg.themes != { }, ensures that home.sessionVariables.LS_COLORS is declared without cfg.enable necessarily being enabled.
There was a problem hiding this comment.
Sorry but that makes no sense to me. We shouldn't set the LS_COLORS env variable unless users explicitly set they want vivid to do so. In any case, you could make the Stylix module to forcefully enable programs.vivid, but I'm not willing to make Home-Manager change user's env just because you think so.
There was a problem hiding this comment.
Sorry but that makes no sense to me. We shouldn't set the
LS_COLORSenv variable unless users explicitly set they want vivid to do so.
His idea was also to include a enableSessionVariables option, so didWeDeclareTheYamlAttributeSet == cfg.enableSessionVariables && cfg.themes != { }. This wouldn't mess with the users environment. I guess this is my fault, since I excluded it due to your comment about not needing additional options.
There was a problem hiding this comment.
To me it doesn't change anything. Still we shouldn't introduce changes that would affect users regardless of them manually setting programs.vivid.enable to true. Home-Manager doesn't do this with any other module, so neither we should do here. Feel free to change the behavior in Stylix, but even then I don't think that would be the right choice.
There was a problem hiding this comment.
What @trueNAHO is proposing, is to still have an additional option of just enabling that one single theme and that's it. Not additional saving of themes in
~/.config/vivid/themesfolder or setting theVIVID_THEMEvariable. None of this is technically needed to just apply theactiveTheme. This would indeed reduce the amount of "bloat" when using Stylix, since we just care for one theme, namely the generated stylix-theme.
Yes. My nix-community/stylix#1938 (comment) comment explains it in more detail. Basically, aside from programs.vivid.enable enabling everything, there should be a second mode where home.sessionVariables.LS_COLORS is declared with the static YAML theme and nothing else is installed. The programs.vivid.enableSessionVariables option allows explicitly disabling the second mode.
Maybe we're trying to over-engineer something that should be kept simple; the end user won't notice it much either way. I guess keeping it simple would be the way to go, i.e., having one
enableoption, which would make themkMergeredundant.And please don't get me wrong, I like the approach of fine-tuning the configuration, allowing the user to keep only the essential parts. But the question is, is it worth the additional "complexity" in the end?
Yes, but since we already figured out how to solve this, we might as well do it:
I'm not really getting why you're so worried about making vivid a runtime dependency, @trueNAHO.
Initially, I only considered improving the shell startup time by running
vivid generateat evaluation time. Based on #6045 (comment), removing the optional runtime dependency seems like the final stage of runningvivid generateat evaluation time. So we might as well drive this concept to its logical conclusion, so that it is definitely done.
Otherwise, Stylix is required to enable programs.vivid.enable, which is a problem:
this Stylix module has no effect unless end-users happen to enable the underlying
programs.vivid.enablemodule. Although this is how all Stylix modules are supposed to work, it might be worth considering to make an exception for this module because it does not theme itself, but rather declares theming for an unknown amount of unrelated applications.
There was a problem hiding this comment.
You mean adding a single theme option and deprecating themes and activeTheme? Yeah that makes sense to me, but make sure to use mkRemovedOptionModule so users face a message saying that they should use programs.vivid.theme instead. Still, that doesn't change my point on setting LS_COLORS without setting programs.vivid.enable = true;
There was a problem hiding this comment.
You mean adding a single
themeoption and deprecatingthemesandactiveTheme?
This PR should:
- Fix the YAML conversion by replacing it with JSON.
- Implement a second enabling mode for setting
LS_COLORSwhen having setcfg.activeThemeto a static YAML attribute set. This second enabling mode should be toggable with something likecfg.enableSessionVariables, just likecfg.enable.
The themes and activeTheme option can stay as-is.
Now, it also further restricts the cfg.colorMode type.
The
cfg.enable || didWeDeclareTheYamlAttributeSetcondition, wheredidWeDeclareTheYamlAttributeSet == cfg.themes != { }, ensures thathome.sessionVariables.LS_COLORSis declared withoutcfg.enablenecessarily being enabled.
The didWeDeclareTheYamlAttributeSet condition is likely too conservative and should be replaced with something like builtins.any builtins.isAttrs (builtins.attrNames cfg.themes). However, we can worry about this detail once we decided on the design, which in this case is the second enabling mode.
There was a problem hiding this comment.
I want you too see things from my perspective and forget about Stylix for a second, @trueNAHO. The problem you're presenting is specific to Stylix. You said that since Stylix would theme other programs through vivid, it might be worth considering to do so by default. IMO, neither Home-Manager or Stylix should make changes to users' env unless they opt-in (in some way) to so. To me, vivid shouldn't be an exception, and making Home-Manager to set LS_COLORS even when programs.vivid.enable = false; will lead to unexpected behavior. AFAICS, the only problem I see in making Stylix enable vivid is that it will be added to users' PATH, but that's not a big deal. The point I want to make is that this is Home-Manager, and we shouldn't implement changes for the sake of another project.
There was a problem hiding this comment.
I want you too see things from my perspective and forget about Stylix for a second, @trueNAHO. The problem you're presenting is specific to Stylix. You said that since Stylix would theme other programs through vivid, it might be worth considering to do so by default. IMO, neither Home-Manager or Stylix should make changes to users' env unless they opt-in (in some way) to so. To me, vivid shouldn't be an exception, and making Home-Manager to set
LS_COLORSeven whenprograms.vivid.enable = false;will lead to unexpected behavior. AFAICS, the only problem I see in making Stylix enable vivid is that it will be added to users' PATH, but that's not a big deal. The point I want to make is that this is Home-Manager, and we shouldn't implement changes for the sake of another project.
Yes, I totally get this.
However, as evident by nix-community/stylix#543 people seem to care about debloating their system closures, which is a general Nix problem and not specific to Stylix. Also, Home Manager's cfg.enable logic not always consistent, which is why nix-community/stylix#543 is still open. Most people simply don't complain about this problem, although it is one.
I am fine with dropping the second enabling logic in this Home Manager module. This is merely a proposal to support this debloatting use case.
Side node: Ensuring Home Manager consistency could be done with a function like mkHomeManagerModule, as inspired by NixVim, or a custom module system, as currently experimented with in Stylix. I would recommend against introducing a custom module system to Home Manager until Stylix has fully figured out how to make the custom module system great. Either way, this is a lot of effort and will probably not be fully resolved any time soon. The simpler solution is to manually fix the problematic Home Manager modules. And this second enabling mode is one such manual fix.
modules/programs/vivid.nix
Outdated
| config = mkMerge [ | ||
| (mkIf (cfg.enable || cfg.themes != { }) { | ||
| home.sessionVariables.LS_COLORS = | ||
| let | ||
| colorMode = lib.optionalString (cfg.colorMode != null) "-m ${cfg.colorMode}"; | ||
| themePath = | ||
| if builtins.isAttrs cfg.themes.${cfg.activeTheme} then | ||
| pkgs.writeText "${cfg.activeTheme}.json" (builtins.toJSON cfg.themes.${cfg.activeTheme}) | ||
| else if config.xdg.configFile ? "vivid/themes/${cfg.activeTheme}.yml" then | ||
| config.xdg.configFile."vivid/themes/${cfg.activeTheme}.yml".source | ||
| else | ||
| cfg.activeTheme; | ||
| in | ||
| "$(cat ${ | ||
| pkgs.runCommand "ls-colors" { | ||
| nativeBuildInputs = [ cfg.package ]; | ||
| } "vivid ${colorMode} generate ${themePath} > $out" | ||
| })"; | ||
| }) | ||
| (mkIf cfg.enable { | ||
| home.sessionVariables = mkIf (cfg.activeTheme != "") { VIVID_THEME = cfg.activeTheme; }; |
There was a problem hiding this comment.
Good overall, but I'm pretty sure we'll face some error during the tests check. You should also add assertions that ensure cfg.themes contains cfg.activeTheme since otherwise the user will face an ugly error message at evaluation time. You should also set an assertion ensuring the user sets the cfg.activeTheme option, since otherwise if we give vivid no arguments it will throw an error and the derivation will fail.
There was a problem hiding this comment.
I'm a bit busy right now, so I can't review this part by part, but I'll share the code I was working on yesterday so you can adapt it. It doesn't use mkMerge, and the logic is a bit different, but take the parts you need. (I didn't made the tests to pass though, so probably there's something wrong with the asserts logic)
{
lib,
pkgs,
config,
...
}:
let
inherit (lib)
mkIf
mkEnableOption
mkPackageOption
mkOption
types
;
cfg = config.programs.vivid;
yamlFormat = pkgs.formats.yaml { };
jsonFormat = pkgs.formats.json { };
in
{
meta.maintainers = with lib.hm.maintainers; [ aguirre-matteo ];
imports = [
(lib.mkRemovedOptionModule [
"programs"
"vivid"
"enableBashIntegration"
] "Now the shell integration is always enabled.")
(lib.mkRemovedOptionModule [
"programs"
"vivid"
"enableZshIntegration"
] "Now the shell integration is always enabled.")
(lib.mkRemovedOptionModule [
"programs"
"vivid"
"enableFishIntegration"
] "Now the shell integration is always enabled.")
];
options.programs.vivid = {
enable = mkEnableOption "vivid";
package = mkPackageOption pkgs "vivid" { };
colorMode = mkOption {
type = types.enum [
"8-bit"
"24-bit"
];
default = "24-bit";
example = "8-bit";
description = ''
Color mode for vivid.
'';
};
filetypes = mkOption {
inherit (yamlFormat) type;
default = { };
example = {
text = {
special = [
"CHANGELOG.md"
"CODE_OF_CONDUCT.md"
"CONTRIBUTING.md"
];
todo = [
"TODO.md"
"TODO.txt"
];
licenses = [
"LICENCE"
"COPYRIGHT"
];
};
};
description = ''
Filetype database for vivid. You can find an example config at:
<https://github.com/sharkdp/vivid/blob/master/config/filetypes.yml>.
'';
};
activeTheme = mkOption {
type = with types; nullOr str;
default = null;
example = "molokai";
description = ''
Active theme for vivid.
'';
};
themes = mkOption {
type = with types; attrsOf (either path yamlFormat.type);
default = { };
example = lib.literalExpression ''
{
ayu = builtins.fetchurl {
url = "https://raw.githubusercontent.com/NearlyTRex/Vivid/refs/heads/master/themes/ayu.yml";
hash = "sha256-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=";
};
mocha = builtins.fetchurl {
url = "https://raw.githubusercontent.com/NearlyTRex/Vivid/refs/heads/master/themes/catppuccin-mocha.yml";
hash = "sha256-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=";
};
my-custom-theme = {
colors = {
blue = "0000ff";
};
core = {
directory = {
foreground = "blue";
font-style = "bold";
};
};
};
}
'';
description = ''
An attribute set of vivid themes.
Each value can either be a path to a theme file or an attribute set
defining the theme directly.
'';
};
};
config =
let
vividThemes = lib.mapAttrs' (
name: value:
lib.nameValuePair name {
source = if lib.isAttrs value then jsonFormat.generate "${name}.yml" value else value;
}
) cfg.themes;
in
mkIf cfg.enable {
assertions = [
{
assertion = cfg.activeTheme != null;
message = "You must set `programs.vivid.activeTheme`.";
}
{
assertion = (cfg.activeTheme != null) -> (vividThemes ? cfg.activeTheme);
message = "Couldn't find the `${cfg.activeTheme}` theme in `programs.vivid.themes`.";
}
];
home.packages = [ cfg.package ];
home.sessionVariables =
mkIf (cfg.activeTheme != null) {
VIVID_THEME = cfg.activeTheme;
}
// (lib.optionalAttrs (vividThemes ? cfg.activeTheme) {
LS_COLORS = "$(cat ${
pkgs.runCommand "vivid-generate" { nativeBuildInputs = [ cfg.package ]; } ''
vivid -m ${cfg.colorMode} generate ${vividThemes.${cfg.activeTheme}}
''
})";
});
xdg.configFile = {
"vivid/filetypes.yml" = mkIf (cfg.filetypes != { }) {
source = yamlFormat.generate "vivid-filetypes" cfg.filetypes;
};
}
// (lib.mapAttrs' (name: value: lib.nameValuePair "vivid/themes/${name}.yml" value) vividThemes);
};
}There was a problem hiding this comment.
Good overall, but I'm pretty sure we'll face some error during the tests check. You should also add assertions that ensure
cfg.themescontainscfg.activeThemesince otherwise the user will face an ugly error message at evaluation time. You should also set an assertion ensuring the user sets thecfg.activeThemeoption, since otherwise if we givevividno arguments it will throw an error and the derivation will fail.
The problem is that we are currently always treating cfg.activeTheme to be defined as a cfg.themes YAML, which is too restrictive. #7996 (comment) addresses this.
I'm a bit busy right now, so I can't review this part by part, but I'll share the code I was working on yesterday so you can adapt it.
Your proposal makes major changes to the guarding and makes default values more restrictive. Without separate discussions this is too much at once to discuss.
Although I am biased, I believe my nix-community/stylix#1938 (comment) proposal to be better.
let vividThemes = lib.mapAttrs' ( name: value: lib.nameValuePair name { source = if lib.isAttrs value then jsonFormat.generate "${name}.yml" value else value; } ) cfg.themes; in
Centralizing the jsonFormat.generate calls seems like a good addition.
There was a problem hiding this comment.
Would be nice to add the following to the commit message:
Co-authored-by: NAHO <[email protected]>
modules/programs/vivid.nix
Outdated
|
|
||
| home.sessionVariables = mkIf (cfg.activeTheme != null) { VIVID_THEME = cfg.activeTheme; }; | ||
| config = mkMerge [ | ||
| (mkIf (cfg.enable || cfg.themes != { }) { |
There was a problem hiding this comment.
This condition seems to have accidentally changed since nix-community/stylix#1938 (comment), as it should have been:
| (mkIf (cfg.enable || cfg.themes != { }) { | |
| (mkIf (cfg.enable || (cfg.enableSessionVariables && cfg.themes != { })) { |
Also, the new cfg.enableSessionVariables option declaration is missing.
There was a problem hiding this comment.
I removed it due to the comment of @aguirre-matteo: #7994 (comment)
So we would have one option, which would set the session variable LS_COLORS without installing the package and setting the files in the $XDG_CONFIG_HOME/vivid folder.
modules/programs/vivid.nix
Outdated
|
|
||
| home.sessionVariables = mkIf (cfg.activeTheme != null) { VIVID_THEME = cfg.activeTheme; }; | ||
| config = mkMerge [ | ||
| (mkIf (cfg.enable || cfg.themes != { }) { |
There was a problem hiding this comment.
Why the OR operator? Shouldn't it be
&&? We don't want to generate files in user's home unless they explicitly enableprograms.vivid
No:
this Stylix module has no effect unless end-users happen to enable the underlying
programs.vivid.enablemodule. Although this is how all Stylix modules are supposed to work, it might be worth considering to make an exception for this module because it does not theme itself, but rather declares theming for an unknown amount of unrelated applications.In other words, Stylix only cares about the
home.sessionVariables.LS_COLORSstring, and nothing else. Technically, this extends to thexdg.configFile."vivid/themes/${cfg.activeTheme}.yml".sourcefile.
The cfg.enable || didWeDeclareTheYamlAttributeSet condition, where didWeDeclareTheYamlAttributeSet == cfg.themes != { }, ensures that home.sessionVariables.LS_COLORS is declared without cfg.enable necessarily being enabled.
modules/programs/vivid.nix
Outdated
| else | ||
| cfg.activeTheme; |
There was a problem hiding this comment.
My proposal from nix-community/stylix#1938 (comment) ensures that pkgs.runCommand does not depend on the $XDG_CONFIG_HOME/vivid/themes runtime population. Should this not crash when accessing cfg.activeTheme like this?
By setting
activeThemeto be only of type string with the empty string as the default, we can also allow the user to set the prebuilt themes too with less checks.
I intentionally left this out in my PoC implementation for simplicity because it requires branching at a higher level:
home.sessionVariables.LS_COLORS =
if static then
"$(cat ${/* ... */})"
else
"$(vivid generate ${cfg.activeTheme})";
modules/programs/vivid.nix
Outdated
| config = mkMerge [ | ||
| (mkIf (cfg.enable || cfg.themes != { }) { | ||
| home.sessionVariables.LS_COLORS = | ||
| let | ||
| colorMode = lib.optionalString (cfg.colorMode != null) "-m ${cfg.colorMode}"; | ||
| themePath = | ||
| if builtins.isAttrs cfg.themes.${cfg.activeTheme} then | ||
| pkgs.writeText "${cfg.activeTheme}.json" (builtins.toJSON cfg.themes.${cfg.activeTheme}) | ||
| else if config.xdg.configFile ? "vivid/themes/${cfg.activeTheme}.yml" then | ||
| config.xdg.configFile."vivid/themes/${cfg.activeTheme}.yml".source | ||
| else | ||
| cfg.activeTheme; | ||
| in | ||
| "$(cat ${ | ||
| pkgs.runCommand "ls-colors" { | ||
| nativeBuildInputs = [ cfg.package ]; | ||
| } "vivid ${colorMode} generate ${themePath} > $out" | ||
| })"; | ||
| }) | ||
| (mkIf cfg.enable { | ||
| home.sessionVariables = mkIf (cfg.activeTheme != "") { VIVID_THEME = cfg.activeTheme; }; |
There was a problem hiding this comment.
Good overall, but I'm pretty sure we'll face some error during the tests check. You should also add assertions that ensure
cfg.themescontainscfg.activeThemesince otherwise the user will face an ugly error message at evaluation time. You should also set an assertion ensuring the user sets thecfg.activeThemeoption, since otherwise if we givevividno arguments it will throw an error and the derivation will fail.
The problem is that we are currently always treating cfg.activeTheme to be defined as a cfg.themes YAML, which is too restrictive. #7996 (comment) addresses this.
I'm a bit busy right now, so I can't review this part by part, but I'll share the code I was working on yesterday so you can adapt it.
Your proposal makes major changes to the guarding and makes default values more restrictive. Without separate discussions this is too much at once to discuss.
Although I am biased, I believe my nix-community/stylix#1938 (comment) proposal to be better.
let vividThemes = lib.mapAttrs' ( name: value: lib.nameValuePair name { source = if lib.isAttrs value then jsonFormat.generate "${name}.yml" value else value; } ) cfg.themes; in
Centralizing the jsonFormat.generate calls seems like a good addition.
arunoruto
force-pushed
the
vivid
branch
2 times, most recently
from
664ce1c to
8198bd4
Compare
|
I think we are discussing too much things at the same time in this PR, so I propose we split it in a series of PRs to clean the commit history and/or better discuss them, and since they are unrelated to each other. We could split them in: 1- Replacing 2- Introducing 3- Change the type of 4- Generating |
arunoruto
force-pushed
the
vivid
branch
2 times, most recently
from
a27aac3 to
bac2948
Compare
Agreed, I changed the contents to just reflect point 1. I guess we can do 4 in a separate PR to get more feedback. I will rewrite the PR title and contents. Btw, I am not sure why the tests are failing... The content is the same, but it says they are different: --- actual
+++ expected
@@ -1 +1 @@
-{"colors":{"primary":"00aaff","secondary":"ff00aa"},"core":{"directory":{"foreground":"primary"},"executable-file":{"font-style":"bold","foreground":"secondary"}}}
\ No newline at end of file
+{"colors":{"primary":"00aaff","secondary":"ff00aa"},"core":{"directory":{"foreground":"primary"},"executable-file":{"font-style":"bold","foreground":"secondary"}}}I even tried it by running |
arunoruto
changed the title
|
Do |
Still the same error 🤔 |
|
Maybe |
Neat, this worked! Thanks! EDIT if its okay, I could also introduce the color mode as an enum in a new commit. Not sure if we need a new PR for such a small change. |
|
I implemented the type change to enum for |
|
You didn't added a comment about why JSON instead of YAML but I guess there's no problem since anyone could just do a git blame and get to this PR. |
This should fix missinterpretation of hex colors (which should be a string) as numbers in scientific notation. For example: 1e2030.
Sorry, totally my bad. It got removed when I reverted the commits back to the original one and implemented the small tweaks. Just in case, I added the comment why JSON is used! |
| type = with types; nullOr str; | ||
| default = null; | ||
| type = types.enum [ | ||
| "8-bit" | ||
| "24-bit" | ||
| ]; | ||
| default = "24-bit"; |
There was a problem hiding this comment.
Not a fan of changes that add maintenance burden for no real benefit. Also forces a cli flag instead of just going with the true app default.
ie: If they ever added different bit supports or changed their default, we'd be enforcing an enum that wouldn't allow a user to change and / or we'd enforce a default that conflicts with upstream.
4 participants
Description
As per the discussion in #7994 and in nix-community/stylix#1938, this PR implements a
home.sessionVariables.LS_COLORSand assigns it a value at evaluation by running vivid withpkgs.runCommand.Since
home.sessionVariables.LS_COLORSsupersedes the shell integrations (#7994 (comment)), they have been removed.Partially addresses the issue #7994.
Values like
1e2030are treated as strings in Nix, but since they have a single non-digit character, don't get enclosed in single quotes. This leads to the problem that such hex codes are treated as numbers in scientific notation.Since json is just a yaml subset (in simple terms, see hackers news), we can use
builtins.toJSON. JSON has much better support in Nix, making it more robust for such a usecase!Checklist
Change is backwards compatible.
Code formatted with
nix fmtornix-shell -p treefmt nixfmt deadnix keep-sorted --run treefmt.Code tested through
nix run .#tests -- test-allornix-shell --pure tests -A run.all.Test cases updated/added. See example.
Commit messages are formatted like
See CONTRIBUTING for more information and recent commit messages for examples.
If this PR adds a new module
If this PR adds an exciting new feature or contains a breaking change.