feat: add kill-session command

[jaeheonji]

Fix #644

Hi, everyone 👋
I simply added a command to close the session.

The purpose of this feature is to close a session via a command inside or outside the session.

In order to give the same experience as tmux as much as possible, I implemented it referring to the commands of tmux like below.

• kill-session [-a] [-t target-session]

By the way, I'm a Rust newbie and I need a lot of feedback from community!

Anyone feel free to give feedback 😸

[imsnif]

Hey, just wanted to apologize for not getting to this yet! I plan on taking a look tomorrow.

[imsnif]

Hey @jaeheonji - first of all, I'm really sorry for taking so long to get to this (and not getting to this when I promised). It's been a week for me.

Second - I'm happy to see this being picked up. We just had a request for this on Discord.

So, before I look at the code - I tried this out and it isn't working for me. Though maybe I'm doing something wrong?

What I'm doing:

1. cargo make run
2. In a different terminal window, target/debug/zellij kill-session -t <session_name>
3. Nothing happens

I also tried with target/debug/zellij kill-session -a... am I missing something?

Also, if I just do cargo make run -- kill-session I get a hard-crash (panicked on unwrapping a None).

[jaeheonji]

Thanks for checking @imsnif

Hm.. interesting.. Actually, when I tested it before merging upstream(90b9ab9), it worked fine. However, as you pointed out, now that I check it, there seems to be something wrong.

It is dawn where I am now. I'll get up tomorrow morning and check it out 👍

[imsnif]

Aha! I think I know why this happened. I recently added a "mirrored sessions" feature to allow users to connect to the same session from different terminal windows. With this feature the behaviour of the QUIT action changed (it only quits if you're the last user connected). So you might want to add a KILL_SESSION action and use it instead (or solve it in some other way that you think is better).

Have a good sleep meanwhile :)

[jaeheonji]

There was such a story! thank you :)

[jaeheonji]

I simply added KillSession Instruction 😄

[imsnif]

Hey, thanks for picking up on my changes! This looks great. I have a few functionality comments and one minor comment on the code itself (left in the code).

Functionality:

1. Running kill-session without anything (eg. zellij kill-session) results in a panic. Maybe we can show an informative error? I think this is something we can achieve with our cli parser.

2. Running kill-session -t <non-existent-session> gives a non-informative error... can we show something nicer here?

3. It's possible to run the command with both -a and -t - which works, but is a little non-explicit. Do you think we can make them mutually exclusive?

[jaeheonji]

@imsnif thanks!

Good! I totally agreed your opinion of Functionality 1, 2. There should be additional information for users.

But, we need to think about Functionality 3 and your comment of #745 (comment).

First, I implemented the kill-session in the same way as possible with tmux. The following is the man page of tmux related to kill-session.

kill-session [-aC] [-t target-session]
             Destroy the given session, closing any windows linked to it
             and no other sessions, and detaching all clients attached
             to it.  If -a is given, all sessions but the specified one
             is killed.  The -C flag clears alerts (bell, activity, or
             silence) in all windows linked to the session.

The -t option kill the target session. the -a flags kill all session except one (maybe last created) and If -a and -t are used together, all killed but a specific session.

Yes I know.. This looks pretty confusing and weird. Perhaps this is why we make a better terminal multiplexer with Zellij. (like modern unix tools)

From here on, this is my personal opinion. As you commented, -a and -t seem to be functionally separated like below.

• zellij kill-session [target-session]
  • alias k
• zellij kill-all-session
  • alias ka (?)

Personally, I think it's simpler and more intuitive, but I'm wonder about other people's opinions. 🤔

[imsnif]

I totally agree with your opinion! Let's do that. Only nitpick is that I think you accidentally dropped the s from the end of kill-all-sessions :)

[jaeheonji]

There is one concern I have added during development.

If the user unintentionally uses kill-all-sessions, the state of all sessions is lost. so I added the interactive question when executing it.

The code can be weird because it's simple to implement without using additional libraries 😭

If you think it's too functionally overkill, please leave a comment.

EDIT:

[imsnif]

chore: adjust clippy warning

You are fast! :D

[imsnif]

This looks great! I love the added warning :)

I fixed some minor issue with the wording and will merge once the CI passes. Thanks for this! Hope to see more contributions for you in the future. Feel free to drop by our Discord if you'd like

[jaeheonji]

I am so happy to be able to contribute to a project I love. thank you! 😄

[HKalbasi]

Just used this feature. Thanks!