feat(ui): Implement 'attach --create' so attach would never fail #588

ottok · 2021-06-27T18:57:07Z

Add support for --create in subcommand attach to enable similar use as in tmux new-session -A -s Foo where attaching a session will always result in a session, either re-using the old or creating a new.

This allows me to have a Zellij session open "forever", even across remote host reboots when the session otherwise dies. For example I can have while ! ssh host -t 'zellij attach --create workspace'; do echo $?; sleep 3; done and that window will forever be an open terminal to my remote machine with my previous session open as far as possible.

Closes: #541

ottok · 2021-06-27T18:59:31Z

PS. This is my first Rust project, so feel free to nitpick about all the things I am doing wrong so that I can learn.

imsnif · 2021-06-29T19:14:29Z

Hey @ottok - thanks for opening this PR, but unfortunately there's already one in the works for this: #587
I'd be very happy if you'd like to maybe pick up a different issue and work on it? We can make sure to mark that you're working on it so no one can take it away. Would also be happy to give it my personal attention and nitpick the code as much as you want :)

ottok · 2021-07-10T07:27:23Z

First-time contributors need a maintainer to approve running workflows.

Could you please approve workflows so I can see if this PR would have passed tests even if I would decide to discard it?

kunalmohan · 2021-07-10T07:40:34Z

Hi @ottok! Looks like the author of #587 will be busy for some time (#587 (comment)). So I'll review this and if this is ready before the other one, we can go ahead and merge this.

kunalmohan

Hey @ottok! Thank you for the PR. This mostly looks good. There are a couple of changes that need to be done to handle all the cases correctly and a few other suggestions regarding the code. And make sure to fix the fmt and clippy lint errors. Feel free to ping me if you need any help 🙂

kunalmohan · 2021-07-10T07:49:59Z

src/main.rs

