Prevent splitting panes into 0 width/height #2401

[richardszalay]

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 the TermControl if the split would fail.

Summary of the Pull Request

References

PR Checklist

• Closes Crash when attempting to split pane with insufficient space available #2401
• CLA signed. If not, go over here and sign the CLA
• Tests added/passed (there are no tests for this part of WT)
• Requires documentation to be updated
• I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: Crash when attempting to split pane with insufficient space available #2401

Detailed Description of the Pull Request / Additional comments

App::_SplitPane calls focusedTab->CanAddHorizontalSplit/CanAddHorizontalSplit before it initializes the TermControl 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 the Half constant.

Validation Steps Performed

1. Open a new tab
2. Attempt to split horizontally/vertically more than 6-8 times

Success: Pane will will eventually stop splitting rather than crashing the process.

[zadjii-msft]

Overall I'm really happy with this, but I think we need to account for the separator size in Pane::_CanSplit

[richardszalay]

Ah yes - I was originally planning on using _GetPaneSizes as it already contained the separator logic, but couldn't in the end because it relied on the _firstPercent/_secondPercent private members and then forgot to include it in my own implementation.

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)

[zadjii-msft]

(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 😄

[richardszalay]

@zadjii-msft Are those updates ok? The issue is still marked as requiring changes.

[zadjii-msft]

Oh, yea I'm happy with that. Sorry I missed the new commit!

[carlos-zamora]

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.

[carlos-zamora]

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.

[miniksa]

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.

[richardszalay]

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

[richardszalay]

@carlos-zamora Both the naming and the use of narrow_cast were used to remain consistent with the existing conventions of the class.

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.

[carlos-zamora]

@richardszalay Fair enough. Congrats on your first contribution :)

[DHowett-MSFT]

Thanks a bunch!

[ghost]

🎉Windows Terminal Preview v0.4.2382.0 has been released which incorporates this pull request.:tada:

Handy links:

• Release Notes
• Store Download