-
Notifications
You must be signed in to change notification settings - Fork 77
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
Add colortheme configuration to kate #95
Conversation
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.
See my comments for some initial possible tweaks
modules/apps/kate.nix
Outdated
${./check-theme-name-free.sh} ${cfg.editor.theme.name} ${pkgs.jq}/bin/jq | ||
''); | ||
|
||
# In case of using a system theme, there should be a check that there exists such a theme |
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.
Is there something missing here?
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.
tldr: No, this is a note that there should be a check – if I just knew how to. (See "known limitations" above.) I improved the comment.
long form: I would like to check that a user can only select valid themes – that are those already packaged in the installed kate package. I could not figure out a viable way, which makes this an error-prone string entry.
modules/apps/kate.nix
Outdated
# In case of using a custom theme, check that there is no name collision | ||
home.activation.checkKateTheme = lib.mkIf (cfg.enable && cfg.editor.theme.src != null) (lib.hm.dag.entryBefore [ "writeBoundary" ] | ||
'' | ||
${./check-theme-name-free.sh} ${cfg.editor.theme.name} ${pkgs.jq}/bin/jq |
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.
Here I would suggest creating a nix-derivation here which runs the script. That way you don't have to send in the jquery path as a command-line argument, and in general is a better practice. See here for how we did with the config-writing script
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.
I changed to to a derivation in the same style. Imo it looks a bit bloated:
checkThemeNameScript = pkgs.writeShellApplication {
name = "checkThemeName";
runtimeInputs = with pkgs; [ jq ];
text = builtins.readFile ./check-theme-name-free.sh;
};
checkThemeName = name:
''
${checkThemeNameScript}/bin/checkThemeName ${name}
'';
script = pkgs.writeScript "kate-check" (checkThemeName cfg.editor.theme.name);
Maybe it would be nicer to do this with just 1-2 definitions? My attempt would be:
checkThemeNameScript = pkgs.writeShellApplication {
name = "checkThemeName";
runtimeInputs = with pkgs; [ jq ];
text = builtins.readFile ./check-theme-name-free.sh;
};
script = pkgs.writeScript "kate-check" "${checkThemeNameScript}/bin/checkThemeName ${name}";
What do you think?
@magnouvean do my changes look alright to you? |
I will check this out later today (when I get home). There has been a change in the configuration syntax lately after #94, so will probably need some tweaks, but they should be fairly straight forward :). Your solutions to the comments look good though :) |
@magnouvean You are right it needs some tweaks. I would have preferred to rebase this branch myself & do a new commit at the same time, so there wouldn't have been this broken state now ;) |
Ah sorry about that. Some prefer different things. I can revert it :) |
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.
Would be good if you could make sure to have ran nixpkgs-fmt
on these files. Other than that if you can get these functional with the latest syntax I think this looks great!
I did the changes you proposed. |
No, immutability isn't needed granted the module works just fine without them. I see there were some problems with the merging leading to changes in |
@magnouvean yes I'm happy with the current state :) |
Great, I'll merge right away then! Thanks for the contribution, prs like this helps |
This is the second contribution to #87.
This contribution adds support for kate's colorthemes. A user can either pick one of the system themes by name, or a file for a custom theme.
The custom file will never override any existing file, nor will it introduce name collisions.
Known limitations
When picking a theme by name there is no check whether such a theme exists. The existing themes live in the
ksyntaxhighlighting
framework indata/themes
.But these existing themes do not live in any of
${XDG_DATA_DIRS}
. They are included as a linked library. I don't know if there is any non-ugly way to access these (option 1: via an overlay. option 2: going through multiple layers of built-inputs).As an artefact of that process (when I tried to find the themes) there is an option
home.kate.package
that allows the user to pick a package to be installed. By default, this ispkgs.kate
. That changes the default behaviour ofprograms.kate.enable
: When enabling this option, a package will be installed. I believe this is a more natural behaviour for the enable option.