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

[1.14] Remove most uses of CompareInBounds #13313

Closed
wants to merge 7 commits into from
Closed

[1.14] Remove most uses of CompareInBounds #13313

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
783b479
Remove most uses of `CompareInBounds`
carlos-zamora Jun 7, 2022
47458ae
address PR comments
carlos-zamora Jun 9, 2022
9c61dcf
clamp to inclusive pos & compare returns 0/1
carlos-zamora Jun 9, 2022
73be370
fix test
carlos-zamora Jun 9, 2022
0f04f8b
make 'MoveToNextGlyph' prettier
carlos-zamora Jun 10, 2022
70ee157
fix build
carlos-zamora Jun 16, 2022
f0e47ed
address PR feedback
carlos-zamora Jun 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 34 additions & 20 deletions src/buffer/out/textBuffer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i still don't totally get this. can't you use til::point{ target }?

{
// if at/past the limit --> clamp to limit
copy = limitOptional->to_win32_coord();
Expand Down Expand Up @@ -1257,8 +1257,8 @@ const COORD TextBuffer::GetWordEnd(const COORD target, const std::wstring_view w

// Already at/past the limit. Can't move forward.
const auto bufferSize{ GetSize() };
const auto limit{ limitOptional.value_or(til::point{ bufferSize.EndExclusive() }) };
if (bufferSize.CompareInBounds(target, limit.to_win32_coord(), true) >= 0)
const auto limit{ limitOptional.value_or(til::point{ bufferSize.Left(), bufferSize.BottomExclusive() }) };
if (til::point{ target.X, target.Y } >= limit)
{
return target;
}
Expand Down Expand Up @@ -1286,7 +1286,7 @@ const COORD TextBuffer::_GetWordEndForAccessibility(const COORD target, const st
const auto bufferSize{ GetSize() };
auto result{ target };

if (bufferSize.CompareInBounds(target, limit, true) >= 0)
if (til::point{ target.X, target.Y } >= til::point{ limit.X, limit.Y })
{
// if we're already on/past the last RegularChar,
// clamp result to that position
Expand Down Expand Up @@ -1421,14 +1421,15 @@ bool TextBuffer::MoveToNextWord(COORD& pos, const std::wstring_view wordDelimite
// This is also the inclusive start of the next word.
const auto bufferSize{ GetSize() };
const auto limit{ limitOptional.value_or(til::point{ bufferSize.EndExclusive() }) };
const auto copy{ _GetWordEndForAccessibility(pos, wordDelimiters, limit.to_win32_coord()) };
const auto copyCoord{ _GetWordEndForAccessibility(pos, wordDelimiters, limit.to_win32_coord()) };
const til::point copy{ copyCoord.X, copyCoord.Y };

if (bufferSize.CompareInBounds(copy, limit.to_win32_coord(), true) >= 0)
if (copy >= limit)
{
return false;
}

pos = copy;
pos = copy.to_win32_coord();
return true;
}

Expand Down Expand Up @@ -1470,7 +1471,8 @@ const til::point TextBuffer::GetGlyphStart(const til::point pos, std::optional<t
const auto limit{ limitOptional.value_or(til::point{ bufferSize.EndExclusive() }) };

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

// Clamp pos to limit
if (bufferSize.CompareInBounds(resultPos, limit.to_win32_coord(), true) > 0)
if (til::point{ resultPos.X, resultPos.Y } > limit)
{
resultPos = limit.to_win32_coord();
}
Expand Down Expand Up @@ -1528,9 +1530,19 @@ const til::point TextBuffer::GetGlyphEnd(const til::point pos, bool accessibilit
bool TextBuffer::MoveToNextGlyph(til::point& pos, bool allowExclusiveEnd, std::optional<til::point> limitOptional) const
{
const auto bufferSize = GetSize();
const auto limit{ limitOptional.value_or(til::point{ 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 til::point endInclusive{ bufferSize.RightInclusive(), bufferSize.BottomInclusive() };
const auto val = limitOptional.value_or(endInclusive);
Comment on lines +1539 to +1540
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old code uses bufferSize.EndExclusive() and not the "inclusive" variant?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

pastEndInclusive = val > endInclusive;
limit = pastEndInclusive ? endInclusive : val;
}

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

if (bufferSize.CompareInBounds(pos.to_win32_coord(), limit.to_win32_coord(), true) > 0)
if (pos > limit)
{
// we're past the end
// clamp us to the limit
Expand Down Expand Up @@ -1606,16 +1618,18 @@ bool TextBuffer::MoveToPreviousGlyph(til::point& pos, std::optional<til::point>
// the buffer rather than the screen.
// Return Value:
// - the delimiter class for the given char
const std::vector<SMALL_RECT> TextBuffer::GetTextRects(COORD start, COORD end, bool blockSelection, bool bufferCoordinates) const
const std::vector<SMALL_RECT> TextBuffer::GetTextRects(COORD startCoord, COORD endCoord, bool blockSelection, bool bufferCoordinates) const
{
std::vector<SMALL_RECT> textRects;

const auto bufferSize = GetSize();
til::point start{ startCoord.X, startCoord.Y };
til::point end{ endCoord.X, endCoord.Y };

// (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 All @@ -1625,19 +1639,19 @@ const std::vector<SMALL_RECT> TextBuffer::GetTextRects(COORD start, COORD end, b
{
SMALL_RECT textRow;

textRow.Top = row;
textRow.Bottom = row;
textRow.Top = base::ClampedNumeric<short>(row);
textRow.Bottom = base::ClampedNumeric<short>(row);

if (blockSelection || higherCoord.Y == lowerCoord.Y)
{
// set the left and right margin to the left-/right-most respectively
textRow.Left = std::min(higherCoord.X, lowerCoord.X);
textRow.Right = std::max(higherCoord.X, lowerCoord.X);
textRow.Left = base::ClampedNumeric<short>(std::min(higherCoord.X, lowerCoord.X));
textRow.Right = base::ClampedNumeric<short>(std::max(higherCoord.X, lowerCoord.X));
}
else
{
textRow.Left = (row == higherCoord.Y) ? higherCoord.X : bufferSize.Left();
textRow.Right = (row == lowerCoord.Y) ? lowerCoord.X : bufferSize.RightInclusive();
textRow.Left = (row == higherCoord.Y) ? base::saturated_cast<SHORT>(higherCoord.X) : bufferSize.Left();
textRow.Right = (row == lowerCoord.Y) ? base::saturated_cast<SHORT>(lowerCoord.X) : bufferSize.RightInclusive();
}

// If we were passed screen coordinates, convert the given range into
Expand Down
6 changes: 3 additions & 3 deletions src/cascadia/TerminalCore/TerminalSelection.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ void Terminal::SetSelectionEnd(const COORD viewportPos, std::optional<SelectionE
// - the new start/end for a selection
std::pair<COORD, COORD> Terminal::_PivotSelection(const COORD targetPos, bool& targetStart) const
{
if (targetStart = _activeBuffer().GetSize().CompareInBounds(targetPos, _selection->pivot) <= 0)
if (targetStart = til::point{ targetPos } <= til::point{ _selection->pivot })
{
// target is before pivot
// treat target as start
Expand Down Expand Up @@ -371,7 +371,7 @@ void Terminal::_MoveByWord(SelectionDirection direction, COORD& pos)
{
case SelectionDirection::Left:
const auto wordStartPos{ _activeBuffer().GetWordStart(pos, _wordDelimiters) };
if (_activeBuffer().GetSize().CompareInBounds(_selection->pivot, pos) < 0)
if (til::point{ _selection->pivot } < til::point{ pos })
{
// If we're moving towards the pivot, move one more cell
pos = wordStartPos;
Expand All @@ -392,7 +392,7 @@ void Terminal::_MoveByWord(SelectionDirection direction, COORD& pos)
break;
case SelectionDirection::Right:
const auto wordEndPos{ _activeBuffer().GetWordEnd(pos, _wordDelimiters) };
if (_activeBuffer().GetSize().CompareInBounds(pos, _selection->pivot) < 0)
if (til::point{ pos } < til::point{ _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
Original file line number Diff line number Diff line change
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
Loading