Add `shell`-configuration to pane layer #672

jerri · 2021-03-03T21:12:33Z

This is supposed to fix #139.

I added a shell-configuration to the pane layer. This way you can specify for every pane what shell/application is supposed to be started. I added an example to the documentation to drive the point home why it makes sense.

I wasn't really able to understand how to add the test cases to the test environment. I hope someone can help with this. The example should hopefully already contain most edge cases in any case.

The more complicated part was to make sure that the shell command for the first pane gets respected by the window creation, as the shell for the first pane has to be handed over to the create-window-command.

codecov · 2021-03-03T21:13:58Z

Codecov Report

Merging #672 (2ad5f96) into master (d20e653) will decrease coverage by 0.28%.
The diff coverage is 45.45%.

@@            Coverage Diff             @@
##           master     #672      +/-   ##
==========================================
- Coverage   75.51%   75.23%   -0.29%     
==========================================
  Files           8        8              
  Lines        1168     1179      +11     
  Branches      295      298       +3     
==========================================
+ Hits          882      887       +5     
- Misses        205      209       +4     
- Partials       81       83       +2

Impacted Files	Coverage Δ
tmuxp/workspacebuilder.py	`85.26% <45.45%> (-2.45%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d20e653...2ad5f96. Read the comment docs.

jerri · 2021-03-16T10:24:52Z

Is there anything I can do, to further this merge requests? I am struggling with the test environment. Maybe someone can give me a hand or a hint.

tony · 2021-07-18T15:51:49Z

@jerri can you rebase this? Sorry for the delay!

Have you seen the developer documentation here? https://tmuxp.git-pull.com/developing.html

Maybe someone can give me a hand or a hint.

Are you able to run the tests locally? What part of this are you having issues with? e.g. writing a new test?

examples/pane-shell.yaml

jerri · 2021-07-24T19:53:03Z

I didn't have an idea how to add an appropriate automatic check. On top the automatic tests don't worked on my mac. I found reasons why, but not how to fix them. Added the information to another ticket, that also mentioned the problems.
I can do the rebase. Sure.

jerri · 2021-07-24T20:17:19Z

I fixed the conflict. And push the new version.
I also found my comment in #620 (comment) about the problem I had with the unit testing. When commenting out the mentioned test, I could complete the whole test procedure. Unfortunately I sort of ran out of time and couldn't continue on understanding how the test environment works and how to add automatic testing for my change.

tony · 2021-10-30T11:57:54Z

@jerri Hi, sorry for the delay!

Can you rebase / solve conflicts? (Interestingly I'm not seeing the tests show up)

jerri · 2021-12-21T22:10:09Z

There shouldn't be any more conflict, no? At least I don't see any conflicts mentioned in this pull request.

tony · 2021-12-21T22:18:58Z

@jerri Hi there!

If you rebase against master, you may have conflicts to solve. But right now (and back then) it wasn't rebased.

I'm not familiar with how to understand merge commits like c43079c. As you can see it's a few pages long.

https://github.com/tmux-python/tmuxp/pull/672/commits

Do you know how to rebase against master? Can I provide a tutorial maybe?

In other words, it would be just your changes as commits, I'm not sure what to make of the commit with "Merge branch 'master' into issue-139".

tony · 2021-12-21T22:23:27Z

I remember now. When I asked that question earlier, I was of the belief that GitHub actions tests weren't running because the merge commit(s). Also we recently migrated to GitHub actions.

I suppose GitHub's interface solves a lot of it (I only see the diff) - I'd still prefer having a clean PR. Is that possible? (Not via merge, but git pull --rebase [tmux-python's remote] master

Example of what I mean:

git checkout issue-139
git remote add tmux-python https://github.com/tmux-python/tmuxp.git
git fetch tmux-python
git pull --rebase tmux-python master
Resolve any conflicts that arise
git push --force-with-lease

(Assume the branch issue-139 is already set to target your repo)

jerri · 2021-12-21T23:08:11Z

I see. I will prepare something fresh then.

tony · 2021-12-22T00:31:19Z

@jerri Sorry about that. Thank you! Take your time.

tony · 2021-12-22T00:32:47Z

@jerri Also, have you figured out how to write the tests? e.g. test_workspacebuilder.py

That would definitely make the PR easier to merge

jerri · 2021-12-22T21:22:30Z

Sorry, no testing as of now. First I didn't find a fix for the problem that occured at the time. Second I really didn't find time to do any additional implementation.

Had to implement the special case for the first pane. In that case the shell for the window that is created has to be overridden. This also fixed a bug (in my opinion) that `window_shell` only affected the first pane, and none of the other panes in a window. The system unfortunatelly still bailes out, when the specified shell is not executable (e.g. doesn't even exist). The problem is an unhandeled exception when the information for the pane can't be extracted (as it never really started). Not sure how to catch that and provide a sensible message to the user.

As the examples.rst files was replaced by the md-files in the main line.

jerri · 2021-12-22T21:32:58Z

OK. Rebase has happened.

tony · 2021-12-22T21:36:48Z

@jerri Thank you!

tony · 2021-12-22T21:37:04Z

@jerri I will take a look at if I can add the tests, too

tony · 2022-01-08T12:32:19Z

@jerri If you give me access to your tmuxp repository, I can also rebase it for you and force push it

CLAassistant · 2022-01-11T03:27:36Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

tony · 2022-01-29T12:50:21Z

examples/pane-shell.yaml

+          - print('This is python 3')
+      - shell: /usr/bin/vim -u none
+        shell_command:
+          - iAll panes have the `remain-on-exit` setting on.


Suggested change

- iAll panes have the `remain-on-exit` setting on.

- All panes have the `remain-on-exit` setting on.

Wait. I'm wrong

i... Insert

I've been in vim every day for 15+ years and this got past me

tony · 2022-01-29T13:03:38Z

@jerri It's in!

tony · 2022-01-29T13:03:56Z

I will add this into next release, which will hopefully be soon!

jerri · 2022-01-29T13:18:37Z

Awesome. Thanks for the info, and hope it helps. :-D

tony · 2022-03-19T15:10:49Z

@jerri This is live in tmuxp 1.10

tony reviewed Jul 18, 2021

View reviewed changes

examples/pane-shell.yaml Show resolved Hide resolved

jerri added 3 commits December 22, 2021 22:27

Added an example for the pane-shell configuration.

ca32056

Added the documentation to the example.md file instead.

2ad5f96

As the examples.rst files was replaced by the md-files in the main line.

jerri force-pushed the issue-139 branch from 357a17d to 2ad5f96 Compare December 22, 2021 21:31

tony reviewed Jan 29, 2022

View reviewed changes

tony merged commit 7b84844 into tmux-python:master Jan 29, 2022

tony added a commit that referenced this pull request Jan 29, 2022

docs(CHANGES): Update for shell pane feature from @jerri in #672

97b7e8d

jerri deleted the issue-139 branch January 29, 2022 13:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `shell`-configuration to pane layer #672

Add `shell`-configuration to pane layer #672

jerri commented Mar 3, 2021

codecov bot commented Mar 3, 2021 •

edited

Loading

jerri commented Mar 16, 2021

tony commented Jul 18, 2021

jerri commented Jul 24, 2021

jerri commented Jul 24, 2021

tony commented Oct 30, 2021

jerri commented Dec 21, 2021

tony commented Dec 21, 2021

tony commented Dec 21, 2021 •

edited

Loading

jerri commented Dec 21, 2021

tony commented Dec 22, 2021

tony commented Dec 22, 2021

jerri commented Dec 22, 2021

jerri commented Dec 22, 2021

tony commented Dec 22, 2021

tony commented Dec 22, 2021

tony commented Jan 8, 2022

CLAassistant commented Jan 11, 2022

tony Jan 29, 2022

tony Jan 29, 2022

tony Jan 29, 2022

tony Jan 29, 2022 •

edited

Loading

tony commented Jan 29, 2022

tony commented Jan 29, 2022

jerri commented Jan 29, 2022

tony commented Mar 19, 2022

	- iAll panes have the `remain-on-exit` setting on.
	- All panes have the `remain-on-exit` setting on.

Add shell-configuration to pane layer #672

Add shell-configuration to pane layer #672

Conversation

jerri commented Mar 3, 2021

codecov bot commented Mar 3, 2021 • edited Loading

Codecov Report

jerri commented Mar 16, 2021

tony commented Jul 18, 2021

jerri commented Jul 24, 2021

jerri commented Jul 24, 2021

tony commented Oct 30, 2021

jerri commented Dec 21, 2021

tony commented Dec 21, 2021

tony commented Dec 21, 2021 • edited Loading

jerri commented Dec 21, 2021

tony commented Dec 22, 2021

tony commented Dec 22, 2021

jerri commented Dec 22, 2021

jerri commented Dec 22, 2021

tony commented Dec 22, 2021

tony commented Dec 22, 2021

tony commented Jan 8, 2022

CLAassistant commented Jan 11, 2022

tony Jan 29, 2022

Choose a reason for hiding this comment

tony Jan 29, 2022

Choose a reason for hiding this comment

tony Jan 29, 2022

Choose a reason for hiding this comment

tony Jan 29, 2022 • edited Loading

Choose a reason for hiding this comment

tony commented Jan 29, 2022

tony commented Jan 29, 2022

jerri commented Jan 29, 2022

tony commented Mar 19, 2022

Add `shell`-configuration to pane layer #672

Add `shell`-configuration to pane layer #672

codecov bot commented Mar 3, 2021 •

edited

Loading

tony commented Dec 21, 2021 •

edited

Loading

tony Jan 29, 2022 •

edited

Loading