Skip to content
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

feat(home-manager): add support for ghostty #446

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

Conversation

natecox
Copy link

@natecox natecox commented Jan 3, 2025

Fixes #427

pkgs/sources.json Outdated Show resolved Hide resolved
modules/home-manager/ghostty.nix Show resolved Hide resolved
modules/home-manager/ghostty.nix Outdated Show resolved Hide resolved
modules/home-manager/ghostty.nix Show resolved Hide resolved
modules/home-manager/ghostty.nix Outdated Show resolved Hide resolved
@natecox natecox closed this Jan 3, 2025
@zakuciael
Copy link

Hi, is there any reason this PR was closed? Or if the author is no longer interested in implementing it, is it okay to make a new PR adding this module?

@natecox
Copy link
Author

natecox commented Jan 4, 2025

Hi, is there any reason this PR was closed? Or if the author is no longer interested in implementing it, is it okay to make a new PR adding this module?

Seemed like I was getting the runaround a bit on this without any real conversation happening to tell me why and I lost interest.

I’d be happy to pick this back up if a clear direction can be provided, I’m just not sure what to do with two reviewers asking for conflicting changes.

No hard feelings or anything, because text tends to present negatively.

@getchoo
Copy link
Member

getchoo commented Jan 4, 2025

We're sorry about that, genuinely

There have been a lot of large refactors recently, and its made us change the approach we use in applying ports to different programs. I'll admit I'm the one who pushed a lot of these changes, and I'm more familiar with the effects they have than @isabelroses. I should have communicated these better, as well as some of the new things to look out for; that's my bad

The issue that caused this difference in review -- Import from Derivation -- is one that has been a pretty hot topic recently, and it's no doubt because of this. I plan on writing an explainer in our docs so others aren't blind sided by this again, as it's not exactly easy to catch

I'd love to see this continued, and I'll make sure to answer any questions along the way

@natecox natecox reopened this Jan 4, 2025
@natecox
Copy link
Author

natecox commented Jan 4, 2025

All right, well I appreciate the context. Can you point out a port that is up to date that I can reference? I started with copying the kitty config (thinking it would make the port simple lol).

@natecox
Copy link
Author

natecox commented Jan 4, 2025

@getchoo I think I've resolved all the requests above.

Copy link
Member

@getchoo getchoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise this LGTM. I'll make sure to test it out soon since I know this is a popular app and port ;)

let
inherit (config.catppuccin) sources;

cfg = config.catppuccin.ghostty;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
cfg = config.catppuccin.ghostty;
cfg = config.catppuccin.ghostty;
enable = cfg.enable && programs.ghostty.enable;

Since we're also placing a file in ~/.config with xdg.configFile, we should make sure that is only done when Ghostty itself is enabled (just like the values of programs.ghostty.settings only being written to its config file when its enabled)

{
options.catppuccin.ghostty = catppuccinLib.mkCatppuccinOption { name = "ghostty"; };

config = lib.mkIf cfg.enable {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
config = lib.mkIf cfg.enable {
config = lib.mkIf enable {

And the way we usually do that is just by guarding all of our changes behind that condition

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to confirm this one, because I actually had that implemented as you suggest and was told it was unnecessary here: #446 (comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At that revision the comment was correct due to the module only touching programs.ghostty options

Those are already only applied when programs.ghostty.enable is true. When we touch options outside of it (in this case xdg.configFile) though, we need to implement that same guard internally (else we may end up placing port files on people's systems for programs they're not actually using)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okie dokie.

@getchoo
Copy link
Member

getchoo commented Jan 5, 2025

Can you point out a port that is up to date that I can reference?

Honestly, I think this is already a great example of how we want these modules to be handled now 😆

But for the future, some of my favorites are

With k9s and btop being the most similar to this one

The general idea is to place our port sources at the right path (or list exactly where they are) so that the program can read that at runtime. This is opposed to us reading these files at evaluation time in the modules to set options like programs.someThemeableApp.settings.hexColor = "#FFFFFF"; -- as even though Nix totally can do that, it causes some small but nasty issues

@getchoo getchoo added this to the 2.0.0 milestone Jan 5, 2025
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.

Add support for Ghostty
5 participants