-
Notifications
You must be signed in to change notification settings - Fork 9
programs/kitty: init module #56
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: main
Are you sure you want to change the base?
Conversation
70bfb2f
to
95c3b98
Compare
413e45f
to
5f7d133
Compare
04620f6
to
a45a920
Compare
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.
LGTM :) thanks a lot for your many contributions
No problem! Thanks for helping building this awesome project that made me go back to using nix to manage dotfiles. |
Let me come in and thank BOTH of you for your contributions. I was involved in a
Both of these would require a ton of work to get to a functional state, and after the RFC didn't receive much positive feedback, progress stalled. But Hjem, paired with Hjem-Rum, is the first project I've seen that actually could change the status quo. However, a project like this needs two things - good standards, and good contributions. You two have been huge parts of both sides of this. We've gotten to a point that this project isn't just "promising" anymore - it's actually starting to be a viable alternative. I want to echo something I've been saying for months - we should actively consider the future of upstreaming. The NixOS new-user experience is limited by home-manager being a de-facto requirement. The idea of home management being built-in, without the need for another project, would be a huge improvement for tons of users. There's still more work to do - especially when it comes to Hjem improvements like the linker. But there's a ton of potential here and I think it's important to begin these conversations sooner rather than later. |
@llakala I agree and think this deserves its own discussion/issue. Ping me if you end up creating one. |
I think Hjem is a great place to start when it comes to upstreaming - modules are always subjective, but Hjem provides the raw functionality missing in the NixOS project. I'm talking to @nezia1 now about the steps we can take and how we should discuss the idea with the Hjem maintainers - I'll likely make an issue/discussion there. |
@llakala hjem has discussions enabled, open one there. |
Okay, I do have more time to give to your PR now and I went through it extensively. Here's a couple of points that, in my opinion, should be addressed: Firstly, the Secondly, I thought about the Themes are (for kitty), in essence, simply a A more generic approach to this would be to have a way for end-users to provide their own Here's an example of what it would look like for setting up adaptive theming: extraConfFiles = {
# we can omit the `.conf` here, as this will/should be handled by the module
"dark-theme.auto".src = "${pkgs.kitty-themes}/share/kitty-themes/themes/Catppuccin-Mocha.conf";
"light-theme.auto".src = "${pkgs.kitty-themes}/share/kitty-themes/themes/Catppuccin-Latte.conf";
} This could be additionally used to provide a global, non adaptive theme too: extraConfFiles = {
theme = {
src = "${pkgs.kitty-themes}/share/kitty-themes/themes/Catppuccin-Mocha.conf";
include = true; # appends an include statement to the end of `kitty.conf`
};
} The reason this would be preferred here, even though hjem-rum being a modules collection, we would like it to ideally stay as minimal and provide as little overhead as possible, and not repeat the mistakes other modules systems might have made in the past. Of course, this would be well documented, with examples for popular use cases such as theming. PS: I would also like to thank you for your patience when we're being incredibly picky about the code-base, and I would like to apologize for giving you a LGTM earlier when I was misinformed on how Kitty worked. |
@nezia1 no problem! I forgot to test without a theme set, sorry! As for the extraConfFiles, I was already thinking about something like this, as kitty tools are also configured trough .conf files inside the kitty config dir (eg.
Actually, thanks for being picky, this kind of stuff need to be properly addressed otherwise it'll become headache in the future! |
Completely in support of this API as a Kitty user! If there's nothing else, I think this can be unblocked now - I can guarantee I won't be changing my mind on this, at least. hjr.programs.kitty.themes =
{
sources =
{
kitty-themes = "${pkgs.kitty-themes}/share/kitty-themes/themes";
# Some other source
local = ./local/my_kitty_themes;
};
current =
{
name = "Catppuccin-Mocha";
source = "kitty-themes";
};
}; |
But why have an attrset of themes? You ideally only want one or two themes (for light/dark mode). I don't see a real value to add an option to set theme.source = "${pkgs.kitty-themes}/share/kitty-themes/theme.conf";
# Or with light/dark theme
theme = {
light.source = "${pkgs.kitty-themes}/share/kitty-themes/light-theme.conf";
dark.source = "${pkgs.kitty-themes}/share/kitty-themes/dark-theme.conf";
}; What do you guys think? |
Do you mean the
It's not
The issue with doing it like this is that this is just What we could do here, we could do with For light and dark, we could do current.light/current.dark, with their content being the same as current (i.e. name and source). |
Oh, sorry! I misunderstood the point. You actually meant putting all kitty-themes by default but being able to customize all the sources, isn't it? I think this might be a good idea, I'm going to see how I can implement this. |
This is overcomplicating it. There is no need to have a separate definition of source directory when that could merely be in the theme setting, and no need to divide the name from the source. Really, we should be making this as minimal and simple as possible. I agree with arthsmn that because the user would only need 1-2 themes in use at one time, there is no point to widen the source scope. In your own example, you only use one of the sources declared, and so the rest ends up as dead code. I believe the only limiting agent is the fact that some users will require two, for light and dark. I actually think arthsmn's example here was the best so far:
We give a simple option of using one theme, where the user includes the source in the option (like any sane module, if we want a single file from a user, that user should be able to supply that file in a single option), but if they use a light and dark mode, they can instead give that. If the user wanted to use a light theme from kitty-themes and a dark theme from a local repository, they could do that. If they wanted to easily switch between using a light/dark theme to using a single theme, they could do that.
So, this is fair point. I do see that there are other programs that kitty uses added conf files for, so here's what I'm thinking our main options are:
Because I am not a kitty user, I am not sure how common this is, or how many there are. If it's just something like themes & plugins, I would vouch for the latter. Personally, I generally prefer to not hide a bunch of files in a single option, but it's definitely something we could do. That being said, here are my API suggestions for each of these. API 1: rum.programs.kitty.extraFiles = {
# The problem here is that you wouldn't be able to provide a string for this file.
"theme.conf".source = "${pkgs.kitty-themes}/share/kitty-themes/theme.conf";
# OR setting this
"light-theme.conf".source = "${pkgs.kitty-themes}/share/kitty-themes/light-theme.conf";
"dark-theme.conf".source = "${pkgs.kitty-themes}/share/kitty-themes/dark-theme.conf";
}; API 2: # If using a single theme
rum.programs.kitty = {
theme = {
default = "${pkgs.kitty-themes}/share/kitty-themes/theme.conf";
# OR setting this for a light-dark theme
light = "${pkgs.kitty-themes}/share/kitty-themes/light-theme.conf";
dark = "${pkgs.kitty-themes}/share/kitty-themes/dark-theme.conf";
};
# Just a hypothetical example of another program that requires an extra conf file
plugins = {
my-plugin = "${pkgs.my-plugin}/share/my-plugin/plugin.conf";
};
}; |
@Lunarnovaa, thanks for your input! I think doing the second API is the best approach, but I'll first see how many kittens have configs to see if it's not going to bloat the options. For themes specifically, we can easily check if a string is a path of a file, so this approach can also enable in-line themes. |
I think these alternative iideas are overcomplicating it further. We're already adding complication over home-manager by letting people customize where the themes are from. I think the split between sources and themes that actually get used is a good balance between usability and extensibility. |
Overcomplicating? Users can only have one or two themes, so it's not necessary to add multiple sources, this is overcomplication. Giving the more verbose and manual, which I and Lunarnovaa are suggesting, is simpler and makes the user aware of where the themes are coming from, as they are going to declare it, without any boilerplate over sources. As I said before, I'll document where to find the "standard" themes and how to set them. |
19a365c
to
4d81766
Compare
@nezia1 @Lunarnovaa @llakala I've implemented the API suggested by Lunarnovaa. Edit: added a simple test based on foot's one. |
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.
Looking great, I appreciate your contribution. Still needs some work though. Thank you!
a809fa6
to
551d04e
Compare
I decided to implement |
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 know there's been a lot of debate on the theme
option above, but I want to add my thoughts:
I dislike the theme.default
option because .config/kitty/current-theme.conf
isn't a special path, it still needs the include
directive, making it the perfect candidate to include in extraConfFiles
.
That said, I'm in favour of theme.light
and theme.dark
with a request to also include theme.no-preference
, since all 3 of these are special files recognised by kitty.
Also, a user being allowed to define their own themes in EDIT: It's auto-imported by kitten, not kitty, meaning that the .config/kitty/themes/
(also a special path -- auto-imported by kitty) is not an option here yet.include
directive is still required.
I'm gonna mess with these around in my module where I missed these special paths.
// optionalAttrs (cfg.extraConfFiles != {}) (mapAttrs' | ||
( | ||
name: val: | ||
nameValuePair ".config/kitty/${name}" {text = val;} | ||
) | ||
cfg.extraConfFiles); |
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'd prefer having include
statements be generated for each file and not symlink them into the kitty config directory.
E.g., I have the following in my config:
extraConfig = ''
include ${pkgs.kitty-themes}/share/kitty-themes/themes/moonlight.conf
'';
which includes the nix store path to the conf file, no need to clutter $HOME.
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.
TBF, now that I think about it, that can simply be done by manually writing down an include
directive as above, we don't need an extraConfFiles
option at all.
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'd be more than okay with an extraConfig
option, this is a much simpler solution and again, we could give an example for themes 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.
Unsure, I scrolled through the Kitty config docs a good bit, and none of the options really justify having an extraConfig
either. Even for the include, settings.include = [];
should suffice.
I could be wrong, feel free to point out if I'm missing something
I like this. I'm not home atm so I can't do reviews, but if you would like to implement these thoughts into your PR, I can take a look at some point in the next couple days and hopefully merge that. |
No description provided.