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

Add new-window command functionality #145

Merged
merged 21 commits into from May 16, 2016

Conversation

@ikirudennis
Copy link
Contributor

@ikirudennis ikirudennis commented May 12, 2016

See issue #142

@coveralls
Copy link

@coveralls coveralls commented May 12, 2016

Coverage Status

Coverage decreased (-0.01%) to 63.794% when pulling f4aef07 on ikirudennis:new-window-command into f0e5600 on tony:master.

@coveralls
Copy link

@coveralls coveralls commented May 12, 2016

Coverage Status

Coverage decreased (-0.01%) to 63.794% when pulling 8a3cff8 on ikirudennis:new-window-command into f0e5600 on tony:master.

@ikirudennis
Copy link
Contributor Author

@ikirudennis ikirudennis commented May 12, 2016

I tried to see if I could figure out how to add a test for it, but it didn't seem to work. :\

@@ -124,7 +124,8 @@ def new_window(self,
window_name=None,
start_directory=None,
attach=True,
window_index=''):
window_index='',
window_command=None):
"""Return :class:`Window` from ``$ tmux new-window``.

This comment has been minimized.

@tony

tony May 13, 2016
Member

Can you add the docstrings (:param window_index: description, so on) for these params?

@kmactavish
Copy link
Contributor

@kmactavish kmactavish commented May 13, 2016

The code you added will only run the command once on building the workspace, after that, any new panes/windows won't be run with this command. This functionality was already available though shell_command_before.

Maybe you want to add tmux key bindings? That will let any new windows/panes you open run a given command when they open.

In order to have the whole session live inside a virtual environment, including any new windows, you can use tmuxp's before_script.

@tony
Copy link
Member

@tony tony commented May 13, 2016

The code you added will only run the command once on building the workspace, after that, any new panes/windows won't be run with this command.

Can you clarify who / what this refers to? Just double checking

@kmactavish giving this and #146 a look tomorrow/sat. You're on a roll!

@ikirudennis
Copy link
Contributor Author

@ikirudennis ikirudennis commented May 13, 2016

I understand what you're saying, but this code is the functionality I'm looking for. I have a configuration where there are some windows which should load the virtual environment, and others which don't. (In my case, it's a directory for a static_files system separate from the virtual environment's project directory.) Adding a before_script for this would be unnecessary. To me, it just seemed like an oversight to cover all the other functionality of the new-window command but leave out the ability to specify a command to run in it.

@coveralls
Copy link

@coveralls coveralls commented May 13, 2016

Coverage Status

Coverage decreased (-0.01%) to 63.794% when pulling b438c3b on ikirudennis:new-window-command into f0e5600 on tony:master.

@kmactavish
Copy link
Contributor

@kmactavish kmactavish commented May 13, 2016

Hmm, I see. Did you want the functionality for the API, or for building a session? I think it makes sense for the API, but maybe not the WorkspaceBuilder.

Right now, in the WorkspaceBuilder, the window_command is the default (your shell), which then has the shell_command from the first pane sent to it via send-keys. So the effective window_command is just the first pane's shell_command running inside of bash.

I see three potential problems for using a window_command in the config file:

  1. What would happen in the WorkspaceBuilder if window_command was ls, or something very transient, and the window closed before the other panes could be created? They might get created in the previous window?
  2. It would be confusing that the first pane's shell_command doesn't get run; rather, the keys get sent to whatever window_command is---I expect you'll see text appearing if window_command is an editor, and the first pane has a shell_command.
  3. New users likely won't be aware that when the window_command ends, that pane/window dissapears, they would expect it to work like the pane's shell_command and drop back to a shell (which it won't): try running tmux new-window "ls; sleep 1" inside a session.

@tony P.S. that comment was me missunderstanding what @ikirudennis was trying to accomplish, which I may still be doing 😉 .

@kmactavish
Copy link
Contributor

@kmactavish kmactavish commented May 13, 2016

BTW, what is the window_command you want to run? If it's loading the virtualenv, it seems well suited to the window's shell_command_before, which just gets prepended to the shell_command of all its children panes (only at the window level, unlike before_script).

@ikirudennis
Copy link
Contributor Author

@ikirudennis ikirudennis commented May 13, 2016

Yes, window_command could be confusing for the uninitiated. However, as you point out, it is part of tmux functionality. And I can understand how it might be troublesome explaining to a user that window_command may have unintended consequences, and that what might appear to be a bug with tmuxp is actually everything working correctly. Perhaps we can put our heads together on explaining how it works in documentation and any helpful error messages returned to the user. If it's any consolation, I feel the tmux documentation on new-window command is kind of thin.

I did a test with a command which exits quickly, and yes it produces an error.

In my setup, I'm loading a virtualenvironment via pew. Pew works a little differently in that it starts a new shell instead of injecting the virtualenv into an existing shell. The reason I dislike shell_command_before or any of the other options I've tried is because it leaves a shell open after I exit the environment. To me, new-window -n name 'pew workon venv_name' is just a cleaner way to start the environment, and I know that when I exit it, I intend to exit the entire shell. It's how I was doing things before I found tmuxp, and it just seemed like a missing feature. But I hope I'm not being troublesome like "longtimeuser4" in XKCD: Workflow. 😄

@kmactavish
Copy link
Contributor

@kmactavish kmactavish commented May 13, 2016

Gotcha. You've won me over, and I like the fact that it's an undocumented feature---there is no example for it, so it won't be confusing for new users. Since people will only find it if they look at the unit test or in workspace builder, maybe comment in both those places its intended use and perils 😉 .

@kmactavish
Copy link
Contributor

@kmactavish kmactavish commented May 13, 2016

P.S. window_shell might be more accurate than window_command.

ikirudennis added 2 commits May 13, 2016
I tried to fix the test, but it seemed to only break things on my computer.
Though I'm not sure if this is the right fix.
@coveralls
Copy link

@coveralls coveralls commented May 14, 2016

Coverage Status

Coverage increased (+0.2%) to 63.992% when pulling 2079cd1 on ikirudennis:new-window-command into f0e5600 on tony:master.

for w, wconf in builder.iter_create_windows(s):
if 'window_shell' in wconf:
self.assertEqual(wconf['window_shell'], text_type('test_command'))

This comment has been minimized.

@tony

tony May 16, 2016
Member

These are intended to be integration tests in the truest sense of the word. See .travis.yml.

We have a minimum tmux version we officially support (though we can bump it of course). I'd like to be sure that this behavior would work on older versions.

Try something like this:

diff --git a/tmuxp/testsuite/workspacebuilder.py b/tmuxp/testsuite/workspacebuilder.py
index 253834e..a8b79dc 100644
--- a/tmuxp/testsuite/workspacebuilder.py
+++ b/tmuxp/testsuite/workspacebuilder.py
@@ -266,7 +266,7 @@ class WindowOptions(TmuxTestCase):
           - pane
           - pane
           window_name: editor
-          window_shell: test_command
+          window_shell: top
         """
         s = self.session
         sconfig = kaptan.Kaptan(handler='yaml')
