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

Emit lines wrapped due to spaces at the end correctly (#5294)

Browse source 
## Summary of the Pull Request

When WSL vim prints the initial empty buffer (the one that's just a bunch of '\~'s), it prints this by doing the following:
* Print '\~' followed by enough spaces to clear the line
* Use CUP (`^[[H`) to move the cursor to the start of the next line
* repeat until the buffer is full

When we'd get the line of "\~     "... in conhost, we'd mark that line as wrapped. 

Logically, it doesn't really make any sense that when we follow that up by moving the cursor, the line is wrapped. However, this is just how conhost is right now. 
This wasn't ever a problem in just conhost before, because we really didn't care if lines in the alt buffer were "wrapped" or not. Plus, when vim would get resized, it would just reprint it's own buffer anyways. Nor was this a problem in conpty before this year (2020). We've only just recently added logic to conpty to try and preserve wrapped lines. 

Initially, I tried fixing this by breaking the line manually when the cursor was moved. This seemed to work great, except for the win32 vim.exe. Vim.exe doesn't emit a newline or a CUP to get to the next line. It just _goes for it_ and keeps printing. So there's _no way_ for us to know the line broke, because they're essentially just printing one long line, assuming we'll automatically move the cursor.

So instead, I'm making sure to emit the proper number of spaces at the end of a line when the line is wrapped. We won't do any funny business in that scenario and try to optimize for them, we'll _just print the spaces_.

## References

* #5181 - This change regressed this
* #4415 - Actually implemented wrapped lines in conpty

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

## Validation Steps Performed
* Wrote a unittest first and foremost
* Checked vtpipeterm to make sure vim still works
* checked Terminal to make sure vim still works
This commit is contained in:
Mike Griese 2020-04-15 10:52:11 -05:00 committed by GitHub
parent 37e62dabd5
commit dc43524eb2
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
4 changed files with 199 additions and 21 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

16
.github/actions/spell-check/dictionary/dictionary.txt vendored
View file

@ -384723,23 +384723,7 @@ spadonic
spadonism
spadrone
spadroon
spae
spaebook
spaecraft
spaed
spaedom
spaeing
spaeings
spae-man
spaeman
spaer
Spaerobee
spaes
spaetzle
spaewife
spaewoman
spaework
spaewright
SPAG
spag
spagetti

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

@ -188,6 +188,8 @@ class TerminalCoreUnitTests::ConptyRoundtripTests final
TEST_METHOD(OutputWrappedLineWithSpace);
TEST_METHOD(OutputWrappedLineWithSpaceAtBottomOfBuffer);
TEST_METHOD(BreakLinesOnCursorMovement);
TEST_METHOD(ScrollWithMargins);
private:
@ -2186,3 +2188,174 @@ void ConptyRoundtripTests::OutputWrappedLineWithSpaceAtBottomOfBuffer()
Log::Comment(L"========== Checking the terminal buffer state ==========");
verifyBuffer(termTb, term->_mutableViewport.ToInclusive());
}
void ConptyRoundtripTests::BreakLinesOnCursorMovement()
{
BEGIN_TEST_METHOD_PROPERTIES()
TEST_METHOD_PROPERTY(L"Data:cursorMovementMode", L"{0, 1, 2, 3, 4, 5, 6}")
END_TEST_METHOD_PROPERTIES();
constexpr int MoveCursorWithCUP = 0;
constexpr int MoveCursorWithCR_LF = 1;
constexpr int MoveCursorWithLF_CR = 2;
constexpr int MoveCursorWithVPR_CR = 3;
constexpr int MoveCursorWithCUB_LF = 4;
constexpr int MoveCursorWithCUD_CR = 5;
constexpr int MoveCursorWithNothing = 6;
INIT_TEST_PROPERTY(int, cursorMovementMode, L"Controls how we move the cursor, either with CUP, newline/carriage-return, or some other VT sequence");
Log::Comment(L"This is a test for GH#5291. WSL vim uses spaces to clear the"
L" ends of blank lines, not EL. This test ensures we emit text"
L" from conpty such that the terminal re-creates the state of"
L" the host, which includes wrapped lines of lots of spaces.");
VERIFY_IS_NOT_NULL(_pVtRenderEngine.get());
auto& g = ServiceLocator::LocateGlobals();
auto& renderer = *g.pRender;
auto& gci = g.getConsoleInformation();
auto& si = gci.GetActiveOutputBuffer();
auto& hostSm = si.GetStateMachine();
auto& termTb = *term->_buffer;
_flushFirstFrame();
// Any of the cursor movements that use a LF will actually hard break the
// line - everything else will leave it marked as wrapped.
const bool expectHardBreak = (cursorMovementMode == MoveCursorWithLF_CR) ||
(cursorMovementMode == MoveCursorWithCR_LF) ||
(cursorMovementMode == MoveCursorWithCUB_LF);
auto verifyBuffer = [&](const TextBuffer& tb,
const til::rectangle viewport) {
const auto lastRow = viewport.bottom<short>() - 1;
const til::point expectedCursor{ 5, lastRow };
VERIFY_ARE_EQUAL(expectedCursor, til::point{ tb.GetCursor().GetPosition() });
VERIFY_IS_TRUE(tb.GetCursor().IsVisible());
for (auto y = viewport.top<short>(); y < lastRow; y++)
{
// We're using CUP to move onto the status line _always_, so the
// second-last row will always be marked as wrapped.
const auto rowWrapped = (!expectHardBreak) || (y == lastRow - 1);
VERIFY_ARE_EQUAL(rowWrapped, tb.GetRowByOffset(y).GetCharRow().WasWrapForced());
TestUtils::VerifyExpectedString(tb, L"~ ", til::point{ 0, y });
}
TestUtils::VerifyExpectedString(tb, L"AAAAA", til::point{ 0, lastRow });
};
// We're _not_ checking the conpty output during this test, only the side effects.
_logConpty = true;
_checkConptyOutput = false;
// Lock must be taken to manipulate alt/main buffer state.
gci.LockConsole();
auto unlock = wil::scope_exit([&] { gci.UnlockConsole(); });
// Use DECALN to fill the buffer with 'E's.
hostSm.ProcessString(L"\x1b#8");
Log::Comment(L"Painting the frame");
VERIFY_SUCCEEDED(renderer.PaintFrame());
Log::Comment(L"Switching to the alt buffer");
hostSm.ProcessString(L"\x1b[?1049h");
auto restoreBuffer = wil::scope_exit([&] { hostSm.ProcessString(L"\x1b[?1049l"); });
auto& altBuffer = gci.GetActiveOutputBuffer();
auto& altTextBuffer = altBuffer.GetTextBuffer();
// Go home and clear the screen.
hostSm.ProcessString(L"\x1b[H");
hostSm.ProcessString(L"\x1b[2J");
// Write out lines of '~' followed by enough spaces to fill the line.
hostSm.ProcessString(L"\x1b[94m");
for (auto y = 0; y < altBuffer.GetViewport().BottomInclusive(); y++)
{
// Vim uses CUP to position the cursor on the first cell of each row, every row.
if (cursorMovementMode == MoveCursorWithCUP)
{
std::wstringstream ss;
ss << L"\x1b[";
ss << y + 1;
ss << L";1H";
hostSm.ProcessString(ss.str());
}
// As an additional test, try breaking lines manually with \r\n
else if (cursorMovementMode == MoveCursorWithCR_LF)
{
// Don't need to newline on the 0'th row
if (y > 0)
{
hostSm.ProcessString(L"\r\n");
}
}
// As an additional test, try breaking lines manually with \n\r
else if (cursorMovementMode == MoveCursorWithLF_CR)
{
// Don't need to newline on the 0'th row
if (y > 0)
{
hostSm.ProcessString(L"\n\r");
}
}
// As an additional test, move the cursor down with VPR, then to the start of the line with CR
else if (cursorMovementMode == MoveCursorWithVPR_CR)
{
// Don't need to newline on the 0'th row
if (y > 0)
{
hostSm.ProcessString(L"\x1b[1e");
hostSm.ProcessString(L"\r");
}
}
// As an additional test, move the cursor back with CUB, then down with LF
else if (cursorMovementMode == MoveCursorWithCUB_LF)
{
// Don't need to newline on the 0'th row
if (y > 0)
{
hostSm.ProcessString(L"\x1b[80D");
hostSm.ProcessString(L"\n");
}
}
// As an additional test, move the cursor down with CUD, then to the start of the line with CR
else if (cursorMovementMode == MoveCursorWithCUD_CR)
{
// Don't need to newline on the 0'th row
if (y > 0)
{
hostSm.ProcessString(L"\x1b[B");
hostSm.ProcessString(L"\r");
}
}
// Win32 vim.exe will simply do _nothing_ in this scenario. It'll just
// print the lines one after the other, without moving the cursor,
// relying on us auto moving to the following line.
else if (cursorMovementMode == MoveCursorWithNothing)
{
}
// IMPORTANT! The way vim writes these blank lines is as '~' followed by
// enough spaces to fill the line.
// This bug (GH#5291 won't repro if you don't have the spaces).
std::wstring line{ L"~" };
line += std::wstring(79, L' ');
hostSm.ProcessString(line);
}
// Print the "Status Line"
hostSm.ProcessString(L"\x1b[32;1H");
hostSm.ProcessString(L"\x1b[m");
hostSm.ProcessString(L"AAAAA");
Log::Comment(L"========== Checking the host buffer state ==========");
verifyBuffer(altTextBuffer, altBuffer.GetViewport().ToInclusive());
Log::Comment(L"Painting the frame");
VERIFY_SUCCEEDED(renderer.PaintFrame());
Log::Comment(L"========== Checking the terminal buffer state ==========");
verifyBuffer(termTb, term->_mutableViewport.ToInclusive());
}

13
src/host/screenInfo.cpp
View file

@ -1663,6 +1663,19 @@ void SCREEN_INFORMATION::SetCursorDBMode(const bool DoubleCursor)
return STATUS_INVALID_PARAMETER;
}
// In GH#5291, we experimented with manually breaking the line on all cursor
// movements here. As we print lines into the buffer, we mark lines as
// wrapped when we print the last cell of the row, not the first cell of the
// subsequent row (the row the first line wrapped onto).
//
// Logically, we thought that manually breaking lines when we move the
// cursor was a good idea. We however, did not have the time to fully
// validate that this was the correct answer, and a simpler solution for the
// bug on hand was found. Furthermore, we thought it would be a more
// comprehensive solution to only mark lines as wrapped when we print the
// first cell of the second row, which would require some WriteCharsLegacy
// work.
cursor.SetPosition(Position);
// If the cursor has moved below the virtual bottom, the bottom should be updated.

