Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 7 additions & 2 deletions src/buffer/out/search.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -119,11 +119,16 @@ bool Search::SelectCurrent() const
{
if (const auto s = GetCurrent())
{
const auto& textBuffer = _renderData->GetTextBuffer();

// GH#20152: s->end is exclusive, but SelectNewRegion expects an inclusive endpoint.
auto inclusiveEnd = s->end;
textBuffer.GetSize().DecrementInBounds(inclusiveEnd);

// Convert buffer selection offsets into the equivalent screen coordinates
// required by SelectNewRegion, taking line renditions into account.
const auto& textBuffer = _renderData->GetTextBuffer();
const auto selStart = textBuffer.BufferToScreenPosition(s->start);
const auto selEnd = textBuffer.BufferToScreenPosition(s->end);
const auto selEnd = textBuffer.BufferToScreenPosition(inclusiveEnd);
_renderData->SelectNewRegion(selStart, selEnd);
return true;
}
Expand Down
5 changes: 2 additions & 3 deletions src/buffer/out/textBuffer.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -198,9 +198,8 @@ class TextBuffer final

struct CopyRequest
{
// beg and end coordinates are inclusive
til::point beg;
til::point end;
til::point beg; // inclusive
til::point end; // exclusive
Comment on lines -201 to +202

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Investigate.

  • when did this occur? How long has this been a problem?


til::CoordType minX;
til::CoordType maxX;
Expand Down
4 changes: 2 additions & 2 deletions src/host/selection.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -520,8 +520,8 @@ void Selection::InitializeMarkSelection()
// Routine Description:
// - Resets the current selection and selects a new region from the start to end coordinates
// Arguments:
// - coordStart - Position to start selection area from
// - coordEnd - Position to select up to
// - coordStart - Position to start selection area from (inclusive)
// - coordEnd - Position to select up to (inclusive)
// Return Value:
// - <none>
void Selection::SelectNewRegion(const til::point coordStart, const til::point coordEnd)
Expand Down
12 changes: 9 additions & 3 deletions src/host/selectionInput.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -673,9 +673,11 @@ bool Selection::_HandleColorSelection(const INPUT_KEY_INFO* const pInputKeyInfo)
if (fShiftPressed)
{
// Search the selection and color *that*
// GH#20152: srSelectionRect.right is inclusive, but CopyRequest::end is
// exclusive, so it must be adjusted by one
const auto req = TextBuffer::CopyRequest::FromConfig(textBuffer,
til::point{ _d->srSelectionRect.left, _d->srSelectionRect.top },
til::point{ _d->srSelectionRect.right, _d->srSelectionRect.bottom },
til::point{ _d->srSelectionRect.right + 1, _d->srSelectionRect.bottom },
Comment on lines +676 to +680

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Investigate.

  • srSelectionRect.right is already inclusive --> right side of the character
  • CopyRequest::end expects exclusive
    Why do we need to add 1, if we're on the right side of the character already.

true /* multi-line search doesn't make sense; concatenate all lines */,
false /* we filtered out block search above */,
true /* trim block selection */,
Expand All @@ -687,15 +689,19 @@ bool Selection::_HandleColorSelection(const INPUT_KEY_INFO* const pInputKeyInfo)
const auto hits = textBuffer.SearchText(str, SearchFlag::CaseInsensitive).value_or(std::vector<til::point_span>{});
for (const auto& s : hits)
{
ColorSelection(s.start, s.end, selectionAttr);
// GH#20152: s.end is exclusive, but ColorSelection expects an inclusive endpoint.
auto inclusiveEnd = s.end;
textBuffer.GetSize().DecrementInBounds(inclusiveEnd);
ColorSelection(s.start, inclusiveEnd, selectionAttr);
}
}
else
{
const auto selection = GetSelectionSpans();
for (auto&& sp : selection)
{
sp.iterate_rows(textBuffer.GetSize().Width(), [&](til::CoordType row, til::CoordType beg, til::CoordType end) {
// GH#20152: GetSelectionSpans() returns exclusive-end spans
sp.iterate_rows_exclusive(textBuffer.GetSize().Width(), [&](til::CoordType row, til::CoordType beg, til::CoordType end) {
ColorSelection({ beg, row, end, row + 1 }, selectionAttr);
});
}
Expand Down
3 changes: 2 additions & 1 deletion src/host/ut_host/SearchTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,9 @@ class SearchTests
{
const auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();

// GH#20152: end is inclusive. Add 1 to point to the last column of the 2-wide match, not 1 past it.
auto coordEndExpected = coordStartExpected;
coordEndExpected.x += 2;
coordEndExpected.x += 1;

VERIFY_IS_TRUE(s.SelectCurrent());
VERIFY_ARE_EQUAL(coordStartExpected, gci.renderData.GetSelectionAnchor());
Expand Down
28 changes: 28 additions & 0 deletions src/host/ut_host/SelectionTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,34 @@ class SelectionTests
VERIFY_ARE_EQUAL(srOriginal.left + sDeltaLeft, srSelection.left);
VERIFY_ARE_EQUAL(srOriginal.right + sDeltaRight, srSelection.right);
}

// GH#20152: verify Alt+Shift+<digit> ("search the selection and color all matches") colors
// every character of each match, including the last one.
TEST_METHOD(TestColorSelectionSearchAndColorAllMatches)
{
auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
auto& screenInfo = gci.GetActiveOutputBuffer();
auto& textBuffer = screenInfo.GetTextBuffer();

RowWriteState state{ .text = L"foo bar foo" };
textBuffer.GetMutableRowByOffset(0).Reset(TextAttribute{});
textBuffer.Replace(0, TextAttribute{}, state);

// Select the first "foo" (columns 0-2, inclusive)
m_pSelection->SelectNewRegion({ 0, 0 }, { 2, 0 });

// Simulate Alt+Shift+'1'
INPUT_KEY_INFO keyInfo{ static_cast<WORD>('1'), LEFT_ALT_PRESSED | SHIFT_PRESSED };
VERIFY_IS_TRUE(m_pSelection->_HandleColorSelection(&keyInfo));

// Both occurrences of "foo" (columns 0-2 and 8-10) should be fully colored
for (til::CoordType x = 0; x < 15; ++x)
{
const auto isColored = textBuffer.GetCellDataAt({ x, 0 })->TextAttr() != TextAttribute{};
const auto shouldBeColored = (x >= 0 && x <= 2) || (x >= 8 && x <= 10);
VERIFY_ARE_EQUAL(shouldBeColored, isColored, NoThrowString().Format(L"column %d", x));
}
}
};

class SelectionInputTests
Expand Down
3 changes: 1 addition & 2 deletions src/types/UiaTextRangeBase.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -637,9 +637,8 @@ try
if (const auto hit = _searcher.GetCurrent())
{
hitBeg = hit->start;
// GH#20152: hit->end is already exclusive, so no conversion is needed here
hitEnd = hit->end;
// we need to increment the position of end because it's exclusive
_pData->GetTextBuffer().GetSize().IncrementInBounds(hitEnd, true);
}

if (hitBeg >= _start && hitEnd <= _end)
Expand Down
Loading