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

Fix and test TextBuffer::MoveToPreviousWord() #7770

Merged
4 commits merged into from
Sep 30, 2020
Merged

Fix and test TextBuffer::MoveToPreviousWord() #7770

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b02e130
Fix and test a11y movement by word
carlos-zamora Sep 28, 2020
3102da7
remove optimization for MoveToNextWord()
carlos-zamora Sep 29, 2020
9b1ee57
add a missing const
carlos-zamora Sep 29, 2020
9d07b2c
un-const
carlos-zamora Sep 29, 2020
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
39 changes: 8 additions & 31 deletions src/buffer/out/textBuffer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1254,7 +1254,7 @@ void TextBuffer::_PruneHyperlinks()
// Return Value:
// - true, if successfully updated pos. False, if we are unable to move (usually due to a buffer boundary)
// - pos - The COORD for the first character on the "word" (inclusive)
bool TextBuffer::MoveToNextWord(COORD& pos, const std::wstring_view wordDelimiters, COORD lastCharPos) const
bool TextBuffer::MoveToNextWord(COORD& pos, const std::wstring_view wordDelimiters, const COORD lastCharPos) const
{
auto copy = pos;
const auto bufferSize = GetSize();
Expand Down Expand Up @@ -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() };

// GH#7663: Treat EndExclusive as EndInclusive so
// that it actually points to a space in the buffer
if (pos == bufferSize.EndExclusive())
{
copy = { bufferSize.RightInclusive(), bufferSize.BottomInclusive() };
}

// 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;
}
}
// move to the beginning of the current word
auto copy{ GetWordStart(pos, wordDelimiters, true) };

// on a word, continue until the beginning of the word
while (_GetDelimiterClassAt(copy, wordDelimiters) == DelimiterClass::RegularChar)
if (!GetSize().DecrementInBounds(copy, true))
{
if (!bufferSize.DecrementInBounds(copy))
{
// first char in buffer is a RegularChar
// there is no previous word
return false;
}
// can't move behind current 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;
}

Expand Down
2 changes: 1 addition & 1 deletion src/buffer/out/textBuffer.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ class TextBuffer final

const COORD GetWordStart(const COORD target, const std::wstring_view wordDelimiters, bool accessibilityMode = false) const;
const COORD GetWordEnd(const COORD target, const std::wstring_view wordDelimiters, bool accessibilityMode = false) const;
bool MoveToNextWord(COORD& pos, const std::wstring_view wordDelimiters, COORD lastCharPos) const;
bool MoveToNextWord(COORD& pos, const std::wstring_view wordDelimiters, const COORD lastCharPos) const;
Copy link
Member

Choose a reason for hiding this comment

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

surprised this compiles

Copy link
Member Author

Choose a reason for hiding this comment

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

derp. Just added it to the cpp too.

Copy link
Member

Choose a reason for hiding this comment

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

The correct way to fix this would be to back out all changes to MoveToNextWord

bool MoveToPreviousWord(COORD& pos, const std::wstring_view wordDelimiters) const;

const til::point GetGlyphStart(const til::point pos) const;
Expand Down
82 changes: 82 additions & 0 deletions src/host/ut_host/TextBufferTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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 }} },
carlos-zamora marked this conversation as resolved.
Show resolved Hide resolved
{ { 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
Expand Down
42 changes: 42 additions & 0 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 @@ -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 });
}
};