Skip to content

Commit

Permalink
Make Conpty emit wrapped lines as actually wrapped lines (#4415)
Browse files Browse the repository at this point in the history 
## Summary of the Pull Request

Changes how conpty emits text to preserve line-wrap state, and additionally adds rudimentary support to the Windows Terminal for wrapped lines.

## References

* Does _not_ fix (!) #3088, but that might be lower down in conhost. This makes wt behave like conhost, so at least there's that
* Still needs a proper deferred EOL wrap implementation in #780, which is left as a todo
* #4200 is the mega bucket with all this work
* MSFT:16485846 was the first attempt at this task, which caused the regression MSFT:18123777 so we backed it out.
* #4403 - I made sure this worked with that PR before I even sent #4403

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

## Detailed Description of the Pull Request / Additional comments

I started with the following implementation:
When conpty is about to write the last column, note that we wrapped this line here. If the next character the vt renderer is told to paint get is supposed to be at the start of the following line, then we know that the previous line had wrapped, so we _won't_ emit the usual `\r\n` here, and we'll just continue emitting text.

However, this isn't _exactly_ right - if someone fills the row _exactly_ with text, the information that's available to the vt renderer isn't enough to know for sure if this line broke or not. It is possible for the client to write a full line of text, with a `\n` at the end, to manually break the line. So, I had to also add the `lineWrapped` param to the `IRenderEngine` interface, which is about half the files in this changelist.

## Validation Steps Performed
* Ran tests
* Checked how the Windows Terminal behaves with these changes
* Made sure that conhost/inception and gnome-terminal both act as you'd expect with wrapped lines from conpty
  • Loading branch information
@zadjii-msft
zadjii-msft authored Feb 27, 2020
1 parent 1de07aa commit e5182fb
Show file tree
Hide file tree
Showing 24 changed files with 512 additions and 35 deletions.
16 changes: 16 additions & 0 deletions src/cascadia/TerminalCore/Terminal.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -454,6 +454,22 @@ void Terminal::_WriteBuffer(const std::wstring_view& stringView)
// With well behaving shells during normal operation this safeguard should normally not be encountered.
proposedCursorPosition.X = 0;
proposedCursorPosition.Y++;

// Try the character again.
i--;

// Mark the line we're currently on as wrapped

// TODO: GH#780 - This should really be a _deferred_ newline. If
// the next character to come in is a newline or a cursor
// movement or anything, then we should _not_ wrap this line
// here.
//
// This is more WriteCharsLegacy2ElectricBoogaloo work. I'm
// leaving it like this for now - it'll break for lines that
// _exactly_ wrap, but we can't re-wrap lines now anyways, so it
// doesn't matter.
_buffer->GetRowByOffset(cursorPosBefore.Y).GetCharRow().SetWrapForced(true);
}

_AdjustCursorPosition(proposedCursorPosition);
Expand Down
265 changes: 263 additions & 2 deletions src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ using namespace Microsoft::Terminal::Core;

namespace TerminalCoreUnitTests
{
class TerminalBufferTests;
class ConptyRoundtripTests;
};
using namespace TerminalCoreUnitTests;

Expand Down Expand Up @@ -152,8 +152,14 @@ class TerminalCoreUnitTests::ConptyRoundtripTests final
TEST_METHOD(SimpleWriteOutputTest);
TEST_METHOD(WriteTwoLinesUsesNewline);
TEST_METHOD(WriteAFewSimpleLines);

TEST_METHOD(PassthroughClearScrollback);

TEST_METHOD(TestWrappingALongString);
TEST_METHOD(TestAdvancedWrapping);
TEST_METHOD(TestExactWrappingWithoutSpaces);
TEST_METHOD(TestExactWrappingWithSpaces);

TEST_METHOD(MoveCursorAtEOL);

private:
Expand Down Expand Up @@ -348,6 +354,262 @@ void ConptyRoundtripTests::WriteAFewSimpleLines()
verifyData(termTb);
}

