maxmustermann/terminal
1
0
Fork 0
Code Issues Pull requests Projects Releases Wiki Activity

Fix and test TextBuffer::MoveToPreviousWord() (#7770)

Browse source 
This fixes a bug when moving backwards by word that resulted in #7742.

This also includes...
- a minor refactor that leverages `GetWordStart` in `MoveToPreviousWord`
- additional unit tests for movement by word
- a feature test comprised of the referenced bug report

`MoveToPreviousWord()` would...
- move backwards for each whitespace character
- then, move backwards for each regular character

This would actually result in moving to the beginning of the current "word" (as defined by a11y).

We actually need to do this process twice:
- the first time gets you to the beginning of the current word
- attempt to move back by one character
- the second time gets you to the beginning of the previous word

Rather than implementing 4 while loops, we leverage `GetWordStart()` to
attempt to move to the beginning of the previous word. We call it twice
(as described above). The logic is unchanged, but we instead reuse a
function that has already undergone more testing.

To make sure this works as expected, additional unit tests were
introduced covering "MoveByWord" in the TextBuffer.

## Validation Steps Performed
Added test for repro steps.
Added unit tests for movement by word.

Closes #7742
This commit is contained in:
Carlos Zamora 2020-09-30 11:13:22 -07:00 committed by GitHub
parent 6f051140da
commit 9ec57a7d3c
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
3 changed files with 131 additions and 30 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

37
src/buffer/out/textBuffer.cpp
View file

@ -1308,40 +1308,17 @@ bool TextBuffer::MoveToNextWord(COORD& pos, const std::wstring_view wordDelimite
// - pos - The COORD for the first character on the "word" (inclusive)
bool TextBuffer::MoveToPreviousWord(COORD& pos, std::wstring_view wordDelimiters) const
{
auto copy = pos;
const auto bufferSize{ GetSize() };
// move to the beginning of the current word
auto copy{ GetWordStart(pos, wordDelimiters, true) };
// GH#7663: Treat EndExclusive as EndInclusive so
// that it actually points to a space in the buffer
if (pos == bufferSize.EndExclusive())
if (!GetSize().DecrementInBounds(copy, true))
{
copy = { bufferSize.RightInclusive(), bufferSize.BottomInclusive() };
// can't move behind current word
return false;
}
// started on whitespace/delimiter, continue until the end of the previous word
while (_GetDelimiterClassAt(copy, wordDelimiters) != DelimiterClass::RegularChar)
{
if (!bufferSize.DecrementInBounds(copy))
{
// first char in buffer is a DelimiterChar or ControlChar
// there is no previous word
return false;
}
}
// on a word, continue until the beginning of the word
while (_GetDelimiterClassAt(copy, wordDelimiters) == DelimiterClass::RegularChar)
{
if (!bufferSize.DecrementInBounds(copy))
{
// first char in buffer is a RegularChar
// there is no previous word
return false;
}
}
// successful move, copy result out
pos = copy;
// move to the beginning of the previous word
pos = GetWordStart(copy, wordDelimiters, true);
return true;
}

82
src/host/ut_host/TextBufferTests.cpp
View file

@ -148,6 +148,7 @@ class TextBufferTests
void WriteLinesToBuffer(const std::vector<std::wstring>& text, TextBuffer& buffer);
TEST_METHOD(GetWordBoundaries);
TEST_METHOD(MoveByWord);
TEST_METHOD(GetGlyphBoundaries);
TEST_METHOD(GetTextRects);
@ -2132,6 +2133,87 @@ void TextBufferTests::GetWordBoundaries()
}
}
void TextBufferTests::MoveByWord()
{
COORD bufferSize{ 80, 9001 };
UINT cursorSize = 12;
TextAttribute attr{ 0x7f };
auto _buffer = std::make_unique<TextBuffer>(bufferSize, attr, cursorSize, _renderTarget);
// Setup: Write lines of text to the buffer
const std::vector<std::wstring> text = { L"word other",
L" more words" };
WriteLinesToBuffer(text, *_buffer);
// Test Data:
// - COORD - starting position
// - COORD - expected result (moving forwards)
// - COORD - expected result (moving backwards)
struct ExpectedResult
{
COORD moveForwards;
COORD moveBackwards;
};
struct Test
{
COORD startPos;
ExpectedResult expected;
};
// Set testData for GetWordStart tests
// clang-format off
std::vector<Test> testData = {
// tests for first line of text
{ { 0, 0 }, {{ 5, 0 }, { 0, 0 }} },
{ { 1, 0 }, {{ 5, 0 }, { 1, 0 }} },
{ { 3, 0 }, {{ 5, 0 }, { 3, 0 }} },
{ { 4, 0 }, {{ 5, 0 }, { 4, 0 }} },
{ { 5, 0 }, {{ 2, 1 }, { 0, 0 }} },
{ { 6, 0 }, {{ 2, 1 }, { 0, 0 }} },
{ { 20, 0 }, {{ 2, 1 }, { 0, 0 }} },
{ { 79, 0 }, {{ 2, 1 }, { 0, 0 }} },
// tests for second line of text
{ { 0, 1 }, {{ 2, 1 }, { 0, 0 }} },
{ { 1, 1 }, {{ 2, 1 }, { 0, 0 }} },
{ { 2, 1 }, {{ 9, 1 }, { 5, 0 }} },
{ { 3, 1 }, {{ 9, 1 }, { 5, 0 }} },
{ { 5, 1 }, {{ 9, 1 }, { 5, 0 }} },
{ { 6, 1 }, {{ 9, 1 }, { 5, 0 }} },
{ { 7, 1 }, {{ 9, 1 }, { 5, 0 }} },
{ { 9, 1 }, {{ 9, 1 }, { 2, 1 }} },
{ { 10, 1 }, {{10, 1 }, { 2, 1 }} },
{ { 20, 1 }, {{20, 1 }, { 2, 1 }} },
{ { 79, 1 }, {{79, 1 }, { 2, 1 }} },
};
// clang-format on
BEGIN_TEST_METHOD_PROPERTIES()
TEST_METHOD_PROPERTY(L"Data:movingForwards", L"{false, true}")
END_TEST_METHOD_PROPERTIES();
bool movingForwards;
VERIFY_SUCCEEDED(TestData::TryGetValue(L"movingForwards", movingForwards), L"Get movingForwards variant");
const std::wstring_view delimiters = L" ";
const COORD lastCharPos = _buffer->GetLastNonSpaceCharacter();
for (const auto& test : testData)
{
Log::Comment(NoThrowString().Format(L"COORD (%hd, %hd)", test.startPos.X, test.startPos.Y));
auto pos{ test.startPos };
const auto result = movingForwards ?
_buffer->MoveToNextWord(pos, delimiters, lastCharPos) :
_buffer->MoveToPreviousWord(pos, delimiters);
const auto expected = movingForwards ? test.expected.moveForwards : test.expected.moveBackwards;
VERIFY_ARE_EQUAL(expected, pos);
// if we moved, result is true and pos != startPos.
// otherwise, result is false and pos == startPos.
VERIFY_ARE_EQUAL(result, pos != test.startPos);
}
}
void TextBufferTests::GetGlyphBoundaries()
{
struct ExpectedResult

42
src/interactivity/win32/ut_interactivity_win32/UiaTextRangeTests.cpp
View file

@ -1190,4 +1190,46 @@ class UiaTextRangeTests
VERIFY_ARE_EQUAL(-1, moveAmt);
}
}
TEST_METHOD(MoveToPreviousWord)
{
// See GH#7742 for more details.
const auto bufferSize{ _pTextBuffer->GetSize() };
const COORD origin{ bufferSize.Origin() };
const COORD originExclusive{ origin.X, origin.Y + 1 };
_pTextBuffer->Write({ L"My name is Carlos" }, origin);
// Create degenerate UTR at origin
Microsoft::WRL::ComPtr<UiaTextRange> utr;
THROW_IF_FAILED(Microsoft::WRL::MakeAndInitialize<UiaTextRange>(&utr, _pUiaData, &_dummyProvider, origin, origin));
// move forward by a word
int moveAmt;
THROW_IF_FAILED(utr->Move(TextUnit::TextUnit_Word, 1, &moveAmt));
VERIFY_ARE_EQUAL(1, moveAmt);
VERIFY_IS_TRUE(utr->IsDegenerate());
// Expand by word
BSTR text;
THROW_IF_FAILED(utr->ExpandToEnclosingUnit(TextUnit::TextUnit_Word));
THROW_IF_FAILED(utr->GetText(-1, &text));
VERIFY_ARE_EQUAL(L"name ", std::wstring_view{ text });
// Collapse utr (move end to start)
const COORD expectedStart{ 3, 0 };
THROW_IF_FAILED(utr->MoveEndpointByRange(TextPatternRangeEndpoint::TextPatternRangeEndpoint_End, utr.Get(), TextPatternRangeEndpoint::TextPatternRangeEndpoint_Start));
VERIFY_ARE_EQUAL(expectedStart, utr->_start);
VERIFY_IS_TRUE(utr->IsDegenerate());
// Move back by a word
THROW_IF_FAILED(utr->Move(TextUnit::TextUnit_Word, -1, &moveAmt));
VERIFY_ARE_EQUAL(-1, moveAmt);
// Expand by character
THROW_IF_FAILED(utr->ExpandToEnclosingUnit(TextUnit::TextUnit_Character));
THROW_IF_FAILED(utr->GetText(-1, &text));
VERIFY_ARE_EQUAL(L"M", std::wstring_view{ text });
}
};