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 1 commit
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
100 changes: 29 additions & 71 deletions src/buffer/out/textBuffer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1118,15 +1118,10 @@ const COORD TextBuffer::GetWordEnd(const COORD target, const std::wstring_view w
// so the words in the example include ["word ", "other "]
// NOTE: the end anchor (this one) is exclusive, whereas the start anchor (GetWordStart) is inclusive

// Already at the end. Can't move forward.
if (target == GetSize().EndExclusive())
{
return target;
}

if (accessibilityMode)
{
return _GetWordEndForAccessibility(target, wordDelimiters);
const auto lastCharPos{ GetLastNonSpaceCharacter() };
return _GetWordEndForAccessibility(target, wordDelimiters, lastCharPos);
}
else
{
Expand All @@ -1139,13 +1134,20 @@ const COORD TextBuffer::GetWordEnd(const COORD target, const std::wstring_view w
// Arguments:
// - target - a COORD on the word you are currently on
// - wordDelimiters - what characters are we considering for the separation of words
// - lastCharPos - the position of the last nonspace character in the text buffer (to improve performance)
// Return Value:
// - The COORD for the first character of the next readable "word". If no next word, return one past the end of the buffer
const COORD TextBuffer::_GetWordEndForAccessibility(const COORD target, const std::wstring_view wordDelimiters) const
const COORD TextBuffer::_GetWordEndForAccessibility(const COORD target, const std::wstring_view wordDelimiters, const COORD lastCharPos) const
{
const auto bufferSize = GetSize();
COORD result = target;

// Check if we're already on/past the last RegularChar
if (bufferSize.CompareInBounds(result, lastCharPos, true) >= 0)
{
return bufferSize.EndExclusive();
}

// ignore right boundary. Continue through readable text found
while (_GetDelimiterClassAt(result, wordDelimiters) == DelimiterClass::RegularChar)
{
Expand All @@ -1155,6 +1157,12 @@ const COORD TextBuffer::_GetWordEndForAccessibility(const COORD target, const st
}
}

// we are already on/past the last RegularChar
if (bufferSize.CompareInBounds(result, lastCharPos, true) >= 0)
{
return bufferSize.EndExclusive();
}

// make sure we expand to the beginning of the NEXT word
while (_GetDelimiterClassAt(result, wordDelimiters) != DelimiterClass::RegularChar)
{
Expand Down Expand Up @@ -1256,44 +1264,17 @@ void TextBuffer::_PruneHyperlinks()
// - pos - The COORD for the first character on the "word" (inclusive)
bool TextBuffer::MoveToNextWord(COORD& pos, const std::wstring_view wordDelimiters, COORD lastCharPos) const
{
auto copy = pos;
const auto bufferSize = GetSize();
// move to the beginning of the next word
// NOTE: _GetWordEnd...() returns the exclusive position of the "end of the word"
// This is also the inclusive start of the next word.
auto copy{ _GetWordEndForAccessibility(pos, wordDelimiters, lastCharPos) };

// Already at the end. Can't move forward.
if (pos == bufferSize.EndExclusive())
if (copy == GetSize().EndExclusive())
{
// there is no next word
return false;
}

// started on a word, continue until the end of the word
while (_GetDelimiterClassAt(copy, wordDelimiters) == DelimiterClass::RegularChar)
{
if (!bufferSize.IncrementInBounds(copy))
{
// last char in buffer is a RegularChar
// thus there is no next word
return false;
}
}

// we are already on/past the last RegularChar
if (bufferSize.CompareInBounds(copy, lastCharPos) >= 0)
{
return false;
}

// on whitespace, continue until the beginning of the next word
while (_GetDelimiterClassAt(copy, wordDelimiters) != DelimiterClass::RegularChar)
{
if (!bufferSize.IncrementInBounds(copy))
{
// last char in buffer is a DelimiterChar or ControlChar
// there is no next word
return false;
}
}

// successful move, copy result out
pos = copy;
return true;
}
Expand All @@ -1308,40 +1289,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
4 changes: 2 additions & 2 deletions 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 Expand Up @@ -224,7 +224,7 @@ class TextBuffer final
const DelimiterClass _GetDelimiterClassAt(const COORD pos, const std::wstring_view wordDelimiters) const;
const COORD _GetWordStartForAccessibility(const COORD target, const std::wstring_view wordDelimiters) const;
const COORD _GetWordStartForSelection(const COORD target, const std::wstring_view wordDelimiters) const;
const COORD _GetWordEndForAccessibility(const COORD target, const std::wstring_view wordDelimiters) const;
const COORD _GetWordEndForAccessibility(const COORD target, const std::wstring_view wordDelimiters, COORD lastCharPos) const;
const COORD _GetWordEndForSelection(const COORD target, const std::wstring_view wordDelimiters) const;

void _PruneHyperlinks();
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 });
}
};