Skip to content

Commit

Permalink
Make sure that EraseAll moves the Terminal viewport (microsoft#5683)
Browse files Browse the repository at this point in the history
The Erase All VT sequence (`^[[2J`) is supposed to erase the entire
contents of the viewport. The way it usually does this is by shifting
the entirety of the viewport contents into scrollback, and starting the
new viewport below it. 

Currently, conpty doesn't propagate that state change correctly. When
conpty gets a 2J, it simply erases the content of the connected
terminal's viewport, by writing over it with spaces. Conpty didn't
really have a good way of communicating "your viewport should move", it
only knew "the buffer is now full of spaces".

This would lead to bugs like microsoft#2832, where pressing <kbd>ctrl+L</kbd> in
`bash` would delete the current contents of the viewport, instead of
moving the viewport down.

This PR makes sure that when conpty sees a 2J, it passes that through
directly to the connected terminal application as well. Fortunately, 2J
was already implemented in the Windows Terminal, so this actually fixes
the behavior of <kbd>ctrl+L</kbd>/`clear` in WSL in the Terminal.

## References

* microsoft#4252 - right now this isn't the _most_ optimal scenario, we're
  literally just printing a 2J, then we'll perform "erase line" `height`
  times. The erase line operations are all redundant at this point - the
  entire viewport is blank, but conpty doesn't really know that.
  Fortunately, microsoft#4252 was already filed for me to come through and
  optimize this path.

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

## Validation Steps Performed
* ran tests
* compared <kbd>ctrl+L</kbd> with its behavior in conhost
* compared `clear` with its behavior in conhost
  • Loading branch information
zadjii-msft authored May 5, 2020
1 parent 9166a14 commit 9fe624f
Show file tree
Hide file tree
Showing 5 changed files with 139 additions and 7 deletions.
126 changes: 125 additions & 1 deletion src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,15 @@ class TerminalCoreUnitTests::ConptyRoundtripTests final
_checkConptyOutput = true;
_logConpty = false;

VERIFY_ARE_EQUAL(gci.GetActiveOutputBuffer().GetViewport().Dimensions(),
gci.GetActiveOutputBuffer().GetBufferSize().Dimensions(),
L"If this test fails, then there's a good chance "
L"another test resized the buffer but didn't use IsolationLevel:Method");
VERIFY_ARE_EQUAL(gci.GetActiveOutputBuffer().GetViewport(),
gci.GetActiveOutputBuffer().GetBufferSize(),
L"If this test fails, then there's a good chance "
L"another test resized the buffer but didn't use IsolationLevel:Method");

return true;
}

Expand All @@ -165,6 +174,8 @@ class TerminalCoreUnitTests::ConptyRoundtripTests final

TEST_METHOD(PassthroughClearScrollback);

TEST_METHOD(PassthroughClearAll);

TEST_METHOD(PassthroughHardReset);

TEST_METHOD(PassthroughCursorShapeImmediately);
Expand Down Expand Up @@ -283,6 +294,10 @@ void ConptyRoundtripTests::_resizeConpty(const unsigned short sx,

[[nodiscard]] std::tuple<TextBuffer*, TextBuffer*> ConptyRoundtripTests::_performResize(const til::size& newSize)
{
// IMPORTANT! Anyone calling this should make sure that the test is running
// in IsolationLevel: Method. If you don't add that, then it might secretly
// pollute other tests!

Log::Comment(L"========== Resize the Terminal and conpty ==========");

auto resizeResult = term->UserResize(newSize);
Expand Down Expand Up @@ -1021,6 +1036,96 @@ void ConptyRoundtripTests::PassthroughClearScrollback()
}
}

