Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix copying wrapped lines by implementing better scrolling #5181

Merged
51 commits merged into from
Apr 9, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
51 commits
Select commit Hold shift + click to select a range
4bbb63d
Add tracing for circling and scrolling operations. Fix improper inval…
miniksa Mar 25, 2020
a543b1f
PR feedback applied. Don't bother making string if no one is listenin…
miniksa Mar 25, 2020
9ba2c69
More tracing is always good
zadjii-msft Mar 25, 2020
f10edc0
Merge remote-tracking branch 'origin/dev/miniksa/tmux_draw' into dev/…
zadjii-msft Mar 26, 2020
d42f960
So this works but I think it will break the EOL backspace test
zadjii-msft Mar 26, 2020
22bdf9b
This can't possibly be right... can it?
zadjii-msft Mar 26, 2020
466e8fc
mysteriously the existing tests all basically pass
zadjii-msft Mar 26, 2020
248d223
this is a simple test for the case that already worked
zadjii-msft Mar 26, 2020
b94cfdb
This test failure helps explain that this doesn't work
zadjii-msft Mar 26, 2020
e21101b
This all wroks far to shockingly well
zadjii-msft Mar 26, 2020
cf1145c
Add a test for this case
zadjii-msft Mar 26, 2020
2665b4c
this fixes this test, but maybe the test was always broken?
zadjii-msft Mar 27, 2020
2a0cc20
I'm honestly shocked that this seems to work
zadjii-msft Mar 27, 2020
870f052
Merge remote-tracking branch 'origin/master' into dev/migrie/b/5113-w…
zadjii-msft Mar 27, 2020
51f060e
cleanup for review, but I have to wait for #5122 to merge first
zadjii-msft Mar 27, 2020
8ecfcec
Merge branch 'master' into dev/migrie/b/5113-with-miniksas-fix
zadjii-msft Mar 30, 2020
436335f
Fix the test @miniksa wrote for #5122
zadjii-msft Mar 30, 2020
022b01a
Merge branch 'master' into dev/migrie/b/5113-with-miniksas-fix
zadjii-msft Apr 1, 2020
ac85d43
pr nits from dustin
zadjii-msft Apr 1, 2020
465f8be
Merge branch 'master' into dev/migrie/b/5113-with-miniksas-fix
zadjii-msft Apr 1, 2020
d317b8b
pr nits from carlos
zadjii-msft Apr 1, 2020
707844d
add the failing test for the case Dustin described
zadjii-msft Apr 1, 2020
8b19fcb
Fix this case for @dhowett-msft
zadjii-msft Apr 1, 2020
b70aacd
good bot
zadjii-msft Apr 1, 2020
0cbcf1f
fix a typo
zadjii-msft Apr 1, 2020
1c9a16f
add a test for #5039
zadjii-msft Apr 1, 2020
c7e5a45
This is the test case that's actaully broken in #5039
zadjii-msft Apr 2, 2020
015844b
finish the test for #5039
zadjii-msft Apr 2, 2020
a939f7e
Update this test with more info from the discussion thread
zadjii-msft Apr 2, 2020
a1cd2c5
This is the latest testcase dustin found for me
zadjii-msft Apr 2, 2020
c39871b
Fix the bug that Dustin reported in the PR
zadjii-msft Apr 3, 2020
746a1c8
Frick forgot to save
zadjii-msft Apr 3, 2020
9ea3960
Spellcheck fixes, thanks to @jsoref
zadjii-msft Apr 3, 2020
12f9c82
Add more comments
zadjii-msft Apr 3, 2020
3495cad
Starting on working on fixing this bug, but I can't get a minimal rep…
zadjii-msft Apr 3, 2020
c0fb26f
This scratch application repros a similar bug, so I'm treating this a…
zadjii-msft Apr 3, 2020
812dc86
wait this test does work for the host
zadjii-msft Apr 3, 2020
87a1486
Add a real test for #5161 and fix the associated bug
zadjii-msft Apr 6, 2020
017f302
Merge branch 'dev/migrie/b/5161-mingw-vim-fix' into dev/migrie/b/5113…
zadjii-msft Apr 6, 2020
c7fd127
Fix the Xterm*Invalidate tests
zadjii-msft Apr 6, 2020
55f81bc
I really wish I could run this bot locally on save or _before_ pushing
zadjii-msft Apr 6, 2020
3137a3b
Revert scratch.exe
zadjii-msft Apr 6, 2020
b188629
Some nits from Dustin
zadjii-msft Apr 6, 2020
6f9bd9a
test commit please ignore
zadjii-msft Apr 8, 2020
45d9f06
Merge remote-tracking branch 'origin/master' into dev/migrie/b/5113-w…
zadjii-msft Apr 8, 2020
1cc1035
some PR feedback
zadjii-msft Apr 8, 2020
c111961
I guess I need this one too
zadjii-msft Apr 8, 2020
15a47d3
Add a test to cover the new case Dustin found
zadjii-msft Apr 8, 2020
5e167cd
Fix the bug that dustin found. 0% change spellcheck bot loves me
zadjii-msft Apr 8, 2020
efe4aaf
love me
zadjii-msft Apr 8, 2020
d5af8c6
This makes more sense
zadjii-msft Apr 8, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/actions/spell-check/patterns/patterns.txt
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,4 @@ Scro\&ll
:\\windows\\syste\b
TestUtils::VerifyExpectedString\(tb, L"[^"]+"
hostSm\.ProcessString\(L"[^"]+"
([A-Za-z])\1{3,}
\b([A-Za-z])\1{3,}\b
236 changes: 231 additions & 5 deletions src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,7 @@ class TerminalCoreUnitTests::ConptyRoundtripTests final
TEST_METHOD(OverstrikeAtBottomOfBuffer);
TEST_METHOD(MarginsWithStatusLine);
TEST_METHOD(OutputWrappedLineWithSpace);
TEST_METHOD(OutputWrappedLineWithSpaceAtBottomOfBuffer);

