Skip to content

Commit

Permalink
Manually copy trailing attributes on a resize (#12637)
Browse files Browse the repository at this point in the history
## THE WHITE WHALE

This is a fairly naive fix for this bug. It's not terribly performant,
but neither is resize in the first place.

When the buffer gets resized, typically we only copy the text up to the
`MeasureRight` point, the last printable char in the row. Then we'd just
use the last char's attributes to fill the remainder of the row.

Instead, this PR changes how reflow behaves when it gets to the end of
the row. After we finish copying text, then manually walk through the
attributes at the end of the row, and copy them over. This ensures that
cells that just have a colored space in them get copied into the new
buffer as well, and we don't just blat the last character's attributes
into the rest of the row. We'll do a similar thing once we get to the
last printable char in the buffer, copying the remaining attributes.

This could DEFINITELY be more performant. I think this current
implementation walks the attrs _on every cell_, then appends the new
attrs to the new ATTR_ROW. That could be optimized by just using the
actual iterator. The copy after the last printable char bit is also
especially bad in this regard. That could likely be a blind copy - I
just wanted to get this into the world.

Finally, we now copy the final attributes to the correct buffer: the new
one.  We used to copy them to the _old_ buffer, which we were about to
destroy.

## Validation

I'll add more gifs in the morning, not enough time to finish spinning a
release Terminal build with this tonight.

Closes #32 🎉🎉🎉🎉🎉🎉🎉🎉🎉
Closes #12567

(cherry picked from commit 855e136)
  • Loading branch information
zadjii-msft authored and DHowett committed Mar 28, 2022
1 parent ca93ad3 commit ef2e0ad
Show file tree
Hide file tree
Showing 5 changed files with 334 additions and 8 deletions.
2 changes: 2 additions & 0 deletions build/pipelines/templates/build-console-steps.yml
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,8 @@ steps:
$(Build.SourcesDirectory)/bin/$(RationalizedBuildPlatform)/$(BuildConfiguration)/*.dll
$(Build.SourcesDirectory)/bin/$(RationalizedBuildPlatform)/$(BuildConfiguration)/*.xml
**/Microsoft.VCLibs.*.appx
**/*unit.test*.dll
**/*unit.test*.manifest
**/TestHostApp/*.exe
**/TestHostApp/*.dll
**/TestHostApp/*.xml
Expand Down
93 changes: 87 additions & 6 deletions src/buffer/out/textBuffer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2197,7 +2197,8 @@ HRESULT TextBuffer::Reflow(TextBuffer& oldBuffer,
bool foundOldVisible = false;
HRESULT hr = S_OK;
// Loop through all the rows of the old buffer and reprint them into the new buffer
for (short iOldRow = 0; iOldRow < cOldRowsTotal; iOldRow++)
short iOldRow = 0;
for (; iOldRow < cOldRowsTotal; iOldRow++)
{
// Fetch the row and its "right" which is the last printable character.
const ROW& row = oldBuffer.GetRowByOffset(iOldRow);
Expand Down Expand Up @@ -2241,7 +2242,11 @@ HRESULT TextBuffer::Reflow(TextBuffer& oldBuffer,
// Loop through every character in the current row (up to
// the "right" boundary, which is one past the final valid
// character)
for (short iOldCol = 0; iOldCol < iRight; iOldCol++)
short iOldCol = 0;
auto chars{ row.GetCharRow().cbegin() };
auto attrs{ row.GetAttrRow().begin() };
const auto copyRight = iRight;
for (; iOldCol < copyRight; iOldCol++)
{
if (iOldCol == cOldCursorPos.X && iOldRow == cOldCursorPos.Y)
{
Expand All @@ -2252,9 +2257,9 @@ HRESULT TextBuffer::Reflow(TextBuffer& oldBuffer,
try
{
// TODO: MSFT: 19446208 - this should just use an iterator and the inserter...
const auto glyph = row.GetCharRow().GlyphAt(iOldCol);
const auto dbcsAttr = row.GetCharRow().DbcsAttrAt(iOldCol);
const auto textAttr = row.GetAttrRow().GetAttrByColumn(iOldCol);
const auto glyph = chars->Char();
const auto dbcsAttr = chars->DbcsAttr();
const auto textAttr = *attrs;

if (!newBuffer.InsertCharacter(glyph, dbcsAttr, textAttr))
{
Expand All @@ -2263,6 +2268,54 @@ HRESULT TextBuffer::Reflow(TextBuffer& oldBuffer,
}
}
CATCH_RETURN();

++chars;
++attrs;
}

// GH#32: Copy the attributes from the rest of the row into this new buffer.
// From where we are in the old buffer, to the end of the row, copy the
// remaining attributes.
// - if the old buffer is smaller than the new buffer, then just copy
// what we have, as it was. We already copied all _text_ with colors,
// but it's possible for someone to just put some color into the
// buffer to the right of that without any text (as just spaces). The
// buffer looks weird to the user when we resize and it starts losing
// those colors, so we need to copy them over too... as long as there
// is space. The last attr in the row will be extended to the end of
// the row in the new buffer.
// - if the old buffer is WIDER, than we might have wrapped onto a new
// line. Use the cursor's position's Y so that we know where the new
// row is, and start writing at the cursor position. Again, the attr
// in the last column of the old row will be extended to the end of the
// row that the text was flowed onto.
// - if the text in the old buffer didn't actually fill the whole
// line in the new buffer, then we didn't wrap. That's fine. just
// copy attributes from the old row till the end of the new row, and
// move on.
const auto newRowY = newCursor.GetPosition().Y;
auto& newRow = newBuffer.GetRowByOffset(newRowY);
auto newAttrColumn = newCursor.GetPosition().X;
const auto newWidth = newBuffer.GetLineWidth(newRowY);
// Stop when we get to the end of the buffer width, or the new position
// for inserting an attr would be past the right of the new buffer.
for (short copyAttrCol = iOldCol;
copyAttrCol < cOldColsTotal && newAttrColumn < newWidth;
copyAttrCol++)
{
try
{
// TODO: MSFT: 19446208 - this should just use an iterator and the inserter...
const auto textAttr = *attrs;
if (!newRow.GetAttrRow().SetAttrToEnd(newAttrColumn, textAttr))
{
break;
}
}
CATCH_LOG(); // Not worth dying over.

++newAttrColumn;
++attrs;
}

// If we found the old row that the caller was interested in, set the
Expand Down Expand Up @@ -2298,7 +2351,7 @@ HRESULT TextBuffer::Reflow(TextBuffer& oldBuffer,
// only because we ran out of space.
if (iRight < cOldColsTotal && !row.WasWrapForced())
{
if (iRight == cOldCursorPos.X && iOldRow == cOldCursorPos.Y)
if (!fFoundCursorPos && (iRight == cOldCursorPos.X && iOldRow == cOldCursorPos.Y))
{
cNewCursorPos = newCursor.GetPosition();
fFoundCursorPos = true;
Expand Down Expand Up @@ -2349,6 +2402,34 @@ HRESULT TextBuffer::Reflow(TextBuffer& oldBuffer,
}
}
}

// Finish copying buffer attributes to remaining rows below the last
// printable character. This is to fix the `color 2f` scenario, where you
// change the buffer colors then resize and everything below the last
// printable char gets reset. See GH #12567
auto newRowY = newCursor.GetPosition().Y + 1;
const auto newHeight = newBuffer.GetSize().Height();
const auto oldHeight = oldBuffer.GetSize().Height();
for (;
iOldRow < oldHeight && newRowY < newHeight;
iOldRow++)
{
const ROW& row = oldBuffer.GetRowByOffset(iOldRow);

// Optimization: Since all these rows are below the last printable char,
// we can reasonably assume that they are filled with just spaces.
// That's convenient, we can just copy the attr row from the old buffer
// into the new one, and resize the row to match. We'll rely on the
// behavior of ATTR_ROW::Resize to trim down when narrower, or extend
// the last attr when wider.
auto& newRow = newBuffer.GetRowByOffset(newRowY);
const auto newWidth = newBuffer.GetLineWidth(newRowY);
newRow.GetAttrRow() = row.GetAttrRow();
newRow.GetAttrRow().Resize(newWidth);

newRowY++;
}

if (SUCCEEDED(hr))
{
// Finish copying remaining parameters from the old text buffer to the new one
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2962,7 +2962,7 @@ void ConptyRoundtripTests::ResizeInitializeBufferWithDefaultAttrs()
else if (leaveTrailingChar && row == 3)
{
auto iter = TestUtils::VerifyLineContains(tb, { 0, row }, L'#', greenAttrs, 1u);
TestUtils::VerifyLineContains(iter, L' ', (afterResize ? greenAttrs : actualDefaultAttrs), static_cast<size_t>(width - 1));
TestUtils::VerifyLineContains(iter, L' ', (actualDefaultAttrs), static_cast<size_t>(width - 1));
}
else
{
Expand Down
2 changes: 1 addition & 1 deletion src/host/screenInfo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1472,7 +1472,7 @@ bool SCREEN_INFORMATION::IsMaximizedY() const
coordCursorHeightDiff.Y = sCursorHeightInViewportAfter - sCursorHeightInViewportBefore;
LOG_IF_FAILED(SetViewportOrigin(false, coordCursorHeightDiff, true));

_textBuffer->SetCurrentAttributes(oldPrimaryAttributes);
newTextBuffer->SetCurrentAttributes(oldPrimaryAttributes);

_textBuffer.swap(newTextBuffer);
}
Expand Down
Loading

0 comments on commit ef2e0ad

Please sign in to comment.