18
src/renderer/vt/paint.cpp
View file

@ -443,15 +443,23 @@ using namespace Microsoft::Console::Types;
const bool useEraseChar = (optimalToUseECH) &&
(!_newBottomLine) &&
(!_clearedAllThisFrame);
const bool printingBottomLine = coord.Y == _lastViewport.BottomInclusive();
// If we're not using erase char, but we did erase all at the start of the
// frame, don't add spaces at the end.
//
// GH#5161: Only removeSpaces when we're in the _newBottomLine state and the
// line we're trying to print right now _actually is the bottom line_
const bool removeSpaces = useEraseChar ||
_clearedAllThisFrame ||
(_newBottomLine && coord.Y == _lastViewport.BottomInclusive());
//
// GH#5291: DON'T remove spaces when the row wrapped. We might need those
// spaces to preserve the wrap state of this line, or the cursor position.
// For example, vim.exe uses "~ "... to clear the line, and then leaves
// the lines _wrapped_. It doesn't care to manually break the lines, but if
// we trimmed the spaces off here, we'd print all the "~"s one after another
// on the same line.
const bool removeSpaces = !lineWrapped && (useEraseChar ||
_clearedAllThisFrame ||
(_newBottomLine && printingBottomLine));
const size_t cchActual = removeSpaces ?
(cchLine - numSpaces) :
cchLine;
@ -547,7 +555,7 @@ using namespace Microsoft::Console::Types;
RETURN_IF_FAILED(_EraseLine());
}
}
else if (_newBottomLine && coord.Y == _lastViewport.BottomInclusive())
else if (_newBottomLine && printingBottomLine)
{
// If we're on a new line, then we don't need to erase the line. The
// line is already empty.
@ -566,7 +574,7 @@ using namespace Microsoft::Console::Types;
// If we printed to the bottom line, and we previously thought that this was
// a new bottom line, it certainly isn't new any longer.
if (coord.Y == _lastViewport.BottomInclusive())
if (printingBottomLine)
{
_newBottomLine = false;
}