Skip to content

Commit

Permalink
Reset _wrapForced when erasing scrollback (#16610)
Browse files Browse the repository at this point in the history
#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 5f71cf3)
Service-Card-Id: 91707937
Service-Version: 1.19
  • Loading branch information
lhecker authored and DHowett committed Jan 29, 2024
1 parent 29895e1 commit bc48eda
Show file tree
Hide file tree
Showing 4 changed files with 51 additions and 23 deletions.
57 changes: 47 additions & 10 deletions src/buffer/out/textBuffer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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<uintptr_t>(_bufferEnd - _commitWatermark);
const auto minimum = gsl::narrow_cast<uintptr_t>(rowEnd - _commitWatermark);
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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;
Expand All @@ -184,6 +184,7 @@ ROW& TextBuffer::_getRowByOffsetDirect(size_t offset)
return *reinterpret_cast<ROW*>(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.
Expand All @@ -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<TextBuffer*>(this)->_getRowByOffsetDirect(gsl::narrow_cast<size_t>(offset) + 1);
}
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand Down
1 change: 1 addition & 0 deletions src/buffer/out/textBuffer.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
3 changes: 2 additions & 1 deletion src/host/ut_host/ScreenBufferTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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()
Expand Down
13 changes: 1 addition & 12 deletions src/terminal/adapter/adaptDispatch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down

1 comment on commit bc48eda

@github-actions
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@check-spelling-bot Report

🔴 Please review

See the 📜action log for details.

Unrecognized words (13)
atvis
CProc
hpj
LPCCH
NONCONST
rgi
spammy
tokeninfo
traceloggingprovider
userbase
VProc
VRaw
wcsnicmp
Previously acknowledged words that are now absent DESTINATIONNAME :arrow_right:
To accept ✔️ these unrecognized words as correct and remove the previously acknowledged and now absent words, run the following commands

... in a clone of the [email protected]:microsoft/terminal.git repository
on the release-1.19 branch (ℹ️ how do I use this?):

curl -s -S -L 'https://raw.githubusercontent.com/check-spelling/check-spelling/v0.0.21/apply.pl' |
perl - 'https://github.com/microsoft/terminal/actions/runs/7703721618/attempts/1'
Pattern suggestions ✂️ (1)

You could add these patterns to .github/actions/spelling/patterns/bc48eda0223a63a2645f846db831613d107af0d8.txt:

# Automatically suggested patterns
# hit-count: 1 file-count: 1
# tput arguments -- https://man7.org/linux/man-pages/man5/terminfo.5.html -- technically they can be more than 5 chars long...
\btput\s+(?:(?:-[SV]|-T\s*\w+)\s+)*\w{3,5}\b

Warnings (1)

See the 📜action log for details.

ℹ️ Warnings Count
ℹ️ candidate-pattern 1

See ℹ️ Event descriptions for more information.

✏️ Contributor please read this

By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.

⚠️ The command is written for posix shells. If it doesn't work for you, you can manually add (one word per line) / remove items to expect.txt and the excludes.txt files.

If the listed items are:

  • ... misspelled, then please correct them instead of using the command.
  • ... names, please add them to .github/actions/spelling/allow/names.txt.
  • ... APIs, you can add them to a file in .github/actions/spelling/allow/.
  • ... just things you're using, please add them to an appropriate file in .github/actions/spelling/expect/.
  • ... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in .github/actions/spelling/patterns/.

See the README.md in each directory for more information.

🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉

If the flagged items are 🤯 false positives

If items relate to a ...

  • binary file (or some other file you wouldn't want to check at all).

    Please add a file path to the excludes.txt file matching the containing file.

    File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

    ^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).

  • well-formed pattern.

    If you can write a pattern that would match it,
    try adding it to the patterns.txt file.

    Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines.

    Note that patterns can't match multiline strings.

Please sign in to comment.