Improve the VT cursor movement implementation

src/host/getset.cpp

@@ -1407,67 +1407,6 @@ void DoSrvPrivateAllowCursorBlinking(SCREEN_INFORMATION& screenInfo, const bool
     return Status;
 }
 
-// Routine Description:
-// - A private API call for moving the cursor vertically in the buffer. This is
-//      because the vertical cursor movements in VT are constrained by the
-//      scroll margins, while the absolute positioning is not.
-// Parameters:
-// - screenInfo - a reference to the screen buffer we should move the cursor for
-// - lines - The number of lines to move the cursor. Up is negative, down positive.
-// Return value:
-// - S_OK if handled successfully. Otherwise an appropriate HRESULT for failing to clamp.
-[[nodiscard]] HRESULT DoSrvMoveCursorVertically(SCREEN_INFORMATION& screenInfo, const short lines)
-{
-    auto& cursor = screenInfo.GetActiveBuffer().GetTextBuffer().GetCursor();
-    COORD clampedPos = { cursor.GetPosition().X, cursor.GetPosition().Y + lines };
-
-    // Make sure the cursor doesn't move outside the viewport.
-    screenInfo.GetViewport().Clamp(clampedPos);
-
-    // Make sure the cursor stays inside the margins
-    if (screenInfo.AreMarginsSet())
-    {
-        const auto margins = screenInfo.GetAbsoluteScrollMargins().ToInclusive();
-
-        const auto cursorY = cursor.GetPosition().Y;
-
-        const auto lo = margins.Top;
-        const auto hi = margins.Bottom;
-
-        // See microsoft/terminal#2929 - If the cursor is _below_ the top
-        // margin, it should stay below the top margin. If it's _above_ the
-        // bottom, it should stay above the bottom. Cursor movements that stay
-        // outside the margins shouldn't necessarily be affected. For example,
-        // moving up while below the bottom margin shouldn't just jump straight
-        // to the bottom margin. See
-        // ScreenBufferTests::CursorUpDownOutsideMargins for a test of that
-        // behavior.
-        const bool cursorBelowTop = cursorY >= lo;
-        const bool cursorAboveBottom = cursorY <= hi;
-
-        if (cursorBelowTop)
-        {
-            try
-            {
-                clampedPos.Y = std::max(clampedPos.Y, lo);
-            }
-            CATCH_RETURN();
-        }
-
-        if (cursorAboveBottom)
-        {
-            try
-            {
-                clampedPos.Y = std::min(clampedPos.Y, hi);
-            }
-            CATCH_RETURN();
-        }
-    }
-    cursor.SetPosition(clampedPos);
-
-    return S_OK;
-}
-
 // Routine Description:
 // - A private API call for swapping to the alternate screen buffer. In virtual terminals, there exists both a "main"
 //     screen buffer and an alternate. ASBSET creates a new alternate, and switches to it. If there is an already

src/host/getset.h

@@ -34,7 +34,6 @@ void DoSrvPrivateAllowCursorBlinking(SCREEN_INFORMATION& screenInfo, const bool
 
 [[nodiscard]] NTSTATUS DoSrvPrivateSetScrollingRegion(SCREEN_INFORMATION& screenInfo, const SMALL_RECT& scrollMargins);
 [[nodiscard]] NTSTATUS DoSrvPrivateReverseLineFeed(SCREEN_INFORMATION& screenInfo);
-[[nodiscard]] HRESULT DoSrvMoveCursorVertically(SCREEN_INFORMATION& screenInfo, const short lines);
 
 [[nodiscard]] NTSTATUS DoSrvPrivateUseAlternateScreenBuffer(SCREEN_INFORMATION& screenInfo);
 void DoSrvPrivateUseMainScreenBuffer(SCREEN_INFORMATION& screenInfo);

src/host/outputStream.cpp

@@ -397,23 +397,6 @@ bool ConhostInternalGetSet::PrivateReverseLineFeed()
     return NT_SUCCESS(DoSrvPrivateReverseLineFeed(_io.GetActiveOutputBuffer()));
 }
 
-// Routine Description:
-// - Connects the MoveCursorVertically call directly into our Driver Message servicing call inside Conhost.exe
-//   MoveCursorVertically is an internal-only "API" call that the vt commands can execute,
-//     but it is not represented as a function call on out public API surface.
-// Return Value:
-// - true if successful (see DoSrvMoveCursorVertically). false otherwise.
-bool ConhostInternalGetSet::MoveCursorVertically(const ptrdiff_t lines)
-{
-    SHORT l;
-    if (FAILED(PtrdiffTToShort(lines, &l)))
-    {
-        return false;
-    }
-
-    return SUCCEEDED(DoSrvMoveCursorVertically(_io.GetActiveOutputBuffer(), l));
-}
-
 // Routine Description:
 // - Connects the SetConsoleTitleW API call directly into our Driver Message servicing call inside Conhost.exe
 // Arguments:

