Improve the VT cursor movement implementation (#3628)

## Summary of the Pull Request

Originally there were 3 different methods for implementing VT cursor movement, and between them they still couldn't handle some of the operations correctly. This PR unifies those operations into a single method that can handle every type of cursor movement, and which fixes some of the issues with the existing implementations. In particular it fixes the `CNL` and `CPL` operations, so they're now correctly constrained by the `DECSTBM` margins.

## References

If this PR is accepted, the method added here should make it trivial to implement the `VPR` and `HPR` commands in issue #3428.

## PR Checklist
* [x] Closes #2926
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [x] Tests added/passed
* [ ] Requires documentation to be updated
* [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

## Detailed Description of the Pull Request / Additional comments

The new [`AdaptDispatch::_CursorMovePosition`](https://github.com/microsoft/terminal/blob/d6c4f35cf60a239cb1b8b7b7cc5796b06f78a884/src/terminal/adapter/adaptDispatch.cpp#L169) method is based on the proposal I made in issue #3428 for the `VPR` and `HPR` comands. It takes three arguments: a row offset (which can be absolute or relative), a column offset (ditto), and a flag specifying whether the position should be constrained by the `DECSTBM` margins.

To make the code more readable, I've implemented the offsets using [a `struct` with some `constexpr` helper functions for the construction](https://github.com/microsoft/terminal/blob/d6c4f35cf60a239cb1b8b7b7cc5796b06f78a884/src/terminal/adapter/adaptDispatch.hpp#L116-L125). This lets you specify the parameters with expressions like `Offset::Absolute(col)` or `Offset::Forward(distance)` which I think makes the calling code a little easier to understand.

While implementing this new method, I noticed a couple of issues in the existing movement implementations which I thought would be good to fix at the same time.

1. When cursor movement is constrained horizontally, it should be constrained by the buffer width, and not the horizontal viewport boundaries. This is an issue I've previously corrected in other parts of the codebase, and I think the cursor movement was one of the last areas where it was still a problem.

2. A number of the commands had range and overflow checks for their parameters that were either unnecessary (testing for a condition that could never occur) or incorrect (if an operation overflows, the correct behavior is to clamp it, and not just fail). The new implementation handles legitimate overflows correctly, but doesn't check for impossible ranges.

Because of the change of behavior in point 1, I also had to update the implementations of [the `DECSC` and `CPR` commands](9cf7a9b) to account for the column offset now being relative to the buffer and not the viewport, otherwise those operations would no longer work correctly.

## Validation Steps Performed

Because of the two changes in behavior mentioned above, there were a number of adapter tests that stopped working and needed to be updated. First off there were those that expected the column offset to be relative to the left viewport position and constrained by the viewport width. These now had to be updated to [use the full buffer width](49887a3) as the allowed horizontal extent.

Then there were all the overflow and out-of-range tests that were testing conditions that could never occur in practice, or where the expected behavior that was tested was actually incorrect. I did spend some time trying to see if there was value in updating these tests somehow, but in the end I decided it was best to just [drop them](6e80d0d) altogether.

For the `CNL` and `CPL` operations, there didn't appear to be any existing tests, so I added some [new screen buffer tests](d6c4f35) to check that those operations now work correctly, both with and without margins.

(cherry picked from commit 2fec178)

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();