TEST_METHOD(ScrollWithMargins);

Expand Down Expand Up @@ -1094,6 +1095,15 @@ void ConptyRoundtripTests::OutputWrappedLinesAtTopOfBuffer()
sm.ProcessString(std::wstring(wrappedLineLength, L'A'));

auto verifyBuffer = [](const TextBuffer& tb) {
// Buffer contents should look like the following: (80 wide)
// (w) means we hard wrapped the line
// (b) means the line is _not_ wrapped (it's broken, the default state.)
// cursor is on the '_'
//
// |AAAAAAAA...AAAA| (w)
// |AAAAA_ ... | (b) (There are 20 'A's on this line.)
// | ... | (b)

VERIFY_IS_TRUE(tb.GetRowByOffset(0).GetCharRow().WasWrapForced());
VERIFY_IS_FALSE(tb.GetRowByOffset(1).GetCharRow().WasWrapForced());
auto iter0 = tb.GetCellDataAt({ 0, 0 });
Expand Down Expand Up @@ -1157,17 +1167,75 @@ void ConptyRoundtripTests::OutputWrappedLinesAtBottomOfBuffer()

const auto wrappedLineLength = TerminalViewWidth + 20;

// The following diagrams show the buffer contents after each string emitted
// from conpty. For each of these diagrams:
// (w) means we hard wrapped the line
// (b) means the line is _not_ wrapped (it's broken, the default state.)
// cursor is on the '_'

// Initial state:
// |X | (b)
// |X | (b)
// ...
// |X | (b)
// |_ | (b)

expectedOutput.push_back(std::string(TerminalViewWidth, 'A'));
// |X | (b)
// |X | (b)
// ...
// |X | (b)
// |AAAAAAAA...AAAA|_ (w) The cursor is actually on the last A here

// TODO GH#5228 might break the "newline & repaint the wrapped char" checks here, that's okay.
expectedOutput.push_back("\r"); // This \r\n is emitted by ScrollFrame to
expectedOutput.push_back("\n"); // add a newline to the bottom of the buffer
// |X | (b)
// |X | (b)
// ...
// |X | (b)
// |AAAAAAAA...AAAA| (b)
// |_ | (b)

expectedOutput.push_back("\x1b[31;80H"); // Move the cursor BACK to the wrapped row
zadjii-msft marked this conversation as resolved.
Show resolved Hide resolved
// |X | (b)
// |X | (b)
// ...
// |X | (b)
// |AAAAAAAA...AAAA| (b) The cursor is actually on the last A here
// | | (b)

expectedOutput.push_back(std::string(1, 'A')); // Reprint the last character of the wrapped row
// |X | (b)
// |X | (b)
// ...
// |X | (b)
// |AAAAAAAA...AAAA|_ (w) The cursor is actually on the last A here
// | | (b)

expectedOutput.push_back(std::string(20, 'A')); // Print the second line.
// |X | (b)
// |X | (b)
// ...
// |X | (b)
// |AAAAAAAA...AAAA| (w)
// |AAAAA_ | (b) There are 20 'A's on this line.

hostSm.ProcessString(std::wstring(wrappedLineLength, L'A'));

auto verifyBuffer = [](const TextBuffer& tb, const short wrappedRow) {
// Buffer contents should look like the following: (80 wide)
// (w) means we hard wrapped the line
// (b) means the line is _not_ wrapped (it's broken, the default state.)
// cursor is on the '_'
//
// |X | (b)
// |X | (b)
// ...
// |X | (b)
// |AAAAAAAA...AAAA| (w)
// |AAAAA_ ... | (b) (There are 20 'A's on this line.)

VERIFY_IS_TRUE(tb.GetRowByOffset(wrappedRow).GetCharRow().WasWrapForced());
VERIFY_IS_FALSE(tb.GetRowByOffset(wrappedRow + 1).GetCharRow().WasWrapForced());

Expand Down Expand Up @@ -1275,6 +1343,7 @@ void ConptyRoundtripTests::ScrollWithChangesInMiddle()

auto iter0 = tb.GetCellDataAt({ 0, wrappedRow });
TestUtils::VerifySpanOfText(L"A", iter0, 0, TerminalViewWidth);

auto iter1 = tb.GetCellDataAt({ 0, wrappedRow + 1 });
TestUtils::VerifySpanOfText(L"A", iter1, 0, 20);
auto iter2 = tb.GetCellDataAt({ 20, wrappedRow + 1 });
Expand Down Expand Up @@ -1934,12 +2003,23 @@ void ConptyRoundtripTests::OutputWrappedLineWithSpace()
sm.ProcessString(std::wstring(secondTextLength, L'B'));

auto verifyBuffer = [&](const TextBuffer& tb) {
// Buffer contents should look like the following: (80 wide)
// (w) means we hard wrapped the line
// (b) means the line is _not_ wrapped (it's broken, the default state.)
//
// |AAAA...AA | (w)
// | B_ ... | (b) (cursor is on the '_')
// | ... | (b)

VERIFY_IS_TRUE(tb.GetRowByOffset(0).GetCharRow().WasWrapForced());
VERIFY_IS_FALSE(tb.GetRowByOffset(1).GetCharRow().WasWrapForced());

// First row
auto iter0 = tb.GetCellDataAt({ 0, 0 });
TestUtils::VerifySpanOfText(L"A", iter0, 0, firstTextLength);
TestUtils::VerifySpanOfText(L" ", iter0, 0, 2);

// Second row
auto iter1 = tb.GetCellDataAt({ 0, 1 });
TestUtils::VerifySpanOfText(L" ", iter1, 0, 1);
auto iter2 = tb.GetCellDataAt({ 1, 1 });
Expand All @@ -1949,14 +2029,160 @@ void ConptyRoundtripTests::OutputWrappedLineWithSpace()
Log::Comment(L"========== Checking the host buffer state ==========");
verifyBuffer(hostTb);

// TODO: Actually check the conpty output here.
_checkConptyOutput = false;
_logConpty = true;
// expectedOutput.push_back(std::string(TerminalViewWidth, 'A'));
// expectedOutput.push_back(std::string(20, 'A'));
std::string firstLine = std::string(firstTextLength, 'A');
firstLine += " ";
std::string secondLine{ " B" };

expectedOutput.push_back(firstLine);
expectedOutput.push_back(secondLine);
Log::Comment(L"Painting the frame");
VERIFY_SUCCEEDED(renderer.PaintFrame());

Log::Comment(L"========== Checking the terminal buffer state ==========");
verifyBuffer(termTb);
}

void ConptyRoundtripTests::OutputWrappedLineWithSpaceAtBottomOfBuffer()
{
// See https://github.com/microsoft/terminal/pull/5181#issuecomment-610110348
// This is the same test as OutputWrappedLineWithSpace, but at the bottom of
// the buffer, so we get scrolling behavior as well.
Log::Comment(L"Ensures that a buffer line in conhost that wrapped _on a "
L"space_ will still be emitted as wrapped.");
VERIFY_IS_NOT_NULL(_pVtRenderEngine.get());

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;

_flushFirstFrame();

// First, fill the buffer with contents, so conpty starts circling
const auto hostView = si.GetViewport();
const auto end = 2 * hostView.Height();
for (auto i = 0; i < end; i++)
{
Log::Comment(NoThrowString().Format(L"Writing line %d/%d", i, end));
expectedOutput.push_back("X");
if (i < hostView.BottomInclusive())
{
expectedOutput.push_back("\r\n");
}
else
{
// After we hit the bottom of the viewport, the newlines come in
// separated by empty writes for whatever reason.
expectedOutput.push_back("\r");
expectedOutput.push_back("\n");
expectedOutput.push_back("");
}

sm.ProcessString(L"X\n");

VERIFY_SUCCEEDED(renderer.PaintFrame());
}

const auto firstTextLength = TerminalViewWidth - 2;
const auto spacesLength = 3;
const auto secondTextLength = 1;

std::string firstLine = std::string(firstTextLength, 'A');
firstLine += " ";
std::string secondLine{ " B" };

// The following diagrams show the buffer contents after each string emitted
// from conpty. For each of these diagrams:
// (w) means we hard wrapped the line
// (b) means the line is _not_ wrapped (it's broken, the default state.)
// cursor is on the '_'

// Initial state:
// |X | (b)
// |X | (b)
// ...
// |X | (b)
// |_ | (b)

expectedOutput.push_back(firstLine);
// |X | (b)
// |X | (b)
// ...
// |X | (b)
// |AAAAAAAA...AA _| (w) The cursor is actually on the last ' ' here

// TODO GH#5228 might break the "newline & repaint the wrapped char" checks here, that's okay.
expectedOutput.push_back("\r"); // This \r\n is emitted by ScrollFrame to
expectedOutput.push_back("\n"); // add a newline to the bottom of the buffer
// |X | (b)
// |X | (b)
// ...
// |X | (b)
// |AAAAAAAA...AA | (b)
// |_ | (b)

expectedOutput.push_back("\x1b[31;80H"); // Move the cursor BACK to the wrapped row
// |X | (b)
// |X | (b)
// ...
// |X | (b)
// |AAAAAAAA...AA _| (b) The cursor is actually on the last ' ' here
// | | (b)

expectedOutput.push_back(std::string(1, ' ')); // Reprint the last character of the wrapped row
// |X | (b)
// |X | (b)
// ...
// |X | (b)
// |AAAAAAAA...AA |_ (w) The cursor is actually on the last ' ' here
// | | (b)

expectedOutput.push_back(secondLine);
// |X | (b)
// |X | (b)
// ...
// |X | (b)
// |AAAAAAAA...AA | (w)
// | B_ | (b)

sm.ProcessString(std::wstring(firstTextLength, L'A'));
sm.ProcessString(std::wstring(spacesLength, L' '));
sm.ProcessString(std::wstring(secondTextLength, L'B'));

auto verifyBuffer = [&](const TextBuffer& tb, const til::rectangle viewport) {
// Buffer contents should look like the following: (80 wide)
// (w) means we hard wrapped the line
// (b) means the line is _not_ wrapped (it's broken, the default state.)
//
// |AAAA...AA | (w)
// | B_ ... | (b) (cursor is on the '_')
// | ... | (b)

const short wrappedRow = viewport.bottom<short>() - 2;
VERIFY_IS_TRUE(tb.GetRowByOffset(wrappedRow).GetCharRow().WasWrapForced());
VERIFY_IS_FALSE(tb.GetRowByOffset(wrappedRow + 1).GetCharRow().WasWrapForced());

// First row
auto iter0 = tb.GetCellDataAt({ 0, wrappedRow });
TestUtils::VerifySpanOfText(L"A", iter0, 0, firstTextLength);
TestUtils::VerifySpanOfText(L" ", iter0, 0, 2);

// Second row
auto iter1 = tb.GetCellDataAt({ 0, wrappedRow + 1 });
TestUtils::VerifySpanOfText(L" ", iter1, 0, 1);
auto iter2 = tb.GetCellDataAt({ 1, wrappedRow + 1 });
TestUtils::VerifySpanOfText(L"B", iter2, 0, secondTextLength);
};

Log::Comment(L"========== Checking the host buffer state ==========");
verifyBuffer(hostTb, hostView.ToInclusive());

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

Log::Comment(L"========== Checking the terminal buffer state ==========");
verifyBuffer(termTb, term->_mutableViewport.ToInclusive());
}
11 changes: 4 additions & 7 deletions src/renderer/vt/paint.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -478,16 +478,13 @@ using namespace Microsoft::Console::Types;
std::wstring wstr = std::wstring(unclusteredString.data(), cchActual);
RETURN_IF_FAILED(VtEngine::_WriteTerminalUtf8(wstr));

// If we've written text to the last column of the viewport, then mark
// GH#4415, GH#5181
// If the renderer told us that this was a wrapped line, then mark
// that we've wrapped this line. The next time we attempt to move the
// cursor, if we're trying to move it to the start of the next line,
// we'll remember that this line was wrapped, and not manually break the
// line.
// Don't do this if the last character we're writing is a space - The last
// char will always be a space, but if we see that, we shouldn't wrap.
const short lastWrittenChar = base::ClampAdd(_lastText.X, base::ClampSub(totalWidth, numSpaces));
if (lineWrapped &&
lastWrittenChar > _lastViewport.RightInclusive())
if (lineWrapped)
{
_wrappedRow = coord.Y;
_trace.TraceSetWrapped(coord.Y);
Expand Down Expand Up @@ -558,7 +555,7 @@ using namespace Microsoft::Console::Types;
{
_deferredCursorPos = { _lastText.X + sNumSpaces, _lastText.Y };
}
else if (numSpaces > 0)
else if (removeSpaces && numSpaces > 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're removing spaces, and there were spaces, print them? I am confus

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Man so am I, I have no idea why that fixed the test and didn't break any of the other ones. It was literally the first thing I guessed, looks like I didn't actually add a comment

{
std::wstring spaces = std::wstring(numSpaces, L' ');
RETURN_IF_FAILED(VtEngine::_WriteTerminalUtf8(spaces));
Expand Down