Skip to content

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

programs/kitty: init module #56

wants to merge 1 commit into from

Conversation

arthsmn
Copy link
Contributor

@arthsmn arthsmn commented May 2, 2025

No description provided.

@arthsmn arthsmn marked this pull request as draft May 2, 2025 12:35
@arthsmn arthsmn force-pushed the kitty branch 6 times, most recently from 70bfb2f to 95c3b98 Compare May 2, 2025 13:21
@arthsmn arthsmn marked this pull request as ready for review May 2, 2025 13:22
@arthsmn arthsmn force-pushed the kitty branch 2 times, most recently from 413e45f to 5f7d133 Compare May 2, 2025 15:29
@arthsmn arthsmn requested a review from nezia1 May 2, 2025 15:29
@arthsmn arthsmn force-pushed the kitty branch 2 times, most recently from 04620f6 to a45a920 Compare May 2, 2025 15:45
@arthsmn arthsmn requested a review from nezia1 May 2, 2025 15:45
nezia1
nezia1 previously approved these changes May 2, 2025
Copy link
Collaborator

@nezia1 nezia1 left a 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

@arthsmn
Copy link
Contributor Author

arthsmn commented May 2, 2025

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.

@llakala
Copy link

llakala commented May 2, 2025

Let me come in and thank BOTH of you for your contributions. I was involved in a NixOS Home Management project a while back, with the goal to find a better way to control home files in NixOS, without having to direct everyone to home-manager. The ideas ended up as either:

  • bring home-manager under the NixOS organization, so it can be more actively maintained and improved
  • start from scratch

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.

@arthsmn
Copy link
Contributor Author

arthsmn commented May 2, 2025

@llakala I agree and think this deserves its own discussion/issue. Ping me if you end up creating one.

@llakala
Copy link

llakala commented May 2, 2025

@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.

@arthsmn
Copy link
Contributor Author

arthsmn commented May 2, 2025

@llakala hjem has discussions enabled, open one there.

@nezia1
Copy link
Collaborator

nezia1 commented May 5, 2025

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 theme option is not optional, so trying it out by simply enabling in my personal config does not work.

Secondly, I thought about the theme option and what it entails, and I actually want to apologize for giving you misinformed feedback (as I asked you to rename theme to themes. I did not yet understand how kitty manages themes. After having more time to look into this, I'm actually not a fan of it for a few reasons:

Themes are (for kitty), in essence, simply a .conf file. This means that all of the options found there are not specific to themes themselves, but rather are just a name given to an include containing theme data. So it would be a little redundant (albeit not heresy, considering NixOS does this constantly to improve module UX). I guess I'm just not too fond on having a second way to provide themes when the end-user could just use settings to set it.

A more generic approach to this would be to have a way for end-users to provide their own .conf files to be put under $XDG_CONFIG_HOME/kitty (and by extension, load anything from within pkgs.kitty-themes). However, for auto theming, we also need an option to not include said conf file inside the main kitty configuration, as it simply expects the files to exist at this location.

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.

@arthsmn
Copy link
Contributor Author

arthsmn commented May 5, 2025

@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. kitten diff). I'll remove the themes option, add this extraConfFiles option and document simple use-cases like theming or configuring tools. As for the inclusion in kitty.conf, single themes can be just set as current-theme.conf, according to the docs.

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.

Actually, thanks for being picky, this kind of stuff need to be properly addressed otherwise it'll become headache in the future!

@llakala
Copy link

llakala commented May 8, 2025

* themes.sources: an attrset of sources, with the attrname as the source name, and a path containing kitty themes

* themes.{current,name}: an attrset consisting of two values: a theme name, and a source, which could be attrNames of sources

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.
Also, here's a mockup, just for fun:

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";
  };
};

@arthsmn
Copy link
Contributor Author

arthsmn commented May 8, 2025

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 name = source;. Also, how light/dark mode will be implemented? Maybe we should have something like this:

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?

@nezia1
Copy link
Collaborator

nezia1 commented May 9, 2025

But why have an attrset of themes?

Do you mean the sources part? This is to avoid the boilerplate you'd have if you do it by setting every single theme's source. It allows referencing themes per name nicely

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 name = source;.

It's not name = source;. The source in the example I gave is kitty-themes, and when setting theme.name, you'd give it the name of a .conf file present inside kitty-themes.

Also, how light/dark mode will be implemented? Maybe we should have something like this:

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?

The issue with doing it like this is that this is just extraConfFiles with a different name.

What we could do here, we could do with extraConfFiles (and it does add quite a bit of boilerplate, compared to having a themes.sources attribute, allowing for easy extensibility and also setting kitty-themes as one of the default values).

For light and dark, we could do current.light/current.dark, with their content being the same as current (i.e. name and source).

@arthsmn
Copy link
Contributor Author

arthsmn commented May 9, 2025

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.

@Lunarnovaa
Copy link
Collaborator

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";
  };
};

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:

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";
};

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.

The issue with doing it like this is that this is just extraConfFiles with a different name.

What we could do here, we could do with extraConfFiles (and it does add quite a bit of boilerplate, compared to having a themes.sources attribute, allowing for easy extensibility and also setting kitty-themes as one of the default values).

For light and dark, we could do current.light/current.dark, with their content being the same as current (i.e. name and source).

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:

  1. Add a special option like extraConfFiles that can handle any extra config files on its own
  2. Add options for each kind of program that would need extra config files.

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";
  };
};

@arthsmn
Copy link
Contributor Author

arthsmn commented May 9, 2025

@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.

@llakala
Copy link

llakala commented May 9, 2025

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.

@arthsmn
Copy link
Contributor Author

arthsmn commented May 9, 2025

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.

@arthsmn arthsmn force-pushed the kitty branch 5 times, most recently from 19a365c to 4d81766 Compare May 10, 2025 02:10
@arthsmn
Copy link
Contributor Author

arthsmn commented May 10, 2025

@nezia1 @Lunarnovaa @llakala I've implemented the API suggested by Lunarnovaa.

Edit: added a simple test based on foot's one.

Copy link
Collaborator

@Lunarnovaa Lunarnovaa left a 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!

@arthsmn arthsmn force-pushed the kitty branch 6 times, most recently from a809fa6 to 551d04e Compare May 11, 2025 13:10
@arthsmn
Copy link
Contributor Author

arthsmn commented May 11, 2025

I decided to implement extraConfFiles as kittens from third-party can be used, and the user might want to create its owns to extend kitty.

Copy link
Contributor

@GetPsyched GetPsyched left a 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 .config/kitty/themes/ (also a special path -- auto-imported by kitty) is not an option here yet. EDIT: It's auto-imported by kitten, not kitty, meaning that the include directive is still required.

I'm gonna mess with these around in my module where I missed these special paths.

Comment on lines +113 to +118
// optionalAttrs (cfg.extraConfFiles != {}) (mapAttrs'
(
name: val:
nameValuePair ".config/kitty/${name}" {text = val;}
)
cfg.extraConfFiles);
Copy link
Contributor

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.

Copy link
Contributor

@GetPsyched GetPsyched May 25, 2025

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.

Copy link
Collaborator

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

Copy link
Contributor

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

@Lunarnovaa
Copy link
Collaborator

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 .config/kitty/themes/ (also a special path -- auto-imported by kitty) is not an option here yet. EDIT: It's auto-imported by kitten, not kitty, meaning that the include directive is still required.

I'm gonna mess with these around in my module where I missed these special paths.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants