From 59f9c64ad68c3424269813487848881d753e283d Mon Sep 17 00:00:00 2001 From: James Holderness Date: Tue, 7 Feb 2023 22:57:37 +0000 Subject: [PATCH 01/13] Implement the line feed functionality in AdaptDispatch. --- src/terminal/adapter/adaptDispatch.cpp | 96 ++++++++++++++++++++++++-- src/terminal/adapter/adaptDispatch.hpp | 2 + 2 files changed, 94 insertions(+), 4 deletions(-) diff --git a/src/terminal/adapter/adaptDispatch.cpp b/src/terminal/adapter/adaptDispatch.cpp index 3990fbdccaa..7c4c7fcc855 100644 --- a/src/terminal/adapter/adaptDispatch.cpp +++ b/src/terminal/adapter/adaptDispatch.cpp @@ -94,7 +94,7 @@ void AdaptDispatch::_WriteToBuffer(const std::wstring_view string) // different position from where the EOL was marked. if (delayedCursorPosition == cursorPosition) { - _api.LineFeed(true, true); + _DoLineFeed(textBuffer, true, true); cursorPosition = cursor.GetPosition(); // We need to recalculate the width when moving to a new line. lineWidth = textBuffer.GetLineWidth(cursorPosition.y); @@ -1984,6 +1984,93 @@ bool AdaptDispatch::CarriageReturn() return _CursorMovePosition(Offset::Unchanged(), Offset::Absolute(1), true); } +// Routine Description: +// - Helper method for executing a line feed, possibly preceded by carriage return. +// Arguments: +// - textBuffer - Target buffer on which the line feed is executed. +// - withReturn - Set to true if a carriage return should be performed as well. +// - wrapForced - Set to true is the line feed was the result of the line wrapping. +// Return Value: +// - +void AdaptDispatch::_DoLineFeed(TextBuffer& textBuffer, const bool withReturn, const bool wrapForced) +{ + const auto viewport = _api.GetViewport(); + const auto [topMargin, bottomMargin] = _GetVerticalMargins(viewport, true); + const auto bufferWidth = textBuffer.GetSize().Width(); + const auto bufferHeight = textBuffer.GetSize().Height(); + + auto& cursor = textBuffer.GetCursor(); + const auto currentPosition = cursor.GetPosition(); + auto newPosition = currentPosition; + + // If the line was forced to wrap, set the wrap status. + // When explicitly moving down a row, clear the wrap status. + textBuffer.GetRowByOffset(currentPosition.y).SetWrapForced(wrapForced); + + if (currentPosition.y != bottomMargin) + { + // If we're not at the bottom margin then there's no scrolling, + // so we make sure we don't move past the bottom of the viewport. + newPosition.y = std::min(currentPosition.y + 1, viewport.bottom - 1); + newPosition = textBuffer.ClampPositionWithinLine(newPosition); + } + else if (topMargin > viewport.top) + { + // If the top margin isn't at the top of the viewport, then we're + // just scrolling the margin area and the cursor stays where it is. + _ScrollRectVertically(textBuffer, { 0, topMargin, bufferWidth, bottomMargin + 1 }, -1); + } + else if (viewport.bottom < bufferHeight) + { + // If the top margin is at the top of the viewport, then we'll scroll + // the content up by panning the viewport down, and also move the cursor + // down a row. But we only do this if the viewport hasn't yet reached + // the end of the buffer. + _api.SetViewportPosition({ viewport.left, viewport.top + 1 }); + newPosition.y++; + + // And if the bottom margin didn't cover the full viewport, we copy the + // lower part of the viewport down so it remains static. But for a full + // pan we reset the newly revealed row with the current attributes. + if (bottomMargin < viewport.bottom - 1) + { + _ScrollRectVertically(textBuffer, { 0, bottomMargin + 1, bufferWidth, viewport.bottom + 1 }, 1); + } + else + { + auto eraseAttributes = textBuffer.GetCurrentAttributes(); + eraseAttributes.SetStandardErase(); + textBuffer.GetRowByOffset(newPosition.y).Reset(eraseAttributes); + } + } + else + { + // If the viewport has reached the end of the buffer, we can't pan down, + // so we cycle the row coordinates, which effectively scrolls the buffer + // content up. In this case we don't need to move the cursor down. + textBuffer.IncrementCircularBuffer(true); + + // We trigger a scroll rather than a redraw, since that's more efficient, + // but we need to turn the cursor off before doing so, otherwise a ghost + // cursor can be left behind in the previous position. + cursor.SetIsOn(false); + textBuffer.TriggerScroll({ 0, -1 }); + + // And again, if the bottom margin didn't cover the full viewport, we + // copy the lower part of the viewport down so it remains static. + if (bottomMargin < viewport.bottom - 1) + { + _ScrollRectVertically(textBuffer, { 0, bottomMargin, bufferWidth, bufferHeight }, 1); + } + } + + // If a carriage return was requested, we also move to the leftmost column. + newPosition.x = withReturn ? 0 : newPosition.x; + + cursor.SetPosition(newPosition); + _ApplyCursorMovementFlags(cursor); +} + // Routine Description: // - IND/NEL - Performs a line feed, possibly preceded by carriage return. // Moves the cursor down one line, and possibly also to the leftmost column. @@ -1993,16 +2080,17 @@ bool AdaptDispatch::CarriageReturn() // - True if handled successfully. False otherwise. bool AdaptDispatch::LineFeed(const DispatchTypes::LineFeedType lineFeedType) { + auto& textBuffer = _api.GetTextBuffer(); switch (lineFeedType) { case DispatchTypes::LineFeedType::DependsOnMode: - _api.LineFeed(_api.GetLineFeedMode(), false); + _DoLineFeed(textBuffer, _api.GetLineFeedMode(), false); return true; case DispatchTypes::LineFeedType::WithoutReturn: - _api.LineFeed(false, false); + _DoLineFeed(textBuffer, false, false); return true; case DispatchTypes::LineFeedType::WithReturn: - _api.LineFeed(true, false); + _DoLineFeed(textBuffer, true, false); return true; default: return false; diff --git a/src/terminal/adapter/adaptDispatch.hpp b/src/terminal/adapter/adaptDispatch.hpp index 1a283a1c987..a45ba89d439 100644 --- a/src/terminal/adapter/adaptDispatch.hpp +++ b/src/terminal/adapter/adaptDispatch.hpp @@ -213,6 +213,8 @@ namespace Microsoft::Console::VirtualTerminal void _DoSetTopBottomScrollingMargins(const VTInt topMargin, const VTInt bottomMargin); + void _DoLineFeed(TextBuffer& textBuffer, const bool withReturn, const bool wrapForced); + void _OperatingStatus() const; void _CursorPositionReport(const bool extendedReport); void _MacroSpaceReport() const; From 26ebc0a359a3946698167c6faa6b0717d94f9f98 Mon Sep 17 00:00:00 2001 From: James Holderness Date: Thu, 9 Feb 2023 11:13:43 +0000 Subject: [PATCH 02/13] Add API for conhost/terminal specific behavior. --- src/cascadia/TerminalCore/Terminal.cpp | 13 ++++ src/cascadia/TerminalCore/Terminal.hpp | 3 + src/cascadia/TerminalCore/TerminalApi.cpp | 78 ++++++++++++++++++- src/host/outputStream.cpp | 19 +++++ src/host/outputStream.hpp | 1 + src/terminal/adapter/ITerminalApi.hpp | 1 + src/terminal/adapter/adaptDispatch.cpp | 1 + .../adapter/ut_adapter/adapterTest.cpp | 5 ++ 8 files changed, 120 insertions(+), 1 deletion(-) diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index 7677910354d..536854bfba9 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -1221,6 +1221,19 @@ void Terminal::_AdjustCursorPosition(const til::point proposedPosition) } } +void Terminal::_PreserveUserScrollOffset(const int viewportDelta) noexcept +{ + // 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)) + { + const auto maxScrollOffset = _activeBuffer().GetSize().Height() - _mutableViewport.Height(); + _scrollOffset = std::min(_scrollOffset + viewportDelta, maxScrollOffset); + } +} + void Terminal::UserScrollViewport(const int viewTop) { if (_inAltBuffer()) diff --git a/src/cascadia/TerminalCore/Terminal.hpp b/src/cascadia/TerminalCore/Terminal.hpp index ec95bf8caf6..c8182d4f107 100644 --- a/src/cascadia/TerminalCore/Terminal.hpp +++ b/src/cascadia/TerminalCore/Terminal.hpp @@ -140,6 +140,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() override; #pragma endregion void ClearMark(); @@ -422,6 +423,8 @@ class Microsoft::Terminal::Core::Terminal final : void _AdjustCursorPosition(const til::point proposedPosition); + void _PreserveUserScrollOffset(const int viewportDelta) noexcept; + void _NotifyScrollEvent() noexcept; void _NotifyTerminalCursorPositionChanged() noexcept; diff --git a/src/cascadia/TerminalCore/TerminalApi.cpp b/src/cascadia/TerminalCore/TerminalApi.cpp index c4ec65d62e5..01255a058c7 100644 --- a/src/cascadia/TerminalCore/TerminalApi.cpp +++ b/src/cascadia/TerminalCore/TerminalApi.cpp @@ -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(); } } @@ -467,3 +469,77 @@ void Terminal::NotifyAccessibilityChange(const til::rect& /*changedRect*/) noexc { // This is only needed in conhost. Terminal handles accessibility in another way. } + +void Terminal::NotifyBufferRotation() +{ + // Update our selection, 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 = {}; + + const auto hasScrollMarks = _scrollMarks.size() > 0; + if (hasScrollMarks) + { + for (auto& mark : _scrollMarks) + { + // Move the mark up + mark.start.y -= 1; + + // If the mark had sub-regions, then move those pointers too + if (mark.commandEnd.has_value()) + { + (*mark.commandEnd).y -= 1; + } + if (mark.outputEnd.has_value()) + { + (*mark.outputEnd).y -= 1; + } + } + + _scrollMarks.erase(std::remove_if(_scrollMarks.begin(), + _scrollMarks.end(), + [](const auto& m) { return m.start.y < 0; }), + _scrollMarks.end()); + } + + const auto oldScrollOffset = _scrollOffset; + _PreserveUserScrollOffset(1); + if (_scrollOffset != oldScrollOffset || hasScrollMarks) + { + _NotifyScrollEvent(); + } +} diff --git a/src/host/outputStream.cpp b/src/host/outputStream.cpp index dbe8f16857e..55d85cb0e7d 100644 --- a/src/host/outputStream.cpp +++ b/src/host/outputStream.cpp @@ -474,6 +474,25 @@ void ConhostInternalGetSet::NotifyAccessibilityChange(const til::rect& changedRe } } +// Routine Description: +// - Implements conhost-specific behavior when the buffer is rotated. +// Arguments: +// - +// Return value: +// - +void ConhostInternalGetSet::NotifyBufferRotation() +{ + auto& screenInfo = _io.GetActiveOutputBuffer(); + if (screenInfo.IsActiveScreenBuffer()) + { + auto pNotifier = ServiceLocator::LocateAccessibilityNotifier(); + if (pNotifier) + { + pNotifier->NotifyConsoleUpdateScrollEvent(0, -1); + } + } +} + void ConhostInternalGetSet::MarkPrompt(const Microsoft::Console::VirtualTerminal::DispatchTypes::ScrollMark& /*mark*/) { // Not implemented for conhost. diff --git a/src/host/outputStream.hpp b/src/host/outputStream.hpp index f673e9110ba..84f7ae26f12 100644 --- a/src/host/outputStream.hpp +++ b/src/host/outputStream.hpp @@ -74,6 +74,7 @@ class ConhostInternalGetSet final : public Microsoft::Console::VirtualTerminal:: bool IsVtInputEnabled() const override; void NotifyAccessibilityChange(const til::rect& changedRect) override; + void NotifyBufferRotation() override; void MarkPrompt(const Microsoft::Console::VirtualTerminal::DispatchTypes::ScrollMark& mark) override; void MarkCommandStart() override; diff --git a/src/terminal/adapter/ITerminalApi.hpp b/src/terminal/adapter/ITerminalApi.hpp index 4705f382817..9d2c9fc6f3c 100644 --- a/src/terminal/adapter/ITerminalApi.hpp +++ b/src/terminal/adapter/ITerminalApi.hpp @@ -77,6 +77,7 @@ namespace Microsoft::Console::VirtualTerminal virtual bool IsConsolePty() const = 0; virtual void NotifyAccessibilityChange(const til::rect& changedRect) = 0; + virtual void NotifyBufferRotation() = 0; virtual void MarkPrompt(const Microsoft::Console::VirtualTerminal::DispatchTypes::ScrollMark& mark) = 0; virtual void MarkCommandStart() = 0; diff --git a/src/terminal/adapter/adaptDispatch.cpp b/src/terminal/adapter/adaptDispatch.cpp index 7c4c7fcc855..abfc498980c 100644 --- a/src/terminal/adapter/adaptDispatch.cpp +++ b/src/terminal/adapter/adaptDispatch.cpp @@ -2049,6 +2049,7 @@ void AdaptDispatch::_DoLineFeed(TextBuffer& textBuffer, const bool withReturn, c // so we cycle the row coordinates, which effectively scrolls the buffer // content up. In this case we don't need to move the cursor down. textBuffer.IncrementCircularBuffer(true); + _api.NotifyBufferRotation(); // We trigger a scroll rather than a redraw, since that's more efficient, // but we need to turn the cursor off before doing so, otherwise a ghost diff --git a/src/terminal/adapter/ut_adapter/adapterTest.cpp b/src/terminal/adapter/ut_adapter/adapterTest.cpp index 45cd8715666..665339be650 100644 --- a/src/terminal/adapter/ut_adapter/adapterTest.cpp +++ b/src/terminal/adapter/ut_adapter/adapterTest.cpp @@ -245,6 +245,11 @@ class TestGetSet final : public ITerminalApi Log::Comment(L"NotifyAccessibilityChange MOCK called..."); } + void NotifyBufferRotation() override + { + Log::Comment(L"NotifyBufferRotation MOCK called..."); + } + void MarkPrompt(const Microsoft::Console::VirtualTerminal::DispatchTypes::ScrollMark& /*mark*/) override { Log::Comment(L"MarkPrompt MOCK called..."); From b6974f0de561825afa8da226b1fb79018d6ef1b9 Mon Sep 17 00:00:00 2001 From: James Holderness Date: Sun, 12 Feb 2023 17:40:15 +0000 Subject: [PATCH 03/13] Allow for multiple buffer rotation cycles. --- src/cascadia/TerminalCore/Terminal.hpp | 2 +- src/cascadia/TerminalCore/TerminalApi.cpp | 45 +++++++------------ src/host/outputStream.cpp | 6 +-- src/host/outputStream.hpp | 2 +- src/terminal/adapter/ITerminalApi.hpp | 2 +- src/terminal/adapter/adaptDispatch.cpp | 2 +- .../adapter/ut_adapter/adapterTest.cpp | 2 +- 7 files changed, 23 insertions(+), 38 deletions(-) diff --git a/src/cascadia/TerminalCore/Terminal.hpp b/src/cascadia/TerminalCore/Terminal.hpp index c8182d4f107..8976bffd6d6 100644 --- a/src/cascadia/TerminalCore/Terminal.hpp +++ b/src/cascadia/TerminalCore/Terminal.hpp @@ -140,7 +140,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() override; + void NotifyBufferRotation(const int delta) override; #pragma endregion void ClearMark(); diff --git a/src/cascadia/TerminalCore/TerminalApi.cpp b/src/cascadia/TerminalCore/TerminalApi.cpp index 01255a058c7..2dd8d83105e 100644 --- a/src/cascadia/TerminalCore/TerminalApi.cpp +++ b/src/cascadia/TerminalCore/TerminalApi.cpp @@ -470,40 +470,25 @@ void Terminal::NotifyAccessibilityChange(const til::rect& /*changedRect*/) noexc // This is only needed in conhost. Terminal handles accessibility in another way. } -void Terminal::NotifyBufferRotation() +void Terminal::NotifyBufferRotation(const int delta) { // Update our selection, 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) + // 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->start.y -= 1; - _selection->end.y -= 1; + _selection.reset(); } 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()) - { + // 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; } } @@ -517,16 +502,16 @@ void Terminal::NotifyBufferRotation() for (auto& mark : _scrollMarks) { // Move the mark up - mark.start.y -= 1; + mark.start.y -= delta; // If the mark had sub-regions, then move those pointers too if (mark.commandEnd.has_value()) { - (*mark.commandEnd).y -= 1; + (*mark.commandEnd).y -= delta; } if (mark.outputEnd.has_value()) { - (*mark.outputEnd).y -= 1; + (*mark.outputEnd).y -= delta; } } @@ -537,7 +522,7 @@ void Terminal::NotifyBufferRotation() } const auto oldScrollOffset = _scrollOffset; - _PreserveUserScrollOffset(1); + _PreserveUserScrollOffset(delta); if (_scrollOffset != oldScrollOffset || hasScrollMarks) { _NotifyScrollEvent(); diff --git a/src/host/outputStream.cpp b/src/host/outputStream.cpp index 55d85cb0e7d..e2d5aabfb1a 100644 --- a/src/host/outputStream.cpp +++ b/src/host/outputStream.cpp @@ -477,10 +477,10 @@ void ConhostInternalGetSet::NotifyAccessibilityChange(const til::rect& changedRe // Routine Description: // - Implements conhost-specific behavior when the buffer is rotated. // Arguments: -// - +// - delta - the number of cycles that the buffer has rotated. // Return value: // - -void ConhostInternalGetSet::NotifyBufferRotation() +void ConhostInternalGetSet::NotifyBufferRotation(const int delta) { auto& screenInfo = _io.GetActiveOutputBuffer(); if (screenInfo.IsActiveScreenBuffer()) @@ -488,7 +488,7 @@ void ConhostInternalGetSet::NotifyBufferRotation() auto pNotifier = ServiceLocator::LocateAccessibilityNotifier(); if (pNotifier) { - pNotifier->NotifyConsoleUpdateScrollEvent(0, -1); + pNotifier->NotifyConsoleUpdateScrollEvent(0, -delta); } } } diff --git a/src/host/outputStream.hpp b/src/host/outputStream.hpp index 84f7ae26f12..a4d21215013 100644 --- a/src/host/outputStream.hpp +++ b/src/host/outputStream.hpp @@ -74,7 +74,7 @@ class ConhostInternalGetSet final : public Microsoft::Console::VirtualTerminal:: bool IsVtInputEnabled() const override; void NotifyAccessibilityChange(const til::rect& changedRect) override; - void NotifyBufferRotation() override; + void NotifyBufferRotation(const int delta) override; void MarkPrompt(const Microsoft::Console::VirtualTerminal::DispatchTypes::ScrollMark& mark) override; void MarkCommandStart() override; diff --git a/src/terminal/adapter/ITerminalApi.hpp b/src/terminal/adapter/ITerminalApi.hpp index 9d2c9fc6f3c..a25de666d4d 100644 --- a/src/terminal/adapter/ITerminalApi.hpp +++ b/src/terminal/adapter/ITerminalApi.hpp @@ -77,7 +77,7 @@ namespace Microsoft::Console::VirtualTerminal virtual bool IsConsolePty() const = 0; virtual void NotifyAccessibilityChange(const til::rect& changedRect) = 0; - virtual void NotifyBufferRotation() = 0; + virtual void NotifyBufferRotation(const int delta) = 0; virtual void MarkPrompt(const Microsoft::Console::VirtualTerminal::DispatchTypes::ScrollMark& mark) = 0; virtual void MarkCommandStart() = 0; diff --git a/src/terminal/adapter/adaptDispatch.cpp b/src/terminal/adapter/adaptDispatch.cpp index abfc498980c..421c1ebcf09 100644 --- a/src/terminal/adapter/adaptDispatch.cpp +++ b/src/terminal/adapter/adaptDispatch.cpp @@ -2049,7 +2049,7 @@ void AdaptDispatch::_DoLineFeed(TextBuffer& textBuffer, const bool withReturn, c // so we cycle the row coordinates, which effectively scrolls the buffer // content up. In this case we don't need to move the cursor down. textBuffer.IncrementCircularBuffer(true); - _api.NotifyBufferRotation(); + _api.NotifyBufferRotation(1); // We trigger a scroll rather than a redraw, since that's more efficient, // but we need to turn the cursor off before doing so, otherwise a ghost diff --git a/src/terminal/adapter/ut_adapter/adapterTest.cpp b/src/terminal/adapter/ut_adapter/adapterTest.cpp index 665339be650..dd951cdbddf 100644 --- a/src/terminal/adapter/ut_adapter/adapterTest.cpp +++ b/src/terminal/adapter/ut_adapter/adapterTest.cpp @@ -245,7 +245,7 @@ class TestGetSet final : public ITerminalApi Log::Comment(L"NotifyAccessibilityChange MOCK called..."); } - void NotifyBufferRotation() override + void NotifyBufferRotation(const int /*delta*/) override { Log::Comment(L"NotifyBufferRotation MOCK called..."); } From b8f8ef311a3dfc20ff81ea847e885be8da9ff2f5 Mon Sep 17 00:00:00 2001 From: James Holderness Date: Sun, 12 Feb 2023 17:56:08 +0000 Subject: [PATCH 04/13] Make sure ED sequence handles buffer rotation correctly. --- src/terminal/adapter/adaptDispatch.cpp | 56 ++++++++++++++++---------- src/terminal/adapter/adaptDispatch.hpp | 4 +- 2 files changed, 37 insertions(+), 23 deletions(-) diff --git a/src/terminal/adapter/adaptDispatch.cpp b/src/terminal/adapter/adaptDispatch.cpp index 421c1ebcf09..013c09705d0 100644 --- a/src/terminal/adapter/adaptDispatch.cpp +++ b/src/terminal/adapter/adaptDispatch.cpp @@ -696,23 +696,11 @@ bool AdaptDispatch::EraseInDisplay(const DispatchTypes::EraseType eraseType) // by moving the current contents of the viewport into the scrollback. if (eraseType == DispatchTypes::EraseType::Scrollback) { - _EraseScrollback(); - // GH#2715 - If this succeeded, but we're in a conpty, return `false` to - // make the state machine propagate this ED sequence to the connected - // terminal application. While we're in conpty mode, we don't really - // have a scrollback, but the attached terminal might. - return !_api.IsConsolePty(); + return _EraseScrollback(); } else if (eraseType == DispatchTypes::EraseType::All) { - // GH#5683 - If this succeeded, but we're in a conpty, return `false` to - // make the state machine propagate this ED sequence to the connected - // terminal application. While we're in conpty mode, when the client - // requests a Erase All operation, we need to manually tell the - // connected terminal to do the same thing, so that the terminal will - // move it's own buffer contents into the scrollback. - _EraseAll(); - return !_api.IsConsolePty(); + return _EraseAll(); } const auto viewport = _api.GetViewport(); @@ -2609,8 +2597,8 @@ bool AdaptDispatch::ScreenAlignmentPattern() // Arguments: // - // Return value: -// - -void AdaptDispatch::_EraseScrollback() +// - True if handled successfully. False otherwise. +bool AdaptDispatch::_EraseScrollback() { const auto viewport = _api.GetViewport(); const auto top = viewport.top; @@ -2631,6 +2619,12 @@ void AdaptDispatch::_EraseScrollback() // Move the cursor to the same relative location. cursor.SetYPosition(row - top); cursor.SetHasMoved(true); + + // GH#2715 - If this succeeded, but we're in a conpty, return `false` to + // make the state machine propagate this ED sequence to the connected + // terminal application. While we're in conpty mode, we don't really + // have a scrollback, but the attached terminal might. + return !_api.IsConsolePty(); } //Routine Description: @@ -2644,13 +2638,14 @@ void AdaptDispatch::_EraseScrollback() // Arguments: // - // Return value: -// - -void AdaptDispatch::_EraseAll() +// - True if handled successfully. False otherwise. +bool AdaptDispatch::_EraseAll() { const auto viewport = _api.GetViewport(); const auto viewportHeight = viewport.bottom - viewport.top; auto& textBuffer = _api.GetTextBuffer(); const auto bufferSize = textBuffer.GetSize(); + const auto inPtyMode = _api.IsConsolePty(); // Stash away the current position of the cursor within the viewport. // We'll need to restore the cursor to that same relative position, after @@ -2665,10 +2660,21 @@ void AdaptDispatch::_EraseAll() auto newViewportTop = lastChar == til::point{} ? 0 : lastChar.y + 1; const auto newViewportBottom = newViewportTop + viewportHeight; const auto delta = newViewportBottom - (bufferSize.Height()); - for (auto i = 0; i < delta; i++) + if (delta > 0) { - textBuffer.IncrementCircularBuffer(); - newViewportTop--; + for (auto i = 0; i < delta; i++) + { + textBuffer.IncrementCircularBuffer(); + } + _api.NotifyBufferRotation(delta); + newViewportTop -= delta; + // We don't want to trigger a scroll in pty mode, because we're going to + // pass through the ED sequence anyway, and this will just result in the + // buffer being scrolled up by two pages instead of one. + if (!inPtyMode) + { + textBuffer.TriggerScroll({ 0, -delta }); + } } // Move the viewport _api.SetViewportPosition({ viewport.left, newViewportTop }); @@ -2683,6 +2689,14 @@ void AdaptDispatch::_EraseAll() // Also reset the line rendition for the erased rows. textBuffer.ResetLineRenditionRange(newViewportTop, newViewportBottom); + + // GH#5683 - If this succeeded, but we're in a conpty, return `false` to + // make the state machine propagate this ED sequence to the connected + // terminal application. While we're in conpty mode, when the client + // requests a Erase All operation, we need to manually tell the + // connected terminal to do the same thing, so that the terminal will + // move it's own buffer contents into the scrollback. + return !inPtyMode; } //Routine Description: diff --git a/src/terminal/adapter/adaptDispatch.hpp b/src/terminal/adapter/adaptDispatch.hpp index a45ba89d439..493c63f69c4 100644 --- a/src/terminal/adapter/adaptDispatch.hpp +++ b/src/terminal/adapter/adaptDispatch.hpp @@ -203,8 +203,8 @@ namespace Microsoft::Console::VirtualTerminal void _ChangeRectAttributes(TextBuffer& textBuffer, const til::rect& changeRect, const ChangeOps& changeOps); void _ChangeRectOrStreamAttributes(const til::rect& changeArea, const ChangeOps& changeOps); til::rect _CalculateRectArea(const VTInt top, const VTInt left, const VTInt bottom, const VTInt right, const til::size bufferSize); - void _EraseScrollback(); - void _EraseAll(); + bool _EraseScrollback(); + bool _EraseAll(); void _ScrollRectVertically(TextBuffer& textBuffer, const til::rect& scrollRect, const VTInt delta); void _ScrollRectHorizontally(TextBuffer& textBuffer, const til::rect& scrollRect, const VTInt delta); void _InsertDeleteCharacterHelper(const VTInt delta); From aa93940da9bfa76e7db648159544cd816365fd5f Mon Sep 17 00:00:00 2001 From: James Holderness Date: Sun, 12 Feb 2023 22:23:27 +0000 Subject: [PATCH 05/13] Fix unit tests. --- .../ConptyRoundtripTests.cpp | 41 ++++++++++--------- src/host/ut_host/ScreenBufferTests.cpp | 1 + .../adapter/ut_adapter/adapterTest.cpp | 33 +-------------- 3 files changed, 24 insertions(+), 51 deletions(-) diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index 1bb2f033e1f..dc66b3a03ed 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -1700,11 +1700,12 @@ 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. @@ -1712,44 +1713,46 @@ void ConptyRoundtripTests::ScrollWithMargins() { // Start with B this time because the A line got scrolled off the top. const std::wstring expectedString(4, static_cast(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('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 @@ -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() diff --git a/src/host/ut_host/ScreenBufferTests.cpp b/src/host/ut_host/ScreenBufferTests.cpp index 33d9eacd78b..5a32660d914 100644 --- a/src/host/ut_host/ScreenBufferTests.cpp +++ b/src/host/ut_host/ScreenBufferTests.cpp @@ -1646,6 +1646,7 @@ void ScreenBufferTests::VtNewlineOutsideMargins() Log::Comment(L"Reset viewport and apply DECSTBM margins"); VERIFY_SUCCEEDED(si.SetViewportOrigin(true, { 0, viewportTop }, true)); + si.UpdateBottom(); stateMachine.ProcessString(L"\x1b[1;5r"); // Make sure we clear the margins on exit so they can't break other tests. auto clearMargins = wil::scope_exit([&] { stateMachine.ProcessString(L"\x1b[r"); }); diff --git a/src/terminal/adapter/ut_adapter/adapterTest.cpp b/src/terminal/adapter/ut_adapter/adapterTest.cpp index dd951cdbddf..277c1d2ce6e 100644 --- a/src/terminal/adapter/ut_adapter/adapterTest.cpp +++ b/src/terminal/adapter/ut_adapter/adapterTest.cpp @@ -143,12 +143,9 @@ class TestGetSet final : public ITerminalApi return _getLineFeedModeResult; } - void LineFeed(const bool withReturn, const bool /*wrapForced*/) override + void LineFeed(const bool /*withReturn*/, const bool /*wrapForced*/) override { Log::Comment(L"LineFeed MOCK called..."); - - THROW_HR_IF(E_FAIL, !_lineFeedResult); - VERIFY_ARE_EQUAL(_expectedLineFeedWithReturn, withReturn); } void SetWindowTitle(const std::wstring_view title) @@ -418,8 +415,6 @@ class TestGetSet final : public ITerminalApi bool _setScrollingRegionResult = false; bool _getLineFeedModeResult = false; - bool _lineFeedResult = false; - bool _expectedLineFeedWithReturn = false; bool _setWindowTitleResult = false; std::wstring_view _expectedWindowTitle{}; @@ -1974,32 +1969,6 @@ class AdapterTest VERIFY_ARE_EQUAL(til::inclusive_rect{}, _testGetSet->_activeScrollRegion); } - TEST_METHOD(LineFeedTest) - { - Log::Comment(L"Starting test..."); - - // All test cases need the LineFeed call to succeed. - _testGetSet->_lineFeedResult = TRUE; - - Log::Comment(L"Test 1: Line feed without carriage return."); - _testGetSet->_expectedLineFeedWithReturn = false; - VERIFY_IS_TRUE(_pDispatch->LineFeed(DispatchTypes::LineFeedType::WithoutReturn)); - - Log::Comment(L"Test 2: Line feed with carriage return."); - _testGetSet->_expectedLineFeedWithReturn = true; - VERIFY_IS_TRUE(_pDispatch->LineFeed(DispatchTypes::LineFeedType::WithReturn)); - - Log::Comment(L"Test 3: Line feed depends on mode, and mode reset."); - _testGetSet->_getLineFeedModeResult = false; - _testGetSet->_expectedLineFeedWithReturn = false; - VERIFY_IS_TRUE(_pDispatch->LineFeed(DispatchTypes::LineFeedType::DependsOnMode)); - - Log::Comment(L"Test 4: Line feed depends on mode, and mode set."); - _testGetSet->_getLineFeedModeResult = true; - _testGetSet->_expectedLineFeedWithReturn = true; - VERIFY_IS_TRUE(_pDispatch->LineFeed(DispatchTypes::LineFeedType::DependsOnMode)); - } - TEST_METHOD(SetConsoleTitleTest) { Log::Comment(L"Starting test..."); From 5042d82b30c9324f7cd70798c7780e763616cd8c Mon Sep 17 00:00:00 2001 From: James Holderness Date: Tue, 14 Feb 2023 00:29:50 +0000 Subject: [PATCH 06/13] Removed unused LineFeed API. --- src/cascadia/TerminalCore/Terminal.cpp | 147 +----------------- src/cascadia/TerminalCore/Terminal.hpp | 3 - src/cascadia/TerminalCore/TerminalApi.cpp | 16 -- src/host/outputStream.cpp | 30 ---- src/host/outputStream.hpp | 1 - src/terminal/adapter/ITerminalApi.hpp | 1 - .../adapter/ut_adapter/adapterTest.cpp | 5 - 7 files changed, 1 insertion(+), 202 deletions(-) diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index 536854bfba9..f879e9216fe 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -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(); @@ -1078,149 +1076,6 @@ Viewport Terminal::_GetVisibleViewport() const noexcept size); } -void Terminal::_AdjustCursorPosition(const til::point proposedPosition) -{ -#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()) - { - 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); - } -} - void Terminal::_PreserveUserScrollOffset(const int viewportDelta) noexcept { // When the mutable viewport is moved down, and there's an active selection, diff --git a/src/cascadia/TerminalCore/Terminal.hpp b/src/cascadia/TerminalCore/Terminal.hpp index 8976bffd6d6..303e34b84c3 100644 --- a/src/cascadia/TerminalCore/Terminal.hpp +++ b/src/cascadia/TerminalCore/Terminal.hpp @@ -116,7 +116,6 @@ class Microsoft::Terminal::Core::Terminal final : 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; @@ -421,8 +420,6 @@ 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; diff --git a/src/cascadia/TerminalCore/TerminalApi.cpp b/src/cascadia/TerminalCore/TerminalApi.cpp index 2dd8d83105e..be2a778c67b 100644 --- a/src/cascadia/TerminalCore/TerminalApi.cpp +++ b/src/cascadia/TerminalCore/TerminalApi.cpp @@ -88,22 +88,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) diff --git a/src/host/outputStream.cpp b/src/host/outputStream.cpp index e2d5aabfb1a..63fe3c8ffd7 100644 --- a/src/host/outputStream.cpp +++ b/src/host/outputStream.cpp @@ -170,36 +170,6 @@ bool ConhostInternalGetSet::GetLineFeedMode() const return WI_IsFlagClear(screenInfo.OutputMode, DISABLE_NEWLINE_AUTO_RETURN); } -// Routine Description: -// - Performs a line feed, possibly preceded by carriage return. -// Arguments: -// - withReturn - Set to true if a carriage return should be performed as well. -// - wrapForced - Set to true is the line feed was the result of the line wrapping. -// Return Value: -// - -void ConhostInternalGetSet::LineFeed(const bool withReturn, const bool wrapForced) -{ - auto& screenInfo = _io.GetActiveOutputBuffer(); - auto& textBuffer = screenInfo.GetTextBuffer(); - auto cursorPosition = textBuffer.GetCursor().GetPosition(); - - // If the line was forced to wrap, set the wrap status. - // When explicitly moving down a row, clear the wrap status. - textBuffer.GetRowByOffset(cursorPosition.y).SetWrapForced(wrapForced); - - cursorPosition.y += 1; - if (withReturn) - { - cursorPosition.x = 0; - } - else - { - cursorPosition = textBuffer.ClampPositionWithinLine(cursorPosition); - } - - THROW_IF_NTSTATUS_FAILED(AdjustCursorPosition(screenInfo, cursorPosition, FALSE, nullptr)); -} - // Routine Description: // - Sends a notify message to play the "SystemHand" sound event. // Return Value: diff --git a/src/host/outputStream.hpp b/src/host/outputStream.hpp index a4d21215013..c1f41bd532b 100644 --- a/src/host/outputStream.hpp +++ b/src/host/outputStream.hpp @@ -46,7 +46,6 @@ class ConhostInternalGetSet final : public Microsoft::Console::VirtualTerminal:: void WarningBell() override; bool GetLineFeedMode() const override; - void LineFeed(const bool withReturn, const bool wrapForced) override; void SetWindowTitle(const std::wstring_view title) override; diff --git a/src/terminal/adapter/ITerminalApi.hpp b/src/terminal/adapter/ITerminalApi.hpp index a25de666d4d..0a7a5bc44e5 100644 --- a/src/terminal/adapter/ITerminalApi.hpp +++ b/src/terminal/adapter/ITerminalApi.hpp @@ -54,7 +54,6 @@ namespace Microsoft::Console::VirtualTerminal virtual void SetScrollingRegion(const til::inclusive_rect& scrollMargins) = 0; virtual void WarningBell() = 0; virtual bool GetLineFeedMode() const = 0; - virtual void LineFeed(const bool withReturn, const bool wrapForced) = 0; virtual void SetWindowTitle(const std::wstring_view title) = 0; virtual void UseAlternateScreenBuffer() = 0; virtual void UseMainScreenBuffer() = 0; diff --git a/src/terminal/adapter/ut_adapter/adapterTest.cpp b/src/terminal/adapter/ut_adapter/adapterTest.cpp index 277c1d2ce6e..6ba5f4c6cfa 100644 --- a/src/terminal/adapter/ut_adapter/adapterTest.cpp +++ b/src/terminal/adapter/ut_adapter/adapterTest.cpp @@ -143,11 +143,6 @@ class TestGetSet final : public ITerminalApi return _getLineFeedModeResult; } - void LineFeed(const bool /*withReturn*/, const bool /*wrapForced*/) override - { - Log::Comment(L"LineFeed MOCK called..."); - } - void SetWindowTitle(const std::wstring_view title) { Log::Comment(L"SetWindowTitle MOCK called..."); From dbd9ef32ed5c79fa15b2f61233286f7566d3d64f Mon Sep 17 00:00:00 2001 From: James Holderness Date: Tue, 14 Feb 2023 01:01:26 +0000 Subject: [PATCH 07/13] Remove unneeded VT code from AdjustCursorPosition. --- src/host/_stream.cpp | 190 +------------------------------------------ 1 file changed, 1 insertion(+), 189 deletions(-) diff --git a/src/host/_stream.cpp b/src/host/_stream.cpp index a86149cc6d6..d6f1e87e80e 100644 --- a/src/host/_stream.cpp +++ b/src/host/_stream.cpp @@ -44,7 +44,6 @@ using Microsoft::Console::VirtualTerminal::StateMachine; const BOOL fKeepCursorVisible, _Inout_opt_ til::CoordType* psScrollY) { - const auto inVtMode = WI_IsFlagSet(screenInfo.OutputMode, ENABLE_VIRTUAL_TERMINAL_PROCESSING); const auto bufferSize = screenInfo.GetBufferSize().Dimensions(); if (coordCursor.x < 0) { @@ -68,180 +67,8 @@ using Microsoft::Console::VirtualTerminal::StateMachine; } else { - if (inVtMode) - { - // In VT mode, the cursor must be left in the last column. - coordCursor.x = bufferSize.width - 1; - } - else - { - // For legacy apps, it is left where it was at the start of the write. - coordCursor.x = screenInfo.GetTextBuffer().GetCursor().GetPosition().x; - } - } - } - - // The VT standard requires the lines revealed when scrolling are filled - // with the current background color, but with no meta attributes set. - auto fillAttributes = screenInfo.GetAttributes(); - fillAttributes.SetStandardErase(); - - const auto relativeMargins = screenInfo.GetRelativeScrollMargins(); - auto viewport = screenInfo.GetViewport(); - auto srMargins = screenInfo.GetAbsoluteScrollMargins().ToInclusive(); - const auto fMarginsSet = srMargins.bottom > srMargins.top; - auto currentCursor = screenInfo.GetTextBuffer().GetCursor().GetPosition(); - const auto iCurrentCursorY = currentCursor.y; - - const auto fCursorInMargins = iCurrentCursorY <= srMargins.bottom && iCurrentCursorY >= srMargins.top; - const auto cursorAboveViewport = coordCursor.y < 0 && inVtMode; - const auto fScrollDown = fMarginsSet && fCursorInMargins && (coordCursor.y > srMargins.bottom); - auto fScrollUp = fMarginsSet && fCursorInMargins && (coordCursor.y < srMargins.top); - - const auto fScrollUpWithoutMargins = (!fMarginsSet) && cursorAboveViewport; - // if we're in VT mode, AND MARGINS AREN'T SET and a Reverse Line Feed took the cursor up past the top of the viewport, - // VT style scroll the contents of the screen. - // This can happen in applications like `less`, that don't set margins, because they're going to - // scroll the entire screen anyways, so no need for them to ever set the margins. - if (fScrollUpWithoutMargins) - { - fScrollUp = true; - srMargins.top = 0; - srMargins.bottom = screenInfo.GetViewport().BottomInclusive(); - } - - const auto scrollDownAtTop = fScrollDown && relativeMargins.Top() == 0; - if (scrollDownAtTop) - { - // We're trying to scroll down, and the top margin is at the top of the viewport. - // In this case, we want the lines that are "scrolled off" to appear in - // the scrollback instead of being discarded. - // To do this, we're going to scroll everything starting at the bottom - // margin down, then move the viewport down. - - const auto delta = coordCursor.y - srMargins.bottom; - til::inclusive_rect scrollRect; - scrollRect.left = 0; - scrollRect.top = srMargins.bottom + 1; // One below margins - scrollRect.bottom = bufferSize.height - 1; // -1, otherwise this would be an exclusive rect. - scrollRect.right = bufferSize.width - 1; // -1, otherwise this would be an exclusive rect. - - // This is the Y position we're moving the contents below the bottom margin to. - auto moveToYPosition = scrollRect.top + delta; - - // This is where the viewport will need to be to give the effect of - // scrolling the contents in the margins. - auto newViewTop = viewport.Top() + delta; - - // This is how many new lines need to be added to the buffer to support this operation. - const auto newRows = (viewport.BottomExclusive() + delta) - bufferSize.height; - - // If we're near the bottom of the buffer, we might need to insert some - // new rows at the bottom. - // If we do this, then the viewport is now one line higher than it used - // to be, so it needs to move down by one less line. - for (auto i = 0; i < newRows; i++) - { - screenInfo.GetTextBuffer().IncrementCircularBuffer(); - moveToYPosition--; - newViewTop--; - scrollRect.top--; - } - - const til::point newPostMarginsOrigin{ 0, moveToYPosition }; - const til::point newViewOrigin{ 0, newViewTop }; - - try - { - ScrollRegion(screenInfo, scrollRect, std::nullopt, newPostMarginsOrigin, UNICODE_SPACE, fillAttributes); - } - CATCH_LOG(); - - // Move the viewport down - auto hr = screenInfo.SetViewportOrigin(true, newViewOrigin, true); - if (FAILED(hr)) - { - return NTSTATUS_FROM_HRESULT(hr); + coordCursor.x = screenInfo.GetTextBuffer().GetCursor().GetPosition().x; } - // If we didn't actually move the viewport, it's because we're at the - // bottom of the buffer, and the top lines of the viewport have - // changed. Manually invalidate here, to make sure the screen - // displays the correct text. - if (newViewOrigin == viewport.Origin()) - { - // Inside this block, we're shifting down at the bottom. - // This means that we had something like this: - // AAAA - // BBBB - // CCCC - // DDDD - // EEEE - // - // Our margins were set for lines A-D, but not on line E. - // So we circled the whole buffer up by one: - // BBBB - // CCCC - // DDDD - // EEEE - // - // - // Then we scrolled the contents of everything OUTSIDE the margin frame down. - // BBBB - // CCCC - // DDDD - // - // EEEE - // - // And now we need to report that only the bottom line didn't "move" as we put the EEEE - // back where it started, but everything else moved. - // In this case, delta was 1. So the amount that moved is the entire viewport height minus the delta. - auto invalid = Viewport::FromDimensions(viewport.Origin(), { viewport.Width(), viewport.Height() - delta }); - screenInfo.GetTextBuffer().TriggerRedraw(invalid); - } - - // reset where our local viewport is, and recalculate the cursor and - // margin positions. - viewport = screenInfo.GetViewport(); - if (newRows > 0) - { - currentCursor.y -= newRows; - coordCursor.y -= newRows; - } - srMargins = screenInfo.GetAbsoluteScrollMargins().ToInclusive(); - } - - // If we did the above scrollDownAtTop case, then we've already scrolled - // the margins content, and we can skip this. - if (fScrollUp || (fScrollDown && !scrollDownAtTop)) - { - auto diff = coordCursor.y - (fScrollUp ? srMargins.top : srMargins.bottom); - - til::inclusive_rect scrollRect; - scrollRect.top = srMargins.top; - scrollRect.bottom = srMargins.bottom; - scrollRect.left = 0; // NOTE: Left/Right Scroll margins don't do anything currently. - scrollRect.right = bufferSize.width - 1; // -1, otherwise this would be an exclusive rect. - - til::point dest; - dest.x = scrollRect.left; - dest.y = scrollRect.top - diff; - - try - { - ScrollRegion(screenInfo, scrollRect, scrollRect, dest, UNICODE_SPACE, fillAttributes); - } - CATCH_LOG(); - - coordCursor.y -= diff; - } - - // If the margins are set, then it shouldn't be possible for the cursor to - // move below the bottom of the viewport. Either it should be constrained - // inside the margins by one of the scrollDown cases handled above, or - // we'll need to clamp it inside the viewport here. - if (fMarginsSet && coordCursor.y > viewport.BottomInclusive()) - { - coordCursor.y = viewport.BottomInclusive(); } auto Status = STATUS_SUCCESS; @@ -263,7 +90,6 @@ using Microsoft::Console::VirtualTerminal::StateMachine; } const auto cursorMovedPastViewport = coordCursor.y > screenInfo.GetViewport().BottomInclusive(); - const auto cursorMovedPastVirtualViewport = coordCursor.y > screenInfo.GetVirtualViewport().BottomInclusive(); if (SUCCEEDED_NTSTATUS(Status)) { // if at right or bottom edge of window, scroll right or down one char. @@ -283,20 +109,6 @@ using Microsoft::Console::VirtualTerminal::StateMachine; screenInfo.MakeCursorVisible(coordCursor); } Status = screenInfo.SetCursorPosition(coordCursor, !!fKeepCursorVisible); - - // MSFT:19989333 - Only re-initialize the cursor row if the cursor moved - // below the terminal section of the buffer (the virtual viewport), - // and the visible part of the buffer (the actual viewport). - // If this is only cursorMovedPastViewport, and you scroll up, then type - // a character, we'll re-initialize the line the cursor is on. - // If this is only cursorMovedPastVirtualViewport and you scroll down, - // (with terminal scrolling disabled) then all lines newly exposed - // will get their attributes constantly cleared out. - // Both cursorMovedPastViewport and cursorMovedPastVirtualViewport works - if (inVtMode && cursorMovedPastViewport && cursorMovedPastVirtualViewport) - { - screenInfo.InitializeCursorRowAttributes(); - } } return Status; From 1b9498563fe432d569bf37860453e446692269d6 Mon Sep 17 00:00:00 2001 From: James Holderness Date: Tue, 14 Feb 2023 01:36:20 +0000 Subject: [PATCH 08/13] Remove unneeded margin APIs. --- src/cascadia/TerminalCore/Terminal.hpp | 1 - src/cascadia/TerminalCore/TerminalApi.cpp | 5 --- src/host/outputStream.cpp | 20 ------------ src/host/outputStream.hpp | 2 -- src/host/screenInfo.cpp | 32 ------------------- src/host/screenInfo.hpp | 6 ---- src/terminal/adapter/ITerminalApi.hpp | 1 - src/terminal/adapter/adaptDispatch.cpp | 4 +-- src/terminal/adapter/adaptDispatch.hpp | 2 +- .../adapter/ut_adapter/adapterTest.cpp | 11 ------- 10 files changed, 2 insertions(+), 82 deletions(-) diff --git a/src/cascadia/TerminalCore/Terminal.hpp b/src/cascadia/TerminalCore/Terminal.hpp index 303e34b84c3..e851f826866 100644 --- a/src/cascadia/TerminalCore/Terminal.hpp +++ b/src/cascadia/TerminalCore/Terminal.hpp @@ -113,7 +113,6 @@ 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 SetWindowTitle(const std::wstring_view title) override; diff --git a/src/cascadia/TerminalCore/TerminalApi.cpp b/src/cascadia/TerminalCore/TerminalApi.cpp index be2a778c67b..f861b16edc3 100644 --- a/src/cascadia/TerminalCore/TerminalApi.cpp +++ b/src/cascadia/TerminalCore/TerminalApi.cpp @@ -72,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(); diff --git a/src/host/outputStream.cpp b/src/host/outputStream.cpp index 63fe3c8ffd7..82be404df62 100644 --- a/src/host/outputStream.cpp +++ b/src/host/outputStream.cpp @@ -138,26 +138,6 @@ bool ConhostInternalGetSet::GetAutoWrapMode() const return WI_IsFlagSet(outputMode, ENABLE_WRAP_AT_EOL_OUTPUT); } -// Routine Description: -// - Sets the top and bottom scrolling margins for the current page. This creates -// a subsection of the screen that scrolls when input reaches the end of the -// region, leaving the rest of the screen untouched. -// Arguments: -// - scrollMargins - A rect who's Top and Bottom members will be used to set -// the new values of the top and bottom margins. If (0,0), then the margins -// will be disabled. NOTE: This is a rect in the case that we'll need the -// left and right margins in the future. -// Return Value: -// - -void ConhostInternalGetSet::SetScrollingRegion(const til::inclusive_rect& scrollMargins) -{ - auto& screenInfo = _io.GetActiveOutputBuffer(); - auto srScrollMargins = screenInfo.GetRelativeScrollMargins().ToInclusive(); - srScrollMargins.top = scrollMargins.top; - srScrollMargins.bottom = scrollMargins.bottom; - screenInfo.SetScrollMargins(Viewport::FromInclusive(srScrollMargins)); -} - // Method Description: // - Retrieves the current Line Feed/New Line (LNM) mode. // Arguments: diff --git a/src/host/outputStream.hpp b/src/host/outputStream.hpp index c1f41bd532b..030dae3f185 100644 --- a/src/host/outputStream.hpp +++ b/src/host/outputStream.hpp @@ -41,8 +41,6 @@ class ConhostInternalGetSet final : public Microsoft::Console::VirtualTerminal:: void SetAutoWrapMode(const bool wrapAtEOL) override; bool GetAutoWrapMode() const override; - void SetScrollingRegion(const til::inclusive_rect& scrollMargins) override; - void WarningBell() override; bool GetLineFeedMode() const override; diff --git a/src/host/screenInfo.cpp b/src/host/screenInfo.cpp index 32395d2f520..d595ec8efb1 100644 --- a/src/host/screenInfo.cpp +++ b/src/host/screenInfo.cpp @@ -47,7 +47,6 @@ SCREEN_INFORMATION::SCREEN_INFORMATION( _pAccessibilityNotifier{ pNotifier }, _api{ *this }, _stateMachine{ nullptr }, - _scrollMargins{ Viewport::Empty() }, _viewport(Viewport::Empty()), _psiAlternateBuffer{ nullptr }, _psiMainBuffer{ nullptr }, @@ -1799,37 +1798,6 @@ void SCREEN_INFORMATION::MakeCursorVisible(const til::point CursorPosition) } } -// Method Description: -// - Sets the scroll margins for this buffer. -// Arguments: -// - margins: The new values of the scroll margins, *relative to the viewport* -void SCREEN_INFORMATION::SetScrollMargins(const Viewport margins) -{ - _scrollMargins = margins; -} - -// Method Description: -// - Returns the scrolling margins boundaries for this screen buffer, relative -// to the origin of the text buffer. Most callers will want the absolute -// positions of the margins, though they are set and stored relative to -// origin of the viewport. -// Arguments: -// - -Viewport SCREEN_INFORMATION::GetAbsoluteScrollMargins() const -{ - return _viewport.ConvertFromOrigin(_scrollMargins); -} - -// Method Description: -// - Returns the scrolling margins boundaries for this screen buffer, relative -// to the current viewport. -// Arguments: -// - -Viewport SCREEN_INFORMATION::GetRelativeScrollMargins() const -{ - return _scrollMargins; -} - // Routine Description: // - Retrieves the active buffer of this buffer. If this buffer has an // alternate buffer, this is the alternate buffer. Otherwise, it is this buffer. diff --git a/src/host/screenInfo.hpp b/src/host/screenInfo.hpp index 6845f369ed3..2f6bbd0cc47 100644 --- a/src/host/screenInfo.hpp +++ b/src/host/screenInfo.hpp @@ -193,10 +193,6 @@ class SCREEN_INFORMATION : public ConsoleObjectHeader, public Microsoft::Console void MakeCursorVisible(const til::point CursorPosition); - Microsoft::Console::Types::Viewport GetRelativeScrollMargins() const; - Microsoft::Console::Types::Viewport GetAbsoluteScrollMargins() const; - void SetScrollMargins(const Microsoft::Console::Types::Viewport margins); - [[nodiscard]] NTSTATUS UseAlternateScreenBuffer(); void UseMainScreenBuffer(); @@ -270,8 +266,6 @@ class SCREEN_INFORMATION : public ConsoleObjectHeader, public Microsoft::Console std::shared_ptr _stateMachine; - Microsoft::Console::Types::Viewport _scrollMargins; //The margins of the VT specified scroll region. Left and Right are currently unused, but could be in the future. - // Specifies which coordinates of the screen buffer are visible in the // window client (the "viewport" into the buffer) Microsoft::Console::Types::Viewport _viewport; diff --git a/src/terminal/adapter/ITerminalApi.hpp b/src/terminal/adapter/ITerminalApi.hpp index 0a7a5bc44e5..bcf635907a9 100644 --- a/src/terminal/adapter/ITerminalApi.hpp +++ b/src/terminal/adapter/ITerminalApi.hpp @@ -51,7 +51,6 @@ namespace Microsoft::Console::VirtualTerminal virtual void SetAutoWrapMode(const bool wrapAtEOL) = 0; virtual bool GetAutoWrapMode() const = 0; - virtual void SetScrollingRegion(const til::inclusive_rect& scrollMargins) = 0; virtual void WarningBell() = 0; virtual bool GetLineFeedMode() const = 0; virtual void SetWindowTitle(const std::wstring_view title) = 0; diff --git a/src/terminal/adapter/adaptDispatch.cpp b/src/terminal/adapter/adaptDispatch.cpp index 013c09705d0..da4ac646ff4 100644 --- a/src/terminal/adapter/adaptDispatch.cpp +++ b/src/terminal/adapter/adaptDispatch.cpp @@ -252,14 +252,13 @@ bool AdaptDispatch::CursorPrevLine(const VTInt distance) // - absolute - Should coordinates be absolute or relative to the viewport. // Return Value: // - A std::pair containing the top and bottom coordinates (inclusive). -std::pair AdaptDispatch::_GetVerticalMargins(const til::rect& viewport, const bool absolute) +std::pair AdaptDispatch::_GetVerticalMargins(const til::rect& viewport, const bool absolute) noexcept { // If the top is out of range, reset the margins completely. const auto bottommostRow = viewport.bottom - viewport.top - 1; if (_scrollMargins.top >= bottommostRow) { _scrollMargins.top = _scrollMargins.bottom = 0; - _api.SetScrollingRegion(_scrollMargins); } // If margins aren't set, use the full extent of the viewport. const auto marginsSet = _scrollMargins.top < _scrollMargins.bottom; @@ -1923,7 +1922,6 @@ void AdaptDispatch::_DoSetTopBottomScrollingMargins(const VTInt topMargin, } _scrollMargins.top = actualTop; _scrollMargins.bottom = actualBottom; - _api.SetScrollingRegion(_scrollMargins); } } diff --git a/src/terminal/adapter/adaptDispatch.hpp b/src/terminal/adapter/adaptDispatch.hpp index 493c63f69c4..7880cb4f6ce 100644 --- a/src/terminal/adapter/adaptDispatch.hpp +++ b/src/terminal/adapter/adaptDispatch.hpp @@ -195,7 +195,7 @@ namespace Microsoft::Console::VirtualTerminal }; void _WriteToBuffer(const std::wstring_view string); - std::pair _GetVerticalMargins(const til::rect& viewport, const bool absolute); + std::pair _GetVerticalMargins(const til::rect& viewport, const bool absolute) noexcept; bool _CursorMovePosition(const Offset rowOffset, const Offset colOffset, const bool clampInMargins); void _ApplyCursorMovementFlags(Cursor& cursor) noexcept; void _FillRect(TextBuffer& textBuffer, const til::rect& fillRect, const wchar_t fillChar, const TextAttribute fillAttrs); diff --git a/src/terminal/adapter/ut_adapter/adapterTest.cpp b/src/terminal/adapter/ut_adapter/adapterTest.cpp index 6ba5f4c6cfa..9ec22f372e5 100644 --- a/src/terminal/adapter/ut_adapter/adapterTest.cpp +++ b/src/terminal/adapter/ut_adapter/adapterTest.cpp @@ -121,17 +121,6 @@ class TestGetSet final : public ITerminalApi _textBuffer->SetCurrentAttributes(attrs); } - void SetScrollingRegion(const til::inclusive_rect& scrollMargins) override - { - Log::Comment(L"SetScrollingRegion MOCK called..."); - - if (_setScrollingRegionResult) - { - VERIFY_ARE_EQUAL(_expectedScrollRegion, scrollMargins); - _activeScrollRegion = scrollMargins; - } - } - void WarningBell() override { Log::Comment(L"WarningBell MOCK called..."); From 90bd976c753f15bfc2b90d7579df41ce9b2713ef Mon Sep 17 00:00:00 2001 From: James Holderness Date: Tue, 14 Feb 2023 10:20:06 +0000 Subject: [PATCH 09/13] Rewrite margin adapter test. --- .../adapter/ut_adapter/adapterTest.cpp | 105 ++++++------------ 1 file changed, 32 insertions(+), 73 deletions(-) diff --git a/src/terminal/adapter/ut_adapter/adapterTest.cpp b/src/terminal/adapter/ut_adapter/adapterTest.cpp index 9ec22f372e5..ee45fa0458d 100644 --- a/src/terminal/adapter/ut_adapter/adapterTest.cpp +++ b/src/terminal/adapter/ut_adapter/adapterTest.cpp @@ -352,15 +352,6 @@ class TestGetSet final : public ITerminalApi VERIFY_ARE_EQUAL(pwszExpectedResponse, _response); } - void _SetMarginsHelper(til::inclusive_rect* rect, til::CoordType top, til::CoordType bottom) - { - rect->top = top; - rect->bottom = bottom; - //The rectangle is going to get converted from VT space to conhost space - _expectedScrollRegion.top = (top > 0) ? rect->top - 1 : rect->top; - _expectedScrollRegion.bottom = (bottom > 0) ? rect->bottom - 1 : rect->bottom; - } - ~TestGetSet() = default; static const WCHAR s_wchErase = (WCHAR)0x20; @@ -385,8 +376,6 @@ class TestGetSet final : public ITerminalApi DummyRenderer _renderer; std::unique_ptr _textBuffer; til::inclusive_rect _viewport; - til::inclusive_rect _expectedScrollRegion; - til::inclusive_rect _activeScrollRegion; til::point _expectedCursorPos; @@ -397,7 +386,6 @@ class TestGetSet final : public ITerminalApi bool _setTextAttributesResult = false; bool _returnResponseResult = false; - bool _setScrollingRegionResult = false; bool _getLineFeedModeResult = false; bool _setWindowTitleResult = false; @@ -1864,93 +1852,64 @@ class AdapterTest auto sScreenHeight = _testGetSet->_viewport.bottom - _testGetSet->_viewport.top; Log::Comment(L"Test 1: Verify having both values is valid."); - _testGetSet->_SetMarginsHelper(&srTestMargins, 2, 6); - _testGetSet->_setScrollingRegionResult = TRUE; - VERIFY_IS_TRUE(_pDispatch->SetTopBottomScrollingMargins(srTestMargins.top, srTestMargins.bottom)); + VERIFY_IS_TRUE(_pDispatch->SetTopBottomScrollingMargins(2, 6)); + VERIFY_ARE_EQUAL(2, _pDispatch->_scrollMargins.top + 1); + VERIFY_ARE_EQUAL(6, _pDispatch->_scrollMargins.bottom + 1); Log::Comment(L"Test 2: Verify having only top is valid."); - - _testGetSet->_SetMarginsHelper(&srTestMargins, 7, 0); - _testGetSet->_expectedScrollRegion.bottom = _testGetSet->_viewport.bottom - 1; // We expect the bottom to be the bottom of the viewport, exclusive. - _testGetSet->_setScrollingRegionResult = TRUE; - VERIFY_IS_TRUE(_pDispatch->SetTopBottomScrollingMargins(srTestMargins.top, srTestMargins.bottom)); + VERIFY_IS_TRUE(_pDispatch->SetTopBottomScrollingMargins(7, 0)); + VERIFY_ARE_EQUAL(7, _pDispatch->_scrollMargins.top + 1); + VERIFY_ARE_EQUAL(sScreenHeight, _pDispatch->_scrollMargins.bottom + 1); Log::Comment(L"Test 3: Verify having only bottom is valid."); - - _testGetSet->_SetMarginsHelper(&srTestMargins, 0, 7); - _testGetSet->_setScrollingRegionResult = TRUE; - VERIFY_IS_TRUE(_pDispatch->SetTopBottomScrollingMargins(srTestMargins.top, srTestMargins.bottom)); + VERIFY_IS_TRUE(_pDispatch->SetTopBottomScrollingMargins(0, 7)); + VERIFY_ARE_EQUAL(1, _pDispatch->_scrollMargins.top + 1); + VERIFY_ARE_EQUAL(7, _pDispatch->_scrollMargins.bottom + 1); Log::Comment(L"Test 4: Verify having no values is valid."); - - _testGetSet->_SetMarginsHelper(&srTestMargins, 0, 0); - _testGetSet->_setScrollingRegionResult = TRUE; - VERIFY_IS_TRUE(_pDispatch->SetTopBottomScrollingMargins(srTestMargins.top, srTestMargins.bottom)); + VERIFY_IS_TRUE(_pDispatch->SetTopBottomScrollingMargins(0, 0)); + VERIFY_ARE_EQUAL(til::inclusive_rect{}, _pDispatch->_scrollMargins); Log::Comment(L"Test 5: Verify having both values, but bad bounds has no effect."); - - _testGetSet->_SetMarginsHelper(&srTestMargins, 7, 3); - _testGetSet->_setScrollingRegionResult = TRUE; - _testGetSet->_activeScrollRegion = {}; - VERIFY_IS_TRUE(_pDispatch->SetTopBottomScrollingMargins(srTestMargins.top, srTestMargins.bottom)); - VERIFY_ARE_EQUAL(til::inclusive_rect{}, _testGetSet->_activeScrollRegion); + _pDispatch->_scrollMargins = {}; + VERIFY_IS_TRUE(_pDispatch->SetTopBottomScrollingMargins(7, 3)); + VERIFY_ARE_EQUAL(til::inclusive_rect{}, _pDispatch->_scrollMargins); Log::Comment(L"Test 6: Verify setting margins to (0, height) clears them"); // First set, - _testGetSet->_setScrollingRegionResult = TRUE; - _testGetSet->_SetMarginsHelper(&srTestMargins, 2, 6); - VERIFY_IS_TRUE(_pDispatch->SetTopBottomScrollingMargins(srTestMargins.top, srTestMargins.bottom)); + VERIFY_IS_TRUE(_pDispatch->SetTopBottomScrollingMargins(2, 6)); // Then clear - _testGetSet->_SetMarginsHelper(&srTestMargins, 0, sScreenHeight); - _testGetSet->_expectedScrollRegion.top = 0; - _testGetSet->_expectedScrollRegion.bottom = 0; - VERIFY_IS_TRUE(_pDispatch->SetTopBottomScrollingMargins(srTestMargins.top, srTestMargins.bottom)); + VERIFY_IS_TRUE(_pDispatch->SetTopBottomScrollingMargins(0, sScreenHeight)); + VERIFY_ARE_EQUAL(til::inclusive_rect{}, _pDispatch->_scrollMargins); Log::Comment(L"Test 7: Verify setting margins to (1, height) clears them"); // First set, - _testGetSet->_setScrollingRegionResult = TRUE; - _testGetSet->_SetMarginsHelper(&srTestMargins, 2, 6); - VERIFY_IS_TRUE(_pDispatch->SetTopBottomScrollingMargins(srTestMargins.top, srTestMargins.bottom)); + VERIFY_IS_TRUE(_pDispatch->SetTopBottomScrollingMargins(2, 6)); // Then clear - _testGetSet->_SetMarginsHelper(&srTestMargins, 1, sScreenHeight); - _testGetSet->_expectedScrollRegion.top = 0; - _testGetSet->_expectedScrollRegion.bottom = 0; - VERIFY_IS_TRUE(_pDispatch->SetTopBottomScrollingMargins(srTestMargins.top, srTestMargins.bottom)); + VERIFY_IS_TRUE(_pDispatch->SetTopBottomScrollingMargins(1, sScreenHeight)); + VERIFY_ARE_EQUAL(til::inclusive_rect{}, _pDispatch->_scrollMargins); Log::Comment(L"Test 8: Verify setting margins to (1, 0) clears them"); // First set, - _testGetSet->_setScrollingRegionResult = TRUE; - _testGetSet->_SetMarginsHelper(&srTestMargins, 2, 6); - VERIFY_IS_TRUE(_pDispatch->SetTopBottomScrollingMargins(srTestMargins.top, srTestMargins.bottom)); + VERIFY_IS_TRUE(_pDispatch->SetTopBottomScrollingMargins(2, 6)); // Then clear - _testGetSet->_SetMarginsHelper(&srTestMargins, 1, 0); - _testGetSet->_expectedScrollRegion.top = 0; - _testGetSet->_expectedScrollRegion.bottom = 0; - VERIFY_IS_TRUE(_pDispatch->SetTopBottomScrollingMargins(srTestMargins.top, srTestMargins.bottom)); + VERIFY_IS_TRUE(_pDispatch->SetTopBottomScrollingMargins(1, 0)); + VERIFY_ARE_EQUAL(til::inclusive_rect{}, _pDispatch->_scrollMargins); Log::Comment(L"Test 9: Verify having top and bottom margin the same has no effect."); - - _testGetSet->_SetMarginsHelper(&srTestMargins, 4, 4); - _testGetSet->_setScrollingRegionResult = TRUE; - _testGetSet->_activeScrollRegion = {}; - VERIFY_IS_TRUE(_pDispatch->SetTopBottomScrollingMargins(srTestMargins.top, srTestMargins.bottom)); - VERIFY_ARE_EQUAL(til::inclusive_rect{}, _testGetSet->_activeScrollRegion); + _pDispatch->_scrollMargins = {}; + VERIFY_IS_TRUE(_pDispatch->SetTopBottomScrollingMargins(4, 4)); + VERIFY_ARE_EQUAL(til::inclusive_rect{}, _pDispatch->_scrollMargins); Log::Comment(L"Test 10: Verify having top margin out of bounds has no effect."); - - _testGetSet->_SetMarginsHelper(&srTestMargins, sScreenHeight + 1, sScreenHeight + 10); - _testGetSet->_setScrollingRegionResult = TRUE; - _testGetSet->_activeScrollRegion = {}; - VERIFY_IS_TRUE(_pDispatch->SetTopBottomScrollingMargins(srTestMargins.top, srTestMargins.bottom)); - VERIFY_ARE_EQUAL(til::inclusive_rect{}, _testGetSet->_activeScrollRegion); + _pDispatch->_scrollMargins = {}; + VERIFY_IS_TRUE(_pDispatch->SetTopBottomScrollingMargins(sScreenHeight + 1, sScreenHeight + 10)); + VERIFY_ARE_EQUAL(til::inclusive_rect{}, _pDispatch->_scrollMargins); Log::Comment(L"Test 11: Verify having bottom margin out of bounds has no effect."); - - _testGetSet->_SetMarginsHelper(&srTestMargins, 1, sScreenHeight + 1); - _testGetSet->_setScrollingRegionResult = TRUE; - _testGetSet->_activeScrollRegion = {}; - VERIFY_IS_TRUE(_pDispatch->SetTopBottomScrollingMargins(srTestMargins.top, srTestMargins.bottom)); - VERIFY_ARE_EQUAL(til::inclusive_rect{}, _testGetSet->_activeScrollRegion); + _pDispatch->_scrollMargins = {}; + VERIFY_IS_TRUE(_pDispatch->SetTopBottomScrollingMargins(1, sScreenHeight + 1)); + VERIFY_ARE_EQUAL(til::inclusive_rect{}, _pDispatch->_scrollMargins); } TEST_METHOD(SetConsoleTitleTest) From d7b3e494df1fad06a7d19274d43aa3fa577c8ad0 Mon Sep 17 00:00:00 2001 From: James Holderness Date: Thu, 16 Feb 2023 21:54:40 +0000 Subject: [PATCH 10/13] Workaround for removed margin API in screen buffer tests. --- src/host/ut_host/ScreenBufferTests.cpp | 38 ++++++++++++++++++++------ 1 file changed, 30 insertions(+), 8 deletions(-) diff --git a/src/host/ut_host/ScreenBufferTests.cpp b/src/host/ut_host/ScreenBufferTests.cpp index 5a32660d914..4a1d5e0e15c 100644 --- a/src/host/ut_host/ScreenBufferTests.cpp +++ b/src/host/ut_host/ScreenBufferTests.cpp @@ -1226,6 +1226,28 @@ void ScreenBufferTests::VtResizeComprehensive() VERIFY_ARE_EQUAL(expectedViewHeight, newViewHeight); } +til::rect _GetRelativeScrollMargins() +{ + auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); + auto& si = gci.GetActiveOutputBuffer().GetActiveBuffer(); + auto& stateMachine = si.GetStateMachine(); + const auto viewport = si.GetViewport(); + auto& cursor = si.GetTextBuffer().GetCursor(); + const auto savePos = cursor.GetPosition(); + + // We can't access the AdaptDispatch internals where the margins are stored, + // but we calculate their boundaries by using VT sequences to move down and + // up as far as possible, and read the cursor positions at the two limits. + stateMachine.ProcessString(L"\033[H\033[9999B"); + const auto bottom = cursor.GetPosition().y - viewport.Top(); + stateMachine.ProcessString(L"\033[9999A"); + const auto top = cursor.GetPosition().y - viewport.Top(); + + cursor.SetPosition(savePos); + const auto noMargins = (top == 0 && bottom == viewport.Height() - 1); + return noMargins ? til::rect{} : til::rect{ 0, top, 0, bottom }; +} + void ScreenBufferTests::VtResizeDECCOLM() { // Run this test in isolation - for one reason or another, this breaks other tests. @@ -1249,13 +1271,13 @@ void ScreenBufferTests::VtResizeDECCOLM() return si.GetTextBuffer().GetCursor().GetPosition() - si.GetViewport().Origin(); }; auto areMarginsSet = [&]() { - const auto margins = si.GetRelativeScrollMargins(); - return margins.BottomInclusive() > margins.Top(); + const auto margins = _GetRelativeScrollMargins(); + return margins.bottom > margins.top; }; stateMachine.ProcessString(setInitialMargins); stateMachine.ProcessString(setInitialCursor); - auto initialMargins = si.GetRelativeScrollMargins(); + auto initialMargins = _GetRelativeScrollMargins(); auto initialCursorPosition = getRelativeCursorPosition(); auto initialSbHeight = si.GetBufferSize().Height(); @@ -1272,7 +1294,7 @@ void ScreenBufferTests::VtResizeDECCOLM() auto newViewWidth = si.GetViewport().Width(); VERIFY_IS_TRUE(areMarginsSet()); - VERIFY_ARE_EQUAL(initialMargins, si.GetRelativeScrollMargins()); + VERIFY_ARE_EQUAL(initialMargins, _GetRelativeScrollMargins()); VERIFY_ARE_EQUAL(initialCursorPosition, getRelativeCursorPosition()); VERIFY_ARE_EQUAL(initialSbHeight, newSbHeight); VERIFY_ARE_EQUAL(initialViewHeight, newViewHeight); @@ -1308,7 +1330,7 @@ void ScreenBufferTests::VtResizeDECCOLM() stateMachine.ProcessString(setInitialMargins); stateMachine.ProcessString(setInitialCursor); - initialMargins = si.GetRelativeScrollMargins(); + initialMargins = _GetRelativeScrollMargins(); initialCursorPosition = getRelativeCursorPosition(); initialSbHeight = newSbHeight; @@ -1326,7 +1348,7 @@ void ScreenBufferTests::VtResizeDECCOLM() newViewWidth = si.GetViewport().Width(); VERIFY_IS_TRUE(areMarginsSet()); - VERIFY_ARE_EQUAL(initialMargins, si.GetRelativeScrollMargins()); + VERIFY_ARE_EQUAL(initialMargins, _GetRelativeScrollMargins()); VERIFY_ARE_EQUAL(initialCursorPosition, getRelativeCursorPosition()); VERIFY_ARE_EQUAL(initialSbHeight, newSbHeight); VERIFY_ARE_EQUAL(initialViewHeight, newViewHeight); @@ -6370,8 +6392,8 @@ void ScreenBufferTests::ScreenAlignmentPattern() WI_SetFlag(si.OutputMode, ENABLE_VIRTUAL_TERMINAL_PROCESSING); auto areMarginsSet = [&]() { - const auto margins = si.GetRelativeScrollMargins(); - return margins.BottomInclusive() > margins.Top(); + const auto margins = _GetRelativeScrollMargins(); + return margins.bottom > margins.top; }; Log::Comment(L"Set the initial buffer state."); From 3cb7cd731f5f7b86b218950c72356e6daf94165f Mon Sep 17 00:00:00 2001 From: James Holderness Date: Tue, 21 Feb 2023 21:51:28 +0000 Subject: [PATCH 11/13] Rewrite line feed adapter test. --- .../adapter/ut_adapter/adapterTest.cpp | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/src/terminal/adapter/ut_adapter/adapterTest.cpp b/src/terminal/adapter/ut_adapter/adapterTest.cpp index ee45fa0458d..7a2771e71a6 100644 --- a/src/terminal/adapter/ut_adapter/adapterTest.cpp +++ b/src/terminal/adapter/ut_adapter/adapterTest.cpp @@ -1912,6 +1912,36 @@ class AdapterTest VERIFY_ARE_EQUAL(til::inclusive_rect{}, _pDispatch->_scrollMargins); } + TEST_METHOD(LineFeedTest) + { + Log::Comment(L"Starting test..."); + + _testGetSet->PrepData(); + auto& cursor = _testGetSet->_textBuffer->GetCursor(); + + Log::Comment(L"Test 1: Line feed without carriage return."); + cursor.SetPosition({ 10, 0 }); + VERIFY_IS_TRUE(_pDispatch->LineFeed(DispatchTypes::LineFeedType::WithoutReturn)); + VERIFY_ARE_EQUAL(til::point(10, 1), cursor.GetPosition()); + + Log::Comment(L"Test 2: Line feed with carriage return."); + cursor.SetPosition({ 10, 0 }); + VERIFY_IS_TRUE(_pDispatch->LineFeed(DispatchTypes::LineFeedType::WithReturn)); + VERIFY_ARE_EQUAL(til::point(0, 1), cursor.GetPosition()); + + Log::Comment(L"Test 3: Line feed depends on mode, and mode reset."); + _testGetSet->_getLineFeedModeResult = false; + cursor.SetPosition({ 10, 0 }); + VERIFY_IS_TRUE(_pDispatch->LineFeed(DispatchTypes::LineFeedType::DependsOnMode)); + VERIFY_ARE_EQUAL(til::point(10, 1), cursor.GetPosition()); + + Log::Comment(L"Test 4: Line feed depends on mode, and mode set."); + _testGetSet->_getLineFeedModeResult = true; + cursor.SetPosition({ 10, 0 }); + VERIFY_IS_TRUE(_pDispatch->LineFeed(DispatchTypes::LineFeedType::DependsOnMode)); + VERIFY_ARE_EQUAL(til::point(0, 1), cursor.GetPosition()); + } + TEST_METHOD(SetConsoleTitleTest) { Log::Comment(L"Starting test..."); From 73db9a795fedc67b6ed6996d6a693d10689406dc Mon Sep 17 00:00:00 2001 From: James Holderness Date: Thu, 16 Mar 2023 16:41:38 +0000 Subject: [PATCH 12/13] Remove unused InitializeCursorRowAttributes. --- src/host/screenInfo.cpp | 28 ---------------------------- src/host/screenInfo.hpp | 2 -- 2 files changed, 30 deletions(-) diff --git a/src/host/screenInfo.cpp b/src/host/screenInfo.cpp index d595ec8efb1..029eca42b7b 100644 --- a/src/host/screenInfo.cpp +++ b/src/host/screenInfo.cpp @@ -2609,34 +2609,6 @@ void SCREEN_INFORMATION::UpdateBottom() _virtualBottom = _viewport.BottomInclusive(); } -// Method Description: -// - Initialize the row with the cursor on it to the standard erase attributes. -// This is executed when we move the cursor below the current viewport in -// VT mode. When that happens in a real terminal, the line is brand new, -// so it gets initialized for the first time with the current attributes. -// Our rows are usually pre-initialized, so re-initialize it here to -// emulate that behavior. -// See MSFT:17415310. -// Arguments: -// - -// Return Value: -// - -void SCREEN_INFORMATION::InitializeCursorRowAttributes() -{ - if (_textBuffer) - { - const auto& cursor = _textBuffer->GetCursor(); - auto& row = _textBuffer->GetRowByOffset(cursor.GetPosition().y); - // The VT standard requires that the new row is initialized with - // the current background color, but with no meta attributes set. - auto fillAttributes = GetAttributes(); - fillAttributes.SetStandardErase(); - row.SetAttrToEnd(0, fillAttributes); - // The row should also be single width to start with. - row.SetLineRendition(LineRendition::SingleWidth); - } -} - // Method Description: // - Returns the "virtual" Viewport - the viewport with its bottom at // `_virtualBottom`. For VT operations, this is essentially the mutable diff --git a/src/host/screenInfo.hpp b/src/host/screenInfo.hpp index 2f6bbd0cc47..d67e3f5684c 100644 --- a/src/host/screenInfo.hpp +++ b/src/host/screenInfo.hpp @@ -222,8 +222,6 @@ class SCREEN_INFORMATION : public ConsoleObjectHeader, public Microsoft::Console FontInfoDesired& GetDesiredFont() noexcept; const FontInfoDesired& GetDesiredFont() const noexcept; - void InitializeCursorRowAttributes(); - void SetIgnoreLegacyEquivalentVTAttributes() noexcept; void ResetIgnoreLegacyEquivalentVTAttributes() noexcept; From 6b9acb90cccc1a37b1453167e547f496b26745c3 Mon Sep 17 00:00:00 2001 From: James Holderness Date: Thu, 16 Mar 2023 23:00:35 +0000 Subject: [PATCH 13/13] Remove unneeded VT mode check. --- src/host/output.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/host/output.cpp b/src/host/output.cpp index bbaceb99218..8b0ce0b7fd9 100644 --- a/src/host/output.cpp +++ b/src/host/output.cpp @@ -302,8 +302,7 @@ static void _ScrollScreen(SCREEN_INFORMATION& screenInfo, const Viewport& source bool StreamScrollRegion(SCREEN_INFORMATION& screenInfo) { // Rotate the circular buffer around and wipe out the previous final line. - const auto inVtMode = WI_IsFlagSet(screenInfo.OutputMode, ENABLE_VIRTUAL_TERMINAL_PROCESSING); - auto fSuccess = screenInfo.GetTextBuffer().IncrementCircularBuffer(inVtMode); + auto fSuccess = screenInfo.GetTextBuffer().IncrementCircularBuffer(); if (fSuccess) { // Trigger a graphical update if we're active.