void ConptyRoundtripTests::PassthroughClearAll()
{
// see https://github.com/microsoft/terminal/issues/2832
Log::Comment(L"This is a test to make sure that when the client emits a "
L"^[[2J, we actually forward the 2J to the terminal, to move "
L"the viewport. 2J importantly moves the viewport, so that "
L"all the _current_ buffer contents are moved to scrollback. "
L"We shouldn't just paint over the current viewport with spaces.");

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

auto& sm = si.GetStateMachine();

_flushFirstFrame();

_checkConptyOutput = false;
_logConpty = true;

const auto hostView = si.GetViewport();
const auto end = 2 * hostView.Height();
for (auto i = 0; i < end; i++)
{
if (i > 0)
{
sm.ProcessString(L"\r\n");
}

sm.ProcessString(L"~");
}

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

// "~" rows
for (short row = 0; row < viewport.bottom<short>(); row++)
{
Log::Comment(NoThrowString().Format(L"Checking row %d", row));
VERIFY_IS_FALSE(tb.GetRowByOffset(row).GetCharRow().WasWrapForced());
auto iter = tb.GetCellDataAt({ 0, row });
if (afterClear && row >= viewport.top<short>())
{
TestUtils::VerifySpanOfText(L" ", iter, 0, width);
}
else
{
TestUtils::VerifySpanOfText(L"~", iter, 0, 1);
TestUtils::VerifySpanOfText(L" ", iter, 0, width - 1);
}
}
};

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

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

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

const til::rectangle originalTerminalView{ term->_mutableViewport.ToInclusive() };

// Here, we'll emit the 2J to EraseAll, and move the viewport contents into
// the scrollback.
sm.ProcessString(L"\x1b[2J");

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

// Make sure that the terminal's new viewport is actually just lower than it
// used to be.
const til::rectangle newTerminalView{ term->_mutableViewport.ToInclusive() };
VERIFY_ARE_EQUAL(end, newTerminalView.top<short>());
VERIFY_IS_GREATER_THAN(newTerminalView.top(), originalTerminalView.top());

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

Log::Comment(L"Painting the frame");
VERIFY_SUCCEEDED(renderer.PaintFrame());
Log::Comment(L"========== Checking the terminal buffer state (after) ==========");
verifyBuffer(*termTb, newTerminalView, true);
}