@@ -277,7 +277,14 @@ class WindowOptions(TmuxTestCase):

         for w, wconf in builder.iter_create_windows(s):
             if 'window_shell' in wconf:
-                self.assertEqual(wconf['window_shell'], text_type('test_command'))
+                self.assertEqual(wconf['window_shell'], text_type('top'))
+            for i in range(10):
+                self.session.server._update_windows()
+                if w['window_name'] != 'top':
+                    break
+                time.sleep(.2)
+
+            self.assertNotEqual(w.get('window_name'), text_type('top'))


 class EnvironmentVariables(TmuxTestCase):
@@ -305,7 +312,7 @@ class EnvironmentVariables(TmuxTestCase):

         self.assertEqual('BAR', self.session.show_environment('FOO'))
         self.assertEqual('/tmp', self.session.show_environment('PATH'))
-        
+
 class WindowAutomaticRename(TmuxTestCase):

     yaml_config = """
tony and others added 3 commits May 16, 2016
Optionally disable shell history suppression
...in accordance with the prophecy.
@coveralls
Copy link

@coveralls coveralls commented May 16, 2016

Coverage Status

Coverage increased (+0.3%) to 64.14% when pulling 7eb9441 on ikirudennis:new-window-command into f0e5600 on tony:master.

@tony
Copy link
Member

@tony tony commented May 16, 2016

That was odd.

screen shot 2016-05-16 at 11 58 06 am

Restarted the process, and its ok now.

@tony
Copy link
Member

@tony tony commented May 16, 2016

Can you rebase against master and squash?

@kmactavish
Copy link
Contributor

@kmactavish kmactavish commented May 16, 2016

@tony

That was odd.
Restarted the process, and its ok now.

Hmm, that is strange. It is correct in identifying that as a test failure, echo isMissing is not supposed to be added to the bash history.

The only thing I can think of is that when the pane was captured, it hadn't finished being initialized, and the history_cmd erroneously refers to the same line as sent_cmd. A potential patch is increasing the timeout before capturing the pane (below). I tried to find a more robust, synchronous way of capturing the bash history, but this was all that I found worked.

If that's seen again, I can submit this patch.

diff --git a/tmuxp/testsuite/workspacebuilder.py b/tmuxp/testsuite/workspacebuilder.py
index 79b5329..206f52e 100644
--- a/tmuxp/testsuite/workspacebuilder.py
+++ b/tmuxp/testsuite/workspacebuilder.py
@@ -238,7 +238,7 @@ class SuppressHistoryTest(TmuxTestCase):

         builder = WorkspaceBuilder(sconf=sconfig)
         builder.build(session=self.session)
-        time.sleep(0.2) # give .bashrc, etc. time to load
+        time.sleep(0.3) # give .bashrc, etc. time to load

         s.server._update_windows()
         for w in s.windows:
@@ -250,7 +250,7 @@ class SuppressHistoryTest(TmuxTestCase):
                 # Print the last-in-history command in the pane
                 self.session.cmd('send-keys', ' fc -ln -1')
                 self.session.cmd('send-keys', 'Enter')
-                time.sleep(0.01) # give fc time to run
+                time.sleep(0.1) # give fc time to run

                 # Get the contents of the pane
                 self.session.cmd('capture-pane')

References #146 .

@tony
Copy link
Member

@tony tony commented May 16, 2016

@kmactavish Yep, that'll give travis a bit more breathing room.

Can you rebase against master and squash too? Once that's good we can merge

@ikirudennis
Copy link
Contributor Author

@ikirudennis ikirudennis commented May 16, 2016

I hope I did that right.

@coveralls
Copy link

@coveralls coveralls commented May 16, 2016

Coverage Status

Coverage increased (+0.3%) to 64.14% when pulling e015b71 on ikirudennis:new-window-command into f0e5600 on tony:master.

@tony
Copy link
Member

@tony tony commented May 16, 2016

re: squashing, I meant to direct that to @ikirudennis.

It seems GH has a "squash and merge" available after you hit "Merge Pull Request".

@tony tony merged commit d2c254b into tmux-python:master May 16, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.3%) to 64.14%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.