Makes the `destroy-unattached` behavior of tmux optional #544

Closed
wants to merge 3 commits into from

6 participants

@xcambar

As per #543, this PR adds the option to destroy the tmux sessions.

@sorin-ionescu

Please update the README as well.

I'll let @ColinHebert decide.

@xcambar

@sorin-ionescu Done, and squashed.

@ColinHebert, I notice the default behaviour of destroy-unattached changes in this PR. Is it ok for you, or do you want it back to what it was ?

@ColinHebert

I'm not sure I follow why you want to do that.

If you have the global option destroy-unattached set to off beforehand, it might be a better solution to

  • Check the status of destroy-unattached before the auto-start
  • Do the auto-start
  • Set destroy-unattached to the value we had before

This way you don't have to set more options in your prezto conf

@xcambar

The pain point of the current tmux module is that it enforces a certain configuration of tmux (which fits most of the users, I guess). Making destroy-unattached a configuration option makes it clear to everyone that 1) "YMMV" and 2) "here's the current behaviour".

I thought this PR would be a good fit because as a framework, prezto must have sensible defaults (about that, see my previous question in my latest comment) while allowing for customization.

I wanted to avoid other prezto+tmux users to have to jump off prezto to customize tmux to their needs.

@sorin-ionescu
@ColinHebert

@xcambar Sure, I totally get that, this options is probably something that we should leave to the user.
Did you consider the option I mention in my previous message (doing a tmux show-options -g destroy-unattached beforehand)? Do you reckon it could be a good way to deal with that?

@fuadsaud

So let me see if I understand this correctly:

this patch will make possible for me to have auto-start, but when I detach from the "main" session it will not kill my underlying shell session? Then I'll be able to maintain multiple tmux sessions? If yes, that sounds good. I'm not sure if it's good default behaviour, as it may feel confusing for some, but definitely a good option.

@xcambar

@ColinHebert OK, I think I get your point now (I'm kinda slow, sometimes ;). In short:

tmux should be configured in tmux.conf.
prezto's tmux module should not configure tmux but provide a better experience,
with helpers, auto-start and so on...

I agree with that. Following the current behaviour and @fuadsaud comment, the default behaviour (when nothing is set about unattached sessions in tmux.conf) is to have destroy-unattached to on, except for the session #Prezto.

Unless some points need to be clarified, I'll update the PR accordingly.

@xcambar

@ColinHebert what do you think?

@sorin-ionescu

What's the status of this?

@sorin-ionescu sorin-ionescu commented on an outdated diff Feb 18, 2014
modules/tmux/README.md
@@ -23,9 +23,14 @@ following line to *zpreztorc*:
In both cases, it will create a background session named _#Prezto_ and attach
every new shell to it.
-To avoid keeping open sessions, this module sets `destroy-unattached off` on
-the background session and `destroy-unattached on` on every other session
-(global setting).
+To control if the unattached sessions must be kept alive, set the
@sorin-ionescu
Owner

It does not inform when the unattached session must be kept alive and when it should not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@xcambar

@sorin-ionescu Does that read better?

@sorin-ionescu sorin-ionescu and 1 other commented on an outdated diff Feb 18, 2014
modules/tmux/init.zsh
- # Enable the destruction of unattached sessions globally to prevent
- # an abundance of open, detached sessions.
- tmux set-option -g destroy-unattached on &> /dev/null
+ #By default, prezto sets destroy-unattached to on
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@sorin-ionescu

It's a bit clearer; nevertheless, this auto-start feature is a mess.

@xcambar

As a tmux user, I use it each and every day and sincerely, it's really cool!

The hard part is in the messaging, I guess.

There may be some misguidance too for the users because

  • the Prezto config has a lesser priority than the tmux config (regarding destroy-unattached).
  • It's a 2-step process: 1) zsh/prezto starts, which starts tmux. 2) Tmux starts an instance of zsh/prezto (which itself doesn't perform any tmux-related actions because of $TMUX being set)

Inception style

@sorin-ionescu

