Skip to content

Commit

Permalink
Add support for "reflow"ing the Terminal buffer (#4741)
Browse files Browse the repository at this point in the history
This PR adds support for "Resize with Reflow" to the Terminal. In
conhost, `ResizeWithReflow` is the function that's responsible for
reflowing wrapped lines of text as the buffer gets resized. Now that
#4415 has merged, we can also implement this in the Terminal. Now, when
the Terminal is resized, it will reflow the lines of it's buffer in the
same way that conhost does. This means, the terminal will no longer chop
off the ends of lines as the buffer is too small to represent them. 

As a happy side effect of this PR, it also fixed #3490. This was a bug
that plagued me during the investigation into this functionality. The
original #3490 PR, #4354, tried to fix this bug with some heavy conpty
changes. Turns out, that only made things worse, and far more
complicated. When I really got to thinking about it, I realized "conhost
can handle this right, why can't the Terminal?". Turns out, by adding
resize with reflow, I was also able to fix this at the same time.
Conhost does a little bit of math after reflowing to attempt to keep the
viewport in the same relative place after a reflow. By re-using that
logic in the Terminal, I was able to fix #3490.

I also included that big ole test from #3490, because everyone likes
adding 60 test cases in a PR.

## References
* #4200 - this scenario
* #405/#4415 - conpty emits wrapped lines, which was needed for this PR
* #4403 - delayed EOL wrapping via conpty, which was also needed for
  this
* #4354 - we don't speak of this PR anymore

## PR Checklist
* [x] Closes #1465
* [x] Closes #3490
* [x] Closes #4771
* [x] Tests added/passed

## EDIT: Changes to this PR on 5 March 2020

I learned more since my original version of this PR. I wrote that in
January, and despite my notes that say it was totally working, it
_really_ wasn't.

Part of the hard problem, as mentioned in #3490, is that the Terminal
might request a resize to (W, H-1), and while conpty is preparing that
frame, or before the terminal has received that frame, the Terminal
resizes to (W, H-2). Now, there aren't enough lines in the terminal
buffer to catch all the lines that conpty is about to emit. When that
happens, lines get duplicated in the buffer. From a UX perspective, this
certainly looks a lot worse than a couple lost lines. It looks like
utter chaos.

So I've introduced a new mode to conpty to try and counteract this
behavior. This behavior I'm calling "quirky resize". The **TL;DR** of
quirky resize mode is that conpty won't emit the entire buffer on a
resize, and will trust that the terminal is prepared to reflow it's
buffer on it's own.

This will enable the quirky resize behavior for applications that are
prepared for it. The "quirky resize" is "don't `InvalidateAll` when the
terminal resizes". This is added as a quirk as to not regress other
terminal applications that aren't prepared for this behavior
(gnome-terminal, conhost in particular). For those kinds of terminals,
when the buffer is resized, it's just going to lose lines. That's what
currently happens for them.  

When the quirk is enabled, conpty won't repaint the entire buffer. This
gets around the "duplicated lines" issue that requesting multiple
resizes in a row can cause. However, for these terminals that are
unprepared, the conpty cursor might end up in the wrong position after a
quirky resize.

The case in point is maximizing the terminal. For maximizing
(height->50) from a buffer that's 30 lines tall, with the cursor on
y=30, this is what happens: 

  * With the quirk disabled, conpty reprints the entire buffer. This is
    60 lines that get printed. This ends up blowing away about 20 lines
    of scrollback history, as the terminal app would have tried to keep
    the text pinned to the bottom of the window. The term. app moved the
    viewport up 20 lines, and then the 50 lines of conpty output (30
    lines of text, and 20 blank lines at the bottom) overwrote the lines
    from the scrollback. This is bad, but not immediately obvious, and
    is **what currently happens**. 


  * With the quirk enabled, conpty doesn't emit any lines, but the
    actual content of the window is still only in the top 30 lines.
    However, the terminal app has still moved 20 lines down from the
    scrollback back into the viewport. So the terminal's cursor is at
    y=50 now, but conpty's is at 30. This means that the terminal and
    conpty are out of sync, and there's not a good way of re-syncing
    these. It's very possible (trivial in `powershell`) that the new
    output will jump up to y=30 override the existing output in the
    terminal buffer. 

The Windows Terminal is already prepared for this quirky behavior, so it
doesn't keep the output at the bottom of the window. It shifts it's
viewport down to match what conpty things the buffer looks like.

What happens when we have passthrough mode and WT is like "I would like
quirky resize"? I guess things will just work fine, cause there won't be
a buffer behind the passthrough app that the terminal cares about. Sure,
in the passthrough case the Terminal could _not_ quirky resize, but the
quirky resize won't be wrong.
  • Loading branch information
zadjii-msft authored Mar 13, 2020
1 parent 068e3e7 commit 93b31f6
Show file tree
Hide file tree
Showing 21 changed files with 577 additions and 127 deletions.
58 changes: 37 additions & 21 deletions src/buffer/out/textBuffer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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:
// - <none>
//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<const Microsoft::Console::Types::Viewport> 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();
Expand Down Expand Up @@ -1872,9 +1866,18 @@ 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.
// - 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 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)
HRESULT TextBuffer::Reflow(TextBuffer& oldBuffer,
TextBuffer& newBuffer,
const std::optional<Viewport> lastCharacterViewport,
std::optional<short>& oldViewportTop)
{
Cursor& oldCursor = oldBuffer.GetCursor();
Cursor& newCursor = newBuffer.GetCursor();
Expand All @@ -1886,14 +1889,14 @@ 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();
const COORD cOldLastChar = oldBuffer.GetLastNonSpaceCharacter(lastCharacterViewport);

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;

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++)
Expand Down Expand Up @@ -1953,6 +1956,19 @@ HRESULT TextBuffer::Reflow(TextBuffer& oldBuffer, TextBuffer& newBuffer)
}
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 the _end_ of that row in the buffer).
if (oldViewportTop.has_value() && !foundOldRow)
{
if (iOldRow >= oldViewportTop.value())
{
oldViewportTop = newCursor.GetPosition().Y;
foundOldRow = true;
}
}

