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.

Sign up for GitHub

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

Jump to bottom

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
Open

Fix ghost space left by SplitPane with top_level=true #5969

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions mux/src/tab.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

}

let mut cursor = self.pane.take().unwrap().cursor();
Expand Down