From 2fa1eeab1713b54fb2c1d72a4caa7496bf73ec8c Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 5 Sep 2019 09:26:47 -0500 Subject: [PATCH 1/4] This fixes #832 by not mucking with roundtripping attributes. Still needs a test --- src/host/getset.cpp | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/src/host/getset.cpp b/src/host/getset.cpp index f576c1d9414..0c6cad8ff57 100644 --- a/src/host/getset.cpp +++ b/src/host/getset.cpp @@ -2055,12 +2055,25 @@ void DoSrvPrivateModifyLinesImpl(const unsigned int count, const bool insert) SMALL_RECT srClip = screenEdges; srClip.Top = cursorPosition.Y; - LOG_IF_FAILED(ServiceLocator::LocateGlobals().api.ScrollConsoleScreenBufferWImpl(screenInfo, - srScroll, - coordDestination, - srClip, - UNICODE_SPACE, - screenInfo.GetAttributes().GetLegacyAttributes())); + // Here we previously called to ScrollConsoleScreenBufferWImpl to + // perform the scrolling operation. However, that function only accepts + // a WORD for the fill attributes. That means we'd lose 256/RGB fidelity + // for fill attributes. So instead, we'll just call ScrollRegion + // ourselves, with the same params that ScrollConsoleScreenBufferWImpl + // would have. + // See microsoft/terminal#832 for more context. + try + { + LockConsole(); + auto Unlock = wil::scope_exit([&] { UnlockConsole(); }); + ScrollRegion(screenInfo, + srScroll, + srClip, + coordDestination, + UNICODE_SPACE, + screenInfo.GetAttributes()); + } + CATCH_LOG(); } } From 8147b7a2bfe2f5a7a2f97e81ba079d05c06c7097 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 5 Sep 2019 10:13:20 -0500 Subject: [PATCH 2/4] Add a test --- src/host/ut_host/ScreenBufferTests.cpp | 69 ++++++++++++++++++++++++++ 1 file changed, 69 insertions(+) diff --git a/src/host/ut_host/ScreenBufferTests.cpp b/src/host/ut_host/ScreenBufferTests.cpp index 94d77faaff9..d68cee60d8c 100644 --- a/src/host/ut_host/ScreenBufferTests.cpp +++ b/src/host/ut_host/ScreenBufferTests.cpp @@ -167,6 +167,8 @@ class ScreenBufferTests TEST_METHOD(DeleteLinesInMargins); TEST_METHOD(ReverseLineFeedInMargins); + TEST_METHOD(DeleteLines256Colors); + TEST_METHOD(SetOriginMode); TEST_METHOD(HardResetBuffer); @@ -3451,6 +3453,73 @@ void ScreenBufferTests::ReverseLineFeedInMargins() } } +void ScreenBufferTests::DeleteLines256Colors() +{ + // This test is largely taken from repro code from + // https://github.com/microsoft/terminal/issues/832#issuecomment-507447272 + Log::Comment( + L"Sets the attributes to a 256/RGB color, then scrolls some lines with" + L" DL. Verifies the rows are cleared with the attributes we'd expect."); + + auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); + auto& si = gci.GetActiveOutputBuffer(); + auto& tbi = si.GetTextBuffer(); + auto& stateMachine = si.GetStateMachine(); + auto& cursor = si.GetTextBuffer().GetCursor(); + + TextAttribute expectedAttr{ si.GetAttributes() }; + expectedAttr.SetBackground(gci.GetColorTableEntry(2)); + + // Set some scrolling margins + stateMachine.ProcessString(L"\x1b[1;3r"); + + // Set the BG color to the table index 2, as a 256-color sequence + stateMachine.ProcessString(L"\x1b[48;5;2m"); + + VERIFY_ARE_EQUAL(expectedAttr, si.GetAttributes()); + + // Move to home + stateMachine.ProcessString(L"\x1b[H"); + + // Delete 10 lines + stateMachine.ProcessString(L"\x1b[10M"); + + Log::Comment(NoThrowString().Format( + L"cursor=%s", VerifyOutputTraits::ToString(cursor.GetPosition()).GetBuffer())); + Log::Comment(NoThrowString().Format( + L"viewport=%s", VerifyOutputTraits::ToString(si.GetViewport().ToInclusive()).GetBuffer())); + + VERIFY_ARE_EQUAL(0, cursor.GetPosition().X); + VERIFY_ARE_EQUAL(0, cursor.GetPosition().Y); + + stateMachine.ProcessString(L"foo"); + Log::Comment(NoThrowString().Format( + L"cursor=%s", VerifyOutputTraits::ToString(cursor.GetPosition()).GetBuffer())); + VERIFY_ARE_EQUAL(3, cursor.GetPosition().X); + VERIFY_ARE_EQUAL(0, cursor.GetPosition().Y); + { + auto iter00 = tbi.GetCellDataAt({ 0, 0 }); + auto iter10 = tbi.GetCellDataAt({ 1, 0 }); + auto iter20 = tbi.GetCellDataAt({ 2, 0 }); + auto iter30 = tbi.GetCellDataAt({ 3, 0 }); + auto iter01 = tbi.GetCellDataAt({ 0, 1 }); + auto iter02 = tbi.GetCellDataAt({ 0, 2 }); + VERIFY_ARE_EQUAL(L"f", iter00->Chars()); + VERIFY_ARE_EQUAL(L"o", iter10->Chars()); + VERIFY_ARE_EQUAL(L"o", iter20->Chars()); + VERIFY_ARE_EQUAL(L"\x20", iter30->Chars()); + VERIFY_ARE_EQUAL(L"\x20", iter01->Chars()); + VERIFY_ARE_EQUAL(L"\x20", iter02->Chars()); + + VERIFY_ARE_EQUAL(expectedAttr, iter00->TextAttr()); + VERIFY_ARE_EQUAL(expectedAttr, iter10->TextAttr()); + VERIFY_ARE_EQUAL(expectedAttr, iter20->TextAttr()); + VERIFY_ARE_EQUAL(expectedAttr, iter30->TextAttr()); + VERIFY_ARE_EQUAL(expectedAttr, iter01->TextAttr()); + VERIFY_ARE_EQUAL(expectedAttr, iter02->TextAttr()); + } +} + void ScreenBufferTests::SetOriginMode() { auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); From 594f918fad20f8793e69b01d64336c1209b7dc9c Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 5 Sep 2019 16:29:16 -0500 Subject: [PATCH 3/4] Lets just make this test test everything @miniksa https://media0.giphy.com/media/d7mMzaGDYkz4ZBziP6/giphy.gif --- src/host/ut_host/ScreenBufferTests.cpp | 45 ++++++++++++++++++++++---- 1 file changed, 39 insertions(+), 6 deletions(-) diff --git a/src/host/ut_host/ScreenBufferTests.cpp b/src/host/ut_host/ScreenBufferTests.cpp index 16ca540cdbe..ab7efe015d8 100644 --- a/src/host/ut_host/ScreenBufferTests.cpp +++ b/src/host/ut_host/ScreenBufferTests.cpp @@ -167,7 +167,10 @@ class ScreenBufferTests TEST_METHOD(DeleteLinesInMargins); TEST_METHOD(ReverseLineFeedInMargins); - TEST_METHOD(DeleteLines256Colors); + TEST_METHOD(InsertDeleteLines256Colors); + // TEST_METHOD(InsertDeleteLines256ColorsFrom16Table); + // TEST_METHOD(InsertDeleteLines256ColorsFrom256Table); + // TEST_METHOD(InsertDeleteLinesRGBColors); TEST_METHOD(SetOriginMode); @@ -3455,8 +3458,24 @@ void ScreenBufferTests::ReverseLineFeedInMargins() } } -void ScreenBufferTests::DeleteLines256Colors() +void ScreenBufferTests::InsertDeleteLines256Colors() { + BEGIN_TEST_METHOD_PROPERTIES() + TEST_METHOD_PROPERTY(L"Data:insert", L"{false, true}") + TEST_METHOD_PROPERTY(L"Data:colorStyle", L"{0, 1, 2}") + END_TEST_METHOD_PROPERTIES(); + + // colorStyle will be used to control whether we use a color from the 16 + // color table, a color from the 256 color table, or a pure RGB color. + const int Use16Color = 0; + const int Use256Color = 1; + const int UseRGBColor = 2; + + bool insert; + int colorStyle; + VERIFY_SUCCEEDED(TestData::TryGetValue(L"insert", insert), L"whether to insert(true) or delete(false) lines"); + VERIFY_SUCCEEDED(TestData::TryGetValue(L"colorStyle", colorStyle), L"controls whether to use the 16 color table, 256 table, or RGB colors"); + // This test is largely taken from repro code from // https://github.com/microsoft/terminal/issues/832#issuecomment-507447272 Log::Comment( @@ -3470,21 +3489,35 @@ void ScreenBufferTests::DeleteLines256Colors() auto& cursor = si.GetTextBuffer().GetCursor(); TextAttribute expectedAttr{ si.GetAttributes() }; - expectedAttr.SetBackground(gci.GetColorTableEntry(2)); + std::wstring sgrSeq = L"\x1b[48;5;2m"; + if (colorStyle == Use16Color) + { + expectedAttr.SetBackground(gci.GetColorTableEntry(2)); + } + else if (colorStyle == Use256Color) + { + expectedAttr.SetBackground(gci.GetColorTableEntry(20)); + sgrSeq = L"\x1b[48;5;20m"; + } + else if (colorStyle == UseRGBColor) + { + expectedAttr.SetBackground(RGB(1, 2, 3)); + sgrSeq = L"\x1b[48;2;1;2;3m"; + } // Set some scrolling margins stateMachine.ProcessString(L"\x1b[1;3r"); // Set the BG color to the table index 2, as a 256-color sequence - stateMachine.ProcessString(L"\x1b[48;5;2m"); + stateMachine.ProcessString(sgrSeq); VERIFY_ARE_EQUAL(expectedAttr, si.GetAttributes()); // Move to home stateMachine.ProcessString(L"\x1b[H"); - // Delete 10 lines - stateMachine.ProcessString(L"\x1b[10M"); + // Insert/Delete 10 lines + stateMachine.ProcessString(insert ? L"\x1b[10L" : L"\x1b[10M"); Log::Comment(NoThrowString().Format( L"cursor=%s", VerifyOutputTraits::ToString(cursor.GetPosition()).GetBuffer())); From a5095ee0e6307639a24568f52dbd381628ce36f0 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Mon, 9 Sep 2019 09:26:05 -0500 Subject: [PATCH 4/4] Remove dead code --- src/host/ut_host/ScreenBufferTests.cpp | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/host/ut_host/ScreenBufferTests.cpp b/src/host/ut_host/ScreenBufferTests.cpp index ab7efe015d8..38d4faf2684 100644 --- a/src/host/ut_host/ScreenBufferTests.cpp +++ b/src/host/ut_host/ScreenBufferTests.cpp @@ -168,9 +168,6 @@ class ScreenBufferTests TEST_METHOD(ReverseLineFeedInMargins); TEST_METHOD(InsertDeleteLines256Colors); - // TEST_METHOD(InsertDeleteLines256ColorsFrom16Table); - // TEST_METHOD(InsertDeleteLines256ColorsFrom256Table); - // TEST_METHOD(InsertDeleteLinesRGBColors); TEST_METHOD(SetOriginMode);