void ConptyRoundtripTests::TestWrappingALongString()
{
auto& g = ServiceLocator::LocateGlobals();
auto& renderer = *g.pRender;
auto& gci = g.getConsoleInformation();
auto& si = gci.GetActiveOutputBuffer();
auto& hostSm = si.GetStateMachine();
auto& hostTb = si.GetTextBuffer();
auto& termTb = *term->_buffer;

_flushFirstFrame();
_checkConptyOutput = false;

const auto initialTermView = term->GetViewport();

const auto charsToWrite = gsl::narrow_cast<short>(TestUtils::Test100CharsString.size());
VERIFY_ARE_EQUAL(100, charsToWrite);

VERIFY_ARE_EQUAL(0, initialTermView.Top());
VERIFY_ARE_EQUAL(32, initialTermView.BottomExclusive());

hostSm.ProcessString(TestUtils::Test100CharsString);

const auto secondView = term->GetViewport();

VERIFY_ARE_EQUAL(0, secondView.Top());
VERIFY_ARE_EQUAL(32, secondView.BottomExclusive());

auto verifyBuffer = [&](const TextBuffer& tb) {
auto& cursor = tb.GetCursor();
// Verify the cursor wrapped to the second line
VERIFY_ARE_EQUAL(charsToWrite % initialTermView.Width(), cursor.GetPosition().X);
VERIFY_ARE_EQUAL(1, cursor.GetPosition().Y);

// Verify that we marked the 0th row as _wrapped_
const auto& row0 = tb.GetRowByOffset(0);
VERIFY_IS_TRUE(row0.GetCharRow().WasWrapForced());

const auto& row1 = tb.GetRowByOffset(1);
VERIFY_IS_FALSE(row1.GetCharRow().WasWrapForced());

TestUtils::VerifyExpectedString(tb, TestUtils::Test100CharsString, { 0, 0 });
};

verifyBuffer(hostTb);

VERIFY_SUCCEEDED(renderer.PaintFrame());

verifyBuffer(termTb);
}

void ConptyRoundtripTests::TestAdvancedWrapping()
{
auto& g = ServiceLocator::LocateGlobals();
auto& renderer = *g.pRender;
auto& gci = g.getConsoleInformation();
auto& si = gci.GetActiveOutputBuffer();
auto& hostSm = si.GetStateMachine();
auto& hostTb = si.GetTextBuffer();
auto& termTb = *term->_buffer;
const auto initialTermView = term->GetViewport();

_flushFirstFrame();

const auto charsToWrite = gsl::narrow_cast<short>(TestUtils::Test100CharsString.size());
VERIFY_ARE_EQUAL(100, charsToWrite);

hostSm.ProcessString(TestUtils::Test100CharsString);
hostSm.ProcessString(L"\n");
hostSm.ProcessString(L" ");
hostSm.ProcessString(L"1234567890");

auto verifyBuffer = [&](const TextBuffer& tb) {
auto& cursor = tb.GetCursor();
// Verify the cursor wrapped to the second line
VERIFY_ARE_EQUAL(2, cursor.GetPosition().Y);
VERIFY_ARE_EQUAL(20, cursor.GetPosition().X);

// Verify that we marked the 0th row as _wrapped_
const auto& row0 = tb.GetRowByOffset(0);
VERIFY_IS_TRUE(row0.GetCharRow().WasWrapForced());

const auto& row1 = tb.GetRowByOffset(1);
VERIFY_IS_FALSE(row1.GetCharRow().WasWrapForced());

TestUtils::VerifyExpectedString(tb, TestUtils::Test100CharsString, { 0, 0 });
TestUtils::VerifyExpectedString(tb, L" 1234567890", { 0, 2 });
};

verifyBuffer(hostTb);

// First write the first 80 characters from the string
expectedOutput.push_back(R"(!"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\]^_`abcdefghijklmnop)");
// Without line breaking, write the remaining 20 chars
expectedOutput.push_back(R"(qrstuvwxyz{|}~!"#$%&)");
// Clear the rest of row 1
expectedOutput.push_back("\x1b[K");
// This is the hard line break
expectedOutput.push_back("\r\n");
// Now write row 2 of the buffer
expectedOutput.push_back(" 1234567890");
// and clear everything after the text, because the buffer is empty.
expectedOutput.push_back("\x1b[K");
VERIFY_SUCCEEDED(renderer.PaintFrame());

verifyBuffer(termTb);
}

void ConptyRoundtripTests::TestExactWrappingWithoutSpaces()
{
// This test (and TestExactWrappingWitSpaces) reveals a bug in the old
// implementation.
//
// If a line _exactly_ wraps to the next line, we can't tell if the line
// should really wrap, or manually break. The client app is writing a line
// that's exactly the width of the buffer that manually linebreaked at the
// end of the line, followed by another line.
//
// With the old PaintBufferLine interface, there's no way to know if this
// case is because the line wrapped or not. Hence, the addition of the
// `lineWrapped` parameter

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

const auto initialTermView = term->GetViewport();

_flushFirstFrame();

const auto charsToWrite = initialTermView.Width();
VERIFY_ARE_EQUAL(80, charsToWrite);

for (auto i = 0; i < charsToWrite; i++)
{
// This is a handy way of just printing the printable characters that
// _aren't_ the space character.
const wchar_t wch = static_cast<wchar_t>(33 + (i % 94));
hostSm.ProcessCharacter(wch);
}

hostSm.ProcessString(L"\n");
hostSm.ProcessString(L"1234567890");

auto verifyBuffer = [&](const TextBuffer& tb, const bool isTerminal) {
auto& cursor = tb.GetCursor();
// Verify the cursor wrapped to the second line
VERIFY_ARE_EQUAL(1, cursor.GetPosition().Y);
VERIFY_ARE_EQUAL(10, cursor.GetPosition().X);

// TODO: GH#780 - In the Terminal, neither line should be wrapped.
// Unfortunately, until WriteCharsLegacy2ElectricBoogaloo is complete,
// the Terminal will still treat the first line as wrapped. When #780 is
// implemented, these tests will fail, and should again expect the first
// line to not be wrapped.

// Verify that we marked the 0th row as _not wrapped_
const auto& row0 = tb.GetRowByOffset(0);
VERIFY_ARE_EQUAL(isTerminal, row0.GetCharRow().WasWrapForced());

const auto& row1 = tb.GetRowByOffset(1);
VERIFY_IS_FALSE(row1.GetCharRow().WasWrapForced());

TestUtils::VerifyExpectedString(tb, LR"(!"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\]^_`abcdefghijklmnop)", { 0, 0 });
TestUtils::VerifyExpectedString(tb, L"1234567890", { 0, 1 });
};

