From 9454d15e816fa4229e25035766259d7889c035ef Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 26 Apr 2024 16:28:17 -0500 Subject: [PATCH 1/8] start with a test --- .../ConptyRoundtripTests.cpp | 145 ++++++++++++++++-- 1 file changed, 128 insertions(+), 17 deletions(-) diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index 4c582d0f44a..39eb3c5ae5f 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -239,12 +239,14 @@ class TerminalCoreUnitTests::ConptyRoundtripTests final TEST_METHOD(MultilinePromptRegions); TEST_METHOD(ManyMultilinePromptsWithTrailingSpaces); TEST_METHOD(ReflowPromptRegions); + TEST_METHOD(ClearMarksTest); private: bool _writeCallback(const char* const pch, const size_t cch); void _flushFirstFrame(); void _resizeConpty(const til::CoordType sx, const til::CoordType sy); void _clearConpty(); + void _clear(int clearBufferMethod, SCREEN_INFORMATION& si); [[nodiscard]] std::tuple _performResize(const til::size newSize); @@ -2720,9 +2722,6 @@ void ConptyRoundtripTests::ClsAndClearHostClearsScrollbackTest() BEGIN_TEST_METHOD_PROPERTIES() TEST_METHOD_PROPERTY(L"Data:clearBufferMethod", L"{0, 1, 2}") END_TEST_METHOD_PROPERTIES(); - constexpr auto ClearLikeCls = 0; - constexpr auto ClearLikeClearHost = 1; - constexpr auto ClearWithVT = 2; INIT_TEST_PROPERTY(int, clearBufferMethod, L"Controls whether we clear the buffer like cmd or like powershell"); Log::Comment(L"This test checks the shims for cmd.exe and powershell.exe. " @@ -2789,6 +2788,31 @@ void ConptyRoundtripTests::ClsAndClearHostClearsScrollbackTest() VERIFY_ARE_EQUAL(si.GetViewport().Dimensions(), si.GetBufferSize().Dimensions()); VERIFY_ARE_EQUAL(si.GetViewport(), si.GetBufferSize()); + _clear(clearBufferMethod, si); + + VERIFY_ARE_EQUAL(si.GetViewport().Dimensions(), si.GetBufferSize().Dimensions()); + VERIFY_ARE_EQUAL(si.GetViewport(), si.GetBufferSize()); + + Log::Comment(L"Painting the frame"); + VERIFY_SUCCEEDED(renderer.PaintFrame()); + + Log::Comment(L"========== Checking the host buffer state (after) =========="); + verifyBuffer(*hostTb, si.GetViewport().ToExclusive(), true); + + Log::Comment(L"Painting the frame"); + VERIFY_SUCCEEDED(renderer.PaintFrame()); + Log::Comment(L"========== Checking the terminal buffer state (after) =========="); + verifyBuffer(*termTb, term->_mutableViewport.ToExclusive(), true); +} + +void ConptyRoundtripTests::_clear(int clearBufferMethod, SCREEN_INFORMATION& si) +{ + constexpr auto ClearLikeCls = 0; + constexpr auto ClearLikeClearHost = 1; + constexpr auto ClearWithVT = 2; + + auto& sm = si.GetStateMachine(); + if (clearBufferMethod == ClearLikeCls) { // Execute the cls, EXACTLY LIKE CMD. @@ -2840,20 +2864,6 @@ void ConptyRoundtripTests::ClsAndClearHostClearsScrollbackTest() sm.ProcessString(L"\x1b[3J"); } - - VERIFY_ARE_EQUAL(si.GetViewport().Dimensions(), si.GetBufferSize().Dimensions()); - VERIFY_ARE_EQUAL(si.GetViewport(), si.GetBufferSize()); - - Log::Comment(L"Painting the frame"); - VERIFY_SUCCEEDED(renderer.PaintFrame()); - - Log::Comment(L"========== Checking the host buffer state (after) =========="); - verifyBuffer(*hostTb, si.GetViewport().ToExclusive(), true); - - Log::Comment(L"Painting the frame"); - VERIFY_SUCCEEDED(renderer.PaintFrame()); - Log::Comment(L"========== Checking the terminal buffer state (after) =========="); - verifyBuffer(*termTb, term->_mutableViewport.ToExclusive(), true); } void ConptyRoundtripTests::TestResizeWithCookedRead() @@ -4945,3 +4955,104 @@ void ConptyRoundtripTests::ReflowPromptRegions() Log::Comment(L"========== Checking the terminal buffer state (after) =========="); verifyBuffer(*termTb, term->_mutableViewport.ToExclusive(), true, true); } + +void ConptyRoundtripTests::ClearMarksTest() +{ + BEGIN_TEST_METHOD_PROPERTIES() + TEST_METHOD_PROPERTY(L"Data:clearBufferMethod", L"{0, 1, 2}") + END_TEST_METHOD_PROPERTIES(); + + INIT_TEST_PROPERTY(int, clearBufferMethod, L"Controls whether we clear the buffer like cmd or like powershell"); + + Log::Comment(L"This test checks the shims for cmd.exe and powershell.exe. " + L"Their build in commands for clearing the console buffer " + L"should work to clear the terminal buffer, not just the " + L"terminal viewport."); + + auto& g = ServiceLocator::LocateGlobals(); + auto& renderer = *g.pRender; + auto& gci = g.getConsoleInformation(); + auto& si = gci.GetActiveOutputBuffer(); + auto* hostTb = &si.GetTextBuffer(); + auto* termTb = term->_mainBuffer.get(); + + auto& sm = si.GetStateMachine(); + + _flushFirstFrame(); + + _checkConptyOutput = false; + _logConpty = false; + + const auto hostView = si.GetViewport(); + const auto end = 2 * hostView.Height(); + + auto writePrompt = [](StateMachine& stateMachine, const auto& path) { + // A prompt looks like: + // `PWSH C:\> ` + // + // which is 10 characters for "C:\" + stateMachine.ProcessString(FTCS_D); + stateMachine.ProcessString(FTCS_A); + stateMachine.ProcessString(L"\x1b]9;9;"); + stateMachine.ProcessString(path); + stateMachine.ProcessString(L"\x7"); + stateMachine.ProcessString(L"PWSH "); + stateMachine.ProcessString(path); + stateMachine.ProcessString(L"> "); + stateMachine.ProcessString(FTCS_B); + }; + auto writeCommand = [](StateMachine& stateMachine, const auto& cmd) { + stateMachine.ProcessString(cmd); + stateMachine.ProcessString(FTCS_C); + stateMachine.ProcessString(L"\r\n"); + }; + + for (auto i = 0; i < end; i++) + { + writePrompt(sm, L"C:\\"); + writeCommand(sm, L"Foo-bar"); + sm.ProcessString(L"This is some text \r\n"); + } + writePrompt(sm, L"C:\\"); + + auto verifyBuffer = [&](const TextBuffer& tb, const til::rect& /*viewport*/, const bool afterClear = false) { + const WEX::TestExecution::DisableVerifyExceptions disableExceptionsScope; + const auto marks = tb.GetMarkExtents(); + if (afterClear) + { + VERIFY_ARE_EQUAL(0u, marks.size()); + } + else + { + VERIFY_IS_GREATER_THAN(marks.size(), 1u, L"There should be at least one mark"); + } + }; + + Log::Comment(L"========== Checking the host buffer state (before) =========="); + verifyBuffer(*hostTb, si.GetViewport().ToExclusive()); + + Log::Comment(L"Painting the frame"); + VERIFY_SUCCEEDED(renderer.PaintFrame()); + + Log::Comment(L"========== Checking the terminal buffer state (before) =========="); + verifyBuffer(*termTb, term->_mutableViewport.ToExclusive()); + + VERIFY_ARE_EQUAL(si.GetViewport().Dimensions(), si.GetBufferSize().Dimensions()); + VERIFY_ARE_EQUAL(si.GetViewport(), si.GetBufferSize()); + + _clear(clearBufferMethod, si); + + VERIFY_ARE_EQUAL(si.GetViewport().Dimensions(), si.GetBufferSize().Dimensions()); + VERIFY_ARE_EQUAL(si.GetViewport(), si.GetBufferSize()); + + Log::Comment(L"Painting the frame"); + VERIFY_SUCCEEDED(renderer.PaintFrame()); + + Log::Comment(L"========== Checking the host buffer state (after) =========="); + verifyBuffer(*hostTb, si.GetViewport().ToExclusive(), true); + + Log::Comment(L"Painting the frame"); + VERIFY_SUCCEEDED(renderer.PaintFrame()); + Log::Comment(L"========== Checking the terminal buffer state (after) =========="); + verifyBuffer(*termTb, term->_mutableViewport.ToExclusive(), true); +} From 3e677cc0e35392ce4dc279bc345121d3512d32bb Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 26 Apr 2024 16:57:23 -0500 Subject: [PATCH 2/8] actually fix this --- src/buffer/out/textBuffer.cpp | 4 ++-- .../UnitTests_TerminalCore/ConptyRoundtripTests.cpp | 6 ------ src/terminal/adapter/adaptDispatch.cpp | 4 ---- 3 files changed, 2 insertions(+), 12 deletions(-) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index 1b5b28c621b..6b9afa512a8 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -1189,6 +1189,8 @@ void TextBuffer::ClearScrollback(const til::CoordType start, const til::CoordTyp return; } + ClearMarksInRange(til::point{ 0, 0 }, til::point{ _width, height }); + // Our goal is to move the viewport to the absolute start of the underlying memory buffer so that we can // MEM_DECOMMIT the remaining memory. _firstRow is used to make the TextBuffer behave like a circular buffer. // The start parameter is relative to the _firstRow. The trick to get the content to the absolute start @@ -1204,8 +1206,6 @@ void TextBuffer::ClearScrollback(const til::CoordType start, const til::CoordTyp { GetMutableRowByOffset(y).Reset(_initialAttributes); } - - ClearMarksInRange(til::point{ 0, height }, til::point{ _width, _height }); } // Routine Description: diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index 39eb3c5ae5f..2dc64562e88 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -5045,12 +5045,6 @@ void ConptyRoundtripTests::ClearMarksTest() VERIFY_ARE_EQUAL(si.GetViewport().Dimensions(), si.GetBufferSize().Dimensions()); VERIFY_ARE_EQUAL(si.GetViewport(), si.GetBufferSize()); - Log::Comment(L"Painting the frame"); - VERIFY_SUCCEEDED(renderer.PaintFrame()); - - Log::Comment(L"========== Checking the host buffer state (after) =========="); - verifyBuffer(*hostTb, si.GetViewport().ToExclusive(), true); - Log::Comment(L"Painting the frame"); VERIFY_SUCCEEDED(renderer.PaintFrame()); Log::Comment(L"========== Checking the terminal buffer state (after) =========="); diff --git a/src/terminal/adapter/adaptDispatch.cpp b/src/terminal/adapter/adaptDispatch.cpp index 34bc5cd80e6..bf038dd75ee 100644 --- a/src/terminal/adapter/adaptDispatch.cpp +++ b/src/terminal/adapter/adaptDispatch.cpp @@ -3294,10 +3294,6 @@ bool AdaptDispatch::_EraseAll() // Also reset the line rendition for the erased rows. textBuffer.ResetLineRenditionRange(newViewportTop, newViewportBottom); - // Clear any marks that remain below the start of the - textBuffer.ClearMarksInRange(til::point{ 0, newViewportTop }, - til::point{ bufferSize.Width(), bufferSize.Height() }); - // GH#5683 - If this succeeded, but we're in a conpty, return `false` to // make the state machine propagate this ED sequence to the connected // terminal application. While we're in conpty mode, when the client From 010e1f60a1f96768f3631c1e0772b9da8d33dfef Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Mon, 29 Apr 2024 09:55:42 -0500 Subject: [PATCH 3/8] sure, this seems to work too --- src/buffer/out/textBuffer.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index 6b9afa512a8..d8185f4a2c1 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -1183,14 +1183,14 @@ void TextBuffer::ClearScrollback(const til::CoordType start, const til::CoordTyp return; } + ClearMarksInRange(til::point{ 0, start }, til::point{ _width, start + height }); + if (height <= 0) { _decommit(); return; } - ClearMarksInRange(til::point{ 0, 0 }, til::point{ _width, height }); - // Our goal is to move the viewport to the absolute start of the underlying memory buffer so that we can // MEM_DECOMMIT the remaining memory. _firstRow is used to make the TextBuffer behave like a circular buffer. // The start parameter is relative to the _firstRow. The trick to get the content to the absolute start From 1a1c6338ed94d852861bca939e52478960704839 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 1 May 2024 10:09:03 -0500 Subject: [PATCH 4/8] Revert "sure, this seems to work too" This reverts commit 010e1f60a1f96768f3631c1e0772b9da8d33dfef. --- src/buffer/out/textBuffer.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index 6de34314bf9..a048c8a52de 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -1183,14 +1183,14 @@ void TextBuffer::ClearScrollback(const til::CoordType start, const til::CoordTyp return; } - ClearMarksInRange(til::point{ 0, start }, til::point{ _width, start + height }); - if (height <= 0) { _decommit(); return; } + ClearMarksInRange(til::point{ 0, 0 }, til::point{ _width, height }); + // Our goal is to move the viewport to the absolute start of the underlying memory buffer so that we can // MEM_DECOMMIT the remaining memory. _firstRow is used to make the TextBuffer behave like a circular buffer. // The start parameter is relative to the _firstRow. The trick to get the content to the absolute start From d40afd861b2365aae0f4972842cf9d3d35673e7f Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 2 May 2024 11:40:48 -0500 Subject: [PATCH 5/8] i know, i'll push right before lunch --- src/buffer/out/textBuffer.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index a048c8a52de..957ec3274c3 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -1189,7 +1189,7 @@ void TextBuffer::ClearScrollback(const til::CoordType start, const til::CoordTyp return; } - ClearMarksInRange(til::point{ 0, 0 }, til::point{ _width, height }); + ClearMarksInRange(til::point{ 0, start }, til::point{ _width, start + height }); // Our goal is to move the viewport to the absolute start of the underlying memory buffer so that we can // MEM_DECOMMIT the remaining memory. _firstRow is used to make the TextBuffer behave like a circular buffer. From 257818c2687421759e0f618d703dfa351672e649 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 2 May 2024 12:36:17 -0500 Subject: [PATCH 6/8] and thats why you always leave a note --- src/buffer/out/textBuffer.cpp | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index 957ec3274c3..8e0975a91cf 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -1176,20 +1176,25 @@ void TextBuffer::Reset() noexcept _initialAttributes = _currentAttributes; } +// Arguments: +// - start: The y-position of the viewport. We'll clear up until here. +// - height: the number of rows to keep in the buffer. void TextBuffer::ClearScrollback(const til::CoordType start, const til::CoordType height) { + // We're already at the top? don't clear ahything. There's no scrollback. if (start <= 0) { return; } + // The new viewport should keep 0 rows? Then just reset everything. if (height <= 0) { _decommit(); return; } - ClearMarksInRange(til::point{ 0, start }, til::point{ _width, start + height }); + ClearMarksInRange(til::point{ 0, 0 }, til::point{ _width, start + height }); // Our goal is to move the viewport to the absolute start of the underlying memory buffer so that we can // MEM_DECOMMIT the remaining memory. _firstRow is used to make the TextBuffer behave like a circular buffer. From 29b1b00a50e4b2fdbd99b7d71243036463df1b23 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 2 May 2024 13:57:10 -0500 Subject: [PATCH 7/8] rename --- src/buffer/out/textBuffer.cpp | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index 8e0975a91cf..25a12694560 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -1177,37 +1177,36 @@ void TextBuffer::Reset() noexcept } // Arguments: -// - start: The y-position of the viewport. We'll clear up until here. -// - height: the number of rows to keep in the buffer. -void TextBuffer::ClearScrollback(const til::CoordType start, const til::CoordType height) +// - newFirstRow: The current y-position of the viewport. We'll clear up until here. +// - rowsToKeep: the number of rows to keep in the buffer. +void TextBuffer::ClearScrollback(const til::CoordType newFirstRow, const til::CoordType rowsToKeep) { - // We're already at the top? don't clear ahything. There's no scrollback. - if (start <= 0) + // We're already at the top? don't clear anything. There's no scrollback. + if (newFirstRow <= 0) { return; } - // The new viewport should keep 0 rows? Then just reset everything. - if (height <= 0) + if (rowsToKeep <= 0) { _decommit(); return; } - ClearMarksInRange(til::point{ 0, 0 }, til::point{ _width, start + height }); + ClearMarksInRange(til::point{ 0, 0 }, til::point{ _width, newFirstRow + rowsToKeep }); // Our goal is to move the viewport to the absolute start of the underlying memory buffer so that we can // MEM_DECOMMIT the remaining memory. _firstRow is used to make the TextBuffer behave like a circular buffer. - // The start parameter is relative to the _firstRow. The trick to get the content to the absolute start + // The newFirstRow parameter is relative to the _firstRow. The trick to get the content to the absolute start // is to simply add _firstRow ourselves and then reset it to 0. This causes ScrollRows() to write into // the absolute start while reading from relative coordinates. This works because GetRowByOffset() // operates modulo the buffer height and so the possibly-too-large startAbsolute won't be an issue. - const auto startAbsolute = _firstRow + start; + const auto startAbsolute = _firstRow + newFirstRow; _firstRow = 0; - ScrollRows(startAbsolute, height, -startAbsolute); + ScrollRows(startAbsolute, rowsToKeep, -startAbsolute); const auto end = _estimateOffsetOfLastCommittedRow(); - for (auto y = height; y <= end; ++y) + for (auto y = rowsToKeep; y <= end; ++y) { GetMutableRowByOffset(y).Reset(_initialAttributes); } From a8ecd7219629303bc9ebf20d561c374bae7ef540 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 2 May 2024 14:09:51 -0500 Subject: [PATCH 8/8] sure --- src/buffer/out/textBuffer.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index 25a12694560..2e1f8c9a651 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -1193,7 +1193,7 @@ void TextBuffer::ClearScrollback(const til::CoordType newFirstRow, const til::Co return; } - ClearMarksInRange(til::point{ 0, 0 }, til::point{ _width, newFirstRow + rowsToKeep }); + ClearMarksInRange(til::point{ 0, 0 }, til::point{ _width, std::max(0, newFirstRow - 1) }); // Our goal is to move the viewport to the absolute start of the underlying memory buffer so that we can // MEM_DECOMMIT the remaining memory. _firstRow is used to make the TextBuffer behave like a circular buffer.