[1.14] Remove most uses of CompareInBounds
#13313
Conversation
| const til::point endInclusive{ bufferSize.RightInclusive(), bufferSize.BottomInclusive() }; | ||
| const auto val = limitOptional.value_or(endInclusive); |
There was a problem hiding this comment.
The old code uses bufferSize.EndExclusive() and not the "inclusive" variant?
There was a problem hiding this comment.
The only reason we used EndExclusive before was for the CompareInBounds call below. So instead, we're using EndInclusive (a valid point in the buffer), so that CompareInBounds works fine. Then we add 1 if we were past EndInclusive (i.e. EndExclusive).
ghost
added
Needs-Author-Feedback
| @@ -1142,7 +1142,7 @@ const COORD TextBuffer::GetWordStart(const COORD target, const std::wstring_view | |||
| // that it actually points to a space in the buffer | |||
| copy = { bufferSize.RightInclusive(), bufferSize.BottomInclusive() }; | |||
| } | |||
| else if (bufferSize.CompareInBounds(target, limit.to_win32_coord(), true) >= 0) | |||
| else if (til::point{ target.X, target.Y } >= limit) | |||
There was a problem hiding this comment.
i still don't totally get this. can't you use til::point{ target }?
| @@ -1730,20 +1726,20 @@ void UiaTextRangeBase::_moveEndpointByUnitDocument(_In_ const int moveCount, | |||
| { | |||
| case MovementDirection::Forward: | |||
| { | |||
| auto documentEnd{ bufferSize.EndExclusive() }; | |||
| til::point documentEnd{ bufferSize.EndExclusive().X, bufferSize.EndExclusive().Y }; | |||
There was a problem hiding this comment.
| til::point documentEnd{ bufferSize.EndExclusive().X, bufferSize.EndExclusive().Y }; | |
| til::point documentEnd{ bufferSize.EndExclusive() }; |
?
| if (til::point{ _start } > documentEnd) | ||
| { | ||
| _start = documentEnd; | ||
| _start = documentEnd.to_win32_coord(); | ||
| } | ||
| if (bufferSize.CompareInBounds(_end, documentEnd, true) > 0) | ||
| if (til::point{ _end } > documentEnd) | ||
| { | ||
| _end = documentEnd; | ||
| _end = documentEnd.to_win32_coord(); |
There was a problem hiding this comment.
i have to admit, I somewhat thought that "porting the coordinate system change for UIA only" would mean that _start and _end were natively til::point, and didn't have to be converted every time we encountered them
There was a problem hiding this comment.
I wish. But the problem is Viewport and TextBuffer still need them to be COORDs. And occasionally, they'll be copied by reference. It's doable, but I figured this would be the easiest path forward with the least risk. Especially since this is a servicing update after all haha
|
On third thought, perhaps it would be more correct to leave 1.14 be with the fix you had 😄 |
|
I'm gonna go ahead and close this PR. Dustin and I chatted about it and it provides little benefit because...
#13244 still makes sense for |
Summary of the Pull Request
Replaces most uses of
Viewport::CompareInBounds()withtil::point's<and>operators.CompareInBoundshas been the cause of a bunch of UIA crashes over the years. Replacing them entirely ensures that theFAILFAST_IFisn't ever touched.This was a bit tricky to do because I wanted to ensure the same functionality was maintained. Since we still have a ton of
COORDs floating around, I just cast totil::pointwhen we're about to do a comparison. We could do smarter changes, but I'm concerned that could cause a bug we could miss.References
#13183
#13244 - same PR but for
main