Skip to content

Commit babd344

Browse files
authored
Account for viewport movement when wrapping over multiple rows (#17353)
## Summary of the Pull Request When we receive a stream of output at the bottom of the page that wraps over more than two lines, that is expected to pan the viewport down by multiple rows to accommodate all of the output. However, when the output is received in a single write, that did not work correctly. The problem was that we were reusing a `Page` instance across multiple `_DoLineFeed` calls, and the viewport cached in that `Page` wasn't valid after the first call. This PR fixes the issue by adjusting the cached viewport when we determine it has been moved by `_DoLineFeed`. ## References and Relevant Issues The bug was introduced in PR #16615 when paging support was added. ## Validation Steps Performed I've verified that the test case in #17351 is now working correctly, and have added a unit test covering this scenario. ## PR Checklist - [x] Closes #17351 - [x] Tests added/passed
1 parent 01e4df1 commit babd344

File tree

5 files changed

+57
-6
lines changed

5 files changed

+57
-6
lines changed

src/host/ut_host/ScreenBufferTests.cpp

+34
Original file line numberDiff line numberDiff line change
@@ -262,6 +262,7 @@ class ScreenBufferTests
262262
TEST_METHOD(CopyDoubleWidthRectangularArea);
263263

264264
TEST_METHOD(DelayedWrapReset);
265+
TEST_METHOD(MultilineWrap);
265266

266267
TEST_METHOD(EraseColorMode);
267268

@@ -8323,6 +8324,39 @@ void ScreenBufferTests::DelayedWrapReset()
83238324
}
83248325
}
83258326

8327+
void ScreenBufferTests::MultilineWrap()
8328+
{
8329+
auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
8330+
auto& si = gci.GetActiveOutputBuffer().GetActiveBuffer();
8331+
auto& stateMachine = si.GetStateMachine();
8332+
const auto bufferAttr = si.GetTextBuffer().GetCurrentAttributes();
8333+
const auto width = si.GetTextBuffer().GetSize().Width();
8334+
const auto bottomRow = si.GetViewport().BottomInclusive();
8335+
8336+
// Starting on the bottom row.
8337+
si.GetTextBuffer().GetCursor().SetPosition({ 0, bottomRow });
8338+
8339+
// Write out enough text to wrap over four lines.
8340+
auto fourLines = std::wstring{};
8341+
fourLines += L"1";
8342+
fourLines += std::wstring(width - 1, L' ');
8343+
fourLines += L"2";
8344+
fourLines += std::wstring(width - 1, L' ');
8345+
fourLines += L"3";
8346+
fourLines += std::wstring(width - 1, L' ');
8347+
fourLines += L"4";
8348+
stateMachine.ProcessString(fourLines);
8349+
8350+
Log::Comment(L"Cursor should have moved down three rows");
8351+
VERIFY_ARE_EQUAL(bottomRow + 3, si.GetTextBuffer().GetCursor().GetPosition().y);
8352+
8353+
Log::Comment(L"Bottom four rows should have the content 1, 2, 3, and 4");
8354+
VERIFY_IS_TRUE(_ValidateLineContains(bottomRow + 0, L"1", bufferAttr));
8355+
VERIFY_IS_TRUE(_ValidateLineContains(bottomRow + 1, L"2", bufferAttr));
8356+
VERIFY_IS_TRUE(_ValidateLineContains(bottomRow + 2, L"3", bufferAttr));
8357+
VERIFY_IS_TRUE(_ValidateLineContains(bottomRow + 3, L"4", bufferAttr));
8358+
}
8359+
83268360
void ScreenBufferTests::EraseColorMode()
83278361
{
83288362
BEGIN_TEST_METHOD_PROPERTIES()

src/terminal/adapter/PageManager.cpp

+6
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,12 @@ til::CoordType Page::YPanOffset() const noexcept
9393
return 0; // Vertical panning is not yet supported
9494
}
9595

96+
void Page::MoveViewportDown() noexcept
97+
{
98+
_viewport.top++;
99+
_viewport.bottom++;
100+
}
101+
96102
PageManager::PageManager(ITerminalApi& api, Renderer& renderer) noexcept :
97103
_api{ api },
98104
_renderer{ renderer }

src/terminal/adapter/PageManager.hpp

+1
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ namespace Microsoft::Console::VirtualTerminal
3333
til::CoordType BufferHeight() const noexcept;
3434
til::CoordType XPanOffset() const noexcept;
3535
til::CoordType YPanOffset() const noexcept;
36+
void MoveViewportDown() noexcept;
3637

