Skip to content

Commit

Permalink
Merge the LineFeed functionality into AdaptDispatch (#14874)
Browse files Browse the repository at this point in the history 
The main purpose of this PR was to merge the `ITerminalApi::LineFeed`
implementations into a shared method in `AdaptDispatch`, and avoid the
VT code path depending on the `AdjustCursorPosition` function (which
could then be massively simplified). This refactoring also fixes some
bugs that existed in the original `LineFeed` implementations.

## References and Relevant Issues

This helps to close the gap between the Conhost and Terminal (#13408).
This improves some of the scrollbar mark bugs mentioned in #11000.

## Detailed Description of the Pull Request / Additional comments

I had initially hoped the line feed functionality could be implemented
entirely within `AdaptDispatch`, but there is still some Conhost and
Terminal-specific behavior that needs to be triggered when we reach the
bottom of the buffer, and the row coordinates are cycled.

In Conhost we need to trigger an accessibility scroll event, and in
Windows Terminal we need to update selection and marker offsets, reset
pattern intervals, and preserve the user's scroll offset. This is now
handled by a new `NotifyBufferRotation` method in `ITerminalApi`.

But this made me realise that the `_EraseAll` method should have been
doing the same thing when it reached the bottom of the buffer. So I've
added a call to the new `NotifyBufferRotation` API from there as well.

And in the case of Windows Terminal, the scroll offset preservation was
something that was also needed for a regular viewport pan. So I've put
that in a separate `_PreserveUserScrollOffset` method which is called
from the `SetViewportPosition` handler as well.

## Validation Steps Performed

Because of the API changes, there were a number of unit tests that
needed to be updated:

- Some of the `ScreenBufferTests` were accessing margin state in the
`SCREEN_INFORMATION` class which doesn't exist anymore, so I had to add
a little helper function which now manually detects the active margins.

- Some of the `AdapterTest` tests were dependent on APIs that no longer
exist, so they needed to be rewritten so they now check the resulting
state rather than expecting a mock API call.

- The `ScrollWithMargins` test in `ConptyRoundtripTests` was testing
functionality that didn't previously work correctly (issue #3673). Now
that it's been fixed, that test needed to be updated accordingly.

Other than getting the unit tests working, I've manually verified that
issue #3673 is now fixed. And I've also checked that the scroll markers,
selections, and user scroll offset are all being updated correctly, both
with a regular viewport pan, as well as when overrunning the buffer.

Closes #3673
  • Loading branch information
@j4james
j4james committed Mar 30, 2023
1 parent da3a33f commit fc95802
Show file tree
Hide file tree
Showing 15 changed files with 330 additions and 638 deletions.
150 changes: 9 additions & 141 deletions src/cascadia/TerminalCore/Terminal.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -490,9 +490,7 @@ void Terminal::Write(std::wstring_view stringView)
const til::point cursorPosAfter{ cursor.GetPosition() };

// Firing the CursorPositionChanged event is very expensive so we try not to
// do that when the cursor does not need to be redrawn. We don't do this
// inside _AdjustCursorPosition, only once we're done writing the whole run
// of output.
// do that when the cursor does not need to be redrawn.
if (cursorPosBefore != cursorPosAfter)
{
_NotifyTerminalCursorPositionChanged();
Expand Down Expand Up @@ -1078,146 +1076,16 @@ Viewport Terminal::_GetVisibleViewport() const noexcept
size);
}

void Terminal::_AdjustCursorPosition(const til::point proposedPosition)
void Terminal::_PreserveUserScrollOffset(const int viewportDelta) noexcept
{
#pragma warning(suppress : 26496) // cpp core checks wants this const but it's modified below.
auto proposedCursorPosition = proposedPosition;
auto& cursor = _activeBuffer().GetCursor();
const auto bufferSize = _activeBuffer().GetSize();

// If we're about to scroll past the bottom of the buffer, instead cycle the
// buffer.
til::CoordType rowsPushedOffTopOfBuffer = 0;
const auto newRows = std::max(0, proposedCursorPosition.y - bufferSize.Height() + 1);
if (proposedCursorPosition.y >= bufferSize.Height())
// When the mutable viewport is moved down, and there's an active selection,
// or the visible viewport isn't already at the bottom, then we want to keep
// the visible viewport where it is. To do this, we adjust the scroll offset
// by the same amount that we've just moved down.
if (viewportDelta > 0 && (IsSelectionActive() || _scrollOffset != 0))
{
for (auto dy = 0; dy < newRows; dy++)
{
_activeBuffer().IncrementCircularBuffer();
proposedCursorPosition.y--;
rowsPushedOffTopOfBuffer++;

// Update our selection too, so it doesn't move as the buffer is cycled
if (_selection)
{
// Stash this, so we can make sure to update the pivot to match later
const auto pivotWasStart = _selection->start == _selection->pivot;
// If the start of the selection is above 0, we can reduce both the start and end by 1
if (_selection->start.y > 0)
{
_selection->start.y -= 1;
_selection->end.y -= 1;
}
else
{
// The start of the selection is at 0, if the end is greater than 0, then only reduce the end
if (_selection->end.y > 0)
{
_selection->start.x = 0;
_selection->end.y -= 1;
}
else
{
// Both the start and end of the selection are at 0, clear the selection
_selection.reset();
}
}

// If we still have a selection, make sure to sync the pivot
// with whichever value is the right one.
//
// Failure to do this might lead to GH #14462
if (_selection.has_value())
{
_selection->pivot = pivotWasStart ? _selection->start : _selection->end;
}
}
}

// manually erase our pattern intervals since the locations have changed now
_patternIntervalTree = {};
}

// Update Cursor Position
cursor.SetPosition(proposedCursorPosition);

// Move the viewport down if the cursor moved below the viewport.
// Obviously, don't need to do this in the alt buffer.
if (!_inAltBuffer())
{
auto updatedViewport = false;
const auto scrollAmount = std::max(0, proposedCursorPosition.y - _mutableViewport.BottomInclusive());
if (scrollAmount > 0)
{
const auto newViewTop = std::max(0, proposedCursorPosition.y - (_mutableViewport.Height() - 1));
// In the alt buffer, we never need to adjust _mutableViewport, which is the viewport of the main buffer.
if (newViewTop != _mutableViewport.Top())
{
_mutableViewport = Viewport::FromDimensions({ 0, newViewTop },
_mutableViewport.Dimensions());
updatedViewport = true;
}
}

// If the viewport moved, or we circled the buffer, we might need to update
// our _scrollOffset
if (updatedViewport || newRows != 0)
{
const auto oldScrollOffset = _scrollOffset;

// scroll if...
// - no selection is active
// - viewport is already at the bottom
const auto scrollToOutput = !IsSelectionActive() && _scrollOffset == 0;

_scrollOffset = scrollToOutput ? 0 : _scrollOffset + scrollAmount + newRows;

// Clamp the range to make sure that we don't scroll way off the top of the buffer
_scrollOffset = std::clamp(_scrollOffset,
0,
_activeBuffer().GetSize().Height() - _mutableViewport.Height());

// If the new scroll offset is different, then we'll still want to raise a scroll event
updatedViewport = updatedViewport || (oldScrollOffset != _scrollOffset);
}

// If the viewport moved, then send a scrolling notification.
if (updatedViewport)
{
_NotifyScrollEvent();
}
}

if (rowsPushedOffTopOfBuffer != 0)
{
if (_scrollMarks.size() > 0)
{
for (auto& mark : _scrollMarks)
{
// Move the mark up
mark.start.y -= rowsPushedOffTopOfBuffer;

// If the mark had sub-regions, then move those pointers too
if (mark.commandEnd.has_value())
{
(*mark.commandEnd).y -= rowsPushedOffTopOfBuffer;
}
if (mark.outputEnd.has_value())
{
(*mark.outputEnd).y -= rowsPushedOffTopOfBuffer;
}
}

_scrollMarks.erase(std::remove_if(_scrollMarks.begin(),
_scrollMarks.end(),
[](const VirtualTerminal::DispatchTypes::ScrollMark& m) { return m.start.y < 0; }),
_scrollMarks.end());
}
// We have to report the delta here because we might have circled the text buffer.
// That didn't change the viewport and therefore the TriggerScroll(void)
// method can't detect the delta on its own.
const til::point delta{ 0, -rowsPushedOffTopOfBuffer };
_activeBuffer().TriggerScroll(delta);
const auto maxScrollOffset = _activeBuffer().GetSize().Height() - _mutableViewport.Height();
_scrollOffset = std::min(_scrollOffset + viewportDelta, maxScrollOffset);
}
}

Expand Down
5 changes: 2 additions & 3 deletions src/cascadia/TerminalCore/Terminal.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,10 +113,8 @@ class Microsoft::Terminal::Core::Terminal final :
void SetTextAttributes(const TextAttribute& attrs) noexcept override;
void SetAutoWrapMode(const bool wrapAtEOL) noexcept override;
bool GetAutoWrapMode() const noexcept override;
void SetScrollingRegion(const til::inclusive_rect& scrollMargins) noexcept override;
void WarningBell() override;
bool GetLineFeedMode() const noexcept override;
void LineFeed(const bool withReturn, const bool wrapForced) override;
void SetWindowTitle(const std::wstring_view title) override;
CursorType GetUserDefaultCursorStyle() const noexcept override;
bool ResizeWindow(const til::CoordType width, const til::CoordType height) noexcept override;
Expand All @@ -140,6 +138,7 @@ class Microsoft::Terminal::Core::Terminal final :
bool IsConsolePty() const noexcept override;
bool IsVtInputEnabled() const noexcept override;
void NotifyAccessibilityChange(const til::rect& changedRect) noexcept override;
void NotifyBufferRotation(const int delta) override;
#pragma endregion

void ClearMark();
Expand Down Expand Up @@ -420,7 +419,7 @@ class Microsoft::Terminal::Core::Terminal final :
Microsoft::Console::Types::Viewport _GetMutableViewport() const noexcept;
Microsoft::Console::Types::Viewport _GetVisibleViewport() const noexcept;

void _AdjustCursorPosition(const til::point proposedPosition);
void _PreserveUserScrollOffset(const int viewportDelta) noexcept;

void _NotifyScrollEvent() noexcept;

Expand Down
84 changes: 62 additions & 22 deletions src/cascadia/TerminalCore/TerminalApi.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,11 @@ void Terminal::SetViewportPosition(const til::point position) noexcept
// The viewport is fixed at 0,0 for the alt buffer, so this is a no-op.
if (!_inAltBuffer())
{
const auto viewportDelta = position.y - _GetMutableViewport().Origin().y;
const auto dimensions = _GetMutableViewport().Dimensions();
_mutableViewport = Viewport::FromDimensions(position, dimensions);
Terminal::_NotifyScrollEvent();
_PreserveUserScrollOffset(viewportDelta);
_NotifyScrollEvent();
}
}

