-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Prevent splitting panes into 0 width/height #2401 #2450
Prevent splitting panes into 0 width/height #2401 #2450
Conversation
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.
Overall I'm really happy with this, but I think we need to account for the separator size in Pane::_CanSplit
a3b48a0
to
8c78574
Compare
Ah yes - I was originally planning on using I've amended the commit to include the separator. (To be honest, I think a lot of Pane.cpp could be refactored to avoid duplicating logic but since this is my first PR I wanted to keep things simple) |
I would so very happily review that PR 😄 |
@zadjii-msft Are those updates ok? The issue is still marked as requiring changes. |
Oh, yea I'm happy with that. Sorry I missed the new commit! |
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.
It feels weird that we have Tab::CanAddHorizontalSplit()
and Tab::CanAddVerticalSplit()
. Could we instead have Tab::CanAddPaneSplit(Pane::SplitState)
? That would cut down on some of the code to maintain.
The same idea applies to Pane.
{ | ||
const bool changeWidth = _splitState == SplitState::Vertical; | ||
|
||
const Size actualSize{ gsl::narrow_cast<float>(_root.ActualWidth()), |
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.
Should this be gsl::narrow_cast
or gsl::narrow
? I honestly don't know, but I know gsl::narrow_cast
can throw an exception and gsl::narrow
doesn't.
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.
The other way around. gsl::narrow_cast
means shrink it and I don't care if it doesn't fit exactly.
gsl::narrow
means if it didn't fit, throw an exception.
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.
@carlos-zamora Since double
->float
is only going to cause a precision loss, I'm assuming that's preferable to keep using gsl::narrow_cast
?
@carlos-zamora Both the naming and the use of I plan on doing a refactor PR as per my comments above, but I didn't want to include it in this PR since it would detract from the fix being made (not to mention that this is my first terminal PR) If you're happy to merge this as-is, I can follow up with the refactor PR. |
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.
@richardszalay Fair enough. Congrats on your first contribution :)
Thanks a bunch! |
🎉 Handy links: |
Fixes a crash that can occur when splitting pane that was so small that the target panes would have a width/height of 0, causing DxRenderer to fail when creating the device resources.
This PR prevents both the call to
App::AddHorizontal/VerticalSplit
and the creation of theTermControl
if the split would fail.Summary of the Pull Request
References
PR Checklist
Detailed Description of the Pull Request / Additional comments
App::_SplitPane
callsfocusedTab->CanAddHorizontalSplit/CanAddHorizontalSplit
before it initializes theTermControl
to avoid having to deal with the cleanup. If a split cannot occur, it will simply return.Question: Should we beep or something here?
It then follows the same naming/flow style as the split operation, so:
Tab::CanAddHorizontalSplit -> Pane::CanSplitHorizontal ->Pane::_CanSplit
. The public pane methods will handle leaf/child the same as the current Split methods._CanSplit
reuses existing logic like_root.GetActualWidth/Height
,Pane::_GetMinSize
, and theHalf
constant.Validation Steps Performed
Success: Pane will will eventually stop splitting rather than crashing the process.