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: avoid child area overflow on split #10620

Merged
merged 1 commit into from
Apr 29, 2024

Conversation

l4l
Copy link
Contributor

@l4l l4l commented Apr 28, 2024

Not really sure it's a proper fix but with the patch original testcase could not be reproduced.

resolves #9877

@pascalkuthe
Copy link
Member

Not really sure it's a proper fix but with the patch original testcase could not be reproduced.

you can reproduce the original crash only if your terminal window is small enough (in the original issue description it describes a really tiny window).

In general the issue is that helix panics if the window is too small to properly render splits. For example as soon as there are atleast three vsplits and you make the window really small helix crashes. Similar problem for hsplits

Not sure if what you are doing here is sufficient but with a reproduction case it should be easy for you to test out

@l4l
Copy link
Contributor Author

l4l commented Apr 28, 2024

I am able to reproduce the original issue. By the pr statement I mean the patch fixes that testcase. Though apparently I've broken some other test, looking into it now.

@pascalkuthe
Copy link
Member

ah yeah sorry I was still tired and missread that fix does look good now that I looked at the surrounding code

@pascalkuthe
Copy link
Member

pascalkuthe commented Apr 28, 2024

I think the changes to that test are probably fine just need tonupdate the assertion

@l4l l4l force-pushed the fix/vsplit-child-underflow branch from ebd7afe to c4a8fdc Compare April 28, 2024 17:17
@l4l
Copy link
Contributor Author

l4l commented Apr 28, 2024

Though a little bit more about expected behavior. I would expect to the space being split in the equal parts and any roundings then added to the last child.

The first patch does not adhere to that. It actually avoid using the gap in the calculation. And the current one (just pushed) should be fine as the child width calculated from the really used space (e.g container - all the gaps). So fixed the failing test values and added one for the original issue.

Copy link
Member

@pascalkuthe pascalkuthe left a comment

Choose a reason for hiding this comment

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

thanks!

@pascalkuthe pascalkuthe added C-bug Category: This is a bug E-easy Call for participation: Experience needed to fix: Easy / not much S-waiting-on-review Status: Awaiting review from a maintainer. A-command Area: Commands labels Apr 28, 2024
@the-mikedavis the-mikedavis merged commit 8db9301 into helix-editor:master Apr 29, 2024
6 checks passed
@l4l l4l deleted the fix/vsplit-child-underflow branch April 29, 2024 13:39
Vulpesx pushed a commit to Vulpesx/helix that referenced this pull request Jun 7, 2024
Chirikumbrah pushed a commit to Chirikumbrah/helix that referenced this pull request Jun 15, 2024
smortime pushed a commit to smortime/helix that referenced this pull request Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-command Area: Commands C-bug Category: This is a bug E-easy Call for participation: Experience needed to fix: Easy / not much S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Panic in vsplit, goto_file_vsplit (Tree::recalculate)
3 participants