-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reimplement TextBuffer::Reflow #15701
Conversation
26548fb
to
a009e0d
Compare
a009e0d
to
3d463f1
Compare
I fixed all the bugs that were obvious enough that I could find them. It'd be super helpful if anyone else could give this PR/branch a look. But it might be easiest to just put it into our test builds. |
}; | ||
CopyTextFrom(state); | ||
|
||
TransferAttributes(source.Attributes(), _columnCount); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change ensures that we take the line rendition into account when determining the capacity of the row in CopyTextFrom
.
@@ -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)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will allow us to "navigate" the cursor into the past-the-end column.
// 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 }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a h.charsConsumed == chars.size()
check at the end of this function. That check only works if the chars.size()
is correct, which this change now fixes. The string will now contain the exact amount of text we're allowed to copy.
@@ -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<const Microsoft::Console::Types::Viewport> viewOptional = std::nullopt) const; | |||
til::point GetLastNonSpaceCharacter(const Microsoft::Console::Types::Viewport* viewOptional = nullptr) const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I like std::optional
, we shouldn't optimally pass Viewport
by value. Also, this pointer-style fits much better with Reflow()
which has 2 optional parameters one of which is an explicit optional reference and why write
std::optional<std::reference_wrapper<PositionInformation>>
if you can also
PositionInformation*
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lmao
_altBufferSize = viewportSize; | ||
_altBuffer->TriggerRedrawAll(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was missing? I think that's a bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'd believe it.
_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(); }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There'll be no need for DeferDrawing
anymore, since the reflow/resize functions don't use the cursor.
} | ||
|
||
int Terminal::_VisibleEndIndex() const noexcept | ||
{ | ||
return _inAltBuffer() ? ViewEndIndex() : | ||
std::max(0, ViewEndIndex() - _scrollOffset); | ||
return _inAltBuffer() ? _altBufferSize.height - 1 : std::max(0, _mutableViewport.BottomInclusive() - _scrollOffset); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if I should revert these changes... I felt like the extra _inAltBuffer()
check in the nested calls is redundant and inlining the code would make it clearer, but realistically this doesn't make any difference, right? Idk...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jeez, yeah, i don't get why these were the way they are. go for it.
const auto lastNonSpaceRow = newTextBuffer->GetLastNonSpaceCharacter().y; | ||
const auto estimatedBottom = cursorRow + cursorDistanceFromBottom; | ||
const auto viewportBottom = _viewport.Height() - 1; | ||
_virtualBottom = std::max({ lastNonSpaceRow, estimatedBottom, viewportBottom }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Indentation changes only. -> Suppress whitespace changes.)
const auto cursorRow = GetCursor().GetPosition().y; | ||
const auto copyableRows = std::min<til::CoordType>(_height, newSize.height); | ||
til::CoordType srcRow = 0; | ||
til::CoordType dstRow = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Indentation changes only. -> Suppress whitespace changes.)
newRow.SetLineRendition(row.GetLineRendition()); | ||
} | ||
const auto oldCursorPos = oldCursor.GetPosition(); | ||
til::point newCursorPos; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function may be easier to review without a diff.
} | ||
|
||
int Terminal::_VisibleEndIndex() const noexcept | ||
{ | ||
return _inAltBuffer() ? ViewEndIndex() : | ||
std::max(0, ViewEndIndex() - _scrollOffset); | ||
return _inAltBuffer() ? _altBufferSize.height - 1 : std::max(0, _mutableViewport.BottomInclusive() - _scrollOffset); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jeez, yeah, i don't get why these were the way they are. go for it.
Well, it sure doesn't! All the reflow tests are failing :D Some of them have been marked buggy, but I tried to capture as much existing behavior as i could when i wrote them. |
if (gci.IsInVtIoMode()) | ||
{ | ||
gci.GetVtIo()->BeginResize(); | ||
} | ||
auto endResize = wil::scope_exit([&] { | ||
if (gci.IsInVtIoMode()) | ||
{ | ||
gci.GetVtIo()->EndResize(); | ||
} | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While working on another PR I realized that this special behavior is only here for TextBuffer::Reflow
, because that one used to use NewlineCursor
which calls IncrementCircularBuffer
which calls Renderer::TriggerFlush
which calls VtEngine::InvalidateFlush
which then set pForcePaint = false
if BeginResize
was called.
In short: This block prevents TextBuffer::Reflow
from setting pForcePaint
to true
.
...and the new code doesn't call IncrementCircularBuffer
anymore so that's moot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(just trying to post a comment I think GH lost)
src/buffer/out/Row.cpp
Outdated
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)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These might warrant a comment as to why they're different than the Navigate*
versions. If I'm reading this right:
- the
Navigate
ones act like "move one cell forward/backward, then find the end/start of the glyph in that cell" - while the
Adjust
ones are just "find the end/start of the glyph in THIS cell"
correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep.
I've removed AdjustForward
(it's unused) and renamed AdjustBackward
to AdjustToGlyphStart
and added a comment. That should hopefully make it easier to read the code.
@@ -867,6 +883,16 @@ til::CoordType ROW::MeasureLeft() const noexcept | |||
|
|||
til::CoordType ROW::MeasureRight() const noexcept | |||
{ | |||
if (_wrapForced) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
brilliant
src/buffer/out/textBuffer.cpp
Outdated
if (oldY == oldCursorPos.y && oldCursorPos.x >= oldX) | ||
{ | ||
cNewCursorPos = newCursor.GetPosition(); | ||
fFoundCursorPos = true; | ||
// 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; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the old cursor was on this row, and the old cursor was beyond this character,
- the new cursor is on this row, adjusted for how many glyphs are in this new row
- don't keep copying beyond this new row + the height of the new buffer
that feels weird? I don't think I understand the use of `newYLimit`` here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be sure:
if the old cursor was on this row, and the old cursor was beyond
this characterthe start of the text we've copied
...because this implies that we've probably just copied the text where the old cursor was on. I've written the if
condition that way because it then matches the oldCursorPos.x - oldX
. It allows you (kind of) to see how oldCursorPos.x >= oldX
ensures that oldCursorPos.x - oldX
doesn't go negative and how this results in the new cursor always being at some X position that is >= 0.
that feels weird? I don't think I understand the use of
newYLimit
here
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.
I've added that as a comment to this line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if like,
- the buffer starts 30 tall
- the cursor is on line 5
- we resize to 20 tall
Does that mean we'd only end up copying the first 25 rows? (newYLimit = 5 + 20
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(this is the last question I have before I s/o)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, exactly that. I did it for this test specifically:
terminal/src/buffer/out/ut_textbuffer/ReflowTests.cpp
Lines 294 to 315 in fc4a37e
TestBuffer{ | |
{ 6, 5 }, | |
{ | |
{ L"ABCDEF", false }, | |
{ L"$ ", false }, | |
{ L"GHIJKL", false }, | |
{ L"MNOPQR", false }, | |
{ L"STUVWX", false }, | |
}, | |
{ 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 }, | |
}, | |
{ 1, 1 } // [BUG] cursor moves to 1,1 instead of sticking with the $ | |
}, |
$
is the cursor. false
in each line means that it has an explicit newline (true
= force wrap).
It has a lot of comments about bugs in the old implementation, which is why the entire text past the cursor fits into the new buffer. In the new, fixed implementation the false
= explicit newlines are correctly handled and so it doesn't fit anymore:
terminal/src/buffer/out/ut_textbuffer/ReflowTests.cpp
Lines 294 to 315 in 6374aec
TestBuffer{ | |
{ 6, 5 }, | |
{ | |
{ L"ABCDEF", false }, | |
{ L"$ ", false }, | |
{ L"GHIJKL", false }, | |
{ L"MNOPQR", false }, | |
{ L"STUVWX", false }, | |
}, | |
{ 0, 1 }, // cursor on $ | |
}, | |
TestBuffer{ | |
{ 5, 5 }, // reduce width by 1 | |
{ | |
{ L"$ ", false }, | |
{ L"GHIJK", true }, | |
{ L"L ", false }, | |
{ L"MNOPQ", true }, | |
{ L"R ", false }, | |
}, | |
{ 0, 0 }, | |
}, |
Basically I had to make a choice over whether I:
- continue copying until all text in the old buffer has been consumed.
If the new cursor position is now somewhere at a negative Y, we just put it at (0,0). - stop copying at some point to prevent the cursor from being lost.
I chose the latter. What do you think we should do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
continue copying until all text in the old buffer has been consumed.
If the new cursor position is now somewhere at a negative Y, we just put it at (0,0).
Probably that one, right? Like, the priority should be to keep the newest text, right?
(/cc @DHowett since this is currently a 1.19 candidate. I'm okay to say merge as is and iterate in the first hotfix since
- we're out of time and I'm about to log out for the day
- this is a RARE edge case
- this is more goodness than badness)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an interesting case. I'm honestly not sure how to handle it -- we should look at how other terminals that do reflow handle it?
I think I'm inclined to s/o on this, though I really don't think I get the Though, there are a TON of reflow roundtrip tests, so if those still pass, then I'm inclined to say this works. |
This comment has been minimized.
This comment has been minimized.
I am taking this as a sign-off for 1.19, plus fixes. Mike, thanks for reviewing. Sorry if I override you and am incorrect. 😄 |
Subjectively speaking, this commit makes 3 improvements: * Most importantly, it now would work with arbitrary Unicode text. (No more `IsGlyphFullWidth` or DBCS handling during reflow.) * Due to the simpler implementation it hopefully makes review of future changes and maintenance simpler. (~3x less LOC.) * It improves perf. by 1-2 orders of magnitude. (At 120x9001 with a full buffer I get 60ms -> 2ms.) Unfortunately, I'm not confident that the new code replicates the old code exactly, because I failed to understand it. During development I simply tried to match its behavior with what I think reflow should do. Closes #797 Closes #3088 Closes #4968 Closes #6546 Closes #6901 Closes #15964 Closes MSFT:19446208 Related to #5800 and #8000 ## Validation Steps Performed * Unit tests ✅ * Feature tests ✅ * Reflow with a scrollback ✅ * Reflowing the cursor cell causes a forced line-wrap ✅ (Even at the end of the buffer. ✅) * `color 8f` and reflowing retains the background color ✅ * Enter alt buffer, Resize window, Exit alt buffer ✅ (cherry picked from commit 7474839) Service-Card-Id: 90642727 Service-Version: 1.19
#15541 changed `AdaptDispatch::_FillRect` which caused it to not affect the `ROW::_wrapForced` flag anymore. This change in behavior was not noticeable as `TextBuffer::GetLastNonSpaceCharacter` had a bug where rows of only whitespace text would always be treated as empty. This would then affect `AdaptDispatch::_EraseAll` to accidentally correctly guess the last row with text despite the `_FillRect` change. #15701 then fixed `GetLastNonSpaceCharacter` indirectly by fixing `ROW::MeasureRight` which now made the previous change apparent. `_EraseAll` would now guess the last row of text incorrectly, because it would find the rows that `_FillRect` cleared but still had `_wrapForced` set to `true`. This PR fixes the issue by replacing the `_FillRect` usage to clear rows with direct calls to `ROW::Reset()`. In the future this could be extended by also `MEM_DECOMMIT`ing the now unused underlying memory. Closes #16603 ## Validation Steps Performed * Enter WSL and resize the window to <40 columns * Execute ```sh cd /bin ls -la printf "\e[3J" ls -la printf "\e[3J" printf "\e[2J" ``` * Only one viewport-height-many lines of whitespace exist between the current prompt line and the previous scrollback contents ✅
#15541 changed `AdaptDispatch::_FillRect` which caused it to not affect the `ROW::_wrapForced` flag anymore. This change in behavior was not noticeable as `TextBuffer::GetLastNonSpaceCharacter` had a bug where rows of only whitespace text would always be treated as empty. This would then affect `AdaptDispatch::_EraseAll` to accidentally correctly guess the last row with text despite the `_FillRect` change. #15701 then fixed `GetLastNonSpaceCharacter` indirectly by fixing `ROW::MeasureRight` which now made the previous change apparent. `_EraseAll` would now guess the last row of text incorrectly, because it would find the rows that `_FillRect` cleared but still had `_wrapForced` set to `true`. This PR fixes the issue by replacing the `_FillRect` usage to clear rows with direct calls to `ROW::Reset()`. In the future this could be extended by also `MEM_DECOMMIT`ing the now unused underlying memory. Closes #16603 ## Validation Steps Performed * Enter WSL and resize the window to <40 columns * Execute ```sh cd /bin ls -la printf "\e[3J" ls -la printf "\e[3J" printf "\e[2J" ``` * Only one viewport-height-many lines of whitespace exist between the current prompt line and the previous scrollback contents ✅ (cherry picked from commit 5f71cf3) Service-Card-Id: 91707937 Service-Version: 1.19
STL iterators have a significant overhead. This improves performance of `GetLastNonSpaceColumn` by >100x (it's too large to measure), and reflow by ~15x in debug builds. This makes text reflow in debug builds today ~10x faster than it used to be in release builds before the large rewrites in #15701 and #13626.
Subjectively speaking, this commit makes 3 improvements:
(No more
IsGlyphFullWidth
or DBCS handling during reflow.)future changes and maintenance simpler. (~3x less LOC.)
(At 120x9001 with a full buffer I get 60ms -> 2ms.)
Unfortunately, I'm not confident that the new code replicates the old
code exactly, because I failed to understand it. During development
I simply tried to match its behavior with what I think reflow should do.
Closes #797
Closes #3088
Closes #4968
Closes #6546
Closes #6901
Closes #15964
Closes MSFT:19446208
Related to #5800 and #8000
Validation Steps Performed
(Even at the end of the buffer. ✅)
color 8f
and reflowing retains the background color ✅