void ConptyRoundtripTests::PassthroughHardReset()
{
// This test is highly similar to PassthroughClearScrollback.
Expand Down Expand Up @@ -2416,6 +2521,10 @@ void ConptyRoundtripTests::TestCursorInDeferredEOLPositionOnNewLineWithSpaces()

void ConptyRoundtripTests::ResizeRepaintVimExeBuffer()
{
BEGIN_TEST_METHOD_PROPERTIES()
TEST_METHOD_PROPERTY(L"IsolationLevel", L"Method")
END_TEST_METHOD_PROPERTIES()

// See https://github.com/microsoft/terminal/issues/5428
Log::Comment(L"This test emulates what happens when you decrease the width "
L"of the window while running vim.exe.");
Expand Down Expand Up @@ -2543,10 +2652,11 @@ void ConptyRoundtripTests::ClsAndClearHostClearsScrollbackTest()
// See https://github.com/microsoft/terminal/issues/3126#issuecomment-620677742

BEGIN_TEST_METHOD_PROPERTIES()
TEST_METHOD_PROPERTY(L"Data:clearBufferMethod", L"{0, 1}")
TEST_METHOD_PROPERTY(L"Data:clearBufferMethod", L"{0, 1, 2}")
END_TEST_METHOD_PROPERTIES();
constexpr int ClearLikeCls = 0;
constexpr int ClearLikeClearHost = 1;
constexpr int ClearWithVT = 2;
INIT_TEST_PROPERTY(int, clearBufferMethod, L"Controls whether we clear the buffer like cmd or like powershell");

Log::Comment(L"This test checks the shims for cmd.exe and powershell.exe. "
Expand Down Expand Up @@ -2611,6 +2721,9 @@ void ConptyRoundtripTests::ClsAndClearHostClearsScrollbackTest()
Log::Comment(L"========== Checking the terminal buffer state (before) ==========");
verifyBuffer(*termTb, term->_mutableViewport.ToInclusive());

VERIFY_ARE_EQUAL(si.GetViewport().Dimensions(), si.GetBufferSize().Dimensions());
VERIFY_ARE_EQUAL(si.GetViewport(), si.GetBufferSize());

if (clearBufferMethod == ClearLikeCls)
{
// Execute the cls, EXACTLY LIKE CMD.
Expand Down Expand Up @@ -2654,6 +2767,17 @@ void ConptyRoundtripTests::ClsAndClearHostClearsScrollbackTest()
{ 0, 0 },
cellsWritten));
}
else if (clearBufferMethod == ClearWithVT)
{
sm.ProcessString(L"\x1b[2J");
VERIFY_ARE_EQUAL(si.GetViewport().Dimensions(), si.GetBufferSize().Dimensions());
VERIFY_ARE_EQUAL(si.GetViewport(), si.GetBufferSize());

sm.ProcessString(L"\x1b[3J");
}

VERIFY_ARE_EQUAL(si.GetViewport().Dimensions(), si.GetBufferSize().Dimensions());
VERIFY_ARE_EQUAL(si.GetViewport(), si.GetBufferSize());

Log::Comment(L"Painting the frame");
VERIFY_SUCCEEDED(renderer.PaintFrame());
Expand Down
6 changes: 3 additions & 3 deletions src/host/getset.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1640,10 +1640,10 @@ void DoSrvPrivateEnableAlternateScroll(const bool fEnable)
// Parameters:
// The ScreenBuffer to perform the erase on.
// Return value:
// - STATUS_SUCCESS if we succeeded, otherwise the NTSTATUS version of the failure.
[[nodiscard]] NTSTATUS DoSrvPrivateEraseAll(SCREEN_INFORMATION& screenInfo)
// - S_OK if we succeeded, otherwise the HRESULT of the failure.
[[nodiscard]] HRESULT DoSrvPrivateEraseAll(SCREEN_INFORMATION& screenInfo)
{
return NTSTATUS_FROM_HRESULT(screenInfo.GetActiveBuffer().VtEraseAll());
return screenInfo.GetActiveBuffer().VtEraseAll();
}

void DoSrvSetCursorStyle(SCREEN_INFORMATION& screenInfo,
Expand Down
2 changes: 1 addition & 1 deletion src/host/getset.h
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ void DoSrvPrivateBoldText(SCREEN_INFORMATION& screenInfo, const bool bolded);
ExtendedAttributes DoSrvPrivateGetExtendedTextAttributes(SCREEN_INFORMATION& screenInfo);
void DoSrvPrivateSetExtendedTextAttributes(SCREEN_INFORMATION& screenInfo, const ExtendedAttributes attrs);

[[nodiscard]] NTSTATUS DoSrvPrivateEraseAll(SCREEN_INFORMATION& screenInfo);
[[nodiscard]] HRESULT DoSrvPrivateEraseAll(SCREEN_INFORMATION& screenInfo);

void DoSrvSetCursorStyle(SCREEN_INFORMATION& screenInfo,
const CursorType cursorType);
Expand Down
2 changes: 1 addition & 1 deletion src/host/outputStream.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -585,7 +585,7 @@ bool ConhostInternalGetSet::PrivateEnableAlternateScroll(const bool enabled)
// - true if successful (see DoSrvPrivateEraseAll). false otherwise.
bool ConhostInternalGetSet::PrivateEraseAll()
{
return NT_SUCCESS(DoSrvPrivateEraseAll(_io.GetActiveOutputBuffer()));
return SUCCEEDED(DoSrvPrivateEraseAll(_io.GetActiveOutputBuffer()));
}

// Routine Description:
Expand Down
10 changes: 9 additions & 1 deletion src/terminal/adapter/adaptDispatch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -588,7 +588,15 @@ bool AdaptDispatch::EraseInDisplay(const DispatchTypes::EraseType eraseType)
}
else if (eraseType == DispatchTypes::EraseType::All)
{
return _EraseAll();
// GH#5683 - If this succeeded, but we're in a conpty, return `false` to
// make the state machine propagate this ED sequence to the connected
// terminal application. While we're in conpty mode, when the client
// requests a Erase All operation, we need to manually tell the
// connected terminal to do the same thing, so that the terminal will
// move it's own buffer contents into the scrollback.
const bool eraseAllResult = _EraseAll();
const bool isPty = _pConApi->IsConsolePty();
return eraseAllResult && (!isPty);
}

CONSOLE_SCREEN_BUFFER_INFOEX csbiex = { 0 };
Expand Down

0 comments on commit 9fe624f

Please sign in to comment.