Fix clearing marks (#17144)

Tests are good, I should write more of them. 


Closes #17130

src/buffer/out/textBuffer.cpp

@@ -1176,36 +1176,40 @@ void TextBuffer::Reset() noexcept
  _initialAttributes = _currentAttributes;
 }
 
-void TextBuffer::ClearScrollback(const til::CoordType start, const til::CoordType height)
+// Arguments:
+// - 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)
 {
- if (start <= 0)
+ // We're already at the top? don't clear anything. There's no scrollback.
+ if (newFirstRow <= 0)
  {
  return;
  }
-
- if (height <= 0)
+ // The new viewport should keep 0 rows? Then just reset everything.
+ if (rowsToKeep <= 0)
  {
  _decommit();
  return;
  }
 
+ 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.
- // 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);
  }
-
- ClearMarksInRange(til::point{ 0, height }, til::point{ _width, _height });
 }
 
 // Routine Description:

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<TextBuffer*, TextBuffer*> _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,98 @@ 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 terminal buffer state (after) ==========");
+ verifyBuffer(*termTb, term->_mutableViewport.ToExclusive(), true);
+}

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