3738
private:
3839
TextBuffer& _buffer;

src/terminal/adapter/adaptDispatch.cpp

+15-5
Original file line numberDiff line numberDiff line change
@@ -73,14 +73,14 @@ void AdaptDispatch::PrintString(const std::wstring_view string)
7373

7474
void AdaptDispatch::_WriteToBuffer(const std::wstring_view string)
7575
{
76-
const auto page = _pages.ActivePage();
76+
auto page = _pages.ActivePage();
7777
auto& textBuffer = page.Buffer();
7878
auto& cursor = page.Cursor();
7979
auto cursorPosition = cursor.GetPosition();
8080
const auto wrapAtEOL = _api.GetSystemMode(ITerminalApi::Mode::AutoWrap);
8181
const auto& attributes = page.Attributes();
8282

83-
const auto [topMargin, bottomMargin] = _GetVerticalMargins(page, true);
83+
auto [topMargin, bottomMargin] = _GetVerticalMargins(page, true);
8484
const auto [leftMargin, rightMargin] = _GetHorizontalMargins(page.Width());
8585

8686
auto lineWidth = textBuffer.GetLineWidth(cursorPosition.y);
@@ -107,7 +107,14 @@ void AdaptDispatch::_WriteToBuffer(const std::wstring_view string)
107107
// different position from where the EOL was marked.
108108
if (delayedCursorPosition == cursorPosition)
109109
{
110-
_DoLineFeed(page, true, true);
110+
if (_DoLineFeed(page, true, true))
111+
{
112+
// If the line feed caused the viewport to move down, we
113+
// need to adjust the page viewport and margins to match.
114+
page.MoveViewportDown();
115+
std::tie(topMargin, bottomMargin) = _GetVerticalMargins(page, true);
116+
}
117+
111118
cursorPosition = cursor.GetPosition();
112119
// We need to recalculate the width when moving to a new line.
113120
lineWidth = textBuffer.GetLineWidth(cursorPosition.y);
@@ -2505,14 +2512,15 @@ bool AdaptDispatch::CarriageReturn()
25052512
// - withReturn - Set to true if a carriage return should be performed as well.
25062513
// - wrapForced - Set to true is the line feed was the result of the line wrapping.
25072514
// Return Value:
2508-
// - <none>
2509-
void AdaptDispatch::_DoLineFeed(const Page& page, const bool withReturn, const bool wrapForced)
2515+
// - True if the viewport panned down. False if not.
2516+
bool AdaptDispatch::_DoLineFeed(const Page& page, const bool withReturn, const bool wrapForced)
25102517
{
25112518
auto& textBuffer = page.Buffer();
25122519
const auto pageWidth = page.Width();
25132520
const auto bufferHeight = page.BufferHeight();
25142521
const auto [topMargin, bottomMargin] = _GetVerticalMargins(page, true);
25152522
const auto [leftMargin, rightMargin] = _GetHorizontalMargins(pageWidth);
2523+
auto viewportMoved = false;
25162524

25172525
auto& cursor = page.Cursor();
25182526
const auto currentPosition = cursor.GetPosition();
@@ -2555,6 +2563,7 @@ void AdaptDispatch::_DoLineFeed(const Page& page, const bool withReturn, const b
25552563
// the end of the buffer.
25562564
_api.SetViewportPosition({ page.XPanOffset(), page.Top() + 1 });
25572565
newPosition.y++;
2566+
viewportMoved = true;
25582567

25592568
// And if the bottom margin didn't cover the full page, we copy the
25602569
// lower part of the page down so it remains static. But for a full
@@ -2594,6 +2603,7 @@ void AdaptDispatch::_DoLineFeed(const Page& page, const bool withReturn, const b
25942603

25952604
cursor.SetPosition(newPosition);
25962605
_ApplyCursorMovementFlags(cursor);
2606+
return viewportMoved;
25972607
}
25982608

25992609
// Routine Description:

src/terminal/adapter/adaptDispatch.hpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -249,7 +249,7 @@ namespace Microsoft::Console::VirtualTerminal
249249
const VTInt rightMargin,
250250
const bool homeCursor = false);
251251

252-
void _DoLineFeed(const Page& page, const bool withReturn, const bool wrapForced);
252+
bool _DoLineFeed(const Page& page, const bool withReturn, const bool wrapForced);
253253

254254
void _DeviceStatusReport(const wchar_t* parameters) const;
255255
void _CursorPositionReport(const bool extendedReport);

0 commit comments

Comments
 (0)