Repository: microsoft/terminal (language: cpp)

Issue:
Update our DECSC/DECRC implementation to match the latest DEC algorithm
# Description of the new feature/enhancement

When origin mode (`DECOM`) is enabled, the cursor is supposed to be constrained within the boundaries of the scrolling margins (`DECSTBM`). But consider what happens when you save the cursor position (with `DECSC`), and then update the margins so that saved position would now be out of bounds.

For example, say the top margin is row 1, the cursor position is row 5. After saving the cursor position, we then update the top margin to row 10. On what row would you expect the cursor to restore?

On the early DEC terminals, the cursor would be restored to its original position (row 5), which is now 5 rows above the top margin. This is not a legal position when origin mode is enabled, and if you requested the coordinates with a `DSR-CPR` query, you'd get a nonsense result with random characters representing the negative coordinates.

Up to now we've avoided this bug by clamping the coordinates within the margin boundaries, but the way DEC ultimately addressed the issue was by using relative coordinates. So in the example above, the start row of 5 is offset 4 from the top margin, and thus the restored row becomes 14 (offset 4 from the new margin of 10).

So I would like to propose we update our implementation to match this later DEC algorithm, since that seems to be the most correct. You can see a demonstration of how this was handled on a range of different DEC terminals in issue https://github.com/microsoft/terminal/issues/14876#issuecomment-1459325217.

# Proposed technical implementation details (optional)

This would actually be somewhat simpler than our current implementation. There's an extra step when saving the cursor to subtract the offset of the top margin (assuming the origin mode is set). But then when restoring the cursor, we can just pass the coordinates straight to the `CursorPosition` method without any additional adjustments (which are required in the current implementation).


Discussion / hints:

Fix patch:
diff --git a/src/host/ut_host/ScreenBufferTests.cpp b/src/host/ut_host/ScreenBufferTests.cpp
index bf3943d9e75..9539cfb17b4 100644
--- a/src/host/ut_host/ScreenBufferTests.cpp
+++ b/src/host/ut_host/ScreenBufferTests.cpp
@@ -6349,17 +6349,17 @@ void ScreenBufferTests::CursorSaveRestore()
     // Verify home position inside margins, i.e. relative origin mode restored.
     VERIFY_ARE_EQUAL(til::point(0, 9), cursor.GetPosition());
 
-    Log::Comment(L"Clamp inside top margin.");
+    Log::Comment(L"Restore relative to new origin.");
     // Reset margins, with absolute origin, and set cursor position.
     stateMachine.ProcessString(L"\x1b[r");
     stateMachine.ProcessString(setDECOM);
-    cursor.SetPosition({ 5, 15 });
+    cursor.SetPosition({ 5, 5 });
     // Save state.
     stateMachine.ProcessString(saveCursor);
     // Set margins and restore state.
-    stateMachine.ProcessString(L"\x1b[20;25r");
+    stateMachine.ProcessString(L"\x1b[15;25r");
     stateMachine.ProcessString(restoreCursor);
-    // Verify Y position is clamped inside the top margin
+    // Verify Y position is now relative to new top margin
     VERIFY_ARE_EQUAL(til::point(5, 19), cursor.GetPosition());
 
     Log::Comment(L"Clamp inside bottom margin.");
diff --git a/src/terminal/adapter/adaptDispatch.cpp b/src/terminal/adapter/adaptDispatch.cpp
index 9dcc434bbde..ddf9d1bb6eb 100644
--- a/src/terminal/adapter/adaptDispatch.cpp
+++ b/src/terminal/adapter/adaptDispatch.cpp
@@ -438,6 +438,12 @@ bool AdaptDispatch::CursorSaveState()
     auto cursorPosition = textBuffer.GetCursor().GetPosition();
     cursorPosition.y -= viewport.top;
 
+    // Although if origin mode is set, the cursor is relative to the margin origin.
+    if (_modes.test(Mode::Origin))
+    {
+        cursorPosition.y -= _GetVerticalMargins(viewport, false).first;
+    }
+
     // VT is also 1 based, not 0 based, so correct by 1.
     auto& savedCursorState = _savedCursorState.at(_usingAltBuffer);
     savedCursorState.Column = cursorPosition.x + 1;
@@ -464,22 +470,11 @@ bool AdaptDispatch::CursorRestoreState()
 {
     auto& savedCursorState = _savedCursorState.at(_usingAltBuffer);
 
-    auto row = savedCursorState.Row;
-    const auto col = savedCursorState.Column;
-
-    // If the origin mode is relative, we need to make sure the restored
-    // position is clamped within the margins.
-    if (savedCursorState.IsOriginModeRelative)
-    {
-        const auto viewport = _api.GetViewport();
-        const auto [topMargin, bottomMargin] = _GetVerticalMargins(viewport, false);
-        // VT origin is at 1,1 so we need to add 1 to these margins.
-        row = std::clamp(row, topMargin + 1, bottomMargin + 1);
-    }
+    // Restore the origin mode first, since the cursor coordinates may be relative.
+    _modes.set(Mode::Origin, savedCursorState.IsOriginModeRelative);
 
-    // The saved coordinates are always absolute, so we need reset the origin mode temporarily.
-    _modes.reset(Mode::Origin);
-    CursorPosition(row, col);
+    // We can then restore the position with a standard CUP operation.
+    CursorPosition(savedCursorState.Row, savedCursorState.Column);
 
     // If the delayed wrap flag was set when the cursor was saved, we need to restore that now.
     if (savedCursorState.IsDelayedEOLWrap)
@@ -487,9 +482,6 @@ bool AdaptDispatch::CursorRestoreState()
         _api.GetTextBuffer().GetCursor().DelayEOLWrap();
     }
 
-    // Once the cursor position is restored, we can then restore the actual origin mode.
-    _modes.set(Mode::Origin, savedCursorState.IsOriginModeRelative);
-
     // Restore text attributes.
     _api.SetTextAttributes(savedCursorState.Attributes);
 


Test patch: