Skip to content

Commit 12d2e17

Browse files
j4jamesDHowett
authored andcommitted
Correct the boundaries of the scrolling commands (microsoft#2505)
There are a number of VT escape sequences that rely on the `ScrollRegion` function to scroll the viewport (RI, DL, IL, SU, SD, ICH, and DCH) , and all of them have got the clipping rect or scroll boundaries wrong in some way, resulting in content being scrolled off the screen that should have been clipped, revealed areas not being correctly filled, or parts of the screen not being moved that should have been. This PR attempts to fix all of those issues. The `ScrollRegion` function is what ultimately handles the scrolling, but it's typically called via the `ApiRoutines::ScrollConsoleScreenBufferWImpl` method, and it's the callers of that method that have needed correcting. One "mistake" that many of these operations made, was in setting a clipping rect that was different from the scrolling rect. This should never have been necessary, since the area being scrolled is also the boundary into which the content needs to be clipped, so the easiest thing to do is just use the same rect for both parameters. Another common mistake was in clipping the horizontal boundaries to the width of the viewport. But it's really the buffer width that represents the active width of the screen - the viewport width and offset are merely a window on that active area. As such, the viewport should only be used to clip vertically - the horizontal extent should typically be the full buffer width. On that note, there is really no need to actually calculate the buffer width when we want to set any of the scrolling parameters to that width. The `ScrollRegion` function already takes care of clipping everything within the buffer boundary, so we can simply set the `Left` of the rect to `0` and the `Right` to `SHORT_MAX`. More details on individual commands: * RI (the `DoSrvPrivateReverseLineFeed` function) This now uses a single rect for both the scroll region and clipping boundary, and the width is set to `SHORT_MAX` to cover the full buffer width. Also the bottom of the scrolling region is now the bottom of the viewport (rather than bottom-1), otherwise it would be off by one. * DL and IL (the `DoSrvPrivateModifyLinesImpl` function) Again this uses a single rect for both the scroll region and clipping boundary, and the width is set to `SHORT_MAX` to cover the full width. The most significant change, though, is that the bottom boundary is now the viewport bottom rather than the buffer bottom. Using the buffer bottom prevented it clipping the content that scrolled off screen when inserting, and failed to fill the revealed area when deleting. * SU and SD (the `AdaptDispatch::_ScrollMovement` method) This was already using a single rect for both the scroll region and clipping boundary, but it was previously constrained to the width of the viewport rather than the buffer width, so some areas of the screen weren't correctly scrolled. Also, the bottom boundary was off by 1, because it was using an exclusive rect while the `ScrollRegion` function expects inclusive rects. * ICH and DCH (the `AdaptDispatch::_InsertDeleteHelper` method) This method has been considerably simplified, because it was reimplementing a lot of functionality that was already provided by the `ScrollRegion` function. And like many of the other cases, it has been updated to use a single rect for both the scroll region and clipping boundary, and clip to the full buffer width rather than the viewport width. I should add that if we were following the specs exactly, then the SU and SD commands should technically be panning the viewport over the buffer instead of moving the buffer contents within the viewport boundary. So SU would be the equivalent of a newline at the bottom of the viewport (assuming no margins). And SD would assumedly do the opposite, scrolling the back buffer back into view (an RI at the top of the viewport should do the same). This doesn't seem to be something that is consistently implemented, though. Some terminals do implement SU as a viewport pan, but I haven't seen anyone implement SD or RI as a pan. If we do want to do something about this, I think it's best addressed as a separate issue. ## Validation Steps Performed There were already existing tests for the SU, SD, ICH, and DCH commands, but they were implemented as adapter tests, which weren't effectively testing anything - the `ScrollConsoleScreenBufferW` method used in those tests was just a mock (an incomplete reimplementation of the `ScrollRegion` function), so confirming that the mock produced the correct result told you nothing about the validity of the real code. To address that, I've now reimplemented those adapter tests as screen buffer tests. For the most part I've tried to duplicate the functionality of the original tests, but there are significant differences to account for the fact that scrolling region now covers the full width of the buffer rather than just the viewport width. I've also extended those tests with additional coverage for the RI, DL, and IL commands, which are really just a variation of the SU and SD functionality. Closes microsoft#2174
1 parent 2da3b49 commit 12d2e17

4 files changed

Lines changed: 525 additions & 827 deletions

File tree

src/host/getset.cpp

Lines changed: 10 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1356,24 +1356,22 @@ void DoSrvPrivateAllowCursorBlinking(SCREEN_INFORMATION& screenInfo, const bool
13561356
if (screenInfo.IsCursorInMargins(oldCursorPosition))
13571357
{
13581358
// Cursor is at the top of the viewport
1359-
const COORD bufferSize = screenInfo.GetBufferSize().Dimensions();
1360-
// Rectangle to cut out of the existing buffer
1359+
// Rectangle to cut out of the existing buffer. This is inclusive.
1360+
// It will be clipped to the buffer boundaries so SHORT_MAX gives us the full buffer width.
13611361
SMALL_RECT srScroll;
13621362
srScroll.Left = 0;
1363-
srScroll.Right = bufferSize.X;
1363+
srScroll.Right = SHORT_MAX;
13641364
srScroll.Top = viewport.Top;
1365-
srScroll.Bottom = viewport.Bottom - 1;
1365+
srScroll.Bottom = viewport.Bottom;
13661366
// Paste coordinate for cut text above
13671367
COORD coordDestination;
13681368
coordDestination.X = 0;
13691369
coordDestination.Y = viewport.Top + 1;
13701370

1371-
SMALL_RECT srClip = viewport;
1372-
13731371
Status = NTSTATUS_FROM_HRESULT(ServiceLocator::LocateGlobals().api.ScrollConsoleScreenBufferWImpl(screenInfo,
13741372
srScroll,
13751373
coordDestination,
1376-
srClip,
1374+
srScroll,
13771375
UNICODE_SPACE,
13781376
screenInfo.GetAttributes().GetLegacyAttributes()));
13791377
}
@@ -2033,13 +2031,13 @@ void DoSrvPrivateModifyLinesImpl(const unsigned int count, const bool insert)
20332031
const auto cursorPosition = textBuffer.GetCursor().GetPosition();
20342032
if (screenInfo.IsCursorInMargins(cursorPosition))
20352033
{
2036-
const auto screenEdges = screenInfo.GetBufferSize().ToInclusive();
2037-
// Rectangle to cut out of the existing buffer
2034+
// Rectangle to cut out of the existing buffer. This is inclusive.
2035+
// It will be clipped to the buffer boundaries so SHORT_MAX gives us the full buffer width.
20382036
SMALL_RECT srScroll;
20392037
srScroll.Left = 0;
2040-
srScroll.Right = screenEdges.Right - screenEdges.Left;
2038+
srScroll.Right = SHORT_MAX;
20412039
srScroll.Top = cursorPosition.Y;
2042-
srScroll.Bottom = screenEdges.Bottom;
2040+
srScroll.Bottom = screenInfo.GetViewport().BottomInclusive();
20432041
// Paste coordinate for cut text above
20442042
COORD coordDestination;
20452043
coordDestination.X = 0;
@@ -2052,9 +2050,6 @@ void DoSrvPrivateModifyLinesImpl(const unsigned int count, const bool insert)
20522050
coordDestination.Y = (cursorPosition.Y) - gsl::narrow<short>(count);
20532051
}
20542052

2055-
SMALL_RECT srClip = screenEdges;
2056-
srClip.Top = cursorPosition.Y;
2057-
20582053
// Here we previously called to ScrollConsoleScreenBufferWImpl to
20592054
// perform the scrolling operation. However, that function only accepts
20602055
// a WORD for the fill attributes. That means we'd lose 256/RGB fidelity
@@ -2068,7 +2063,7 @@ void DoSrvPrivateModifyLinesImpl(const unsigned int count, const bool insert)
20682063
auto Unlock = wil::scope_exit([&] { UnlockConsole(); });
20692064
ScrollRegion(screenInfo,
20702065
srScroll,
2071-
srClip,
2066+
srScroll,
20722067
coordDestination,
20732068
UNICODE_SPACE,
20742069
screenInfo.GetAttributes());

0 commit comments

Comments
 (0)