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

zellij: Add additional options for shell integration #6037

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Adda0
Copy link
Contributor

@Adda0 Adda0 commented Nov 3, 2024

Description

This PR adds additional options for integrating Zellij with shells, namely:

  1. autostarting Zellij on shell start.
  2. automatically attaching to an existing session on Zellij autostart if such session exists.
  3. automatically exiting the shell when the Zellij session is terminated.

The new Zellij configurations can look like this:

  programs.zellij = {
    enable = true;

    autostartOnShellStart = {
      enable = true;
      attachExistingSession = false;
      exitShellOnExit = true;
    };

    enableZshIntegration = true;
    enableFishIntegration = true;
    enableBashIntegration = true;
  };

Currently, the options are implemented such a single configuration, which can get applied to shells by using enable<Shell>Integration. This means that one cannot have one configuration for, Zsh, another for Bash, and completely different one for Fish.

  • Would separate configurations for each shell be useful? For example, use autostart in one shell, but load completions only in another?

The PR is supposed to supersede #5333 as it implements the same integration with Fish, albeit with different options. Closes #5333.

I tested the PR primarily on Zsh and Bash. Everything works for Bash and Zsh as expected.

However, Fish seems to be having issues. Namely, the completions are loaded, but for each shell instance, one must manually activate completions on zellij command by pressing Tab to allow for the functions (such as ze, zfr etc.) to register and the completions to load. Autostart for Fish does not work at all, as it seems that the interactiveShellInit does not write to the config file at all. Needs further investigation. Maybe a similar workaround as for Zsh is needed?
Fish integration works well, too.

I need to perform some more testing on Fish shell. When this is completed, I will open the PR for merging. Nevertheless, I ask for a review for the PR now, especially regarding the possibility of allowing different configurations for each shell (basically just having autostartOn<Shell>Start module for each shell instead of a shared autostartOnShellStart). But then the enable<Shell>Integration seems kinda pointless.

Checklist

  • Change is backwards compatible.

  • Code formatted with ./format.

  • Code tested through nix-shell --pure tests -A run.all or nix develop --ignore-environment .#all using Flakes.

  • Test cases updated/added. See example.

  • Commit messages are formatted like

    {component}: {description}
    
    {long description}
    

    See CONTRIBUTING for more information and recent commit messages for examples.

  • If this PR adds a new module

    • Added myself as module maintainer. See example.

Maintainer CC

@mainrs

@Adda0
Copy link
Contributor Author

Adda0 commented Nov 6, 2024

The Fish integration seems to work well. I just forgot to enable Fish shell in Home Manager during my testing. Therefore, I am opening the PR from its draft state. The remaining question is whether to enable shell-specific autostart options, or not (see above).

@Adda0 Adda0 marked this pull request as ready for review November 6, 2024 17:09
Dietr1ch added a commit to Dietr1ch/home-manager that referenced this pull request Nov 14, 2024
@Dietr1ch
Copy link

Dietr1ch commented Nov 14, 2024

EDIT: No issues. I just fat-fingered a typo on a tuple name while pasting :/

I was interested in this and merged it into my testing branch1 (after having successfully built my config from it).

It works fine, it'd be nice to get it merged.


Footnotes

  1. https://github.com/Dietr1ch/home-manager/tree/test

@Adda0
Copy link
Contributor Author

Adda0 commented Nov 14, 2024

Aha. I remember that I encountered this issue when implementing this and have thought I already fixed it. Glad I am not crazy, looking at the code now and thinking that this is already resolved. Thanks for the testing :)

@Adda0 Adda0 mentioned this pull request Nov 14, 2024
5 tasks
@Dietr1ch
Copy link

BTW, it seems this lets you shoot you in the foot because of a zellij bug, but there's nothing to change over here.

zellij-org/zellij#3773

Copy link
Contributor

@mainrs mainrs left a comment

Choose a reason for hiding this comment

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

Besides that this looks good to me. Tests are also included. I think for the whole YAML to KDL issue it might be better to create a final PR that includes all the KDL code instead of having multiples like right now.

