Fix a crash when resizing conhost in Debug#13001
Conversation
Don't clamp here, it's unnecessary. See notes in #12917 (comment) Closes #12917
ghost
added
Area-Output
|
|
| cursorView = view.Clamp(cursorView); | ||
|
|
||
| if (cursorView.IsValid()) | ||
| if (view.IsInBounds(cursorView)) |
There was a problem hiding this comment.
What a strange way for us to do this. And IsInBounds doesn't beef? Neat!
There was a problem hiding this comment.
I mean the old way was strange.
There was a problem hiding this comment.
I guess it's a bit late to tell you now, but I don't think this is right. I haven't had a chance to check, but I think IsInBounds is testing whether the whole cursorView is inside the view, but that means a cursor that is partially off screen is probably not going to be refreshed.
There was a problem hiding this comment.
Hmmmmm..
Would this come up when the cursor is on a double-width glyph and the viewport is scrolled such that it cuts that glyph in half?
There was a problem hiding this comment.
Yeah. Also on a double-width line, or even a double-width glyph on a double-width line, which can be four cells wide.
Depending on the line rendition, and whether the cursor is over a wide character or not, it's possible for the width to take up anywhere from 1 to 4 cells. And when it's more than 1 cell wide, part of the cursor may end up off screen. However, our bounds check requires the entire cursor to be on screen, otherwise it doesn't render anything, and that can result in cursor droppings being left behind. This PR fixes that. The bounds check that is causing this issue was introduced in #13001 to fix a debug assertion. To fix this, I've removed the bounds checking, and instead clip the cursor rect to the boundaries of the viewport. This is what the original code was trying to do prior to the #13001 fix, but was mistakenly using the `Viewport:Clamp` method, instead of `TrimToViewport`. Since this implementation doesn't require a clamp, there should be no risk of the debug assertion returning. ## Validation Steps Performed I've confirmed that the test case in #14657 is now working correctly, and not leaving cursor droppings behind. I've also tested under conhost with buffer sizes wider than the viewport, and confirmed it can handle a wide cursor partially scrolled off screen. Closes #14657
3 participants
Don't clamp here, it's unnecessary. See notes in #12917 (comment)
Closes #12917