if (SUCCEEDED(hr))
{
// If we didn't have a full row to copy, insert a new
Expand Down
8 changes: 5 additions & 3 deletions src/buffer/out/textBuffer.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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<const Microsoft::Console::Types::Viewport> viewOptional = std::nullopt) const;

Cursor& GetCursor() noexcept;
const Cursor& GetCursor() const noexcept;
Expand Down Expand Up @@ -162,7 +161,10 @@ 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<Microsoft::Console::Types::Viewport> lastCharacterViewport,
std::optional<short>& oldViewportTop);

private:
std::deque<ROW> _storage;
Expand Down
1 change: 0 additions & 1 deletion src/cascadia/TerminalApp/TerminalPage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -519,7 +519,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.
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalConnection/ConptyConnection.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
try
{
const COORD dimensions{ gsl::narrow_cast<SHORT>(_initialCols), gsl::narrow_cast<SHORT>(_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();
Expand Down
140 changes: 128 additions & 12 deletions src/cascadia/TerminalCore/Terminal.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -173,24 +173,143 @@ void Terminal::UpdateSettings(winrt::Microsoft::Terminal::Settings::ICoreSetting
{
return S_FALSE;
}
const auto dx = viewportSize.X - oldDimensions.X;

const auto oldTop = _mutableViewport.Top();

const short newBufferHeight = viewportSize.Y + _scrollbackLines;
COORD bufferSize{ viewportSize.X, newBufferHeight };
RETURN_IF_FAILED(_buffer->ResizeTraditional(bufferSize));

auto proposedTop = oldTop;
const auto newView = Viewport::FromDimensions({ 0, proposedTop }, viewportSize);
const auto proposedBottom = newView.BottomExclusive();
// 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.
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<TextBuffer> newTextBuffer;
try
{
newTextBuffer = std::make_unique<TextBuffer>(bufferSize,
_buffer->GetCurrentAttributes(),
0, // temporarily set size to 0 so it won't render.
_buffer->GetRenderTarget());

std::optional<short> oldViewStart{ oldViewportTop };
RETURN_IF_FAILED(TextBuffer::Reflow(*_buffer.get(),
*newTextBuffer.get(),
_mutableViewport,
oldViewStart));
newViewportTop = oldViewStart.value();
}
CATCH_RETURN();

// 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.
//
// 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 newLastChar = newCursorPos;
try
{
newLastChar = newTextBuffer->GetLastNonSpaceCharacter();
}
CATCH_LOG();
#pragma warning(pop)

const auto maxRow = std::max(newLastChar.Y, newCursorPos.Y);

const short proposedTopFromLastLine = ::base::saturated_cast<short>(maxRow - viewportSize.Y + 1);
const short proposedTopFromScrollback = newViewportTop;

short proposedTop = std::max(proposedTopFromLastLine,
proposedTopFromScrollback);

// 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)
{
const auto proposedViewFromTop = Viewport::FromDimensions({ 0, proposedTopFromScrollback }, viewportSize);
if (maxRow < proposedViewFromTop.BottomInclusive())
{
if (dx < 0 && proposedTop > 0)
{
try
{
auto row = newTextBuffer->GetRowByOffset(::base::saturated_cast<short>(proposedTop - 1));
if (row.GetCharRow().WasWrapForced())
{
proposedTop--;
}
}
CATCH_LOG();
}
}
}

// 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;
}

// Make sure the proposed viewport is within the bounds of the buffer.
// First make sure the top is >=0
proposedTop = std::max(static_cast<short>(0), proposedTop);

// 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 -= (proposedBottom - bufferSize.Y);
proposedTop = ::base::saturated_cast<short>(proposedTop - (proposedBottom - bufferSize.Y));
}

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

_buffer.swap(newTextBuffer);

_scrollOffset = 0;
_NotifyScrollEvent();

Expand Down Expand Up @@ -456,18 +575,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);
Expand Down
5 changes: 5 additions & 0 deletions src/cascadia/TerminalCore/TerminalApi.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down
Loading

0 comments on commit 93b31f6

Please sign in to comment.