Manually copy trailing attributes on a resize (#12637)

## 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
This commit is contained in:
Mike Griese
2022-03-21 12:02:36 -05:00
committed by GitHub
parent 8a34a0e59a
commit 855e1360c0
5 changed files with 334 additions and 8 deletions

View File

@@ -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

View File

@@ -2251,7 +2251,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);
@@ -2295,7 +2296,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)
{
@@ -2306,9 +2311,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))
{
@@ -2317,6 +2322,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
@@ -2352,7 +2405,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;
@@ -2403,6 +2456,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

View File

@@ -2964,7 +2964,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
{

View File

@@ -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);
}

View File

@@ -17,6 +17,8 @@
#include "../../inc/conattrs.hpp"
#include "../../types/inc/Viewport.hpp"
#include "../../cascadia/UnitTests_TerminalCore/TestUtils.h"
#include <sstream>
using namespace WEX::Common;
@@ -26,6 +28,7 @@ using namespace Microsoft::Console::Types;
using namespace Microsoft::Console::Interactivity;
using namespace Microsoft::Console::Render;
using namespace Microsoft::Console::VirtualTerminal;
using namespace TerminalCoreUnitTests;
class ScreenBufferTests
{
@@ -224,6 +227,10 @@ class ScreenBufferTests
TEST_METHOD(RetainHorizontalOffsetWhenMovingToBottom);
TEST_METHOD(TestWriteConsoleVTQuirkMode);
TEST_METHOD(TestReflowEndOfLineColor);
TEST_METHOD(TestReflowSmallerLongLineWithColor);
TEST_METHOD(TestReflowBiggerLongLineWithColor);
};
void ScreenBufferTests::SingleAlternateBufferCreationTest()
@@ -6269,3 +6276,239 @@ void ScreenBufferTests::TestWriteConsoleVTQuirkMode()
verifyLastAttribute(vtWhiteOnBlack256Attribute);
}
}
void ScreenBufferTests::TestReflowEndOfLineColor()
{
BEGIN_TEST_METHOD_PROPERTIES()
TEST_METHOD_PROPERTY(L"Data:dx", L"{-1, 0, 1}")
TEST_METHOD_PROPERTY(L"Data:dy", L"{-1, 0, 1}")
END_TEST_METHOD_PROPERTIES();
INIT_TEST_PROPERTY(int, dx, L"The change in width of the buffer");
INIT_TEST_PROPERTY(int, dy, L"The change in height of the buffer");
auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
auto& si = gci.GetActiveOutputBuffer();
auto& stateMachine = si.GetStateMachine();
gci.SetWrapText(true);
auto defaultAttrs = si.GetAttributes();
auto red = defaultAttrs;
red.SetIndexedBackground(TextColor::DARK_RED);
auto green = defaultAttrs;
green.SetIndexedBackground(TextColor::DARK_GREEN);
auto blue = defaultAttrs;
blue.SetIndexedBackground(TextColor::DARK_BLUE);
auto yellow = defaultAttrs;
yellow.SetIndexedBackground(TextColor::DARK_YELLOW);
Log::Comment(L"Make sure the viewport is at 0,0");
VERIFY_SUCCEEDED(si.SetViewportOrigin(true, COORD({ 0, 0 }), true));
Log::Comment(L"Fill buffer with some data");
stateMachine.ProcessString(L"\x1b[H");
stateMachine.ProcessString(L"\x1b[41m"); // Red BG
stateMachine.ProcessString(L"AAAAA"); // AAAAA
stateMachine.ProcessString(L"\x1b[42m"); // Green BG
stateMachine.ProcessString(L"\nBBBBB\n"); // BBBBB
stateMachine.ProcessString(L"\x1b[44m"); // Blue BG
stateMachine.ProcessString(L" CCC \n"); // " abc " (with spaces on either side)
stateMachine.ProcessString(L"\x1b[43m"); // yellow BG
stateMachine.ProcessString(L"\x1b[K"); // Erase line
stateMachine.ProcessString(L"\x1b[2;6H"); // move the cursor to the end of the BBBBB's
auto verifyBuffer = [&](const TextBuffer& tb, const til::rect& /*viewport*/, const bool /*before*/) {
const short width = tb.GetSize().Width();
Log::Comment(NoThrowString().Format(L"Buffer width: %d", width));
auto iter0 = TestUtils::VerifyLineContains(tb, { 0, 0 }, L'A', red, 5u);
TestUtils::VerifyLineContains(iter0, L' ', defaultAttrs, static_cast<size_t>(width - 5));
auto iter1 = tb.GetCellLineDataAt({ 0, 1 });
TestUtils::VerifyLineContains(iter1, L'B', green, 5u);
TestUtils::VerifyLineContains(iter1, L' ', defaultAttrs, static_cast<size_t>(width - 5));
auto iter2 = tb.GetCellLineDataAt({ 0, 2 });
TestUtils::VerifyLineContains(iter2, L' ', blue, 1u);
TestUtils::VerifyLineContains(iter2, L'C', blue, 3u);
TestUtils::VerifyLineContains(iter2, L' ', blue, 1u);
TestUtils::VerifyLineContains(iter2, L' ', defaultAttrs, static_cast<size_t>(width - 5));
auto iter3 = tb.GetCellLineDataAt({ 0, 3 });
TestUtils::VerifyLineContains(iter3, L' ', yellow, static_cast<size_t>(width));
};
Log::Comment(L"========== Checking the buffer state (before) ==========");
verifyBuffer(si.GetTextBuffer(), til::rect{ si.GetViewport().ToInclusive() }, true);
Log::Comment(L"========== resize buffer ==========");
const til::point delta{ dx, dy };
const til::point oldSize{ si.GetBufferSize().Dimensions() };
const til::point newSize{ oldSize + delta };
VERIFY_SUCCEEDED(si.ResizeWithReflow(newSize.to_win32_coord()));
Log::Comment(L"========== Checking the buffer state (after) ==========");
verifyBuffer(si.GetTextBuffer(), til::rect{ si.GetViewport().ToInclusive() }, false);
}
void ScreenBufferTests::TestReflowSmallerLongLineWithColor()
{
Log::Comment(L"Reflow the buffer such that a long, line of text flows onto "
L"the next line. Check that the trailing attributes were copied"
L" to the new row.");
auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
auto& si = gci.GetActiveOutputBuffer();
auto& stateMachine = si.GetStateMachine();
gci.SetWrapText(true);
auto defaultAttrs = si.GetAttributes();
auto red = defaultAttrs;
red.SetIndexedBackground(TextColor::DARK_RED);
auto green = defaultAttrs;
green.SetIndexedBackground(TextColor::DARK_GREEN);
auto blue = defaultAttrs;
blue.SetIndexedBackground(TextColor::DARK_BLUE);
auto yellow = defaultAttrs;
yellow.SetIndexedBackground(TextColor::DARK_YELLOW);
Log::Comment(L"Make sure the viewport is at 0,0");
VERIFY_SUCCEEDED(si.SetViewportOrigin(true, COORD({ 0, 0 }), true));
Log::Comment(L"Fill buffer with some data");
stateMachine.ProcessString(L"\x1b[H");
stateMachine.ProcessString(L"\x1b[41m"); // Red BG
stateMachine.ProcessString(L"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"); // 35 A's
stateMachine.ProcessString(L"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"); // 35 more, 70 total
stateMachine.ProcessString(L"\x1b[42m"); // Green BG
stateMachine.ProcessString(L" BBB \n"); // " BBB " with spaces on either side).
// Attributes fill 70 red, 5 green, the last 5 are {whatever it was before}
auto verifyBuffer = [&](const TextBuffer& tb, const til::rect& /*viewport*/, const bool before) {
const short width = tb.GetSize().Width();
Log::Comment(NoThrowString().Format(L"Buffer width: %d", width));
if (before)
{
auto iter0 = TestUtils::VerifyLineContains(tb, { 0, 0 }, L'A', red, 70u);
TestUtils::VerifyLineContains(iter0, L' ', green, 1u);
TestUtils::VerifyLineContains(iter0, L'B', green, 3u);
TestUtils::VerifyLineContains(iter0, L' ', green, 1u);
TestUtils::VerifyLineContains(iter0, L' ', defaultAttrs, 5u);
}
else
{
auto iter0 = TestUtils::VerifyLineContains(tb, { 0, 0 }, L'A', red, 65u);
auto iter1 = tb.GetCellLineDataAt({ 0, 1 });
TestUtils::VerifyLineContains(iter1, L'A', red, 5u);
TestUtils::VerifyLineContains(iter1, L' ', green, 1u);
TestUtils::VerifyLineContains(iter1, L'B', green, 3u);
TestUtils::VerifyLineContains(iter1, L' ', green, 1u);
// We don't want to necessarily verify the contents of the rest of the
// line, but we will anyways. Right now, we expect the last attrs on the
// original row to fill the remainder of the row it flowed onto. We may
// want to change that in the future. If we do, this check can always be
// changed.
TestUtils::VerifyLineContains(iter1, L' ', defaultAttrs, static_cast<size_t>(width - 10));
}
};
Log::Comment(L"========== Checking the buffer state (before) ==========");
verifyBuffer(si.GetTextBuffer(), til::rect{ si.GetViewport().ToInclusive() }, true);
Log::Comment(L"========== resize buffer ==========");
const til::point delta{ -15, 0 };
const til::point oldSize{ si.GetBufferSize().Dimensions() };
const til::point newSize{ oldSize + delta };
VERIFY_SUCCEEDED(si.ResizeWithReflow(newSize.to_win32_coord()));
// Buffer is now 65 wide. 65 A's that wrapped onto the next row, where there
// are also 3 B's wrapped in spaces.
Log::Comment(L"========== Checking the buffer state (after) ==========");
verifyBuffer(si.GetTextBuffer(), til::rect{ si.GetViewport().ToInclusive() }, false);
}
void ScreenBufferTests::TestReflowBiggerLongLineWithColor()
{
Log::Comment(L"Reflow the buffer such that a wrapped line of text 'de-flows' onto the previous line.");
auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
auto& si = gci.GetActiveOutputBuffer();
auto& stateMachine = si.GetStateMachine();
gci.SetWrapText(true);
auto defaultAttrs = si.GetAttributes();
auto red = defaultAttrs;
red.SetIndexedBackground(TextColor::DARK_RED);
auto green = defaultAttrs;
green.SetIndexedBackground(TextColor::DARK_GREEN);
auto blue = defaultAttrs;
blue.SetIndexedBackground(TextColor::DARK_BLUE);
auto yellow = defaultAttrs;
yellow.SetIndexedBackground(TextColor::DARK_YELLOW);
Log::Comment(L"Make sure the viewport is at 0,0");
VERIFY_SUCCEEDED(si.SetViewportOrigin(true, COORD({ 0, 0 }), true));
Log::Comment(L"Fill buffer with some data");
stateMachine.ProcessString(L"\x1b[H");
stateMachine.ProcessString(L"\x1b[41m"); // Red BG
stateMachine.ProcessString(L"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"); // 40 A's
stateMachine.ProcessString(L"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"); // 40 more, to wrap
stateMachine.ProcessString(L"AAAAA"); // 5 more. 85 total.
stateMachine.ProcessString(L"\x1b[42m"); // Green BG
stateMachine.ProcessString(L" BBB \n"); // " BBB " with spaces on either side).
// Attributes fill 85 red, 5 green, the rest are {whatever it was before}
auto verifyBuffer = [&](const TextBuffer& tb, const til::rect& /*viewport*/, const bool before) {
const short width = tb.GetSize().Width();
Log::Comment(NoThrowString().Format(L"Buffer width: %d", width));
if (before)
{
auto iter0 = TestUtils::VerifyLineContains(tb, { 0, 0 }, L'A', red, 80u);
auto iter1 = tb.GetCellLineDataAt({ 0, 1 });
TestUtils::VerifyLineContains(iter1, L'A', red, 5u);
TestUtils::VerifyLineContains(iter1, L' ', green, 1u);
TestUtils::VerifyLineContains(iter1, L'B', green, 3u);
TestUtils::VerifyLineContains(iter1, L' ', green, 1u);
TestUtils::VerifyLineContains(iter1, L' ', defaultAttrs, static_cast<size_t>(width - 10));
}
else
{
auto iter0 = TestUtils::VerifyLineContains(tb, { 0, 0 }, L'A', red, 85u);
TestUtils::VerifyLineContains(iter0, L' ', green, 1u);
TestUtils::VerifyLineContains(iter0, L'B', green, 3u);
TestUtils::VerifyLineContains(iter0, L' ', green, 1u);
// We don't want to necessarily verify the contents of the rest of
// the line, but we will anyways. Differently than
// TestReflowSmallerLongLineWithColor: In this test, the since we're
// de-flowing row 1 onto row 0, and the trailing spaces in row 1 are
// default attrs, then that's the attrs we'll use to finish filling
// up the 0th row in the new buffer. Again, we may want to change
// that in the future. If we do, this check can always be changed.
TestUtils::VerifyLineContains(iter0, L' ', defaultAttrs, static_cast<size_t>(width - 90));
}
};
Log::Comment(L"========== Checking the buffer state (before) ==========");
verifyBuffer(si.GetTextBuffer(), til::rect{ si.GetViewport().ToInclusive() }, true);
Log::Comment(L"========== resize buffer ==========");
const til::point delta{ 15, 0 };
const til::point oldSize{ si.GetBufferSize().Dimensions() };
const til::point newSize{ oldSize + delta };
VERIFY_SUCCEEDED(si.ResizeWithReflow(newSize.to_win32_coord()));
// Buffer is now 95 wide. 85 A's that de-flowed onto the first row, where
// there are also 3 B's wrapped in spaces, and finally 5 trailing spaces.
Log::Comment(L"========== Checking the buffer state (after) ==========");
verifyBuffer(si.GetTextBuffer(), til::rect{ si.GetViewport().ToInclusive() }, false);
}