Some of them do more than others, some are already superseeded. Would be great if you could help out on that front! I don't have access to a nix machine that easily right now, so it's hard to test all the stuff.

'' else
"")));

programs.bash.initExtra = mkIf (cfg.enableBashIntegration) (mkOrder 200
Copy link
Contributor

Choose a reason for hiding this comment

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

Why exactly is the mkOrder 200 required?

Copy link
Contributor Author

@Adda0 Adda0 Nov 14, 2024

Choose a reason for hiding this comment

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

It is not required. I used the template used in #5333. Some packages in hm use this to place the generated completions/aliases a bit earlier in the config files for each respective shell so that the generated options can be easily modified by manual configuration or other programs. However, it is not necessary at all and I can remove them.

@Adda0
Copy link
Contributor Author

Adda0 commented Nov 14, 2024

What about whether to support per-shell autostart options, i.e. using autostartOn<Shell>Start submodules instead of the generic autostartOnShellStart option? Any thoughts?

I think for the whole YAML to KDL issue it might be better to create a final PR that includes all the KDL code instead of having multiples like right now.

I agree that it might be necessary. I intend to work on this. First, I will need to investigate each PR, and then I will see what makes the most sense to me. I would welcome any and all suggestions.

@mainrs
Copy link
Contributor

mainrs commented Nov 14, 2024

What about whether to support per-shell autostart options, i.e. using autostartOn<Shell>Start submodules instead of the generic autostartOnShellStart option? Any thoughts?

I'm not sure what the best approach would be. Maybe rycee can chime in before merging the PR!

@Adda0
Copy link
Contributor Author

Adda0 commented Nov 17, 2024

I was made aware of the fact that shell completions are already loaded in the nixpkgs zellij package and should never be an option in home manager modules. The issue with the accompanying aliases is that they are not loaded until a first Zellij autocomplete is executed. It is an issue with the upstream Zellij. Therefore, I will perform the following steps:

  1. remove the option to enable autocompletion (which also includes the aliases mentioned above) from this PR.
  2. create a nixpkgs PR to temporary fix the inclusion of the aliases in the nixpkgs package zellij, as per Cannot use zr or zrf without autocompleting zellij command once zellij-org/zellij#1933 (comment), and
  3. convert this PR into a draft until this issue is resolved.

@Adda0 Adda0 marked this pull request as draft November 17, 2024 17:26
@KamWithK
Copy link

Would be cool to add Nushell as another shell option

@Dietr1ch
Copy link

Would be cool to add Nushell as another shell option

You might want upstream support from Zellij first,

zellij setup --generate-auto-start nushell
zellij setup --generate-auto-start nu
Unsupported shell: nushell
Unsupported shell: nu

@KamWithK
Copy link

Would be cool to add Nushell as another shell option

You might want upstream support from Zellij first,

zellij setup --generate-auto-start nushell
zellij setup --generate-auto-start nu
Unsupported shell: nushell
Unsupported shell: nu

Ah I'll look into that then!

@KamWithK
Copy link

Would be cool to add Nushell as another shell option

You might want upstream support from Zellij first,

zellij setup --generate-auto-start nushell
zellij setup --generate-auto-start nu
Unsupported shell: nushell
Unsupported shell: nu

Looks like there is currently a PR for it awaiting review:
zellij-org/zellij#3206

@Adda0
Copy link
Contributor Author

Adda0 commented Jan 5, 2025

  1. remove the option to enable autocompletion (which also includes the aliases mentioned above) from this PR.

Done. I consider this first PR finished and ready for reviews (and if deemed suitable, merging). I think it would be wise to first merge this PR, and only in a next PR try to fix the completions. The initial experiment to fix this issue with the zellij package in nixpkgs failed.

The question of per-shell autostart options still remains open. Currently, only a single configuration for all shells is possible.

@Adda0 Adda0 marked this pull request as ready for review January 5, 2025 19:12
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.

4 participants