Repository: microsoft/terminal (language: cpp)

Issue:
[1.21] Selecting commands, output with the context menu is totally in the wrong place
open pwsh, with shell integration enabled

do a 

```
cd Z:\dev\public\OpenConsole\
git status
dir
foo
whatever
```

try and right-click select the output of `dir`, and it's... wrong:

![image](https://github.com/microsoft/terminal/assets/18356694/99bee5a6-5e62-4d8a-8aea-b7d8acf018f6)


surely, I'm the murderer, but I could have swore there were tests for this



Discussion / hints:
https://github.com/microsoft/terminal/blame/f36d589a8eb60fe9ff4c8efaad8eac088647e8a8/src/cascadia/TerminalCore/terminalrenderdata.cpp#L205-L214

or prior:

https://github.com/microsoft/terminal/blame/d632c39cc3cef3075f09d881e03801518310d641/src/cascadia/TerminalCore/terminalrenderdata.cpp#L171-L207

Just staring at that in isolation, it's not clear to me what coordinate system the coord params of 

```c++
SelectNewRegion(const til::point coordStart, const til::point coordEnd)
```

are
```diff
diff --git a/src/cascadia/TerminalCore/terminalrenderdata.cpp b/src/cascadia/TerminalCore/terminalrenderdata.cpp
index ec28b59d5..3d989f31f 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)
```

this fixes it. Need to write a test

Fix patch:
diff --git a/src/cascadia/TerminalControl/ControlCore.cpp b/src/cascadia/TerminalControl/ControlCore.cpp
index 7e2d13ee7d1..3bb3248053f 100644
--- a/src/cascadia/TerminalControl/ControlCore.cpp
+++ b/src/cascadia/TerminalControl/ControlCore.cpp
@@ -2625,12 +2625,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,


Test patch: