Skip to content

Commit

Permalink
Initialize the text buffer with the default attributes on a resize (#…
Browse files Browse the repository at this point in the history
…5792)

When we resize the text buffer, initialize the buffer with the
_default_¹ attributes, not the _current_ ones. If we use the current
attributes, then we can get into scenarios where something like `vim` is
running, and left the attributes set to something other than the
defaults, and when we resized the buffer, we'd fill it up with color, as
opposed to whatever the default would be.

This PR instead initializes the buffers with the default colors. It also
makes sure to set the active attributes of the newly created buffers
back to whatever the current attributes of the old buffer were.

[1]: For the Terminal, the default attributes are "default on default".
For conhost, the default attributes are whatever the result of
`Settings::GetDefaultAttributes` is, which could be any combo of the
legacy indices and the default color.

## PR Checklist
* [x] Closes #3848
* [x] I work here
* [x] Tests added/passed
* [n/a] Requires documentation to be updated

## Validation Steps Performed
* ran tests
  • Loading branch information
zadjii-msft authored Apr 21, 2021
1 parent 546322b commit 913cf4b
Show file tree
Hide file tree
Showing 3 changed files with 169 additions and 3 deletions.
11 changes: 10 additions & 1 deletion src/cascadia/TerminalCore/Terminal.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -245,8 +245,14 @@ void Terminal::UpdateAppearance(const ICoreAppearance& appearance)
std::unique_ptr<TextBuffer> newTextBuffer;
try
{
// GH#3848 - Stash away the current attributes the old text buffer is
// using. We'll initialize the new buffer with the default attributes,
// but after the resize, we'll want to make sure that the new buffer's
// current attributes (the ones used for printing new text) match the
// old buffer's.
const auto oldBufferAttributes = _buffer->GetCurrentAttributes();
newTextBuffer = std::make_unique<TextBuffer>(bufferSize,
_buffer->GetCurrentAttributes(),
TextAttribute{},
0, // temporarily set size to 0 so it won't render.
_buffer->GetRenderTarget());

Expand Down Expand Up @@ -274,6 +280,9 @@ void Terminal::UpdateAppearance(const ICoreAppearance& appearance)

newViewportTop = oldRows.mutableViewportTop;
newVisibleTop = oldRows.visibleViewportTop;

// Restore the active text attributes
newTextBuffer->SetCurrentAttributes(oldBufferAttributes);
}
CATCH_RETURN();

Expand Down
151 changes: 150 additions & 1 deletion src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,8 @@ class TerminalCoreUnitTests::ConptyRoundtripTests final

TEST_METHOD(HyperlinkIdConsistency);

TEST_METHOD(ResizeInitializeBufferWithDefaultAttrs);

private:
bool _writeCallback(const char* const pch, size_t const cch);
void _flushFirstFrame();
Expand Down Expand Up @@ -2816,7 +2818,7 @@ void ConptyRoundtripTests::TestResizeWithCookedRead()
// Don't let the cooked read pollute other tests
BEGIN_TEST_METHOD_PROPERTIES()
TEST_METHOD_PROPERTY(L"IsolationLevel", L"Method")
TEST_METHOD_PROPERTY(L"Data:dx", L"{-10, -1, 0, 1, -10}")
TEST_METHOD_PROPERTY(L"Data:dx", L"{-10, -1, 0, 1, 10}")
TEST_METHOD_PROPERTY(L"Data:dy", L"{-10, -1, 0, 1, 10}")
END_TEST_METHOD_PROPERTIES()

Expand Down Expand Up @@ -2855,6 +2857,153 @@ void ConptyRoundtripTests::TestResizeWithCookedRead()
// By simply reaching the end of this test, we know that we didn't crash. Hooray!
}

