Skip to content

Commit

Permalink
Cleanup for review - this is a _great_ fix for #3490 as well as #1465
Browse files Browse the repository at this point in the history
  • Loading branch information
@zadjii-msft
zadjii-msft committed Jan 30, 2020
1 parent 9580715 commit edea9a3
Show file tree
Hide file tree
Showing 3 changed files with 249 additions and 96 deletions.
7 changes: 6 additions & 1 deletion src/buffer/out/textBuffer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1554,9 +1554,14 @@ std::string TextBuffer::GenRTF(const TextAndColor& rows, const int fontHeightPoi
// Arguments:
// - oldBuffer - the text buffer to copy the contents FROM
// - newBuffer - the text buffer to copy the contents TO
// - lastCharacterViewport - Optional. If the caller knows that the last
// nonspace character is in a particular Viewport, the caller can provide this
// parameter as an optimization, as opposed to searching the entire buffer.
// Return Value:
// - S_OK if we successfully copied the contents to the new buffer, otherwise an appropriate HRESULT.
HRESULT TextBuffer::Reflow(TextBuffer& oldBuffer, TextBuffer& newBuffer, const std::optional<Viewport> lastCharacterViewport)
HRESULT TextBuffer::Reflow(TextBuffer& oldBuffer,
TextBuffer& newBuffer,
const std::optional<Viewport> lastCharacterViewport)
{
Cursor& oldCursor = oldBuffer.GetCursor();
Cursor& newCursor = newBuffer.GetCursor();
Expand Down
124 changes: 29 additions & 95 deletions src/cascadia/TerminalCore/Terminal.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -187,115 +187,49 @@ void Terminal::UpdateSettings(winrt::Microsoft::Terminal::Settings::ICoreSetting
{
newTextBuffer = std::make_unique<TextBuffer>(bufferSize,
_buffer->GetCurrentAttributes(),
0,
_buffer->GetRenderTarget()); // temporarily set size to 0 so it won't render.
0, // temporarily set size to 0 so it won't render.
_buffer->GetRenderTarget());
}
CATCH_RETURN();

// RETURN_IF_FAILED(_buffer->ResizeTraditional(bufferSize));
RETURN_IF_FAILED(TextBuffer::Reflow(*_buffer.get(), *newTextBuffer.get(), _mutableViewport));

// OLD WAY OF FINDING NEW TERMINAL VIEWPORT
{
// auto proposedTop = oldTop;
// const auto newView = Viewport::FromDimensions({ 0, proposedTop }, viewportSize);
// const auto proposedBottom = newView.BottomExclusive();
// // If the new bottom would be below the bottom of the buffer, then slide the
// // top up so that we'll still fit within the buffer.
// if (proposedBottom > bufferSize.Y)
// {
// proposedTop -= (proposedBottom - bufferSize.Y);
// }
// _mutableViewport = Viewport::FromDimensions({ 0, proposedTop }, viewportSize);
}

// SCREENINFO WAY OF FINDING NEW TERMINAL VIEWPORT
// However conpty resizes a little oddly - if the height decreased, and
// there were blank lines at the bottom, those lines will get trimmed.
// If there's not blank lines, then the top will get "shifted down",
// moving the top line into scrollback.
// See GH#3490 for more details.

// If the final position in the buffer is on the bottom row of the new
// viewport, then we're going to need to move the top down. Otherwise,
// move the bottom up.
const auto dy = viewportSize.Y - oldDimensions.Y;
const COORD oldCursorPos = _buffer->GetCursor().GetPosition();
COORD oldLastChar = oldCursorPos;
try
{
// // Save cursor's relative height versus the viewport
// SHORT const sCursorHeightInViewportBefore = _buffer->GetCursor().GetPosition().Y - _mutableViewport.Top();
// auto& newCursor = newTextBuffer->GetCursor();
// // Adjust the viewport so the cursor doesn't wildly fly off up or down.
// const short cursorHeightInViewportAfter = newCursor.GetPosition().Y - _mutableViewport.Top();
// const short cursorHeightDiff = cursorHeightInViewportAfter - sCursorHeightInViewportBefore;
// // LOG_IF_FAILED(SetViewportOrigin(false, coordCursorHeightDiff, true));

// auto proposedTop = static_cast<short>(oldTop + cursorHeightDiff);
// const auto newView = Viewport::FromDimensions({ 0, proposedTop }, viewportSize);
// const auto proposedBottom = newView.BottomExclusive();
// // If the new bottom would be below the bottom of the buffer, then slide the
// // top up so that we'll still fit within the buffer.
// if (proposedBottom > bufferSize.Y)
// {
// proposedTop -= (proposedBottom - bufferSize.Y);
// }

// _mutableViewport = Viewport::FromDimensions({ 0, proposedTop }, viewportSize);

oldLastChar = _buffer->GetLastNonSpaceCharacter(_mutableViewport);
}
CATCH_LOG();

// #4354 way of doing things
{
const auto maxRow = std::max(oldLastChar.Y, oldCursorPos.Y);

// const auto dy = viewportSize.Y - oldDimensions.Y;
// const COORD cOldCursorPos = _buffer->GetCursor().GetPosition();
// COORD cOldLastChar = cOldCursorPos;
// try
// {
// cOldLastChar = _buffer->GetLastNonSpaceCharacter();
// }
// CATCH_LOG();
const bool beforeLastRowOfView = maxRow < _mutableViewport.BottomInclusive();
const auto adjustment = beforeLastRowOfView ? 0 : std::max(0, -dy);

// const auto maxRow = std::max(cOldLastChar.Y, cOldCursorPos.Y);

// const bool beforeLastRow = maxRow < bufferSize.Y - 1;
// const auto adjustment = beforeLastRow ? 0 : std::max(0, -dy);

// auto proposedTop = oldTop + adjustment;

// const auto newView = Viewport::FromDimensions({ 0, gsl::narrow_cast<short>(proposedTop) }, viewportSize);
// const auto proposedBottom = newView.BottomExclusive();
// // If the new bottom would be below the bottom of the buffer, then slide the
// // top up so that we'll still fit within the buffer.
// if (proposedBottom > bufferSize.Y)
// {
// proposedTop -= (proposedBottom - bufferSize.Y);
// }

// _mutableViewport = Viewport::FromDimensions({ 0, gsl::narrow_cast<short>(proposedTop) }, viewportSize);
}
auto proposedTop = oldTop + adjustment;

// Attempt No. 4
const auto newView = Viewport::FromDimensions({ 0, gsl::narrow_cast<short>(proposedTop) }, viewportSize);
const auto proposedBottom = newView.BottomExclusive();
// If the new bottom would be below the bottom of the buffer, then slide the
// top up so that we'll still fit within the buffer.
if (proposedBottom > bufferSize.Y)
{
const auto dy = viewportSize.Y - oldDimensions.Y;
const COORD cOldCursorPos = _buffer->GetCursor().GetPosition();
COORD cOldLastChar = cOldCursorPos;
try
{
cOldLastChar = _buffer->GetLastNonSpaceCharacter(_mutableViewport);
}
CATCH_LOG();

const auto maxRow = std::max(cOldLastChar.Y, cOldCursorPos.Y);

// const bool beforeLastRow = maxRow < bufferSize.Y - 1;
const bool beforeLastRowOfView = maxRow < _mutableViewport.BottomInclusive();
// const auto adjustment = beforeLastRow ? 0 : std::max(0, -dy);
const auto adjustment = beforeLastRowOfView ? 0 : std::max(0, -dy);

auto proposedTop = oldTop + adjustment;

const auto newView = Viewport::FromDimensions({ 0, gsl::narrow_cast<short>(proposedTop) }, viewportSize);
const auto proposedBottom = newView.BottomExclusive();
// If the new bottom would be below the bottom of the buffer, then slide the
// top up so that we'll still fit within the buffer.
if (proposedBottom > bufferSize.Y)
{
proposedTop -= (proposedBottom - bufferSize.Y);
}

_mutableViewport = Viewport::FromDimensions({ 0, gsl::narrow_cast<short>(proposedTop) }, viewportSize);
proposedTop -= (proposedBottom - bufferSize.Y);
}

_mutableViewport = Viewport::FromDimensions({ 0, gsl::narrow_cast<short>(proposedTop) }, viewportSize);

_buffer.swap(newTextBuffer);

_scrollOffset = 0;
Expand Down
214 changes: 214 additions & 0 deletions src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,8 @@ class TerminalCoreUnitTests::ConptyRoundtripTests final
TEST_METHOD(TestExactWrappingWithoutSpaces);
TEST_METHOD(TestExactWrappingWithSpaces);

TEST_METHOD(TestResizeHeight);

private:
bool _writeCallback(const char* const pch, size_t const cch);
void _flushFirstFrame();
Expand Down Expand Up @@ -598,3 +600,215 @@ void ConptyRoundtripTests::TestExactWrappingWithSpaces()

verifyBuffer(termTb);
}
void ConptyRoundtripTests::TestResizeHeight()
{
// This test class is _60_ tests to ensure that resizing the terminal works
// with conpty correctly. There's a lot of min/maxing in expressions here,
// to account for the sheer number of cases here, and that we have to handle
// both resizing larger and smaller all in one test.

BEGIN_TEST_METHOD_PROPERTIES()
TEST_METHOD_PROPERTY(L"IsolationLevel", L"Method")
TEST_METHOD_PROPERTY(L"Data:dx", L"{-1, 0, 1}")
TEST_METHOD_PROPERTY(L"Data:dy", L"{-10, -1, 0, 1, 10}")
TEST_METHOD_PROPERTY(L"Data:printedRows", L"{1, 10, 50, 200}")
END_TEST_METHOD_PROPERTIES()
int dx, dy;
int printedRows;
VERIFY_SUCCEEDED(TestData::TryGetValue(L"dx", dx), L"change in width of buffer");
VERIFY_SUCCEEDED(TestData::TryGetValue(L"dy", dy), L"change in height of buffer");
VERIFY_SUCCEEDED(TestData::TryGetValue(L"printedRows", printedRows), L"Number of rows of text to print");

_checkConptyOutput = false;

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.get();
const auto initialHostView = si.GetViewport();
const auto initialTermView = term->GetViewport();
const auto initialTerminalBufferHeight = term->GetTextBuffer().GetSize().Height();

VERIFY_ARE_EQUAL(0, initialHostView.Top());
VERIFY_ARE_EQUAL(TerminalViewHeight, initialHostView.BottomExclusive());
VERIFY_ARE_EQUAL(0, initialTermView.Top());
VERIFY_ARE_EQUAL(TerminalViewHeight, initialTermView.BottomExclusive());

Log::Comment(NoThrowString().Format(
L"Print %d lines of output, which will scroll the viewport", printedRows));

for (auto i = 0; i < printedRows; i++)
{
// This looks insane, but this expression is carefully crafted to give
// us only printable characters, starting with `!` (0n33).
// Similar statements are used elsewhere throughout this test.
auto wstr = std::wstring(1, static_cast<wchar_t>((i) % 93) + 33);
hostSm.ProcessString(wstr);
hostSm.ProcessString(L"\r\n");
}

// Conpty doesn't have a scrollback, it's view's origin is always 0,0
const auto secondHostView = si.GetViewport();
VERIFY_ARE_EQUAL(0, secondHostView.Top());
VERIFY_ARE_EQUAL(TerminalViewHeight, secondHostView.BottomExclusive());

VERIFY_SUCCEEDED(renderer.PaintFrame());

const auto secondTermView = term->GetViewport();
// If we've printed more lines than the height of the buffer, then we're
// expecting the viewport to have moved down. Otherwise, the terminal's
// viewport will stay at 0,0.
const auto expectedTerminalViewBottom = std::max(std::min(gsl::narrow_cast<short>(printedRows + 1),
term->GetBufferHeight()),
term->GetViewport().Height());

VERIFY_ARE_EQUAL(expectedTerminalViewBottom, secondTermView.BottomExclusive());
VERIFY_ARE_EQUAL(expectedTerminalViewBottom - initialTermView.Height(), secondTermView.Top());

auto verifyTermData = [&expectedTerminalViewBottom, &printedRows, this, &initialTerminalBufferHeight](TextBuffer& termTb, const int resizeDy = 0) {
// Some number of lines of text were lost from the scrollback. The
// number of lines lost will be determined by whichever of the initial
// or current buffer is smaller.
const auto numLostRows = std::max(0,
printedRows - std::min(term->GetTextBuffer().GetSize().Height(), initialTerminalBufferHeight) + 1);

const auto rowsWithText = std::min(gsl::narrow_cast<short>(printedRows),
expectedTerminalViewBottom) -
1 + std::min(resizeDy, 0);

for (short row = 0; row < rowsWithText; row++)
{
SetVerifyOutput settings(VerifyOutputSettings::LogOnlyFailures);
auto iter = termTb.GetCellDataAt({ 0, row });
const wchar_t expectedChar = static_cast<wchar_t>((row + numLostRows) % 93) + 33;

auto expectedString = std::wstring(1, expectedChar);

if (iter->Chars() != expectedString)
{
Log::Comment(NoThrowString().Format(L"row [%d] was mismatched", row));
}
VERIFY_ARE_EQUAL(expectedString, (iter++)->Chars());
VERIFY_ARE_EQUAL(L" ", (iter)->Chars());
}
};
auto verifyHostData = [&si, &initialHostView, &printedRows](TextBuffer& hostTb, const int resizeDy = 0) {
const auto hostView = si.GetViewport();

// In the host, there are two regions we're interested in:

// 1. the first section of the buffer with the output in it. Before
// we're resized, this will be filled with one character on each row.
// 2. The second area below the first that's empty (filled with spaces).
// Initially, this is only one row.
// After we resize, different things will happen.
// * If we decrease the height of the buffer, the characters in the
// buffer will all move _up_ the same number of rows. We'll want to
// only check the first initialView+dy rows for characters.
// * If we increase the height, rows will be added at the bottom. We'll
// want to check the initial viewport height for the original
// characters, but then we'll want to look for more blank rows at the
// bottom. The characters in the initial viewport won't have moved.

const short originalViewHeight = gsl::narrow_cast<short>(resizeDy < 0 ?
initialHostView.Height() + resizeDy :
initialHostView.Height());
const auto rowsWithText = std::min(originalViewHeight - 1, printedRows);
const bool scrolled = printedRows > initialHostView.Height();
// The last row of the viewport should be empty
// The second last row will have '0'+50
// The third last row will have '0'+49
// ...
// The <height> last row will have '0'+(50-height+1)
const auto firstChar = static_cast<wchar_t>(scrolled ?
(printedRows - originalViewHeight + 1) :
0);

short row = 0;
// Don't include the last row of the viewport in this check, since it'll
// be blank. We'll check it in the below loop.
for (; row < rowsWithText; row++)
{
SetVerifyOutput settings(VerifyOutputSettings::LogOnlyFailures);
auto iter = hostTb.GetCellDataAt({ 0, row });

const auto expectedChar = static_cast<wchar_t>(((firstChar + row) % 93) + 33);
auto expectedString = std::wstring(1, static_cast<wchar_t>(expectedChar));

if (iter->Chars() != expectedString)
{
Log::Comment(NoThrowString().Format(L"row [%d] was mismatched", row));
}
VERIFY_ARE_EQUAL(expectedString, (iter++)->Chars(), NoThrowString().Format(L"%s", expectedString.data()));
VERIFY_ARE_EQUAL(L" ", (iter)->Chars());
}

// Check that the remaining rows in the viewport are empty.
for (; row < hostView.Height(); row++)
{
SetVerifyOutput settings(VerifyOutputSettings::LogOnlyFailures);
auto iter = hostTb.GetCellDataAt({ 0, row });
VERIFY_ARE_EQUAL(L" ", (iter)->Chars());
}
};

verifyHostData(*hostTb);
verifyTermData(*termTb);

const COORD newViewportSize{
gsl::narrow_cast<short>(TerminalViewWidth + dx),
gsl::narrow_cast<short>(TerminalViewHeight + dy)
};

Log::Comment(NoThrowString().Format(L"Resize the Terminal and conpty here"));
auto resizeResult = term->UserResize(newViewportSize);
VERIFY_SUCCEEDED(resizeResult);
_resizeConpty(newViewportSize.X, newViewportSize.Y);

// After we resize, make sure to get the new textBuffers
hostTb = &si.GetTextBuffer();
termTb = term->_buffer.get();

// Conpty's doesn't have a scrollback, it's view's origin is always 0,0
const auto thirdHostView = si.GetViewport();
VERIFY_ARE_EQUAL(0, thirdHostView.Top());
VERIFY_ARE_EQUAL(newViewportSize.Y, thirdHostView.BottomExclusive());

// The Terminal should be stuck to the top of the viewport.
const auto thirdTermView = term->GetViewport();

// TODO: For dy<0, rows=50, this is not true. In that set of cases, we
// _didn't_ pin the top of the Terminal to the old top, we actually shifted
// it down (because the output was at the bottom of the window, not empty
// lines).
// TODO: VERIFY_ARE_EQUAL(secondTermView.Top(), thirdTermView.Top());
// TODO: VERIFY_ARE_EQUAL(expectedTerminalViewBottom + dy, thirdTermView.BottomExclusive());

verifyHostData(*hostTb, dy);
// Note that at this point, nothing should have changed with the Terminal.
verifyTermData(*termTb, dy);

Log::Comment(NoThrowString().Format(L"Paint a frame to update the Terminal"));
VERIFY_SUCCEEDED(renderer.PaintFrame());

// Conpty's doesn't have a scrollback, it's view's origin is always 0,0
const auto fourthHostView = si.GetViewport();
VERIFY_ARE_EQUAL(0, fourthHostView.Top());
VERIFY_ARE_EQUAL(newViewportSize.Y, fourthHostView.BottomExclusive());

// The Terminal should be stuck to the top of the viewport.
const auto fourthTermView = term->GetViewport();

// TODO: For dy<0, rows=50, this is not true. In that set of cases, we
// _didn't_ pin the top of the Terminal to the old top, we actually shifted
// it down (because the output was at the bottom of the window, not empty
// lines).
// TODO: VERIFY_ARE_EQUAL(secondTermView.Top(), fourthTermView.Top());
// TODO: VERIFY_ARE_EQUAL(expectedTerminalViewBottom + dy, fourthTermView.BottomExclusive());

verifyHostData(*hostTb, dy);
verifyTermData(*termTb, dy);
}

0 comments on commit edea9a3

Please sign in to comment.