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.

Sign up for GitHub

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

Jump to bottom

Remove most uses of CompareInBounds #13244

Merged
7 commits merged into from Jul 1, 2022
Merged

Remove most uses of CompareInBounds #13244

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9596b06
Remove most uses of `CompareInBounds`
carlos-zamora Jun 7, 2022
4ce925e
address PR comments
carlos-zamora Jun 9, 2022
b105d68
clamp to inclusive pos & compare returns 0/1
carlos-zamora Jun 9, 2022
e9b2ffc
fix test
carlos-zamora Jun 9, 2022
e0695ec
make 'MoveToNextGlyph' prettier
carlos-zamora Jun 10, 2022
49c33d9
Merge branch 'main' into dev/cazamor/remove-compareInBounds
carlos-zamora Jun 10, 2022
c98940a
backwards
carlos-zamora Jun 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
30 changes: 20 additions & 10 deletions src/buffer/out/textBuffer.cpp
View file
Open in desktop
Expand Up @@ -1138,7 +1138,7 @@ til::point TextBuffer::GetWordStart(const til::point target, const std::wstring_
// that it actually points to a space in the buffer
copy = bufferSize.BottomRightInclusive();
}
else if (bufferSize.CompareInBounds(target, limit, true) >= 0)
else if (target >= limit)
{
// if at/past the limit --> clamp to limit
copy = limitOptional.value_or(bufferSize.BottomRightInclusive());
Expand Down Expand Up @@ -1254,7 +1254,7 @@ til::point TextBuffer::GetWordEnd(const til::point target, const std::wstring_vi
// Already at/past the limit. Can't move forward.
const auto bufferSize{ GetSize() };
const auto limit{ limitOptional.value_or(bufferSize.EndExclusive()) };
if (bufferSize.CompareInBounds(target, limit, true) >= 0)
if (target >= limit)
{
return target;
}
Expand Down Expand Up @@ -1282,7 +1282,7 @@ til::point TextBuffer::_GetWordEndForAccessibility(const til::point target, cons
const auto bufferSize{ GetSize() };
auto result{ target };

if (bufferSize.CompareInBounds(target, limit, true) >= 0)
if (target >= limit)
{
// if we're already on/past the last RegularChar,
// clamp result to that position
Expand Down Expand Up @@ -1419,7 +1419,7 @@ bool TextBuffer::MoveToNextWord(til::point& pos, const std::wstring_view wordDel
const auto limit{ limitOptional.value_or(bufferSize.EndExclusive()) };
const auto copy{ _GetWordEndForAccessibility(pos, wordDelimiters, limit) };

if (bufferSize.CompareInBounds(copy, limit, true) >= 0)
if (copy >= limit)
{
return false;
}
Expand Down Expand Up @@ -1466,7 +1466,7 @@ til::point TextBuffer::GetGlyphStart(const til::point pos, std::optional<til::po
const auto limit{ limitOptional.value_or(bufferSize.EndExclusive()) };

// Clamp pos to limit
if (bufferSize.CompareInBounds(resultPos, limit, true) > 0)
if (resultPos > limit)
{
resultPos = limit;
}
Expand Down Expand Up @@ -1494,7 +1494,7 @@ til::point TextBuffer::GetGlyphEnd(const til::point pos, bool accessibilityMode,
const auto limit{ limitOptional.value_or(bufferSize.EndExclusive()) };

// Clamp pos to limit
if (bufferSize.CompareInBounds(resultPos, limit, true) > 0)
if (resultPos > limit)
{
resultPos = limit;
}
Expand Down Expand Up @@ -1524,9 +1524,19 @@ til::point TextBuffer::GetGlyphEnd(const til::point pos, bool accessibilityMode,
bool TextBuffer::MoveToNextGlyph(til::point& pos, bool allowExclusiveEnd, std::optional<til::point> limitOptional) const
{
const auto bufferSize = GetSize();
const auto limit{ limitOptional.value_or(bufferSize.EndExclusive()) };
bool pastEndInclusive;
til::point limit;
{
// if the limit is past the end of the buffer,
// 1) clamp limit to end of buffer
// 2) set pastEndInclusive
const auto endInclusive{ bufferSize.BottomRightInclusive() };
const auto val = limitOptional.value_or(endInclusive);
pastEndInclusive = val > endInclusive;
limit = pastEndInclusive ? endInclusive : val;
}

const auto distanceToLimit{ bufferSize.CompareInBounds(pos, limit, true) };
const auto distanceToLimit{ bufferSize.CompareInBounds(pos, limit) + (pastEndInclusive ? 1 : 0) };
if (distanceToLimit >= 0)
{
// Corner Case: we're on/past the limit
Expand Down Expand Up @@ -1569,7 +1579,7 @@ bool TextBuffer::MoveToPreviousGlyph(til::point& pos, std::optional<til::point>
const auto bufferSize = GetSize();
const auto limit{ limitOptional.value_or(bufferSize.EndExclusive()) };

if (bufferSize.CompareInBounds(pos, limit, true) > 0)
if (pos > limit)
{
// we're past the end
// clamp us to the limit
Expand Down Expand Up @@ -1611,7 +1621,7 @@ const std::vector<til::inclusive_rect> TextBuffer::GetTextRects(til::point start
// (0,0) is the top-left of the screen
// the physically "higher" coordinate is closer to the top-left
// the physically "lower" coordinate is closer to the bottom-right
const auto [higherCoord, lowerCoord] = bufferSize.CompareInBounds(start, end) <= 0 ?
const auto [higherCoord, lowerCoord] = start <= end ?
std::make_tuple(start, end) :
std::make_tuple(end, start);

Expand Down
6 changes: 3 additions & 3 deletions src/cascadia/TerminalCore/TerminalSelection.cpp
View file
Open in desktop
Expand Up @@ -182,7 +182,7 @@ void Terminal::SetSelectionEnd(const til::point viewportPos, std::optional<Selec
// - the new start/end for a selection
std::pair<til::point, til::point> Terminal::_PivotSelection(const til::point targetPos, bool& targetStart) const
{
if (targetStart = _activeBuffer().GetSize().CompareInBounds(targetPos, _selection->pivot) <= 0)
if (targetStart = targetPos <= _selection->pivot)
{
// target is before pivot
// treat target as start
Expand Down Expand Up @@ -424,7 +424,7 @@ void Terminal::_MoveByWord(SelectionDirection direction, til::point& pos)
case SelectionDirection::Left:
{
const auto wordStartPos{ _activeBuffer().GetWordStart(pos, _wordDelimiters) };
if (_activeBuffer().GetSize().CompareInBounds(_selection->pivot, pos) < 0)
if (_selection->pivot < pos)
{
// If we're moving towards the pivot, move one more cell
pos = wordStartPos;
Expand All @@ -447,7 +447,7 @@ void Terminal::_MoveByWord(SelectionDirection direction, til::point& pos)
case SelectionDirection::Right:
{
const auto wordEndPos{ _activeBuffer().GetWordEnd(pos, _wordDelimiters) };
if (_activeBuffer().GetSize().CompareInBounds(pos, _selection->pivot) < 0)
if (pos < _selection->pivot)
{
// If we're moving towards the pivot, move one more cell
pos = _activeBuffer().GetWordEnd(pos, _wordDelimiters);
Expand Down
4 changes: 2 additions & 2 deletions src/interactivity/win32/ut_interactivity_win32/UiaTextRangeTests.cpp
View file
Open in desktop
Expand Up @@ -484,9 +484,9 @@ class UiaTextRangeTests

Log::Comment(L"_start and end should be 2 units apart. Sign depends on order of comparison.");
THROW_IF_FAILED(utr1->CompareEndpoints(TextPatternRangeEndpoint_End, utr2.Get(), TextPatternRangeEndpoint_End, &comparison));
VERIFY_IS_TRUE(comparison == -2);
VERIFY_IS_TRUE(comparison == -1);
THROW_IF_FAILED(utr2->CompareEndpoints(TextPatternRangeEndpoint_End, utr1.Get(), TextPatternRangeEndpoint_End, &comparison));
VERIFY_IS_TRUE(comparison == 2);
VERIFY_IS_TRUE(comparison == 1);
}

TEST_METHOD(ExpandToEnclosingUnit)
Expand Down
52 changes: 23 additions & 29 deletions src/types/UiaTextRangeBase.cpp
View file
Open in desktop
Expand Up @@ -70,7 +70,7 @@ try
RETURN_IF_FAILED(RuntimeClassInitialize(pData, pProvider, wordDelimiters));

// start is before/at end, so this is valid
if (_pData->GetTextBuffer().GetSize().CompareInBounds(start, end, true) <= 0)
if (start <= end)
{
_start = start;
_end = end;
Expand Down Expand Up @@ -166,7 +166,7 @@ bool UiaTextRangeBase::SetEndpoint(TextPatternRangeEndpoint endpoint, const til:
case TextPatternRangeEndpoint_End:
_end = val;
// if end is before start...
if (bufferSize.CompareInBounds(_end, _start, true) < 0)
if (_end < _start)
{
// make this range degenerate at end
_start = _end;
Expand All @@ -175,7 +175,7 @@ bool UiaTextRangeBase::SetEndpoint(TextPatternRangeEndpoint endpoint, const til:
case TextPatternRangeEndpoint_Start:
_start = val;
// if start is after end...
if (bufferSize.CompareInBounds(_start, _end, true) > 0)
if (_start > _end)
{
// make this range degenerate at start
_end = _start;
Expand Down Expand Up @@ -246,13 +246,13 @@ try
const auto mine = GetEndpoint(endpoint);

// TODO GH#5406: create a different UIA parent object for each TextBuffer
// This is a temporary solution to comparing two UTRs from different TextBuffers
// Ensure both endpoints fit in the current buffer.
const auto bufferSize = _pData->GetTextBuffer().GetSize();
RETURN_HR_IF(E_FAIL, !bufferSize.IsInBounds(mine, true) || !bufferSize.IsInBounds(other, true));
// We should return E_FAIL if we detect that the endpoints are from two different TextBuffer objects.
// For now, we're fine to not do that because we're not using any code that can FAIL_FAST anymore.

// compare them
*pRetVal = bufferSize.CompareInBounds(mine, other, true);
*pRetVal = mine < other ? -1 :
mine > other ? 1 :
0;

UiaTracing::TextRange::CompareEndpoints(*this, endpoint, *range, targetEndpoint, *pRetVal);
return S_OK;
Expand Down Expand Up @@ -293,7 +293,7 @@ void UiaTextRangeBase::_expandToEnclosingUnit(TextUnit unit)
// If we're past document end,
// set us to ONE BEFORE the document end.
// This allows us to expand properly.
if (bufferSize.CompareInBounds(_start, documentEnd, true) >= 0)
if (_start >= documentEnd)
{
_start = documentEnd;
bufferSize.DecrementInBounds(_start, true);
Expand Down Expand Up @@ -653,8 +653,8 @@ try
bufferSize.IncrementInBounds(end, true);

// make sure what was found is within the bounds of the current range
if ((searchDirection == Search::Direction::Forward && bufferSize.CompareInBounds(end, _end, true) < 0) ||
(searchDirection == Search::Direction::Backward && bufferSize.CompareInBounds(start, _start) > 0))
if ((searchDirection == Search::Direction::Forward && end < _end) ||
(searchDirection == Search::Direction::Backward && start > _start))
{
RETURN_IF_FAILED(Clone(ppRetVal));
auto& range = static_cast<UiaTextRangeBase&>(**ppRetVal);
Expand Down Expand Up @@ -872,15 +872,15 @@ IFACEMETHODIMP UiaTextRangeBase::GetBoundingRectangles(_Outptr_result_maybenull_

// startAnchor: the earliest til::point we will get a bounding rect for
auto startAnchor = GetEndpoint(TextPatternRangeEndpoint_Start);
if (bufferSize.CompareInBounds(startAnchor, viewportOrigin, true) < 0)
if (startAnchor < viewportOrigin)
{
// earliest we can be is the origin
startAnchor = viewportOrigin;
}

// endAnchor: the latest til::point we will get a bounding rect for
auto endAnchor = GetEndpoint(TextPatternRangeEndpoint_End);
if (bufferSize.CompareInBounds(endAnchor, viewportEnd, true) > 0)
if (endAnchor > viewportEnd)
{
// latest we can be is the viewport end
endAnchor = viewportEnd;
Expand All @@ -889,7 +889,7 @@ IFACEMETHODIMP UiaTextRangeBase::GetBoundingRectangles(_Outptr_result_maybenull_
// _end is exclusive, let's be inclusive so we don't have to think about it anymore for bounding rects
bufferSize.DecrementInBounds(endAnchor, true);

if (IsDegenerate() || bufferSize.CompareInBounds(_start, viewportEnd, true) > 0 || bufferSize.CompareInBounds(_end, viewportOrigin, true) < 0)
if (IsDegenerate() || _start > viewportEnd || _end < viewportOrigin)
{
// An empty array is returned for a degenerate (empty) text range.
// reference: https://docs.microsoft.com/en-us/windows/win32/api/uiautomationclient/nf-uiautomationclient-iuiautomationtextrange-getboundingrectangles
Expand Down Expand Up @@ -994,10 +994,7 @@ std::wstring UiaTextRangeBase::_getTextValue(til::CoordType maxLength) const
// TODO GH#5406: create a different UIA parent object for each TextBuffer
// nvaccess/nvda#11428: Ensure our endpoints are in bounds
// otherwise, we'll FailFast catastrophically
if (!bufferSize.IsInBounds(_start, true) || !bufferSize.IsInBounds(_end, true))
{
THROW_HR(E_FAIL);
}
THROW_HR_IF(E_FAIL, !bufferSize.IsInBounds(_start) || !bufferSize.IsInBounds(_end));

// convert _end to be inclusive
auto inclusiveEnd = _end;
Expand Down Expand Up @@ -1045,11 +1042,11 @@ try
constexpr auto endpoint = TextPatternRangeEndpoint::TextPatternRangeEndpoint_Start;
const auto bufferSize{ _pData->GetTextBuffer().GetSize() };
const auto documentEnd = _getDocumentEnd();
if (bufferSize.CompareInBounds(_start, documentEnd, true) > 0)
if (_start > documentEnd)
{
_start = documentEnd;
}
if (bufferSize.CompareInBounds(_end, documentEnd, true) > 0)
if (_end > documentEnd)
{
_end = documentEnd;
}
Expand Down Expand Up @@ -1119,11 +1116,11 @@ IFACEMETHODIMP UiaTextRangeBase::MoveEndpointByUnit(_In_ TextPatternRangeEndpoin
}
CATCH_LOG();

if (bufferSize.CompareInBounds(_start, documentEnd, true) > 0)
if (_start > documentEnd)
{
_start = documentEnd;
}
if (bufferSize.CompareInBounds(_end, documentEnd, true) > 0)
if (_end > documentEnd)
{
_end = documentEnd;
}
Expand Down Expand Up @@ -1173,7 +1170,7 @@ try
const auto bufferSize = _pData->GetTextBuffer().GetSize();
const auto mine = GetEndpoint(endpoint);
const auto other = range->GetEndpoint(targetEndpoint);
RETURN_HR_IF(E_FAIL, !bufferSize.IsInBounds(mine, true) || !bufferSize.IsInBounds(other, true));
RETURN_HR_IF(E_FAIL, !bufferSize.IsInBounds(mine) || !bufferSize.IsInBounds(other));

SetEndpoint(endpoint, range->GetEndpoint(targetEndpoint));

Expand All @@ -1199,10 +1196,7 @@ try
else
{
const auto bufferSize = _pData->GetTextBuffer().GetSize();
if (!bufferSize.IsInBounds(_start, true) || !bufferSize.IsInBounds(_end, true))
{
return E_FAIL;
}
RETURN_HR_IF(E_FAIL, !bufferSize.IsInBounds(_start) || !bufferSize.IsInBounds(_end));
auto inclusiveEnd = _end;
bufferSize.DecrementInBounds(inclusiveEnd);
_pData->SelectNewRegion(_start, inclusiveEnd);
Expand Down Expand Up @@ -1516,7 +1510,7 @@ void UiaTextRangeBase::_moveEndpointByUnitWord(_In_ const int moveCount,
{
case MovementDirection::Forward:
{
if (bufferSize.CompareInBounds(nextPos, documentEnd, true) >= 0)
if (nextPos >= documentEnd)
{
success = false;
}
Expand Down Expand Up @@ -1737,7 +1731,7 @@ void UiaTextRangeBase::_moveEndpointByUnitDocument(_In_ const int moveCount,
}
CATCH_LOG();

if (preventBoundary || bufferSize.CompareInBounds(target, documentEnd, true) >= 0)
if (preventBoundary || target >= documentEnd)
{
return;
}
Expand Down
4 changes: 2 additions & 2 deletions src/types/inc/viewport.hpp
View file
Open in desktop
Expand Up @@ -55,7 +55,7 @@ namespace Microsoft::Console::Types
til::size Dimensions() const noexcept;

bool IsInBounds(const Viewport& other) const noexcept;
bool IsInBounds(const til::point pos, bool allowEndExclusive = false) const noexcept;
bool IsInBounds(const til::point pos) const noexcept;

void Clamp(til::point& pos) const;
Viewport Clamp(const Viewport& other) const noexcept;
Expand All @@ -65,7 +65,7 @@ namespace Microsoft::Console::Types
bool IncrementInBoundsCircular(til::point& pos) const noexcept;
bool DecrementInBounds(til::point& pos, bool allowEndExclusive = false) const noexcept;
bool DecrementInBoundsCircular(til::point& pos) const noexcept;
int CompareInBounds(const til::point first, const til::point second, bool allowEndExclusive = false) const noexcept;
int CompareInBounds(const til::point first, const til::point second) const noexcept;

enum class XWalk
{
Expand Down
22 changes: 5 additions & 17 deletions src/types/viewport.cpp
View file
Open in desktop
Expand Up @@ -203,18 +203,10 @@ bool Viewport::IsInBounds(const Viewport& other) const noexcept
// - Determines if the given coordinate position lies within this viewport.
// Arguments:
// - pos - Coordinate position
// - allowEndExclusive - if true, allow the EndExclusive til::point as a valid position.
// Used in accessibility to signify that the exclusive end
// includes the last til::point in a given viewport.
// Return Value:
// - True if it lies inside the viewport. False otherwise.
bool Viewport::IsInBounds(const til::point pos, bool allowEndExclusive) const noexcept
bool Viewport::IsInBounds(const til::point pos) const noexcept
{
if (allowEndExclusive && pos == EndExclusive())
{
return true;
}

lhecker marked this conversation as resolved.
Show resolved Hide resolved
return pos.X >= Left() && pos.X < RightExclusive() &&
pos.Y >= Top() && pos.Y < BottomExclusive();
}
Expand Down Expand Up @@ -355,22 +347,18 @@ bool Viewport::DecrementInBoundsCircular(til::point& pos) const noexcept
// Arguments:
// - first- The first coordinate position
// - second - The second coordinate position
// - allowEndExclusive - if true, allow the EndExclusive til::point as a valid position.
// Used in accessibility to signify that the exclusive end
// includes the last til::point in a given viewport.
// Return Value:
// - Negative if First is to the left of the Second.
// - 0 if First and Second are the same coordinate.
// - Positive if First is to the right of the Second.
// - This is so you can do s_CompareCoords(first, second) <= 0 for "first is left or the same as second".
// (the < looks like a left arrow :D)
// - The magnitude of the result is the distance between the two coordinates when typing characters into the buffer (left to right, top to bottom)
#pragma warning(suppress : 4100)
int Viewport::CompareInBounds(const til::point first, const til::point second, bool allowEndExclusive) const noexcept
int Viewport::CompareInBounds(const til::point first, const til::point second) const noexcept
{
// Assert that our coordinates are within the expected boundaries
assert(IsInBounds(first, allowEndExclusive));
assert(IsInBounds(second, allowEndExclusive));
assert(IsInBounds(first));
assert(IsInBounds(second));

// First set the distance vertically
// If first is on row 4 and second is on row 6, first will be -2 rows behind second * an 80 character row would be -160.
Expand Down Expand Up @@ -433,7 +421,7 @@ bool Viewport::WalkInBounds(til::point& pos, const WalkDir dir, bool allowEndExc
bool Viewport::WalkInBoundsCircular(til::point& pos, const WalkDir dir, bool allowEndExclusive) const noexcept
{
// Assert that the position given fits inside this viewport.
assert(IsInBounds(pos, allowEndExclusive));
assert((allowEndExclusive && pos == EndExclusive()) || IsInBounds(pos));

if (dir.x == XWalk::LeftToRight)
{
Expand Down