src/host/outputStream.hpp

@@ -100,8 +100,6 @@ class ConhostInternalGetSet final : public Microsoft::Console::VirtualTerminal::
 
     bool PrivateReverseLineFeed() override;
 
-    bool MoveCursorVertically(const ptrdiff_t lines) override;
-
     bool SetConsoleTitleW(const std::wstring_view title) override;
 
     bool PrivateUseAlternateScreenBuffer() override;

src/host/ut_host/ScreenBufferTests.cpp

@@ -199,6 +199,8 @@ class ScreenBufferTests
     TEST_METHOD(CursorUpDownOutsideMargins);
     TEST_METHOD(CursorUpDownExactlyAtMargins);
 
+    TEST_METHOD(CursorNextPreviousLine);
+
     TEST_METHOD(CursorSaveRestore);
 
     TEST_METHOD(ScreenAlignmentPattern);
@@ -5295,6 +5297,70 @@ void ScreenBufferTests::CursorUpDownExactlyAtMargins()
     stateMachine.ProcessString(L"\x1b[r");
 }
 
+void ScreenBufferTests::CursorNextPreviousLine()
+{
+    auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
+    auto& si = gci.GetActiveOutputBuffer();
+    auto& stateMachine = si.GetStateMachine();
+    auto& cursor = si.GetTextBuffer().GetCursor();
+
+    Log::Comment(L"Make sure the viewport is at 0,0");
+    VERIFY_SUCCEEDED(si.SetViewportOrigin(true, COORD({ 0, 0 }), true));
+
+    Log::Comment(L"CNL without margins");
+    // Starting from column 20 of line 10.
+    cursor.SetPosition(COORD{ 20, 10 });
+    // Move down 5 lines (CNL).
+    stateMachine.ProcessString(L"\x1b[5E");
+    // We should end up in column 0 of line 15.
+    VERIFY_ARE_EQUAL(COORD({ 0, 15 }), cursor.GetPosition());
+
+    Log::Comment(L"CPL without margins");
+    // Starting from column 20 of line 10.
+    cursor.SetPosition(COORD{ 20, 10 });
+    // Move up 5 lines (CPL).
+    stateMachine.ProcessString(L"\x1b[5F");
+    // We should end up in column 0 of line 5.
+    VERIFY_ARE_EQUAL(COORD({ 0, 5 }), cursor.GetPosition());
+
+    // Set the margins to 8:12 (9:13 in VT coordinates).
+    stateMachine.ProcessString(L"\x1b[9;13r");
+    // Make sure we clear the margins on exit so they can't break other tests.
+    auto clearMargins = wil::scope_exit([&] { stateMachine.ProcessString(L"\x1b[r"); });
+
+    Log::Comment(L"CNL inside margins");
+    // Starting from column 20 of line 10.
+    cursor.SetPosition(COORD{ 20, 10 });
+    // Move down 5 lines (CNL).
+    stateMachine.ProcessString(L"\x1b[5E");
+    // We should stop on line 12, the bottom margin.
+    VERIFY_ARE_EQUAL(COORD({ 0, 12 }), cursor.GetPosition());
+
+    Log::Comment(L"CPL inside margins");
+    // Starting from column 20 of line 10.
+    cursor.SetPosition(COORD{ 20, 10 });
+    // Move up 5 lines (CPL).
+    stateMachine.ProcessString(L"\x1b[5F");
+    // We should stop on line 8, the top margin.
+    VERIFY_ARE_EQUAL(COORD({ 0, 8 }), cursor.GetPosition());
+
+    Log::Comment(L"CNL below bottom");
+    // Starting from column 20 of line 13 (1 below bottom margin).
+    cursor.SetPosition(COORD{ 20, 13 });
+    // Move down 5 lines (CNL).
+    stateMachine.ProcessString(L"\x1b[5E");
+    // We should end up in column 0 of line 18.
+    VERIFY_ARE_EQUAL(COORD({ 0, 18 }), cursor.GetPosition());
+
+    Log::Comment(L"CPL above top margin");
+    // Starting from column 20 of line 7 (1 above top margin).
+    cursor.SetPosition(COORD{ 20, 7 });
+    // Move up 5 lines (CPL).
+    stateMachine.ProcessString(L"\x1b[5F");
+    // We should end up in column 0 of line 2.
+    VERIFY_ARE_EQUAL(COORD({ 0, 2 }), cursor.GetPosition());
+}
+
 void ScreenBufferTests::CursorSaveRestore()
 {
     auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();