+                if sessions.len() == 1 {
+                    // If a session name was omitted but there is only one session, attach it
+                    session_name = sessions.pop();
+                    println!("Attaching session {:?}", session_name);


I don't think we need the println!() statements.

I have them at least now while developing, otherwise I don't get any feedback from Zellij on what code path it took.

kunalmohan · 2021-07-10T07:50:12Z

src/main.rs

+                    // If a session name was omitted, but --create was used, attach to new session
+                    // with random name
+                    start_new_session = true;
+                    println!("Creating a new session to attach");


Same as above.

kunalmohan · 2021-07-10T07:52:15Z

src/main.rs

+                    // If a session name was given, but does not exist while --create was used,
+                    // attach a new session using that name
+                    start_new_session = true;
+                    println!("Creating new session {:?} to attach", session_name.clone().unwrap());


Same. We don't need this.

kunalmohan · 2021-07-10T07:59:27Z

src/main.rs

@@ -20,11 +20,39 @@ use zellij_utils::{
    structopt::StructOpt,
 };

+fn start_new_session_name(opts: &CliArgs) -> String {


We can have this function to accept just the opts.session.as_ref(). So the parameter would be session_name: Option<&String>.

But session_name might be empty. I'll think about something.

If the session_name is empty, the parameter session_name will be None. So, you can then operate on it the same way you do now:

fn start_new_session_name(name: Option<&String>) -> String { let session_name = name .map_or_else(|| names::Generator::default().next().unwrap(), |s| s.clone()); assert_session_ne(&session_name); return session_name; }

I tried to get this working in various ways, but always every avenue I took ended up in failures. I guess I don't yet fully master the Rust ownership and types. Could you please show me what exactly I should have in the calling function?

Please see kunalmohan@ec464a2 for the relevant changes. Feel free to reach out in case there's any confusion.

kunalmohan · 2021-07-10T08:08:40Z

src/sessions.rs

@@ -47,7 +47,17 @@ fn assert_socket(name: &str) -> bool {
    }
 }

-fn print_sessions(sessions: Vec<String>) {
+pub(crate) fn get_sessions() -> Vec<String> {


This function looks redundant to me now. We can simply unwrap() on the result returned by read_sessions() to get the same behaviour.

kunalmohan · 2021-07-10T08:11:30Z

src/main.rs

+                    // If a session name was omitted but there is only one session, attach it
+                    session_name = sessions.pop();
+                    println!("Attaching session {:?}", session_name);
+                } else if create {


This should be checked only if there are no active sessions. If there is only one active session, we attach to it. If there are more than one active sessions we exit with a message. If there are no active sessions, we create one if create flag was passed, otherwise exit.

Well this is how tmux does it to allow one to create a new fresh session if the previous one had died. I think this is useful.

If we wan't the <name> to be compulsory for zellij attach --create <name> then that should maybe be checked already in the command-line argument parsing phase.

I agree that we should allow users to create a fresh session if one does not exist. But this case here is the one where the user does not mention the session name. Since there is no session name, we will create a new session only if there are no active sessions. Otherwise, zellij attach --create will keep on creating a new session each time even though there are more than 1 active sessions and the user might want to attach to one of those.

IMO this is the behaviour that zellij should have with --create flag:

If session name is specified:

If a session with name <session_name> is present, attach to it.

If a session with name <session_name> does not exist, create a new session.

If no session name is specified:

If only a single session exists, attach to it.

If more than one session exists, exit with a message (and list the sessions).

If no session exists, create a new one with a random name

Do you have something else in mind?

Add support for --create in subcommand attach to enable similar use as in 'tmux new-session -A -s Foo' where attaching a session will always result in a session, either re-using the old or creating a new. Closes: zellij-org#541

ottok · 2021-07-18T19:04:02Z

Apparently the workflows don't run when I refresh the PR. Which I did to make sure CI passes. But now I can't know if it does for sure..

kunalmohan · 2021-08-01T07:20:55Z

Hi @ottok! Are you still working on this? This is an important feature and we would like to have it in main branch as soon as possible. In case you're busy, I'll be more than happy to take it off your hands :)

ottok · 2021-08-02T15:37:28Z

I will take a new look at it today. Did you have any follow-up comments to my responses? su 1. elok. 2021 klo 0.21 Kunal Mohan ***@***.***> kirjoitti:

…

Hi @ottok <https://github.com/ottok>! Are you still working on this? This is an important feature and we would like to have it in main branch as soon as possible. In case you're busy, I'll be more than happy to take it off your hands :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#588 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFDINC6QDPATQ3SZQ4UKLDT2TYWDANCNFSM47MVH73Q> .

kunalmohan · 2021-08-02T15:46:41Z

Did you have any follow-up comments to my responses?

I had replied to them in the inline comments themselves. Are there more concerns?

a-kenji · 2021-09-19T12:51:35Z

Thank you for the work on this feature!
It has now been implemented by #731.

jeffschwartz · 2024-02-29T01:57:43Z

Please update cli help to reflect new feature.

ottok · 2024-03-01T06:06:04Z

It is already in the cli:

± zellij attach --help
zellij-attach 
Attach to a session

USAGE:
    zellij attach [OPTIONS] [SESSION_NAME] [SUBCOMMAND]

ARGS:
    <SESSION_NAME>    Name of the session to attach to

OPTIONS:
    -c, --create                Create a session if one does not exist
    -f, --force-run-commands    If resurrecting a dead session, immediately run all its commands on
                                startup
    -h, --help                  Print help information
        --index <INDEX>         Number of the session index in the active sessions ordered creation
                                date

SUBCOMMANDS:
    help       Print this message or the help of the given subcommand(s)
    options    Change the behaviour of zellij

imsnif requested a review from kunalmohan June 29, 2021 12:54

kunalmohan suggested changes Jul 10, 2021

View reviewed changes

ottok force-pushed the feature/attach-create branch from e74ddee to 9ade2ce Compare July 18, 2021 19:02

WIP: Collapse 'if' to satisfy Clippy error

ac69395

a-kenji closed this Sep 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ui): Implement 'attach --create' so attach would never fail #588

feat(ui): Implement 'attach --create' so attach would never fail #588

ottok commented Jun 27, 2021

ottok commented Jun 27, 2021

imsnif commented Jun 29, 2021

ottok commented Jul 10, 2021

kunalmohan commented Jul 10, 2021

kunalmohan left a comment

kunalmohan Jul 10, 2021

ottok Jul 18, 2021

kunalmohan Jul 10, 2021

kunalmohan Jul 10, 2021

kunalmohan Jul 10, 2021

ottok Jul 18, 2021

kunalmohan Jul 19, 2021

ottok Aug 9, 2021

kunalmohan Aug 9, 2021

kunalmohan Jul 10, 2021

kunalmohan Jul 10, 2021

ottok Jul 18, 2021

kunalmohan Jul 19, 2021

ottok commented Jul 18, 2021

kunalmohan commented Aug 1, 2021

ottok commented Aug 2, 2021 via email

kunalmohan commented Aug 2, 2021

a-kenji commented Sep 19, 2021

jeffschwartz commented Feb 29, 2024

ottok commented Mar 1, 2024

feat(ui): Implement 'attach --create' so attach would never fail #588

feat(ui): Implement 'attach --create' so attach would never fail #588

Conversation

ottok commented Jun 27, 2021

ottok commented Jun 27, 2021

imsnif commented Jun 29, 2021

ottok commented Jul 10, 2021

kunalmohan commented Jul 10, 2021

kunalmohan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ottok commented Jul 18, 2021

kunalmohan commented Aug 1, 2021

ottok commented Aug 2, 2021 via email

kunalmohan commented Aug 2, 2021

a-kenji commented Sep 19, 2021

jeffschwartz commented Feb 29, 2024

ottok commented Mar 1, 2024