From 3d463f1a870144db22fc8150974c2e8daa3fe4e7 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Tue, 15 Aug 2023 15:59:51 +0200 Subject: [PATCH 01/11] Reimplement TextBuffer::Reflow --- src/buffer/out/Row.cpp | 38 +- src/buffer/out/Row.hpp | 2 + src/buffer/out/textBuffer.cpp | 460 +++++++----------- src/buffer/out/textBuffer.hpp | 9 +- src/buffer/out/ut_textbuffer/ReflowTests.cpp | 2 +- src/cascadia/TerminalCore/Terminal.cpp | 147 ++---- src/host/screenInfo.cpp | 76 ++- src/host/ut_host/ApiRoutinesTests.cpp | 2 +- src/host/ut_host/ScreenBufferTests.cpp | 4 +- src/host/ut_host/TextBufferTests.cpp | 8 +- .../adapter/ut_adapter/adapterTest.cpp | 10 +- src/types/UiaTextRangeBase.cpp | 2 +- 12 files changed, 321 insertions(+), 439 deletions(-) diff --git a/src/buffer/out/Row.cpp b/src/buffer/out/Row.cpp index 760f8cf501f..34189a2b2b2 100644 --- a/src/buffer/out/Row.cpp +++ b/src/buffer/out/Row.cpp @@ -266,11 +266,16 @@ void ROW::TransferAttributes(const til::small_rle& a void ROW::CopyFrom(const ROW& source) { - RowCopyTextFromState state{ .source = source }; - CopyTextFrom(state); - TransferAttributes(source.Attributes(), _columnCount); _lineRendition = source._lineRendition; _wrapForced = source._wrapForced; + + RowCopyTextFromState state{ + .source = source, + .columnLimit = LineRenditionColumns(), + }; + CopyTextFrom(state); + + TransferAttributes(source.Attributes(), _columnCount); } // Returns the previous possible cursor position, preceding the given column. @@ -284,7 +289,17 @@ til::CoordType ROW::NavigateToPrevious(til::CoordType column) const noexcept // Returns the row width if column is beyond the width of the row. til::CoordType ROW::NavigateToNext(til::CoordType column) const noexcept { - return _adjustForward(_clampedColumn(column + 1)); + return _adjustForward(_clampedColumnInclusive(column + 1)); +} + +til::CoordType ROW::AdjustBackward(til::CoordType column) const noexcept +{ + return _adjustBackward(_clampedColumn(column)); +} + +til::CoordType ROW::AdjustForward(til::CoordType column) const noexcept +{ + return _adjustForward(_clampedColumnInclusive(column)); } uint16_t ROW::_adjustBackward(uint16_t column) const noexcept @@ -641,11 +656,12 @@ try if (sourceColBeg < sourceColLimit) { charOffsets = source._charOffsets.subspan(sourceColBeg, static_cast(sourceColLimit) - sourceColBeg + 1); - const auto charsOffset = charOffsets.front() & CharOffsetsMask; + const auto beg = size_t{ charOffsets.front() } & CharOffsetsMask; + const auto end = size_t{ charOffsets.back() } & CharOffsetsMask; // We _are_ using span. But C++ decided that string_view and span aren't convertible. // _chars is a std::span for performance and because it refers to raw, shared memory. #pragma warning(suppress : 26481) // Don't use pointer arithmetic. Use span instead (bounds.1). - chars = { source._chars.data() + charsOffset, source._chars.size() - charsOffset }; + chars = { source._chars.data() + beg, end - beg }; } WriteHelper h{ *this, state.columnBegin, state.columnLimit, chars }; @@ -867,6 +883,16 @@ til::CoordType ROW::MeasureLeft() const noexcept til::CoordType ROW::MeasureRight() const noexcept { + if (_wrapForced) + { + auto width = _columnCount; + if (_doubleBytePadded) + { + width--; + } + return width; + } + const auto text = GetText(); const auto beg = text.begin(); const auto end = text.end(); diff --git a/src/buffer/out/Row.hpp b/src/buffer/out/Row.hpp index c19f343f235..2812d3cfa63 100644 --- a/src/buffer/out/Row.hpp +++ b/src/buffer/out/Row.hpp @@ -114,6 +114,8 @@ class ROW final til::CoordType NavigateToPrevious(til::CoordType column) const noexcept; til::CoordType NavigateToNext(til::CoordType column) const noexcept; + til::CoordType AdjustBackward(til::CoordType column) const noexcept; + til::CoordType AdjustForward(til::CoordType column) const noexcept; void ClearCell(til::CoordType column); OutputCellIterator WriteCells(OutputCellIterator it, til::CoordType columnBegin, std::optional wrap = std::nullopt, std::optional limitRight = std::nullopt); diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index 7854ae2def5..eb8fdcd1db9 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -810,9 +810,9 @@ void TextBuffer::IncrementCircularBuffer(const TextAttribute& fillAttributes) // - The viewport //Return value: // - Coordinate position (relative to the text buffer) -til::point TextBuffer::GetLastNonSpaceCharacter(std::optional viewOptional) const +til::point TextBuffer::GetLastNonSpaceCharacter(const Viewport* viewOptional) const { - const auto viewport = viewOptional.has_value() ? viewOptional.value() : GetSize(); + const auto viewport = viewOptional ? *viewOptional : GetSize(); til::point coordEndOfText; // Search the given viewport by starting at the bottom. @@ -1069,46 +1069,40 @@ void TextBuffer::Reset() noexcept // - newSize - new size of screen. // Return Value: // - Success if successful. Invalid parameter if screen buffer size is unexpected. No memory if allocation failed. -[[nodiscard]] NTSTATUS TextBuffer::ResizeTraditional(til::size newSize) noexcept +void TextBuffer::ResizeTraditional(til::size newSize) { // Guard against resizing the text buffer to 0 columns/rows, which would break being able to insert text. newSize.width = std::max(newSize.width, 1); newSize.height = std::max(newSize.height, 1); - try - { - TextBuffer newBuffer{ newSize, _currentAttributes, 0, false, _renderer }; - const auto cursorRow = GetCursor().GetPosition().y; - const auto copyableRows = std::min(_height, newSize.height); - til::CoordType srcRow = 0; - til::CoordType dstRow = 0; - - if (cursorRow >= newSize.height) - { - srcRow = cursorRow - newSize.height + 1; - } + TextBuffer newBuffer{ newSize, _currentAttributes, 0, false, _renderer }; + const auto cursorRow = GetCursor().GetPosition().y; + const auto copyableRows = std::min(_height, newSize.height); + til::CoordType srcRow = 0; + til::CoordType dstRow = 0; - for (; dstRow < copyableRows; ++dstRow, ++srcRow) - { - newBuffer.GetRowByOffset(dstRow).CopyFrom(GetRowByOffset(srcRow)); - } - - // NOTE: Keep this in sync with _reserve(). - _buffer = std::move(newBuffer._buffer); - _bufferEnd = newBuffer._bufferEnd; - _commitWatermark = newBuffer._commitWatermark; - _initialAttributes = newBuffer._initialAttributes; - _bufferRowStride = newBuffer._bufferRowStride; - _bufferOffsetChars = newBuffer._bufferOffsetChars; - _bufferOffsetCharOffsets = newBuffer._bufferOffsetCharOffsets; - _width = newBuffer._width; - _height = newBuffer._height; + if (cursorRow >= newSize.height) + { + srcRow = cursorRow - newSize.height + 1; + } - _SetFirstRowIndex(0); + for (; dstRow < copyableRows; ++dstRow, ++srcRow) + { + newBuffer.GetRowByOffset(dstRow).CopyFrom(GetRowByOffset(srcRow)); } - CATCH_RETURN(); - return S_OK; + // NOTE: Keep this in sync with _reserve(). + _buffer = std::move(newBuffer._buffer); + _bufferEnd = newBuffer._bufferEnd; + _commitWatermark = newBuffer._commitWatermark; + _initialAttributes = newBuffer._initialAttributes; + _bufferRowStride = newBuffer._bufferRowStride; + _bufferOffsetChars = newBuffer._bufferOffsetChars; + _bufferOffsetCharOffsets = newBuffer._bufferOffsetCharOffsets; + _width = newBuffer._width; + _height = newBuffer._height; + + _SetFirstRowIndex(0); } void TextBuffer::SetAsActiveBuffer(const bool isActiveBuffer) noexcept @@ -2411,204 +2405,146 @@ void TextBuffer::_AppendRTFText(std::ostringstream& contentBuilder, const std::w // the new buffer. The rows's new value is placed back into this parameter. // Return Value: // - S_OK if we successfully copied the contents to the new buffer, otherwise an appropriate HRESULT. -HRESULT TextBuffer::Reflow(TextBuffer& oldBuffer, - TextBuffer& newBuffer, - const std::optional lastCharacterViewport, - std::optional> positionInfo) -try +void TextBuffer::Reflow(TextBuffer& oldBuffer, TextBuffer& newBuffer, const Viewport* lastCharacterViewport, PositionInformation* positionInfo) { const auto& oldCursor = oldBuffer.GetCursor(); auto& newCursor = newBuffer.GetCursor(); - // We need to save the old cursor position so that we can - // place the new cursor back on the equivalent character in - // the new buffer. - const auto cOldCursorPos = oldCursor.GetPosition(); - const auto cOldLastChar = oldBuffer.GetLastNonSpaceCharacter(lastCharacterViewport); - - const auto cOldRowsTotal = cOldLastChar.y + 1; - - til::point cNewCursorPos; - auto fFoundCursorPos = false; - auto foundOldMutable = false; - auto foundOldVisible = false; - // Loop through all the rows of the old buffer and reprint them into the new buffer - til::CoordType iOldRow = 0; - for (; iOldRow < cOldRowsTotal; iOldRow++) - { - // Fetch the row and its "right" which is the last printable character. - const auto& row = oldBuffer.GetRowByOffset(iOldRow); - const auto cOldColsTotal = oldBuffer.GetLineWidth(iOldRow); - auto iRight = row.MeasureRight(); - - // If we're starting a new row, try and preserve the line rendition - // from the row in the original buffer. - const auto newBufferPos = newCursor.GetPosition(); - if (newBufferPos.x == 0) - { - auto& newRow = newBuffer.GetRowByOffset(newBufferPos.y); - newRow.SetLineRendition(row.GetLineRendition()); - } + const auto oldCursorPos = oldCursor.GetPosition(); + til::point newCursorPos; + + const auto lastRowWithText = oldBuffer.GetLastNonSpaceCharacter(lastCharacterViewport).y; + + auto mutableViewportTop = positionInfo ? positionInfo->mutableViewportTop : til::CoordTypeMax; + auto visibleViewportTop = positionInfo ? positionInfo->visibleViewportTop : til::CoordTypeMax; + + til::CoordType oldY = 0; + til::CoordType newY = 0; + til::CoordType newX = 0; + til::CoordType newWidth = newBuffer.GetSize().Width(); + const auto oldHeight = std::max(lastRowWithText, oldCursorPos.y) + 1; + const auto newHeight = newBuffer.GetSize().Height(); + const auto newWidthU16 = gsl::narrow_cast(newWidth); + + // Copy oldBuffer into newBuffer until oldBuffer has been fully consumed. + for (; oldY < oldHeight; ++oldY) + { + const auto& oldRow = oldBuffer.GetRowByOffset(oldY); - // There is a special case here. If the row has a "wrap" - // flag on it, but the right isn't equal to the width (one - // index past the final valid index in the row) then there - // were a bunch trailing of spaces in the row. - // (But the measuring functions for each row Left/Right do - // not count spaces as "displayable" so they're not - // included.) - // As such, adjust the "right" to be the width of the row - // to capture all these spaces - if (row.WasWrapForced()) + // A pair of double height rows should optimally wrap as a union (i.e. after wrapping there should be 4 lines). + // But for this initial implementation I chose the alternative approach: Just truncate them. + if (oldRow.GetLineRendition() != LineRendition::SingleWidth) { - iRight = cOldColsTotal; - - // And a combined special case. - // If we wrapped off the end of the row by adding a - // piece of padding because of a double byte LEADING - // character, then remove one from the "right" to - // leave this padding out of the copy process. - if (row.WasDoubleBytePadded()) + // Since rows with a non-standard line rendition should be truncated it's important + // that we pretend as if the previous row ended in a newline, even if it didn't. + // This is what this if does: It newlines. + if (newX) { - iRight--; + newX = 0; + newY++; } - } - // Loop through every character in the current row (up to - // the "right" boundary, which is one past the final valid - // character) - til::CoordType iOldCol = 0; - const auto copyRight = iRight; - for (; iOldCol < copyRight; iOldCol++) - { - if (iOldCol == cOldCursorPos.x && iOldRow == cOldCursorPos.y) + auto& newRow = newBuffer.GetRowByOffset(newY); + + // See the comment marked with "REFLOW_RESET". + if (newY >= newHeight) { - cNewCursorPos = newCursor.GetPosition(); - fFoundCursorPos = true; + newRow.Reset(newBuffer._initialAttributes); } - // TODO: MSFT: 19446208 - this should just use an iterator and the inserter... - const auto glyph = row.GlyphAt(iOldCol); - const auto dbcsAttr = row.DbcsAttrAt(iOldCol); - const auto textAttr = row.GetAttrByColumn(iOldCol); + newRow.CopyFrom(oldRow); + newRow.SetWrapForced(false); - newBuffer.InsertCharacter(glyph, dbcsAttr, textAttr); - } + if (oldY == oldCursorPos.y) + { + newCursorPos = { oldCursorPos.x, newY }; + } + if (oldY >= mutableViewportTop) + { + positionInfo->mutableViewportTop = newY; + mutableViewportTop = til::CoordTypeMax; + } + if (oldY >= visibleViewportTop) + { + positionInfo->visibleViewportTop = newY; + visibleViewportTop = til::CoordTypeMax; + } - // GH#32: Copy the attributes from the rest of the row into this new buffer. - // From where we are in the old buffer, to the end of the row, copy the - // remaining attributes. - // - if the old buffer is smaller than the new buffer, then just copy - // what we have, as it was. We already copied all _text_ with colors, - // but it's possible for someone to just put some color into the - // buffer to the right of that without any text (as just spaces). The - // buffer looks weird to the user when we resize and it starts losing - // those colors, so we need to copy them over too... as long as there - // is space. The last attr in the row will be extended to the end of - // the row in the new buffer. - // - if the old buffer is WIDER, than we might have wrapped onto a new - // line. Use the cursor's position's Y so that we know where the new - // row is, and start writing at the cursor position. Again, the attr - // in the last column of the old row will be extended to the end of the - // row that the text was flowed onto. - // - if the text in the old buffer didn't actually fill the whole - // line in the new buffer, then we didn't wrap. That's fine. just - // copy attributes from the old row till the end of the new row, and - // move on. - const auto newRowY = newCursor.GetPosition().y; - auto& newRow = newBuffer.GetRowByOffset(newRowY); - auto newAttrColumn = newCursor.GetPosition().x; - const auto newWidth = newBuffer.GetLineWidth(newRowY); - // Stop when we get to the end of the buffer width, or the new position - // for inserting an attr would be past the right of the new buffer. - for (auto copyAttrCol = iOldCol; - copyAttrCol < cOldColsTotal && newAttrColumn < newWidth; - copyAttrCol++, newAttrColumn++) - { - // TODO: MSFT: 19446208 - this should just use an iterator and the inserter... - const auto textAttr = row.GetAttrByColumn(copyAttrCol); - newRow.SetAttrToEnd(newAttrColumn, textAttr); + newY++; + continue; } - // If we found the old row that the caller was interested in, set the - // out value of that parameter to the cursor's current Y position (the - // new location of the _end_ of that row in the buffer). - if (positionInfo.has_value()) + // Rows don't store any information for what column the last written character is in. + // We simply truncate all trailing whitespace in this implementation. + const auto oldRowLimit = oldRow.MeasureRight(); + til::CoordType oldX = 0; + + // Copy oldRow into newBuffer until oldRow has been fully consumed. + // We use a do-while loop to ensure that line wrapping occurs and + // that attributes are copied over even for seemingly empty rows. + do { - if (!foundOldMutable) + // This if condition handles line wrapping. + // Only if we write past the last column we should wrap and as such this if + // condition is in front of the text insertion code instead of behind it. + // A SetWrapForced of false implies an explicit newline, which is the default. + if (newX >= newWidth) { - if (iOldRow >= positionInfo.value().get().mutableViewportTop) - { - positionInfo.value().get().mutableViewportTop = newCursor.GetPosition().y; - foundOldMutable = true; - } + newBuffer.GetRowByOffset(newY).SetWrapForced(true); + newX = 0; + newY++; } - if (!foundOldVisible) + // REFLOW_RESET: + // If we shrink the buffer vertically, for instance from 100 rows to 90 rows, we will write 10 rows in the + // new buffer twice. We need to reset them before copying text, or otherwise we'll see the previous contents. + // We don't need to be smart about this. Reset() is fast and shrinking doesn't occur often. + if (newY >= newHeight && newX == 0) { - if (iOldRow >= positionInfo.value().get().visibleViewportTop) - { - positionInfo.value().get().visibleViewportTop = newCursor.GetPosition().y; - foundOldVisible = true; - } + newBuffer.GetRowByOffset(newY).Reset(newBuffer._initialAttributes); } - } - // If we didn't have a full row to copy, insert a new - // line into the new buffer. - // Only do so if we were not forced to wrap. If we did - // force a word wrap, then the existing line break was - // only because we ran out of space. - if (iRight < cOldColsTotal && !row.WasWrapForced()) - { - if (!fFoundCursorPos && (iRight == cOldCursorPos.x && iOldRow == cOldCursorPos.y)) + auto& newRow = newBuffer.GetRowByOffset(newY); + + RowCopyTextFromState state{ + .source = oldRow, + .columnBegin = newX, + .columnLimit = til::CoordTypeMax, + .sourceColumnBegin = oldX, + .sourceColumnLimit = oldRowLimit, + }; + newRow.CopyTextFrom(state); + + const auto& oldAttr = oldRow.Attributes(); + auto& newAttr = newRow.Attributes(); + const auto attributes = oldAttr.slice(gsl::narrow_cast(oldX), oldAttr.size()); + newAttr.replace(gsl::narrow_cast(newX), newAttr.size(), attributes); + newAttr.resize_trailing_extent(newWidthU16); + + if (oldY == oldCursorPos.y && oldCursorPos.x >= oldX) { - cNewCursorPos = newCursor.GetPosition(); - fFoundCursorPos = true; + newCursorPos = { newRow.AdjustForward(oldCursorPos.x - oldX + newX), newY }; } - // Only do this if it's not the final line in the buffer. - // On the final line, we want the cursor to sit - // where it is done printing for the cursor - // adjustment to follow. - if (iOldRow < cOldRowsTotal - 1) + if (oldY >= mutableViewportTop) { - newBuffer.NewlineCursor(); + positionInfo->mutableViewportTop = newY; + mutableViewportTop = til::CoordTypeMax; } - else + if (oldY >= visibleViewportTop) { - // If we are on the final line of the buffer, we have one more check. - // We got into this code path because we are at the right most column of a row in the old buffer - // that had a hard return (no wrap was forced). - // However, as we're inserting, the old row might have just barely fit into the new buffer and - // caused a new soft return (wrap was forced) putting the cursor at x=0 on the line just below. - // We need to preserve the memory of the hard return at this point by inserting one additional - // hard newline, otherwise we've lost that information. - // We only do this when the cursor has just barely poured over onto the next line so the hard return - // isn't covered by the soft one. - // e.g. - // The old line was: - // |aaaaaaaaaaaaaaaaaaa | with no wrap which means there was a newline after that final a. - // The cursor was here ^ - // And the new line will be: - // |aaaaaaaaaaaaaaaaaaa| and show a wrap at the end - // | | - // ^ and the cursor is now there. - // If we leave it like this, we've lost the newline information. - // So we insert one more newline so a continued reflow of this buffer by resizing larger will - // continue to look as the original output intended with the newline data. - // After this fix, it looks like this: - // |aaaaaaaaaaaaaaaaaaa| no wrap at the end (preserved hard newline) - // | | - // ^ and the cursor is now here. - const auto coordNewCursor = newCursor.GetPosition(); - if (coordNewCursor.x == 0 && coordNewCursor.y > 0) - { - if (newBuffer.GetRowByOffset(coordNewCursor.y - 1).WasWrapForced()) - { - newBuffer.NewlineCursor(); - } - } + positionInfo->visibleViewportTop = newY; + visibleViewportTop = til::CoordTypeMax; } + + oldX = state.sourceColumnEnd; + newX = state.columnEnd; + } while (oldX < oldRowLimit); + + // If the row had an explicit newline we also need to newline. :) + if (!oldRow.WasWrapForced()) + { + newX = 0; + newY++; } } @@ -2616,86 +2552,68 @@ try // printable character. This is to fix the `color 2f` scenario, where you // change the buffer colors then resize and everything below the last // printable char gets reset. See GH #12567 - auto newRowY = newCursor.GetPosition().y + 1; - const auto newHeight = newBuffer.GetSize().Height(); - const auto oldHeight = oldBuffer._estimateOffsetOfLastCommittedRow() + 1; - for (; - iOldRow < oldHeight && newRowY < newHeight; - iOldRow++) + const auto initializedRowsEnd = oldBuffer._estimateOffsetOfLastCommittedRow() + 1; + for (; oldY < initializedRowsEnd && newY < newHeight; oldY++, newY++) { - const auto& row = oldBuffer.GetRowByOffset(iOldRow); - - // Optimization: Since all these rows are below the last printable char, - // we can reasonably assume that they are filled with just spaces. - // That's convenient, we can just copy the attr row from the old buffer - // into the new one, and resize the row to match. We'll rely on the - // behavior of ATTR_ROW::Resize to trim down when narrower, or extend - // the last attr when wider. - auto& newRow = newBuffer.GetRowByOffset(newRowY); - const auto newWidth = newBuffer.GetLineWidth(newRowY); - newRow.TransferAttributes(row.Attributes(), newWidth); - - newRowY++; + auto& oldRow = oldBuffer.GetRowByOffset(oldY); + auto& newRow = newBuffer.GetRowByOffset(newY); + auto& newAttr = newRow.Attributes(); + newAttr = oldRow.Attributes(); + newAttr.resize_trailing_extent(newWidthU16); } - // Finish copying remaining parameters from the old text buffer to the new one - newBuffer.CopyProperties(oldBuffer); - newBuffer.CopyHyperlinkMaps(oldBuffer); - newBuffer.CopyPatterns(oldBuffer); - - // If we found where to put the cursor while placing characters into the buffer, - // just put the cursor there. Otherwise we have to advance manually. - if (fFoundCursorPos) - { - newCursor.SetPosition(cNewCursorPos); - } - else + // This replicates the conhost behavior where the cursor is always + // within the buffer boundaries (= no delay wrap, etc.). + // In other words: If the cursor is out of bounds, we line-wrap it. + if (newCursorPos.x >= newWidth) { - // Advance the cursor to the same offset as before - // get the number of newlines and spaces between the old end of text and the old cursor, - // then advance that many newlines and chars - auto iNewlines = cOldCursorPos.y - cOldLastChar.y; - const auto iIncrements = cOldCursorPos.x - cOldLastChar.x; - const auto cNewLastChar = newBuffer.GetLastNonSpaceCharacter(); + newBuffer.GetRowByOffset(newCursorPos.y).SetWrapForced(true); + newCursorPos.x = 0; + newCursorPos.y++; - // If the last row of the new buffer wrapped, there's going to be one less newline needed, - // because the cursor is already on the next line - if (newBuffer.GetRowByOffset(cNewLastChar.y).WasWrapForced()) + // If we line-wrap the cursor past the end of the buffer we need to circle the buffer, just like we do for text. + if (newCursorPos.y >= newY) { - iNewlines = std::max(iNewlines - 1, 0); + newBuffer.GetRowByOffset(newY).Reset(newBuffer._initialAttributes); + newX = 0; + newY++; + // Fundamentally, newY is a past-the-end index (it equals the line count that we copied), whereas newCursorPos isn't. + // As such, the above if condition might as well been newCursorPos.y == newY. This assertion ensures the math is right. + assert(newCursorPos.y < newY); } - else + } + // We recalculate newCursorPos.y relative to the end of the buffer, so that a + // cursor 10 lines above the end of the buffer stays 10 lines above the end. + // NOTE: If newCursorPos.y >= newHeight, then also newY > newHeight. + if (newCursorPos.y >= newHeight) + { + newCursorPos.y += newHeight - newY; + if (newCursorPos.y < 0) { - // if this buffer didn't wrap, but the old one DID, then the d(columns) of the - // old buffer will be one more than in this buffer, so new need one LESS. - if (oldBuffer.GetRowByOffset(cOldLastChar.y).WasWrapForced()) - { - iNewlines = std::max(iNewlines - 1, 0); - } + newCursorPos = {}; } + } - for (auto r = 0; r < iNewlines; r++) - { - newBuffer.NewlineCursor(); - } - for (auto c = 0; c < iIncrements - 1; c++) - { - newBuffer.IncrementCursor(); - } + // Since we didn't use IncrementCircularBuffer() we need to compute the proper + // _firstRow offset now, in a way that replicates IncrementCircularBuffer(). + // We need to do the same for newCursorPos.y for basically the same reason. + if (newY > newHeight) + { + newBuffer._firstRow = newY % newHeight; } - // Save old cursor size before we delete it - const auto ulSize = oldCursor.GetSize(); + newBuffer.CopyProperties(oldBuffer); + newBuffer.CopyHyperlinkMaps(oldBuffer); + newBuffer.CopyPatterns(oldBuffer); - // Set size back to real size as it will be taking over the rendering duties. - newCursor.SetSize(ulSize); + assert(newCursorPos.x >= 0 && newCursorPos.x < newWidth); + assert(newCursorPos.y >= 0 && newCursorPos.y < newHeight); + newCursor.SetSize(oldCursor.GetSize()); + newCursor.SetPosition(newCursorPos); newBuffer._marks = oldBuffer._marks; newBuffer._trimMarksOutsideBuffer(); - - return S_OK; } -CATCH_RETURN() // Method Description: // - Adds or updates a hyperlink in our hyperlink table @@ -2975,14 +2893,10 @@ void TextBuffer::AddMark(const ScrollMark& m) void TextBuffer::_trimMarksOutsideBuffer() { - const auto height = GetSize().Height(); - _marks.erase(std::remove_if(_marks.begin(), - _marks.end(), - [height](const auto& m) { - return (m.start.y < 0) || - (m.start.y >= height); - }), - _marks.end()); + const til::CoordType height = _height; + std::erase_if(_marks, [height](const auto& m) { + return (m.start.y < 0) || (m.start.y >= height); + }); } void TextBuffer::SetCurrentPromptEnd(const til::point pos) noexcept diff --git a/src/buffer/out/textBuffer.hpp b/src/buffer/out/textBuffer.hpp index fe8d7e26834..2fddd8d5bf3 100644 --- a/src/buffer/out/textBuffer.hpp +++ b/src/buffer/out/textBuffer.hpp @@ -159,7 +159,7 @@ class TextBuffer final // Scroll needs access to this to quickly rotate around the buffer. void IncrementCircularBuffer(const TextAttribute& fillAttributes = {}); - til::point GetLastNonSpaceCharacter(std::optional viewOptional = std::nullopt) const; + til::point GetLastNonSpaceCharacter(const Microsoft::Console::Types::Viewport* viewOptional = nullptr) const; Cursor& GetCursor() noexcept; const Cursor& GetCursor() const noexcept; @@ -189,7 +189,7 @@ class TextBuffer final void Reset() noexcept; - [[nodiscard]] HRESULT ResizeTraditional(const til::size newSize) noexcept; + void ResizeTraditional(const til::size newSize); void SetAsActiveBuffer(const bool isActiveBuffer) noexcept; bool IsActiveBuffer() const noexcept; @@ -257,10 +257,7 @@ class TextBuffer final til::CoordType visibleViewportTop{ 0 }; }; - static HRESULT Reflow(TextBuffer& oldBuffer, - TextBuffer& newBuffer, - const std::optional lastCharacterViewport, - std::optional> positionInfo); + static void Reflow(TextBuffer& oldBuffer, TextBuffer& newBuffer, const Microsoft::Console::Types::Viewport* lastCharacterViewport = nullptr, PositionInformation* positionInfo = nullptr); const size_t AddPatternRecognizer(const std::wstring_view regexString); void ClearPatternRecognizers() noexcept; diff --git a/src/buffer/out/ut_textbuffer/ReflowTests.cpp b/src/buffer/out/ut_textbuffer/ReflowTests.cpp index 2d4a5c4a1e7..618628e5de2 100644 --- a/src/buffer/out/ut_textbuffer/ReflowTests.cpp +++ b/src/buffer/out/ut_textbuffer/ReflowTests.cpp @@ -761,7 +761,7 @@ class ReflowTests static std::unique_ptr _textBufferByReflowingTextBuffer(TextBuffer& originalBuffer, const til::size newSize) { auto buffer = std::make_unique(newSize, TextAttribute{ 0x7 }, 0, false, renderer); - TextBuffer::Reflow(originalBuffer, *buffer, std::nullopt, std::nullopt); + TextBuffer::Reflow(originalBuffer, *buffer); return buffer; } diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index 7d35ef502dc..8593304097f 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -226,6 +226,7 @@ std::wstring_view Terminal::GetWorkingDirectory() noexcept // nothing to do (the viewportSize is the same as our current size), or an // appropriate HRESULT for failing to resize. [[nodiscard]] HRESULT Terminal::UserResize(const til::size viewportSize) noexcept +try { const auto oldDimensions = _GetMutableViewport().Dimensions(); if (viewportSize == oldDimensions) @@ -233,97 +234,60 @@ std::wstring_view Terminal::GetWorkingDirectory() noexcept return S_FALSE; } - // Shortcut: if we're in the alt buffer, just resize the - // alt buffer and put off resizing the main buffer till we switch back. Fortunately, this is easy. We don't need to - // worry about the viewport and scrollback at all! The alt buffer never has - // any scrollback, so we just need to resize it and presto, we're done. + // GH#3494: We don't need to reflow the alt buffer. Apps that use the + // alt buffer will redraw themselves. This prevents graphical artifacts. + // + // This is consistent with VTE if (_inAltBuffer()) { - // stash this resize for the future. + // _deferredResize will indicate to UseMainScreenBuffer() that it needs to reflow the main buffer. + // It's unknown why we defer the reflow to a later time. Performance perhaps? _deferredResize = viewportSize; - _altBuffer->GetCursor().StartDeferDrawing(); - // we're capturing `this` here because when we exit, we want to EndDefer on the (newly created) active buffer. - auto endDefer = wil::scope_exit([this]() noexcept { _altBuffer->GetCursor().EndDeferDrawing(); }); + _altBuffer->ResizeTraditional(viewportSize); - // GH#3494: We don't need to reflow the alt buffer. Apps that use the - // alt buffer will redraw themselves. This prevents graphical artifacts. - // - // This is consistent with VTE - RETURN_IF_FAILED(_altBuffer->ResizeTraditional(viewportSize)); - - // Since the _mutableViewport is no longer the size of the actual - // viewport, then update our _altBufferSize tracker we're using to help - // us out here. _altBufferSize = viewportSize; + _altBuffer->TriggerRedrawAll(); return S_OK; } - const auto dx = viewportSize.width - oldDimensions.width; - const auto newBufferHeight = std::clamp(viewportSize.height + _scrollbackLines, 0, SHRT_MAX); - - til::size bufferSize{ viewportSize.width, newBufferHeight }; - - // This will be used to determine where the viewport should be in the new buffer. - const auto oldViewportTop = _mutableViewport.Top(); - auto newViewportTop = oldViewportTop; - auto newVisibleTop = _VisibleStartIndex(); + const auto newBufferHeight = std::clamp(viewportSize.height + _scrollbackLines, 1, SHRT_MAX); + const til::size bufferSize{ viewportSize.width, newBufferHeight }; // If the original buffer had _no_ scroll offset, then we should be at the // bottom in the new buffer as well. Track that case now. const auto originalOffsetWasZero = _scrollOffset == 0; - // skip any drawing updates that might occur until we swap _buffer with the new buffer or if we exit early. - _mainBuffer->GetCursor().StartDeferDrawing(); - // we're capturing `this` here because when we exit, we want to EndDefer on the (newly created) active buffer. - auto endDefer = wil::scope_exit([this]() noexcept { _mainBuffer->GetCursor().EndDeferDrawing(); }); + // GH#3848 - Stash away the current attributes the old text buffer is + // using. We'll initialize the new buffer with the default attributes, + // but after the resize, we'll want to make sure that the new buffer's + // current attributes (the ones used for printing new text) match the + // old buffer's. + auto newTextBuffer = std::make_unique(bufferSize, + TextAttribute{}, + 0, + _mainBuffer->IsActiveBuffer(), + _mainBuffer->GetRenderer()); + + // Build a PositionInformation to track the position of both the top of + // the mutable viewport and the top of the visible viewport in the new + // buffer. + // * the new value of mutableViewportTop will be used to figure out + // where we should place the mutable viewport in the new buffer. This + // requires a bit of trickiness to remain consistent with conpty's + // buffer (as seen below). + // * the new value of visibleViewportTop will be used to calculate the + // new scrollOffset in the new buffer, so that the visible lines on + // the screen remain roughly the same. + TextBuffer::PositionInformation positionInfo{ + .mutableViewportTop = _mutableViewport.Top(), + .visibleViewportTop = _VisibleStartIndex(), + }; - // First allocate a new text buffer to take the place of the current one. - std::unique_ptr newTextBuffer; - try - { - // GH#3848 - Stash away the current attributes the old text buffer is - // using. We'll initialize the new buffer with the default attributes, - // but after the resize, we'll want to make sure that the new buffer's - // current attributes (the ones used for printing new text) match the - // old buffer's. - const auto oldBufferAttributes = _mainBuffer->GetCurrentAttributes(); - newTextBuffer = std::make_unique(bufferSize, - TextAttribute{}, - 0, // temporarily set size to 0 so it won't render. - _mainBuffer->IsActiveBuffer(), - _mainBuffer->GetRenderer()); - - // start defer drawing on the new buffer - newTextBuffer->GetCursor().StartDeferDrawing(); - - // Build a PositionInformation to track the position of both the top of - // the mutable viewport and the top of the visible viewport in the new - // buffer. - // * the new value of mutableViewportTop will be used to figure out - // where we should place the mutable viewport in the new buffer. This - // requires a bit of trickiness to remain consistent with conpty's - // buffer (as seen below). - // * the new value of visibleViewportTop will be used to calculate the - // new scrollOffset in the new buffer, so that the visible lines on - // the screen remain roughly the same. - TextBuffer::PositionInformation oldRows{ 0 }; - oldRows.mutableViewportTop = oldViewportTop; - oldRows.visibleViewportTop = newVisibleTop; - - const std::optional oldViewStart{ oldViewportTop }; - RETURN_IF_FAILED(TextBuffer::Reflow(*_mainBuffer.get(), - *newTextBuffer.get(), - _mutableViewport, - { oldRows })); - - newViewportTop = oldRows.mutableViewportTop; - newVisibleTop = oldRows.visibleViewportTop; - - // Restore the active text attributes - newTextBuffer->SetCurrentAttributes(oldBufferAttributes); - } - CATCH_RETURN(); + TextBuffer::Reflow(*_mainBuffer.get(), *newTextBuffer.get(), &_mutableViewport, &positionInfo); + + // Restore the active text attributes + newTextBuffer->SetCurrentAttributes(_mainBuffer->GetCurrentAttributes()); // Conpty resizes a little oddly - if the height decreased, and there were // blank lines at the bottom, those lines will get trimmed. If there's not @@ -366,7 +330,7 @@ std::wstring_view Terminal::GetWorkingDirectory() noexcept const auto maxRow = std::max(newLastChar.y, newCursorPos.y); const auto proposedTopFromLastLine = maxRow - viewportSize.height + 1; - const auto proposedTopFromScrollback = newViewportTop; + const auto proposedTopFromScrollback = positionInfo.mutableViewportTop; auto proposedTop = std::max(proposedTopFromLastLine, proposedTopFromScrollback); @@ -389,17 +353,13 @@ std::wstring_view Terminal::GetWorkingDirectory() noexcept const auto proposedViewFromTop = Viewport::FromDimensions({ 0, proposedTopFromScrollback }, viewportSize); if (maxRow < proposedViewFromTop.BottomInclusive()) { - if (dx < 0 && proposedTop > 0) + if (viewportSize.width < oldDimensions.width && proposedTop > 0) { - try + const auto& row = newTextBuffer->GetRowByOffset(proposedTop - 1); + if (row.WasWrapForced()) { - const auto& row = newTextBuffer->GetRowByOffset(::base::ClampSub(proposedTop, 1)); - if (row.WasWrapForced()) - { - proposedTop--; - } + proposedTop--; } - CATCH_LOG(); } } } @@ -432,7 +392,7 @@ std::wstring_view Terminal::GetWorkingDirectory() noexcept // GH#3494: Maintain scrollbar position during resize // Make sure that we don't scroll past the mutableViewport at the bottom of the buffer - newVisibleTop = std::min(newVisibleTop, _mutableViewport.Top()); + auto newVisibleTop = std::min(positionInfo.visibleViewportTop, _mutableViewport.Top()); // Make sure we don't scroll past the top of the scrollback newVisibleTop = std::max(newVisibleTop, 0); @@ -440,16 +400,11 @@ std::wstring_view Terminal::GetWorkingDirectory() noexcept // before, and shouldn't be now either. _scrollOffset = originalOffsetWasZero ? 0 : static_cast(::base::ClampSub(_mutableViewport.Top(), newVisibleTop)); - // GH#5029 - make sure to InvalidateAll here, so that we'll paint the entire visible viewport. - try - { - _activeBuffer().TriggerRedrawAll(); - } - CATCH_LOG(); + _mainBuffer->TriggerRedrawAll(); _NotifyScrollEvent(); - return S_OK; } +CATCH_RETURN() void Terminal::Write(std::wstring_view stringView) { @@ -1034,14 +989,12 @@ int Terminal::ViewEndIndex() const noexcept // _VisibleStartIndex is the first visible line of the buffer int Terminal::_VisibleStartIndex() const noexcept { - return _inAltBuffer() ? ViewStartIndex() : - std::max(0, ViewStartIndex() - _scrollOffset); + return _inAltBuffer() ? 0 : std::max(0, _mutableViewport.Top() - _scrollOffset); } int Terminal::_VisibleEndIndex() const noexcept { - return _inAltBuffer() ? ViewEndIndex() : - std::max(0, ViewEndIndex() - _scrollOffset); + return _inAltBuffer() ? _altBufferSize.height - 1 : std::max(0, _mutableViewport.BottomInclusive() - _scrollOffset); } Viewport Terminal::_GetVisibleViewport() const noexcept diff --git a/src/host/screenInfo.cpp b/src/host/screenInfo.cpp index 048e819a34e..fca23df4c4f 100644 --- a/src/host/screenInfo.cpp +++ b/src/host/screenInfo.cpp @@ -1418,6 +1418,7 @@ bool SCREEN_INFORMATION::IsMaximizedY() const // Return Value: // - Success if successful. Invalid parameter if screen buffer size is unexpected. No memory if allocation failed. [[nodiscard]] NTSTATUS SCREEN_INFORMATION::ResizeWithReflow(const til::size coordNewScreenSize) +try { if ((USHORT)coordNewScreenSize.width >= SHORT_MAX || (USHORT)coordNewScreenSize.height >= SHORT_MAX) { @@ -1425,26 +1426,15 @@ bool SCREEN_INFORMATION::IsMaximizedY() const return STATUS_INVALID_PARAMETER; } - // First allocate a new text buffer to take the place of the current one. - std::unique_ptr newTextBuffer; - // GH#3848 - Stash away the current attributes the old text buffer is using. // We'll initialize the new buffer with the default attributes, but after // the resize, we'll want to make sure that the new buffer's current // attributes (the ones used for printing new text) match the old buffer's. - const auto oldPrimaryAttributes = _textBuffer->GetCurrentAttributes(); - try - { - newTextBuffer = std::make_unique(coordNewScreenSize, - TextAttribute{}, - 0, // temporarily set size to 0 so it won't render. - _textBuffer->IsActiveBuffer(), - _textBuffer->GetRenderer()); - } - catch (...) - { - return NTSTATUS_FROM_HRESULT(wil::ResultFromCaughtException()); - } + auto newTextBuffer = std::make_unique(coordNewScreenSize, + TextAttribute{}, + 0, // temporarily set size to 0 so it won't render. + _textBuffer->IsActiveBuffer(), + _textBuffer->GetRenderer()); // Save cursor's relative height versus the viewport const auto sCursorHeightInViewportBefore = _textBuffer->GetCursor().GetPosition().y - _viewport.Top(); @@ -1457,38 +1447,35 @@ bool SCREEN_INFORMATION::IsMaximizedY() const // we're capturing _textBuffer by reference here because when we exit, we want to EndDefer on the current active buffer. auto endDefer = wil::scope_exit([&]() noexcept { _textBuffer->GetCursor().EndDeferDrawing(); }); - auto hr = TextBuffer::Reflow(*_textBuffer.get(), *newTextBuffer.get(), std::nullopt, std::nullopt); - - if (SUCCEEDED(hr)) - { - // Since the reflow doesn't preserve the virtual bottom, we try and - // estimate where it ought to be by making it the same distance from - // the cursor row as it was before the resize. However, we also need - // to make sure it is far enough down to include the last non-space - // row, and it shouldn't be less than the height of the viewport, - // otherwise the top of the virtual viewport would end up negative. - const auto cursorRow = newTextBuffer->GetCursor().GetPosition().y; - const auto lastNonSpaceRow = newTextBuffer->GetLastNonSpaceCharacter().y; - const auto estimatedBottom = cursorRow + cursorDistanceFromBottom; - const auto viewportBottom = _viewport.Height() - 1; - _virtualBottom = std::max({ lastNonSpaceRow, estimatedBottom, viewportBottom }); + TextBuffer::Reflow(*_textBuffer.get(), *newTextBuffer.get()); - // We can't let it extend past the bottom of the buffer either. - _virtualBottom = std::min(_virtualBottom, newTextBuffer->GetSize().BottomInclusive()); + // Since the reflow doesn't preserve the virtual bottom, we try and + // estimate where it ought to be by making it the same distance from + // the cursor row as it was before the resize. However, we also need + // to make sure it is far enough down to include the last non-space + // row, and it shouldn't be less than the height of the viewport, + // otherwise the top of the virtual viewport would end up negative. + const auto cursorRow = newTextBuffer->GetCursor().GetPosition().y; + const auto lastNonSpaceRow = newTextBuffer->GetLastNonSpaceCharacter().y; + const auto estimatedBottom = cursorRow + cursorDistanceFromBottom; + const auto viewportBottom = _viewport.Height() - 1; + _virtualBottom = std::max({ lastNonSpaceRow, estimatedBottom, viewportBottom }); - // Adjust the viewport so the cursor doesn't wildly fly off up or down. - const auto sCursorHeightInViewportAfter = cursorRow - _viewport.Top(); - til::point coordCursorHeightDiff; - coordCursorHeightDiff.y = sCursorHeightInViewportAfter - sCursorHeightInViewportBefore; - LOG_IF_FAILED(SetViewportOrigin(false, coordCursorHeightDiff, false)); + // We can't let it extend past the bottom of the buffer either. + _virtualBottom = std::min(_virtualBottom, newTextBuffer->GetSize().BottomInclusive()); - newTextBuffer->SetCurrentAttributes(oldPrimaryAttributes); + // Adjust the viewport so the cursor doesn't wildly fly off up or down. + const auto sCursorHeightInViewportAfter = cursorRow - _viewport.Top(); + til::point coordCursorHeightDiff; + coordCursorHeightDiff.y = sCursorHeightInViewportAfter - sCursorHeightInViewportBefore; + LOG_IF_FAILED(SetViewportOrigin(false, coordCursorHeightDiff, false)); - _textBuffer.swap(newTextBuffer); - } + newTextBuffer->SetCurrentAttributes(_textBuffer->GetCurrentAttributes()); - return NTSTATUS_FROM_HRESULT(hr); + _textBuffer = std::move(newTextBuffer); + return STATUS_SUCCESS; } +NT_CATCH_RETURN() // // Routine Description: @@ -1498,11 +1485,14 @@ bool SCREEN_INFORMATION::IsMaximizedY() const // Return Value: // - Success if successful. Invalid parameter if screen buffer size is unexpected. No memory if allocation failed. [[nodiscard]] NTSTATUS SCREEN_INFORMATION::ResizeTraditional(const til::size coordNewScreenSize) +try { _textBuffer->GetCursor().StartDeferDrawing(); auto endDefer = wil::scope_exit([&]() noexcept { _textBuffer->GetCursor().EndDeferDrawing(); }); - return NTSTATUS_FROM_HRESULT(_textBuffer->ResizeTraditional(coordNewScreenSize)); + _textBuffer->ResizeTraditional(coordNewScreenSize); + return STATUS_SUCCESS; } +NT_CATCH_RETURN() // // Routine Description: diff --git a/src/host/ut_host/ApiRoutinesTests.cpp b/src/host/ut_host/ApiRoutinesTests.cpp index 90d3fd1044b..611eb55f14c 100644 --- a/src/host/ut_host/ApiRoutinesTests.cpp +++ b/src/host/ut_host/ApiRoutinesTests.cpp @@ -607,7 +607,7 @@ class ApiRoutinesTests auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); auto& si = gci.GetActiveOutputBuffer(); - VERIFY_SUCCEEDED(si.GetTextBuffer().ResizeTraditional({ 5, 5 }), L"Make the buffer small so this doesn't take forever."); + si.GetTextBuffer().ResizeTraditional({ 5, 5 }); // Tests are run both with and without the DECSTBM margins set. This should not alter // the results, since ScrollConsoleScreenBuffer should not be affected by VT margins. diff --git a/src/host/ut_host/ScreenBufferTests.cpp b/src/host/ut_host/ScreenBufferTests.cpp index b26801be0a1..d2d6f6ee91d 100644 --- a/src/host/ut_host/ScreenBufferTests.cpp +++ b/src/host/ut_host/ScreenBufferTests.cpp @@ -2307,7 +2307,7 @@ void ScreenBufferTests::GetWordBoundary() // Make the buffer as big as our test text. const til::size newBufferSize = { gsl::narrow(length), 10 }; - VERIFY_SUCCEEDED(si.GetTextBuffer().ResizeTraditional(newBufferSize)); + si.GetTextBuffer().ResizeTraditional(newBufferSize); const OutputCellIterator it(text, si.GetAttributes()); si.Write(it, { 0, 0 }); @@ -2383,7 +2383,7 @@ void ScreenBufferTests::GetWordBoundaryTrimZeros(const bool on) // Make the buffer as big as our test text. const til::size newBufferSize = { gsl::narrow(length), 10 }; - VERIFY_SUCCEEDED(si.GetTextBuffer().ResizeTraditional(newBufferSize)); + si.GetTextBuffer().ResizeTraditional(newBufferSize); const OutputCellIterator it(text, si.GetAttributes()); si.Write(it, { 0, 0 }); diff --git a/src/host/ut_host/TextBufferTests.cpp b/src/host/ut_host/TextBufferTests.cpp index bcb4cec4a99..091cf4c744b 100644 --- a/src/host/ut_host/TextBufferTests.cpp +++ b/src/host/ut_host/TextBufferTests.cpp @@ -1795,7 +1795,7 @@ void TextBufferTests::ResizeTraditional() auto expectedSpace = UNICODE_SPACE; std::wstring_view expectedSpaceView(&expectedSpace, 1); - VERIFY_SUCCEEDED(buffer.ResizeTraditional(newSize)); + buffer.ResizeTraditional(newSize); Log::Comment(L"Verify every cell in the X dimension is still the same as when filled and the new Y row is just empty default cells."); { @@ -1861,7 +1861,7 @@ void TextBufferTests::ResizeTraditionalRotationPreservesHighUnicode() _buffer->_SetFirstRowIndex(pos.y); // Perform resize to rotate the rows around - VERIFY_NT_SUCCESS(_buffer->ResizeTraditional(bufferSize)); + _buffer->ResizeTraditional(bufferSize); // Retrieve the text at the old and new positions. const auto shouldBeEmptyText = *_buffer->GetTextDataAt(pos); @@ -1933,7 +1933,7 @@ void TextBufferTests::ResizeTraditionalHighUnicodeRowRemoval() // Perform resize to trim off the row of the buffer that included the emoji til::size trimmedBufferSize{ bufferSize.width, bufferSize.height - 1 }; - VERIFY_NT_SUCCESS(_buffer->ResizeTraditional(trimmedBufferSize)); + _buffer->ResizeTraditional(trimmedBufferSize); } // This tests that columns removed from the buffer while resizing traditionally will also drop the high unicode @@ -1963,7 +1963,7 @@ void TextBufferTests::ResizeTraditionalHighUnicodeColumnRemoval() // Perform resize to trim off the column of the buffer that included the emoji til::size trimmedBufferSize{ bufferSize.width - 1, bufferSize.height }; - VERIFY_NT_SUCCESS(_buffer->ResizeTraditional(trimmedBufferSize)); + _buffer->ResizeTraditional(trimmedBufferSize); } void TextBufferTests::TestBurrito() diff --git a/src/terminal/adapter/ut_adapter/adapterTest.cpp b/src/terminal/adapter/ut_adapter/adapterTest.cpp index 4661c4b7ecf..d2d911d69f2 100644 --- a/src/terminal/adapter/ut_adapter/adapterTest.cpp +++ b/src/terminal/adapter/ut_adapter/adapterTest.cpp @@ -1947,26 +1947,26 @@ class AdapterTest auto& stateMachine = *_testGetSet->_stateMachine; Log::Comment(L"Default tabs stops in 80-column mode"); - VERIFY_SUCCEEDED(textBuffer.ResizeTraditional({ 80, 600 })); + textBuffer.ResizeTraditional({ 80, 600 }); _pDispatch->RequestPresentationStateReport(DispatchTypes::PresentationReportFormat::TabulationStopReport); _testGetSet->ValidateInputEvent(L"\033P2$u9/17/25/33/41/49/57/65/73\033\\"); Log::Comment(L"Default tabs stops in 132-column mode"); - VERIFY_SUCCEEDED(textBuffer.ResizeTraditional({ 132, 600 })); + textBuffer.ResizeTraditional({ 132, 600 }); _pDispatch->RequestPresentationStateReport(DispatchTypes::PresentationReportFormat::TabulationStopReport); _testGetSet->ValidateInputEvent(L"\033P2$u9/17/25/33/41/49/57/65/73/81/89/97/105/113/121/129\033\\"); Log::Comment(L"Custom tab stops in 80 columns"); - VERIFY_SUCCEEDED(textBuffer.ResizeTraditional({ 80, 600 })); + textBuffer.ResizeTraditional({ 80, 600 }); _testGetSet->_stateMachine->ProcessString(L"\033P2$t30/60/120/240\033\\"); _pDispatch->RequestPresentationStateReport(DispatchTypes::PresentationReportFormat::TabulationStopReport); _testGetSet->ValidateInputEvent(L"\033P2$u30/60\033\\"); Log::Comment(L"After expanding width to 132 columns"); - VERIFY_SUCCEEDED(textBuffer.ResizeTraditional({ 132, 600 })); + textBuffer.ResizeTraditional({ 132, 600 }); _pDispatch->RequestPresentationStateReport(DispatchTypes::PresentationReportFormat::TabulationStopReport); _testGetSet->ValidateInputEvent(L"\033P2$u30/60/120\033\\"); - VERIFY_SUCCEEDED(textBuffer.ResizeTraditional({ 80, 600 })); + textBuffer.ResizeTraditional({ 80, 600 }); Log::Comment(L"Out of order tab stops"); stateMachine.ProcessString(L"\033P2$t44/22/66\033\\"); diff --git a/src/types/UiaTextRangeBase.cpp b/src/types/UiaTextRangeBase.cpp index da6352358e8..ef421cf872c 100644 --- a/src/types/UiaTextRangeBase.cpp +++ b/src/types/UiaTextRangeBase.cpp @@ -1340,7 +1340,7 @@ til::point UiaTextRangeBase::_getDocumentEnd() const { const auto optimizedBufferSize{ _getOptimizedBufferSize() }; const auto& buffer{ _pData->GetTextBuffer() }; - const auto lastCharPos{ buffer.GetLastNonSpaceCharacter(optimizedBufferSize) }; + const auto lastCharPos{ buffer.GetLastNonSpaceCharacter(&optimizedBufferSize) }; const auto cursorPos{ buffer.GetCursor().GetPosition() }; return { optimizedBufferSize.Left(), std::max(lastCharPos.y, cursorPos.y) + 1 }; } From f725a6a0fd8eb7fb463e23e66b3395e16a2514a7 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Tue, 15 Aug 2023 16:19:59 +0200 Subject: [PATCH 02/11] Fix AuditMode failures --- src/inc/til/rle.h | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/inc/til/rle.h b/src/inc/til/rle.h index d94c3a744e6..34b6c929a0b 100644 --- a/src/inc/til/rle.h +++ b/src/inc/til/rle.h @@ -426,7 +426,7 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned" // Returns the range [start_index, end_index) as a new vector. // It works just like std::string::substr(), but with absolute indices. - [[nodiscard]] basic_rle slice(size_type start_index, size_type end_index) const noexcept + [[nodiscard]] basic_rle slice(size_type start_index, size_type end_index) const { if (end_index > _total_length) { @@ -446,14 +446,14 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned" // --> It's safe to subtract 1 from end_index rle_scanner scanner(_runs.begin(), _runs.end()); - auto [begin_run, start_run_pos] = scanner.scan(start_index); - auto [end_run, end_run_pos] = scanner.scan(end_index - 1); + const auto [begin_run, start_run_pos] = scanner.scan(start_index); + const auto [end_run, end_run_pos] = scanner.scan(end_index - 1); container slice{ begin_run, end_run + 1 }; slice.back().length = end_run_pos + 1; slice.front().length -= start_run_pos; - return { std::move(slice), static_cast(end_index - start_index) }; + return { std::move(slice), gsl::narrow_cast(end_index - start_index) }; } // Replace the range [start_index, end_index) with the given value. @@ -463,7 +463,7 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned" { _check_indices(start_index, end_index); - const rle_type replacement{ value, static_cast(end_index - start_index) }; + const rle_type replacement{ value, gsl::narrow_cast(end_index - start_index) }; _replace_unchecked(start_index, end_index, { &replacement, 1 }); } @@ -651,7 +651,7 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned" size_type total = 0; }; - basic_rle(container&& runs, size_type size) : + basic_rle(container&& runs, size_type size) noexcept : _runs(std::forward(runs)), _total_length(size) { From 0230743be2d92c1e9b686cbbe29272555954bc7d Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Fri, 25 Aug 2023 14:08:44 +0200 Subject: [PATCH 03/11] Address feedback --- src/cascadia/TerminalCore/Terminal.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index 8593304097f..80ad58c653d 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -234,16 +234,16 @@ try return S_FALSE; } - // GH#3494: We don't need to reflow the alt buffer. Apps that use the - // alt buffer will redraw themselves. This prevents graphical artifacts. - // - // This is consistent with VTE if (_inAltBuffer()) { // _deferredResize will indicate to UseMainScreenBuffer() that it needs to reflow the main buffer. - // It's unknown why we defer the reflow to a later time. Performance perhaps? + // Deferring the reflow of the main buffer has the benefit that it avoids destroying the state + // of the text buffer any more than necessary. For ConPTY in particular a reflow is destructive, + // because it "forgets" text that wraps beyond the top of its viewport when shrinking it. _deferredResize = viewportSize; + // GH#3494: We don't need to reflow the alt buffer. Apps that use the alt buffer will + // redraw themselves. This prevents graphical artifacts and is consistent with VTE. _altBuffer->ResizeTraditional(viewportSize); _altBufferSize = viewportSize; From 6374aece11edbf8b1329d56ab963e6ea2089f83d Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Fri, 25 Aug 2023 17:25:25 +0200 Subject: [PATCH 04/11] Fix 2 cursor related bugs and all TestReflowCases --- .github/actions/spelling/expect/expect.txt | 1 + src/buffer/out/textBuffer.cpp | 65 +++--- src/buffer/out/ut_textbuffer/ReflowTests.cpp | 219 ++++++++----------- 3 files changed, 123 insertions(+), 162 deletions(-) diff --git a/.github/actions/spelling/expect/expect.txt b/.github/actions/spelling/expect/expect.txt index 2e552912cfa..36f9aaf96bd 100644 --- a/.github/actions/spelling/expect/expect.txt +++ b/.github/actions/spelling/expect/expect.txt @@ -2280,6 +2280,7 @@ YCast YCENTER YCount YDPI +YLimit YOffset YSubstantial YVIRTUALSCREEN diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index 4ebe6e9de06..5e43c16bfbc 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -2365,12 +2365,14 @@ void TextBuffer::Reflow(TextBuffer& oldBuffer, TextBuffer& newBuffer, const View til::CoordType newY = 0; til::CoordType newX = 0; til::CoordType newWidth = newBuffer.GetSize().Width(); + til::CoordType newYLimit = til::CoordTypeMax; + const auto oldHeight = std::max(lastRowWithText, oldCursorPos.y) + 1; const auto newHeight = newBuffer.GetSize().Height(); const auto newWidthU16 = gsl::narrow_cast(newWidth); // Copy oldBuffer into newBuffer until oldBuffer has been fully consumed. - for (; oldY < oldHeight; ++oldY) + for (; oldY < oldHeight && newY < newYLimit; ++oldY) { const auto& oldRow = oldBuffer.GetRowByOffset(oldY); @@ -2400,7 +2402,7 @@ void TextBuffer::Reflow(TextBuffer& oldBuffer, TextBuffer& newBuffer, const View if (oldY == oldCursorPos.y) { - newCursorPos = { oldCursorPos.x, newY }; + newCursorPos = { newRow.AdjustBackward(oldCursorPos.x), newY }; } if (oldY >= mutableViewportTop) { @@ -2419,7 +2421,17 @@ void TextBuffer::Reflow(TextBuffer& oldBuffer, TextBuffer& newBuffer, const View // Rows don't store any information for what column the last written character is in. // We simply truncate all trailing whitespace in this implementation. - const auto oldRowLimit = oldRow.MeasureRight(); + auto oldRowLimit = oldRow.MeasureRight(); + if (oldY == oldCursorPos.y) + { + // REFLOW_JANK_CURSOR_WRAP: + // Pretending as if there's always at least whitespace in front of the cursor has the benefit that + // * the cursor retains its distance from any preceding text. + // * when a client application starts writing on this new, empty line, + // enlarging the buffer unwraps the text onto the preceding line. + oldRowLimit = std::max(oldRowLimit, oldCursorPos.x + 1); + } + til::CoordType oldX = 0; // Copy oldRow into newBuffer until oldRow has been fully consumed. @@ -2444,6 +2456,11 @@ void TextBuffer::Reflow(TextBuffer& oldBuffer, TextBuffer& newBuffer, const View // We don't need to be smart about this. Reset() is fast and shrinking doesn't occur often. if (newY >= newHeight && newX == 0) { + // We need to ensure not to overwrite the row the cursor is on. + if (newY >= newYLimit) + { + break; + } newBuffer.GetMutableRowByOffset(newY).Reset(newBuffer._initialAttributes); } @@ -2466,7 +2483,10 @@ void TextBuffer::Reflow(TextBuffer& oldBuffer, TextBuffer& newBuffer, const View if (oldY == oldCursorPos.y && oldCursorPos.x >= oldX) { - newCursorPos = { newRow.AdjustForward(oldCursorPos.x - oldX + newX), newY }; + // In theory AdjustBackward ensures we don't put the cursor on a trailing wide glyph. + // In practice I don't think that this can possibly happen. Better safe than sorry. + newCursorPos = { newRow.AdjustBackward(oldCursorPos.x - oldX + newX), newY }; + newYLimit = newY + newHeight; } if (oldY >= mutableViewportTop) { @@ -2505,44 +2525,17 @@ void TextBuffer::Reflow(TextBuffer& oldBuffer, TextBuffer& newBuffer, const View newAttr.resize_trailing_extent(newWidthU16); } - // This replicates the conhost behavior where the cursor is always - // within the buffer boundaries (= no delay wrap, etc.). - // In other words: If the cursor is out of bounds, we line-wrap it. - if (newCursorPos.x >= newWidth) - { - newBuffer.GetMutableRowByOffset(newCursorPos.y).SetWrapForced(true); - newCursorPos.x = 0; - newCursorPos.y++; - - // If we line-wrap the cursor past the end of the buffer we need to circle the buffer, just like we do for text. - if (newCursorPos.y >= newY) - { - newBuffer.GetMutableRowByOffset(newY).Reset(newBuffer._initialAttributes); - newX = 0; - newY++; - // Fundamentally, newY is a past-the-end index (it equals the line count that we copied), whereas newCursorPos isn't. - // As such, the above if condition might as well been newCursorPos.y == newY. This assertion ensures the math is right. - assert(newCursorPos.y < newY); - } - } - // We recalculate newCursorPos.y relative to the end of the buffer, so that a - // cursor 10 lines above the end of the buffer stays 10 lines above the end. - // NOTE: If newCursorPos.y >= newHeight, then also newY > newHeight. - if (newCursorPos.y >= newHeight) - { - newCursorPos.y += newHeight - newY; - if (newCursorPos.y < 0) - { - newCursorPos = {}; - } - } - // Since we didn't use IncrementCircularBuffer() we need to compute the proper // _firstRow offset now, in a way that replicates IncrementCircularBuffer(). // We need to do the same for newCursorPos.y for basically the same reason. if (newY > newHeight) { newBuffer._firstRow = newY % newHeight; + // _firstRow maps from API coordinates that always start at 0,0 in the top left corner of the + // terminal's scrollback, to the underlying buffer Y coordinate via `(y + _firstRow) % height`. + // Here, we need to un-map the `newCursorPos.y` from the underlying Y coordinate to the API coordinate + // and so we do `(y - _firstRow) % height`, but we add `+ newHeight` to avoid getting negative results. + newCursorPos.y = (newCursorPos.y - newBuffer._firstRow + newHeight) % newHeight; } newBuffer.CopyProperties(oldBuffer); diff --git a/src/buffer/out/ut_textbuffer/ReflowTests.cpp b/src/buffer/out/ut_textbuffer/ReflowTests.cpp index 68d32cef9ba..b4cb692c6bd 100644 --- a/src/buffer/out/ut_textbuffer/ReflowTests.cpp +++ b/src/buffer/out/ut_textbuffer/ReflowTests.cpp @@ -61,7 +61,7 @@ namespace { L"EFG ", false }, { L" ", false }, }, - { 0, 1 } // cursor on $ + { 0, 1 }, // cursor on $ }, TestBuffer{ { 5, 5 }, // reduce width by 1 @@ -72,7 +72,7 @@ namespace { L"EFG ", false }, { L" ", false }, }, - { 0, 1 } // cursor on $ + { 0, 1 }, // cursor on $ }, TestBuffer{ { 4, 5 }, @@ -83,7 +83,7 @@ namespace { L"EFG ", false }, { L" ", false }, }, - { 0, 1 } // cursor on $ + { 0, 1 }, // cursor on $ }, }, }, @@ -99,40 +99,40 @@ namespace { L" ", false }, { L" ", false }, }, - { 0, 1 } // cursor on $ + { 0, 1 }, // cursor on $ }, TestBuffer{ { 5, 5 }, // reduce width by 1 { { L"ABCDE", true }, - { L"F$ ", false }, // [BUG] EXACT WRAP. $ should be alone on next line. - { L" ", false }, + { L"F ", false }, + { L"$ ", false }, { L" ", false }, { L" ", false }, }, - { 1, 1 } // cursor on $ + { 0, 2 }, // cursor on $ }, TestBuffer{ { 6, 5 }, // grow width back to original { - { L"ABCDEF", true_due_to_exact_wrap_bug }, + { L"ABCDEF", false }, { L"$ ", false }, { L" ", false }, { L" ", false }, { L" ", false }, }, - { 0, 1 } // cursor on $ + { 0, 1 }, // cursor on $ }, TestBuffer{ { 7, 5 }, // grow width wider than original { - { L"ABCDEF$", true_due_to_exact_wrap_bug }, // EXACT WRAP BUG: $ should be alone on next line - { L" ", false }, + { L"ABCDEF ", false }, + { L"$ ", false }, { L" ", false }, { L" ", false }, { L" ", false }, }, - { 6, 0 } // cursor on $ + { 0, 1 }, // cursor on $ }, }, }, @@ -148,7 +148,7 @@ namespace { L" ", false }, { L" ", false }, }, - { 1, 1 } // cursor on $ + { 1, 1 }, // cursor on $ }, TestBuffer{ { 5, 5 }, // reduce width by 1 @@ -159,7 +159,7 @@ namespace { L" ", false }, { L" ", false }, }, - { 2, 1 } // cursor on $ + { 2, 1 }, // cursor on $ }, TestBuffer{ { 6, 5 }, // grow width back to original @@ -170,7 +170,7 @@ namespace { L" ", false }, { L" ", false }, }, - { 1, 1 } // cursor on $ + { 1, 1 }, // cursor on $ }, TestBuffer{ { 7, 5 }, // grow width wider than original @@ -181,7 +181,7 @@ namespace { L" ", false }, { L" ", false }, }, - { 0, 1 } // cursor on $ + { 0, 1 }, // cursor on $ }, }, }, @@ -197,29 +197,29 @@ namespace { L"EFG ", false }, { L" ", false }, }, - { 0, 1 } // cursor on $ + { 0, 1 }, // cursor on $ }, TestBuffer{ { 7, 5 }, // reduce width by 1 { { L"AB $", true }, - { L" CD", true_due_to_exact_wrap_bug }, - { L" ", false }, + { L" CD", false }, // CD ends with a newline -> .wrap = false { L"EFG ", false }, { L" ", false }, + { L" ", false }, }, - { 6, 0 } // cursor on $ + { 6, 0 }, // cursor on $ }, TestBuffer{ { 8, 5 }, { { L"AB $ ", true }, - { L" CD ", false }, // Goes to false because we hit the end of ..CD - { L"EFG ", false }, // [BUG] EFG moves up due to exact wrap bug above + { L" CD ", false }, + { L"EFG ", false }, { L" ", false }, { L" ", false }, }, - { 6, 0 } // cursor on $ + { 6, 0 }, // cursor on $ }, }, }, @@ -236,19 +236,19 @@ namespace { L" ", false }, { L" ", false }, }, - { 2, 1 } // cursor on $ + { 2, 1 }, // cursor on $ }, TestBuffer{ { 5, 5 }, // reduce width by 1 { //--012345-- { L"カタ ", true }, // KA TA [FORCED SPACER] - { L"カナ$", true_due_to_exact_wrap_bug }, // KA NA + { L"カナ$", false }, // KA NA { L" ", false }, { L" ", false }, { L" ", false }, }, - { 4, 1 } // cursor on $ + { 4, 1 }, // cursor on $ }, TestBuffer{ { 6, 5 }, // grow width back to original @@ -260,7 +260,7 @@ namespace { L" ", false }, { L" ", false }, }, - { 2, 1 } // cursor on $ + { 2, 1 }, // cursor on $ }, TestBuffer{ { 7, 5 }, // grow width wider than original (by one; no visible change!) @@ -272,7 +272,7 @@ namespace { L" ", false }, { L" ", false }, }, - { 2, 1 } // cursor on $ + { 2, 1 }, // cursor on $ }, TestBuffer{ { 8, 5 }, // grow width enough to fit second DBCS @@ -284,7 +284,7 @@ namespace { L" ", false }, { L" ", false }, }, - { 0, 1 } // cursor on $ + { 0, 1 }, // cursor on $ }, }, }, @@ -300,41 +300,29 @@ namespace { L"MNOPQR", false }, { L"STUVWX", false }, }, - { 0, 1 } // cursor on $ + { 0, 1 }, // cursor on $ }, TestBuffer{ { 5, 5 }, // reduce width by 1 { - { L"F$ ", false }, - { L"GHIJK", true }, // [BUG] We should see GHIJK\n L\n MNOPQ\n R\n - { L"LMNOP", true }, // The wrapping here is irregular - { L"QRSTU", true }, - { L"VWX ", false }, + { L"$ ", false }, + { L"GHIJK", true }, + { L"L ", false }, + { L"MNOPQ", true }, + { L"R ", false }, }, - { 1, 1 } // [BUG] cursor moves to 1,1 instead of sticking with the $ + { 0, 0 }, }, TestBuffer{ { 6, 5 }, // going back to 6,5, the data lost has been destroyed { - //{ L"F$ ", false }, // [BUG] this line is erroneously destroyed too! - { L"GHIJKL", true }, - { L"MNOPQR", true }, - { L"STUVWX", true }, + { L"$ ", false }, + { L"GHIJKL", false }, + { L"MNOPQR", false }, + { L" ", false }, { L" ", false }, - { L" ", false }, // [BUG] this line is added - }, - { 1, 1 }, // [BUG] cursor does not follow [H], it sticks at 1,1 - }, - TestBuffer{ - { 7, 5 }, // a number of errors are carried forward from the previous buffer - { - { L"GHIJKLM", true }, - { L"NOPQRST", true }, - { L"UVWX ", false }, // [BUG] This line loses wrap for some reason - { L" ", false }, - { L" ", false }, }, - { 0, 1 }, // [BUG] The cursor moves to 0, 1 now, sticking with the [N] from before + { 0, 0 }, }, }, }, @@ -353,18 +341,18 @@ namespace { L" ", false }, { L" ", false }, }, - { 1, 1 } // cursor *after* $ + { 1, 1 }, // cursor *after* $ }, TestBuffer{ { 5, 5 }, // reduce width by 1 { { L"ABCDE", true }, - { L"F$ ", false }, // [BUG] EXACT WRAP. $ should be alone on next line. - { L" ", false }, + { L"F ", false }, + { L"$ ", false }, { L" ", false }, { L" ", false }, }, - { 2, 1 } // cursor follows space after $ to next line + { 1, 2 }, // cursor follows space after $ to next line }, }, }, @@ -380,7 +368,7 @@ namespace { L"STUVWX", true }, { L"YZ0 $ ", false }, }, - { 5, 4 } // cursor *after* $ + { 5, 4 }, // cursor *after* $ }, TestBuffer{ { 5, 5 }, // reduce width by 1 @@ -391,7 +379,7 @@ namespace { L"UVWXY", true }, { L"Z0 $ ", false }, }, - { 4, 4 } // cursor follows space after $ to newly introduced bottom line + { 4, 4 }, // cursor follows space after $ to newly introduced bottom line }, }, }, @@ -402,40 +390,36 @@ namespace { 6, 5 }, { { L"ABCDEF", false }, - // v cursor { L"$ ", false }, - // ^ cursor { L" ", false }, { L" ", false }, { L" ", false }, }, - { 5, 1 } // cursor in space far after $ + { 5, 1 }, // The cursor is 5 columns to the right of the $ (last column). }, TestBuffer{ { 5, 5 }, // reduce width by 1 { { L"ABCDE", true }, - { L"F$ ", true }, // [BUG] This line is marked wrapped, and I do not know why - // v cursor - { L" ", false }, - // ^ cursor + { L"F ", false }, + // The reflow implementation marks a wrapped cursor as a forced row-wrap (= the row is padded with whitespace), so that when + // the buffer is enlarged again, we restore the original cursor position correctly. That's why it says .cursor={5,1} below. + { L"$ ", true }, { L" ", false }, { L" ", false }, }, - { 1, 2 } // cursor stays same linear distance from $ + { 0, 3 }, // $ is now at 0,2 and the cursor used to be 5 columns to the right. -> 0,3 }, TestBuffer{ { 6, 5 }, // grow back to original size { - { L"ABCDEF", true_due_to_exact_wrap_bug }, - // v cursor [BUG] cursor does not retain linear distance from $ + { L"ABCDEF", false }, { L"$ ", false }, - // ^ cursor { L" ", false }, { L" ", false }, { L" ", false }, }, - { 4, 1 } // cursor stays same linear distance from $ + { 5, 1 }, }, }, }, @@ -446,39 +430,37 @@ namespace { 6, 5 }, { { L"ABCDEF", false }, - // v cursor { L"$ ", false }, - // ^ cursor { L"BLAH ", false }, { L"BLAH ", false }, { L" ", false }, }, - { 5, 1 } // cursor in space far after $ + { 5, 1 }, // The cursor is 5 columns to the right of the $ (last column). }, TestBuffer{ { 5, 5 }, // reduce width by 1 { - { L"ABCDE", true }, - { L"F$ ", false }, - { L"BLAH ", false }, - { L"BLAH ", true }, // [BUG] this line wraps, no idea why - // v cursor [BUG] cursor erroneously moved to end of all content + { L"F ", false }, + // The reflow implementation pads the row with the cursor with whitespace. + // Search for "REFLOW_JANK_CURSOR_WRAP" to find the corresponding code. + { L"$ ", true }, { L" ", false }, - // ^ cursor + { L"BLAH ", false }, + { L"BLAH ", false }, }, - { 0, 4 } }, + { 0, 2 }, + }, TestBuffer{ { 6, 5 }, // grow back to original size { - { L"ABCDEF", true }, + { L"F ", false }, { L"$ ", false }, { L"BLAH ", false }, - // v cursor [BUG] cursor is pulled up to previous line because it was marked wrapped { L"BLAH ", false }, - // ^ cursor { L" ", false }, }, - { 5, 3 } }, + { 5, 1 }, + }, }, }, TestCase{ @@ -489,27 +471,24 @@ namespace { 6, 5 }, { { L"ABCDEF", false }, - // v cursor { L"$ ", false }, - // ^ cursor { L" ", false }, { L" ", false }, { L" ", false }, }, - { 5, 1 } // cursor in space far after $ + { 5, 1 }, // The cursor is 5 columns to the right of the $ (last column). }, TestBuffer{ { 2, 5 }, // reduce width aggressively { { L"CD", true }, - { L"EF", true }, + { L"EF", false }, { L"$ ", true }, { L" ", true }, - // v cursor { L" ", false }, - // ^ cursor }, - { 1, 4 } }, + { 1, 4 }, + }, }, }, TestCase{ @@ -519,27 +498,24 @@ namespace { 6, 5 }, { { L"ABCDEF", true }, - // v cursor { L"$ ", true }, - // ^ cursor { L" ", true }, { L" ", true }, { L" ", true }, }, - { 5, 1 } // cursor in space far after $ + { 5, 1 }, // cursor in space far after $ }, TestBuffer{ { 2, 5 }, // reduce width aggressively { - { L"EF", true }, - { L"$ ", true }, { L" ", true }, { L" ", true }, - // v cursor [BUG] cursor does not maintain linear distance from $ - { L" ", false }, - // ^ cursor + { L" ", true }, + { L" ", true }, + { L" ", true }, }, - { 1, 4 } }, + { 1, 0 }, + }, }, }, TestCase{ @@ -549,14 +525,12 @@ namespace { 6, 5 }, { { L"ABCDEF", true }, - // v cursor { L"$ ", true }, - // ^ cursor { L" ", true }, { L" ", true }, { L" Q", true }, }, - { 5, 1 } // cursor in space far after $ + { 5, 1 }, // cursor in space far after $ }, TestBuffer{ { 2, 5 }, // reduce width aggressively @@ -564,12 +538,11 @@ namespace { L" ", true }, { L" ", true }, { L" ", true }, - { L" Q", true }, - // v cursor [BUG] cursor jumps to end of world - { L" ", false }, // POTENTIAL [BUG] a whole new blank line is added for the cursor - // ^ cursor + { L" ", true }, + { L" ", true }, }, - { 1, 4 } }, + { 1, 0 }, + }, }, }, TestCase{ @@ -579,27 +552,24 @@ namespace { 6, 5 }, { { L"ABCDEF", false }, - // v cursor { L"$ ", false }, - // ^ cursor { L" ", false }, { L" ", true }, { L" Q", true }, }, - { 5, 1 } // cursor in space far after $ + { 5, 1 }, // cursor in space far after $ }, TestBuffer{ { 2, 5 }, // reduce width aggressively { + { L" ", false }, + { L" ", false }, { L" ", true }, { L" ", true }, { L" ", true }, - { L" Q", true }, - // v cursor [BUG] cursor jumps to different place than fully wrapped case - { L" ", false }, - // ^ cursor }, - { 0, 4 } }, + { 1, 0 }, + }, }, }, TestCase{ @@ -614,24 +584,21 @@ namespace { L"$ ", false }, { L" Q", true }, { L" ", true }, - // v cursor { L" ", true }, - // ^ cursor }, - { 5, 4 } // cursor at end of buffer + { 5, 4 }, // cursor at end of buffer }, TestBuffer{ { 2, 5 }, // reduce width aggressively { + { L" ", false }, + { L" ", true }, + { L" ", true }, { L" ", true }, { L" ", true }, - { L" Q", true }, - { L" ", false }, - // v cursor [BUG] cursor loses linear distance from Q; is this important? - { L" ", false }, - // ^ cursor }, - { 0, 4 } }, + { 1, 0 }, + }, }, }, }; From 349082a0017e958748ae8f608725e80c73bbf767 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Fri, 25 Aug 2023 18:50:42 +0200 Subject: [PATCH 05/11] Cleaned up some small leftovers --- src/buffer/out/ut_textbuffer/ReflowTests.cpp | 2 -- src/cascadia/TerminalCore/Terminal.cpp | 3 +-- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/src/buffer/out/ut_textbuffer/ReflowTests.cpp b/src/buffer/out/ut_textbuffer/ReflowTests.cpp index b4cb692c6bd..d1fad4c407f 100644 --- a/src/buffer/out/ut_textbuffer/ReflowTests.cpp +++ b/src/buffer/out/ut_textbuffer/ReflowTests.cpp @@ -46,8 +46,6 @@ namespace std::vector buffers; }; - static constexpr auto true_due_to_exact_wrap_bug{ true }; - static const TestCase testCases[] = { TestCase{ L"No reflow required", diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index 8331a32a6d7..6306de5ad41 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -259,8 +259,7 @@ try // bottom in the new buffer as well. Track that case now. const auto originalOffsetWasZero = _scrollOffset == 0; - // GH#3848 - Stash away the current attributes the old text buffer is - // using. We'll initialize the new buffer with the default attributes, + // GH#3848 - We'll initialize the new buffer with the default attributes, // but after the resize, we'll want to make sure that the new buffer's // current attributes (the ones used for printing new text) match the // old buffer's. From 0fbb882e90b8688b34ef827b3af2e1193dd5cc9e Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Fri, 25 Aug 2023 19:49:24 +0200 Subject: [PATCH 06/11] Address feedback --- src/host/screenInfo.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/host/screenInfo.cpp b/src/host/screenInfo.cpp index fca23df4c4f..a20b1192ca1 100644 --- a/src/host/screenInfo.cpp +++ b/src/host/screenInfo.cpp @@ -1426,9 +1426,8 @@ try return STATUS_INVALID_PARAMETER; } - // GH#3848 - Stash away the current attributes the old text buffer is using. - // We'll initialize the new buffer with the default attributes, but after - // the resize, we'll want to make sure that the new buffer's current + // GH#3848 - We'll initialize the new buffer with the default attributes, + // but after the resize, we'll want to make sure that the new buffer's current // attributes (the ones used for printing new text) match the old buffer's. auto newTextBuffer = std::make_unique(coordNewScreenSize, TextAttribute{}, From a72e2cb973ad8e54dfcd15203c9cb1d800ec37d2 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Tue, 5 Sep 2023 15:16:25 +0200 Subject: [PATCH 07/11] Reflow doesn't call IncrementCircularBuffer anymore --- src/host/VtIo.cpp | 34 ---------------------------------- src/host/VtIo.hpp | 3 --- src/host/screenInfo.cpp | 13 ------------- src/renderer/vt/invalidate.cpp | 16 ++++------------ src/renderer/vt/state.cpp | 29 ----------------------------- src/renderer/vt/vtrenderer.hpp | 1 - 6 files changed, 4 insertions(+), 92 deletions(-) diff --git a/src/host/VtIo.cpp b/src/host/VtIo.cpp index 5d33e607931..b7e8f22c132 100644 --- a/src/host/VtIo.cpp +++ b/src/host/VtIo.cpp @@ -463,40 +463,6 @@ void VtIo::SendCloseEvent() } } -// Method Description: -// - Tell the vt renderer to begin a resize operation. During a resize -// operation, the vt renderer should _not_ request to be repainted during a -// text buffer circling event. Any callers of this method should make sure to -// call EndResize to make sure the renderer returns to normal behavior. -// See GH#1795 for context on this method. -// Arguments: -// - -// Return Value: -// - -void VtIo::BeginResize() -{ - if (_pVtRenderEngine) - { - _pVtRenderEngine->BeginResizeRequest(); - } -} - -// Method Description: -// - Tell the vt renderer to end a resize operation. -// See BeginResize for more details. -// See GH#1795 for context on this method. -// Arguments: -// - -// Return Value: -// - -void VtIo::EndResize() -{ - if (_pVtRenderEngine) - { - _pVtRenderEngine->EndResizeRequest(); - } -} - #ifdef UNIT_TESTING // Method Description: // - This is a test helper method. It can be used to trick VtIo into responding diff --git a/src/host/VtIo.hpp b/src/host/VtIo.hpp index a15653ad084..9b4af712daa 100644 --- a/src/host/VtIo.hpp +++ b/src/host/VtIo.hpp @@ -40,9 +40,6 @@ namespace Microsoft::Console::VirtualTerminal void CloseInput(); void CloseOutput(); - void BeginResize(); - void EndResize(); - #ifdef UNIT_TESTING void EnableConptyModeForTests(std::unique_ptr vtRenderEngine); #endif diff --git a/src/host/screenInfo.cpp b/src/host/screenInfo.cpp index a20b1192ca1..8d1b673b84e 100644 --- a/src/host/screenInfo.cpp +++ b/src/host/screenInfo.cpp @@ -1513,19 +1513,6 @@ NT_CATCH_RETURN() auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); auto status = STATUS_SUCCESS; - // If we're in conpty mode, suppress any immediate painting we might do - // during the resize. - if (gci.IsInVtIoMode()) - { - gci.GetVtIo()->BeginResize(); - } - auto endResize = wil::scope_exit([&] { - if (gci.IsInVtIoMode()) - { - gci.GetVtIo()->EndResize(); - } - }); - // cancel any active selection before resizing or it will not necessarily line up with the new buffer positions Selection::Instance().ClearSelection(); diff --git a/src/renderer/vt/invalidate.cpp b/src/renderer/vt/invalidate.cpp index 1c7f3fe498f..4491a6feb3c 100644 --- a/src/renderer/vt/invalidate.cpp +++ b/src/renderer/vt/invalidate.cpp @@ -106,19 +106,11 @@ CATCH_RETURN(); // - S_OK [[nodiscard]] HRESULT VtEngine::InvalidateFlush(_In_ const bool circled, _Out_ bool* const pForcePaint) noexcept { - // If we're in the middle of a resize request, don't try to immediately start a frame. - if (_inResizeRequest) - { - *pForcePaint = false; - } - else - { - *pForcePaint = true; + *pForcePaint = true; - // Keep track of the fact that we circled, we'll need to do some work on - // end paint to specifically handle this. - _circled = circled; - } + // Keep track of the fact that we circled, we'll need to do some work on + // end paint to specifically handle this. + _circled = circled; // If we flushed for any reason other than circling (i.e, a sequence that we // didn't understand), we don't need to push the buffer out on EndPaint. diff --git a/src/renderer/vt/state.cpp b/src/renderer/vt/state.cpp index 29a8657cd5a..9687f0f8e58 100644 --- a/src/renderer/vt/state.cpp +++ b/src/renderer/vt/state.cpp @@ -50,7 +50,6 @@ VtEngine::VtEngine(_In_ wil::unique_hfile pipe, _terminalOwner{ nullptr }, _newBottomLine{ false }, _deferredCursorPos{ INVALID_COORDS }, - _inResizeRequest{ false }, _trace{}, _bufferLine{}, _buffer{}, @@ -446,34 +445,6 @@ HRESULT VtEngine::RequestCursor() noexcept return S_OK; } -// Method Description: -// - Tell the vt renderer to begin a resize operation. During a resize -// operation, the vt renderer should _not_ request to be repainted during a -// text buffer circling event. Any callers of this method should make sure to -// call EndResize to make sure the renderer returns to normal behavior. -// See GH#1795 for context on this method. -// Arguments: -// - -// Return Value: -// - -void VtEngine::BeginResizeRequest() -{ - _inResizeRequest = true; -} - -// Method Description: -// - Tell the vt renderer to end a resize operation. -// See BeginResize for more details. -// See GH#1795 for context on this method. -// Arguments: -// - -// Return Value: -// - -void VtEngine::EndResizeRequest() -{ - _inResizeRequest = false; -} - // Method Description: // - Configure the renderer for the resize quirk. This changes the behavior of // conpty to _not_ InvalidateAll the entire viewport on a resize operation. diff --git a/src/renderer/vt/vtrenderer.hpp b/src/renderer/vt/vtrenderer.hpp index 3ba1e702af6..eb877b8428d 100644 --- a/src/renderer/vt/vtrenderer.hpp +++ b/src/renderer/vt/vtrenderer.hpp @@ -131,7 +131,6 @@ namespace Microsoft::Console::Render Microsoft::Console::VirtualTerminal::VtIo* _terminalOwner; Microsoft::Console::VirtualTerminal::RenderTracing _trace; - bool _inResizeRequest{ false }; std::optional _wrappedRow{ std::nullopt }; From 1546a99acf55b546a1e00aa7f859ddf9e35a989e Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Wed, 6 Sep 2023 16:43:24 +0200 Subject: [PATCH 08/11] Fix OOB cursor coordinates when exiting alt buffer --- src/buffer/out/textBuffer.cpp | 11 ++++- src/cascadia/TerminalCore/TerminalApi.cpp | 50 +++++++++++------------ 2 files changed, 35 insertions(+), 26 deletions(-) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index a9ce9ee1000..0e1f2eaf9ff 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -2411,9 +2411,18 @@ void TextBuffer::Reflow(TextBuffer& oldBuffer, TextBuffer& newBuffer, const View const auto& oldCursor = oldBuffer.GetCursor(); auto& newCursor = newBuffer.GetCursor(); - const auto oldCursorPos = oldCursor.GetPosition(); + til::point oldCursorPos = oldCursor.GetPosition(); til::point newCursorPos; + // BODGY: We use oldCursorPos in two critical places below: + // * To compute an oldHeight that includes at a minimum the cursor row + // * For REFLOW_JANK_CURSOR_WRAP (see comment below) + // Both of these would break the reflow algorithm, but the latter of the two in particular + // would cause the main copy loop below to deadlock. In other words, these two lines + // protect this function against yet-unknown bugs in other parts of the code base. + oldCursorPos.x = std::clamp(oldCursorPos.x, 0, oldBuffer._width - 1); + oldCursorPos.y = std::clamp(oldCursorPos.y, 0, oldBuffer._height - 1); + const auto lastRowWithText = oldBuffer.GetLastNonSpaceCharacter(lastCharacterViewport).y; auto mutableViewportTop = positionInfo ? positionInfo->mutableViewportTop : til::CoordTypeMax; diff --git a/src/cascadia/TerminalCore/TerminalApi.cpp b/src/cascadia/TerminalCore/TerminalApi.cpp index 4c31cbc4bbc..a5e17ee0c7e 100644 --- a/src/cascadia/TerminalCore/TerminalApi.cpp +++ b/src/cascadia/TerminalCore/TerminalApi.cpp @@ -236,33 +236,19 @@ void Terminal::UseAlternateScreenBuffer(const TextAttribute& attrs) } void Terminal::UseMainScreenBuffer() { - // Short-circuit: do nothing. - if (!_inAltBuffer()) + // To make UserResize() work as if we're back in the main buffer, we first need to unset + // _altBuffer, which is used throughout this class as an indicator via _inAltBuffer(). + // + // We delay destroying the alt buffer instance to get a valid altBuffer->GetCursor() reference below. + const auto altBuffer = std::exchange(_altBuffer, nullptr); + if (!altBuffer) { return; } ClearSelection(); - // Copy our cursor state back to the main buffer's cursor - { - // Update the alt buffer's cursor style, visibility, and position to match our own. - const auto& myCursor = _altBuffer->GetCursor(); - auto& tgtCursor = _mainBuffer->GetCursor(); - tgtCursor.SetStyle(myCursor.GetSize(), myCursor.GetType()); - tgtCursor.SetIsVisible(myCursor.IsVisible()); - tgtCursor.SetBlinkingAllowed(myCursor.IsBlinkingAllowed()); - - // The new position should match the viewport-relative position of the main buffer. - // This is the equal and opposite effect of what we did in UseAlternateScreenBuffer - auto tgtCursorPos = myCursor.GetPosition(); - tgtCursorPos.y += _mutableViewport.Top(); - tgtCursor.SetPosition(tgtCursorPos); - } - _mainBuffer->SetAsActiveBuffer(true); - // destroy the alt buffer - _altBuffer = nullptr; if (_deferredResize.has_value()) { @@ -270,6 +256,24 @@ void Terminal::UseMainScreenBuffer() _deferredResize = std::nullopt; } + // After exiting the alt buffer, the main buffer should adopt the current cursor position and style. + // This is the equal and opposite effect of what we did in UseAlternateScreenBuffer and matches xterm. + // + // We have to do this after the call to UserResize() to ensure that the TextBuffer sizes match up. + // Otherwise the cursor position may be temporarily out of bounds and some code may choke on that. + { + const auto& altCursor = altBuffer->GetCursor(); + auto& mainCursor = _mainBuffer->GetCursor(); + + mainCursor.SetStyle(altCursor.GetSize(), altCursor.GetType()); + mainCursor.SetIsVisible(altCursor.IsVisible()); + mainCursor.SetBlinkingAllowed(altCursor.IsBlinkingAllowed()); + + auto tgtCursorPos = altCursor.GetPosition(); + tgtCursorPos.y += _mutableViewport.Top(); + mainCursor.SetPosition(tgtCursorPos); + } + // update all the hyperlinks on the screen _updateUrlDetection(); @@ -281,11 +285,7 @@ void Terminal::UseMainScreenBuffer() _NotifyScrollEvent(); // redraw the screen - try - { - _activeBuffer().TriggerRedrawAll(); - } - CATCH_LOG(); + _activeBuffer().TriggerRedrawAll(); } // NOTE: This is the version of AddMark that comes from VT From 80e453d981d0b7eff6544022843415bd91c2048f Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Tue, 19 Sep 2023 16:47:12 +0200 Subject: [PATCH 09/11] Address feedback --- src/buffer/out/Row.cpp | 16 ++++++++++------ src/buffer/out/Row.hpp | 3 +-- src/buffer/out/textBuffer.cpp | 10 +++++++--- 3 files changed, 18 insertions(+), 11 deletions(-) diff --git a/src/buffer/out/Row.cpp b/src/buffer/out/Row.cpp index 539b1742cb4..4a098bf9ea1 100644 --- a/src/buffer/out/Row.cpp +++ b/src/buffer/out/Row.cpp @@ -152,11 +152,15 @@ til::CoordType CharToColumnMapper::GetTrailingColumnAt(ptrdiff_t offset) noexcep return col; } +// If given a pointer pointer inside the ROW's text buffer, this function will return the corresponding column. +// This function in particular returns the glyph's first column. til::CoordType CharToColumnMapper::GetLeadingColumnAt(const wchar_t* str) noexcept { return GetLeadingColumnAt(str - _chars); } +// If given a pointer pointer inside the ROW's text buffer, this function will return the corresponding column. +// This function in particular returns the glyph's last column (this matters for wide glyphs). til::CoordType CharToColumnMapper::GetTrailingColumnAt(const wchar_t* str) noexcept { return GetTrailingColumnAt(str - _chars); @@ -390,16 +394,16 @@ til::CoordType ROW::NavigateToNext(til::CoordType column) const noexcept return _adjustForward(_clampedColumnInclusive(column + 1)); } -til::CoordType ROW::AdjustBackward(til::CoordType column) const noexcept +// Returns the starting column of the glyph at the given column. +// In other words, if you have 3 wide glyphs +// AA BB CC +// 01 23 45 <-- column +// then `AdjustToGlyphStart(3)` returns 2. +til::CoordType ROW::AdjustToGlyphStart(til::CoordType column) const noexcept { return _adjustBackward(_clampedColumn(column)); } -til::CoordType ROW::AdjustForward(til::CoordType column) const noexcept -{ - return _adjustForward(_clampedColumnInclusive(column)); -} - // Routine Description: // - clears char data in column in row // Arguments: diff --git a/src/buffer/out/Row.hpp b/src/buffer/out/Row.hpp index 5261bc7809b..29f7fe18b8c 100644 --- a/src/buffer/out/Row.hpp +++ b/src/buffer/out/Row.hpp @@ -136,8 +136,7 @@ class ROW final til::CoordType NavigateToPrevious(til::CoordType column) const noexcept; til::CoordType NavigateToNext(til::CoordType column) const noexcept; - til::CoordType AdjustBackward(til::CoordType column) const noexcept; - til::CoordType AdjustForward(til::CoordType column) const noexcept; + til::CoordType AdjustToGlyphStart(til::CoordType column) const noexcept; void ClearCell(til::CoordType column); OutputCellIterator WriteCells(OutputCellIterator it, til::CoordType columnBegin, std::optional wrap = std::nullopt, std::optional limitRight = std::nullopt); diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index 0e1f2eaf9ff..2182b6f5ba4 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -2469,7 +2469,7 @@ void TextBuffer::Reflow(TextBuffer& oldBuffer, TextBuffer& newBuffer, const View if (oldY == oldCursorPos.y) { - newCursorPos = { newRow.AdjustBackward(oldCursorPos.x), newY }; + newCursorPos = { newRow.AdjustToGlyphStart(oldCursorPos.x), newY }; } if (oldY >= mutableViewportTop) { @@ -2550,9 +2550,13 @@ void TextBuffer::Reflow(TextBuffer& oldBuffer, TextBuffer& newBuffer, const View if (oldY == oldCursorPos.y && oldCursorPos.x >= oldX) { - // In theory AdjustBackward ensures we don't put the cursor on a trailing wide glyph. + // In theory AdjustToGlyphStart ensures we don't put the cursor on a trailing wide glyph. // In practice I don't think that this can possibly happen. Better safe than sorry. - newCursorPos = { newRow.AdjustBackward(oldCursorPos.x - oldX + newX), newY }; + newCursorPos = { newRow.AdjustToGlyphStart(oldCursorPos.x - oldX + newX), newY }; + // Imagine that the terminal has no scrollback (= TextBuffer is as tall as the viewport). + // If we make the window significantly less taller (for instance 1/3rd), we need to ensure + // we don't continue copying so much text that it overwrites the row the cursor was on. + // In other words, newYLimit ensures that the row with the cursor remains visible. newYLimit = newY + newHeight; } if (oldY >= mutableViewportTop) From a8ef491ef074cdb5dcbecb0b8d54597cebef6aeb Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Tue, 19 Sep 2023 16:52:43 +0200 Subject: [PATCH 10/11] Spel --- .github/actions/spelling/expect/expect.txt | 7 ------- src/buffer/out/Row.cpp | 4 ++-- 2 files changed, 2 insertions(+), 9 deletions(-) diff --git a/.github/actions/spelling/expect/expect.txt b/.github/actions/spelling/expect/expect.txt index d357c9cebe1..7505061fab2 100644 --- a/.github/actions/spelling/expect/expect.txt +++ b/.github/actions/spelling/expect/expect.txt @@ -749,7 +749,6 @@ gfx GGI GHIJK GHIJKL -GHIJKLM gitfilters gitmodules gle @@ -1000,7 +999,6 @@ listptrsize lld llx LMENU -LMNOP lnk lnkd lnkfile @@ -1223,7 +1221,6 @@ nonspace NOOWNERZORDER Nop NOPAINT -NOPQRST noprofile NOREDRAW NOREMOVE @@ -1498,7 +1495,6 @@ pwsz pythonw Qaabbcc qos -QRSTU QUERYOPEN QUESTIONMARK quickedit @@ -1861,7 +1857,6 @@ testname TESTNULL testpass testpasses -testtimeout TEXCOORD texel TExpected @@ -2019,7 +2014,6 @@ utext UText UTEXT utr -UVWX UVWXY UVWXYZ uwa @@ -2078,7 +2072,6 @@ VTRGBTo vtseq vtterm vttest -VWX waitable WANSUNG WANTARROWS diff --git a/src/buffer/out/Row.cpp b/src/buffer/out/Row.cpp index 4a098bf9ea1..4036b991d81 100644 --- a/src/buffer/out/Row.cpp +++ b/src/buffer/out/Row.cpp @@ -152,14 +152,14 @@ til::CoordType CharToColumnMapper::GetTrailingColumnAt(ptrdiff_t offset) noexcep return col; } -// If given a pointer pointer inside the ROW's text buffer, this function will return the corresponding column. +// If given a pointer inside the ROW's text buffer, this function will return the corresponding column. // This function in particular returns the glyph's first column. til::CoordType CharToColumnMapper::GetLeadingColumnAt(const wchar_t* str) noexcept { return GetLeadingColumnAt(str - _chars); } -// If given a pointer pointer inside the ROW's text buffer, this function will return the corresponding column. +// If given a pointer inside the ROW's text buffer, this function will return the corresponding column. // This function in particular returns the glyph's last column (this matters for wide glyphs). til::CoordType CharToColumnMapper::GetTrailingColumnAt(const wchar_t* str) noexcept { From fd4e89b7201ad18f7f176e81b2f04d59d6762b8c Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Thu, 21 Sep 2023 14:09:51 +0200 Subject: [PATCH 11/11] Improve newYLimit comment --- src/buffer/out/textBuffer.cpp | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index 2182b6f5ba4..26c8152ea8e 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -2553,10 +2553,12 @@ void TextBuffer::Reflow(TextBuffer& oldBuffer, TextBuffer& newBuffer, const View // In theory AdjustToGlyphStart ensures we don't put the cursor on a trailing wide glyph. // In practice I don't think that this can possibly happen. Better safe than sorry. newCursorPos = { newRow.AdjustToGlyphStart(oldCursorPos.x - oldX + newX), newY }; - // Imagine that the terminal has no scrollback (= TextBuffer is as tall as the viewport). - // If we make the window significantly less taller (for instance 1/3rd), we need to ensure - // we don't continue copying so much text that it overwrites the row the cursor was on. - // In other words, newYLimit ensures that the row with the cursor remains visible. + // If there's so much text past the old cursor position that it doesn't fit into new buffer, + // then the new cursor position will be "lost", because it's overwritten by unrelated text. + // We have two choices how can handle this: + // * If the new cursor is at an y < 0, just put the cursor at (0,0) + // * Stop writing into the new buffer before we overwrite the new cursor position + // This implements the second option. There's no fundamental reason why this is better. newYLimit = newY + newHeight; } if (oldY >= mutableViewportTop)