void ConptyRoundtripTests::ResizeInitializeBufferWithDefaultAttrs()
{
// See https://github.com/microsoft/terminal/issues/3848
Log::Comment(L"This test checks that the attributes in the text buffer are "
L"initialized to a sensible value during a resize. The entire "
L"buffer shouldn't be filled with _whatever the current "
L"attributes are_, it should be filled with the default "
L"attributes (however the application defines that). Then, "
L"after the resize, we should still be able to print to the "
L"buffer with the old \"current attributes\"");

BEGIN_TEST_METHOD_PROPERTIES()
TEST_METHOD_PROPERTY(L"IsolationLevel", L"Method")
TEST_METHOD_PROPERTY(L"Data:dx", L"{-1, 0, 1}")
TEST_METHOD_PROPERTY(L"Data:dy", L"{-1, 0, 1}")
TEST_METHOD_PROPERTY(L"Data:leaveTrailingChar", L"{false, true}")
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");
INIT_TEST_PROPERTY(bool, leaveTrailingChar, L"If true, we'll print one additional '#' on row 3");

// Do nothing if the resize would just be a no-op.
if (dx == 0 && dy == 0)
{
return;
}

auto& g = ServiceLocator::LocateGlobals();
auto& renderer = *g.pRender;
auto& gci = g.getConsoleInformation();
auto& si = gci.GetActiveOutputBuffer();
auto& sm = si.GetStateMachine();
auto* hostTb = &si.GetTextBuffer();
auto* termTb = term->_buffer.get();

_flushFirstFrame();

_checkConptyOutput = false;
_logConpty = true;

auto defaultAttrs = si.GetAttributes();
auto conhostGreenAttrs = TextAttribute();

// Conhost and Terminal store attributes in different bits.
// conhostGreenAttrs.SetIndexedAttributes(std::nullopt,
// { static_cast<BYTE>(FOREGROUND_GREEN) });
conhostGreenAttrs.SetIndexedBackground(FOREGROUND_GREEN);
auto terminalGreenAttrs = TextAttribute();
// terminalGreenAttrs.SetIndexedAttributes(std::nullopt,
// { static_cast<BYTE>(XTERM_GREEN_ATTR) });
terminalGreenAttrs.SetIndexedBackground(XTERM_GREEN_ATTR);

const size_t width = static_cast<size_t>(TerminalViewWidth);

// Use an initial ^[[m to start printing with default-on-default
sm.ProcessString(L"\x1b[m");

// Print three lines with "# #", where the first "# " are in
// default-on-green.
for (int i = 0; i < 3; i++)
{
sm.ProcessString(L"\x1b[42m");
sm.ProcessString(L"# ");
sm.ProcessString(L"\x1b[m");
sm.ProcessString(L"#");
sm.ProcessString(L"\r\n");
}

// Now, leave the active attributes as default-on-green. When we resize the
// buffers, we don't want them initialized with default-on-green, we want
// them to use whatever the set default attributes are.
sm.ProcessString(L"\x1b[42m");

// If leaveTrailingChar is true, we'll leave one default-on-green '#' on row
// 3. This will force conpty to change the Terminal's colors to
// default-on-green, so we can check that not only conhost initialize the
// buffer colors correctly, but so does the Terminal.
if (leaveTrailingChar)
{
sm.ProcessString(L"#");
}

auto verifyBuffer = [&](const TextBuffer& tb, const til::rectangle viewport, const bool isTerminal, const bool afterResize) {
const auto width = viewport.width<short>();

// Conhost and Terminal store attributes in different bits.
const auto greenAttrs = isTerminal ? terminalGreenAttrs : conhostGreenAttrs;

for (short row = 0; row < tb.GetSize().Height(); row++)
{
Log::Comment(NoThrowString().Format(L"Checking row %d...", row));

VERIFY_IS_FALSE(tb.GetRowByOffset(row).WasWrapForced());

const bool hasChar = row < 3;
const auto actualDefaultAttrs = isTerminal ? TextAttribute() : defaultAttrs;

if (hasChar)
{
auto iter = TestUtils::VerifyLineContains(tb, { 0, row }, L'#', greenAttrs, 1u);
TestUtils::VerifyLineContains(iter, L' ', greenAttrs, 1u);
TestUtils::VerifyLineContains(iter, L'#', TextAttribute(), 1u);
// After the resize, the default attrs of the last char will
// extend to fill the rest of the row. This is GH#32. If that
// bug ever gets fixed, this test will break, but that's
// ABSOLUTELY OKAY.
TestUtils::VerifyLineContains(iter, L' ', (afterResize ? TextAttribute() : actualDefaultAttrs), static_cast<size_t>(width - 3));
}
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));
}
else
{
TestUtils::VerifyLineContains(tb, { 0, row }, L' ', actualDefaultAttrs, viewport.width<size_t>());
}
}
};

Log::Comment(L"========== Checking the host buffer state (before) ==========");
verifyBuffer(*hostTb, si.GetViewport().ToInclusive(), false, false);

Log::Comment(L"Painting the frame");
VERIFY_SUCCEEDED(renderer.PaintFrame());

Log::Comment(L"========== Checking the terminal buffer state (before) ==========");
verifyBuffer(*termTb, term->_mutableViewport.ToInclusive(), true, false);

// After we resize, make sure to get the new textBuffers
std::tie(hostTb, termTb) = _performResize({ TerminalViewWidth + dx,
TerminalViewHeight + dy });

Log::Comment(L"Painting the frame");
VERIFY_SUCCEEDED(renderer.PaintFrame());

Log::Comment(L"========== Checking the host buffer state (after) ==========");
verifyBuffer(*hostTb, si.GetViewport().ToInclusive(), false, true);

Log::Comment(L"Painting the frame");
VERIFY_SUCCEEDED(renderer.PaintFrame());

Log::Comment(L"========== Checking the terminal buffer state (after) ==========");
verifyBuffer(*termTb, term->_mutableViewport.ToInclusive(), true, true);
}

void ConptyRoundtripTests::NewLinesAtBottomWithBackground()
{
BEGIN_TEST_METHOD_PROPERTIES()
Expand Down
10 changes: 9 additions & 1 deletion src/host/screenInfo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1412,10 +1412,16 @@ bool SCREEN_INFORMATION::IsMaximizedY() const

// First allocate a new text buffer to take the place of the current one.
std::unique_ptr<TextBuffer> newTextBuffer;

// GH#3848 - Stash away the current attributes the old text buffer is using.
// We'll initialize the new buffer with the default attributes, but after
// the resize, we'll want to make sure that the new buffer's current
// attributes (the ones used for printing new text) match the old buffer's.
const auto oldPrimaryAttributes = _textBuffer->GetCurrentAttributes();
try
{
newTextBuffer = std::make_unique<TextBuffer>(coordNewScreenSize,
GetAttributes(),
TextAttribute{},
0,
_renderTarget); // temporarily set size to 0 so it won't render.
}
Expand Down Expand Up @@ -1444,6 +1450,8 @@ bool SCREEN_INFORMATION::IsMaximizedY() const
coordCursorHeightDiff.Y = sCursorHeightInViewportAfter - sCursorHeightInViewportBefore;
LOG_IF_FAILED(SetViewportOrigin(false, coordCursorHeightDiff, true));

_textBuffer->SetCurrentAttributes(oldPrimaryAttributes);

_textBuffer.swap(newTextBuffer);
}

Expand Down

0 comments on commit 913cf4b

Please sign in to comment.