Expand All @@ -70,11 +72,6 @@ bool Terminal::GetAutoWrapMode() const noexcept
return true;
}

void Terminal::SetScrollingRegion(const til::inclusive_rect& /*scrollMargins*/) noexcept
{
// TODO: This will be needed to fully support DECSTBM.
}

void Terminal::WarningBell()
{
_pfnWarningBell();
Expand All @@ -86,22 +83,6 @@ bool Terminal::GetLineFeedMode() const noexcept
return false;
}

void Terminal::LineFeed(const bool withReturn, const bool wrapForced)
{
auto cursorPos = _activeBuffer().GetCursor().GetPosition();

// If the line was forced to wrap, set the wrap status.
// When explicitly moving down a row, clear the wrap status.
_activeBuffer().GetRowByOffset(cursorPos.y).SetWrapForced(wrapForced);

cursorPos.y++;
if (withReturn)
{
cursorPos.x = 0;
}
_AdjustCursorPosition(cursorPos);
}

void Terminal::SetWindowTitle(const std::wstring_view title)
{
if (!_suppressApplicationTitle)
Expand Down Expand Up @@ -467,3 +448,62 @@ void Terminal::NotifyAccessibilityChange(const til::rect& /*changedRect*/) noexc
{
// This is only needed in conhost. Terminal handles accessibility in another way.
}

void Terminal::NotifyBufferRotation(const int delta)
{
// Update our selection, so it doesn't move as the buffer is cycled
if (_selection)
{
// If the end of the selection will be out of range after the move, we just
// clear the selection. Otherwise we move both the start and end points up
// by the given delta and clamp to the first row.
if (_selection->end.y < delta)
{
_selection.reset();
}
else
{
// Stash this, so we can make sure to update the pivot to match later.
const auto pivotWasStart = _selection->start == _selection->pivot;
_selection->start.y = std::max(_selection->start.y - delta, 0);
_selection->end.y = std::max(_selection->end.y - delta, 0);
// Make sure to sync the pivot with whichever value is the right one.
_selection->pivot = pivotWasStart ? _selection->start : _selection->end;
}
}

// manually erase our pattern intervals since the locations have changed now
_patternIntervalTree = {};

const auto hasScrollMarks = _scrollMarks.size() > 0;
if (hasScrollMarks)
{
for (auto& mark : _scrollMarks)
{
// Move the mark up
mark.start.y -= delta;

// If the mark had sub-regions, then move those pointers too
if (mark.commandEnd.has_value())
{
(*mark.commandEnd).y -= delta;
}
if (mark.outputEnd.has_value())
{
(*mark.outputEnd).y -= delta;
}
}

_scrollMarks.erase(std::remove_if(_scrollMarks.begin(),
_scrollMarks.end(),
[](const auto& m) { return m.start.y < 0; }),
_scrollMarks.end());
}

const auto oldScrollOffset = _scrollOffset;
_PreserveUserScrollOffset(delta);
if (_scrollOffset != oldScrollOffset || hasScrollMarks)
{
_NotifyScrollEvent();
}
}
41 changes: 22 additions & 19 deletions src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1700,56 +1700,59 @@ void ConptyRoundtripTests::ScrollWithMargins()
hostSm.ProcessString(completeCursorAtPromptLine);

// Set up the verifications like above.
auto verifyBufferAfter = [&](const TextBuffer& tb) {
auto verifyBufferAfter = [&](const TextBuffer& tb, const auto panOffset) {
auto& cursor = tb.GetCursor();
// Verify the cursor is waiting on the freshly revealed line (1 above mode line)
// and in the left most column.
VERIFY_ARE_EQUAL(initialTermView.Height() - 2, cursor.GetPosition().y);
const auto bottomLine = initialTermView.BottomInclusive() + panOffset;
VERIFY_ARE_EQUAL(bottomLine - 1, cursor.GetPosition().y);
VERIFY_ARE_EQUAL(0, cursor.GetPosition().x);

// For all rows except the last two, verify that we have a run of four letters.
for (auto i = 0; i < rowsToWrite - 1; ++i)
{
// Start with B this time because the A line got scrolled off the top.
const std::wstring expectedString(4, static_cast<wchar_t>(L'B' + i));
const til::point expectedPos{ 0, i };
const til::point expectedPos{ 0, panOffset + i };
TestUtils::VerifyExpectedString(tb, expectedString, expectedPos);
}

// For the second to last row, verify that it is blank.
{
const std::wstring expectedBlankLine(initialTermView.Width(), L' ');
const til::point blankLinePos{ 0, rowsToWrite - 1 };
const til::point blankLinePos{ 0, panOffset + rowsToWrite - 1 };
TestUtils::VerifyExpectedString(tb, expectedBlankLine, blankLinePos);
}

// For the last row, verify we have an entire row of asterisks for the mode line.
{
const std::wstring expectedModeLine(initialTermView.Width() - 1, L'*');
const til::point modeLinePos{ 0, rowsToWrite };
const til::point modeLinePos{ 0, panOffset + rowsToWrite };
TestUtils::VerifyExpectedString(tb, expectedModeLine, modeLinePos);
}
};

// This will verify the text emitted from the PTY.

expectedOutput.push_back("\x1b[H"); // cursor returns to top left corner.
for (auto i = 0; i < rowsToWrite - 1; ++i)
expectedOutput.push_back("\r\n"); // cursor moved to bottom left corner
expectedOutput.push_back("\n"); // linefeed pans the viewport down
{
const std::string expectedString(4, static_cast<char>('B' + i));
expectedOutput.push_back(expectedString);
expectedOutput.push_back("\x1b[K"); // erase the rest of the line.
expectedOutput.push_back("\r\n");
// Cursor gets reset into second line from bottom, left most column
std::stringstream ss;
ss << "\x1b[" << initialTermView.Height() - 1 << ";1H";
expectedOutput.push_back(ss.str());
}
{
expectedOutput.push_back(""); // nothing for the empty line
expectedOutput.push_back("\x1b[K"); // erase the rest of the line.
expectedOutput.push_back("\r\n");
// Bottom of the scroll region is replaced with a blank line
const std::string expectedString(initialTermView.Width(), ' ');
expectedOutput.push_back(expectedString);
}
expectedOutput.push_back("\r\n"); // cursor moved to bottom left corner
{
// Mode line is redrawn at the bottom of the viewport
const std::string expectedString(initialTermView.Width() - 1, '*');
// There will be one extra blank space at the end of the line, to prevent delayed EOL wrapping
expectedOutput.push_back(expectedString + " ");
expectedOutput.push_back(expectedString);
expectedOutput.push_back(" ");
}
{
// Cursor gets reset into second line from bottom, left most column
Expand All @@ -1761,15 +1764,15 @@ void ConptyRoundtripTests::ScrollWithMargins()

Log::Comment(L"Verify host buffer contains pattern moved up one and mode line still in place.");
// Verify the host side.
verifyBufferAfter(hostTb);
verifyBufferAfter(hostTb, 0);

Log::Comment(L"Emit PTY frame and validate it transmits the right data.");
// Paint the frame
VERIFY_SUCCEEDED(renderer.PaintFrame());

Log::Comment(L"Verify terminal buffer contains pattern moved up one and mode line still in place.");
// Verify the terminal side.
verifyBufferAfter(termTb);
// Verify the terminal side. Note the viewport has panned down a line.
verifyBufferAfter(termTb, 1);
}

void ConptyRoundtripTests::DontWrapMoveCursorInSingleFrame()
Expand Down
Loading

0 comments on commit fc95802

Please sign in to comment.