From dab0c3a0101eabb25e3855beab89aef34ceae646 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Mon, 8 Aug 2022 06:48:04 -0500 Subject: [PATCH] Fix conpty not emitting colored spaces on the VERY FIRST frame (#13665) This bug arose from a "race condition" in the first frame handling of conpty. We'd try to optimize out spaces if we've cleared the entire frame (which always happens on the first frame). However, doing that even for colored spaces meant that things like powerline prompts could be emitted to conhost during the first frame, and we'd optimize the spaces out. That's silly. This is hard to repro naturally, but this comment has another repro I used https://github.com/microsoft/terminal/issues/8341#issuecomment-731310022 Modified to facilitate simpler testing, it looks like: #### Before ![image](https://user-images.githubusercontent.com/18356694/182680119-bb22179c-a328-43f3-b64a-0d1d5773b813.png) #### After ![image](https://user-images.githubusercontent.com/18356694/182680159-805964c5-c4cc-411a-8865-3866fca8d6e9.png) * [x] Closes #8341 * [x] Tests added Co-authored by @DHowett (cherry picked from commit 210a98e44940f880175436b050151fb8c1e4b45d) Service-Card-Id: 84836597 Service-Version: 1.15 --- .../ConptyRoundtripTests.cpp | 73 +++++++++++++++++++ src/host/ut_host/ConptyOutputTests.cpp | 28 +++++++ src/inc/TestUtils.h | 8 +- src/renderer/vt/paint.cpp | 11 ++- 4 files changed, 114 insertions(+), 6 deletions(-) diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index 07f246b68ff..d5aee1d8b06 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -227,6 +227,8 @@ class TerminalCoreUnitTests::ConptyRoundtripTests final TEST_METHOD(SimpleAltBufferTest); TEST_METHOD(AltBufferToAltBufferTest); + TEST_METHOD(TestPowerLineFirstFrame); + TEST_METHOD(AltBufferResizeCrash); TEST_METHOD(TestNoExtendedAttrsOptimization); @@ -4111,6 +4113,77 @@ void ConptyRoundtripTests::AltBufferToAltBufferTest() verifyBuffer(*termAltTb, term->_GetMutableViewport().ToExclusive(), Frame::StillInAltBuffer); } +void ConptyRoundtripTests::TestPowerLineFirstFrame() +{ + Log::Comment(L"This is a test for GH#8341. If we received colored spaces " + L"BEFORE the first frame, we should still emit them!"); + + auto& g = ServiceLocator::LocateGlobals(); + auto& renderer = *g.pRender; + auto& gci = g.getConsoleInformation(); + auto& si = gci.GetActiveOutputBuffer(); + auto& sm = si.GetStateMachine(); + + auto* hostTb = &si.GetTextBuffer(); + auto* termTb = term->_mainBuffer.get(); + + _checkConptyOutput = false; + + TextAttribute whiteOnGreen{}; + whiteOnGreen.SetIndexedForeground(TextColor::DARK_WHITE); + whiteOnGreen.SetIndexedBackground(TextColor::BRIGHT_GREEN); + + TextAttribute greenOnBlack{}; + greenOnBlack.SetIndexedForeground(TextColor::BRIGHT_GREEN); + greenOnBlack.SetIndexedBackground(TextColor::BRIGHT_BLACK); + + TextAttribute whiteOnBlack{}; + whiteOnBlack.SetIndexedForeground(TextColor::DARK_WHITE); + whiteOnBlack.SetIndexedBackground(TextColor::BRIGHT_BLACK); + + TextAttribute blackOnDefault{}; + blackOnDefault.SetIndexedForeground(TextColor::BRIGHT_BLACK); + + TextAttribute defaultOnDefault{}; + + Log::Comment(L"========== Fill test content =========="); + + // As a pwsh one-liner: + // + // "`e[37m`e[102m foo\bar `e[92m`e[100m▶ `e[37mBar `e[90m`e[49m▶ `e[m" + // + // Generally taken from + // https://github.com/microsoft/terminal/issues/8341#issuecomment-731310022, + // but minimized for easier testing. + + sm.ProcessString(L"\x1b[37m\x1b[102m" // dark white on bright green + L" foo\\bar "); + sm.ProcessString(L"\x1b[92m\x1b[100m" // bright green on bright black + L"▶ "); + sm.ProcessString(L"\x1b[37m" // dark white on bright black + L"Bar "); + sm.ProcessString(L"\x1b[90m\x1b[49m" // bright black on default + L"▶ "); + sm.ProcessString(L"\x1b[m\n"); // default on default + + auto verifyBuffer = [&](const TextBuffer& tb) { + // If this test fails on character 8, then it's because we didn't emit the space, we just moved ahead. + auto iter0 = TestUtils::VerifyLineContains(tb, { 0, 0 }, whiteOnGreen, 9u); + TestUtils::VerifyLineContains(iter0, OutputCellIterator{ greenOnBlack, 2u }); + TestUtils::VerifyLineContains(iter0, OutputCellIterator{ whiteOnBlack, 4u }); + TestUtils::VerifyLineContains(iter0, OutputCellIterator{ blackOnDefault, 2u }); + }; + + Log::Comment(L"========== Check host buffer =========="); + verifyBuffer(*hostTb); + + Log::Comment(L"========== Paint first frame =========="); + VERIFY_SUCCEEDED(renderer.PaintFrame()); + + Log::Comment(L"========== Check terminal buffer =========="); + verifyBuffer(*termTb); +} + void ConptyRoundtripTests::AltBufferResizeCrash() { Log::Comment(L"During the review for GH#12719, it was noticed that this " diff --git a/src/host/ut_host/ConptyOutputTests.cpp b/src/host/ut_host/ConptyOutputTests.cpp index 29e0e28f20f..1ebff63cac2 100644 --- a/src/host/ut_host/ConptyOutputTests.cpp +++ b/src/host/ut_host/ConptyOutputTests.cpp @@ -121,6 +121,7 @@ class ConptyOutputTests TEST_METHOD(WriteAFewSimpleLines); TEST_METHOD(InvalidateUntilOneBeforeEnd); TEST_METHOD(SetConsoleTitleWithControlChars); + TEST_METHOD(IncludeBackgroundColorChangesInFirstFrame); private: bool _writeCallback(const char* const pch, const size_t cch); @@ -371,6 +372,7 @@ void ConptyOutputTests::SetConsoleTitleWithControlChars() { BEGIN_TEST_METHOD_PROPERTIES() TEST_METHOD_PROPERTY(L"Data:control", L"{0x00, 0x0A, 0x1B, 0x80, 0x9B, 0x9C}") + TEST_METHOD_PROPERTY(L"IsolationLevel", L"Method") END_TEST_METHOD_PROPERTIES() int control; @@ -400,3 +402,29 @@ void ConptyOutputTests::SetConsoleTitleWithControlChars() VERIFY_SUCCEEDED(renderer.PaintFrame()); } + +void ConptyOutputTests::IncludeBackgroundColorChangesInFirstFrame() +{ + auto& g = ServiceLocator::LocateGlobals(); + auto& renderer = *g.pRender; + auto& gci = g.getConsoleInformation(); + auto& si = gci.GetActiveOutputBuffer(); + auto& sm = si.GetStateMachine(); + + sm.ProcessString(L"\x1b[41mRun 1 \x1b[42mRun 2 \x1b[43mRun 3 \x1b[m"); + + expectedOutput.push_back("\x1b[2J"); // standard init sequence for the first frame + expectedOutput.push_back("\x1b[m"); // standard init sequence for the first frame + expectedOutput.push_back("\x1b[41m"); + expectedOutput.push_back("\x1b[H"); // standard init sequence for the first frame + expectedOutput.push_back("Run 1 "); + expectedOutput.push_back("\x1b[42m"); + expectedOutput.push_back("Run 2 "); + expectedOutput.push_back("\x1b[43m"); + expectedOutput.push_back("Run 3 "); + + // This is also part of the standard init sequence. + expectedOutput.push_back("\x1b[?25h"); + + VERIFY_SUCCEEDED(renderer.PaintFrame()); +} diff --git a/src/inc/TestUtils.h b/src/inc/TestUtils.h index de8a94f35e8..d6e8f146593 100644 --- a/src/inc/TestUtils.h +++ b/src/inc/TestUtils.h @@ -168,15 +168,19 @@ class TerminalCoreUnitTests::TestUtils auto actualAttrs = actual->TextAttr(); auto expectedAttrs = expected->TextAttr(); - auto mismatched = (actualChars != expectedChars || actualAttrs != expectedAttrs); + auto mismatched = ((!expectedChars.empty() && actualChars != expectedChars) || actualAttrs != expectedAttrs); if (mismatched) { WEX::Logging::Log::Comment(WEX::Common::NoThrowString().Format( L"Character or attribute at index %d was mismatched", charsProcessed)); } - VERIFY_ARE_EQUAL(expectedChars, actualChars); + if (!expectedChars.empty()) + { + VERIFY_ARE_EQUAL(expectedChars, actualChars); + } VERIFY_ARE_EQUAL(expectedAttrs, actualAttrs); + if (mismatched) { return false; diff --git a/src/renderer/vt/paint.cpp b/src/renderer/vt/paint.cpp index 42a7a2496d1..e1555ea18fb 100644 --- a/src/renderer/vt/paint.cpp +++ b/src/renderer/vt/paint.cpp @@ -453,10 +453,13 @@ using namespace Microsoft::Console::Types; // the lines _wrapped_. It doesn't care to manually break the lines, but if // we trimmed the spaces off here, we'd print all the "~"s one after another // on the same line. - const auto removeSpaces = !lineWrapped && (useEraseChar || - _clearedAllThisFrame || - (_newBottomLine && printingBottomLine && bgMatched)); - const auto cchActual = removeSpaces ? nonSpaceLength : cchLine; + static const TextAttribute defaultAttrs{}; + const bool removeSpaces = !lineWrapped && (useEraseChar // we determined earlier that ECH is optimal + || (_clearedAllThisFrame && _lastTextAttributes == defaultAttrs) // OR we cleared the last frame to the default attributes (specifically) + || (_newBottomLine && printingBottomLine && bgMatched)); // OR we just scrolled a new line onto the bottom of the screen with the correct attributes + const size_t cchActual = removeSpaces ? + (cchLine - numSpaces) : + cchLine; const auto columnsActual = removeSpaces ? (totalWidth - numSpaces) :