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.

Sign up for GitHub

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

Jump to bottom

Don't duplicate spaces from potentially-wrapped EOL-deferred lines #5398

Merged
2 commits merged into from
Apr 20, 2020
Merged

Don't duplicate spaces from potentially-wrapped EOL-deferred lines #5398

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
501871d
Don't duplicate spaces from potentially-wrapped EOL-deferred lines
DHowett Apr 17, 2020
876b2e3
Update src/renderer/vt/paint.cpp
DHowett Apr 20, 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
56 changes: 56 additions & 0 deletions src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,8 @@ class TerminalCoreUnitTests::ConptyRoundtripTests final

TEST_METHOD(ScrollWithMargins);

TEST_METHOD(TestCursorInDeferredEOLPositionOnNewLineWithSpaces);

private:
bool _writeCallback(const char* const pch, size_t const cch);
void _flushFirstFrame();
Expand Down Expand Up @@ -2359,3 +2361,57 @@ void ConptyRoundtripTests::BreakLinesOnCursorMovement()

verifyBuffer(termTb, term->_mutableViewport.ToInclusive());
}

void ConptyRoundtripTests::TestCursorInDeferredEOLPositionOnNewLineWithSpaces()
{
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 termView = term->GetViewport();

_flushFirstFrame();
_checkConptyOutput = false;

// newline down to the bottom
hostSm.ProcessString(std::wstring(gsl::narrow_cast<size_t>(TerminalViewHeight), L'\n'));
// fill width-1 with "A", then add one space and another character..
hostSm.ProcessString(std::wstring(gsl::narrow_cast<size_t>(TerminalViewWidth) - 1, L'A') + L" B");

auto verifyBuffer = [&](const TextBuffer& tb, SHORT bottomRow) {
// 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 '_'
//
// | ^ ^ ^ ^ ^ ^ ^ | ( ) (entire buffer above us; contents do not matter)
// | | ( ) (entire buffer above us; contents do not matter)
// |AAAAAAAA...AAA | (w) (79 'A's, one space)
// |B_ ... | (b) (There's only one 'B' on this line)

miniksa marked this conversation as resolved.
Show resolved Hide resolved
til::point cursorPos{ tb.GetCursor().GetPosition() };
// The cursor should be on the second char of the last line
VERIFY_ARE_EQUAL((til::point{ 1, bottomRow }), cursorPos);

const auto& secondToLastRow = tb.GetRowByOffset(bottomRow - 1);
const auto& lastRow = tb.GetRowByOffset(bottomRow);
VERIFY_IS_TRUE(secondToLastRow.GetCharRow().WasWrapForced());
VERIFY_IS_FALSE(lastRow.GetCharRow().WasWrapForced());

auto expectedStringSecondToLastRow{ std::wstring(gsl::narrow_cast<size_t>(tb.GetSize().Width()) - 1, L'A') + L" " };
TestUtils::VerifyExpectedString(tb, expectedStringSecondToLastRow, { 0, bottomRow - 1 });
TestUtils::VerifyExpectedString(tb, L"B", { 0, bottomRow });
};

const auto hostView = si.GetViewport();
verifyBuffer(hostTb, hostView.BottomInclusive());

VERIFY_SUCCEEDED(renderer.PaintFrame());

const auto newTermView = term->GetViewport();
verifyBuffer(termTb, newTermView.BottomInclusive());
}
3 changes: 2 additions & 1 deletion src/renderer/vt/paint.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -563,8 +563,9 @@ using namespace Microsoft::Console::Types;
{
_deferredCursorPos = { _lastText.X + sNumSpaces, _lastText.Y };
}
else if (numSpaces > 0)
else if (numSpaces > 0 && removeSpaces) // if we deleted the spaces... re-add them
Copy link
Member

Choose a reason for hiding this comment

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

Leave the "why" we're readding them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so part of my confusion is I actually don't know why we would re-add them if we chose to delete them. we should just not choose to delete them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zadjii-msft can you remember why we'd re-add them if we'd deleted them?

Copy link
Member

Choose a reason for hiding this comment

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

Eh if we can't totally suss it out, just leave the MSFT to the code health item here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay, follow-up filing.

Copy link
Member

Choose a reason for hiding this comment

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

I can't remember tbh. This screams organic growth to me - I think we first just decided to do ECH and removing spaces, when we didn't have wrapping (and spaces didn't matter). Then we did some deferring the cursor movement & not erasing optimizations in 20H1, which added cases where we wouldn't ECH. Now we're adding cases where we still need the spaces. I suppose if we'd written it from scratch now, accounting for wrapping, we'd do it better.

I'd TODO it tbh.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#5430, linking in the comments

Copy link
Member

Choose a reason for hiding this comment

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

I bet we were trying to optimize out the cursor positioning that we'd do after a new line is printed. If we did optimize that scenario, and used ECH to erase some spaces, we also used CUF to move the cursor. However, if we couldn't do that, we'd still want to leave the cursor in the right place...

yea we probably should have just not removed them in the first place.

{
// TODO GH#5430 - Determine why and when we would do this.
std::wstring spaces = std::wstring(numSpaces, L' ');
DHowett-MSFT marked this conversation as resolved.
Show resolved Hide resolved
RETURN_IF_FAILED(VtEngine::_WriteTerminalUtf8(spaces));

Expand Down