From b5c8c854cc2d0000a09672112dc20e6a98639713 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Mon, 13 Jan 2020 11:23:42 -0600 Subject: [PATCH 01/39] let's first move reflowing to the text buffer --- src/buffer/out/textBuffer.cpp | 212 ++++++++++++++++++++++++++++++++++ src/buffer/out/textBuffer.hpp | 2 + src/host/screenInfo.cpp | 211 +-------------------------------- 3 files changed, 218 insertions(+), 207 deletions(-) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index a4eaa73140d..c5829dc16f3 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -1541,3 +1541,215 @@ std::string TextBuffer::GenRTF(const TextAndColor& rows, const int fontHeightPoi return {}; } } + +HRESULT TextBuffer::ReflowBuffer(TextBuffer& oldBuffer, TextBuffer& newBuffer) +{ + Cursor& oldCursor = oldBuffer.GetCursor(); + Cursor& newCursor = newBuffer.GetCursor(); + // skip any drawing updates that might occur as we manipulate the new buffer + oldCursor.StartDeferDrawing(); + newCursor.StartDeferDrawing(); + + // We need to save the old cursor position so that we can + // place the new cursor back on the equivalent character in + // the new buffer. + COORD cOldCursorPos = oldCursor.GetPosition(); + COORD cOldLastChar = oldBuffer.GetLastNonSpaceCharacter(); + + short const cOldRowsTotal = cOldLastChar.Y + 1; + short const cOldColsTotal = oldBuffer.GetSize().Width(); + + COORD cNewCursorPos = { 0 }; + bool fFoundCursorPos = false; + + HRESULT hr = S_OK; + // Loop through all the rows of the old buffer and reprint them into the new buffer + for (short iOldRow = 0; iOldRow < cOldRowsTotal; iOldRow++) + { + // Fetch the row and its "right" which is the last printable character. + const ROW& row = oldBuffer.GetRowByOffset(iOldRow); + const CharRow& charRow = row.GetCharRow(); + short iRight = static_cast(charRow.MeasureRight()); + + // There is a special case here. If the row has a "wrap" + // flag on it, but the right isn't equal to the width (one + // index past the final valid index in the row) then there + // were a bunch trailing of spaces in the row. + // (But the measuring functions for each row Left/Right do + // not count spaces as "displayable" so they're not + // included.) + // As such, adjust the "right" to be the width of the row + // to capture all these spaces + if (charRow.WasWrapForced()) + { + iRight = cOldColsTotal; + + // And a combined special case. + // If we wrapped off the end of the row by adding a + // piece of padding because of a double byte LEADING + // character, then remove one from the "right" to + // leave this padding out of the copy process. + if (charRow.WasDoubleBytePadded()) + { + iRight--; + } + } + + // Loop through every character in the current row (up to + // the "right" boundary, which is one past the final valid + // character) + for (short iOldCol = 0; iOldCol < iRight; iOldCol++) + { + if (iOldCol == cOldCursorPos.X && iOldRow == cOldCursorPos.Y) + { + cNewCursorPos = newCursor.GetPosition(); + fFoundCursorPos = true; + } + + try + { + // TODO: MSFT: 19446208 - this should just use an iterator and the inserter... + const auto glyph = row.GetCharRow().GlyphAt(iOldCol); + const auto dbcsAttr = row.GetCharRow().DbcsAttrAt(iOldCol); + const auto textAttr = row.GetAttrRow().GetAttrByColumn(iOldCol); + + if (!newBuffer.InsertCharacter(glyph, dbcsAttr, textAttr)) + { + hr = E_OUTOFMEMORY; + break; + } + } + CATCH_RETURN(); + } + if (SUCCEEDED(hr)) + { + // If we didn't have a full row to copy, insert a new + // line into the new buffer. + // Only do so if we were not forced to wrap. If we did + // force a word wrap, then the existing line break was + // only because we ran out of space. + if (iRight < cOldColsTotal && !charRow.WasWrapForced()) + { + if (iRight == cOldCursorPos.X && iOldRow == cOldCursorPos.Y) + { + cNewCursorPos = newCursor.GetPosition(); + fFoundCursorPos = true; + } + // Only do this if it's not the final line in the buffer. + // On the final line, we want the cursor to sit + // where it is done printing for the cursor + // adjustment to follow. + if (iOldRow < cOldRowsTotal - 1) + { + hr = newBuffer.NewlineCursor() ? hr : E_OUTOFMEMORY; + } + else + { + // If we are on the final line of the buffer, we have one more check. + // We got into this code path because we are at the right most column of a row in the old buffer + // that had a hard return (no wrap was forced). + // However, as we're inserting, the old row might have just barely fit into the new buffer and + // caused a new soft return (wrap was forced) putting the cursor at x=0 on the line just below. + // We need to preserve the memory of the hard return at this point by inserting one additional + // hard newline, otherwise we've lost that information. + // We only do this when the cursor has just barely poured over onto the next line so the hard return + // isn't covered by the soft one. + // e.g. + // The old line was: + // |aaaaaaaaaaaaaaaaaaa | with no wrap which means there was a newline after that final a. + // The cursor was here ^ + // And the new line will be: + // |aaaaaaaaaaaaaaaaaaa| and show a wrap at the end + // | | + // ^ and the cursor is now there. + // If we leave it like this, we've lost the newline information. + // So we insert one more newline so a continued reflow of this buffer by resizing larger will + // continue to look as the original output intended with the newline data. + // After this fix, it looks like this: + // |aaaaaaaaaaaaaaaaaaa| no wrap at the end (preserved hard newline) + // | | + // ^ and the cursor is now here. + const COORD coordNewCursor = newCursor.GetPosition(); + if (coordNewCursor.X == 0 && coordNewCursor.Y > 0) + { + if (newBuffer.GetRowByOffset(coordNewCursor.Y - 1).GetCharRow().WasWrapForced()) + { + hr = newBuffer.NewlineCursor() ? hr : E_OUTOFMEMORY; + } + } + } + } + } + } + if (SUCCEEDED(hr)) + { + // Finish copying remaining parameters from the old text buffer to the new one + newBuffer.CopyProperties(oldBuffer); + + // If we found where to put the cursor while placing characters into the buffer, + // just put the cursor there. Otherwise we have to advance manually. + if (fFoundCursorPos) + { + newCursor.SetPosition(cNewCursorPos); + } + else + { + // Advance the cursor to the same offset as before + // get the number of newlines and spaces between the old end of text and the old cursor, + // then advance that many newlines and chars + int iNewlines = cOldCursorPos.Y - cOldLastChar.Y; + int iIncrements = cOldCursorPos.X - cOldLastChar.X; + const COORD cNewLastChar = newBuffer.GetLastNonSpaceCharacter(); + + // If the last row of the new buffer wrapped, there's going to be one less newline needed, + // because the cursor is already on the next line + if (newBuffer.GetRowByOffset(cNewLastChar.Y).GetCharRow().WasWrapForced()) + { + iNewlines = std::max(iNewlines - 1, 0); + } + else + { + // if this buffer didn't wrap, but the old one DID, then the d(columns) of the + // old buffer will be one more than in this buffer, so new need one LESS. + if (oldBuffer.GetRowByOffset(cOldLastChar.Y).GetCharRow().WasWrapForced()) + { + iNewlines = std::max(iNewlines - 1, 0); + } + } + + for (int r = 0; r < iNewlines; r++) + { + if (!newBuffer.NewlineCursor()) + { + hr = E_OUTOFMEMORY; + break; + } + } + if (SUCCEEDED(hr)) + { + for (int c = 0; c < iIncrements - 1; c++) + { + if (!newBuffer.IncrementCursor()) + { + hr = E_OUTOFMEMORY; + break; + } + } + } + } + } + + if (SUCCEEDED(hr)) + { + // Save old cursor size before we delete it + ULONG const ulSize = oldCursor.GetSize(); + + // Set size back to real size as it will be taking over the rendering duties. + newCursor.SetSize(ulSize); + } + + newCursor.EndDeferDrawing(); + oldCursor.EndDeferDrawing(); + + return hr; +} diff --git a/src/buffer/out/textBuffer.hpp b/src/buffer/out/textBuffer.hpp index f77f879a82b..f8b23ac1c24 100644 --- a/src/buffer/out/textBuffer.hpp +++ b/src/buffer/out/textBuffer.hpp @@ -158,6 +158,8 @@ class TextBuffer final const std::wstring_view fontFaceName, const COLORREF backgroundColor); + static HRESULT ReflowBuffer(TextBuffer& oldBuffer, TextBuffer& newBuffer); + private: std::deque _storage; Cursor _cursor; diff --git a/src/host/screenInfo.cpp b/src/host/screenInfo.cpp index f6d7e4407af..34e5c522b4c 100644 --- a/src/host/screenInfo.cpp +++ b/src/host/screenInfo.cpp @@ -1425,224 +1425,21 @@ bool SCREEN_INFORMATION::IsMaximizedY() const // Save cursor's relative height versus the viewport SHORT const sCursorHeightInViewportBefore = _textBuffer->GetCursor().GetPosition().Y - _viewport.Top(); - Cursor& oldCursor = _textBuffer->GetCursor(); - Cursor& newCursor = newTextBuffer->GetCursor(); - // skip any drawing updates that might occur as we manipulate the new buffer - oldCursor.StartDeferDrawing(); - newCursor.StartDeferDrawing(); + HRESULT hr = TextBuffer::ReflowBuffer(*_textBuffer.get(), *newTextBuffer.get()); - // We need to save the old cursor position so that we can - // place the new cursor back on the equivalent character in - // the new buffer. - COORD cOldCursorPos = oldCursor.GetPosition(); - COORD cOldLastChar = _textBuffer->GetLastNonSpaceCharacter(); - - short const cOldRowsTotal = cOldLastChar.Y + 1; - short const cOldColsTotal = GetBufferSize().Width(); - - COORD cNewCursorPos = { 0 }; - bool fFoundCursorPos = false; - - NTSTATUS status = STATUS_SUCCESS; - // Loop through all the rows of the old buffer and reprint them into the new buffer - for (short iOldRow = 0; iOldRow < cOldRowsTotal; iOldRow++) - { - // Fetch the row and its "right" which is the last printable character. - const ROW& Row = _textBuffer->GetRowByOffset(iOldRow); - const CharRow& charRow = Row.GetCharRow(); - short iRight = static_cast(charRow.MeasureRight()); - - // There is a special case here. If the row has a "wrap" - // flag on it, but the right isn't equal to the width (one - // index past the final valid index in the row) then there - // were a bunch trailing of spaces in the row. - // (But the measuring functions for each row Left/Right do - // not count spaces as "displayable" so they're not - // included.) - // As such, adjust the "right" to be the width of the row - // to capture all these spaces - if (charRow.WasWrapForced()) - { - iRight = cOldColsTotal; - - // And a combined special case. - // If we wrapped off the end of the row by adding a - // piece of padding because of a double byte LEADING - // character, then remove one from the "right" to - // leave this padding out of the copy process. - if (charRow.WasDoubleBytePadded()) - { - iRight--; - } - } - - // Loop through every character in the current row (up to - // the "right" boundary, which is one past the final valid - // character) - for (short iOldCol = 0; iOldCol < iRight; iOldCol++) - { - if (iOldCol == cOldCursorPos.X && iOldRow == cOldCursorPos.Y) - { - cNewCursorPos = newCursor.GetPosition(); - fFoundCursorPos = true; - } - - try - { - // TODO: MSFT: 19446208 - this should just use an iterator and the inserter... - const auto glyph = Row.GetCharRow().GlyphAt(iOldCol); - const auto dbcsAttr = Row.GetCharRow().DbcsAttrAt(iOldCol); - const auto textAttr = Row.GetAttrRow().GetAttrByColumn(iOldCol); - - if (!newTextBuffer->InsertCharacter(glyph, dbcsAttr, textAttr)) - { - status = STATUS_NO_MEMORY; - break; - } - } - catch (...) - { - return NTSTATUS_FROM_HRESULT(wil::ResultFromCaughtException()); - } - } - if (NT_SUCCESS(status)) - { - // If we didn't have a full row to copy, insert a new - // line into the new buffer. - // Only do so if we were not forced to wrap. If we did - // force a word wrap, then the existing line break was - // only because we ran out of space. - if (iRight < cOldColsTotal && !charRow.WasWrapForced()) - { - if (iRight == cOldCursorPos.X && iOldRow == cOldCursorPos.Y) - { - cNewCursorPos = newCursor.GetPosition(); - fFoundCursorPos = true; - } - // Only do this if it's not the final line in the buffer. - // On the final line, we want the cursor to sit - // where it is done printing for the cursor - // adjustment to follow. - if (iOldRow < cOldRowsTotal - 1) - { - status = newTextBuffer->NewlineCursor() ? status : STATUS_NO_MEMORY; - } - else - { - // If we are on the final line of the buffer, we have one more check. - // We got into this code path because we are at the right most column of a row in the old buffer - // that had a hard return (no wrap was forced). - // However, as we're inserting, the old row might have just barely fit into the new buffer and - // caused a new soft return (wrap was forced) putting the cursor at x=0 on the line just below. - // We need to preserve the memory of the hard return at this point by inserting one additional - // hard newline, otherwise we've lost that information. - // We only do this when the cursor has just barely poured over onto the next line so the hard return - // isn't covered by the soft one. - // e.g. - // The old line was: - // |aaaaaaaaaaaaaaaaaaa | with no wrap which means there was a newline after that final a. - // The cursor was here ^ - // And the new line will be: - // |aaaaaaaaaaaaaaaaaaa| and show a wrap at the end - // | | - // ^ and the cursor is now there. - // If we leave it like this, we've lost the newline information. - // So we insert one more newline so a continued reflow of this buffer by resizing larger will - // continue to look as the original output intended with the newline data. - // After this fix, it looks like this: - // |aaaaaaaaaaaaaaaaaaa| no wrap at the end (preserved hard newline) - // | | - // ^ and the cursor is now here. - const COORD coordNewCursor = newCursor.GetPosition(); - if (coordNewCursor.X == 0 && coordNewCursor.Y > 0) - { - if (newTextBuffer->GetRowByOffset(coordNewCursor.Y - 1).GetCharRow().WasWrapForced()) - { - status = newTextBuffer->NewlineCursor() ? status : STATUS_NO_MEMORY; - } - } - } - } - } - } - if (NT_SUCCESS(status)) - { - // Finish copying remaining parameters from the old text buffer to the new one - newTextBuffer->CopyProperties(*_textBuffer); - - // If we found where to put the cursor while placing characters into the buffer, - // just put the cursor there. Otherwise we have to advance manually. - if (fFoundCursorPos) - { - newCursor.SetPosition(cNewCursorPos); - } - else - { - // Advance the cursor to the same offset as before - // get the number of newlines and spaces between the old end of text and the old cursor, - // then advance that many newlines and chars - int iNewlines = cOldCursorPos.Y - cOldLastChar.Y; - int iIncrements = cOldCursorPos.X - cOldLastChar.X; - const COORD cNewLastChar = newTextBuffer->GetLastNonSpaceCharacter(); - - // If the last row of the new buffer wrapped, there's going to be one less newline needed, - // because the cursor is already on the next line - if (newTextBuffer->GetRowByOffset(cNewLastChar.Y).GetCharRow().WasWrapForced()) - { - iNewlines = std::max(iNewlines - 1, 0); - } - else - { - // if this buffer didn't wrap, but the old one DID, then the d(columns) of the - // old buffer will be one more than in this buffer, so new need one LESS. - if (_textBuffer->GetRowByOffset(cOldLastChar.Y).GetCharRow().WasWrapForced()) - { - iNewlines = std::max(iNewlines - 1, 0); - } - } - - for (int r = 0; r < iNewlines; r++) - { - if (!newTextBuffer->NewlineCursor()) - { - status = STATUS_NO_MEMORY; - break; - } - } - if (NT_SUCCESS(status)) - { - for (int c = 0; c < iIncrements - 1; c++) - { - if (!newTextBuffer->IncrementCursor()) - { - status = STATUS_NO_MEMORY; - break; - } - } - } - } - } - - if (NT_SUCCESS(status)) + if (SUCCEEDED(hr)) { + Cursor& newCursor = newTextBuffer->GetCursor(); // Adjust the viewport so the cursor doesn't wildly fly off up or down. SHORT const sCursorHeightInViewportAfter = newCursor.GetPosition().Y - _viewport.Top(); COORD coordCursorHeightDiff = { 0 }; coordCursorHeightDiff.Y = sCursorHeightInViewportAfter - sCursorHeightInViewportBefore; LOG_IF_FAILED(SetViewportOrigin(false, coordCursorHeightDiff, true)); - // Save old cursor size before we delete it - ULONG const ulSize = oldCursor.GetSize(); - _textBuffer.swap(newTextBuffer); - - // Set size back to real size as it will be taking over the rendering duties. - newCursor.SetSize(ulSize); - newCursor.EndDeferDrawing(); } - oldCursor.EndDeferDrawing(); - return status; + return NTSTATUS_FROM_HRESULT(hr); } // From 9aec69467cf72c536fca001abc4e82222012307e Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Mon, 13 Jan 2020 11:34:50 -0600 Subject: [PATCH 02/39] add a doc comment because I'm not a barbarian --- src/buffer/out/textBuffer.cpp | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index c5829dc16f3..cce094ceea0 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -1542,6 +1542,16 @@ std::string TextBuffer::GenRTF(const TextAndColor& rows, const int fontHeightPoi } } +// Function Description: +// - Reflow the contents from the old buffer into the new buffer. The new buffer +// can have different dimensions than the old buffer. If it does, then this +// function will attempt to maintain the logical contents of the old buffer, +// by continuing wrapped lines onto the next line in the new buffer. +// Arguments: +// - oldBuffer - the text buffer to copy the contents FROM +// - newBuffer - the text buffer to copy the contents TO +// Return Value: +// - S_OK if we successfully copied the contents to the new buffer, otherwise an appropriate HRESULT. HRESULT TextBuffer::ReflowBuffer(TextBuffer& oldBuffer, TextBuffer& newBuffer) { Cursor& oldCursor = oldBuffer.GetCursor(); From 1a2654d291e530a189c376702d6f47ecc05c84bc Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Mon, 13 Jan 2020 17:07:43 -0600 Subject: [PATCH 03/39] Try to wrap the line properly with conpty This confusingly doesn't always work --- src/renderer/vt/XtermEngine.cpp | 13 +++++++++++-- src/renderer/vt/paint.cpp | 15 +++++++++++++++ src/renderer/vt/vtrenderer.hpp | 2 ++ 3 files changed, 28 insertions(+), 2 deletions(-) diff --git a/src/renderer/vt/XtermEngine.cpp b/src/renderer/vt/XtermEngine.cpp index 2c1d8a08ed1..556381c5661 100644 --- a/src/renderer/vt/XtermEngine.cpp +++ b/src/renderer/vt/XtermEngine.cpp @@ -19,7 +19,7 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, _ColorTable(ColorTable), _cColorTable(cColorTable), _fUseAsciiOnly(fUseAsciiOnly), - _previousLineWrapped(false), + // _previousLineWrapped(false), _usingUnderLine(false), _needToDisableCursor(false), _lastCursorIsVisible(false), @@ -248,7 +248,13 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, // If the previous line wrapped, then the cursor is already at this // position, we just don't know it yet. Don't emit anything. - if (_previousLineWrapped) + bool previousLineWrapped = false; + if (_wrappedRow.has_value()) + { + previousLineWrapped = coord.Y == _wrappedRow.value() + 1; + } + + if (previousLineWrapped) { hr = S_OK; } @@ -298,6 +304,9 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, _newBottomLine = false; } _deferredCursorPos = INVALID_COORDS; + + _wrappedRow = std::nullopt; + return hr; } diff --git a/src/renderer/vt/paint.cpp b/src/renderer/vt/paint.cpp index 920dc86aeb2..02b35895968 100644 --- a/src/renderer/vt/paint.cpp +++ b/src/renderer/vt/paint.cpp @@ -449,6 +449,21 @@ 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 + // 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 is 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. + + // TODO: This seems to be off by one char. Resizing cmd.exe, the '.' at the + // end of the initial message sometimes gets cut off weirdly. + if ((_lastText.X + (totalWidth - numSpaces)) > _lastViewport.RightInclusive()) + { + _wrappedRow = coord.Y; + } + // Update our internal tracker of the cursor's position. // See MSFT:20266233 // If the cursor is at the rightmost column of the terminal, and we write a diff --git a/src/renderer/vt/vtrenderer.hpp b/src/renderer/vt/vtrenderer.hpp index 5adf1cd1d9c..3b180e5e7c7 100644 --- a/src/renderer/vt/vtrenderer.hpp +++ b/src/renderer/vt/vtrenderer.hpp @@ -136,6 +136,8 @@ namespace Microsoft::Console::Render Microsoft::Console::VirtualTerminal::RenderTracing _trace; bool _inResizeRequest{ false }; + std::optional _wrappedRow{ std::nullopt }; + [[nodiscard]] HRESULT _Write(std::string_view const str) noexcept; [[nodiscard]] HRESULT _WriteFormattedString(const std::string* const pFormat, ...) noexcept; [[nodiscard]] HRESULT _Flush() noexcept; From aae6ce60a491036f5917042efe6a85fee76ea67a Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Tue, 14 Jan 2020 16:07:22 -0600 Subject: [PATCH 04/39] This works, fixing the ECH at end of wrapped line bug The trick was that when a line that was exactly filled was followed by an empty line, we'd ECH when the cursor is virtually on the last char of the wrapped line. That was wrong. For a scenario like: |ABCDEF| | | We'd incorrectly render that as ["ABCDEF", "^[[K", "\r\n"]. That ECH in the middle there would erase the 'F', because the cursor was virtually still on the 'F' (because it had deferred wrapping to the next char). So, when we're about to ECH following a wrapped line, reset the _wrappedRow first, to make sure we correctly \r\n first. --- src/renderer/vt/XtermEngine.cpp | 3 +++ src/renderer/vt/paint.cpp | 14 +++++++++++++- src/renderer/vt/tracing.cpp | 27 +++++++++++++++++++++++++++ src/renderer/vt/tracing.hpp | 2 ++ 4 files changed, 45 insertions(+), 1 deletion(-) diff --git a/src/renderer/vt/XtermEngine.cpp b/src/renderer/vt/XtermEngine.cpp index 556381c5661..f652ceb3d52 100644 --- a/src/renderer/vt/XtermEngine.cpp +++ b/src/renderer/vt/XtermEngine.cpp @@ -235,6 +235,8 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, { HRESULT hr = S_OK; + _trace.TraceMoveCursor(coord); + if (coord.X != _lastText.X || coord.Y != _lastText.Y) { if (coord.X == 0 && coord.Y == 0) @@ -256,6 +258,7 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, if (previousLineWrapped) { + _trace.TraceWrapped(); hr = S_OK; } else diff --git a/src/renderer/vt/paint.cpp b/src/renderer/vt/paint.cpp index 02b35895968..6edf254bd96 100644 --- a/src/renderer/vt/paint.cpp +++ b/src/renderer/vt/paint.cpp @@ -376,7 +376,9 @@ using namespace Microsoft::Console::Types; return S_OK; } - RETURN_IF_FAILED(_MoveCursor(coord)); + // auto lastPaintWrapped = _wrappedRow; + + // RETURN_IF_FAILED(_MoveCursor(coord)); std::wstring unclusteredString; unclusteredString.reserve(clusters.size()); @@ -445,6 +447,16 @@ using namespace Microsoft::Console::Types; (totalWidth - numSpaces) : totalWidth; + if (removeSpaces && _wrappedRow.has_value() /* && columnsActual == 0*/) + { + // If the previous row _exactly_ filled the line, and we're about to + // clear _the entire line_, then we actually do want to move the + // cursor down. + // RETURN_IF_FAILED(_MoveCursor(coord)); + _wrappedRow = std::nullopt; + } + RETURN_IF_FAILED(_MoveCursor(coord)); + // Write the actual text string std::wstring wstr = std::wstring(unclusteredString.data(), cchActual); RETURN_IF_FAILED(VtEngine::_WriteTerminalUtf8(wstr)); diff --git a/src/renderer/vt/tracing.cpp b/src/renderer/vt/tracing.cpp index 3c5f4e0aa63..04c8d54c8b5 100644 --- a/src/renderer/vt/tracing.cpp +++ b/src/renderer/vt/tracing.cpp @@ -210,3 +210,30 @@ void RenderTracing::TraceLastText(const COORD lastTextPos) const UNREFERENCED_PARAMETER(lastTextPos); #endif UNIT_TESTING } + +void RenderTracing::TraceMoveCursor(const COORD pos) const +{ +#ifndef UNIT_TESTING + const auto lastTextStr = _CoordToString(pos); + const auto cursor = lastTextStr.c_str(); + TraceLoggingWrite(g_hConsoleVtRendererTraceProvider, + "VtEngine_TraceMoveCursor", + TraceLoggingString(cursor), + TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE)); +#else + UNREFERENCED_PARAMETER(pos); +#endif UNIT_TESTING +} + +void RenderTracing::TraceWrapped() const +{ +#ifndef UNIT_TESTING + const auto* const msg = "Wrapped instead of \\r\\n"; + TraceLoggingWrite(g_hConsoleVtRendererTraceProvider, + "VtEngine_TraceWrapped", + TraceLoggingString(msg), + TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE)); +#else + UNREFERENCED_PARAMETER(pos); +#endif UNIT_TESTING +} diff --git a/src/renderer/vt/tracing.hpp b/src/renderer/vt/tracing.hpp index 8d923adc813..ef00bea81be 100644 --- a/src/renderer/vt/tracing.hpp +++ b/src/renderer/vt/tracing.hpp @@ -29,6 +29,8 @@ namespace Microsoft::Console::VirtualTerminal void TraceString(const std::string_view& str) const; void TraceInvalidate(const Microsoft::Console::Types::Viewport view) const; void TraceLastText(const COORD lastText) const; + void TraceMoveCursor(const COORD pos) const; + void TraceWrapped() const; void TraceInvalidateAll(const Microsoft::Console::Types::Viewport view) const; void TraceTriggerCircling(const bool newFrame) const; void TraceStartPaint(const bool quickReturn, From 416be4656febb708193ed7ecc36914c6e98c1333 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Tue, 14 Jan 2020 16:46:33 -0600 Subject: [PATCH 05/39] This is some cleanup, almost ready for PR but I need to write tests which are blocked on #4213 --- src/renderer/vt/paint.cpp | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/src/renderer/vt/paint.cpp b/src/renderer/vt/paint.cpp index 6edf254bd96..bda79f6197a 100644 --- a/src/renderer/vt/paint.cpp +++ b/src/renderer/vt/paint.cpp @@ -376,10 +376,6 @@ using namespace Microsoft::Console::Types; return S_OK; } - // auto lastPaintWrapped = _wrappedRow; - - // RETURN_IF_FAILED(_MoveCursor(coord)); - std::wstring unclusteredString; unclusteredString.reserve(clusters.size()); short totalWidth = 0; @@ -447,14 +443,19 @@ using namespace Microsoft::Console::Types; (totalWidth - numSpaces) : totalWidth; - if (removeSpaces && _wrappedRow.has_value() /* && columnsActual == 0*/) + if (cchActual == 0) { - // If the previous row _exactly_ filled the line, and we're about to - // clear _the entire line_, then we actually do want to move the - // cursor down. - // RETURN_IF_FAILED(_MoveCursor(coord)); + // If the previous row wrapped, but this line is empty, then we actually + // do want to move the cursor down. Otherwise, we'll possibly end up + // accidentally erasing the last character from the previous line, as + // the cursor is still waiting on that character for the next character + // to follow it. _wrappedRow = std::nullopt; + // TODO:: Write a test that emulates ~/vttests/reflow-120.py + // TODO:: Write a test that emulates ~/vttests/reflow-advanced.py } + + // Move the cursor to the start of this run. RETURN_IF_FAILED(_MoveCursor(coord)); // Write the actual text string @@ -468,9 +469,6 @@ using namespace Microsoft::Console::Types; // line. // Don't do this is 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. - - // TODO: This seems to be off by one char. Resizing cmd.exe, the '.' at the - // end of the initial message sometimes gets cut off weirdly. if ((_lastText.X + (totalWidth - numSpaces)) > _lastViewport.RightInclusive()) { _wrappedRow = coord.Y; From edeb3463253d29d8edc72ad591d11e790eea7ab8 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Mon, 27 Jan 2020 17:15:38 -0600 Subject: [PATCH 06/39] I think this is all I need to support wrapped lines in the Terminal (cherry picked from commit c21f74029d1186bec69846258ca7bdcb82346964) --- src/cascadia/TerminalCore/Terminal.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index f71ffb76472..e79ba5b0238 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -483,6 +483,13 @@ 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. + // TODO/DANGER: Does this actually work? The above comment seems + // to mention the i-=1, but there was no such line here. This is + // _scary_. Perhaps this was never hit before we had conpty + // wrapping? + i--; } } From ce3138c6853840e6e1bf4f13c374501cbf26a352 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Tue, 28 Jan 2020 09:24:04 -0600 Subject: [PATCH 07/39] Let's pull all the test fixes into their own file --- src/cascadia/TerminalCore/Terminal.hpp | 9 +- .../ConptyRoundtripTests.cpp | 148 ++++++++---------- .../TerminalBufferTests.cpp | 68 ++++++++ .../UnitTests_TerminalCore/TestUtils.h | 94 +++++++++++ .../UnitTests_TerminalCore/UnitTests.vcxproj | 1 + src/host/ConsoleArguments.cpp | 14 ++ src/host/ConsoleArguments.hpp | 4 + src/host/VtIo.cpp | 15 ++ src/host/VtIo.hpp | 4 + src/host/globals.cpp | 16 ++ src/host/globals.h | 4 + src/host/screenInfo.hpp | 10 +- src/host/ut_host/ConptyOutputTests.cpp | 5 + src/inc/test/CommonState.hpp | 21 ++- src/renderer/vt/vtrenderer.hpp | 7 +- 15 files changed, 321 insertions(+), 99 deletions(-) create mode 100644 src/cascadia/UnitTests_TerminalCore/TerminalBufferTests.cpp create mode 100644 src/cascadia/UnitTests_TerminalCore/TestUtils.h diff --git a/src/cascadia/TerminalCore/Terminal.hpp b/src/cascadia/TerminalCore/Terminal.hpp index 0980b51393a..bf487622313 100644 --- a/src/cascadia/TerminalCore/Terminal.hpp +++ b/src/cascadia/TerminalCore/Terminal.hpp @@ -30,7 +30,11 @@ namespace Microsoft::Terminal::Core // fwdecl unittest classes #ifdef UNIT_TESTING -class ConptyRoundtripTests; +namespace TerminalCoreUnitTests +{ + class TerminalBufferTests; + class ConptyRoundtripTests; +}; #endif class Microsoft::Terminal::Core::Terminal final : @@ -252,6 +256,7 @@ class Microsoft::Terminal::Core::Terminal final : #pragma endregion #ifdef UNIT_TESTING - friend class ::ConptyRoundtripTests; + friend class TerminalCoreUnitTests::TerminalBufferTests; + friend class TerminalCoreUnitTests::ConptyRoundtripTests; #endif }; diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index badc0548e9b..c8732ca2dc1 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -27,6 +27,8 @@ class InputBuffer; // This for some reason needs to be fwd-decl'd #include "../cascadia/TerminalCore/Terminal.hpp" +#include "TestUtils.h" + using namespace WEX::Common; using namespace WEX::Logging; using namespace WEX::TestExecution; @@ -40,8 +42,17 @@ using namespace Microsoft::Console::Types; using namespace Microsoft::Terminal::Core; -class ConptyRoundtripTests +namespace TerminalCoreUnitTests +{ + class TerminalBufferTests; +}; +using namespace TerminalCoreUnitTests; + +class TerminalCoreUnitTests::ConptyRoundtripTests final { + static const SHORT TerminalViewWidth = 80; + static const SHORT TerminalViewHeight = 32; + TEST_CLASS(ConptyRoundtripTests); TEST_CLASS_SETUP(ClassSetup) @@ -50,7 +61,7 @@ class ConptyRoundtripTests m_state->InitEvents(); m_state->PrepareGlobalFont(); - m_state->PrepareGlobalScreenBuffer(); + m_state->PrepareGlobalScreenBuffer(TerminalViewWidth, TerminalViewHeight, TerminalViewWidth, TerminalViewHeight); m_state->PrepareGlobalInputBuffer(); return true; @@ -71,18 +82,19 @@ class ConptyRoundtripTests { // STEP 1: Set up the Terminal term = std::make_unique(); - term->Create({ CommonState::s_csBufferWidth, CommonState::s_csBufferHeight }, 0, emptyRT); + term->Create({ TerminalViewWidth, TerminalViewHeight }, 100, emptyRT); // STEP 2: Set up the Conpty // Set up some sane defaults auto& g = ServiceLocator::LocateGlobals(); auto& gci = g.getConsoleInformation(); + gci.SetDefaultForegroundColor(INVALID_COLOR); gci.SetDefaultBackgroundColor(INVALID_COLOR); gci.SetFillAttribute(0x07); // DARK_WHITE on DARK_BLACK - m_state->PrepareNewTextBufferInfo(true); + m_state->PrepareNewTextBufferInfo(true, TerminalViewWidth, TerminalViewHeight); auto& currentBuffer = gci.GetActiveOutputBuffer(); // Make sure a test hasn't left us in the alt buffer on accident VERIFY_IS_FALSE(currentBuffer._IsAltBuffer()); @@ -106,6 +118,13 @@ class ConptyRoundtripTests g.pRender->AddRenderEngine(_pVtRenderEngine.get()); gci.GetActiveOutputBuffer().SetTerminalConnection(_pVtRenderEngine.get()); + _pConApi = std::make_unique(gci); + + // Manually set the console into conpty mode. We're not actually going + // to set up the pipes for conpty, but we want the console to behave + // like it would in conpty mode. + g.EnableConptyModeForTests(); + expectedOutput.clear(); return true; @@ -133,9 +152,15 @@ class ConptyRoundtripTests private: bool _writeCallback(const char* const pch, size_t const cch); void _flushFirstFrame(); + void _resizeConpty(const unsigned short sx, const unsigned short sy); std::deque expectedOutput; std::unique_ptr _pVtRenderEngine; std::unique_ptr m_state; + std::unique_ptr _pConApi; + + // Tests can set these variables how they link to configure the behavior of the test harness. + bool _checkConptyOutput{ true }; // If true, the test class will check that the output from conpty was expected + bool _logConpty{ false }; // If true, the test class will log all the output from conpty. Helpful for debugging. DummyRenderTarget emptyRT; std::unique_ptr term; @@ -144,18 +169,27 @@ class ConptyRoundtripTests bool ConptyRoundtripTests::_writeCallback(const char* const pch, size_t const cch) { std::string actualString = std::string(pch, cch); - VERIFY_IS_GREATER_THAN(expectedOutput.size(), - static_cast(0), - NoThrowString().Format(L"writing=\"%hs\", expecting %u strings", actualString.c_str(), expectedOutput.size())); - std::string first = expectedOutput.front(); - expectedOutput.pop_front(); + if (_checkConptyOutput) + { + VERIFY_IS_GREATER_THAN(expectedOutput.size(), + static_cast(0), + NoThrowString().Format(L"writing=\"%hs\", expecting %u strings", actualString.c_str(), expectedOutput.size())); - Log::Comment(NoThrowString().Format(L"Expected =\t\"%hs\"", first.c_str())); - Log::Comment(NoThrowString().Format(L"Actual =\t\"%hs\"", actualString.c_str())); + std::string first = expectedOutput.front(); + expectedOutput.pop_front(); - VERIFY_ARE_EQUAL(first.length(), cch); - VERIFY_ARE_EQUAL(first, actualString); + Log::Comment(NoThrowString().Format(L"Expected =\t\"%hs\"", first.c_str())); + Log::Comment(NoThrowString().Format(L"Actual =\t\"%hs\"", actualString.c_str())); + + VERIFY_ARE_EQUAL(first.length(), cch); + VERIFY_ARE_EQUAL(first, actualString); + } + else if (_logConpty) + { + Log::Comment(NoThrowString().Format( + L"Writing \"%hs\" to Terminal", actualString.c_str())); + } // Write the string back to our Terminal const auto converted = ConvertToW(CP_UTF8, actualString); @@ -177,75 +211,17 @@ void ConptyRoundtripTests::_flushFirstFrame() VERIFY_SUCCEEDED(renderer.PaintFrame()); } -// Function Description: -// - Helper function to validate that a number of characters in a row are all -// the same. Validates that the next end-start characters are all equal to the -// provided string. Will move the provided iterator as it validates. The -// caller should ensure that `iter` starts where they would like to validate. -// Arguments: -// - expectedChar: The character (or characters) we're expecting -// - iter: a iterator pointing to the cell we'd like to start validating at. -// - start: the first index in the range we'd like to validate -// - end: the last index in the range we'd like to validate -// Return Value: -// - -void _verifySpanOfText(const wchar_t* const expectedChar, - TextBufferCellIterator& iter, - const int start, - const int end) +void ConptyRoundtripTests::_resizeConpty(const unsigned short sx, + const unsigned short sy) { - for (int x = start; x < end; x++) + // Largely taken from implementation in PtySignalInputThread::_InputThread + if (DispatchCommon::s_ResizeWindow(*_pConApi, sx, sy)) { - SetVerifyOutput settings(VerifyOutputSettings::LogOnlyFailures); - if (iter->Chars() != expectedChar) - { - Log::Comment(NoThrowString().Format(L"character [%d] was mismatched", x)); - } - VERIFY_ARE_EQUAL(expectedChar, (iter++)->Chars()); + // Instead of going through the VtIo to suppress the resize repaint, + // just call the method directly on the renderer. This is implemented in + // VtIo::SuppressResizeRepaint + VERIFY_SUCCEEDED(_pVtRenderEngine->SuppressResizeRepaint()); } - Log::Comment(NoThrowString().Format( - L"Successfully validated %d characters were '%s'", end - start, expectedChar)); -} - -// Function Description: -// - Helper function to validate that the next characters pointed to by `iter` -// are the provided string. Will increment iter as it walks the provided -// string of characters. It will leave `iter` on the first character after the -// expectedString. -// Arguments: -// - expectedString: The characters we're expecting -// - iter: a iterator pointing to the cell we'd like to start validating at. -// Return Value: -// - -void _verifyExpectedString(std::wstring_view expectedString, - TextBufferCellIterator& iter) -{ - for (const auto wch : expectedString) - { - wchar_t buffer[]{ wch, L'\0' }; - std::wstring_view view{ buffer, 1 }; - VERIFY_IS_TRUE(iter, L"Ensure iterator is still valid"); - VERIFY_ARE_EQUAL(view, (iter++)->Chars(), NoThrowString().Format(L"%s", view.data())); - } -} - -// Function Description: -// - Helper function to validate that the next characters in the buffer at the -// given location are the provided string. Will return an iterator on the -// first character after the expectedString. -// Arguments: -// - tb: the buffer who's content we should check -// - expectedString: The characters we're expecting -// - pos: the starting position in the buffer to check the contents of -// Return Value: -// - an iterator on the first character after the expectedString. -TextBufferCellIterator _verifyExpectedString(const TextBuffer& tb, - std::wstring_view expectedString, - const COORD pos) -{ - auto iter = tb.GetCellDataAt(pos); - _verifyExpectedString(expectedString, iter); - return iter; } void ConptyRoundtripTests::ConptyOutputTestCanary() @@ -278,7 +254,7 @@ void ConptyRoundtripTests::SimpleWriteOutputTest() VERIFY_SUCCEEDED(renderer.PaintFrame()); - _verifyExpectedString(termTb, L"Hello World ", { 0, 0 }); + TestUtils::VerifyExpectedString(termTb, L"Hello World ", { 0, 0 }); } void ConptyRoundtripTests::WriteTwoLinesUsesNewline() @@ -302,8 +278,8 @@ void ConptyRoundtripTests::WriteTwoLinesUsesNewline() hostSm.ProcessString(L"BBB"); auto verifyData = [](TextBuffer& tb) { - _verifyExpectedString(tb, L"AAA", { 0, 0 }); - _verifyExpectedString(tb, L"BBB", { 0, 1 }); + TestUtils::VerifyExpectedString(tb, L"AAA", { 0, 0 }); + TestUtils::VerifyExpectedString(tb, L"BBB", { 0, 1 }); }; verifyData(hostTb); @@ -338,10 +314,10 @@ void ConptyRoundtripTests::WriteAFewSimpleLines() hostSm.ProcessString(L"\n"); hostSm.ProcessString(L"CCC"); auto verifyData = [](TextBuffer& tb) { - _verifyExpectedString(tb, L"AAA", { 0, 0 }); - _verifyExpectedString(tb, L"BBB", { 0, 1 }); - _verifyExpectedString(tb, L" ", { 0, 2 }); - _verifyExpectedString(tb, L"CCC", { 0, 3 }); + TestUtils::VerifyExpectedString(tb, L"AAA", { 0, 0 }); + TestUtils::VerifyExpectedString(tb, L"BBB", { 0, 1 }); + TestUtils::VerifyExpectedString(tb, L" ", { 0, 2 }); + TestUtils::VerifyExpectedString(tb, L"CCC", { 0, 3 }); }; verifyData(hostTb); diff --git a/src/cascadia/UnitTests_TerminalCore/TerminalBufferTests.cpp b/src/cascadia/UnitTests_TerminalCore/TerminalBufferTests.cpp new file mode 100644 index 00000000000..57285c4e390 --- /dev/null +++ b/src/cascadia/UnitTests_TerminalCore/TerminalBufferTests.cpp @@ -0,0 +1,68 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +#include "precomp.h" +#include + +#include "../renderer/inc/DummyRenderTarget.hpp" +#include "../cascadia/TerminalCore/Terminal.hpp" +#include "MockTermSettings.h" +#include "consoletaeftemplates.hpp" +#include "TestUtils.h" + +using namespace winrt::Microsoft::Terminal::Settings; +using namespace Microsoft::Terminal::Core; + +using namespace WEX::Common; +using namespace WEX::Logging; +using namespace WEX::TestExecution; + +namespace TerminalCoreUnitTests +{ + class TerminalBufferTests; +}; +using namespace TerminalCoreUnitTests; + +class TerminalCoreUnitTests::TerminalBufferTests final +{ + TEST_CLASS(TerminalBufferTests); + + TEST_METHOD(TestSimpleBufferWriting); + + TEST_METHOD_SETUP(MethodSetup) + { + // STEP 1: Set up the Terminal + term = std::make_unique(); + term->Create({ 80, 32 }, 100, emptyRT); + return true; + } + + TEST_METHOD_CLEANUP(MethodCleanup) + { + term = nullptr; + return true; + } + +private: + DummyRenderTarget emptyRT; + std::unique_ptr term; +}; + +void TerminalBufferTests::TestSimpleBufferWriting() +{ + auto& termTb = *term->_buffer; + auto& termSm = *term->_stateMachine; + const auto initialView = term->GetViewport(); + + VERIFY_ARE_EQUAL(0, initialView.Top()); + VERIFY_ARE_EQUAL(32, initialView.BottomExclusive()); + + termSm.ProcessString(L"Hello World"); + + const auto secondView = term->GetViewport(); + + VERIFY_ARE_EQUAL(0, secondView.Top()); + VERIFY_ARE_EQUAL(32, secondView.BottomExclusive()); + + TestUtils::VerifyExpectedString(termTb, L"Hello World", { 0, 0 }); +} diff --git a/src/cascadia/UnitTests_TerminalCore/TestUtils.h b/src/cascadia/UnitTests_TerminalCore/TestUtils.h new file mode 100644 index 00000000000..1c8afe34b2d --- /dev/null +++ b/src/cascadia/UnitTests_TerminalCore/TestUtils.h @@ -0,0 +1,94 @@ +/*++ +Copyright (c) Microsoft Corporation +Licensed under the MIT license. + +Module Name: +- TestUtils.h + +Abstract: +- This file has helper functions for writing tests for the TerminalCore project. + +Author(s): + Mike Griese (migrie) January-2020 +--*/ + +#include "../../buffer/out/textBuffer.hpp" + +namespace TerminalCoreUnitTests +{ + class TestUtils; +}; +class TerminalCoreUnitTests::TestUtils +{ +public: + // Function Description: + // - Helper function to validate that a number of characters in a row are all + // the same. Validates that the next end-start characters are all equal to the + // provided string. Will move the provided iterator as it validates. The + // caller should ensure that `iter` starts where they would like to validate. + // Arguments: + // - expectedChar: The character (or characters) we're expecting + // - iter: a iterator pointing to the cell we'd like to start validating at. + // - start: the first index in the range we'd like to validate + // - end: the last index in the range we'd like to validate + // Return Value: + // - + static void VerifySpanOfText(const wchar_t* const expectedChar, + TextBufferCellIterator& iter, + const int start, + const int end) + { + for (int x = start; x < end; x++) + { + WEX::TestExecution::SetVerifyOutput settings(WEX::TestExecution::VerifyOutputSettings::LogOnlyFailures); + if (iter->Chars() != expectedChar) + { + WEX::Logging::Log::Comment(WEX::Common::NoThrowString().Format(L"character [%d] was mismatched", x)); + } + VERIFY_ARE_EQUAL(expectedChar, (iter++)->Chars()); + } + WEX::Logging::Log::Comment(WEX::Common::NoThrowString().Format( + L"Successfully validated %d characters were '%s'", end - start, expectedChar)); + }; + + // Function Description: + // - Helper function to validate that the next characters pointed to by `iter` + // are the provided string. Will increment iter as it walks the provided + // string of characters. It will leave `iter` on the first character after the + // expectedString. + // Arguments: + // - expectedString: The characters we're expecting + // - iter: a iterator pointing to the cell we'd like to start validating at. + // Return Value: + // - + static void VerifyExpectedString(std::wstring_view expectedString, + TextBufferCellIterator& iter) + { + for (const auto wch : expectedString) + { + wchar_t buffer[]{ wch, L'\0' }; + std::wstring_view view{ buffer, 1 }; + VERIFY_IS_TRUE(iter, L"Ensure iterator is still valid"); + VERIFY_ARE_EQUAL(view, (iter++)->Chars(), WEX::Common::NoThrowString().Format(L"%s", view.data())); + } + }; + + // Function Description: + // - Helper function to validate that the next characters in the buffer at the + // given location are the provided string. Will return an iterator on the + // first character after the expectedString. + // Arguments: + // - tb: the buffer who's content we should check + // - expectedString: The characters we're expecting + // - pos: the starting position in the buffer to check the contents of + // Return Value: + // - an iterator on the first character after the expectedString. + static TextBufferCellIterator VerifyExpectedString(const TextBuffer& tb, + std::wstring_view expectedString, + const COORD pos) + { + auto iter = tb.GetCellDataAt(pos); + VerifyExpectedString(expectedString, iter); + return iter; + }; +}; diff --git a/src/cascadia/UnitTests_TerminalCore/UnitTests.vcxproj b/src/cascadia/UnitTests_TerminalCore/UnitTests.vcxproj index abb6ec70d7b..1d28fc7c129 100644 --- a/src/cascadia/UnitTests_TerminalCore/UnitTests.vcxproj +++ b/src/cascadia/UnitTests_TerminalCore/UnitTests.vcxproj @@ -19,6 +19,7 @@ + diff --git a/src/host/ConsoleArguments.cpp b/src/host/ConsoleArguments.cpp index c6e5e12c22e..b33256a84aa 100644 --- a/src/host/ConsoleArguments.cpp +++ b/src/host/ConsoleArguments.cpp @@ -638,3 +638,17 @@ void ConsoleArguments::SetExpectedSize(COORD dimensions) noexcept _recievedEarlySizeChange = true; } } + +#ifdef UNIT_TESTING +// Method Description: +// - This is a test helper method. It can be used to trick us into thinking +// we're headless (in conpty mode), even without parsing any arguments. +// Arguments: +// - +// Return Value: +// - +void ConsoleArguments::EnableConptyModeForTests() +{ + _headless = true; +} +#endif diff --git a/src/host/ConsoleArguments.hpp b/src/host/ConsoleArguments.hpp index 9a903ea18df..8b0daeb7d68 100644 --- a/src/host/ConsoleArguments.hpp +++ b/src/host/ConsoleArguments.hpp @@ -53,6 +53,10 @@ class ConsoleArguments void SetExpectedSize(COORD dimensions) noexcept; +#ifdef UNIT_TESTING + void EnableConptyModeForTests(); +#endif + static const std::wstring_view VT_MODE_ARG; static const std::wstring_view HEADLESS_ARG; static const std::wstring_view SERVER_HANDLE_ARG; diff --git a/src/host/VtIo.cpp b/src/host/VtIo.cpp index cf7ab1ecbe5..de732a79edd 100644 --- a/src/host/VtIo.cpp +++ b/src/host/VtIo.cpp @@ -431,3 +431,18 @@ void VtIo::EndResize() _pVtRenderEngine->EndResizeRequest(); } } + +#ifdef UNIT_TESTING +// Method Description: +// - This is a test helper method. It can be used to trick VtIo into responding +// true to `IsUsingVt`, which will cause the console host to act in conpty +// mode. +// Arguments: +// - +// Return Value: +// - +void VtIo::EnableConptyModeForTests() +{ + _objectsCreated = true; +} +#endif diff --git a/src/host/VtIo.hpp b/src/host/VtIo.hpp index 0516d9ab0ea..b2600c10270 100644 --- a/src/host/VtIo.hpp +++ b/src/host/VtIo.hpp @@ -39,6 +39,10 @@ namespace Microsoft::Console::VirtualTerminal void BeginResize(); void EndResize(); +#ifdef UNIT_TESTING + void EnableConptyModeForTests(); +#endif + private: // After CreateIoHandlers is called, these will be invalid. wil::unique_hfile _hInput; diff --git a/src/host/globals.cpp b/src/host/globals.cpp index b9f82175cb4..f39a0d106ca 100644 --- a/src/host/globals.cpp +++ b/src/host/globals.cpp @@ -16,3 +16,19 @@ bool Globals::IsHeadless() const { return launchArgs.IsHeadless(); } + +#ifdef UNIT_TESTING +// Method Description: +// - This is a test helper method. It can be used to trick us into responding +// true to `IsHeadless`, which will cause the console host to act in conpty +// mode. +// Arguments: +// - +// Return Value: +// - +void Globals::EnableConptyModeForTests() +{ + launchArgs.EnableConptyModeForTests(); + getConsoleInformation().GetVtIo()->EnableConptyModeForTests(); +} +#endif diff --git a/src/host/globals.h b/src/host/globals.h index 459bd3689e7..bd320d90d0f 100644 --- a/src/host/globals.h +++ b/src/host/globals.h @@ -69,6 +69,10 @@ class Globals ApiRoutines api; +#ifdef UNIT_TESTING + void EnableConptyModeForTests(); +#endif + private: CONSOLE_INFORMATION ciConsoleInformation; }; diff --git a/src/host/screenInfo.hpp b/src/host/screenInfo.hpp index eb257081227..1e8eb3c75e5 100644 --- a/src/host/screenInfo.hpp +++ b/src/host/screenInfo.hpp @@ -50,6 +50,14 @@ Revision History: #include "../types/IConsoleWindow.hpp" class ConversionAreaInfo; // forward decl window. circular reference +// fwdecl unittest classes +#ifdef UNIT_TESTING +namespace TerminalCoreUnitTests +{ + class ConptyRoundtripTests; +}; +#endif + class SCREEN_INFORMATION : public ConsoleObjectHeader, public Microsoft::Console::IIoProvider { public: @@ -308,6 +316,6 @@ class SCREEN_INFORMATION : public ConsoleObjectHeader, public Microsoft::Console friend class ScreenBufferTests; friend class CommonState; friend class ConptyOutputTests; - friend class ConptyRoundtripTests; + friend class TerminalCoreUnitTests::ConptyRoundtripTests; #endif }; diff --git a/src/host/ut_host/ConptyOutputTests.cpp b/src/host/ut_host/ConptyOutputTests.cpp index 1bc6a189900..42e9f36b633 100644 --- a/src/host/ut_host/ConptyOutputTests.cpp +++ b/src/host/ut_host/ConptyOutputTests.cpp @@ -87,6 +87,11 @@ class ConptyOutputTests expectedOutput.clear(); + // Manually set the console into conpty mode. We're not actually going + // to set up the pipes for conpty, but we want the console to behave + // like it would in conpty mode. + g.EnableConptyModeForTests(); + return true; } diff --git a/src/inc/test/CommonState.hpp b/src/inc/test/CommonState.hpp index 85dfeb960c1..7d1fbd7f4ea 100644 --- a/src/inc/test/CommonState.hpp +++ b/src/inc/test/CommonState.hpp @@ -81,16 +81,19 @@ class CommonState } } - void PrepareGlobalScreenBuffer() + void PrepareGlobalScreenBuffer(const short viewWidth = s_csWindowWidth, + const short viewHeight = s_csWindowHeight, + const short bufferWidth = s_csBufferWidth, + const short bufferHeight = s_csBufferHeight) { CONSOLE_INFORMATION& gci = Microsoft::Console::Interactivity::ServiceLocator::LocateGlobals().getConsoleInformation(); COORD coordWindowSize; - coordWindowSize.X = s_csWindowWidth; - coordWindowSize.Y = s_csWindowHeight; + coordWindowSize.X = viewWidth; + coordWindowSize.Y = viewHeight; COORD coordScreenBufferSize; - coordScreenBufferSize.X = s_csBufferWidth; - coordScreenBufferSize.Y = s_csBufferHeight; + coordScreenBufferSize.X = bufferWidth; + coordScreenBufferSize.Y = bufferHeight; UINT uiCursorSize = 12; @@ -143,12 +146,14 @@ class CommonState gci.SetCookedReadData(nullptr); } - void PrepareNewTextBufferInfo(const bool useDefaultAttributes = false) + void PrepareNewTextBufferInfo(const bool useDefaultAttributes = false, + const short bufferWidth = s_csBufferWidth, + const short bufferHeight = s_csBufferHeight) { CONSOLE_INFORMATION& gci = Microsoft::Console::Interactivity::ServiceLocator::LocateGlobals().getConsoleInformation(); COORD coordScreenBufferSize; - coordScreenBufferSize.X = s_csBufferWidth; - coordScreenBufferSize.Y = s_csBufferHeight; + coordScreenBufferSize.X = bufferWidth; + coordScreenBufferSize.Y = bufferHeight; UINT uiCursorSize = 12; diff --git a/src/renderer/vt/vtrenderer.hpp b/src/renderer/vt/vtrenderer.hpp index cc6fed764c5..27658e7f9c8 100644 --- a/src/renderer/vt/vtrenderer.hpp +++ b/src/renderer/vt/vtrenderer.hpp @@ -26,7 +26,10 @@ Author(s): // fwdecl unittest classes #ifdef UNIT_TESTING -class ConptyRoundtripTests; +namespace TerminalCoreUnitTests +{ + class ConptyRoundtripTests; +}; #endif namespace Microsoft::Console::Render @@ -226,7 +229,7 @@ namespace Microsoft::Console::Render friend class VtRendererTest; friend class ConptyOutputTests; - friend class ConptyRoundtripTests; + friend class TerminalCoreUnitTests::ConptyRoundtripTests; #endif void SetTestCallback(_In_ std::function pfn); From bfde821b2fab316765edec3764c07b992e7cad5c Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Tue, 28 Jan 2020 10:42:34 -0600 Subject: [PATCH 08/39] A simple test for wrapped lines --- src/cascadia/TerminalCore/Terminal.cpp | 3 + .../TerminalBufferTests.cpp | 80 +++++++++++++++++++ src/renderer/vt/XtermEngine.cpp | 2 +- src/renderer/vt/tracing.cpp | 18 +++-- src/renderer/vt/tracing.hpp | 2 +- 5 files changed, 96 insertions(+), 9 deletions(-) diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index e79ba5b0238..a82f96a4cf1 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -490,6 +490,9 @@ void Terminal::_WriteBuffer(const std::wstring_view& stringView) // _scary_. Perhaps this was never hit before we had conpty // wrapping? i--; + + // Mark the line we're currently on as wrapped + _buffer->GetRowByOffset(cursorPosBefore.Y).GetCharRow().SetWrapForced(true); } } diff --git a/src/cascadia/UnitTests_TerminalCore/TerminalBufferTests.cpp b/src/cascadia/UnitTests_TerminalCore/TerminalBufferTests.cpp index 57285c4e390..65b2900fe9c 100644 --- a/src/cascadia/UnitTests_TerminalCore/TerminalBufferTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/TerminalBufferTests.cpp @@ -23,12 +23,19 @@ namespace TerminalCoreUnitTests }; using namespace TerminalCoreUnitTests; +const static std::wstring test100CharsString{ + LR"(!"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\]^_`abcdefghijklmnopqrstuvwxyz{|}~!"#$%&)" +}; + class TerminalCoreUnitTests::TerminalBufferTests final { TEST_CLASS(TerminalBufferTests); TEST_METHOD(TestSimpleBufferWriting); + TEST_METHOD(TestWrappingCharByChar); + TEST_METHOD(TestWrappingALongString); + TEST_METHOD_SETUP(MethodSetup) { // STEP 1: Set up the Terminal @@ -66,3 +73,76 @@ void TerminalBufferTests::TestSimpleBufferWriting() TestUtils::VerifyExpectedString(termTb, L"Hello World", { 0, 0 }); } + +void TerminalBufferTests::TestWrappingCharByChar() +{ + auto& termTb = *term->_buffer; + auto& termSm = *term->_stateMachine; + const auto initialView = term->GetViewport(); + auto& cursor = termTb.GetCursor(); + + const auto charsToWrite = test100CharsString.size(); + + VERIFY_ARE_EQUAL(0, initialView.Top()); + VERIFY_ARE_EQUAL(32, initialView.BottomExclusive()); + + 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(33 + (i % 94)); + termSm.ProcessCharacter(wch); + } + + const auto secondView = term->GetViewport(); + + VERIFY_ARE_EQUAL(0, secondView.Top()); + VERIFY_ARE_EQUAL(32, secondView.BottomExclusive()); + + // Verify the cursor wrapped to the second line + VERIFY_ARE_EQUAL(charsToWrite % initialView.Width(), cursor.GetPosition().X); + VERIFY_ARE_EQUAL(1, cursor.GetPosition().Y); + + // Verify that we marked the 0th row as _wrapped_ + const auto& row0 = termTb.GetRowByOffset(0); + VERIFY_IS_TRUE(row0.GetCharRow().WasWrapForced()); + + const auto& row1 = termTb.GetRowByOffset(1); + VERIFY_IS_FALSE(row1.GetCharRow().WasWrapForced()); + + TestUtils::VerifyExpectedString(termTb, test100CharsString, { 0, 0 }); +} + +void TerminalBufferTests::TestWrappingALongString() +{ + auto& termTb = *term->_buffer; + auto& termSm = *term->_stateMachine; + const auto initialView = term->GetViewport(); + auto& cursor = termTb.GetCursor(); + + const auto charsToWrite = test100CharsString.size(); + VERIFY_ARE_EQUAL(100u, charsToWrite); + + VERIFY_ARE_EQUAL(0, initialView.Top()); + VERIFY_ARE_EQUAL(32, initialView.BottomExclusive()); + + termSm.ProcessString(test100CharsString); + + const auto secondView = term->GetViewport(); + + VERIFY_ARE_EQUAL(0, secondView.Top()); + VERIFY_ARE_EQUAL(32, secondView.BottomExclusive()); + + // Verify the cursor wrapped to the second line + VERIFY_ARE_EQUAL(charsToWrite % initialView.Width(), cursor.GetPosition().X); + VERIFY_ARE_EQUAL(1, cursor.GetPosition().Y); + + // Verify that we marked the 0th row as _wrapped_ + const auto& row0 = termTb.GetRowByOffset(0); + VERIFY_IS_TRUE(row0.GetCharRow().WasWrapForced()); + + const auto& row1 = termTb.GetRowByOffset(1); + VERIFY_IS_FALSE(row1.GetCharRow().WasWrapForced()); + + TestUtils::VerifyExpectedString(termTb, test100CharsString, { 0, 0 }); +} diff --git a/src/renderer/vt/XtermEngine.cpp b/src/renderer/vt/XtermEngine.cpp index f652ceb3d52..2c92c711704 100644 --- a/src/renderer/vt/XtermEngine.cpp +++ b/src/renderer/vt/XtermEngine.cpp @@ -235,7 +235,7 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, { HRESULT hr = S_OK; - _trace.TraceMoveCursor(coord); + _trace.TraceMoveCursor(_lastText, coord); if (coord.X != _lastText.X || coord.Y != _lastText.Y) { diff --git a/src/renderer/vt/tracing.cpp b/src/renderer/vt/tracing.cpp index 04c8d54c8b5..e87b4b5f017 100644 --- a/src/renderer/vt/tracing.cpp +++ b/src/renderer/vt/tracing.cpp @@ -210,18 +210,23 @@ void RenderTracing::TraceLastText(const COORD lastTextPos) const UNREFERENCED_PARAMETER(lastTextPos); #endif UNIT_TESTING } - -void RenderTracing::TraceMoveCursor(const COORD pos) const +void RenderTracing::TraceMoveCursor(const COORD lastTextPos, const COORD cursor) const { #ifndef UNIT_TESTING - const auto lastTextStr = _CoordToString(pos); - const auto cursor = lastTextStr.c_str(); + const auto lastTextStr = _CoordToString(lastTextPos); + const auto lastText = lastTextStr.c_str(); + + const auto cursorStr = _CoordToString(cursor); + const auto cursorPos = cursorStr.c_str(); + TraceLoggingWrite(g_hConsoleVtRendererTraceProvider, "VtEngine_TraceMoveCursor", - TraceLoggingString(cursor), + TraceLoggingString(lastText), + TraceLoggingString(cursorPos), TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE)); #else - UNREFERENCED_PARAMETER(pos); + UNREFERENCED_PARAMETER(lastTextPos); + UNREFERENCED_PARAMETER(cursor); #endif UNIT_TESTING } @@ -234,6 +239,5 @@ void RenderTracing::TraceWrapped() const TraceLoggingString(msg), TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE)); #else - UNREFERENCED_PARAMETER(pos); #endif UNIT_TESTING } diff --git a/src/renderer/vt/tracing.hpp b/src/renderer/vt/tracing.hpp index ef00bea81be..9c4ac93f1f0 100644 --- a/src/renderer/vt/tracing.hpp +++ b/src/renderer/vt/tracing.hpp @@ -29,7 +29,7 @@ namespace Microsoft::Console::VirtualTerminal void TraceString(const std::string_view& str) const; void TraceInvalidate(const Microsoft::Console::Types::Viewport view) const; void TraceLastText(const COORD lastText) const; - void TraceMoveCursor(const COORD pos) const; + void TraceMoveCursor(const COORD lastText, const COORD cursor) const; void TraceWrapped() const; void TraceInvalidateAll(const Microsoft::Console::Types::Viewport view) const; void TraceTriggerCircling(const bool newFrame) const; From e000388bafcf9958e569c5a4ae654570fac4580f Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Tue, 28 Jan 2020 11:17:56 -0600 Subject: [PATCH 09/39] add a roundtrip test --- .../ConptyRoundtripTests.cpp | 57 +++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index c8732ca2dc1..cc9f96d9a58 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -149,6 +149,8 @@ class TerminalCoreUnitTests::ConptyRoundtripTests final TEST_METHOD(WriteTwoLinesUsesNewline); TEST_METHOD(WriteAFewSimpleLines); + TEST_METHOD(TestWrappingALongString); + private: bool _writeCallback(const char* const pch, size_t const cch); void _flushFirstFrame(); @@ -340,3 +342,58 @@ 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 std::wstring test100CharsString{ + LR"(!"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\]^_`abcdefghijklmnopqrstuvwxyz{|}~!"#$%&)" + }; + + const auto charsToWrite = test100CharsString.size(); + VERIFY_ARE_EQUAL(100u, charsToWrite); + + VERIFY_ARE_EQUAL(0, initialTermView.Top()); + VERIFY_ARE_EQUAL(32, initialTermView.BottomExclusive()); + + hostSm.ProcessString(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, test100CharsString, { 0, 0 }); + }; + + verifyBuffer(hostTb); + + VERIFY_SUCCEEDED(renderer.PaintFrame()); + + verifyBuffer(termTb); +} From c040a82e627c422e8565ac1c5d9788e0aa3225b9 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Tue, 28 Jan 2020 12:34:55 -0600 Subject: [PATCH 10/39] I've found a bug with the line wrapping, going to go update the PaintBufferLine interface --- .../ConptyRoundtripTests.cpp | 219 +++++++++++++++++- .../TerminalBufferTests.cpp | 14 +- .../UnitTests_TerminalCore/TestUtils.h | 4 + 3 files changed, 221 insertions(+), 16 deletions(-) diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index cc9f96d9a58..2d8ea1dbbd8 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -126,6 +126,8 @@ class TerminalCoreUnitTests::ConptyRoundtripTests final g.EnableConptyModeForTests(); expectedOutput.clear(); + _checkConptyOutput = true; + _logConpty = false; return true; } @@ -150,6 +152,9 @@ class TerminalCoreUnitTests::ConptyRoundtripTests final TEST_METHOD(WriteAFewSimpleLines); TEST_METHOD(TestWrappingALongString); + TEST_METHOD(TestAdvancedWrapping); + TEST_METHOD(TestExactWrappingWithoutSpaces); + TEST_METHOD(TestExactWrappingWithSpaces); private: bool _writeCallback(const char* const pch, size_t const cch); @@ -358,17 +363,13 @@ void ConptyRoundtripTests::TestWrappingALongString() const auto initialTermView = term->GetViewport(); - const std::wstring test100CharsString{ - LR"(!"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\]^_`abcdefghijklmnopqrstuvwxyz{|}~!"#$%&)" - }; - - const auto charsToWrite = test100CharsString.size(); + const auto charsToWrite = TestUtils::Test100CharsString.size(); VERIFY_ARE_EQUAL(100u, charsToWrite); VERIFY_ARE_EQUAL(0, initialTermView.Top()); VERIFY_ARE_EQUAL(32, initialTermView.BottomExclusive()); - hostSm.ProcessString(test100CharsString); + hostSm.ProcessString(TestUtils::Test100CharsString); const auto secondView = term->GetViewport(); @@ -388,11 +389,215 @@ void ConptyRoundtripTests::TestWrappingALongString() const auto& row1 = tb.GetRowByOffset(1); VERIFY_IS_FALSE(row1.GetCharRow().WasWrapForced()); - TestUtils::VerifyExpectedString(tb, test100CharsString, { 0, 0 }); + 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(); + // _checkConptyOutput = false; + + const auto charsToWrite = TestUtils::Test100CharsString.size(); + VERIFY_ARE_EQUAL(100u, charsToWrite); + + // VERIFY_ARE_EQUAL(0, initialTermView.Top()); + // VERIFY_ARE_EQUAL(32, initialTermView.BottomExclusive()); + + 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() +{ + 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(); + // _checkConptyOutput = false; + + 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(33 + (i % 94)); + hostSm.ProcessCharacter(wch); + } + + hostSm.ProcessString(L"\n"); + hostSm.ProcessString(L"1234567890"); + + auto verifyBuffer = [&](const TextBuffer& tb) { + 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); + + // Verify that we marked the 0th row as _wrapped_ + const auto& row0 = tb.GetRowByOffset(0); + VERIFY_IS_FALSE(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); + // First write the first 80 characters from the string + expectedOutput.push_back(R"(!"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\]^_`abcdefghijklmnop)"); + + // This test has revealed a bug in the current implementation. + // + // If a line _exactly_ wraps to the next line, we can't tell if the line + // should really wrap, or manually break. The cline 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 current PaintBufferLine interface, there's no way to know if + // this case is because the line wrapped or not. + + // 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::TestExactWrappingWithSpaces() +{ + 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(); + // _checkConptyOutput = false; + + 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(33 + (i % 94)); + hostSm.ProcessCharacter(wch); + } + + 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(1, 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_FALSE(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); + + // First write the first 80 characters from the string + expectedOutput.push_back(R"(!"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\]^_`abcdefghijklmnop)"); + + // This test has revealed a bug in the current implementation. + // + // If a line _exactly_ wraps to the next line, we can't tell if the line + // should really wrap, or manually break. The cline 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 current PaintBufferLine interface, there's no way to know if + // this case is because the line wrapped or not. + + // 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); diff --git a/src/cascadia/UnitTests_TerminalCore/TerminalBufferTests.cpp b/src/cascadia/UnitTests_TerminalCore/TerminalBufferTests.cpp index 65b2900fe9c..2341a7ace0b 100644 --- a/src/cascadia/UnitTests_TerminalCore/TerminalBufferTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/TerminalBufferTests.cpp @@ -23,10 +23,6 @@ namespace TerminalCoreUnitTests }; using namespace TerminalCoreUnitTests; -const static std::wstring test100CharsString{ - LR"(!"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\]^_`abcdefghijklmnopqrstuvwxyz{|}~!"#$%&)" -}; - class TerminalCoreUnitTests::TerminalBufferTests final { TEST_CLASS(TerminalBufferTests); @@ -81,7 +77,7 @@ void TerminalBufferTests::TestWrappingCharByChar() const auto initialView = term->GetViewport(); auto& cursor = termTb.GetCursor(); - const auto charsToWrite = test100CharsString.size(); + const auto charsToWrite = TestUtils::Test100CharsString.size(); VERIFY_ARE_EQUAL(0, initialView.Top()); VERIFY_ARE_EQUAL(32, initialView.BottomExclusive()); @@ -110,7 +106,7 @@ void TerminalBufferTests::TestWrappingCharByChar() const auto& row1 = termTb.GetRowByOffset(1); VERIFY_IS_FALSE(row1.GetCharRow().WasWrapForced()); - TestUtils::VerifyExpectedString(termTb, test100CharsString, { 0, 0 }); + TestUtils::VerifyExpectedString(termTb, TestUtils::Test100CharsString, { 0, 0 }); } void TerminalBufferTests::TestWrappingALongString() @@ -120,13 +116,13 @@ void TerminalBufferTests::TestWrappingALongString() const auto initialView = term->GetViewport(); auto& cursor = termTb.GetCursor(); - const auto charsToWrite = test100CharsString.size(); + const auto charsToWrite = TestUtils::Test100CharsString.size(); VERIFY_ARE_EQUAL(100u, charsToWrite); VERIFY_ARE_EQUAL(0, initialView.Top()); VERIFY_ARE_EQUAL(32, initialView.BottomExclusive()); - termSm.ProcessString(test100CharsString); + termSm.ProcessString(TestUtils::Test100CharsString); const auto secondView = term->GetViewport(); @@ -144,5 +140,5 @@ void TerminalBufferTests::TestWrappingALongString() const auto& row1 = termTb.GetRowByOffset(1); VERIFY_IS_FALSE(row1.GetCharRow().WasWrapForced()); - TestUtils::VerifyExpectedString(termTb, test100CharsString, { 0, 0 }); + TestUtils::VerifyExpectedString(termTb, TestUtils::Test100CharsString, { 0, 0 }); } diff --git a/src/cascadia/UnitTests_TerminalCore/TestUtils.h b/src/cascadia/UnitTests_TerminalCore/TestUtils.h index 1c8afe34b2d..24403207176 100644 --- a/src/cascadia/UnitTests_TerminalCore/TestUtils.h +++ b/src/cascadia/UnitTests_TerminalCore/TestUtils.h @@ -21,6 +21,10 @@ namespace TerminalCoreUnitTests class TerminalCoreUnitTests::TestUtils { public: + static constexpr std::wstring_view Test100CharsString{ + LR"(!"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\]^_`abcdefghijklmnopqrstuvwxyz{|}~!"#$%&)" + }; + // Function Description: // - Helper function to validate that a number of characters in a row are all // the same. Validates that the next end-start characters are all equal to the From 0755fd73e1b5a6c7aee21291d51b4eeb8b8472fd Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Tue, 28 Jan 2020 15:12:42 -0600 Subject: [PATCH 11/39] This is polished for PR, ready to go in after #4382 --- src/cascadia/TerminalCore/Terminal.cpp | 14 ++-- .../ConptyRoundtripTests.cpp | 70 +++++++++---------- src/interactivity/onecore/BgfxEngine.cpp | 3 +- src/interactivity/onecore/BgfxEngine.hpp | 3 +- src/renderer/base/renderer.cpp | 16 +++-- src/renderer/base/renderer.hpp | 3 +- src/renderer/dx/DxRenderer.cpp | 3 +- src/renderer/dx/DxRenderer.hpp | 3 +- src/renderer/gdi/gdirenderer.hpp | 3 +- src/renderer/gdi/paint.cpp | 3 +- src/renderer/inc/IRenderEngine.hpp | 3 +- src/renderer/uia/UiaRenderer.cpp | 3 +- src/renderer/uia/UiaRenderer.hpp | 3 +- src/renderer/vt/XtermEngine.cpp | 8 ++- src/renderer/vt/XtermEngine.hpp | 3 +- src/renderer/vt/paint.cpp | 12 +++- src/renderer/vt/vtrenderer.hpp | 6 +- src/renderer/wddmcon/WddmConRenderer.cpp | 3 +- src/renderer/wddmcon/WddmConRenderer.hpp | 3 +- 19 files changed, 100 insertions(+), 65 deletions(-) diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index a82f96a4cf1..ccafea911be 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -485,13 +485,19 @@ void Terminal::_WriteBuffer(const std::wstring_view& stringView) proposedCursorPosition.Y++; // Try the character again. - // TODO/DANGER: Does this actually work? The above comment seems - // to mention the i-=1, but there was no such line here. This is - // _scary_. Perhaps this was never hit before we had conpty - // wrapping? 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); } } diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index 2d8ea1dbbd8..55b211e5c48 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -411,14 +411,10 @@ void ConptyRoundtripTests::TestAdvancedWrapping() const auto initialTermView = term->GetViewport(); _flushFirstFrame(); - // _checkConptyOutput = false; const auto charsToWrite = TestUtils::Test100CharsString.size(); VERIFY_ARE_EQUAL(100u, charsToWrite); - // VERIFY_ARE_EQUAL(0, initialTermView.Top()); - // VERIFY_ARE_EQUAL(32, initialTermView.BottomExclusive()); - hostSm.ProcessString(TestUtils::Test100CharsString); hostSm.ProcessString(L"\n"); hostSm.ProcessString(L" "); @@ -462,6 +458,18 @@ void ConptyRoundtripTests::TestAdvancedWrapping() 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(); @@ -472,7 +480,6 @@ void ConptyRoundtripTests::TestExactWrappingWithoutSpaces() const auto initialTermView = term->GetViewport(); _flushFirstFrame(); - // _checkConptyOutput = false; const auto charsToWrite = initialTermView.Width(); VERIFY_ARE_EQUAL(80, charsToWrite); @@ -492,14 +499,18 @@ void ConptyRoundtripTests::TestExactWrappingWithoutSpaces() 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); + VERIFY_ARE_EQUAL(10, cursor.GetPosition().X); - // Verify that we marked the 0th row as _wrapped_ - const auto& row0 = tb.GetRowByOffset(0); - VERIFY_IS_FALSE(row0.GetCharRow().WasWrapForced()); + // 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. - const auto& row1 = tb.GetRowByOffset(1); - VERIFY_IS_FALSE(row1.GetCharRow().WasWrapForced()); + // // Verify that we marked the 0th row as _wrapped_ + // const auto& row0 = tb.GetRowByOffset(0); + // VERIFY_IS_FALSE(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 }); @@ -510,16 +521,6 @@ void ConptyRoundtripTests::TestExactWrappingWithoutSpaces() // First write the first 80 characters from the string expectedOutput.push_back(R"(!"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\]^_`abcdefghijklmnop)"); - // This test has revealed a bug in the current implementation. - // - // If a line _exactly_ wraps to the next line, we can't tell if the line - // should really wrap, or manually break. The cline 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 current PaintBufferLine interface, there's no way to know if - // this case is because the line wrapped or not. - // This is the hard line break expectedOutput.push_back("\r\n"); // Now write row 2 of the buffer @@ -533,6 +534,8 @@ void ConptyRoundtripTests::TestExactWrappingWithoutSpaces() 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(); @@ -543,7 +546,6 @@ void ConptyRoundtripTests::TestExactWrappingWithSpaces() const auto initialTermView = term->GetViewport(); _flushFirstFrame(); - // _checkConptyOutput = false; const auto charsToWrite = initialTermView.Width(); VERIFY_ARE_EQUAL(80, charsToWrite); @@ -566,12 +568,16 @@ void ConptyRoundtripTests::TestExactWrappingWithSpaces() VERIFY_ARE_EQUAL(1, 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_FALSE(row0.GetCharRow().WasWrapForced()); + // 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. - const auto& row1 = tb.GetRowByOffset(1); - VERIFY_IS_FALSE(row1.GetCharRow().WasWrapForced()); + // // Verify that we marked the 0th row as _wrapped_ + // const auto& row0 = tb.GetRowByOffset(0); + // VERIFY_IS_FALSE(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 }); @@ -582,16 +588,6 @@ void ConptyRoundtripTests::TestExactWrappingWithSpaces() // First write the first 80 characters from the string expectedOutput.push_back(R"(!"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\]^_`abcdefghijklmnop)"); - // This test has revealed a bug in the current implementation. - // - // If a line _exactly_ wraps to the next line, we can't tell if the line - // should really wrap, or manually break. The cline 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 current PaintBufferLine interface, there's no way to know if - // this case is because the line wrapped or not. - // This is the hard line break expectedOutput.push_back("\r\n"); // Now write row 2 of the buffer diff --git a/src/interactivity/onecore/BgfxEngine.cpp b/src/interactivity/onecore/BgfxEngine.cpp index b73c769c7a5..e3a6e2e16c6 100644 --- a/src/interactivity/onecore/BgfxEngine.cpp +++ b/src/interactivity/onecore/BgfxEngine.cpp @@ -147,7 +147,8 @@ BgfxEngine::BgfxEngine(PVOID SharedViewBase, LONG DisplayHeight, LONG DisplayWid [[nodiscard]] HRESULT BgfxEngine::PaintBufferLine(const std::basic_string_view clusters, const COORD coord, - const bool /*trimLeft*/) noexcept + const bool /*trimLeft*/, + const bool /*lineWrapped*/) noexcept { try { diff --git a/src/interactivity/onecore/BgfxEngine.hpp b/src/interactivity/onecore/BgfxEngine.hpp index 9ce46e35708..dcb20b99735 100644 --- a/src/interactivity/onecore/BgfxEngine.hpp +++ b/src/interactivity/onecore/BgfxEngine.hpp @@ -51,7 +51,8 @@ namespace Microsoft::Console::Render [[nodiscard]] HRESULT PaintBackground() noexcept override; [[nodiscard]] HRESULT PaintBufferLine(const std::basic_string_view clusters, const COORD coord, - const bool trimLeft) noexcept override; + const bool trimLeft, + const bool lineWrapped) noexcept override; [[nodiscard]] HRESULT PaintBufferGridLines(GridLines const lines, COLORREF const color, size_t const cchLine, COORD const coordTarget) noexcept override; [[nodiscard]] HRESULT PaintSelection(const SMALL_RECT rect) noexcept override; diff --git a/src/renderer/base/renderer.cpp b/src/renderer/base/renderer.cpp index 0b573c796ad..5a61239acbb 100644 --- a/src/renderer/base/renderer.cpp +++ b/src/renderer/base/renderer.cpp @@ -589,15 +589,23 @@ void Renderer::_PaintBufferOutput(_In_ IRenderEngine* const pEngine) // Retrieve the cell information iterator limited to just this line we want to redraw. auto it = buffer.GetCellDataAt(bufferLine.Origin(), bufferLine); + // TODO: calculate if two things are true: + // 1. this row wrapped + // 2. We're painting the last col of the row. + // In that case, set lineWrapped=true for the _PaintBufferOutputHelper call. + const auto lineWrapped = (buffer.GetRowByOffset(bufferLine.Origin().Y).GetCharRow().WasWrapForced()) && + (bufferLine.RightExclusive() == buffer.GetSize().Width()); + // Ask the helper to paint through this specific line. - _PaintBufferOutputHelper(pEngine, it, screenLine.Origin()); + _PaintBufferOutputHelper(pEngine, it, screenLine.Origin(), lineWrapped); } } } void Renderer::_PaintBufferOutputHelper(_In_ IRenderEngine* const pEngine, TextBufferCellIterator it, - const COORD target) + const COORD target, + const bool lineWrapped) { // If we have valid data, let's figure out how to draw it. if (it) @@ -656,7 +664,7 @@ void Renderer::_PaintBufferOutputHelper(_In_ IRenderEngine* const pEngine, // Do the painting. // TODO: Calculate when trim left should be TRUE - THROW_IF_FAILED(pEngine->PaintBufferLine({ clusters.data(), clusters.size() }, screenPoint, false)); + THROW_IF_FAILED(pEngine->PaintBufferLine({ clusters.data(), clusters.size() }, screenPoint, false, lineWrapped)); // If we're allowed to do grid drawing, draw that now too (since it will be coupled with the color data) if (_pData->IsGridLineDrawingAllowed()) @@ -805,7 +813,7 @@ void Renderer::_PaintOverlay(IRenderEngine& engine, auto it = overlay.buffer.GetCellLineDataAt(source); - _PaintBufferOutputHelper(&engine, it, target); + _PaintBufferOutputHelper(&engine, it, target, false); } } } diff --git a/src/renderer/base/renderer.hpp b/src/renderer/base/renderer.hpp index d16fe7fa130..7dfe3fe1885 100644 --- a/src/renderer/base/renderer.hpp +++ b/src/renderer/base/renderer.hpp @@ -96,7 +96,8 @@ namespace Microsoft::Console::Render void _PaintBufferOutputHelper(_In_ IRenderEngine* const pEngine, TextBufferCellIterator it, - const COORD target); + const COORD target, + const bool lineWrapped); static IRenderEngine::GridLines s_GetGridlines(const TextAttribute& textAttribute) noexcept; diff --git a/src/renderer/dx/DxRenderer.cpp b/src/renderer/dx/DxRenderer.cpp index ac19819ae70..726920cbc32 100644 --- a/src/renderer/dx/DxRenderer.cpp +++ b/src/renderer/dx/DxRenderer.cpp @@ -1149,7 +1149,8 @@ void DxEngine::_InvalidOr(RECT rc) noexcept // - S_OK or relevant DirectX error [[nodiscard]] HRESULT DxEngine::PaintBufferLine(std::basic_string_view const clusters, COORD const coord, - const bool /*trimLeft*/) noexcept + const bool /*trimLeft*/, + const bool /*lineWrapped*/) noexcept { try { diff --git a/src/renderer/dx/DxRenderer.hpp b/src/renderer/dx/DxRenderer.hpp index 38175b0b334..fbeeff83510 100644 --- a/src/renderer/dx/DxRenderer.hpp +++ b/src/renderer/dx/DxRenderer.hpp @@ -72,7 +72,8 @@ namespace Microsoft::Console::Render [[nodiscard]] HRESULT PaintBackground() noexcept override; [[nodiscard]] HRESULT PaintBufferLine(std::basic_string_view const clusters, COORD const coord, - bool const fTrimLeft) noexcept override; + bool const fTrimLeft, + const bool lineWrapped) noexcept override; [[nodiscard]] HRESULT PaintBufferGridLines(GridLines const lines, COLORREF const color, size_t const cchLine, COORD const coordTarget) noexcept override; [[nodiscard]] HRESULT PaintSelection(const SMALL_RECT rect) noexcept override; diff --git a/src/renderer/gdi/gdirenderer.hpp b/src/renderer/gdi/gdirenderer.hpp index 4840a181985..1fb1c09b05c 100644 --- a/src/renderer/gdi/gdirenderer.hpp +++ b/src/renderer/gdi/gdirenderer.hpp @@ -44,7 +44,8 @@ namespace Microsoft::Console::Render [[nodiscard]] HRESULT PaintBackground() noexcept override; [[nodiscard]] HRESULT PaintBufferLine(std::basic_string_view const clusters, const COORD coord, - const bool trimLeft) noexcept override; + const bool trimLeft, + const bool lineWrapped) noexcept override; [[nodiscard]] HRESULT PaintBufferGridLines(const GridLines lines, const COLORREF color, const size_t cchLine, diff --git a/src/renderer/gdi/paint.cpp b/src/renderer/gdi/paint.cpp index b0f49e1ea9c..70fd357928a 100644 --- a/src/renderer/gdi/paint.cpp +++ b/src/renderer/gdi/paint.cpp @@ -286,7 +286,8 @@ using namespace Microsoft::Console::Render; //#define MAX_POLY_LINES 80 [[nodiscard]] HRESULT GdiEngine::PaintBufferLine(std::basic_string_view const clusters, const COORD coord, - const bool trimLeft) noexcept + const bool trimLeft, + const bool /*lineWrapped*/) noexcept { try { diff --git a/src/renderer/inc/IRenderEngine.hpp b/src/renderer/inc/IRenderEngine.hpp index aff56a5bb58..455b6e8bb22 100644 --- a/src/renderer/inc/IRenderEngine.hpp +++ b/src/renderer/inc/IRenderEngine.hpp @@ -93,7 +93,8 @@ namespace Microsoft::Console::Render [[nodiscard]] virtual HRESULT PaintBackground() noexcept = 0; [[nodiscard]] virtual HRESULT PaintBufferLine(std::basic_string_view const clusters, const COORD coord, - const bool fTrimLeft) noexcept = 0; + const bool fTrimLeft, + const bool lineWrapped) noexcept = 0; [[nodiscard]] virtual HRESULT PaintBufferGridLines(const GridLines lines, const COLORREF color, const size_t cchLine, diff --git a/src/renderer/uia/UiaRenderer.cpp b/src/renderer/uia/UiaRenderer.cpp index 7b1ca85ca13..399cf065b4d 100644 --- a/src/renderer/uia/UiaRenderer.cpp +++ b/src/renderer/uia/UiaRenderer.cpp @@ -276,7 +276,8 @@ UiaEngine::UiaEngine(IUiaEventDispatcher* dispatcher) : // - S_FALSE [[nodiscard]] HRESULT UiaEngine::PaintBufferLine(std::basic_string_view const /*clusters*/, COORD const /*coord*/, - const bool /*trimLeft*/) noexcept + const bool /*trimLeft*/, + const bool /*lineWrapped*/) noexcept { return S_FALSE; } diff --git a/src/renderer/uia/UiaRenderer.hpp b/src/renderer/uia/UiaRenderer.hpp index 056a63b00a5..4dfbdccce41 100644 --- a/src/renderer/uia/UiaRenderer.hpp +++ b/src/renderer/uia/UiaRenderer.hpp @@ -53,7 +53,8 @@ namespace Microsoft::Console::Render [[nodiscard]] HRESULT PaintBackground() noexcept override; [[nodiscard]] HRESULT PaintBufferLine(std::basic_string_view const clusters, COORD const coord, - bool const fTrimLeft) noexcept override; + bool const fTrimLeft, + const bool lineWrapped) noexcept override; [[nodiscard]] HRESULT PaintBufferGridLines(GridLines const lines, COLORREF const color, size_t const cchLine, COORD const coordTarget) noexcept override; [[nodiscard]] HRESULT PaintSelection(const SMALL_RECT rect) noexcept override; diff --git a/src/renderer/vt/XtermEngine.cpp b/src/renderer/vt/XtermEngine.cpp index 2c92c711704..fb6ef5e35aa 100644 --- a/src/renderer/vt/XtermEngine.cpp +++ b/src/renderer/vt/XtermEngine.cpp @@ -427,15 +427,19 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, // - trimLeft - This specifies whether to trim one character width off the left // side of the output. Used for drawing the right-half only of a // double-wide character. +// - lineWrapped: true if this run we're painting is the end of a line that +// wrapped. If we're not painting the last column of a wrapped line, then this +// will be false. // Return Value: // - S_OK or suitable HRESULT error from writing pipe. [[nodiscard]] HRESULT XtermEngine::PaintBufferLine(std::basic_string_view const clusters, const COORD coord, - const bool /*trimLeft*/) noexcept + const bool /*trimLeft*/, + const bool lineWrapped) noexcept { return _fUseAsciiOnly ? VtEngine::_PaintAsciiBufferLine(clusters, coord) : - VtEngine::_PaintUtf8BufferLine(clusters, coord); + VtEngine::_PaintUtf8BufferLine(clusters, coord, lineWrapped); } // Method Description: diff --git a/src/renderer/vt/XtermEngine.hpp b/src/renderer/vt/XtermEngine.hpp index 28bdfc44080..2d52c89e215 100644 --- a/src/renderer/vt/XtermEngine.hpp +++ b/src/renderer/vt/XtermEngine.hpp @@ -48,7 +48,8 @@ namespace Microsoft::Console::Render const bool isSettingDefaultBrushes) noexcept override; [[nodiscard]] HRESULT PaintBufferLine(std::basic_string_view const clusters, const COORD coord, - const bool trimLeft) noexcept override; + const bool trimLeft, + const bool lineWrapped) noexcept override; [[nodiscard]] HRESULT ScrollFrame() noexcept override; [[nodiscard]] HRESULT InvalidateScroll(const COORD* const pcoordDelta) noexcept override; diff --git a/src/renderer/vt/paint.cpp b/src/renderer/vt/paint.cpp index bda79f6197a..cfa8db395a7 100644 --- a/src/renderer/vt/paint.cpp +++ b/src/renderer/vt/paint.cpp @@ -115,11 +115,15 @@ using namespace Microsoft::Console::Types; // - trimLeft - This specifies whether to trim one character width off the left // side of the output. Used for drawing the right-half only of a // double-wide character. +// - lineWrapped: true if this run we're painting is the end of a line that +// wrapped. If we're not painting the last column of a wrapped line, then this +// will be false. // Return Value: // - S_OK or suitable HRESULT error from writing pipe. [[nodiscard]] HRESULT VtEngine::PaintBufferLine(std::basic_string_view const clusters, const COORD coord, - const bool /*trimLeft*/) noexcept + const bool /*trimLeft*/, + const bool /*lineWrapped*/) noexcept { return VtEngine::_PaintAsciiBufferLine(clusters, coord); } @@ -369,7 +373,8 @@ using namespace Microsoft::Console::Types; // Return Value: // - S_OK or suitable HRESULT error from writing pipe. [[nodiscard]] HRESULT VtEngine::_PaintUtf8BufferLine(std::basic_string_view const clusters, - const COORD coord) noexcept + const COORD coord, + const bool lineWrapped) noexcept { if (coord.Y < _virtualTop) { @@ -469,7 +474,8 @@ using namespace Microsoft::Console::Types; // line. // Don't do this is 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. - if ((_lastText.X + (totalWidth - numSpaces)) > _lastViewport.RightInclusive()) + if (lineWrapped && + (_lastText.X + (totalWidth - numSpaces)) > _lastViewport.RightInclusive()) { _wrappedRow = coord.Y; } diff --git a/src/renderer/vt/vtrenderer.hpp b/src/renderer/vt/vtrenderer.hpp index 673de9fcf1b..77b7c688deb 100644 --- a/src/renderer/vt/vtrenderer.hpp +++ b/src/renderer/vt/vtrenderer.hpp @@ -65,7 +65,8 @@ namespace Microsoft::Console::Render [[nodiscard]] HRESULT PaintBackground() noexcept override; [[nodiscard]] virtual HRESULT PaintBufferLine(std::basic_string_view const clusters, const COORD coord, - const bool trimLeft) noexcept override; + const bool trimLeft, + const bool lineWrapped) noexcept override; [[nodiscard]] HRESULT PaintBufferGridLines(const GridLines lines, const COLORREF color, const size_t cchLine, @@ -214,7 +215,8 @@ namespace Microsoft::Console::Render bool _WillWriteSingleChar() const; [[nodiscard]] HRESULT _PaintUtf8BufferLine(std::basic_string_view const clusters, - const COORD coord) noexcept; + const COORD coord, + const bool lineWrapped) noexcept; [[nodiscard]] HRESULT _PaintAsciiBufferLine(std::basic_string_view const clusters, const COORD coord) noexcept; diff --git a/src/renderer/wddmcon/WddmConRenderer.cpp b/src/renderer/wddmcon/WddmConRenderer.cpp index 23804ec31f2..7d40b9533a8 100644 --- a/src/renderer/wddmcon/WddmConRenderer.cpp +++ b/src/renderer/wddmcon/WddmConRenderer.cpp @@ -260,7 +260,8 @@ bool WddmConEngine::IsInitialized() [[nodiscard]] HRESULT WddmConEngine::PaintBufferLine(std::basic_string_view const clusters, const COORD coord, - const bool /*trimLeft*/) noexcept + const bool /*trimLeft*/, + const bool /*lineWrapped*/) noexcept { try { diff --git a/src/renderer/wddmcon/WddmConRenderer.hpp b/src/renderer/wddmcon/WddmConRenderer.hpp index c214d585a8e..70213ca95f8 100644 --- a/src/renderer/wddmcon/WddmConRenderer.hpp +++ b/src/renderer/wddmcon/WddmConRenderer.hpp @@ -43,7 +43,8 @@ namespace Microsoft::Console::Render [[nodiscard]] HRESULT PaintBackground() noexcept override; [[nodiscard]] HRESULT PaintBufferLine(std::basic_string_view const clusters, const COORD coord, - const bool trimLeft) noexcept override; + const bool trimLeft, + const bool lineWrapped) noexcept override; [[nodiscard]] HRESULT PaintBufferGridLines(GridLines const lines, COLORREF const color, size_t const cchLine, COORD const coordTarget) noexcept override; [[nodiscard]] HRESULT PaintSelection(const SMALL_RECT rect) noexcept override; From 86623f57d54ed90bdcb2a829dab90a6ad6e9bb86 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 29 Jan 2020 11:38:07 -0600 Subject: [PATCH 12/39] Add PaintCursor tracing --- src/renderer/vt/paint.cpp | 2 ++ src/renderer/vt/tracing.cpp | 14 ++++++++++++++ src/renderer/vt/tracing.hpp | 1 + 3 files changed, 17 insertions(+) diff --git a/src/renderer/vt/paint.cpp b/src/renderer/vt/paint.cpp index cfa8db395a7..c28cba8464b 100644 --- a/src/renderer/vt/paint.cpp +++ b/src/renderer/vt/paint.cpp @@ -153,6 +153,8 @@ using namespace Microsoft::Console::Types; // - S_OK or suitable HRESULT error from writing pipe. [[nodiscard]] HRESULT VtEngine::PaintCursor(const IRenderEngine::CursorOptions& options) noexcept { + _trace.TracePaintCursor(options.coordCursor); + // MSFT:15933349 - Send the terminal the updated cursor information, if it's changed. LOG_IF_FAILED(_MoveCursor(options.coordCursor)); diff --git a/src/renderer/vt/tracing.cpp b/src/renderer/vt/tracing.cpp index e87b4b5f017..acca1c7be97 100644 --- a/src/renderer/vt/tracing.cpp +++ b/src/renderer/vt/tracing.cpp @@ -241,3 +241,17 @@ void RenderTracing::TraceWrapped() const #else #endif UNIT_TESTING } + +void RenderTracing::TracePaintCursor(const COORD coordCursor) const +{ +#ifndef UNIT_TESTING + const auto cursorPosString = _CoordToString(coordCursor); + const auto cursorPos = cursorPosString.c_str(); + TraceLoggingWrite(g_hConsoleVtRendererTraceProvider, + "VtEngine_TracePaintCursor", + TraceLoggingString(cursorPos), + TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE)); +#else + UNREFERENCED_PARAMETER(coordCursor); +#endif UNIT_TESTING +} diff --git a/src/renderer/vt/tracing.hpp b/src/renderer/vt/tracing.hpp index 9c4ac93f1f0..0fcd1a97aac 100644 --- a/src/renderer/vt/tracing.hpp +++ b/src/renderer/vt/tracing.hpp @@ -31,6 +31,7 @@ namespace Microsoft::Console::VirtualTerminal void TraceLastText(const COORD lastText) const; void TraceMoveCursor(const COORD lastText, const COORD cursor) const; void TraceWrapped() const; + void TracePaintCursor(const COORD coordCursor) const; void TraceInvalidateAll(const Microsoft::Console::Types::Viewport view) const; void TraceTriggerCircling(const bool newFrame) const; void TraceStartPaint(const bool quickReturn, From 7fd5d515b28dfb857732ac2faa65bdd41dd8e026 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 29 Jan 2020 12:52:19 -0600 Subject: [PATCH 13/39] This actually fixes this bug - different terminals EOL wrap differently, esp conhost v wt v gnome-terminal. Remove ambiguity - just hardcore move the cursor in this scenario. --- src/renderer/vt/XtermEngine.cpp | 5 +++++ src/renderer/vt/paint.cpp | 7 ++++++- src/renderer/vt/vtrenderer.hpp | 2 ++ 3 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/renderer/vt/XtermEngine.cpp b/src/renderer/vt/XtermEngine.cpp index 2c1d8a08ed1..4281dcf1c67 100644 --- a/src/renderer/vt/XtermEngine.cpp +++ b/src/renderer/vt/XtermEngine.cpp @@ -242,6 +242,10 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, _needToDisableCursor = true; hr = _CursorHome(); } + else if (_delayedEolWrap) + { + hr = _CursorPosition(coord); + } else if (coord.X == 0 && coord.Y == (_lastText.Y + 1)) { // Down one line, at the start of the line. @@ -298,6 +302,7 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, _newBottomLine = false; } _deferredCursorPos = INVALID_COORDS; + _delayedEolWrap = false; return hr; } diff --git a/src/renderer/vt/paint.cpp b/src/renderer/vt/paint.cpp index 920dc86aeb2..3839c6423ba 100644 --- a/src/renderer/vt/paint.cpp +++ b/src/renderer/vt/paint.cpp @@ -461,11 +461,16 @@ using namespace Microsoft::Console::Types; // we'll determine that we need to emit a \b to put the cursor in the // right position. This is wrong, and will cause us to move the cursor // back one character more than we wanted. - if (_lastText.X < _lastViewport.RightInclusive()) + if (_lastText.X < _lastViewport.RightExclusive()) { _lastText.X += static_cast(columnsActual); } + if (_lastText.X >= _lastViewport.RightInclusive()) + { + _delayedEolWrap = true; + } + short sNumSpaces; try { diff --git a/src/renderer/vt/vtrenderer.hpp b/src/renderer/vt/vtrenderer.hpp index 27658e7f9c8..5e9cdb1e716 100644 --- a/src/renderer/vt/vtrenderer.hpp +++ b/src/renderer/vt/vtrenderer.hpp @@ -144,6 +144,8 @@ namespace Microsoft::Console::Render Microsoft::Console::VirtualTerminal::RenderTracing _trace; bool _inResizeRequest{ false }; + bool _delayedEolWrap{ false }; + [[nodiscard]] HRESULT _Write(std::string_view const str) noexcept; [[nodiscard]] HRESULT _WriteFormattedString(const std::string* const pFormat, ...) noexcept; [[nodiscard]] HRESULT _Flush() noexcept; From 9b6554b10f3f6f679c90097547bd72ad0e06ffb7 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 29 Jan 2020 16:05:53 -0600 Subject: [PATCH 14/39] Add some comments for PR --- src/renderer/vt/XtermEngine.cpp | 18 ++++++++++++++---- src/renderer/vt/paint.cpp | 13 +++++++++++-- 2 files changed, 25 insertions(+), 6 deletions(-) diff --git a/src/renderer/vt/XtermEngine.cpp b/src/renderer/vt/XtermEngine.cpp index 4281dcf1c67..dfbe434f2d2 100644 --- a/src/renderer/vt/XtermEngine.cpp +++ b/src/renderer/vt/XtermEngine.cpp @@ -242,10 +242,6 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, _needToDisableCursor = true; hr = _CursorHome(); } - else if (_delayedEolWrap) - { - hr = _CursorPosition(coord); - } else if (coord.X == 0 && coord.Y == (_lastText.Y + 1)) { // Down one line, at the start of the line. @@ -262,6 +258,20 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, hr = _Write(seq); } } + else if (_delayedEolWrap) + { + // GH#1245, GH#357 - If we were in the delayed EOL wrap state, make + // sure to _manually_ position the cursor now, with a full CUP + // sequence, don't try and be clever with \b or \r or other control + // sequences. Different terminals (conhost, gnome-terminal, wt) all + // behave differently with how the cursor behaves at an end of line. + // This is the only solution that works in all of them, and also + // works wrapped lines emitted by conpty. + // + // Make sure to do this _after_ the possible \r\n branch above, + // otherwise we might accidentally break wrapped lines (GH#405) + hr = _CursorPosition(coord); + } else if (coord.X == 0 && coord.Y == _lastText.Y) { // Start of this line diff --git a/src/renderer/vt/paint.cpp b/src/renderer/vt/paint.cpp index 3839c6423ba..35b0bd66d99 100644 --- a/src/renderer/vt/paint.cpp +++ b/src/renderer/vt/paint.cpp @@ -450,7 +450,7 @@ using namespace Microsoft::Console::Types; RETURN_IF_FAILED(VtEngine::_WriteTerminalUtf8(wstr)); // Update our internal tracker of the cursor's position. - // See MSFT:20266233 + // See MSFT:20266233 (which is also GH#357) // If the cursor is at the rightmost column of the terminal, and we write a // space, the cursor won't actually move to the next cell (which would // be {0, _lastText.Y++}). The cursor will stay visibly in that last @@ -461,11 +461,20 @@ using namespace Microsoft::Console::Types; // we'll determine that we need to emit a \b to put the cursor in the // right position. This is wrong, and will cause us to move the cursor // back one character more than we wanted. + // + // GH#1245: This needs to be RightExclusive, _not_ inclusive. Otherwise, we + // won't update our internal cursor position tracker correctly at the last + // character of the row. if (_lastText.X < _lastViewport.RightExclusive()) { _lastText.X += static_cast(columnsActual); } - + // GH#1245: If we wrote the exactly last char of the row, then we're + // probably in the "delayed EOL wrap" state. Different terminals (conhost, + // gnome-terminal, wt) all behave differently with how the cursor behaves at + // an end of line. Marke that we're in the delayed EOL wrap state - we don't + // want to be clever about how we move the cursor in this state, since + // different terminals will handle a backspace differently in this state. if (_lastText.X >= _lastViewport.RightInclusive()) { _delayedEolWrap = true; From 0a98cceddb79d2b41dd1e45473e30f3a48c7330c Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 29 Jan 2020 16:06:25 -0600 Subject: [PATCH 15/39] Try adding a test, but I can't get the test to repro the original behavior quite right --- .../ConptyRoundtripTests.cpp | 55 +++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index 3f9b7563cce..52e9053d071 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -149,6 +149,8 @@ class TerminalCoreUnitTests::ConptyRoundtripTests final TEST_METHOD(WriteTwoLinesUsesNewline); TEST_METHOD(WriteAFewSimpleLines); + TEST_METHOD(DelayEOLWrap); + private: bool _writeCallback(const char* const pch, size_t const cch); void _flushFirstFrame(); @@ -340,3 +342,56 @@ void ConptyRoundtripTests::WriteAFewSimpleLines() verifyData(termTb); } + +void ConptyRoundtripTests::DelayEOLWrap() +{ + 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(); + + for (auto i = 0; i < TerminalViewWidth; i++) + { + hostSm.ProcessString(L"A"); + } + + auto verifyData0 = [](TextBuffer& tb) { + auto iter = tb.GetCellDataAt({ 0, 0 }); + TestUtils::VerifySpanOfText(L"A", iter, 0, 80); + }; + + verifyData0(hostTb); + + expectedOutput.push_back(std::string(80, 'A')); + + // TODO:GH#405 - When conpty wrapped lines lands, this will behave + // differently, but this has been verified to test it's current behavior, + // and this change works even with conpty wrapped lines. + + expectedOutput.push_back("\x1b[1;80H"); + VERIFY_SUCCEEDED(renderer.PaintFrame()); + + verifyData0(termTb); + + hostSm.ProcessString(L"\b"); + hostSm.ProcessString(L" "); + + auto verifyData1 = [](TextBuffer& tb) { + auto iter = tb.GetCellDataAt({ 0, 0 }); + TestUtils::VerifySpanOfText(L"A", iter, 0, 79); + }; + verifyData1(hostTb); + + // expectedOutput.push_back("\x1b[1;79H"); + expectedOutput.push_back("\b"); + expectedOutput.push_back(" "); + // expectedOutput.push_back("\x1b[1;80H"); + + VERIFY_SUCCEEDED(renderer.PaintFrame()); + verifyData1(termTb); +} From 40b4966782d78ab89b0f40f772c16765fe84b0ef Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 29 Jan 2020 16:06:34 -0600 Subject: [PATCH 16/39] Revert "Try adding a test, but I can't get the test to repro the original behavior quite right" This reverts commit 0a98cceddb79d2b41dd1e45473e30f3a48c7330c. --- .../ConptyRoundtripTests.cpp | 55 ------------------- 1 file changed, 55 deletions(-) diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index 52e9053d071..3f9b7563cce 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -149,8 +149,6 @@ class TerminalCoreUnitTests::ConptyRoundtripTests final TEST_METHOD(WriteTwoLinesUsesNewline); TEST_METHOD(WriteAFewSimpleLines); - TEST_METHOD(DelayEOLWrap); - private: bool _writeCallback(const char* const pch, size_t const cch); void _flushFirstFrame(); @@ -342,56 +340,3 @@ void ConptyRoundtripTests::WriteAFewSimpleLines() verifyData(termTb); } - -void ConptyRoundtripTests::DelayEOLWrap() -{ - 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(); - - for (auto i = 0; i < TerminalViewWidth; i++) - { - hostSm.ProcessString(L"A"); - } - - auto verifyData0 = [](TextBuffer& tb) { - auto iter = tb.GetCellDataAt({ 0, 0 }); - TestUtils::VerifySpanOfText(L"A", iter, 0, 80); - }; - - verifyData0(hostTb); - - expectedOutput.push_back(std::string(80, 'A')); - - // TODO:GH#405 - When conpty wrapped lines lands, this will behave - // differently, but this has been verified to test it's current behavior, - // and this change works even with conpty wrapped lines. - - expectedOutput.push_back("\x1b[1;80H"); - VERIFY_SUCCEEDED(renderer.PaintFrame()); - - verifyData0(termTb); - - hostSm.ProcessString(L"\b"); - hostSm.ProcessString(L" "); - - auto verifyData1 = [](TextBuffer& tb) { - auto iter = tb.GetCellDataAt({ 0, 0 }); - TestUtils::VerifySpanOfText(L"A", iter, 0, 79); - }; - verifyData1(hostTb); - - // expectedOutput.push_back("\x1b[1;79H"); - expectedOutput.push_back("\b"); - expectedOutput.push_back(" "); - // expectedOutput.push_back("\x1b[1;80H"); - - VERIFY_SUCCEEDED(renderer.PaintFrame()); - verifyData1(termTb); -} From 96642deb39d5e58e8b611d5e5925feb3f9af1df4 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 30 Jan 2020 11:10:45 -0600 Subject: [PATCH 17/39] remove other dead code for PR --- src/renderer/vt/XtermEngine.cpp | 1 - src/renderer/vt/XtermEngine.hpp | 1 - 2 files changed, 2 deletions(-) diff --git a/src/renderer/vt/XtermEngine.cpp b/src/renderer/vt/XtermEngine.cpp index fb6ef5e35aa..abf6b9400c0 100644 --- a/src/renderer/vt/XtermEngine.cpp +++ b/src/renderer/vt/XtermEngine.cpp @@ -19,7 +19,6 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, _ColorTable(ColorTable), _cColorTable(cColorTable), _fUseAsciiOnly(fUseAsciiOnly), - // _previousLineWrapped(false), _usingUnderLine(false), _needToDisableCursor(false), _lastCursorIsVisible(false), diff --git a/src/renderer/vt/XtermEngine.hpp b/src/renderer/vt/XtermEngine.hpp index 2d52c89e215..e6a0a868c18 100644 --- a/src/renderer/vt/XtermEngine.hpp +++ b/src/renderer/vt/XtermEngine.hpp @@ -60,7 +60,6 @@ namespace Microsoft::Console::Render const COLORREF* const _ColorTable; const WORD _cColorTable; const bool _fUseAsciiOnly; - bool _previousLineWrapped; bool _usingUnderLine; bool _needToDisableCursor; bool _lastCursorIsVisible; From 74a528357c2da38f02151b76af44264e48a610b0 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 30 Jan 2020 14:28:27 -0600 Subject: [PATCH 18/39] ResizeWithReflow the Terminal buffer --- src/buffer/out/textBuffer.cpp | 12 ++++++-- src/buffer/out/textBuffer.hpp | 2 +- src/cascadia/TerminalCore/Terminal.cpp | 39 ++++++++++++++++++++++++-- 3 files changed, 48 insertions(+), 5 deletions(-) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index 99cdbc78e54..04941fc2448 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -1556,7 +1556,7 @@ std::string TextBuffer::GenRTF(const TextAndColor& rows, const int fontHeightPoi // - newBuffer - the text buffer to copy the contents TO // 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) +HRESULT TextBuffer::Reflow(TextBuffer& oldBuffer, TextBuffer& newBuffer, const std::optional lastCharacterViewport) { Cursor& oldCursor = oldBuffer.GetCursor(); Cursor& newCursor = newBuffer.GetCursor(); @@ -1568,7 +1568,15 @@ HRESULT TextBuffer::Reflow(TextBuffer& oldBuffer, TextBuffer& newBuffer) // place the new cursor back on the equivalent character in // the new buffer. const COORD cOldCursorPos = oldCursor.GetPosition(); - const COORD cOldLastChar = oldBuffer.GetLastNonSpaceCharacter(); + COORD cOldLastChar; + if (lastCharacterViewport.has_value()) + { + cOldLastChar = oldBuffer.GetLastNonSpaceCharacter(lastCharacterViewport.value()); + } + else + { + cOldLastChar = oldBuffer.GetLastNonSpaceCharacter(); + } short const cOldRowsTotal = cOldLastChar.Y + 1; short const cOldColsTotal = oldBuffer.GetSize().Width(); diff --git a/src/buffer/out/textBuffer.hpp b/src/buffer/out/textBuffer.hpp index 4d70bb48303..4c9d2320171 100644 --- a/src/buffer/out/textBuffer.hpp +++ b/src/buffer/out/textBuffer.hpp @@ -158,7 +158,7 @@ class TextBuffer final const std::wstring_view fontFaceName, const COLORREF backgroundColor); - static HRESULT Reflow(TextBuffer& oldBuffer, TextBuffer& newBuffer); + static HRESULT Reflow(TextBuffer& oldBuffer, TextBuffer& newBuffer, const std::optional lastCharacterViewport = std::nullopt); private: std::deque _storage; diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index 750e4b8ae74..0e4f8cc483b 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -180,9 +180,41 @@ void Terminal::UpdateSettings(winrt::Microsoft::Terminal::Settings::ICoreSetting const short newBufferHeight = viewportSize.Y + _scrollbackLines; COORD bufferSize{ viewportSize.X, newBufferHeight }; - RETURN_IF_FAILED(_buffer->ResizeTraditional(bufferSize)); - auto proposedTop = oldTop; + // First allocate a new text buffer to take the place of the current one. + std::unique_ptr newTextBuffer; + try + { + newTextBuffer = std::make_unique(bufferSize, + _buffer->GetCurrentAttributes(), + 0, + _buffer->GetRenderTarget()); // temporarily set size to 0 so it won't render. + } + CATCH_RETURN(); + + // Save cursor's relative height versus the viewport + SHORT const sCursorHeightInViewportBefore = _buffer->GetCursor().GetPosition().Y - _mutableViewport.Top(); + + // RETURN_IF_FAILED(_buffer->ResizeTraditional(bufferSize)); + RETURN_IF_FAILED(TextBuffer::Reflow(*_buffer.get(), *newTextBuffer.get(), _mutableViewport)); + + // 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); + // } + + 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(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 @@ -193,6 +225,9 @@ void Terminal::UpdateSettings(winrt::Microsoft::Terminal::Settings::ICoreSetting } _mutableViewport = Viewport::FromDimensions({ 0, proposedTop }, viewportSize); + + _buffer.swap(newTextBuffer); + _scrollOffset = 0; _NotifyScrollEvent(); From 95807154b856eaf35eaeeef60928183f0e054b2d Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 30 Jan 2020 14:51:58 -0600 Subject: [PATCH 19/39] wait why does this work so well --- src/cascadia/TerminalCore/Terminal.cpp | 124 +++++++++++++++++++------ 1 file changed, 97 insertions(+), 27 deletions(-) diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index 0e4f8cc483b..fcda670e784 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -192,39 +192,109 @@ void Terminal::UpdateSettings(winrt::Microsoft::Terminal::Settings::ICoreSetting } CATCH_RETURN(); - // Save cursor's relative height versus the viewport - SHORT const sCursorHeightInViewportBefore = _buffer->GetCursor().GetPosition().Y - _mutableViewport.Top(); - // RETURN_IF_FAILED(_buffer->ResizeTraditional(bufferSize)); RETURN_IF_FAILED(TextBuffer::Reflow(*_buffer.get(), *newTextBuffer.get(), _mutableViewport)); - // 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); - // } - - 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(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) + // OLD WAY OF FINDING NEW TERMINAL VIEWPORT { - proposedTop -= (proposedBottom - bufferSize.Y); + // 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); } - _mutableViewport = Viewport::FromDimensions({ 0, proposedTop }, viewportSize); + // SCREENINFO WAY OF FINDING NEW TERMINAL VIEWPORT + { + // // 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(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); + + } + + // #4354 way of doing things + { + + // const auto dy = viewportSize.Y - oldDimensions.Y; + // const COORD cOldCursorPos = _buffer->GetCursor().GetPosition(); + // COORD cOldLastChar = cOldCursorPos; + // try + // { + // cOldLastChar = _buffer->GetLastNonSpaceCharacter(); + // } + // CATCH_LOG(); + + // 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(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(proposedTop) }, viewportSize); + } + + // Attempt No. 4 + { + 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(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(proposedTop) }, viewportSize); + } _buffer.swap(newTextBuffer); From edea9a335c1e826dcd2027e3f0124cd5fdc0c3b1 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 30 Jan 2020 15:27:34 -0600 Subject: [PATCH 20/39] Cleanup for review - this is a _great_ fix for #3490 as well as #1465 --- src/buffer/out/textBuffer.cpp | 7 +- src/cascadia/TerminalCore/Terminal.cpp | 124 +++------- .../ConptyRoundtripTests.cpp | 214 ++++++++++++++++++ 3 files changed, 249 insertions(+), 96 deletions(-) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index 04941fc2448..0a6b06ad72d 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -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 lastCharacterViewport) +HRESULT TextBuffer::Reflow(TextBuffer& oldBuffer, + TextBuffer& newBuffer, + const std::optional lastCharacterViewport) { Cursor& oldCursor = oldBuffer.GetCursor(); Cursor& newCursor = newBuffer.GetCursor(); diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index fcda670e784..e6e2bf87a04 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -187,115 +187,49 @@ void Terminal::UpdateSettings(winrt::Microsoft::Terminal::Settings::ICoreSetting { newTextBuffer = std::make_unique(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(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(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(proposedTop) }, viewportSize); - } + auto proposedTop = oldTop + adjustment; - // Attempt No. 4 + const auto newView = Viewport::FromDimensions({ 0, gsl::narrow_cast(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(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(proposedTop) }, viewportSize); + proposedTop -= (proposedBottom - bufferSize.Y); } + _mutableViewport = Viewport::FromDimensions({ 0, gsl::narrow_cast(proposedTop) }, viewportSize); + _buffer.swap(newTextBuffer); _scrollOffset = 0; diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index b6dac60bb92..e3bbdd9be42 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -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(); @@ -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((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(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(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((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(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 last row will have '0'+(50-height+1) + const auto firstChar = static_cast(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(((firstChar + row) % 93) + 33); + auto expectedString = std::wstring(1, static_cast(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(TerminalViewWidth + dx), + gsl::narrow_cast(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); +} From 2e815c895406ae97e3de0cb629ef0bcf36212840 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 31 Jan 2020 17:05:10 -0600 Subject: [PATCH 21/39] fix tests --- .../ConptyRoundtripTests.cpp | 45 ++++++++++++------- 1 file changed, 28 insertions(+), 17 deletions(-) diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index e3bbdd9be42..afcf5bad046 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -777,15 +777,21 @@ void ConptyRoundtripTests::TestResizeHeight() VERIFY_ARE_EQUAL(0, thirdHostView.Top()); VERIFY_ARE_EQUAL(newViewportSize.Y, thirdHostView.BottomExclusive()); - // The Terminal should be stuck to the top of the viewport. + // The Terminal should be stuck to the top of the viewport, unless dy<0, + // rows=50. 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). 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()); + if (dy < 0 && (printedRows > initialTermView.Height() && printedRows < initialTerminalBufferHeight)) + { + VERIFY_ARE_EQUAL(secondTermView.Top() - dy, thirdTermView.Top()); + VERIFY_ARE_EQUAL(expectedTerminalViewBottom, thirdTermView.BottomExclusive()); + } + else + { + VERIFY_ARE_EQUAL(secondTermView.Top(), thirdTermView.Top()); + VERIFY_ARE_EQUAL(expectedTerminalViewBottom + dy, thirdTermView.BottomExclusive()); + } verifyHostData(*hostTb, dy); // Note that at this point, nothing should have changed with the Terminal. @@ -799,16 +805,21 @@ void ConptyRoundtripTests::TestResizeHeight() VERIFY_ARE_EQUAL(0, fourthHostView.Top()); VERIFY_ARE_EQUAL(newViewportSize.Y, fourthHostView.BottomExclusive()); - // The Terminal should be stuck to the top of the viewport. + // The Terminal should be stuck to the top of the viewport, unless dy<0, + // rows=50. 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). 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()); - + if (dy < 0 && (printedRows > initialTermView.Height() && printedRows < initialTerminalBufferHeight)) + { + VERIFY_ARE_EQUAL(secondTermView.Top() - dy, thirdTermView.Top()); + VERIFY_ARE_EQUAL(expectedTerminalViewBottom, thirdTermView.BottomExclusive()); + } + else + { + VERIFY_ARE_EQUAL(secondTermView.Top(), thirdTermView.Top()); + VERIFY_ARE_EQUAL(expectedTerminalViewBottom + dy, thirdTermView.BottomExclusive()); + } verifyHostData(*hostTb, dy); verifyTermData(*termTb, dy); } From 0c91a9b4d47a6b57b0cb458255b3d4878d160fad Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 28 Feb 2020 12:15:32 -0600 Subject: [PATCH 22/39] @ our static analysis build: you're wrong here, but fine --- src/buffer/out/textBuffer.cpp | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index b277c7d138a..5de80e1522e 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -1919,15 +1919,9 @@ HRESULT TextBuffer::Reflow(TextBuffer& oldBuffer, // place the new cursor back on the equivalent character in // the new buffer. const COORD cOldCursorPos = oldCursor.GetPosition(); - COORD cOldLastChar; - if (lastCharacterViewport.has_value()) - { - cOldLastChar = oldBuffer.GetLastNonSpaceCharacter(lastCharacterViewport.value()); - } - else - { - cOldLastChar = oldBuffer.GetLastNonSpaceCharacter(); - } + COORD cOldLastChar = lastCharacterViewport.has_value() ? + oldBuffer.GetLastNonSpaceCharacter(lastCharacterViewport.value()) : + oldBuffer.GetLastNonSpaceCharacter(); short const cOldRowsTotal = cOldLastChar.Y + 1; short const cOldColsTotal = oldBuffer.GetSize().Width(); From 2ef2d88d0a7947d85390086a7f273f97100a2dba Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Mon, 2 Mar 2020 11:13:25 -0600 Subject: [PATCH 23/39] add safemath for carlos --- src/buffer/out/textBuffer.cpp | 10 +++++----- src/cascadia/TerminalCore/Terminal.cpp | 4 ++-- .../ConptyRoundtripTests.cpp | 14 +++++++------- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index 5de80e1522e..2b0e2757db1 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -1919,12 +1919,12 @@ HRESULT TextBuffer::Reflow(TextBuffer& oldBuffer, // place the new cursor back on the equivalent character in // the new buffer. const COORD cOldCursorPos = oldCursor.GetPosition(); - COORD cOldLastChar = lastCharacterViewport.has_value() ? - oldBuffer.GetLastNonSpaceCharacter(lastCharacterViewport.value()) : - oldBuffer.GetLastNonSpaceCharacter(); + const COORD cOldLastChar = lastCharacterViewport.has_value() ? + oldBuffer.GetLastNonSpaceCharacter(lastCharacterViewport.value()) : + oldBuffer.GetLastNonSpaceCharacter(); - short const cOldRowsTotal = cOldLastChar.Y + 1; - short const cOldColsTotal = oldBuffer.GetSize().Width(); + const short cOldRowsTotal = cOldLastChar.Y + 1; + const short cOldColsTotal = oldBuffer.GetSize().Width(); COORD cNewCursorPos = { 0 }; bool fFoundCursorPos = false; diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index 25b98f0f41c..8fca2c37989 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -217,7 +217,7 @@ void Terminal::UpdateSettings(winrt::Microsoft::Terminal::Settings::ICoreSetting auto proposedTop = oldTop + adjustment; - const auto newView = Viewport::FromDimensions({ 0, gsl::narrow_cast(proposedTop) }, viewportSize); + const auto newView = Viewport::FromDimensions({ 0, ::base::ClampedNumeric(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. @@ -226,7 +226,7 @@ void Terminal::UpdateSettings(winrt::Microsoft::Terminal::Settings::ICoreSetting proposedTop -= (proposedBottom - bufferSize.Y); } - _mutableViewport = Viewport::FromDimensions({ 0, gsl::narrow_cast(proposedTop) }, viewportSize); + _mutableViewport = Viewport::FromDimensions({ 0, ::base::ClampedNumeric(proposedTop) }, viewportSize); _buffer.swap(newTextBuffer); diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index b0df89f41e6..a90206afad8 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -738,7 +738,7 @@ void ConptyRoundtripTests::TestResizeHeight() // 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(printedRows + 1), + const auto expectedTerminalViewBottom = std::max(std::min(::base::ClampedNumeric(printedRows + 1), term->GetBufferHeight()), term->GetViewport().Height()); @@ -752,7 +752,7 @@ void ConptyRoundtripTests::TestResizeHeight() const auto numLostRows = std::max(0, printedRows - std::min(term->GetTextBuffer().GetSize().Height(), initialTerminalBufferHeight) + 1); - const auto rowsWithText = std::min(gsl::narrow_cast(printedRows), + const auto rowsWithText = std::min(::base::ClampedNumeric(printedRows), expectedTerminalViewBottom) - 1 + std::min(resizeDy, 0); @@ -790,9 +790,9 @@ void ConptyRoundtripTests::TestResizeHeight() // 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(resizeDy < 0 ? - initialHostView.Height() + resizeDy : - initialHostView.Height()); + const short originalViewHeight = ::base::ClampedNumeric(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 @@ -836,8 +836,8 @@ void ConptyRoundtripTests::TestResizeHeight() verifyTermData(*termTb); const COORD newViewportSize{ - gsl::narrow_cast(TerminalViewWidth + dx), - gsl::narrow_cast(TerminalViewHeight + dy) + ::base::ClampedNumeric(TerminalViewWidth + dx), + ::base::ClampedNumeric(TerminalViewHeight + dy) }; Log::Comment(NoThrowString().Format(L"Resize the Terminal and conpty here")); From cc7b2d34e17e38c86167a18ec28553ce474aa4dd Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Mon, 2 Mar 2020 15:05:17 -0600 Subject: [PATCH 24/39] okay I'll actually build the SA locally --- src/buffer/out/textBuffer.cpp | 2 +- src/buffer/out/textBuffer.hpp | 2 +- src/cascadia/TerminalCore/Terminal.cpp | 8 ++++++-- .../UnitTests_TerminalCore/ConptyRoundtripTests.cpp | 10 +++++----- 4 files changed, 13 insertions(+), 9 deletions(-) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index 2b0e2757db1..bd2c9a6b787 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -1907,7 +1907,7 @@ std::string TextBuffer::GenRTF(const TextAndColor& rows, const int fontHeightPoi // - 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 lastCharacterViewport) + const std::optional lastCharacterViewport) noexcept { Cursor& oldCursor = oldBuffer.GetCursor(); Cursor& newCursor = newBuffer.GetCursor(); diff --git a/src/buffer/out/textBuffer.hpp b/src/buffer/out/textBuffer.hpp index ea5928dcd08..571a40801da 100644 --- a/src/buffer/out/textBuffer.hpp +++ b/src/buffer/out/textBuffer.hpp @@ -162,7 +162,7 @@ class TextBuffer final const std::wstring_view fontFaceName, const COLORREF backgroundColor); - static HRESULT Reflow(TextBuffer& oldBuffer, TextBuffer& newBuffer, const std::optional lastCharacterViewport = std::nullopt); + static HRESULT Reflow(TextBuffer& oldBuffer, TextBuffer& newBuffer, const std::optional lastCharacterViewport = std::nullopt) noexcept; private: std::deque _storage; diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index 8fca2c37989..a988af6fc5d 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -203,12 +203,16 @@ void Terminal::UpdateSettings(winrt::Microsoft::Terminal::Settings::ICoreSetting // move the bottom up. const auto dy = viewportSize.Y - oldDimensions.Y; const COORD oldCursorPos = _buffer->GetCursor().GetPosition(); + +#pragma warning(push) +#pragma warning(disable : 26496) // cpp core checks wants this const, but it's assigned immediately below... COORD oldLastChar = oldCursorPos; try { oldLastChar = _buffer->GetLastNonSpaceCharacter(_mutableViewport); } CATCH_LOG(); +#pragma warning(pop) const auto maxRow = std::max(oldLastChar.Y, oldCursorPos.Y); @@ -217,7 +221,7 @@ void Terminal::UpdateSettings(winrt::Microsoft::Terminal::Settings::ICoreSetting auto proposedTop = oldTop + adjustment; - const auto newView = Viewport::FromDimensions({ 0, ::base::ClampedNumeric(proposedTop) }, viewportSize); + const auto newView = Viewport::FromDimensions({ 0, ::base::saturated_cast(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. @@ -226,7 +230,7 @@ void Terminal::UpdateSettings(winrt::Microsoft::Terminal::Settings::ICoreSetting proposedTop -= (proposedBottom - bufferSize.Y); } - _mutableViewport = Viewport::FromDimensions({ 0, ::base::ClampedNumeric(proposedTop) }, viewportSize); + _mutableViewport = Viewport::FromDimensions({ 0, ::base::saturated_cast(proposedTop) }, viewportSize); _buffer.swap(newTextBuffer); diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index a90206afad8..2da6b3769cf 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -738,7 +738,7 @@ void ConptyRoundtripTests::TestResizeHeight() // 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(::base::ClampedNumeric(printedRows + 1), + const auto expectedTerminalViewBottom = std::max(std::min(::base::saturated_cast(printedRows + 1), term->GetBufferHeight()), term->GetViewport().Height()); @@ -752,7 +752,7 @@ void ConptyRoundtripTests::TestResizeHeight() const auto numLostRows = std::max(0, printedRows - std::min(term->GetTextBuffer().GetSize().Height(), initialTerminalBufferHeight) + 1); - const auto rowsWithText = std::min(::base::ClampedNumeric(printedRows), + const auto rowsWithText = std::min(::base::saturated_cast(printedRows), expectedTerminalViewBottom) - 1 + std::min(resizeDy, 0); @@ -790,7 +790,7 @@ void ConptyRoundtripTests::TestResizeHeight() // 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 = ::base::ClampedNumeric(resizeDy < 0 ? + const short originalViewHeight = ::base::saturated_cast(resizeDy < 0 ? initialHostView.Height() + resizeDy : initialHostView.Height()); const auto rowsWithText = std::min(originalViewHeight - 1, printedRows); @@ -836,8 +836,8 @@ void ConptyRoundtripTests::TestResizeHeight() verifyTermData(*termTb); const COORD newViewportSize{ - ::base::ClampedNumeric(TerminalViewWidth + dx), - ::base::ClampedNumeric(TerminalViewHeight + dy) + ::base::saturated_cast(TerminalViewWidth + dx), + ::base::saturated_cast(TerminalViewHeight + dy) }; Log::Comment(NoThrowString().Format(L"Resize the Terminal and conpty here")); From a8913ced6d8adc3100abc4604e2e07eaa55bb1ee Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 5 Mar 2020 12:27:34 -0600 Subject: [PATCH 25/39] Squash merge of dev/migrie/f/resize-quirk --- src/buffer/out/textBuffer.cpp | 20 ++++- src/buffer/out/textBuffer.hpp | 5 +- src/cascadia/TerminalApp/TerminalPage.cpp | 1 - .../TerminalConnection/ConptyConnection.cpp | 2 +- src/cascadia/TerminalCore/Terminal.cpp | 67 +++++++++++----- .../ConptyRoundtripTests.cpp | 3 + src/host/ConsoleArguments.cpp | 11 +++ src/host/ConsoleArguments.hpp | 4 + src/host/VtIo.cpp | 18 +++++ src/host/VtIo.hpp | 4 + src/host/getset.cpp | 10 ++- src/host/screenInfo.cpp | 12 ++- src/inc/conpty-static.h | 2 + src/renderer/vt/state.cpp | 76 +++++++++++++------ src/renderer/vt/vtrenderer.hpp | 4 + src/winconpty/winconpty.cpp | 4 +- src/winconpty/winconpty.h | 3 + 17 files changed, 193 insertions(+), 53 deletions(-) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index bd2c9a6b787..fb04761b636 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -1903,11 +1903,15 @@ std::string TextBuffer::GenRTF(const TextAndColor& rows, const int fontHeightPoi // - 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. +// - oldViewportTop - Optional. The caller can provide a row in this parameter +// and we'll calculate that row's position in the new buffer. The row's new +// value is placed back into this pointer. // 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 lastCharacterViewport) noexcept + const std::optional lastCharacterViewport, + short* const oldViewportTop) noexcept { Cursor& oldCursor = oldBuffer.GetCursor(); Cursor& newCursor = newBuffer.GetCursor(); @@ -1928,11 +1932,23 @@ HRESULT TextBuffer::Reflow(TextBuffer& oldBuffer, COORD cNewCursorPos = { 0 }; bool fFoundCursorPos = false; - + bool foundOldRow = false; HRESULT hr = S_OK; // Loop through all the rows of the old buffer and reprint them into the new buffer for (short iOldRow = 0; iOldRow < cOldRowsTotal; iOldRow++) { + // If we found the old row that the caller was interested in, set the + // out value of that parameter to the cursor's current Y position (the + // new location of that row in the buffer). + if (oldViewportTop && !foundOldRow) + { + if (iOldRow >= *oldViewportTop) + { + *oldViewportTop = newCursor.GetPosition().Y; + foundOldRow = true; + } + } + // Fetch the row and its "right" which is the last printable character. const ROW& row = oldBuffer.GetRowByOffset(iOldRow); const CharRow& charRow = row.GetCharRow(); diff --git a/src/buffer/out/textBuffer.hpp b/src/buffer/out/textBuffer.hpp index 571a40801da..a9bc71139d6 100644 --- a/src/buffer/out/textBuffer.hpp +++ b/src/buffer/out/textBuffer.hpp @@ -162,7 +162,10 @@ class TextBuffer final const std::wstring_view fontFaceName, const COLORREF backgroundColor); - static HRESULT Reflow(TextBuffer& oldBuffer, TextBuffer& newBuffer, const std::optional lastCharacterViewport = std::nullopt) noexcept; + static HRESULT Reflow(TextBuffer& oldBuffer, + TextBuffer& newBuffer, + const std::optional lastCharacterViewport = std::nullopt, + short* const lastScrollbackRow = nullptr) noexcept; private: std::deque _storage; diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index 78a8caec9f6..10fb7faed39 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -510,7 +510,6 @@ namespace winrt::TerminalApp::implementation // Create a connection based on the values in our settings object. const auto connection = _CreateConnectionFromSettings(profileGuid, settings); - TermControl term{ settings, connection }; // Add the new tab to the list of our tabs. diff --git a/src/cascadia/TerminalConnection/ConptyConnection.cpp b/src/cascadia/TerminalConnection/ConptyConnection.cpp index a42d34ff6e2..1bb1d233b2d 100644 --- a/src/cascadia/TerminalConnection/ConptyConnection.cpp +++ b/src/cascadia/TerminalConnection/ConptyConnection.cpp @@ -201,7 +201,7 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation try { const COORD dimensions{ gsl::narrow_cast(_initialCols), gsl::narrow_cast(_initialRows) }; - THROW_IF_FAILED(_CreatePseudoConsoleAndPipes(dimensions, 0, &_inPipe, &_outPipe, &_hPC)); + THROW_IF_FAILED(_CreatePseudoConsoleAndPipes(dimensions, PSEUDOCONSOLE_RESIZE_QUIRK, &_inPipe, &_outPipe, &_hPC)); THROW_IF_FAILED(_LaunchAttachedClient()); _startTime = std::chrono::high_resolution_clock::now(); diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index a988af6fc5d..189a1c0625f 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -179,6 +179,12 @@ void Terminal::UpdateSettings(winrt::Microsoft::Terminal::Settings::ICoreSetting const short newBufferHeight = viewportSize.Y + _scrollbackLines; COORD bufferSize{ viewportSize.X, newBufferHeight }; + // Save cursor's relative height versus the viewport + const short sCursorHeightInViewportBefore = _buffer->GetCursor().GetPosition().Y - _mutableViewport.Top(); + + // This will be used to determine where the viewport should be in the new buffer. + short oldViewportTop = _mutableViewport.Top(); + // First allocate a new text buffer to take the place of the current one. std::unique_ptr newTextBuffer; try @@ -190,47 +196,68 @@ void Terminal::UpdateSettings(winrt::Microsoft::Terminal::Settings::ICoreSetting } CATCH_RETURN(); - RETURN_IF_FAILED(TextBuffer::Reflow(*_buffer.get(), *newTextBuffer.get(), _mutableViewport)); - - // 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. + RETURN_IF_FAILED(TextBuffer::Reflow(*_buffer.get(), + *newTextBuffer.get(), + _mutableViewport, + &oldViewportTop)); + // 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(); + // viewport, then we're going to need to move the top down. Otherwise, move + // the bottom up. + // + // There are also important things to consider with line wrapping. + // * If a line in scrollback wrapped that didn't previously, we'll need to + // make sure to have the new viewport down another line. This will cause + // our top to move down. + // * If a line _in the viewport_ wrapped that didn't previously, then the + // conpty buffer will also have that wrapped line, and will move the + // cursor & text down a line in response. This causes our bottom to move + // down. + // + // We're going to use a combo of both these things to calculate where the + // new viewport should be. To keep in sync with conpty, we'll need to make + // sure that any lines that entered the scrollback _stay in scrollback_. We + // do that by taking the max of + // * Where the old top line in the viewport exists in the new buffer (as + // calculated by TextBuffer::Reflow) + // * Where the bottom of the text in the new buffer is (and using that to + // calculate another proposed top location). + + const COORD newCursorPos = newTextBuffer->GetCursor().GetPosition(); #pragma warning(push) #pragma warning(disable : 26496) // cpp core checks wants this const, but it's assigned immediately below... - COORD oldLastChar = oldCursorPos; + COORD newLastChar = newCursorPos; try { - oldLastChar = _buffer->GetLastNonSpaceCharacter(_mutableViewport); + newLastChar = newTextBuffer->GetLastNonSpaceCharacter(_mutableViewport); } CATCH_LOG(); #pragma warning(pop) - const auto maxRow = std::max(oldLastChar.Y, oldCursorPos.Y); + const auto maxRow = std::max(newLastChar.Y, newCursorPos.Y); - const bool beforeLastRowOfView = maxRow < _mutableViewport.BottomInclusive(); - const auto adjustment = beforeLastRowOfView ? 0 : std::max(0, -dy); + const short proposedTopFromLastLine = ::base::saturated_cast(maxRow - viewportSize.Y + 1); + const short proposedTopFromScrollback = oldViewportTop; - auto proposedTop = oldTop + adjustment; + short proposedTop = std::max(proposedTopFromLastLine, + proposedTopFromScrollback); - const auto newView = Viewport::FromDimensions({ 0, ::base::saturated_cast(proposedTop) }, viewportSize); + 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); + proposedTop = ::base::saturated_cast(proposedTop - (proposedBottom - bufferSize.Y)); } - _mutableViewport = Viewport::FromDimensions({ 0, ::base::saturated_cast(proposedTop) }, viewportSize); + _mutableViewport = Viewport::FromDimensions({ 0, proposedTop }, viewportSize); _buffer.swap(newTextBuffer); diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index 2da6b3769cf..b446251aea8 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -115,6 +115,9 @@ class TerminalCoreUnitTests::ConptyRoundtripTests final auto pfn = std::bind(&ConptyRoundtripTests::_writeCallback, this, std::placeholders::_1, std::placeholders::_2); _pVtRenderEngine->SetTestCallback(pfn); + // Enable the resize quirk, as the Terminal is going to be reacting as if it's enabled. + _pVtRenderEngine->SetResizeQuirk(true); + // Configure the OutputStateMachine's _pfnFlushToTerminal // Use OutputStateMachineEngine::SetTerminalConnection g.pRender->AddRenderEngine(_pVtRenderEngine.get()); diff --git a/src/host/ConsoleArguments.cpp b/src/host/ConsoleArguments.cpp index 8a18f6df288..84daa6734ae 100644 --- a/src/host/ConsoleArguments.cpp +++ b/src/host/ConsoleArguments.cpp @@ -18,6 +18,7 @@ const std::wstring_view ConsoleArguments::FILEPATH_LEADER_PREFIX = L"\\??\\"; const std::wstring_view ConsoleArguments::WIDTH_ARG = L"--width"; const std::wstring_view ConsoleArguments::HEIGHT_ARG = L"--height"; const std::wstring_view ConsoleArguments::INHERIT_CURSOR_ARG = L"--inheritcursor"; +const std::wstring_view ConsoleArguments::RESIZE_QUIRK = L"--resizeQuirk"; const std::wstring_view ConsoleArguments::FEATURE_ARG = L"--feature"; const std::wstring_view ConsoleArguments::FEATURE_PTY_ARG = L"pty"; @@ -479,6 +480,12 @@ void ConsoleArguments::s_ConsumeArg(_Inout_ std::vector& args, _In s_ConsumeArg(args, i); hr = S_OK; } + else if (arg == RESIZE_QUIRK) + { + _resizeQuirk = true; + s_ConsumeArg(args, i); + hr = S_OK; + } else if (arg == CLIENT_COMMANDLINE_ARG) { // Everything after this is the explicit commandline @@ -611,6 +618,10 @@ bool ConsoleArguments::GetInheritCursor() const { return _inheritCursor; } +bool ConsoleArguments::IsResizeQuirkEnabled() const +{ + return _resizeQuirk; +} // Method Description: // - Tell us to use a different size than the one parsed as the size of the diff --git a/src/host/ConsoleArguments.hpp b/src/host/ConsoleArguments.hpp index e63dbd7badf..248248abb70 100644 --- a/src/host/ConsoleArguments.hpp +++ b/src/host/ConsoleArguments.hpp @@ -50,6 +50,7 @@ class ConsoleArguments short GetWidth() const; short GetHeight() const; bool GetInheritCursor() const; + bool IsResizeQuirkEnabled() const; void SetExpectedSize(COORD dimensions) noexcept; @@ -68,6 +69,7 @@ class ConsoleArguments static const std::wstring_view WIDTH_ARG; static const std::wstring_view HEIGHT_ARG; static const std::wstring_view INHERIT_CURSOR_ARG; + static const std::wstring_view RESIZE_QUIRK; static const std::wstring_view FEATURE_ARG; static const std::wstring_view FEATURE_PTY_ARG; @@ -100,6 +102,7 @@ class ConsoleArguments _serverHandle(serverHandle), _signalHandle(signalHandle), _inheritCursor(inheritCursor), + _resizeQuirk(false), _receivedEarlySizeChange{ false }, _originalWidth{ -1 }, _originalHeight{ -1 } @@ -127,6 +130,7 @@ class ConsoleArguments DWORD _serverHandle; DWORD _signalHandle; bool _inheritCursor; + bool _resizeQuirk{ false }; bool _receivedEarlySizeChange; short _originalWidth; diff --git a/src/host/VtIo.cpp b/src/host/VtIo.cpp index 462771bc9de..6743983fadf 100644 --- a/src/host/VtIo.cpp +++ b/src/host/VtIo.cpp @@ -73,6 +73,7 @@ VtIo::VtIo() : [[nodiscard]] HRESULT VtIo::Initialize(const ConsoleArguments* const pArgs) { _lookingForCursorPosition = pArgs->GetInheritCursor(); + _resizeQuirk = pArgs->IsResizeQuirkEnabled(); // If we were already given VT handles, set up the VT IO engine to use those. if (pArgs->InConptyMode()) @@ -192,6 +193,7 @@ VtIo::VtIo() : if (_pVtRenderEngine) { _pVtRenderEngine->SetTerminalOwner(this); + _pVtRenderEngine->SetResizeQuirk(_resizeQuirk); } } } @@ -446,3 +448,19 @@ void VtIo::EnableConptyModeForTests() _objectsCreated = true; } #endif + +// Method Description: +// - Returns true if the Resize Quirk is enabled. This changes the behavior of +// conpty to _not_ InvalidateAll the entire viewport on a resize operation. +// This is used by the Windows Terminal, because it is prepared to be +// connected to a conpty, and handles it's own buffer specifically for a +// conpty scenario. +// - See also: GH#3490, #4354, #4741 +// Arguments: +// - +// Return Value: +// - true iff we were started with the `--resizeQuirk` flag enabled. +bool VtIo::IsResizeQuirkEnabled() const +{ + return _resizeQuirk; +} diff --git a/src/host/VtIo.hpp b/src/host/VtIo.hpp index b2600c10270..37186ef0c85 100644 --- a/src/host/VtIo.hpp +++ b/src/host/VtIo.hpp @@ -43,6 +43,8 @@ namespace Microsoft::Console::VirtualTerminal void EnableConptyModeForTests(); #endif + bool IsResizeQuirkEnabled() const; + private: // After CreateIoHandlers is called, these will be invalid. wil::unique_hfile _hInput; @@ -57,6 +59,8 @@ namespace Microsoft::Console::VirtualTerminal bool _lookingForCursorPosition; std::mutex _shutdownLock; + bool _resizeQuirk{ false }; + std::unique_ptr _pVtRenderEngine; std::unique_ptr _pVtInputThread; std::unique_ptr _pPtySignalInputThread; diff --git a/src/host/getset.cpp b/src/host/getset.cpp index cba5bc3a12e..da725dc66c7 100644 --- a/src/host/getset.cpp +++ b/src/host/getset.cpp @@ -776,7 +776,15 @@ void ApiRoutines::GetLargestConsoleWindowSizeImpl(const SCREEN_INFORMATION& cont { // TODO: MSFT: 9574827 - shouldn't we be looking at or at least logging the failure codes here? (Or making them non-void?) context.PostUpdateWindowSize(); - WriteToScreen(context, context.GetViewport()); + + // Use WriteToScreen to invalidate the viewport with the renderer. + // GH#3490 - If we're in conpty mode, don't invalidate the entire + // viewport. In conpty mode, the VtEngine will later decide what + // part of the buffer actually needs to be re-sent to the terminal. + if (!(g.getConsoleInformation().IsInVtIoMode() && g.getConsoleInformation().GetVtIo()->IsResizeQuirkEnabled())) + { + WriteToScreen(context, context.GetViewport()); + } } return S_OK; } diff --git a/src/host/screenInfo.cpp b/src/host/screenInfo.cpp index 8437d4999fc..d468595b226 100644 --- a/src/host/screenInfo.cpp +++ b/src/host/screenInfo.cpp @@ -1428,6 +1428,9 @@ bool SCREEN_INFORMATION::IsMaximizedY() const _textBuffer.swap(newTextBuffer); } + // const auto cursorPos = _textBuffer->GetCursor().GetPosition(); + // GetRenderTarget().TriggerRedrawCursor(&cursorPos); + return NTSTATUS_FROM_HRESULT(hr); } @@ -2176,8 +2179,13 @@ void SCREEN_INFORMATION::SetDefaultAttributes(const TextAttribute& attributes, commandLine.UpdatePopups(attributes, popupAttributes, oldPrimaryAttributes, oldPopupAttributes); } - // force repaint of entire viewport - GetRenderTarget().TriggerRedrawAll(); + // Force repaint of entire viewport, unless we're in conpty mode. In that + // case, we don't really need to force a redraw of the entire screen just + // because the text attributes changed. + if (!(gci.IsInVtIoMode())) + { + GetRenderTarget().TriggerRedrawAll(); + } gci.ConsoleIme.RefreshAreaAttributes(); diff --git a/src/inc/conpty-static.h b/src/inc/conpty-static.h index 7f85b22eddf..a6561b7b30f 100644 --- a/src/inc/conpty-static.h +++ b/src/inc/conpty-static.h @@ -15,6 +15,8 @@ extern "C" { #endif +#define PSEUDOCONSOLE_RESIZE_QUIRK (2u) + HRESULT WINAPI ConptyCreatePseudoConsole(COORD size, HANDLE hInput, HANDLE hOutput, DWORD dwFlags, HPCON* phPC); HRESULT WINAPI ConptyResizePseudoConsole(HPCON hPC, COORD size); diff --git a/src/renderer/vt/state.cpp b/src/renderer/vt/state.cpp index 705a5fd1fb9..02fa842644d 100644 --- a/src/renderer/vt/state.cpp +++ b/src/renderer/vt/state.cpp @@ -278,38 +278,50 @@ VtEngine::VtEngine(_In_ wil::unique_hfile pipe, // lead to the first _actual_ resize being suppressed. _suppressResizeRepaint = false; - if (SUCCEEDED(hr)) + if (_resizeQuirk) { - // Viewport is smaller now - just update it all. - if (oldView.Height() > newView.Height() || oldView.Width() > newView.Width()) - { - hr = InvalidateAll(); - } - else + // GH#3490 - When the viewport width changed, don't do anything extra here. + // If the buffer had areas that were invalid due to the resize, then the + // buffer will have triggered it's own invalidations for what it knows is + // invalid. Previously, we'd invalidate everything if the width changed, + // because we couldn't be sure if lines were reflowed. + } + else + { + if (SUCCEEDED(hr)) { - // At least one of the directions grew. - // First try and add everything to the right of the old viewport, - // then everything below where the old viewport ended. - if (oldView.Width() < newView.Width()) + // Viewport is smaller now - just update it all. + if (oldView.Height() > newView.Height() || oldView.Width() > newView.Width()) { - short left = oldView.RightExclusive(); - short top = 0; - short right = newView.RightInclusive(); - short bottom = oldView.BottomInclusive(); - Viewport rightOfOldViewport = Viewport::FromInclusive({ left, top, right, bottom }); - hr = _InvalidCombine(rightOfOldViewport); + hr = InvalidateAll(); } - if (SUCCEEDED(hr) && oldView.Height() < newView.Height()) + else { - short left = 0; - short top = oldView.BottomExclusive(); - short right = newView.RightInclusive(); - short bottom = newView.BottomInclusive(); - Viewport belowOldViewport = Viewport::FromInclusive({ left, top, right, bottom }); - hr = _InvalidCombine(belowOldViewport); + // At least one of the directions grew. + // First try and add everything to the right of the old viewport, + // then everything below where the old viewport ended. + if (oldView.Width() < newView.Width()) + { + short left = oldView.RightExclusive(); + short top = 0; + short right = newView.RightInclusive(); + short bottom = oldView.BottomInclusive(); + Viewport rightOfOldViewport = Viewport::FromInclusive({ left, top, right, bottom }); + hr = _InvalidCombine(rightOfOldViewport); + } + if (SUCCEEDED(hr) && oldView.Height() < newView.Height()) + { + short left = 0; + short top = oldView.BottomExclusive(); + short right = newView.RightInclusive(); + short bottom = newView.BottomInclusive(); + Viewport belowOldViewport = Viewport::FromInclusive({ left, top, right, bottom }); + hr = _InvalidCombine(belowOldViewport); + } } } } + _resized = true; return hr; } @@ -456,3 +468,19 @@ void VtEngine::EndResizeRequest() { _inResizeRequest = false; } + +// Method Description: +// - Configure the renderer for the resize quirk. This changes the behavior of +// conpty to _not_ InvalidateAll the entire viewport on a resize operation. +// This is used by the Windows Terminal, because it is prepared to be +// connected to a conpty, and handles it's own buffer specifically for a +// conpty scenario. +// - See also: GH#3490, #4354, #4741 +// Arguments: +// - +// Return Value: +// - true iff we were started with the `--resizeQuirk` flag enabled. +void VtEngine::SetResizeQuirk(const bool resizeQuirk) +{ + _resizeQuirk = resizeQuirk; +} diff --git a/src/renderer/vt/vtrenderer.hpp b/src/renderer/vt/vtrenderer.hpp index b3a4c47deab..14bb23a04dc 100644 --- a/src/renderer/vt/vtrenderer.hpp +++ b/src/renderer/vt/vtrenderer.hpp @@ -106,6 +106,8 @@ namespace Microsoft::Console::Render void BeginResizeRequest(); void EndResizeRequest(); + void SetResizeQuirk(const bool resizeQuirk); + protected: wil::unique_hfile _hFile; std::string _buffer; @@ -149,6 +151,8 @@ namespace Microsoft::Console::Render bool _delayedEolWrap{ false }; + bool _resizeQuirk{ false }; + [[nodiscard]] HRESULT _Write(std::string_view const str) noexcept; [[nodiscard]] HRESULT _WriteFormattedString(const std::string* const pFormat, ...) noexcept; [[nodiscard]] HRESULT _Flush() noexcept; diff --git a/src/winconpty/winconpty.cpp b/src/winconpty/winconpty.cpp index be440ce00e3..fef6da1529c 100644 --- a/src/winconpty/winconpty.cpp +++ b/src/winconpty/winconpty.cpp @@ -83,15 +83,17 @@ HRESULT _CreatePseudoConsole(const HANDLE hToken, RETURN_IF_WIN32_BOOL_FALSE(SetHandleInformation(signalPipeConhostSide.get(), HANDLE_FLAG_INHERIT, HANDLE_FLAG_INHERIT)); // GH4061: Ensure that the path to executable in the format is escaped so C:\Program.exe cannot collide with C:\Program Files - const wchar_t* pwszFormat = L"\"%s\" --headless %s--width %hu --height %hu --signal 0x%x --server 0x%x"; + const wchar_t* pwszFormat = L"\"%s\" --headless %s%s --width %hu --height %hu --signal 0x%x --server 0x%x"; // This is plenty of space to hold the formatted string wchar_t cmd[MAX_PATH]{}; const BOOL bInheritCursor = (dwFlags & PSEUDOCONSOLE_INHERIT_CURSOR) == PSEUDOCONSOLE_INHERIT_CURSOR; + const BOOL bResizeQuirk = (dwFlags & PSEUDOCONSOLE_RESIZE_QUIRK) == PSEUDOCONSOLE_RESIZE_QUIRK; swprintf_s(cmd, MAX_PATH, pwszFormat, _ConsoleHostPath(), bInheritCursor ? L"--inheritcursor " : L"", + bResizeQuirk ? L"--resizeQuirk" : L"", size.X, size.Y, signalPipeConhostSide.get(), diff --git a/src/winconpty/winconpty.h b/src/winconpty/winconpty.h index 829befc5473..30a2354f3e7 100644 --- a/src/winconpty/winconpty.h +++ b/src/winconpty/winconpty.h @@ -19,6 +19,9 @@ typedef struct _PseudoConsole // the signal pipe. #define PTY_SIGNAL_RESIZE_WINDOW (8u) +// The other flag (PSEUDOCONSOLE_INHERIT_CURSOR) is actually defined in consoleapi.h in the OS repo +#define PSEUDOCONSOLE_RESIZE_QUIRK (2u) + // Implementations of the various PseudoConsole functions. HRESULT _CreatePseudoConsole(const HANDLE hToken, const COORD size, From e0626c81011edd881b908d7bad31950ff4061761 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 5 Mar 2020 12:56:48 -0600 Subject: [PATCH 26/39] audit mode is hard sometimes --- src/buffer/out/textBuffer.cpp | 2 +- src/buffer/out/textBuffer.hpp | 2 +- src/cascadia/TerminalCore/Terminal.cpp | 10 +++++----- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index fb04761b636..7a9fc2c3a0a 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -1911,7 +1911,7 @@ std::string TextBuffer::GenRTF(const TextAndColor& rows, const int fontHeightPoi HRESULT TextBuffer::Reflow(TextBuffer& oldBuffer, TextBuffer& newBuffer, const std::optional lastCharacterViewport, - short* const oldViewportTop) noexcept + short* const oldViewportTop) { Cursor& oldCursor = oldBuffer.GetCursor(); Cursor& newCursor = newBuffer.GetCursor(); diff --git a/src/buffer/out/textBuffer.hpp b/src/buffer/out/textBuffer.hpp index a9bc71139d6..09c2b3ebc83 100644 --- a/src/buffer/out/textBuffer.hpp +++ b/src/buffer/out/textBuffer.hpp @@ -165,7 +165,7 @@ class TextBuffer final static HRESULT Reflow(TextBuffer& oldBuffer, TextBuffer& newBuffer, const std::optional lastCharacterViewport = std::nullopt, - short* const lastScrollbackRow = nullptr) noexcept; + short* const lastScrollbackRow = nullptr); private: std::deque _storage; diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index 189a1c0625f..197c4cb4e8d 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -193,14 +193,14 @@ void Terminal::UpdateSettings(winrt::Microsoft::Terminal::Settings::ICoreSetting _buffer->GetCurrentAttributes(), 0, // temporarily set size to 0 so it won't render. _buffer->GetRenderTarget()); + + RETURN_IF_FAILED(TextBuffer::Reflow(*_buffer.get(), + *newTextBuffer.get(), + _mutableViewport, + &oldViewportTop)); } CATCH_RETURN(); - RETURN_IF_FAILED(TextBuffer::Reflow(*_buffer.get(), - *newTextBuffer.get(), - _mutableViewport, - &oldViewportTop)); - // 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 From 4c368c3452cfd457a5e65a3819cc35428ee97a61 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 5 Mar 2020 16:35:15 -0600 Subject: [PATCH 27/39] delete some old dead code --- src/host/screenInfo.cpp | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/host/screenInfo.cpp b/src/host/screenInfo.cpp index d468595b226..e4213771d09 100644 --- a/src/host/screenInfo.cpp +++ b/src/host/screenInfo.cpp @@ -1428,9 +1428,6 @@ bool SCREEN_INFORMATION::IsMaximizedY() const _textBuffer.swap(newTextBuffer); } - // const auto cursorPos = _textBuffer->GetCursor().GetPosition(); - // GetRenderTarget().TriggerRedrawCursor(&cursorPos); - return NTSTATUS_FROM_HRESULT(hr); } From 607009525201c9183504cd11487c91d90c7ddf37 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 6 Mar 2020 14:16:55 -0600 Subject: [PATCH 28/39] Fix wrapping lines far too frequently --- src/cascadia/TerminalCore/Terminal.cpp | 11 ++++------- src/cascadia/TerminalCore/TerminalApi.cpp | 5 +++++ 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index 197c4cb4e8d..d2d79d8400d 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -526,18 +526,15 @@ void Terminal::_WriteBuffer(const std::wstring_view& stringView) // Try the character again. i--; - // Mark the line we're currently on as wrapped + // If we write the last cell of the row here, TextBuffer::Write will + // mark this line as wrapped for us. If the next character we + // process is a newline, the Terminal::CursorLineFeed will unmark + // this line 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); diff --git a/src/cascadia/TerminalCore/TerminalApi.cpp b/src/cascadia/TerminalCore/TerminalApi.cpp index 3a6a543be07..f8cc24021ad 100644 --- a/src/cascadia/TerminalCore/TerminalApi.cpp +++ b/src/cascadia/TerminalCore/TerminalApi.cpp @@ -142,6 +142,11 @@ bool Terminal::CursorLineFeed(const bool withReturn) noexcept try { auto cursorPos = _buffer->GetCursor().GetPosition(); + + // since we explicitly just moved down a row, clear the wrap status on the + // row we just came from + _buffer->GetRowByOffset(cursorPos.Y).GetCharRow().SetWrapForced(false); + cursorPos.Y++; if (withReturn) { From 7cca547eeeb9024ac75a3ec35e707ac039c9f8d4 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 6 Mar 2020 14:26:36 -0600 Subject: [PATCH 29/39] Some minor comments from Michael --- src/winconpty/winconpty.cpp | 4 ++-- src/winconpty/winconpty.h | 4 +++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/winconpty/winconpty.cpp b/src/winconpty/winconpty.cpp index fef6da1529c..ed250b55a6e 100644 --- a/src/winconpty/winconpty.cpp +++ b/src/winconpty/winconpty.cpp @@ -83,7 +83,7 @@ HRESULT _CreatePseudoConsole(const HANDLE hToken, RETURN_IF_WIN32_BOOL_FALSE(SetHandleInformation(signalPipeConhostSide.get(), HANDLE_FLAG_INHERIT, HANDLE_FLAG_INHERIT)); // GH4061: Ensure that the path to executable in the format is escaped so C:\Program.exe cannot collide with C:\Program Files - const wchar_t* pwszFormat = L"\"%s\" --headless %s%s --width %hu --height %hu --signal 0x%x --server 0x%x"; + const wchar_t* pwszFormat = L"\"%s\" --headless %s%s--width %hu --height %hu --signal 0x%x --server 0x%x"; // This is plenty of space to hold the formatted string wchar_t cmd[MAX_PATH]{}; const BOOL bInheritCursor = (dwFlags & PSEUDOCONSOLE_INHERIT_CURSOR) == PSEUDOCONSOLE_INHERIT_CURSOR; @@ -93,7 +93,7 @@ HRESULT _CreatePseudoConsole(const HANDLE hToken, pwszFormat, _ConsoleHostPath(), bInheritCursor ? L"--inheritcursor " : L"", - bResizeQuirk ? L"--resizeQuirk" : L"", + bResizeQuirk ? L"--resizeQuirk " : L"", size.X, size.Y, signalPipeConhostSide.get(), diff --git a/src/winconpty/winconpty.h b/src/winconpty/winconpty.h index 30a2354f3e7..53c2279a3f2 100644 --- a/src/winconpty/winconpty.h +++ b/src/winconpty/winconpty.h @@ -19,8 +19,10 @@ typedef struct _PseudoConsole // the signal pipe. #define PTY_SIGNAL_RESIZE_WINDOW (8u) +// CreatePseudoConsole Flags // The other flag (PSEUDOCONSOLE_INHERIT_CURSOR) is actually defined in consoleapi.h in the OS repo -#define PSEUDOCONSOLE_RESIZE_QUIRK (2u) +// #define PSEUDOCONSOLE_INHERIT_CURSOR (0x1) +#define PSEUDOCONSOLE_RESIZE_QUIRK (0x2) // Implementations of the various PseudoConsole functions. HRESULT _CreatePseudoConsole(const HANDLE hToken, From 277f280ed6436a0b68c292a204fa162a7acbc100 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Tue, 10 Mar 2020 14:38:25 -0500 Subject: [PATCH 30/39] Enable conpty to be resized even when headed --- src/host/getset.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/host/getset.cpp b/src/host/getset.cpp index da725dc66c7..0688d20a209 100644 --- a/src/host/getset.cpp +++ b/src/host/getset.cpp @@ -759,16 +759,16 @@ void ApiRoutines::GetLargestConsoleWindowSizeImpl(const SCREEN_INFORMATION& cont // if we're headless, not so much. However, GetMaxWindowSizeInCharacters // will only return the buffer size, so we can't use that to clip the arg here. // So only clip the requested size if we're not headless + if (g.getConsoleInformation().IsInVtIoMode()) + { + // SetViewportRect doesn't cause the buffer to resize. Manually resize the buffer. + RETURN_IF_NTSTATUS_FAILED(context.ResizeScreenBuffer(Viewport::FromInclusive(Window).Dimensions(), false)); + } if (!g.IsHeadless()) { COORD const coordMax = context.GetMaxWindowSizeInCharacters(); RETURN_HR_IF(E_INVALIDARG, (NewWindowSize.X > coordMax.X || NewWindowSize.Y > coordMax.Y)); } - else if (g.getConsoleInformation().IsInVtIoMode()) - { - // SetViewportRect doesn't cause the buffer to resize. Manually resize the buffer. - RETURN_IF_NTSTATUS_FAILED(context.ResizeScreenBuffer(Viewport::FromInclusive(Window).Dimensions(), false)); - } // Even if it's the same size, we need to post an update in case the scroll bars need to go away. context.SetViewport(Viewport::FromInclusive(Window), true); From 7a40fe33347421030d1b26caed9e65b37af7a224 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Tue, 10 Mar 2020 14:39:27 -0500 Subject: [PATCH 31/39] These bugs were twofold one was weird: after the reflow, the conpty would know that the cursor was in a different position than the last time it wrote text, so it would emit a sequence to position the cursor. However, with quirky resize, the terminal's cursor actually _was_ in the right position. So when conpty would emit a \n to move the cursor one line down, that would cause the Terminal to be out of sync. The second: we were calculating the new position of the top row of the buffer using the first cell of the row, not the last. This meant that if the top row wrapped in this new resize, we'd place the top of the new viewport one row higher than conpty would --- src/buffer/out/textBuffer.cpp | 25 +++++++++++++------------ src/renderer/vt/XtermEngine.cpp | 4 ++++ 2 files changed, 17 insertions(+), 12 deletions(-) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index 62e98fef2be..653aaea0f30 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -1909,18 +1909,6 @@ HRESULT TextBuffer::Reflow(TextBuffer& oldBuffer, // Loop through all the rows of the old buffer and reprint them into the new buffer for (short iOldRow = 0; iOldRow < cOldRowsTotal; iOldRow++) { - // If we found the old row that the caller was interested in, set the - // out value of that parameter to the cursor's current Y position (the - // new location of that row in the buffer). - if (oldViewportTop && !foundOldRow) - { - if (iOldRow >= *oldViewportTop) - { - *oldViewportTop = newCursor.GetPosition().Y; - foundOldRow = true; - } - } - // Fetch the row and its "right" which is the last printable character. const ROW& row = oldBuffer.GetRowByOffset(iOldRow); const CharRow& charRow = row.GetCharRow(); @@ -1976,6 +1964,19 @@ HRESULT TextBuffer::Reflow(TextBuffer& oldBuffer, } CATCH_RETURN(); } + + // If we found the old row that the caller was interested in, set the + // out value of that parameter to the cursor's current Y position (the + // new location of that row in the buffer). + if (oldViewportTop && !foundOldRow) + { + if (iOldRow >= *oldViewportTop) + { + *oldViewportTop = newCursor.GetPosition().Y; + foundOldRow = true; + } + } + if (SUCCEEDED(hr)) { // If we didn't have a full row to copy, insert a new diff --git a/src/renderer/vt/XtermEngine.cpp b/src/renderer/vt/XtermEngine.cpp index 3a8491c7204..cfc59d5e7f4 100644 --- a/src/renderer/vt/XtermEngine.cpp +++ b/src/renderer/vt/XtermEngine.cpp @@ -243,6 +243,10 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, _needToDisableCursor = true; hr = _CursorHome(); } + else if (_resized && _resizeQuirk) + { + hr = _CursorPosition(coord); + } else if (coord.X == 0 && coord.Y == (_lastText.Y + 1)) { // Down one line, at the start of the line. From a3a946465fd978ec8b44ce41040efb09cdeeaff1 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Tue, 10 Mar 2020 16:03:19 -0500 Subject: [PATCH 32/39] When we increase the width of the buffer, always use the old viewport top to calculate our new position The trick with this case was that when there were multiple wrapped lines inside the viewport, the lastLine of the buffer would be farther down than we actually wanted. We'd still want the position of the top row in the buffer here, because the lines in the buffer could have _only_ de-wrapped, taking up less lines than before. This is however, _not_ right. When you execute `ll`, then increase the height, then decrease the width, we'll snape the bottom of the viewport to the bottom of the output text, as opposed to pinning to the top of the text. --- src/cascadia/TerminalCore/Terminal.cpp | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index d2d79d8400d..a63b655527f 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -173,6 +173,10 @@ void Terminal::UpdateSettings(winrt::Microsoft::Terminal::Settings::ICoreSetting { return S_FALSE; } + const auto dx = viewportSize.X - oldDimensions.X; + const auto dy = viewportSize.Y - oldDimensions.Y; + dx; + dy; const auto oldTop = _mutableViewport.Top(); @@ -248,6 +252,15 @@ void Terminal::UpdateSettings(winrt::Microsoft::Terminal::Settings::ICoreSetting short proposedTop = std::max(proposedTopFromLastLine, proposedTopFromScrollback); + if (dx < 0) + { + proposedTop = proposedTopFromLastLine; + } + else if (dx > 0) + { + proposedTop = proposedTopFromScrollback; + } + 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 From 4ca280cad46d6fd57fbc2c5585f7534d49a057d4 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Tue, 10 Mar 2020 16:10:29 -0500 Subject: [PATCH 33/39] When we change the width at all, use the top, not the bottom. This fixes the bug from before. This breaks when we simultaneously change width and height. Scenario is powershell, `type rwr-temp.txt`. Increase font size. Viewport is not at the actual bottom of the buffer anymore -.- I think we need to use the scrollback value unless we're decreasing the height of the buffer as well --- src/cascadia/TerminalCore/Terminal.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index a63b655527f..faea75d8974 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -252,14 +252,14 @@ void Terminal::UpdateSettings(winrt::Microsoft::Terminal::Settings::ICoreSetting short proposedTop = std::max(proposedTopFromLastLine, proposedTopFromScrollback); - if (dx < 0) - { - proposedTop = proposedTopFromLastLine; - } - else if (dx > 0) + if (dx != 0) { proposedTop = proposedTopFromScrollback; } + // else if (dx > 0) + // { + // proposedTop = proposedTopFromScrollback; + // } const auto newView = Viewport::FromDimensions({ 0, proposedTop }, viewportSize); const auto proposedBottom = newView.BottomExclusive(); From 077f67835eb8797efd17bc4c30167e834de41c99 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Tue, 10 Mar 2020 16:48:36 -0500 Subject: [PATCH 34/39] This seems like it finally does it If the terminal height is getting smaller, always use the lastline to figure out where the new bottom line should be oh no that's not going to work is it. what happens with dy<0, dx>0? * dy<0, dx<0: - The Terminal is getting shorter, buffer less wide. Use the end of the text to find where we should place the viewport, since lines might have newly wrapped * dy<0, dx>0: - The Terminal is getting shorter, buffer wider. ?? Use the old top to find the new position. Lines migh de-wrap, and we'll want to stay stuck to the topmost line of the old buffer. ?? * dy>0, dx<0: - The Terminal is getting taller, buffer less wide. * dy>0, dx>0: - The Terminal is getting taller, buffer wider. Use the old top to find the new position. Lines migh de-wrap, and we'll want to stay stuck to the topmost line of the old buffer. --- src/cascadia/TerminalCore/Terminal.cpp | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index faea75d8974..412235b0f97 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -252,15 +252,25 @@ void Terminal::UpdateSettings(winrt::Microsoft::Terminal::Settings::ICoreSetting short proposedTop = std::max(proposedTopFromLastLine, proposedTopFromScrollback); - if (dx != 0) + if (dy < 0) + { + proposedTop = proposedTopFromLastLine; + } + else if (dx != 0 || dy > 0) { proposedTop = proposedTopFromScrollback; } - // else if (dx > 0) + /*else if (dy < 0) + { + proposedTop = proposedTopFromLastLine; + }*/ + // else if (dx < 0) // { - // proposedTop = proposedTopFromScrollback; + // proposedTop = proposedTopFromLastLine; // } + proposedTop = std::max(static_cast(0), proposedTop); + 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 From 85efaa7f949ca4377dcfd691428b7a668673c18c Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 11 Mar 2020 16:13:55 -0500 Subject: [PATCH 35/39] git merge --squash dev/migrie/f/more-reflow-experiments --- src/buffer/out/textBuffer.cpp | 6 +-- src/cascadia/TerminalCore/Terminal.cpp | 57 +++++++++++++++++--------- 2 files changed, 41 insertions(+), 22 deletions(-) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index 653aaea0f30..945945e8a1e 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -1876,8 +1876,8 @@ std::string TextBuffer::GenRTF(const TextAndColor& rows, const int fontHeightPoi // nonspace character is in a particular Viewport, the caller can provide this // parameter as an optimization, as opposed to searching the entire buffer. // - oldViewportTop - Optional. The caller can provide a row in this parameter -// and we'll calculate that row's position in the new buffer. The row's new -// value is placed back into this pointer. +// and we'll calculate the position of the _end_ of that row in the new +// buffer. The row's new value is placed back into this pointer. // Return Value: // - S_OK if we successfully copied the contents to the new buffer, otherwise an appropriate HRESULT. HRESULT TextBuffer::Reflow(TextBuffer& oldBuffer, @@ -1967,7 +1967,7 @@ HRESULT TextBuffer::Reflow(TextBuffer& oldBuffer, // If we found the old row that the caller was interested in, set the // out value of that parameter to the cursor's current Y position (the - // new location of that row in the buffer). + // new location of the _end_ of that row in the buffer). if (oldViewportTop && !foundOldRow) { if (iOldRow >= *oldViewportTop) diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index 412235b0f97..76732f8ded6 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -174,9 +174,6 @@ void Terminal::UpdateSettings(winrt::Microsoft::Terminal::Settings::ICoreSetting return S_FALSE; } const auto dx = viewportSize.X - oldDimensions.X; - const auto dy = viewportSize.Y - oldDimensions.Y; - dx; - dy; const auto oldTop = _mutableViewport.Top(); @@ -233,13 +230,12 @@ void Terminal::UpdateSettings(winrt::Microsoft::Terminal::Settings::ICoreSetting // calculate another proposed top location). const COORD newCursorPos = newTextBuffer->GetCursor().GetPosition(); - #pragma warning(push) #pragma warning(disable : 26496) // cpp core checks wants this const, but it's assigned immediately below... COORD newLastChar = newCursorPos; try { - newLastChar = newTextBuffer->GetLastNonSpaceCharacter(_mutableViewport); + newLastChar = newTextBuffer->GetLastNonSpaceCharacter(); } CATCH_LOG(); #pragma warning(pop) @@ -252,29 +248,52 @@ void Terminal::UpdateSettings(winrt::Microsoft::Terminal::Settings::ICoreSetting short proposedTop = std::max(proposedTopFromLastLine, proposedTopFromScrollback); - if (dy < 0) - { - proposedTop = proposedTopFromLastLine; - } - else if (dx != 0 || dy > 0) + // If we're using the new location of the old top line to place the + // viewport, we might need to make an adjustment to it. + // + // We're using the last cell of the line to calculate where the top line is + // in the new buffer. If that line wrapped, then all the lines below it + // shifted down in the buffer. If there's space for all those lines in the + // conpty buffer, then the originally unwrapped top line will _still_ be in + // the buffer. In that case, don't stick to the _end_ of the old top line, + // instead stick to the _start_, which is one line up. + // + // We can know if there's space in the conpty buffer by checking if the + // maxRow (the highest row we've written text to) is above the viewport from + // this proposed top position. + if (proposedTop == proposedTopFromScrollback) { - proposedTop = proposedTopFromScrollback; + const auto proposedViewFromTop = Viewport::FromDimensions({ 0, proposedTopFromScrollback }, viewportSize); + if (maxRow < proposedViewFromTop.BottomInclusive()) + { + if (dx < 0 && proposedTop > 0) + { + auto row = newTextBuffer->GetRowByOffset(proposedTop - 1); + if (row.GetCharRow().WasWrapForced()) + { + proposedTop--; + } + } + } } - /*else if (dy < 0) + + // If the new bottom would be higher than the last row of text, then we + // definitely want to use the last row of text to determine where the + // viewport should be. + const auto proposedViewFromTop = Viewport::FromDimensions({ 0, proposedTopFromScrollback }, viewportSize); + if (maxRow > proposedViewFromTop.BottomInclusive()) { proposedTop = proposedTopFromLastLine; - }*/ - // else if (dx < 0) - // { - // proposedTop = proposedTopFromLastLine; - // } + } + // Make sure the proposed viewport is within the bounds of the buffer. + // First make sure the top is >=0 proposedTop = std::max(static_cast(0), proposedTop); - 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. + const auto newView = Viewport::FromDimensions({ 0, proposedTop }, viewportSize); + const auto proposedBottom = newView.BottomExclusive(); if (proposedBottom > bufferSize.Y) { proposedTop = ::base::saturated_cast(proposedTop - (proposedBottom - bufferSize.Y)); From 61ccc28ee98397ccbdb7f2d84b9b738cb98d2b8f Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 12 Mar 2020 14:46:15 -0500 Subject: [PATCH 36/39] fix the tests --- src/cascadia/TerminalCore/Terminal.cpp | 10 +++++-- .../ConptyRoundtripTests.cpp | 28 ++++++------------- src/renderer/vt/XtermEngine.cpp | 22 +++++++-------- src/renderer/vt/state.cpp | 2 +- 4 files changed, 27 insertions(+), 35 deletions(-) diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index 76732f8ded6..20d84ef1da9 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -268,11 +268,15 @@ void Terminal::UpdateSettings(winrt::Microsoft::Terminal::Settings::ICoreSetting { if (dx < 0 && proposedTop > 0) { - auto row = newTextBuffer->GetRowByOffset(proposedTop - 1); - if (row.GetCharRow().WasWrapForced()) + try { - proposedTop--; + auto row = newTextBuffer->GetRowByOffset(::base::saturated_cast(proposedTop - 1)); + if (row.GetCharRow().WasWrapForced()) + { + proposedTop--; + } } + CATCH_LOG(); } } } diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index b446251aea8..9c5b2491345 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -507,21 +507,15 @@ void ConptyRoundtripTests::TestExactWrappingWithoutSpaces() hostSm.ProcessString(L"\n"); hostSm.ProcessString(L"1234567890"); - auto verifyBuffer = [&](const TextBuffer& tb, const bool isTerminal) { + auto verifyBuffer = [&](const TextBuffer& tb) { 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()); + VERIFY_IS_FALSE(row0.GetCharRow().WasWrapForced()); const auto& row1 = tb.GetRowByOffset(1); VERIFY_IS_FALSE(row1.GetCharRow().WasWrapForced()); @@ -530,7 +524,7 @@ void ConptyRoundtripTests::TestExactWrappingWithoutSpaces() TestUtils::VerifyExpectedString(tb, L"1234567890", { 0, 1 }); }; - verifyBuffer(hostTb, false); + verifyBuffer(hostTb); // First write the first 80 characters from the string expectedOutput.push_back(R"(!"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\]^_`abcdefghijklmnop)"); @@ -543,7 +537,7 @@ void ConptyRoundtripTests::TestExactWrappingWithoutSpaces() expectedOutput.push_back("\x1b[K"); VERIFY_SUCCEEDED(renderer.PaintFrame()); - verifyBuffer(termTb, true); + verifyBuffer(termTb); } void ConptyRoundtripTests::TestExactWrappingWithSpaces() @@ -577,21 +571,15 @@ void ConptyRoundtripTests::TestExactWrappingWithSpaces() hostSm.ProcessString(L" "); hostSm.ProcessString(L"1234567890"); - auto verifyBuffer = [&](const TextBuffer& tb, const bool isTerminal) { + auto verifyBuffer = [&](const TextBuffer& tb) { 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()); + VERIFY_IS_FALSE(row0.GetCharRow().WasWrapForced()); const auto& row1 = tb.GetRowByOffset(1); VERIFY_IS_FALSE(row1.GetCharRow().WasWrapForced()); @@ -600,7 +588,7 @@ void ConptyRoundtripTests::TestExactWrappingWithSpaces() TestUtils::VerifyExpectedString(tb, L" 1234567890", { 0, 1 }); }; - verifyBuffer(hostTb, false); + verifyBuffer(hostTb); // First write the first 80 characters from the string expectedOutput.push_back(R"(!"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\]^_`abcdefghijklmnop)"); @@ -613,7 +601,7 @@ void ConptyRoundtripTests::TestExactWrappingWithSpaces() expectedOutput.push_back("\x1b[K"); VERIFY_SUCCEEDED(renderer.PaintFrame()); - verifyBuffer(termTb, true); + verifyBuffer(termTb); } void ConptyRoundtripTests::MoveCursorAtEOL() diff --git a/src/renderer/vt/XtermEngine.cpp b/src/renderer/vt/XtermEngine.cpp index 94fe6b95a57..bf59535a8d7 100644 --- a/src/renderer/vt/XtermEngine.cpp +++ b/src/renderer/vt/XtermEngine.cpp @@ -74,17 +74,17 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, dirtyView = Viewport::Union(dirtyView, Viewport::FromInclusive(til::at(dirty, i))); } - // This is expecting the dirty view to be the union of all dirty regions as one big - // rectangle descrbing them all. - if (!_resized && dirtyView == _lastViewport) - { - // TODO: MSFT:21096414 - This is never actually hit. We set - // _resized=true on every frame (see VtEngine::UpdateViewport). - // Unfortunately, not always setting _resized is not a good enough - // solution, see that work item for a description why. - RETURN_IF_FAILED(_ClearScreen()); - _clearedAllThisFrame = true; - } + // // This is expecting the dirty view to be the union of all dirty regions as one big + // // rectangle descrbing them all. + // if (!_resized && dirtyView == _lastViewport) + // { + // // TODO: MSFT:21096414 - This is never actually hit. We set + // // _resized=true on every frame (see VtEngine::UpdateViewport). + // // Unfortunately, not always setting _resized is not a good enough + // // solution, see that work item for a description why. + // RETURN_IF_FAILED(_ClearScreen()); + // _clearedAllThisFrame = true; + // } } if (!_quickReturn) diff --git a/src/renderer/vt/state.cpp b/src/renderer/vt/state.cpp index f1e5232fd9f..c83ac780ddd 100644 --- a/src/renderer/vt/state.cpp +++ b/src/renderer/vt/state.cpp @@ -298,6 +298,7 @@ CATCH_RETURN(); { hr = _ResizeWindow(newView.Width(), newView.Height()); } + _resized = true; } // See MSFT:19408543 @@ -353,7 +354,6 @@ CATCH_RETURN(); } } - _resized = true; return hr; } From aff3cc5fe6fbeadbfbf1a72544f24543aac9e26f Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 12 Mar 2020 14:47:25 -0500 Subject: [PATCH 37/39] someday I"ll actually save all the files with changes in them before committing and pushing --- src/renderer/vt/XtermEngine.cpp | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/src/renderer/vt/XtermEngine.cpp b/src/renderer/vt/XtermEngine.cpp index bf59535a8d7..bc838fa229a 100644 --- a/src/renderer/vt/XtermEngine.cpp +++ b/src/renderer/vt/XtermEngine.cpp @@ -73,18 +73,6 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, { dirtyView = Viewport::Union(dirtyView, Viewport::FromInclusive(til::at(dirty, i))); } - - // // This is expecting the dirty view to be the union of all dirty regions as one big - // // rectangle descrbing them all. - // if (!_resized && dirtyView == _lastViewport) - // { - // // TODO: MSFT:21096414 - This is never actually hit. We set - // // _resized=true on every frame (see VtEngine::UpdateViewport). - // // Unfortunately, not always setting _resized is not a good enough - // // solution, see that work item for a description why. - // RETURN_IF_FAILED(_ClearScreen()); - // _clearedAllThisFrame = true; - // } } if (!_quickReturn) From 0532c6694299b0bd814a76beedb7b520b49fe5d9 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 12 Mar 2020 16:24:10 -0500 Subject: [PATCH 38/39] fix the renderer test --- src/host/ut_host/VtRendererTests.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/host/ut_host/VtRendererTests.cpp b/src/host/ut_host/VtRendererTests.cpp index ec3ac8e33e0..92410495822 100644 --- a/src/host/ut_host/VtRendererTests.cpp +++ b/src/host/ut_host/VtRendererTests.cpp @@ -262,7 +262,6 @@ void VtRendererTest::Xterm256TestInvalidate() Log::Comment(NoThrowString().Format( L"Make sure that invalidating all invalidates the whole viewport.")); VERIFY_SUCCEEDED(engine->InvalidateAll()); - qExpectedInput.push_back("\x1b[2J"); TestPaint(*engine, [&]() { VERIFY_ARE_EQUAL(view, engine->_invalidRect); }); @@ -730,7 +729,6 @@ void VtRendererTest::XtermTestInvalidate() Log::Comment(NoThrowString().Format( L"Make sure that invalidating all invalidates the whole viewport.")); VERIFY_SUCCEEDED(engine->InvalidateAll()); - qExpectedInput.push_back("\x1b[2J"); TestPaint(*engine, [&]() { VERIFY_ARE_EQUAL(view, engine->_invalidRect); }); From 5394b30897cbd2139e1aafcb8bbd89e84a84620f Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 12 Mar 2020 16:56:19 -0500 Subject: [PATCH 39/39] make this a reference to an optional. These tests better pass... --- src/buffer/out/textBuffer.cpp | 40 +++++++++++--------------- src/buffer/out/textBuffer.hpp | 7 ++--- src/cascadia/TerminalCore/Terminal.cpp | 9 ++++-- src/host/screenInfo.cpp | 4 ++- 4 files changed, 28 insertions(+), 32 deletions(-) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index 945945e8a1e..f37ff1a2810 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -573,27 +573,21 @@ bool TextBuffer::IncrementCircularBuffer(const bool inVtMode) } //Routine Description: -// - Retrieves the position of the last non-space character on the final line of the text buffer. -// - By default, we search the entire buffer to find the last non-space character -//Arguments: -// - -//Return Value: -// - Coordinate position in screen coordinates (offset coordinates, not array index coordinates). -COORD TextBuffer::GetLastNonSpaceCharacter() const -{ - return GetLastNonSpaceCharacter(GetSize()); -} - -//Routine Description: -// - Retrieves the position of the last non-space character in the given viewport -// - This is basically an optimized version of GetLastNonSpaceCharacter(), and can be called when -// - we know the last character is within the given viewport (so we don't need to check the entire buffer) +// - Retrieves the position of the last non-space character in the given +// viewport +// - By default, we search the entire buffer to find the last non-space +// character. +// - If we know the last character is within the given viewport (so we don't +// need to check the entire buffer), we can provide a value in viewOptional +// that we'll use to search for the last character in. //Arguments: // - The viewport //Return value: // - Coordinate position (relative to the text buffer) -COORD TextBuffer::GetLastNonSpaceCharacter(const Microsoft::Console::Types::Viewport viewport) const +COORD TextBuffer::GetLastNonSpaceCharacter(std::optional viewOptional) const { + const auto viewport = viewOptional.has_value() ? viewOptional.value() : GetSize(); + COORD coordEndOfText = { 0 }; // Search the given viewport by starting at the bottom. coordEndOfText.Y = viewport.BottomInclusive(); @@ -1877,13 +1871,13 @@ std::string TextBuffer::GenRTF(const TextAndColor& rows, const int fontHeightPoi // parameter as an optimization, as opposed to searching the entire buffer. // - oldViewportTop - Optional. The caller can provide a row in this parameter // and we'll calculate the position of the _end_ of that row in the new -// buffer. The row's new value is placed back into this pointer. +// buffer. The row's new value is placed back into this parameter. // 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 lastCharacterViewport, - short* const oldViewportTop) + std::optional& oldViewportTop) { Cursor& oldCursor = oldBuffer.GetCursor(); Cursor& newCursor = newBuffer.GetCursor(); @@ -1895,9 +1889,7 @@ HRESULT TextBuffer::Reflow(TextBuffer& oldBuffer, // place the new cursor back on the equivalent character in // the new buffer. const COORD cOldCursorPos = oldCursor.GetPosition(); - const COORD cOldLastChar = lastCharacterViewport.has_value() ? - oldBuffer.GetLastNonSpaceCharacter(lastCharacterViewport.value()) : - oldBuffer.GetLastNonSpaceCharacter(); + const COORD cOldLastChar = oldBuffer.GetLastNonSpaceCharacter(lastCharacterViewport); const short cOldRowsTotal = cOldLastChar.Y + 1; const short cOldColsTotal = oldBuffer.GetSize().Width(); @@ -1968,11 +1960,11 @@ HRESULT TextBuffer::Reflow(TextBuffer& oldBuffer, // If we found the old row that the caller was interested in, set the // out value of that parameter to the cursor's current Y position (the // new location of the _end_ of that row in the buffer). - if (oldViewportTop && !foundOldRow) + if (oldViewportTop.has_value() && !foundOldRow) { - if (iOldRow >= *oldViewportTop) + if (iOldRow >= oldViewportTop.value()) { - *oldViewportTop = newCursor.GetPosition().Y; + oldViewportTop = newCursor.GetPosition().Y; foundOldRow = true; } } diff --git a/src/buffer/out/textBuffer.hpp b/src/buffer/out/textBuffer.hpp index 457a6738d86..7b2ee52df39 100644 --- a/src/buffer/out/textBuffer.hpp +++ b/src/buffer/out/textBuffer.hpp @@ -103,8 +103,7 @@ class TextBuffer final // Scroll needs access to this to quickly rotate around the buffer. bool IncrementCircularBuffer(const bool inVtMode = false); - COORD GetLastNonSpaceCharacter() const; - COORD GetLastNonSpaceCharacter(const Microsoft::Console::Types::Viewport viewport) const; + COORD GetLastNonSpaceCharacter(std::optional viewOptional = std::nullopt) const; Cursor& GetCursor() noexcept; const Cursor& GetCursor() const noexcept; @@ -164,8 +163,8 @@ class TextBuffer final static HRESULT Reflow(TextBuffer& oldBuffer, TextBuffer& newBuffer, - const std::optional lastCharacterViewport = std::nullopt, - short* const lastScrollbackRow = nullptr); + const std::optional lastCharacterViewport, + std::optional& oldViewportTop); private: std::deque _storage; diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index 20d84ef1da9..378a510bff2 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -184,7 +184,8 @@ void Terminal::UpdateSettings(winrt::Microsoft::Terminal::Settings::ICoreSetting const short sCursorHeightInViewportBefore = _buffer->GetCursor().GetPosition().Y - _mutableViewport.Top(); // This will be used to determine where the viewport should be in the new buffer. - short oldViewportTop = _mutableViewport.Top(); + const short oldViewportTop = _mutableViewport.Top(); + short newViewportTop = oldViewportTop; // First allocate a new text buffer to take the place of the current one. std::unique_ptr newTextBuffer; @@ -195,10 +196,12 @@ void Terminal::UpdateSettings(winrt::Microsoft::Terminal::Settings::ICoreSetting 0, // temporarily set size to 0 so it won't render. _buffer->GetRenderTarget()); + std::optional oldViewStart{ oldViewportTop }; RETURN_IF_FAILED(TextBuffer::Reflow(*_buffer.get(), *newTextBuffer.get(), _mutableViewport, - &oldViewportTop)); + oldViewStart)); + newViewportTop = oldViewStart.value(); } CATCH_RETURN(); @@ -243,7 +246,7 @@ void Terminal::UpdateSettings(winrt::Microsoft::Terminal::Settings::ICoreSetting const auto maxRow = std::max(newLastChar.Y, newCursorPos.Y); const short proposedTopFromLastLine = ::base::saturated_cast(maxRow - viewportSize.Y + 1); - const short proposedTopFromScrollback = oldViewportTop; + const short proposedTopFromScrollback = newViewportTop; short proposedTop = std::max(proposedTopFromLastLine, proposedTopFromScrollback); diff --git a/src/host/screenInfo.cpp b/src/host/screenInfo.cpp index e4213771d09..306a4c7c30b 100644 --- a/src/host/screenInfo.cpp +++ b/src/host/screenInfo.cpp @@ -1414,7 +1414,9 @@ bool SCREEN_INFORMATION::IsMaximizedY() const // Save cursor's relative height versus the viewport SHORT const sCursorHeightInViewportBefore = _textBuffer->GetCursor().GetPosition().Y - _viewport.Top(); - HRESULT hr = TextBuffer::Reflow(*_textBuffer.get(), *newTextBuffer.get()); + // Reflow requires a optional& , which can't just be done inline with the call. + std::optional unused{ std::nullopt }; + HRESULT hr = TextBuffer::Reflow(*_textBuffer.get(), *newTextBuffer.get(), std::nullopt, unused); if (SUCCEEDED(hr)) {