Skip to content

Commit

Permalink
Wrap word-wise selection when the word is actually wrapped (#16441)
Browse files Browse the repository at this point in the history 
Added wrapping to highlighted selection when selecting a word, added
tests for it

## Detailed Description of the Pull Request / Additional comments
- Modified GetWordStart and GetWordEnd and their helpers to no longer be
bounded by the right and left viewport ranges
- Kept same functionality (does not wrap) when selecting wrapped
whitespace
- Added tests to TextBufferTests.cpp to include cases of wrapping text

## Validation Steps Performed
- Ran locally and verified selection works properly
- Tests passed locally

Closes #4009
  • Loading branch information
@js324
js324 authored Dec 15, 2023
1 parent bc18348 commit 63c3573
Show file tree
Hide file tree
Showing 3 changed files with 128 additions and 40 deletions.
5 changes: 5 additions & 0 deletions .github/actions/spelling/expect/04cdb9b77d6827c0202f51acd4205b017015bfff.txt
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
EOB
swrapped
wordi
wordiswrapped
wrappe
63 changes: 30 additions & 33 deletions src/buffer/out/textBuffer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1329,36 +1329,32 @@ til::point TextBuffer::_GetWordStartForAccessibility(const til::point target, co
{
auto result = target;
const auto bufferSize = GetSize();
auto stayAtOrigin = false;

// ignore left boundary. Continue until readable text found
while (_GetDelimiterClassAt(result, wordDelimiters) != DelimiterClass::RegularChar)
{
if (!bufferSize.DecrementInBounds(result))
if (result == bufferSize.Origin())
{
// first char in buffer is a DelimiterChar or ControlChar
// we can't move any further back
stayAtOrigin = true;
break;
//looped around and hit origin (no word between origin and target)
return result;
}
bufferSize.DecrementInBounds(result);
}

// make sure we expand to the left boundary or the beginning of the word
while (_GetDelimiterClassAt(result, wordDelimiters) == DelimiterClass::RegularChar)
{
if (!bufferSize.DecrementInBounds(result))
if (result == bufferSize.Origin())
{
// first char in buffer is a RegularChar
// we can't move any further back
break;
return result;
}
bufferSize.DecrementInBounds(result);
}

// move off of delimiter and onto word start
if (!stayAtOrigin && _GetDelimiterClassAt(result, wordDelimiters) != DelimiterClass::RegularChar)
{
bufferSize.IncrementInBounds(result);
}
// move off of delimiter
bufferSize.IncrementInBounds(result);

return result;
}
Expand All @@ -1376,10 +1372,16 @@ til::point TextBuffer::_GetWordStartForSelection(const til::point target, const
const auto bufferSize = GetSize();

const auto initialDelimiter = _GetDelimiterClassAt(result, wordDelimiters);
const bool isControlChar = initialDelimiter == DelimiterClass::ControlChar;

// expand left until we hit the left boundary or a different delimiter class
while (result.x > bufferSize.Left() && (_GetDelimiterClassAt(result, wordDelimiters) == initialDelimiter))
while (result != bufferSize.Origin() && _GetDelimiterClassAt(result, wordDelimiters) == initialDelimiter)
{
//prevent selection wrapping on whitespace selection
if (isControlChar && result.x == bufferSize.Left())
{
break;
}
bufferSize.DecrementInBounds(result);
}

Expand Down Expand Up @@ -1457,25 +1459,21 @@ til::point TextBuffer::_GetWordEndForAccessibility(const til::point target, cons
}
else
{
auto iter{ GetCellDataAt(result, bufferSize) };
while (iter && iter.Pos() != limit && _GetDelimiterClassAt(iter.Pos(), wordDelimiters) == DelimiterClass::RegularChar)
while (result != limit && result != bufferSize.BottomRightInclusive() && _GetDelimiterClassAt(result, wordDelimiters) == DelimiterClass::RegularChar)
{
// Iterate through readable text
++iter;
bufferSize.IncrementInBounds(result);
}

while (iter && iter.Pos() != limit && _GetDelimiterClassAt(iter.Pos(), wordDelimiters) != DelimiterClass::RegularChar)
while (result != limit && result != bufferSize.BottomRightInclusive() && _GetDelimiterClassAt(result, wordDelimiters) != DelimiterClass::RegularChar)
{
// expand to the beginning of the NEXT word
++iter;
bufferSize.IncrementInBounds(result);
}

result = iter.Pos();

// Special case: we tried to move one past the end of the buffer,
// but iter prevented that (because that pos doesn't exist).
// Special case: we tried to move one past the end of the buffer
// Manually increment onto the EndExclusive point.
if (!iter)
if (result == bufferSize.BottomRightInclusive())
{
bufferSize.IncrementInBounds(result, true);
}
Expand All @@ -1495,19 +1493,18 @@ til::point TextBuffer::_GetWordEndForSelection(const til::point target, const st
{
const auto bufferSize = GetSize();

// can't expand right
if (target.x == bufferSize.RightInclusive())
{
return target;
}

auto result = target;
const auto initialDelimiter = _GetDelimiterClassAt(result, wordDelimiters);
const bool isControlChar = initialDelimiter == DelimiterClass::ControlChar;

// expand right until we hit the right boundary or a different delimiter class
while (result.x < bufferSize.RightInclusive() && (_GetDelimiterClassAt(result, wordDelimiters) == initialDelimiter))
// expand right until we hit the right boundary as a ControlChar or a different delimiter class
while (result != bufferSize.BottomRightInclusive() && _GetDelimiterClassAt(result, wordDelimiters) == initialDelimiter)
{
bufferSize.IncrementInBounds(result);
if (isControlChar && result.x == bufferSize.RightInclusive())
{
break;
}
bufferSize.IncrementInBoundsCircular(result);
}

if (_GetDelimiterClassAt(result, wordDelimiters) != initialDelimiter)
Expand Down
100 changes: 93 additions & 7 deletions src/host/ut_host/TextBufferTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2118,10 +2118,11 @@ void TextBufferTests::TestAppendRTFText()
void TextBufferTests::WriteLinesToBuffer(const std::vector<std::wstring>& text, TextBuffer& buffer)
{
const auto bufferSize = buffer.GetSize();

int rowsWrapped{};
for (size_t row = 0; row < text.size(); ++row)
{
auto line = text[row];

if (!line.empty())
{
// TODO GH#780: writing up to (but not past) the end of the line
Expand All @@ -2133,7 +2134,12 @@ void TextBufferTests::WriteLinesToBuffer(const std::vector<std::wstring>& text,
}

OutputCellIterator iter{ line };
buffer.Write(iter, { 0, gsl::narrow<til::CoordType>(row) }, wrap);
buffer.Write(iter, { 0, gsl::narrow<til::CoordType>(row + rowsWrapped) }, wrap);
//prevent bug that overwrites wrapped rows
if (line.size() > static_cast<size_t>(bufferSize.RightExclusive()))
{
rowsWrapped += static_cast<int>(line.size()) / bufferSize.RightExclusive();
}
}
}
}
Expand Down Expand Up @@ -2245,6 +2251,88 @@ void TextBufferTests::GetWordBoundaries()
const auto expected = accessibilityMode ? test.expected.accessibilityModeEnabled : test.expected.accessibilityModeDisabled;
VERIFY_ARE_EQUAL(expected, result);
}

_buffer->Reset();
_buffer->ResizeTraditional({ 10, 5 });
const std::vector<std::wstring> secondText = { L"this wordiswrapped",
L"spaces wrapped reachEOB" };
//Buffer looks like:
// this wordi
// swrapped
// spaces
// wrappe
// d reachEOB
WriteLinesToBuffer(secondText, *_buffer);
testData = {
{ { 0, 0 }, { { 0, 0 }, { 0, 0 } } },
{ { 1, 0 }, { { 0, 0 }, { 0, 0 } } },
{ { 4, 0 }, { { 4, 0 }, { 0, 0 } } },
{ { 5, 0 }, { { 5, 0 }, { 5, 0 } } },
{ { 7, 0 }, { { 5, 0 }, { 5, 0 } } },

{ { 4, 1 }, { { 5, 0 }, { 5, 0 } } },
{ { 7, 1 }, { { 5, 0 }, { 5, 0 } } },
{ { 9, 1 }, { { 8, 1 }, { 5, 0 } } },

{ { 0, 2 }, { { 0, 2 }, { 0, 2 } } },
{ { 7, 2 }, { { 6, 2 }, { 0, 2 } } },

{ { 1, 3 }, { { 0, 3 }, { 0, 2 } } },
{ { 4, 3 }, { { 4, 3 }, { 4, 3 } } },
{ { 8, 3 }, { { 4, 3 }, { 4, 3 } } },

{ { 0, 4 }, { { 4, 3 }, { 4, 3 } } },
{ { 1, 4 }, { { 1, 4 }, { 4, 3 } } },
{ { 9, 4 }, { { 2, 4 }, { 2, 4 } } },
};
for (const auto& test : testData)
{
Log::Comment(NoThrowString().Format(L"Testing til::point (%hd, %hd)", test.startPos.x, test.startPos.y));
const auto result = _buffer->GetWordStart(test.startPos, delimiters, accessibilityMode);
const auto expected = accessibilityMode ? test.expected.accessibilityModeEnabled : test.expected.accessibilityModeDisabled;
VERIFY_ARE_EQUAL(expected, result);
}

//GetWordEnd for Wrapping Text
//Buffer looks like:
// this wordi
// swrapped
// spaces
// wrappe
// d reachEOB
testData = {
// tests for first line of text
{ { 0, 0 }, { { 3, 0 }, { 5, 0 } } },
{ { 1, 0 }, { { 3, 0 }, { 5, 0 } } },
{ { 4, 0 }, { { 4, 0 }, { 5, 0 } } },
{ { 5, 0 }, { { 7, 1 }, { 0, 2 } } },
{ { 7, 0 }, { { 7, 1 }, { 0, 2 } } },

{ { 4, 1 }, { { 7, 1 }, { 0, 2 } } },
{ { 7, 1 }, { { 7, 1 }, { 0, 2 } } },
{ { 9, 1 }, { { 9, 1 }, { 0, 2 } } },

{ { 0, 2 }, { { 5, 2 }, { 4, 3 } } },
{ { 7, 2 }, { { 9, 2 }, { 4, 3 } } },

{ { 1, 3 }, { { 3, 3 }, { 4, 3 } } },
{ { 4, 3 }, { { 0, 4 }, { 2, 4 } } },
{ { 8, 3 }, { { 0, 4 }, { 2, 4 } } },

{ { 0, 4 }, { { 0, 4 }, { 2, 4 } } },
{ { 1, 4 }, { { 1, 4 }, { 2, 4 } } },
{ { 4, 4 }, { { 9, 4 }, { 0, 5 } } },
{ { 9, 4 }, { { 9, 4 }, { 0, 5 } } },
};
// clang-format on

for (const auto& test : testData)
{
Log::Comment(NoThrowString().Format(L"TestEnd til::point (%hd, %hd)", test.startPos.x, test.startPos.y));
auto result = _buffer->GetWordEnd(test.startPos, delimiters, accessibilityMode);
const auto expected = accessibilityMode ? test.expected.accessibilityModeEnabled : test.expected.accessibilityModeDisabled;
VERIFY_ARE_EQUAL(expected, result);
}
}

void TextBufferTests::MoveByWord()
Expand Down Expand Up @@ -2571,9 +2659,7 @@ void TextBufferTests::GetText()

// Setup: Write lines of text to the buffer
const std::vector<std::wstring> bufferText = { L"1234567",
L"",
L" 345",
L"123 ",
L" 345123 ",
L"" };
WriteLinesToBuffer(bufferText, *_buffer);
// buffer should look like this:
Expand Down Expand Up @@ -2653,15 +2739,15 @@ void TextBufferTests::GetText()
Log::Comment(L"Standard Copy to Clipboard");
expectedText += L"12345";
expectedText += L"67\r\n";
expectedText += L" 345\r\n";
expectedText += L" 345";
expectedText += L"123 \r\n";
}
else
{
Log::Comment(L"UI Automation");
expectedText += L"12345";
expectedText += L"67 \r\n";
expectedText += L" 345\r\n";
expectedText += L" 345";
expectedText += L"123 ";
expectedText += L" \r\n";
expectedText += L" ";
Expand Down

0 comments on commit 63c3573

Please sign in to comment.