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

Set pane title if provided in config file #891

Merged
merged 10 commits into from
Feb 8, 2024
Merged

Set pane title if provided in config file #891

merged 10 commits into from
Feb 8, 2024

Conversation

augustelalande
Copy link
Contributor

Pane titles are a commonly desired feature in tmux sessions (e.g. https://stackoverflow.com/questions/47800955/how-to-set-pane-titles-with-tmuxinator), but not explicitly supported by tmuxinator (although a pane can be named).

To pass a title to tmux it is generally required to use a somewhat hacky method, and run the command:
printf '\033]2;%s\033\\' '<title>' (tmux select-pane -T <title> is also supported).

Since tmuxinator already provides the ability to define a pane title, I thought I would take it a step further and pass that title to tmux. As it stands right now, tmux would still need to be configured with the appropriate settings to display the title, e.g.:

set -g pane-border-format "#{pane_index} #{pane_title}"
set -g pane-border-status bottom

but at least the title will be passed directly from the tmuxinator config, to tmux. As a follow up to this PR, we could make the pane title configuration part of the tmuxinator config.

@ethagnawl
Copy link
Member

This is great, @augustelalande!

A few initial bits of feedback:

  • It'd be great to get some test coverage around these changes
  • Should template.rb detect incompatible versions of tmux and print a warning?
  • This will need to be documented in the README and it might also be worth adding to the default template
  • Please update the CHANGELOG 😸

Do you have any thoughts, @akofink?

Copy link
Contributor

@akofink akofink left a comment

Choose a reason for hiding this comment

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

The changes look good to me, and it seems to work as expected. Very cool improvement! 💯
I agree with all of @ethagnawl's points as well. It looks like only tmux 2.6 and above will support this, from here:

CHANGES FROM 2.5 TO 2.6, 05 October 2017

* Add select-pane -T to set pane title.

CHANGELOG.md Outdated Show resolved Hide resolved
@augustelalande
Copy link
Contributor Author

Thanks for the feedback guys. I think I've implemented all the changes, with two caveats

  • I check tmux version but don't print a warning. I'm not sure it should print a warning since right now setting the title won't do anything unless explicitly configured in tmux settings as stated above, so it would be weird to print a warning for a feature which is "disabled" by default.
  • For the same reason I didn't add it to the default template since it needs additional configuration to work.

My proposal would be once this gets merged, to add configuration options to the tmuxinator config, to

  • enable pane titles
  • configure pane title position
  • configure pane title format

Copy link
Contributor

@akofink akofink left a comment

Choose a reason for hiding this comment

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

Looks ready to merge to me. Nice work! The tests look great. 🙌

@akofink akofink added the ready label Feb 5, 2024
@ethagnawl
Copy link
Member

My one remaining bit of feedback is that the README should specify the minimum version of tmux required to make use of this functionality.

Otherwise, LGTM. Thanks again, @augustelalande!

@augustelalande
Copy link
Contributor Author

augustelalande commented Feb 5, 2024

My one remaining bit of feedback is that the README should specify the minimum version of tmux required to make use of this functionality.

Otherwise, LGTM. Thanks again, @augustelalande!

Done

@akofink akofink merged commit ba62c5d into tmuxinator:master Feb 8, 2024
79 checks passed
@akofink
Copy link
Contributor

akofink commented Feb 8, 2024

Thanks for this awesome improvement @augustelalande! 🦖

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

Successfully merging this pull request may close these issues.

None yet

3 participants