verifyBuffer(hostTb, false);

// First write the first 80 characters from the string
expectedOutput.push_back(R"(!"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\]^_`abcdefghijklmnop)");

// This is the hard line break
expectedOutput.push_back("\r\n");
// Now write row 2 of the buffer
expectedOutput.push_back("1234567890");
// and clear everything after the text, because the buffer is empty.
expectedOutput.push_back("\x1b[K");
VERIFY_SUCCEEDED(renderer.PaintFrame());

verifyBuffer(termTb, true);
}

void ConptyRoundtripTests::TestExactWrappingWithSpaces()
{
// This test is also explained by the comment at the top of TestExactWrappingWithoutSpaces

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

_flushFirstFrame();

const auto charsToWrite = initialTermView.Width();
VERIFY_ARE_EQUAL(80, charsToWrite);

for (auto i = 0; i < charsToWrite; i++)
{
// This is a handy way of just printing the printable characters that
// _aren't_ the space character.
const wchar_t wch = static_cast<wchar_t>(33 + (i % 94));
hostSm.ProcessCharacter(wch);
}

hostSm.ProcessString(L"\n");
hostSm.ProcessString(L" ");
hostSm.ProcessString(L"1234567890");

auto verifyBuffer = [&](const TextBuffer& tb, const bool isTerminal) {
auto& cursor = tb.GetCursor();
// Verify the cursor wrapped to the second line
VERIFY_ARE_EQUAL(1, cursor.GetPosition().Y);
VERIFY_ARE_EQUAL(20, cursor.GetPosition().X);

// TODO: GH#780 - In the Terminal, neither line should be wrapped.
// Unfortunately, until WriteCharsLegacy2ElectricBoogaloo is complete,
// the Terminal will still treat the first line as wrapped. When #780 is
// implemented, these tests will fail, and should again expect the first
// line to not be wrapped.

// Verify that we marked the 0th row as _not wrapped_
const auto& row0 = tb.GetRowByOffset(0);
VERIFY_ARE_EQUAL(isTerminal, row0.GetCharRow().WasWrapForced());

const auto& row1 = tb.GetRowByOffset(1);
VERIFY_IS_FALSE(row1.GetCharRow().WasWrapForced());

TestUtils::VerifyExpectedString(tb, LR"(!"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\]^_`abcdefghijklmnop)", { 0, 0 });
TestUtils::VerifyExpectedString(tb, L" 1234567890", { 0, 1 });
};

verifyBuffer(hostTb, false);

// First write the first 80 characters from the string
expectedOutput.push_back(R"(!"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\]^_`abcdefghijklmnop)");

// This is the hard line break
expectedOutput.push_back("\r\n");
// Now write row 2 of the buffer
expectedOutput.push_back(" 1234567890");
// and clear everything after the text, because the buffer is empty.
expectedOutput.push_back("\x1b[K");
VERIFY_SUCCEEDED(renderer.PaintFrame());

verifyBuffer(termTb, true);
}

void ConptyRoundtripTests::MoveCursorAtEOL()
{
// This is a test for GH#1245
Expand All @@ -360,7 +622,6 @@ void ConptyRoundtripTests::MoveCursorAtEOL()
auto& hostSm = si.GetStateMachine();
auto& hostTb = si.GetTextBuffer();
auto& termTb = *term->_buffer;

_flushFirstFrame();

Log::Comment(NoThrowString().Format(
Expand Down
Loading

0 comments on commit e5182fb

Please sign in to comment.