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

Fix layout related issues #737, #667, #704 #793

Merged
merged 4 commits into from
Aug 21, 2022
Merged

Conversation

nvasilas
Copy link
Contributor

@nvasilas nvasilas commented Aug 7, 2022

Use tmux default session size 80x24 when creating a new session

re: #737, #667, #704

@tony
Copy link
Member

tony commented Aug 7, 2022

@nvasilas Can you take a look at the tests?

It looks like this may need a version conditional from libtmux.common, example:

@tony
Copy link
Member

tony commented Aug 7, 2022

@nvasilas
Copy link
Contributor Author

nvasilas commented Aug 8, 2022

@tony Sure mate I 'll fix them.

@tony
Copy link
Member

tony commented Aug 14, 2022

@nvasilas I rebased your branch via GitHub's rebase button (git pull --rebase --autostash to pull the changes)

The changes include basic mypy support #786

@codecov
Copy link

codecov bot commented Aug 18, 2022

Codecov Report

Merging #793 (28c4366) into master (b70cd70) will not change coverage.
The diff coverage is 100.00%.

@@           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           
Impacted Files Coverage Δ
tmuxp/workspacebuilder.py 85.29% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@tony tony force-pushed the master branch 2 times, most recently from 402acbc to 927cf1f Compare August 21, 2022 21:12
@tony
Copy link
Member

tony commented Aug 21, 2022

@nvasilas It really works. Nice.

Tested by doing pip install -e ~/path/to/your/tmuxp/branch and doing tmuxp load ./workspacefile.yaml -L another-tmux-server.

Master: b70cd70: Layout is scrunched
This PR: Maintains the layout!

@tony tony merged commit 129300d into tmux-python:master Aug 21, 2022
tony added a commit that referenced this pull request Aug 21, 2022
@tony
Copy link
Member

tony commented Aug 21, 2022

@nvasilas Thank you for this!

Live in v1.13.1

PyPI, GitHub Tag

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"])

Copy link
Member

Choose a reason for hiding this comment

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

@nvasilas Do you recall the rationale behind the removing / adding of the .select_layout's here, here, and here?

Copy link
Contributor Author

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.

Copy link
Member

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])
Copy link
Member

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

@tony
Copy link
Member

tony commented Sep 10, 2022

@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 libtmux so issues like this are easier to diagnose. The experience is going to get better, and we'll improve at catching these issues, I promise.

In #809 I added a test for us to check for tmux spacing issues

@tony
Copy link
Member

tony commented Sep 11, 2022

@nvasilas I've started a new issue at #815 the opportunity arises you'd like to collab (sorry about retracting on this PR, it is my 100% my mess up)

@nvasilas
Copy link
Contributor Author

@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.

@tony
Copy link
Member

tony commented Sep 17, 2022

@nvasilas I will keep you apprised either way 👍 (and vice-versa, hopefully!)

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

Successfully merging this pull request may close these issues.

2 participants