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

Take wrapping into account when expanding wordwise selections #17170

Merged
merged 2 commits into from
May 2, 2024
Merged

Take wrapping into account when expanding wordwise selections #17170

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
97c3180
Take wrapping into account when expanding wordwise selections
DHowett May 1, 2024
175d669
okay fine
DHowett May 1, 2024
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
1 change: 1 addition & 0 deletions .github/actions/spelling/allow/allow.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ mnt
mru
nje
noreply
notwrapped
ogonek
ok'd
overlined
Expand Down
35 changes: 30 additions & 5 deletions src/buffer/out/textBuffer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1440,10 +1440,23 @@ til::point TextBuffer::_GetWordStartForSelection(const til::point target, const
// expand left until we hit the left boundary or a different delimiter class
while (result != bufferSize.Origin() && _GetDelimiterClassAt(result, wordDelimiters) == initialDelimiter)
{
//prevent selection wrapping on whitespace selection
if (isControlChar && result.x == bufferSize.Left())
if (result.x == bufferSize.Left())
{
break;
// Prevent wrapping to the previous line if the selection begins on whitespace
if (isControlChar)
{
break;
}
Comment on lines +1445 to +1449
Copy link
Member

@lhecker lhecker May 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

...but should we? If I print a b and it force-wraps in the middle, shouldn't I be able to select all 4 spaces with a double click?

Copy link
Member Author

Choose a reason for hiding this comment

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

Eh, I didn't want to revisit too many of the decisions from the previous code.

But also, this is what prevents us from selecting the entire text buffer when you double-click an empty space lmao


if (result.y > 0)
{
// Prevent wrapping to the previous line if it was hard-wrapped (e.g. not forced by us to wrap)
const auto& priorRow = GetRowByOffset(result.y - 1);
if (!priorRow.WasWrapForced())
{
break;
}
}
}
bufferSize.DecrementInBounds(result);
}
Expand Down Expand Up @@ -1563,10 +1576,22 @@ til::point TextBuffer::_GetWordEndForSelection(const til::point target, const st
// expand right until we hit the right boundary as a ControlChar or a different delimiter class
while (result != bufferSize.BottomRightInclusive() && _GetDelimiterClassAt(result, wordDelimiters) == initialDelimiter)
{
if (isControlChar && result.x == bufferSize.RightInclusive())
if (result.x == bufferSize.RightInclusive())
{
break;
// Prevent wrapping to the next line if the selection begins on whitespace
if (isControlChar)
{
break;
}

// Prevent wrapping to the next line if this one was hard-wrapped (e.g. not forced by us to wrap)
const auto& row = GetRowByOffset(result.y);
if (!row.WasWrapForced())
{
break;
}
}

bufferSize.IncrementInBoundsCircular(result);
}

Expand Down
87 changes: 56 additions & 31 deletions src/host/ut_host/TextBufferTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2336,16 +2336,32 @@ void TextBufferTests::GetWordBoundaries()
}

_buffer->Reset();
_buffer->ResizeTraditional({ 10, 5 });
_buffer->ResizeTraditional({ 10, 6 });
const std::vector<std::wstring> secondText = { L"this wordiswrapped",
L"notwrapped"
Fixed Show fixed Hide fixed
L"spaces wrapped reachEOB" };
//Buffer looks like:
// this wordi
// swrapped
// spaces
// wrappe
// d reachEOB

WriteLinesToBuffer(secondText, *_buffer);

//Buffer looks like:
// 0123456789
// 0|this wordi| < wrapped
// 1|swrapped | < not wrapped
// 2|notwrapped| < not wrapped
Fixed Show fixed Hide fixed
// 3|spaces | < wrapped
// 4| wrappe| < wrapped
// 5|d reachEOB| < wrapped

VERIFY_IS_TRUE(_buffer->GetRowByOffset(0).WasWrapForced());
VERIFY_IS_FALSE(_buffer->GetRowByOffset(1).WasWrapForced());
// GH#780 See the comment in WriteLinesToBuffer
// VERIFY_IS_FALSE(_buffer->GetRowByOffset(2).WasWrapForced());
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// VERIFY_IS_FALSE(_buffer->GetRowByOffset(2).WasWrapForced());

dead code?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, pertinent to the comment above it

_buffer->GetMutableRowByOffset(2).SetWrapForced(false); // Ugh
VERIFY_IS_TRUE(_buffer->GetRowByOffset(3).WasWrapForced());
VERIFY_IS_TRUE(_buffer->GetRowByOffset(4).WasWrapForced());
VERIFY_IS_TRUE(_buffer->GetRowByOffset(5).WasWrapForced());

// clang-format off
testData = {
{ { 0, 0 }, { { 0, 0 }, { 0, 0 } } },
{ { 1, 0 }, { { 0, 0 }, { 0, 0 } } },
Expand All @@ -2358,15 +2374,18 @@ void TextBufferTests::GetWordBoundaries()
{ { 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 } } },
{ { 9, 2 }, { { 0, 2 }, { 0, 2 } } },
// v accessibility does not consider wrapping
{ { 0, 3 }, { { 0, 3 }, { 0, 2 } } },
{ { 7, 3 }, { { 6, 3 }, { 0, 2 } } },
// v accessibility does not consider wrapping
{ { 1, 4 }, { { 0, 4 }, { 0, 2 } } },
{ { 4, 4 }, { { 4, 4 }, { 4, 4 } } },
{ { 8, 4 }, { { 4, 4 }, { 4, 4 } } },

{ { 0, 5 }, { { 4, 4 }, { 4, 4 } } },
{ { 1, 5 }, { { 1, 5 }, { 4, 4 } } },
{ { 9, 5 }, { { 2, 5 }, { 2, 5 } } },
};
for (const auto& test : testData)
{
Expand All @@ -2377,12 +2396,15 @@ void TextBufferTests::GetWordBoundaries()
}

//GetWordEnd for Wrapping Text
//Buffer looks like:
// this wordi
// swrapped
// spaces
// wrappe
// d reachEOB
// Buffer:
// 0123456789
// 0|this wordi| < wrapped
// 1|swrapped | < not wrapped
// 2|notwrapped| < not wrapped
Fixed Show fixed Hide fixed
// 3|spaces | < wrapped
// 4| wrappe| < wrapped
// 5|d reachEOB| < wrapped
// clang-format off
testData = {
// tests for first line of text
{ { 0, 0 }, { { 3, 0 }, { 5, 0 } } },
Expand All @@ -2395,17 +2417,20 @@ void TextBufferTests::GetWordBoundaries()
{ { 7, 1 }, { { 7, 1 }, { 0, 2 } } },
{ { 9, 1 }, { { 9, 1 }, { 0, 2 } } },

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

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

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

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

Expand Down
Loading