From 0c8a4df9636982fc92137c47b31877f2c7b5efdb Mon Sep 17 00:00:00 2001 From: James Holderness Date: Tue, 24 Sep 2019 00:16:54 +0100 Subject: [PATCH] Remove unwanted DECSTBM clipping (#2764) The `DECSTBM` margins are meant to define the range of lines within which certain vertical scrolling operations take place. However, we were applying these margin restrictions in the `ScrollRegion` function, which is also used in a number of places that shouldn't be affected by `DECSTBM`. This includes the `ICH` and `DCH` escape sequences (which are only affected by the horizontal margins, which we don't yet support), the `ScrollConsoleScreenBuffer` API (which is public Console API, not meant to be affected by the VT terminal emulation), and the `CSI 3 J` erase scrollback extension (which isn't really scrolling as such, but uses the `ScrollRegion` function to manipulate the scrollback buffer). This commit moves the margin clipping out of the `ScrollRegion` function, so it can be applied exclusively in the places that need it. With the margin clipping removed from the `ScrollRegion` function, it now had to be applied manually in the places it was actually required. This included: * The `DoSrvPrivateReverseLineFeed` function (for the `RI` control): This was * just a matter of updating the bottom of the scroll rect to the bottom margin * (at least when the margins were actually set), since the top of the scroll * rect would always be the top of the viewport. The * `DoSrvPrivateModifyLinesImpl` function (for the `IL` and `DL` commands): * Again this was just a matter of updating the bottom of the scroll rect, since * the cursor position would always determine the top of the scroll rect. The * `AdaptDispatch::_ScrollMovement` method (for the `SU` and `SD` commands): * This required updating both the top and bottom coordinates of the scroll * rect, and also a simpler destination Y coordinate (the way the `ScrollRegion` * function worked before, the caller was expected to take the margins into * account when determining the destination). On the plus side, there was now no longer a need to override the margins when calling `ScrollRegion` in the `AdjustCursorPosition` function. In the first case, the margins had needed to be cleared (_stream.cpp 143-145), but that is now the default behaviour. In the second case, there had been a more complicated adjustment of the margins (_stream.cpp 196-209), but that code was never actually used so could be removed completely (to get to that point either _fScrollUp_ was true, so _scrollDownAtTop_ couldn't also be true, or _fScrollDown_ was true, but in that case there is a check to make sure _scrollDownAtTop_ is false). While testing, I also noticed that one of the `ScrollRegion` calls in the `AdjustCursorPosition` function was not setting the horizontal range correctly - the scrolling should always affect the full buffer width rather than just the viewport width - so I've fixed that now as well. ## Validation Steps Performed For commands like `RI`, `IL`, `DL`, etc. where we've changed the implementation but not the behaviour, there were already unit tests that could confirm that the new implementation was still producing the correct results. Where there has been a change in behaviour - namely for the `ICH` and `DCH` commands, and the `ScrollConsoleScreenBuffer` API - I've extended the existing unit tests to check that they still function correctly even when the `DECSTBM` margins are set (which would previously have caused them to fail). I've also tested manually with the test cases in issues #2543 and #2659, and confirmed that they now work as expected. Closes #2543 Closes #2659 --- src/host/_stream.cpp | 30 +++----------------------- src/host/getset.cpp | 10 +++++++++ src/host/output.cpp | 25 --------------------- src/host/ut_host/ApiRoutinesTests.cpp | 10 +++++++++ src/host/ut_host/ScreenBufferTests.cpp | 26 ++++++++++++++++++++++ src/terminal/adapter/adaptDispatch.cpp | 10 ++++++--- 6 files changed, 56 insertions(+), 55 deletions(-) diff --git a/src/host/_stream.cpp b/src/host/_stream.cpp index 3e99d36ec55..94d4f7c74c3 100644 --- a/src/host/_stream.cpp +++ b/src/host/_stream.cpp @@ -140,15 +140,11 @@ using Microsoft::Console::VirtualTerminal::StateMachine; const COORD newPostMarginsOrigin = { 0, moveToYPosition }; const COORD newViewOrigin = { 0, newViewTop }; - // Unset the margins to scroll the content below the margins, - // then restore them after. - screenInfo.SetScrollMargins(Viewport::FromInclusive({ 0 })); try { ScrollRegion(screenInfo, scrollRect, std::nullopt, newPostMarginsOrigin, UNICODE_SPACE, bufferAttributes); } CATCH_LOG(); - screenInfo.SetScrollMargins(relativeMargins); // Move the viewport down auto hr = screenInfo.SetViewportOrigin(true, newViewOrigin, true); @@ -186,39 +182,19 @@ using Microsoft::Console::VirtualTerminal::StateMachine; SMALL_RECT scrollRect = { 0 }; scrollRect.Top = srMargins.Top; scrollRect.Bottom = srMargins.Bottom; - scrollRect.Left = screenInfo.GetViewport().Left(); // NOTE: Left/Right Scroll margins don't do anything currently. - scrollRect.Right = screenInfo.GetViewport().RightInclusive(); + scrollRect.Left = 0; // NOTE: Left/Right Scroll margins don't do anything currently. + scrollRect.Right = bufferSize.X - 1; // -1, otherwise this would be an exclusive rect. COORD dest; dest.X = scrollRect.Left; dest.Y = scrollRect.Top - diff; - SMALL_RECT clipRect = scrollRect; - // Typically ScrollRegion() clips by the scroll margins. However, if - // we're scrolling down at the top of the viewport, we'll need to - // not clip at the margins, instead move the contents of the margins - // up above the viewport. So we'll clear out the current margins, and - // set them to the viewport+(#diff rows above the viewport). - if (scrollDownAtTop) - { - clipRect.Top -= diff; - auto fakeMargins = srMargins; - fakeMargins.Top -= diff; - auto fakeRelative = viewport.ConvertToOrigin(Viewport::FromInclusive(fakeMargins)); - screenInfo.SetScrollMargins(fakeRelative); - } - try { - ScrollRegion(screenInfo, scrollRect, clipRect, dest, UNICODE_SPACE, bufferAttributes); + ScrollRegion(screenInfo, scrollRect, scrollRect, dest, UNICODE_SPACE, bufferAttributes); } CATCH_LOG(); - if (scrollDownAtTop) - { - // Undo the fake margins we set above - screenInfo.SetScrollMargins(relativeMargins); - } coordCursor.Y -= diff; } diff --git a/src/host/getset.cpp b/src/host/getset.cpp index 7bf5c74d97e..16aa6633a71 100644 --- a/src/host/getset.cpp +++ b/src/host/getset.cpp @@ -1368,6 +1368,11 @@ void DoSrvPrivateAllowCursorBlinking(SCREEN_INFORMATION& screenInfo, const bool srScroll.Right = SHORT_MAX; srScroll.Top = viewport.Top; srScroll.Bottom = viewport.Bottom; + // Clip to the DECSTBM margin boundary + if (screenInfo.AreMarginsSet()) + { + srScroll.Bottom = screenInfo.GetAbsoluteScrollMargins().BottomInclusive(); + } // Paste coordinate for cut text above COORD coordDestination; coordDestination.X = 0; @@ -2044,6 +2049,11 @@ void DoSrvPrivateModifyLinesImpl(const unsigned int count, const bool insert) srScroll.Right = SHORT_MAX; srScroll.Top = cursorPosition.Y; srScroll.Bottom = screenInfo.GetViewport().BottomInclusive(); + // Clip to the DECSTBM margin boundary + if (screenInfo.AreMarginsSet()) + { + srScroll.Bottom = screenInfo.GetAbsoluteScrollMargins().BottomInclusive(); + } // Paste coordinate for cut text above COORD coordDestination; coordDestination.X = 0; diff --git a/src/host/output.cpp b/src/host/output.cpp index 8147de84535..8b4e0bbbbda 100644 --- a/src/host/output.cpp +++ b/src/host/output.cpp @@ -361,19 +361,6 @@ void ScrollRegion(SCREEN_INFORMATION& screenInfo, // If there was no clip rect, we'll clip to the entire buffer size. auto clip = Viewport::FromInclusive(clipRectGiven.value_or(buffer.ToInclusive())); - // Account for the scroll margins set by DECSTBM - // DECSTBM command can sometimes apply a clipping behavior as well. Check if we have any - // margins defined by DECSTBM and further restrict the clipping area here. - if (screenInfo.AreMarginsSet()) - { - const auto margin = screenInfo.GetScrollingRegion(); - - // Update the clip rectangle to only include the area that is also in the margin. - clip = Viewport::Intersect(clip, margin); - - // We'll also need to update the source rectangle, but we need to do that later. - } - // OK, make sure that the clip rectangle also fits inside the buffer clip = Viewport::Intersect(buffer, clip); @@ -416,18 +403,6 @@ void ScrollRegion(SCREEN_INFORMATION& screenInfo, targetOrigin.Y += currentSourceOrigin.Y - originalSourceOrigin.Y; } - // See MSFT:20204600 - Update the source rectangle to only include the region - // inside the scroll margins. We need to do this AFTER we calculate the - // delta between the currentSourceOrigin and the originalSourceOrigin. - // Don't combine this with the above block, because if there are margins set - // and the source rectangle was clipped by the buffer, we still want to - // adjust the target origin point based on the clipping of the buffer. - if (screenInfo.AreMarginsSet()) - { - const auto margin = screenInfo.GetScrollingRegion(); - source = Viewport::Intersect(source, margin); - } - // And now the target viewport is the same size as the source viewport but at the different position. auto target = Viewport::FromDimensions(targetOrigin, source.Dimensions()); diff --git a/src/host/ut_host/ApiRoutinesTests.cpp b/src/host/ut_host/ApiRoutinesTests.cpp index a8e425882b9..eb5d6c0a29e 100644 --- a/src/host/ut_host/ApiRoutinesTests.cpp +++ b/src/host/ut_host/ApiRoutinesTests.cpp @@ -594,9 +594,12 @@ class ApiRoutinesTests TEST_METHOD(ApiScrollConsoleScreenBufferW) { BEGIN_TEST_METHOD_PROPERTIES() + TEST_METHOD_PROPERTY(L"data:setMargins", L"{false, true}") TEST_METHOD_PROPERTY(L"data:checkClipped", L"{false, true}") END_TEST_METHOD_PROPERTIES(); + bool setMargins; + VERIFY_SUCCEEDED(TestData::TryGetValue(L"setMargins", setMargins), L"Get whether or not we should set the DECSTBM margins."); bool checkClipped; VERIFY_SUCCEEDED(TestData::TryGetValue(L"checkClipped", checkClipped), L"Get whether or not we should check all the options using a clipping rectangle."); @@ -605,6 +608,13 @@ class ApiRoutinesTests VERIFY_SUCCEEDED(si.GetTextBuffer().ResizeTraditional({ 5, 5 }), L"Make the buffer small so this doesn't take forever."); + // Tests are run both with and without the DECSTBM margins set. This should not alter + // the results, since ScrollConsoleScreenBuffer should not be affected by VT margins. + auto& stateMachine = si.GetStateMachine(); + stateMachine.ProcessString(setMargins ? L"\x1b[2;4r" : L"\x1b[r"); + // 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"); }); + gci.LockConsole(); auto Unlock = wil::scope_exit([&] { gci.UnlockConsole(); }); diff --git a/src/host/ut_host/ScreenBufferTests.cpp b/src/host/ut_host/ScreenBufferTests.cpp index 71d3aa7ea06..aa9195ce322 100644 --- a/src/host/ut_host/ScreenBufferTests.cpp +++ b/src/host/ut_host/ScreenBufferTests.cpp @@ -3282,6 +3282,13 @@ void ScreenBufferTests::ScrollOperations() void ScreenBufferTests::InsertChars() { + BEGIN_TEST_METHOD_PROPERTIES() + TEST_METHOD_PROPERTY(L"Data:setMargins", L"{false, true}") + END_TEST_METHOD_PROPERTIES(); + + bool setMargins; + VERIFY_SUCCEEDED(TestData::TryGetValue(L"setMargins", setMargins)); + auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); auto& si = gci.GetActiveOutputBuffer().GetActiveBuffer(); auto& stateMachine = si.GetStateMachine(); @@ -3295,6 +3302,12 @@ void ScreenBufferTests::InsertChars() VERIFY_SUCCEEDED(si.ResizeScreenBuffer({ bufferWidth, bufferHeight }, false)); si.SetViewport(Viewport::FromExclusive({ viewportStart, 0, viewportEnd, 25 }), true); + // Tests are run both with and without the DECSTBM margins set. This should not alter + // the results, since the ICH operation is not affected by vertical margins. + stateMachine.ProcessString(setMargins ? L"\x1b[15;20r" : L"\x1b[r"); + // 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"Test 1: Fill the line with Qs. Write some text within the viewport boundaries. " L"Then insert 5 spaces at the cursor. Watch spaces get inserted, text slides right " @@ -3425,6 +3438,13 @@ void ScreenBufferTests::InsertChars() void ScreenBufferTests::DeleteChars() { + BEGIN_TEST_METHOD_PROPERTIES() + TEST_METHOD_PROPERTY(L"Data:setMargins", L"{false, true}") + END_TEST_METHOD_PROPERTIES(); + + bool setMargins; + VERIFY_SUCCEEDED(TestData::TryGetValue(L"setMargins", setMargins)); + auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); auto& si = gci.GetActiveOutputBuffer().GetActiveBuffer(); auto& stateMachine = si.GetStateMachine(); @@ -3438,6 +3458,12 @@ void ScreenBufferTests::DeleteChars() VERIFY_SUCCEEDED(si.ResizeScreenBuffer({ bufferWidth, bufferHeight }, false)); si.SetViewport(Viewport::FromExclusive({ viewportStart, 0, viewportEnd, 25 }), true); + // Tests are run both with and without the DECSTBM margins set. This should not alter + // the results, since the DCH operation is not affected by vertical margins. + stateMachine.ProcessString(setMargins ? L"\x1b[15;20r" : L"\x1b[r"); + // 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"Test 1: Fill the line with Qs. Write some text within the viewport boundaries. " L"Then delete 5 characters at the cursor. Watch the rest of the line slide left, " diff --git a/src/terminal/adapter/adaptDispatch.cpp b/src/terminal/adapter/adaptDispatch.cpp index 1e6089ed22d..7123e695087 100644 --- a/src/terminal/adapter/adaptDispatch.cpp +++ b/src/terminal/adapter/adaptDispatch.cpp @@ -963,13 +963,17 @@ bool AdaptDispatch::_ScrollMovement(const ScrollDirection sdDirection, _In_ unsi srScreen.Right = SHORT_MAX; srScreen.Top = csbiex.srWindow.Top; srScreen.Bottom = csbiex.srWindow.Bottom - 1; // srWindow is exclusive, hence the - 1 + // Clip to the DECSTBM margin boundaries + if (_srScrollMargins.Top < _srScrollMargins.Bottom) + { + srScreen.Top = csbiex.srWindow.Top + _srScrollMargins.Top; + srScreen.Bottom = csbiex.srWindow.Top + _srScrollMargins.Bottom; + } // Paste coordinate for cut text above COORD coordDestination; coordDestination.X = srScreen.Left; - // Scroll starting from the top of the scroll margins. - coordDestination.Y = (_srScrollMargins.Top + srScreen.Top) + sDistance * (sdDirection == ScrollDirection::Up ? -1 : 1); - // We don't need to worry about clipping the margins at all, ScrollRegion inside conhost will do that correctly for us + coordDestination.Y = srScreen.Top + sDistance * (sdDirection == ScrollDirection::Up ? -1 : 1); // Fill character for remaining space left behind by "cut" operation (or for fill if we "cut" the entire line) CHAR_INFO ciFill;