-
Notifications
You must be signed in to change notification settings - Fork 233
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
Fix layout related issues #737, #667, #704 #793
Conversation
@nvasilas Can you take a look at the tests? It looks like this may need a version conditional from libtmux.common, example:
|
Docs on related functions: https://libtmux.git-pull.com/reference/common.html?highlight=has_gt_version#libtmux.common.has_gt_version |
@tony Sure mate I 'll fix them. |
Codecov Report
@@ Coverage Diff @@
## master #793 +/- ##
=======================================
Coverage 75.19% 75.19%
=======================================
Files 18 18
Lines 1431 1431
Branches 337 336 -1
=======================================
Hits 1076 1076
Misses 267 267
Partials 88 88
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
…on#704 Use tmux default session size 80x24 when creating a new session
402acbc
to
927cf1f
Compare
@nvasilas Thank you for this! Live in v1.13.1 Any better? I will reach out to users in the other issues as well |
@@ -264,9 +269,6 @@ def build(self, session=None, append=False): | |||
assert isinstance(p, Pane) | |||
p = p | |||
|
|||
if "layout" in wconf: | |||
w.select_layout(wconf["layout"]) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tony If I recall correctly without this change the layout wasn't updated properly. It seems like I broke some things with this PR. I will investigate and I'll try to provide a fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nvasilas Thank you! Let's see how it goes!
def width(p): | ||
return int(p._info["pane_width"]) | ||
|
||
assert height(main_horizontal_pane) > height(panes[0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I learned something new:
main-horizontal
doesn't necessarily guarantee the top pane will be bigger.
A large (main) pane is shown at the top of the window and the remaining panes are spread from left to right in the leftover space at the bottom. Use the main-pane-height window option to specify the height of the top pane. manpage
I will try to remedy this in #809
@nvasilas I am deliberating fully reversing the change while we understand the problem better. You can recreate the same PR again, but it's my fault since my tests didn't catch the issues - so I feel bad The reason why I may reverse it (v1.13.2 and v1.13.3) is it had more of an effect on users with certain kinds of sessions than we both thought it would (#800) If you have more ideas on this - I am all ears - a bulk of my effort is improving In #809 I added a test for us to check for tmux spacing issues |
@tony Sorry mate I was abroad and I wasn't able to follow the recent changes. I introduced the bug with my PR and reverting it was the best choice. So, no worries. When I get some free time I will try to find a proper fix. |
@nvasilas I will keep you apprised either way 👍 (and vice-versa, hopefully!) |
Use tmux default session size 80x24 when creating a new session
re: #737, #667, #704