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 ghost space left by SplitPane with top_level=true #5969

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

boazy
Copy link

@boazy boazy commented Aug 15, 2024

Fixes #2579, #4984

When there is more than one pane in the tab, the tab.resize() is
called and the tab resize logic is reused to correctly resize all the
panes in the tab before adding the new pane split, so the new split
pane will not overwrite any existing pane.

The issue seems to be that after this resize, the tab "remembers" its
new size, and when the new split pane is closed, the tab is left with
ghost space which cannot be reused.

The fix is quite simple: we just need to immediately resize the tab
back to its original proportions. Since the underlying panes have
already been resized by tab.resize(), we don't need to keep the
restricted tab size.

I may have missed something, but this seems to work correctly.

When there is more than one pane in the tab, the tab.resize() is
called and the tab resize logic is reused to correctly resize all the
panes in the tab before adding the new pane split, so the new split
pane will not overwrite any existing pane.

The issue seems to be that after this resize, the tab "remembers" its
new size, and when the new split pane is closed, the tab is left with
ghost space which cannot be reused.

The fix is quite simple: we just need to immediately resize the tab
back to its original proportions. Since the underlying panes have
already been resized by tab.resize(), we don't need to keep the
restricted tab size.
@@ -2004,6 +2004,9 @@ impl TabInner {
} else {
self.resize(split_info.second.clone());
}
// Resize the tab back to the original tab size
// to avoid leaving unusable space within the tab
self.resize(tab_size)
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for diving in. I'm not sure about this though; if this results in the right outcome, should the above logic to resize be retained?
Can you provide more details on the precise problem scenario, and explain what you meant by the size being remembered?

Copy link
Author

Choose a reason for hiding this comment

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

I didn't write this logic, so I wasn't sure about why it exists in the first place. So of course I tried to remove it. But when I removed it completely, something else broke: The new pane text just wrote over the old pane's text without removing it. For instance, If I had a situation like this:

Before Split:

┌─────────────┬──────┐            
│> ls -1      │ P    │
│file.txt     │ a    │
│file2.txt    │ n    │
│file3.txt    │ e    │
│             │      │
│> echo Hi    │      │
│Hi           │      │
│             │      │
│> vim file.t │      │
└─────────────┴──────┘            

After Split (with direction=Down, top_level=true):

┌──────────────┬─────┐
│> ls -1       | P   │
│file.txt      | a   │
│file2.txt     | n   │
│file3.txt     | e   │
├──────────────┴─────┤
│mysh 1.0 Hi         │
│>                   │
│                    │
│> vim file.t        │
└────────────────────┘

It was a lot uglier in practice, but that's the basic gist. I'm not 100% what is the underlying issue, but I guess the existing panes' visible area doesn't get redrawn as necessary without calling resize if there is more than one pane.

So the resize() call does solve an actual issue. The main problem is that after the new pane you added is closed, the pane(s) that you resized won't refill the area of the closed pane, because the tab size is now smaller than full window size. This is exactly what issue #4984 refers to. Adding this extra resize() call resizes the tab to the full window size again, but the original (pre-split) panes' contents remain constrained to their new size.

Copy link
Author

Choose a reason for hiding this comment

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

An example of the issue that happens with the current code (without my changes) is provided in #4984 (reproduction of issue).

Due to this bug, top_level splits are practically useless. They are not needed to begin with if you just have 1 pane before the split, but if you have more than 1 pane they always leave unusable space behind. I don't think this fix can possibly make anything worse.

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.

Split created with top_level=true cannot be split any further if started from "non top-level split"
2 participants