What I am trying to understand, is why do we have to mess with destroy-unattached?

@xcambar

The following script is an attempt to demonstrate the usage of destroy-unattached:

Open a terminal, you'll end up in the #Prezto session.
Create a new session in tmux (<Prefix> : new-session -s mySession).
Create another session in tmux (<Prefix> : new-session -s mySession2).

List your sessions (<Prefix> : choose-tree)

If destroy-unattached is on, mySession has been destroyed.
Otherwise, you can still get back to it and continue to work.

We have to mess with it because as a configuration framework, we want to provide sensible defaults, yet not (of course) overriding tmux config.
Moreover, where the auto-start feature shines (IMO) is in the ability to switch between various sessions, forcing us as contributors of this module to deal with this tmux configuration option.

@sorin-ionescu sorin-ionescu and 1 other commented on an outdated diff Feb 20, 2014
modules/tmux/init.zsh
- # Enable the destruction of unattached sessions globally to prevent
- # an abundance of open, detached sessions.
- tmux set-option -g destroy-unattached on &> /dev/null
+ #By default, prezto sets destroy-unattached to on
+ tmux_destroy_unattached=`tmux show-options -v -g destroy-unattached`
+ if [[ -z "$tmux_destroy_unattached" ]]; then
@sorin-ionescu
Owner

I'm not sure that this check is good. It may say "off" instead of being empty. Maybe, if [[ "$(tmux show-options -g -v destroy-unattached)" == 'off' ]]?

@xcambar
xcambar added a note Feb 21, 2014

I think it fits like it is. It reads as follows:

If the `destroy-unattached` option has no value (empty check), we set the Prezto defaults (ie, on).
Otherwise, don't touch it.
@sorin-ionescu
Owner
@xcambar
xcambar added a note Feb 21, 2014

I've just had a look into tmux's code, and the option defaults to off:
http://sourceforge.net/p/tmux/tmux-code/ci/master/tree/options-table.c#l138

So the -z check in the module is irrelevant as there's always a value, set either by the user or tmux itself. (It's very nice of tmux to make the default explicit for us, by the way!)

The code of the module must take this into account, I'll update accordingly.

Thanks for pointing that out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@xcambar

Following up on the default tmux value for destroy-unattached,
it appears there's no default to be set from within Prezto, so it will always be the responsibility of the tmux user to accept/change tmux's behaviour.

Updating the code accordingly will make the code of the module more simple, BUT if you agree on it, it will come at the cost of backward-compatibility for this particular module of Prezto (currently forcing the value to on).

Is it acceptable to you @sorin-ionescu @ColinHebert ?

My view on this is very simple: Tmux should be configured the tmux-way. Prezto should only provide sugar on top of it.

But in the end, it's up to you guys.

@sorin-ionescu

Backwards-compatibility is not an issue. Prezto is not post-v1.0. My goal for Prezto has always been to limit the number of issues opened; that tells me that I'm doing something right.

@xcambar xcambar closed this Feb 23, 2014
@xcambar xcambar reopened this Feb 23, 2014
@xcambar

Any update for this?

@sorin-ionescu
@nasenatmer

How would I go about having my own session setup (which is defined in tmux.conf, but which is overridden by the auto-start feature) while still being able to use the auto-start feature?

This is my setup (tmux.conf):

  new "ncmpcpp"
  splitw -h
  resize-pane -x 109
  resize-pane -R
  select-pane -t :.+
  splitw -v -p 60  

This gets me a window with 3 panes of which the top left has ncmpcpp running in it. If I want to use auto-start, what's the best way to keep this setup? Should I add it to tmux module?

@xcambar

Are you using the code from this pull-request?
It is definitively not supposed to kill your custom sessions.

The current master of prezto's tmux module kills all existing unattached sessions because of the forced settings of destroy-unattached, which id the main reason of this PR and discussion 😄

Can you confirm which version of prezto and tmux module you're using, please?

@nasenatmer

Hah, I read the whole issue here, but I wasn't aware that it is about the thing I'm asking for here! I'm on master with the last commit 7722dd4, so up to date with no modifications to the tmux module itself or so.

