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 name when hash is passed to panes config #548

Closed
wants to merge 8 commits into from

Conversation

LoveIsGrief
Copy link

@LoveIsGrief LoveIsGrief commented Aug 14, 2017

Motivation

tmux allows setting pane titles and with the set-option set-title on it is possible to pass these on to the terminal emulator e.g konsole, gnome-terminal, etc.

man tmux

When a pane is first created, its title is the hostname. A pane's title
can be set via the OSC title setting sequence, for example:

      $ printf '\033]2;My Title\033\\'

Example config

windows:
  - editor:
      pre:
        - echo "I get run in each pane, before each pane command!"
        -
      layout: main-vertical
      panes:
        - vim
        - #empty, will just run plain bash
        - top
        # The following pane will have the name of the hash/dict
        - pane_with_multiple_commands:
            - ssh server
            - echo "Hello"

Result

When calling display-message -p "#{pane_title}" in tmux, the pane title (or the window title if the pane title isn't set) is returned

Tests have been added and rubocop is ok with it too.

Extra

I was running on windows machine and had no way of testing, so I added a Vagrantfile with the bare minimum.

The tmux man says the pane title can be set via the "OSC title setting"

That's exactly what this does, just after the pane is created and before all other commands are sent
Constructor changed
Passed rubocop but broke the condition in a previous commit
It's fixed nao
@adamstrickland
Copy link
Contributor

OK, a couple thoughts:

  • Just looking at this, it's not really clear what some of the implications are... E.g. the newly-defined Pane#commands accessor method adds printf '\033]2;My Title\033\\' to the command array without checking the extant commands already there. Further, Pane#name is also overridden, its definition substantively changing. There is but one test for the former and none for the latter; DAE think that we need some more edge cases covered here?
  • Once we've decided what should happen, it's going to need to be documented in the README and the change added to the CHANGELOG (please see the contributing guide). This alone may alleviate my concerns WRT edge cases, so maybe start there?
  • While I don't have anything in particular against including a Vagrantfile in the project, that shouldn't be mixed in with a functional PR. I'd suggest creating a separate issue for lack of vagrant support and submit a PR with only those changes. Then re-submit this one independently, so the team can decide each on their own merits.

@LoveIsGrief
Copy link
Author

Hi @adamstrickland

This was indeed a quick and dirty (possibly too dirty) PR while on holiday.
Thanks for having a look. I'll see when there's time again to address all your concerns to possibly get this in.

Would a "flag" at the project or window level make this PR more acceptable (of course with all the other concerns addressed)? e.g

pane_titles: True  # Project level flag
windows:
  - editor:
     pane_titles: True # Window level flag

Cheers

@adamstrickland
Copy link
Contributor

adamstrickland commented Aug 15, 2017

@LoveIsGrief not sure I understand the purpose of a flag in this case... What would that be for? If it's to simply enable/disable the functionality, I don't think that's necessary.

I feel as though my comments were taken to be critical of the intent of the PR; far from it, I think this is a good change. But it's a change nonetheless, which means we have to make sure it's documented in the README and noted in the CHANGELOG (per the contribution guidelines). I do think that there's probably some edge cases that should be covered by tests, but this is a gut-feel more than anything else (@Soliah, @ethagnawl, @J3RN any opinions?). IMO it's just better to work through any issues before we merge it in than after.

As for the Vagrant stuff, again, I'm not against it, but it'd just be better to not mix it in with this functionality. I don't see us rejecting that (it's not very invasive), but personally I haven't used Vagrant in years, so someone with fresher knowledge in that area would be better-suited to take a look at that. Looks like you could simply create a branch with just a6e5862, fc1feb3, and e42416a and submit that as a PR, and then either push this without Vagrant support (probably best) or rebase it off the vagrant branch. While you're at it, you could probably also squash 92008a1 and 4016bcf, as well.

@ethagnawl
Copy link
Member

A few thoughts:

  • Since Pane#name is being overloaded, perhaps we should change title to name or vice versa?
  • A few tests which document the different paths Pane#name could take would be nice.
  • I think a helper method or some named variables would help make Pane#commands more readable, maintainable and extensible.
  • The introduction of a Vagrant file should be broken out into its own PR.

@ethagnawl
Copy link
Member

Thank you for this contribution @LoveIsGrief. I'm sorry that it sat dormant for so long.

However, I think a better (i.e. persistent) way forward would be to use a user provided variable and pane-border-format.

@ethagnawl ethagnawl closed this May 18, 2020
@LoveIsGrief
Copy link
Author

Hi.

Don't worry about it. I think it was a quick hack. It's so old now 😂

Have a great day

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