From 63c3573a13cc96bfa3d655f0f1898ae250e2fe35 Mon Sep 17 00:00:00 2001 From: js324 Date: Fri, 15 Dec 2023 18:50:45 -0500 Subject: [PATCH] Wrap word-wise selection when the word is actually wrapped (#16441) 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 --- ...cdb9b77d6827c0202f51acd4205b017015bfff.txt | 5 + src/buffer/out/textBuffer.cpp | 63 ++++++----- src/host/ut_host/TextBufferTests.cpp | 100 ++++++++++++++++-- 3 files changed, 128 insertions(+), 40 deletions(-) create mode 100644 .github/actions/spelling/expect/04cdb9b77d6827c0202f51acd4205b017015bfff.txt diff --git a/.github/actions/spelling/expect/04cdb9b77d6827c0202f51acd4205b017015bfff.txt b/.github/actions/spelling/expect/04cdb9b77d6827c0202f51acd4205b017015bfff.txt new file mode 100644 index 00000000000..f117f5081da --- /dev/null +++ b/.github/actions/spelling/expect/04cdb9b77d6827c0202f51acd4205b017015bfff.txt @@ -0,0 +1,5 @@ +EOB +swrapped +wordi +wordiswrapped +wrappe diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index 52a5c6e3b72..528d8fbf4d3 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -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; } @@ -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); } @@ -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); } @@ -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) diff --git a/src/host/ut_host/TextBufferTests.cpp b/src/host/ut_host/TextBufferTests.cpp index d1c7395635f..9b330e91d86 100644 --- a/src/host/ut_host/TextBufferTests.cpp +++ b/src/host/ut_host/TextBufferTests.cpp @@ -2118,10 +2118,11 @@ void TextBufferTests::TestAppendRTFText() void TextBufferTests::WriteLinesToBuffer(const std::vector& 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 @@ -2133,7 +2134,12 @@ void TextBufferTests::WriteLinesToBuffer(const std::vector& text, } OutputCellIterator iter{ line }; - buffer.Write(iter, { 0, gsl::narrow(row) }, wrap); + buffer.Write(iter, { 0, gsl::narrow(row + rowsWrapped) }, wrap); + //prevent bug that overwrites wrapped rows + if (line.size() > static_cast(bufferSize.RightExclusive())) + { + rowsWrapped += static_cast(line.size()) / bufferSize.RightExclusive(); + } } } } @@ -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 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() @@ -2571,9 +2659,7 @@ void TextBufferTests::GetText() // Setup: Write lines of text to the buffer const std::vector bufferText = { L"1234567", - L"", - L" 345", - L"123 ", + L" 345123 ", L"" }; WriteLinesToBuffer(bufferText, *_buffer); // buffer should look like this: @@ -2653,7 +2739,7 @@ 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 @@ -2661,7 +2747,7 @@ void TextBufferTests::GetText() 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" ";