Fix the location that selecting a mark uses (#17138)

I think this subtly regressed in #16611. Jump to
90b8bb7#diff-f9112caf8cb75e7a48a7b84987724d754181227385fbfcc2cc09a879b1f97c12L171-L223

`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

@@ -2614,12 +2614,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();
  }
 

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)

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,