From 974e95ebf7704b08da51c1f728ab2bc06108881f Mon Sep 17 00:00:00 2001 From: James Holderness Date: Wed, 28 Aug 2019 02:45:38 +0100 Subject: [PATCH] Make the RIS command clear the display and scrollback correctly (#2367) When the scrollback buffer is empty, the RIS escape sequence (Reset to Initial State) will fail to clear the screen, or reset any of the state. And when there is something in the scrollback, it doesn't get cleared completely, and the screen may get filled with the wrong background color (it should use the default color, but it actually uses the previously active background color). This commit attempts to fix those issues. The initial failure is caused by the `SCREEN_INFORMATION::WriteRect` method throwing an exception when passed an empty viewport. And the reason it's passed an empty viewport is because that's what the `Viewport::Subtract` method returns when the result of the subtraction is nothing. The PR fixes the problem by making the `Viewport::Subtract` method actually return nothing in that situation. This is a change in the defined behavior that also required the associated viewport tests to be updated. However, it does seem a sensible change, since the `Subtract` method never returns empty viewports under any other circumstances. And the only place the method seems to be used is in the `ScrollRegion` implementation, where the previous behavior is guaranteed to throw an exception. The other issues are fixed simply by changing the order in which things are reset in the `AdaptDispatch::HardReset` method. The call to `SoftReset` needed to be made first, so that the SGR attributes would be reset before the screen was cleared, thus making sure that the default background color would be used. And the screen needed to be cleared before the scrollback was erased, otherwise the last view of the screen would be retained in the scrollback buffer. These changes also required existing adapter tests to be updated, but not because of a change in the expected behaviour. It's just that certain tests relied on the `SoftReset` happening later in the order, so weren't expecting it to be called if say the scrollback erase had failed. It doesn't seem like the tests were deliberately trying to verify that the SoftReset _hadn't_ been called. In addition to the updates to existing tests, this PR also add a new screen buffer test which verifies the display and scrollback are correctly cleared under the conditions that were previously failing. Fixes #2307. --- src/host/ut_host/ScreenBufferTests.cpp | 65 +++++++++++++++++++ src/host/ut_host/ViewportTests.cpp | 12 +--- src/terminal/adapter/adaptDispatch.cpp | 9 +-- .../adapter/ut_adapter/adapterTest.cpp | 48 ++++++++------ src/types/viewport.cpp | 56 +++++++--------- 5 files changed, 123 insertions(+), 67 deletions(-) diff --git a/src/host/ut_host/ScreenBufferTests.cpp b/src/host/ut_host/ScreenBufferTests.cpp index aa56b1b4806..94d77faaff9 100644 --- a/src/host/ut_host/ScreenBufferTests.cpp +++ b/src/host/ut_host/ScreenBufferTests.cpp @@ -168,6 +168,8 @@ class ScreenBufferTests TEST_METHOD(ReverseLineFeedInMargins); TEST_METHOD(SetOriginMode); + + TEST_METHOD(HardResetBuffer); }; void ScreenBufferTests::SingleAlternateBufferCreationTest() @@ -3516,3 +3518,66 @@ void ScreenBufferTests::SetOriginMode() // Reset DECOM so we don't affect future tests stateMachine.ProcessString(L"\x1B[?6l"); } + +void ScreenBufferTests::HardResetBuffer() +{ + auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); + auto& si = gci.GetActiveOutputBuffer().GetActiveBuffer(); + auto& stateMachine = si.GetStateMachine(); + const auto& viewport = si.GetViewport(); + const auto& cursor = si.GetTextBuffer().GetCursor(); + WI_SetFlag(si.OutputMode, ENABLE_VIRTUAL_TERMINAL_PROCESSING); + + auto isBufferClear = [&]() { + auto offset = 0; + auto width = si.GetBufferSize().Width(); + for (auto iter = si.GetCellDataAt({}); iter; ++iter, ++offset) + { + if (iter->Chars() != L" " || iter->TextAttr() != TextAttribute{}) + { + Log::Comment(NoThrowString().Format( + L"Buffer not clear at (X:%d, Y:%d)", + offset % width, + offset / width)); + return false; + } + } + return true; + }; + + const auto resetToInitialState = L"\033c"; + + Log::Comment(L"Start with a clear buffer, viewport and cursor at 0,0"); + si.SetAttributes(TextAttribute()); + si.ClearTextData(); + VERIFY_SUCCEEDED(si.SetViewportOrigin(true, { 0, 0 }, true)); + VERIFY_SUCCEEDED(si.SetCursorPosition({ 0, 0 }, true)); + VERIFY_IS_TRUE(isBufferClear()); + + Log::Comment(L"Write a single line of text to the buffer"); + stateMachine.ProcessString(L"Hello World!\n"); + VERIFY_IS_FALSE(isBufferClear()); + VERIFY_ARE_EQUAL(COORD({ 0, 1 }), cursor.GetPosition()); + + Log::Comment(L"After a reset, buffer should be clear, with cursor at 0,0"); + stateMachine.ProcessString(resetToInitialState); + VERIFY_IS_TRUE(isBufferClear()); + VERIFY_ARE_EQUAL(COORD({ 0, 0 }), cursor.GetPosition()); + + Log::Comment(L"Set the background color to red"); + stateMachine.ProcessString(L"\x1b[41m"); + Log::Comment(L"Write multiple pages of text to the buffer"); + for (auto i = 0; i < viewport.Height() * 2; i++) + { + stateMachine.ProcessString(L"Hello World!\n"); + } + VERIFY_IS_FALSE(isBufferClear()); + VERIFY_IS_GREATER_THAN(viewport.Top(), viewport.Height()); + VERIFY_IS_GREATER_THAN(cursor.GetPosition().Y, viewport.Height()); + + Log::Comment(L"After a reset, buffer should be clear, with viewport and cursor at 0,0"); + stateMachine.ProcessString(resetToInitialState); + VERIFY_IS_TRUE(isBufferClear()); + VERIFY_ARE_EQUAL(COORD({ 0, 0 }), viewport.Origin()); + VERIFY_ARE_EQUAL(COORD({ 0, 0 }), cursor.GetPosition()); +} diff --git a/src/host/ut_host/ViewportTests.cpp b/src/host/ut_host/ViewportTests.cpp index d82028d9a53..c5bdc3aa9e6 100644 --- a/src/host/ut_host/ViewportTests.cpp +++ b/src/host/ut_host/ViewportTests.cpp @@ -1044,18 +1044,8 @@ class ViewportTests const auto original = Viewport::FromInclusive(srOriginal); const auto remove = original; - std::vector expected; - expected.emplace_back(Viewport::FromDimensions(original.Origin(), { 0, 0 })); - const auto actual = Viewport::Subtract(original, remove); - VERIFY_ARE_EQUAL(expected.size(), actual.size(), L"Same number of viewports in expected and actual"); - Log::Comment(L"Now validate that each viewport has the expected area."); - for (size_t i = 0; i < expected.size(); i++) - { - const auto& exp = expected.at(i); - const auto& act = actual.at(i); - VERIFY_ARE_EQUAL(exp, act); - } + VERIFY_ARE_EQUAL(0u, actual.size(), L"There should be no viewports returned"); } }; diff --git a/src/terminal/adapter/adaptDispatch.cpp b/src/terminal/adapter/adaptDispatch.cpp index 9a045213a3c..4b853910dd0 100644 --- a/src/terminal/adapter/adaptDispatch.cpp +++ b/src/terminal/adapter/adaptDispatch.cpp @@ -1601,17 +1601,18 @@ bool AdaptDispatch::SoftReset() // True if handled successfully. False otherwise. bool AdaptDispatch::HardReset() { + // Sets the SGR state to normal - this must be done before EraseInDisplay + // to ensure that it clears with the default background color. + bool fSuccess = SoftReset(); + // Clears the screen - Needs to be done in two operations. - bool fSuccess = _EraseScrollback(); if (fSuccess) { fSuccess = EraseInDisplay(DispatchTypes::EraseType::All); } - - // Sets the SGR state to normal. if (fSuccess) { - fSuccess = SoftReset(); + fSuccess = _EraseScrollback(); } // Cursor to 1,1 - the Soft Reset guarantees this is absolute diff --git a/src/terminal/adapter/ut_adapter/adapterTest.cpp b/src/terminal/adapter/ut_adapter/adapterTest.cpp index 5c584a82998..bc65cae83ee 100644 --- a/src/terminal/adapter/ut_adapter/adapterTest.cpp +++ b/src/terminal/adapter/ut_adapter/adapterTest.cpp @@ -3426,30 +3426,33 @@ class AdapterTest // The cursor will be moved to the same relative location in the new viewport with origin @ 0, 0 const COORD coordRelativeCursor = { _testGetSet->_coordCursorPos.X - _testGetSet->_srViewport.Left, _testGetSet->_coordCursorPos.Y - _testGetSet->_srViewport.Top }; - - // Cursor to 1,1 - _testGetSet->_coordExpectedCursorPos = { 0, 0 }; - _testGetSet->_fSetConsoleCursorPositionResult = true; - _testGetSet->_fPrivateSetLegacyAttributesResult = true; - _testGetSet->_fPrivateSetDefaultAttributesResult = true; - _testGetSet->_fPrivateBoldTextResult = true; - _testGetSet->_fExpectedForeground = true; - _testGetSet->_fExpectedBackground = true; - _testGetSet->_fExpectedMeta = true; - _testGetSet->_fExpectedIsBold = false; - _testGetSet->_expectedShowCursor = true; - _testGetSet->_privateShowCursorResult = true; const COORD coordExpectedCursorPos = { 0, 0 }; - // We're expecting _SetDefaultColorHelper to call - // PrivateSetLegacyAttributes with 0 as the wAttr param. - _testGetSet->_wExpectedAttribute = 0; + auto prepExpectedParameters = [&]() { + // Cursor to 1,1 + _testGetSet->_coordExpectedCursorPos = { 0, 0 }; + _testGetSet->_fSetConsoleCursorPositionResult = true; + _testGetSet->_fPrivateSetLegacyAttributesResult = true; + _testGetSet->_fPrivateSetDefaultAttributesResult = true; + _testGetSet->_fPrivateBoldTextResult = true; + _testGetSet->_fExpectedForeground = true; + _testGetSet->_fExpectedBackground = true; + _testGetSet->_fExpectedMeta = true; + _testGetSet->_fExpectedIsBold = false; + _testGetSet->_expectedShowCursor = true; + _testGetSet->_privateShowCursorResult = true; + + // We're expecting _SetDefaultColorHelper to call + // PrivateSetLegacyAttributes with 0 as the wAttr param. + _testGetSet->_wExpectedAttribute = 0; - // Prepare the results of SoftReset api calls - _testGetSet->_fPrivateSetCursorKeysModeResult = true; - _testGetSet->_fPrivateSetKeypadModeResult = true; - _testGetSet->_fGetConsoleScreenBufferInfoExResult = true; - _testGetSet->_fPrivateSetScrollingRegionResult = true; + // Prepare the results of SoftReset api calls + _testGetSet->_fPrivateSetCursorKeysModeResult = true; + _testGetSet->_fPrivateSetKeypadModeResult = true; + _testGetSet->_fGetConsoleScreenBufferInfoExResult = true; + _testGetSet->_fPrivateSetScrollingRegionResult = true; + }; + prepExpectedParameters(); VERIFY_IS_TRUE(_pDispatch->HardReset()); VERIFY_ARE_EQUAL(_testGetSet->_coordCursorPos, coordExpectedCursorPos); @@ -3457,18 +3460,21 @@ class AdapterTest Log::Comment(L"Test 2: Gracefully fail when getting console information fails."); _testGetSet->PrepData(); + prepExpectedParameters(); _testGetSet->_fGetConsoleScreenBufferInfoExResult = false; VERIFY_IS_FALSE(_pDispatch->HardReset()); Log::Comment(L"Test 3: Gracefully fail when filling the rectangle fails."); _testGetSet->PrepData(); + prepExpectedParameters(); _testGetSet->_fFillConsoleOutputCharacterWResult = false; VERIFY_IS_FALSE(_pDispatch->HardReset()); Log::Comment(L"Test 4: Gracefully fail when setting the window fails."); _testGetSet->PrepData(); + prepExpectedParameters(); _testGetSet->_fSetConsoleWindowInfoResult = false; VERIFY_IS_FALSE(_pDispatch->HardReset()); diff --git a/src/types/viewport.cpp b/src/types/viewport.cpp index 8484b6536a3..cb088c0519a 100644 --- a/src/types/viewport.cpp +++ b/src/types/viewport.cpp @@ -909,7 +909,8 @@ Viewport Viewport::ToOrigin() const noexcept // Just put the original rectangle into the results and return early. result.viewports.at(result.used++) = original; } - else + // If the original rectangle matches the intersection, there is nothing to return. + else if (original != intersection) { // Generate our potential four viewports that represent the region of the original that falls outside of the remove area. // We will bias toward generating wide rectangles over tall rectangles (if possible) so that optimizations that apply @@ -922,8 +923,8 @@ Viewport Viewport::ToOrigin() const noexcept // | | | | // | | | | // | | | | - // | | ======> | intersect | ======> early return of 0x0 Viewport - // | | | | at Original's origin + // | | ======> | intersect | ======> early return of nothing + // | | | | // | | | | // | | | | // |---------removeMe---------| |--------------------------| @@ -992,39 +993,32 @@ Viewport Viewport::ToOrigin() const noexcept // | removeMe | // |---------------| - if (original == intersection) + // We generate these rectangles by the original and intersection points, but some of them might be empty when the intersection + // lines up with the edge of the original. That's OK. That just means that the subtraction didn't leave anything behind. + // We will filter those out below when adding them to the result. + const auto top = Viewport({ original.Left(), original.Top(), original.RightInclusive(), intersection.Top() - 1 }); + const auto bottom = Viewport({ original.Left(), intersection.BottomExclusive(), original.RightInclusive(), original.BottomInclusive() }); + const auto left = Viewport({ original.Left(), intersection.Top(), intersection.Left() - 1, intersection.BottomInclusive() }); + const auto right = Viewport({ intersection.RightExclusive(), intersection.Top(), original.RightInclusive(), intersection.BottomInclusive() }); + + if (top.IsValid()) { - result.viewports.at(result.used++) = Viewport::FromDimensions(original.Origin(), { 0, 0 }); + result.viewports.at(result.used++) = top; } - else - { - // We generate these rectangles by the original and intersection points, but some of them might be empty when the intersection - // lines up with the edge of the original. That's OK. That just means that the subtraction didn't leave anything behind. - // We will filter those out below when adding them to the result. - const auto top = Viewport({ original.Left(), original.Top(), original.RightInclusive(), intersection.Top() - 1 }); - const auto bottom = Viewport({ original.Left(), intersection.BottomExclusive(), original.RightInclusive(), original.BottomInclusive() }); - const auto left = Viewport({ original.Left(), intersection.Top(), intersection.Left() - 1, intersection.BottomInclusive() }); - const auto right = Viewport({ intersection.RightExclusive(), intersection.Top(), original.RightInclusive(), intersection.BottomInclusive() }); - - if (top.IsValid()) - { - result.viewports.at(result.used++) = top; - } - if (bottom.IsValid()) - { - result.viewports.at(result.used++) = bottom; - } + if (bottom.IsValid()) + { + result.viewports.at(result.used++) = bottom; + } - if (left.IsValid()) - { - result.viewports.at(result.used++) = left; - } + if (left.IsValid()) + { + result.viewports.at(result.used++) = left; + } - if (right.IsValid()) - { - result.viewports.at(result.used++) = right; - } + if (right.IsValid()) + { + result.viewports.at(result.used++) = right; } }