From aa74f45381dbd0e8dc4e9ecd85feba63c9b3a4c3 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 26 Apr 2024 06:12:31 -0500 Subject: [PATCH] Fix the location that selecting a mark uses I think this subtly regressed in `Terminal::SelectNewRegion` is the only thing that uses the return value from `Terminal::_ScrollToPoints`. Before that PR, `_ScrollToPoints` was just a part of `SelectNewRegion`, and it moved the start & end coords by the `_VisibleStartIndex`, not the `_scrollOffset`. Kinda weird there weren't any _other_ tests for `SelectNewRegion`? I also caught a second bug while I was here - If you had a line with an exact wrap, and tried to select that like with selectOutput, we'd explode. Closes #17131 --- src/cascadia/TerminalControl/ControlCore.cpp | 19 ++- .../TerminalCore/terminalrenderdata.cpp | 2 +- .../UnitTests_Control/ControlCoreTests.cpp | 136 ++++++++++++++++++ 3 files changed, 154 insertions(+), 3 deletions(-) diff --git a/src/cascadia/TerminalControl/ControlCore.cpp b/src/cascadia/TerminalControl/ControlCore.cpp index da46f21b655..cf2a3906f18 100644 --- a/src/cascadia/TerminalControl/ControlCore.cpp +++ b/src/cascadia/TerminalControl/ControlCore.cpp @@ -2620,12 +2620,27 @@ namespace winrt::Microsoft::Terminal::Control::implementation CompletionsChanged.raise(*this, *args); } + + // Select the region of text between [s.start, s.end), in buffer space void ControlCore::_selectSpan(til::point_span s) { + // s.end is an _exclusive_ point. We need an inclusive one. But + // decrement in bounds wants an inclusive one. If you pass an exclusive + // one, then it might assert at you for being out of bounds. So we also + // take care of the case that the end point is outside the viewport + // manually. const auto bufferSize{ _terminal->GetTextBuffer().GetSize() }; - bufferSize.DecrementInBounds(s.end); + til::point inclusiveEnd = s.end; + if (s.end.x == bufferSize.Width()) + { + inclusiveEnd = til::point{ std::max(0, s.end.x - 1), s.end.y }; + } + else + { + bufferSize.DecrementInBounds(inclusiveEnd); + } - _terminal->SelectNewRegion(s.start, s.end); + _terminal->SelectNewRegion(s.start, inclusiveEnd); _renderer->TriggerSelection(); } diff --git a/src/cascadia/TerminalCore/terminalrenderdata.cpp b/src/cascadia/TerminalCore/terminalrenderdata.cpp index ec28b59d56d..3d989f31f07 100644 --- a/src/cascadia/TerminalCore/terminalrenderdata.cpp +++ b/src/cascadia/TerminalCore/terminalrenderdata.cpp @@ -199,7 +199,7 @@ til::CoordType Terminal::_ScrollToPoints(const til::point coordStart, const til: _NotifyScrollEvent(); } - return _scrollOffset; + return _VisibleStartIndex(); } void Terminal::SelectNewRegion(const til::point coordStart, const til::point coordEnd) diff --git a/src/cascadia/UnitTests_Control/ControlCoreTests.cpp b/src/cascadia/UnitTests_Control/ControlCoreTests.cpp index aeefdca9950..63f20f9058e 100644 --- a/src/cascadia/UnitTests_Control/ControlCoreTests.cpp +++ b/src/cascadia/UnitTests_Control/ControlCoreTests.cpp @@ -40,6 +40,8 @@ namespace ControlUnitTests TEST_METHOD(TestSelectCommandSimple); TEST_METHOD(TestSelectOutputSimple); + TEST_METHOD(TestSelectOutputScrolling); + TEST_METHOD(TestSelectOutputExactWrap); TEST_METHOD(TestSimpleClickSelection); @@ -508,6 +510,140 @@ namespace ControlUnitTests } } + void ControlCoreTests::TestSelectOutputScrolling() + { + auto [settings, conn] = _createSettingsAndConnection(); + Log::Comment(L"Create ControlCore object"); + auto core = createCore(*settings, *conn); + VERIFY_IS_NOT_NULL(core); + _standardInit(core); + + Log::Comment(L"Print some text"); + + _writePrompt(conn, L"C:\\Windows"); // row 0 + conn->WriteInput(L"Foo-bar"); // row 0 + conn->WriteInput(L"\x1b]133;C\x7"); + + conn->WriteInput(L"\r\n"); + conn->WriteInput(L"This is some text \r\n"); // row 1 + conn->WriteInput(L"with varying amounts \r\n"); // row 2 + conn->WriteInput(L"of whitespace \r\n"); // row 3 + + _writePrompt(conn, L"C:\\Windows"); // row 4 + conn->WriteInput(L"gci"); + conn->WriteInput(L"\x1b]133;C\x7"); + conn->WriteInput(L"\r\n"); + + // enough to scroll + for (auto i = 0; i < 30; i++) // row 5-34 + { + conn->WriteInput(L"-a--- 2/8/2024 9:47 README\r\n"); + } + + _writePrompt(conn, L"C:\\Windows"); + + Log::Comment(L"Check the buffer contents"); + const auto& buffer = core->_terminal->GetTextBuffer(); + const auto& cursor = buffer.GetCursor(); + + { + const til::point expectedCursor{ 17, 35 }; + VERIFY_ARE_EQUAL(expectedCursor, cursor.GetPosition()); + } + + VERIFY_IS_FALSE(core->HasSelection()); + + // The second mark is the first one we'll see + core->SelectOutput(true); + VERIFY_IS_TRUE(core->HasSelection()); + { + const auto& start = core->_terminal->GetSelectionAnchor(); + const auto& end = core->_terminal->GetSelectionEnd(); + const til::point expectedStart{ 20, 4 }; // The character after the prompt + const til::point expectedEnd{ 26, 34 }; // x = the end of the text + VERIFY_ARE_EQUAL(expectedStart, start); + VERIFY_ARE_EQUAL(expectedEnd, end); + } + core->SelectOutput(true); + VERIFY_IS_TRUE(core->HasSelection()); + { + const auto& start = core->_terminal->GetSelectionAnchor(); + const auto& end = core->_terminal->GetSelectionEnd(); + const til::point expectedStart{ 24, 0 }; // The character after the prompt + const til::point expectedEnd{ 21, 3 }; // x = the end of the text + VERIFY_ARE_EQUAL(expectedStart, start); + VERIFY_ARE_EQUAL(expectedEnd, end); + } + } + + void ControlCoreTests::TestSelectOutputExactWrap() + { + // Just like the TestSelectOutputScrolling test, but these lines will + // exactly wrap to the right edge of the buffer, to catch a edge case + // present in `ControlCore::_selectSpan` + auto [settings, conn] = _createSettingsAndConnection(); + Log::Comment(L"Create ControlCore object"); + auto core = createCore(*settings, *conn); + VERIFY_IS_NOT_NULL(core); + _standardInit(core); + + Log::Comment(L"Print some text"); + + _writePrompt(conn, L"C:\\Windows"); // row 0 + conn->WriteInput(L"Foo-bar"); // row 0 + conn->WriteInput(L"\x1b]133;C\x7"); + + conn->WriteInput(L"\r\n"); + conn->WriteInput(L"This is some text \r\n"); // row 1 + conn->WriteInput(L"with varying amounts \r\n"); // row 2 + conn->WriteInput(L"of whitespace \r\n"); // row 3 + + _writePrompt(conn, L"C:\\Windows"); // row 4 + conn->WriteInput(L"gci"); + conn->WriteInput(L"\x1b]133;C\x7"); + conn->WriteInput(L"\r\n"); + + // enough to scroll + for (auto i = 0; i < 30; i++) // row 5-35 + { + conn->WriteInput(L"-a--- 2/8/2024 9:47 README.md\r\n"); + } + + _writePrompt(conn, L"C:\\Windows"); + + Log::Comment(L"Check the buffer contents"); + const auto& buffer = core->_terminal->GetTextBuffer(); + const auto& cursor = buffer.GetCursor(); + + { + const til::point expectedCursor{ 17, 35 }; + VERIFY_ARE_EQUAL(expectedCursor, cursor.GetPosition()); + } + + VERIFY_IS_FALSE(core->HasSelection()); + // The second mark is the first one we'll see + core->SelectOutput(true); + VERIFY_IS_TRUE(core->HasSelection()); + { + const auto& start = core->_terminal->GetSelectionAnchor(); + const auto& end = core->_terminal->GetSelectionEnd(); + const til::point expectedStart{ 20, 4 }; // The character after the prompt + const til::point expectedEnd{ 29, 34 }; // x = the end of the text + VERIFY_ARE_EQUAL(expectedStart, start); + VERIFY_ARE_EQUAL(expectedEnd, end); + } + core->SelectOutput(true); + VERIFY_IS_TRUE(core->HasSelection()); + { + const auto& start = core->_terminal->GetSelectionAnchor(); + const auto& end = core->_terminal->GetSelectionEnd(); + const til::point expectedStart{ 24, 0 }; // The character after the prompt + const til::point expectedEnd{ 21, 3 }; // x = the end of the text + VERIFY_ARE_EQUAL(expectedStart, start); + VERIFY_ARE_EQUAL(expectedEnd, end); + } + } + void ControlCoreTests::TestSimpleClickSelection() { // Create a simple selection with the mouse, then click somewhere else,