maxmustermann/terminal
1
0
Fork 0
Code Issues Pull requests Projects Releases Wiki Activity

Initialize the text buffer with the default attributes on a resize (#5792)

Browse source 
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
This commit is contained in:
Mike Griese 2021-04-21 16:34:28 -05:00 committed by GitHub
parent 546322b5c1
commit 913cf4b1a8
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
3 changed files with 169 additions and 3 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

11
src/cascadia/TerminalCore/Terminal.cpp
View file

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

151
src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
View file

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

10
src/host/screenInfo.cpp
View file

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