From bc48eda0223a63a2645f846db831613d107af0d8 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Mon, 29 Jan 2024 23:49:42 +0100 Subject: [PATCH] Reset _wrapForced when erasing scrollback (#16610) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #15541 changed `AdaptDispatch::_FillRect` which caused it to not affect the `ROW::_wrapForced` flag anymore. This change in behavior was not noticeable as `TextBuffer::GetLastNonSpaceCharacter` had a bug where rows of only whitespace text would always be treated as empty. This would then affect `AdaptDispatch::_EraseAll` to accidentally correctly guess the last row with text despite the `_FillRect` change. #15701 then fixed `GetLastNonSpaceCharacter` indirectly by fixing `ROW::MeasureRight` which now made the previous change apparent. `_EraseAll` would now guess the last row of text incorrectly, because it would find the rows that `_FillRect` cleared but still had `_wrapForced` set to `true`. This PR fixes the issue by replacing the `_FillRect` usage to clear rows with direct calls to `ROW::Reset()`. In the future this could be extended by also `MEM_DECOMMIT`ing the now unused underlying memory. Closes #16603 ## Validation Steps Performed * Enter WSL and resize the window to <40 columns * Execute ```sh cd /bin ls -la printf "\e[3J" ls -la printf "\e[3J" printf "\e[2J" ``` * Only one viewport-height-many lines of whitespace exist between the current prompt line and the previous scrollback contents ✅ (cherry picked from commit 5f71cf3e94c35fb052632aa8368f01cae77b89c6) Service-Card-Id: 91707937 Service-Version: 1.19 --- src/buffer/out/textBuffer.cpp | 57 +++++++++++++++++++++----- src/buffer/out/textBuffer.hpp | 1 + src/host/ut_host/ScreenBufferTests.cpp | 3 +- src/terminal/adapter/adaptDispatch.cpp | 13 +----- 4 files changed, 51 insertions(+), 23 deletions(-) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index 52a5c6e3b72..600f1b182ff 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -126,6 +126,8 @@ void TextBuffer::_reserve(til::size screenBufferSize, const TextAttribute& defau // The compiler doesn't understand the likelihood of our branches. (PGO does, but that's imperfect.) __declspec(noinline) void TextBuffer::_commit(const std::byte* row) { + assert(row >= _commitWatermark); + const auto rowEnd = row + _bufferRowStride; const auto remaining = gsl::narrow_cast(_bufferEnd - _commitWatermark); const auto minimum = gsl::narrow_cast(rowEnd - _commitWatermark); @@ -146,7 +148,7 @@ void TextBuffer::_decommit() noexcept _commitWatermark = _buffer.get(); } -// Constructs ROWs up to (excluding) the ROW pointed to by `until`. +// Constructs ROWs between [_commitWatermark,until). void TextBuffer::_construct(const std::byte* until) noexcept { for (; _commitWatermark < until; _commitWatermark += _bufferRowStride) @@ -158,8 +160,7 @@ void TextBuffer::_construct(const std::byte* until) noexcept } } -// Destroys all previously constructed ROWs. -// Be careful! This doesn't reset any of the members, in particular the _commitWatermark. +// Destructs ROWs between [_buffer,_commitWatermark). void TextBuffer::_destroy() const noexcept { for (auto it = _buffer.get(); it < _commitWatermark; it += _bufferRowStride) @@ -168,9 +169,8 @@ void TextBuffer::_destroy() const noexcept } } -// This function is "direct" because it trusts the caller to properly wrap the "offset" -// parameter modulo the _height of the buffer, etc. But keep in mind that a offset=0 -// is the GetScratchpadRow() and not the GetRowByOffset(0). That one is offset=1. +// This function is "direct" because it trusts the caller to properly +// wrap the "offset" parameter modulo the _height of the buffer. ROW& TextBuffer::_getRowByOffsetDirect(size_t offset) { const auto row = _buffer.get() + _bufferRowStride * offset; @@ -184,6 +184,7 @@ ROW& TextBuffer::_getRowByOffsetDirect(size_t offset) return *reinterpret_cast(row); } +// See GetRowByOffset(). ROW& TextBuffer::_getRow(til::CoordType y) const { // Rows are stored circularly, so the index you ask for is offset by the start position and mod the total of rows. @@ -197,6 +198,7 @@ ROW& TextBuffer::_getRow(til::CoordType y) const } // We add 1 to the row offset, because row "0" is the one returned by GetScratchpadRow(). + // See GetScratchpadRow() for more explanation. #pragma warning(suppress : 26492) // Don't use const_cast to cast away const or volatile (type.3). return const_cast(this)->_getRowByOffsetDirect(gsl::narrow_cast(offset) + 1); } @@ -238,6 +240,9 @@ ROW& TextBuffer::GetScratchpadRow() // Returns a row filled with whitespace and the given attributes, for you to freely use. ROW& TextBuffer::GetScratchpadRow(const TextAttribute& attributes) { + // The scratchpad row is mapped to the underlying index 0, whereas all regular rows are mapped to + // index 1 and up. We do it this way instead of the other way around (scratchpad row at index _height), + // because that would force us to MEM_COMMIT the entire buffer whenever this function is called. auto& r = _getRowByOffsetDirect(0); r.Reset(attributes); return r; @@ -902,15 +907,14 @@ til::point TextBuffer::GetLastNonSpaceCharacter(const Viewport* viewOptional) co // If the X coordinate turns out to be -1, the row was empty, we need to search backwards for the real end of text. const auto viewportTop = viewport.Top(); - auto fDoBackUp = (coordEndOfText.x < 0 && coordEndOfText.y > viewportTop); // this row is empty, and we're not at the top - while (fDoBackUp) + + // while (this row is empty, and we're not at the top) + while (coordEndOfText.x < 0 && coordEndOfText.y > viewportTop) { coordEndOfText.y--; const auto& backupRow = GetRowByOffset(coordEndOfText.y); // We need to back up to the previous row if this line is empty, AND there are more rows - coordEndOfText.x = backupRow.MeasureRight() - 1; - fDoBackUp = (coordEndOfText.x < 0 && coordEndOfText.y > viewportTop); } // don't allow negative results @@ -1146,6 +1150,39 @@ void TextBuffer::Reset() noexcept _initialAttributes = _currentAttributes; } +void TextBuffer::ClearScrollback(const til::CoordType start, const til::CoordType height) +{ + if (start <= 0) + { + return; + } + + if (height <= 0) + { + _decommit(); + return; + } + + // 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 + // 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; + _firstRow = 0; + ScrollRows(startAbsolute, height, -startAbsolute); + + const auto end = _estimateOffsetOfLastCommittedRow(); + for (auto y = height; y <= end; ++y) + { + GetMutableRowByOffset(y).Reset(_initialAttributes); + } + + ScrollMarks(-start); + ClearMarksInRange(til::point{ 0, height }, til::point{ _width, _height }); +} + // Routine Description: // - This is the legacy screen resize with minimal changes // Arguments: diff --git a/src/buffer/out/textBuffer.hpp b/src/buffer/out/textBuffer.hpp index 8ba16f97e75..39cc9fe9a59 100644 --- a/src/buffer/out/textBuffer.hpp +++ b/src/buffer/out/textBuffer.hpp @@ -194,6 +194,7 @@ class TextBuffer final til::point BufferToScreenPosition(const til::point position) const; void Reset() noexcept; + void ClearScrollback(const til::CoordType start, const til::CoordType height); void ResizeTraditional(const til::size newSize); diff --git a/src/host/ut_host/ScreenBufferTests.cpp b/src/host/ut_host/ScreenBufferTests.cpp index d6687e19bd6..98a6f09b2e5 100644 --- a/src/host/ut_host/ScreenBufferTests.cpp +++ b/src/host/ut_host/ScreenBufferTests.cpp @@ -4515,6 +4515,7 @@ void ScreenBufferTests::EraseScrollbackTests() auto& si = gci.GetActiveOutputBuffer().GetActiveBuffer(); auto& stateMachine = si.GetStateMachine(); const auto& cursor = si.GetTextBuffer().GetCursor(); + const auto initialAttributes = si.GetAttributes(); WI_SetFlag(si.OutputMode, ENABLE_VIRTUAL_TERMINAL_PROCESSING); const auto bufferWidth = si.GetBufferSize().Width(); @@ -4571,7 +4572,7 @@ void ScreenBufferTests::EraseScrollbackTests() } Log::Comment(L"The rest of the buffer should be cleared with default attributes."); - VERIFY_IS_TRUE(_ValidateLinesContain(viewportLine, bufferHeight, L' ', TextAttribute{})); + VERIFY_IS_TRUE(_ValidateLinesContain(viewportLine, bufferHeight, L' ', initialAttributes)); } void ScreenBufferTests::EraseTests() diff --git a/src/terminal/adapter/adaptDispatch.cpp b/src/terminal/adapter/adaptDispatch.cpp index b6e07b77ffb..fb504c20200 100644 --- a/src/terminal/adapter/adaptDispatch.cpp +++ b/src/terminal/adapter/adaptDispatch.cpp @@ -3200,18 +3200,7 @@ bool AdaptDispatch::_EraseScrollback() auto& cursor = textBuffer.GetCursor(); const auto row = cursor.GetPosition().y; - // Clear all the marks below the new viewport position. - textBuffer.ClearMarksInRange(til::point{ 0, height }, - til::point{ bufferSize.width, bufferSize.height }); - // Then scroll all the remaining marks up. This will trim ones that are now "outside" the buffer - textBuffer.ScrollMarks(-top); - - // Scroll the viewport content to the top of the buffer. - textBuffer.ScrollRows(top, height, -top); - // Clear everything after the viewport. - _FillRect(textBuffer, { 0, height, bufferSize.width, bufferSize.height }, whitespace, {}); - // Also reset the line rendition for all of the cleared rows. - textBuffer.ResetLineRenditionRange(height, bufferSize.height); + textBuffer.ClearScrollback(top, height); // Move the viewport _api.SetViewportPosition({ viewport.left, 0 }); // Move the cursor to the same relative location.