@sorin-ionescu sorin-ionescu added Revision and removed Enhancement labels Feb 26, 2014
@xcambar

If you ever feel like trying this PR, kill your tmux server (to start with a blank slate) and give us feedback... ;)

@nasenatmer

How can I pull your changes into my repository?

@xcambar

@nasenatmer I'm happy to help, but that would be irrelevant to this thread. Send me an email, I'll answer shortly. (you can find it in my github profile page, btw)

@sorin-ionescu

Interestingly, a tmux server can be started as a service.

@sorin-ionescu sorin-ionescu added Fix and removed Revision labels Mar 7, 2014
@sorin-ionescu

Merged via 478653f.

@nasenatmer

Alright, so I gfred 478653f, activated the tmux module and set local auto-start to yes, and have two active sessions in any new terminal I open, with tmux being attached to the prezto session. How is it possible to switch session or to tell tmux to connect to my custom session instead of prezo default? I guess I'll have to edit init.zsh to do that?

@sorin-ionescu
@nasenatmer

Right, and if this is the first invocation of tmux on a freshly started machine it will default to the prezto session as this one's created later than the one set in tmux.conf

@sorin-ionescu

The latest code seems to not create the prezto session if one already exists.

@nasenatmer

Well, L27 will always create an additional session in the current version of init.zsh since:

jakob ❯ tmux has-session                                                                                                                                                                                           ~
failed to connect to server: Connection refused
[1]    7595 exit 1     tmux has-session
jakob ❯ print $?~
1

If you have a session defined in tmux.conf, it will be created as lately as L28-L29.

I don't know, however, how to prevent this. One way could be to introduce an additional zstyle to define a default session that tmux attaches to when starting. But IIRC, you're not a big fan of more zstyles to be included in prezto.

@sorin-ionescu

Yes, I could add another zstyle to name the session. Will that work though? You could also start a tmux server at login as a service.

Personally, I think auto-start has been a pain. It seems, multiplexers ought to be used with session managers, of which, there are a few. No session should be defined in tmux.conf nor Prezto.

@xcambar xcambar deleted the xcambar:feature/optional-tmux-destroy-unattached branch Mar 11, 2014
@xcambar

On a fresh server, and with sessions defined in tmux.conf (set's say s1 and s2), you will end up with s1, s2 and prezto.

In any other case, prezto will attach to he latest session used as described by @sorin-ionescu.

What we could do is split https://github.com/sorin-ionescu/prezto/blob/master/modules/tmux/init.zsh#L28-L31 in to two separate calls, like:

  tmux start-server
  if ! tmux has-session 2> /dev/null; then
    tmux \
      start-server \; \
      new-session -d -s "$tmux_session" \; \
      set-option -t "$tmux_session" destroy-unattached off &> /dev/null
  fi

The first line creates your tmux.conf-defined sessions, the if fallbacks to creating the prezto session if none are found (making sure the server remains alive).

It could fit all the configs at the expense of one tmux start-server call (not that expensive, really ;)

@xcambar

@nasenatmer Care to give the above snippet a try?

@nasenatmer

@xcambar : I tried this and it seems to work like a charm! Wouldn't it even be possible to leave out the second start-server \; \? That would look like this then (also works here…)

  tmux start-server
  if ! tmux has-session 2> /dev/null; then
    tmux \
      new-session -d -s "$tmux_session" \; \
      set-option -t "$tmux_session" destroy-unattached off &> /dev/null
  fi
@xcambar

By design, tmux shuts down the server if no sessions are attached.

Removing the second start-server works for you because your tmux.conf starts sessions.
Otherwise, start-server is mandatory.

@xcambar xcambar added a commit to xcambar/prezto that referenced this pull request Mar 12, 2014
@xcambar xcambar refs #544 Only creates default session if none created in tmux.conf def720f
@xcambar

Sounds like a nice small improvement to me.

@milkypostman

This should not attach to session by default or should be configurable, see #581

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment