feat: tmux session attach mode #901
Conversation
|
Hi, sorry for the late reply! Would you mind telling me the difference between |
|
Thanks for the reply! Basically, here's how it works Here's a video that, hopefully, demonstrates that. out.mp4 |
I get it, thanks for the explanation and the demo video:) |
config.example.toml
Outdated
| # the newly crated tmux session, creating the session | ||
| # and only attaching to it if not inside tmux | ||
| # (default: create, allowed values: "create", "create_and_switch_client") | ||
| # tmux_session_attach_mode = "create" |
There was a problem hiding this comment.
I am thinking about the naming, what about we call them:
- AttachIfNotInASession
- AlwasyAttach
I think switch_client is too technical, thoughts on this?
There was a problem hiding this comment.
I changed some names, also dropping attach infix from tmux params, to not repeat it
src/config.rs
Outdated
| /// The preferred way to run the new tmux session | ||
| #[arg(long = "tmux-session-attach-mode")] | ||
| tmux_session_attach_mode: Option<TmuxSessionAttachMode>, |
There was a problem hiding this comment.
Is it necessary to add this CLI option, I think this feature is more like a static feature, one won't need to frequently change it 🤔
src/config.rs
Outdated
| @@ -967,6 +986,19 @@ impl Config { | |||
| .unwrap_or(false) | |||
| } | |||
|
|
|||
| /// The preferred way to run the new tmux session. | |||
| pub fn tmux_session_attach_mode(&self) -> TmuxSessionAttachMode { | |||
There was a problem hiding this comment.
We should make this function private as well if we want developers to use .tmux_config() function
src/steps/tmux.rs
Outdated
| // Only attach to the newly-created session if we're not currently in a tmux session. | ||
| TmuxSessionAttachMode::Create if is_inside_tmux => { | ||
| println!("Topgrade launched in a new tmux session"); | ||
| return Ok(()); | ||
| } | ||
| TmuxSessionAttachMode::CreateAndSwitchClient if is_inside_tmux => { | ||
| tmux.build().args(["switch-client", "-t", &session]).exec() | ||
| } | ||
| TmuxSessionAttachMode::Create | TmuxSessionAttachMode::CreateAndSwitchClient => { | ||
| tmux.build().args(["attach-session", "-t", &session]).exec() | ||
| } | ||
| }; |
There was a problem hiding this comment.
What about writing it in this way, would it be a little bit clearer?
TmuxSessionAttachModule::Create => {
if is_inside_tmux {
println!("Topgrade launched in a new tmux session");
return Ok(());
} else {
tmux.build().args(["attach-session", "-t", &session]).exec()
}
}
TmuxSessionAttachModule::CreateAndSwitchClient => {
if is_inside_tmux {
tmux.build().args(["switch-client", "-t", &session]).exec()
} else {
tmux.build().args(["attach-session", "-t", &session]).exec()
}
}There was a problem hiding this comment.
I can do it if it's easier, just felt bad to repeat attach-session call twice
There was a problem hiding this comment.
just felt bad to repeat attach-session call twice
Yeah, that's indeed one drawback. Though I prefer readability, which will make maintenance easier:)
What does this PR do
This will allow the user to choose between just creating a session when in tmux, or switching the client to the newly created session. It's useful for my workflow revolving around tmux, where having forced a separate session with no quick way to attach to it created friction
Standards checklist
CONTRIBUTING.mdFor new steps
--dry-runoption works with this step--yesoption works with this step if it is supported bythe underlying command
If you developed a feature or a bug fix for someone else and you do not have the
means to test it, please tag this person here.