Manually copy trailing attributes on a resize by zadjii-msft · Pull Request #12637 · microsoft/terminal

zadjii-msft · 2022-03-07T23:02:48Z

THE WHITE WHALE

This is a fairly naive fix for this bug. It's not terribly performant,
but neither is resize in the first place.

When the buffer gets resized, typically we only copy the text up to the
MeasureRight point, the last printable char in the row. Then we'd just
use the last char's attributes to fill the remainder of the row.

Instead, this PR changes how reflow behaves when it gets to the end of
the row. After we finish copying text, then manually walk through the
attributes at the end of the row, and copy them over. This ensures that
cells that just have a colored space in them get copied into the new
buffer as well, and we don't just blat the last character's attributes
into the rest of the row. We'll do a similar thing once we get to the
last printable char in the buffer, copying the remaining attributes.

This could DEFINITELY be more performant. I think this current
implementation walks the attrs on every cell, then appends the new
attrs to the new ATTR_ROW. That could be optimized by just using the
actual iterator. The copy after the last printable char bit is also
especially bad in this regard. That could likely be a blind copy - I
just wanted to get this into the world.

Finally, we now copy the final attributes to the correct buffer: the new
one. We used to copy them to the old buffer, which we were about to
destroy.

Validation

I'll add more gifs in the morning, not enough time to finish spinning a
release Terminal build with this tonight.

Closes #32 🎉🎉🎉🎉🎉🎉🎉🎉🎉
Closes #12567

bunch of todos. It also totally takes care of #12567 as well

DHowett · 2022-03-09T23:53:36Z

What if we resize down to the righthand edge of a color and then back to the left. Does it expand that color to the edge forever?

zadjii-msft · 2022-03-10T12:31:18Z

What if we resize down to the righthand edge of a color and then back to the left. Does it expand that color to the edge forever?

Yea, that does. So that's the same as it was before:

That could probably be fixed by the proposed "'bookend' with default attrs at the end of the attrs from the pre-resize buffer" idea, which I haven't tested. In that case, resizing down to the white w above would work as expected, then resizing back out would have default attrs.

j4james · 2022-03-10T15:09:39Z

I think I quite like it the way you have it now actually. This way if you've cleared the screen with a different background color, it should retain that color for the full width when you resize (or so I assume). But I don't think the default bookend idea would be bad either - both have their pros and cons.

It's also worth noting that Gnome Terminal seems to treat cells with non-default attributes as wrappable content (even when they're just spaces). But that behaves quite weirdly when you've cleared the screen with a non-default color. I'm not a fan of that approach myself, but I don't think it's unreasonable either.

lhecker · 2022-03-10T15:17:19Z

BTW regarding performance: This PR increases the CPU cost of TextBuffer::Reflow by about 18% if my 120x9001 buffer is full with enwik8. About half of the cost is due to InsertCharacter however, which is a bit silly anyways and can be improved by doing batch-insertions. I'm not concerned about this change from that POV.

DHowett · 2022-03-10T19:03:48Z

@msftbot make sure @miniksa signs off

ghost · 2022-03-10T19:03:53Z

Hello @DHowett!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

I'll only merge this pull request if it's approved by @miniksa

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

DHowett · 2022-03-10T19:05:10Z

That could be optimized by just using the actual iterator.

How expensive would you estimate doing it this way would be? Code-time-wise?

zadjii-msft · 2022-03-10T20:21:31Z

How expensive would you estimate doing it this way would be? Code-time-wise?

I could probably spend half a day sanity checking if that would take half a day or many days and bail if it's too hard. After {the esb bug, the nav view one, the win11 backports, the ppt} in the queue.

zadjii-msft · 2022-03-16T14:34:37Z

How expensive would you estimate doing it this way would be? Code-time-wise?

I could probably spend half a day sanity checking if that would take half a day or many days and bail if it's too hard.

I did this - thank you for bringing 61b0697 into my life. That's so so SO much faster.

build/pipelines/templates/build-console-steps.yml

DHowett

I'll unmark after some questions :)

DHowett · 2022-03-16T15:36:13Z

src/buffer/out/textBuffer.cpp

-                const auto glyph = row.GetCharRow().GlyphAt(iOldCol);
-                const auto dbcsAttr = row.GetCharRow().DbcsAttrAt(iOldCol);
-                const auto textAttr = row.GetAttrRow().GetAttrByColumn(iOldCol);
+                const auto glyph = chars->Char();


Hopefully this doesn't cause any trouble with DBCS. It might be worth just smoke testing that -- fill a buffer with U+30AB KATAKANA LETTER KA (カ) and size it around to make sure it reflows properly.

DHowett · 2022-03-16T15:37:30Z

src/buffer/out/textBuffer.cpp

            if (iRight < cOldColsTotal && !row.WasWrapForced())
            {
-                if (iRight == cOldCursorPos.X && iOldRow == cOldCursorPos.Y)
+                if (!fFoundCursorPos && (iRight == cOldCursorPos.X && iOldRow == cOldCursorPos.Y))


Unrelated condition change -- scary? I guess as long as the reflow tests still pass...

It actually did end up being related. Because we're now also copying the trailing attrs, there was a case where we'd copy the attrs to the end of the row, but then set the new cursor position to the last column of the row the cursor was on.

(it may not be relevant anymore, but it's not more wrong.)

DHowett · 2022-03-16T15:37:41Z

src/buffer/out/textBuffer.cpp

+
+    // Finish copying buffer attributes to remaining rows below the last
+    // printable character. This is to fix the `color 2f` scenario, where you
+    // change the buffer colors then resize and everything below the past


Suggested change

// change the buffer colors then resize and everything below the past

// change the buffer colors then resize and everything below the last

this comment suggests we'll have to inbox this change to fix COLOR

yea, we would.

DHowett · 2022-03-16T15:40:45Z

src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp

            {
                auto iter = TestUtils::VerifyLineContains(tb, { 0, row }, L'#', greenAttrs, 1u);
-                TestUtils::VerifyLineContains(iter, L' ', (afterResize ? greenAttrs : actualDefaultAttrs), static_cast<size_t>(width - 1));
+                TestUtils::VerifyLineContains(iter, L' ', (actualDefaultAttrs), static_cast<size_t>(width - 1));


Confirming: this test was coded around the bad behavior, right?

DHowett · 2022-03-16T15:41:04Z

src/host/screenInfo.cpp

        LOG_IF_FAILED(SetViewportOrigin(false, coordCursorHeightDiff, true));

-        _textBuffer->SetCurrentAttributes(oldPrimaryAttributes);
+        newTextBuffer->SetCurrentAttributes(oldPrimaryAttributes);


HUH. This seems like an unrelated but important bug fix

yea this was mentioned here: #12567 (comment) and in the previous reply

DHowett · 2022-03-16T15:41:37Z

src/host/ut_host/ScreenBufferTests.cpp

+    // there are also 3 B's wrapped in spaces, and finally 5 trailing spaces.
+    Log::Comment(L"========== Checking the buffer state (after) ==========");
+    verifyBuffer(si.GetTextBuffer(), til::rect{ si.GetViewport().ToInclusive() }, false);
+}


Do we need a test for the "copied the attributes after the last printable character" case?

meh, I could write one

I'll sign, but I would like to see that ^

…pt-2

DHowett · 2022-03-17T22:24:45Z

Test harness is crashing -- not sure if it is this PR's fault

Error: TAEF: [HRESULT 0x800706BE] A failure occurred while running a test operation: 'SelectionInputTests::ClassSetup'. (The process hosting the test code was unexpectedly terminated with exit code 0x0000DEAD while invoking a test operation.)

DHowett · 2022-03-18T22:35:10Z

Test failure is not from this PR, and is addressed in #17274

miniksa · 2022-03-18T23:24:01Z

Fix the test, of course. But otherwise, I'm happy.

…pt-2

DHowett · 2022-03-21T16:29:18Z

Image from PR body

ghost · 2022-03-21T16:37:44Z

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

## THE WHITE WHALE This is a fairly naive fix for this bug. It's not terribly performant, but neither is resize in the first place. When the buffer gets resized, typically we only copy the text up to the `MeasureRight` point, the last printable char in the row. Then we'd just use the last char's attributes to fill the remainder of the row. Instead, this PR changes how reflow behaves when it gets to the end of the row. After we finish copying text, then manually walk through the attributes at the end of the row, and copy them over. This ensures that cells that just have a colored space in them get copied into the new buffer as well, and we don't just blat the last character's attributes into the rest of the row. We'll do a similar thing once we get to the last printable char in the buffer, copying the remaining attributes. This could DEFINITELY be more performant. I think this current implementation walks the attrs _on every cell_, then appends the new attrs to the new ATTR_ROW. That could be optimized by just using the actual iterator. The copy after the last printable char bit is also especially bad in this regard. That could likely be a blind copy - I just wanted to get this into the world. Finally, we now copy the final attributes to the correct buffer: the new one. We used to copy them to the _old_ buffer, which we were about to destroy. ## Validation I'll add more gifs in the morning, not enough time to finish spinning a release Terminal build with this tonight. Closes #32 🎉🎉🎉🎉🎉🎉🎉🎉🎉 Closes #12567 (cherry picked from commit 855e136)

ghost · 2022-04-19T19:56:44Z

🎉Windows Terminal v1.12.1098 has been released which incorporates this pull request.:tada:

Handy links:

ghost · 2022-04-19T19:58:59Z

🎉Windows Terminal Preview v1.13.1098 has been released which incorporates this pull request.:tada:

Handy links:

## THE WHITE WHALE This is a fairly naive fix for this bug. It's not terribly performant, but neither is resize in the first place. When the buffer gets resized, typically we only copy the text up to the `MeasureRight` point, the last printable char in the row. Then we'd just use the last char's attributes to fill the remainder of the row. Instead, this PR changes how reflow behaves when it gets to the end of the row. After we finish copying text, then manually walk through the attributes at the end of the row, and copy them over. This ensures that cells that just have a colored space in them get copied into the new buffer as well, and we don't just blat the last character's attributes into the rest of the row. We'll do a similar thing once we get to the last printable char in the buffer, copying the remaining attributes. This could DEFINITELY be more performant. I think this current implementation walks the attrs _on every cell_, then appends the new attrs to the new ATTR_ROW. That could be optimized by just using the actual iterator. The copy after the last printable char bit is also especially bad in this regard. That could likely be a blind copy - I just wanted to get this into the world. Finally, we now copy the final attributes to the correct buffer: the new one. We used to copy them to the _old_ buffer, which we were about to destroy. ## Validation I'll add more gifs in the morning, not enough time to finish spinning a release Terminal build with this tonight. Closes #32 🎉🎉🎉🎉🎉🎉🎉🎉🎉 Closes #12567 (cherry picked from commit 855e136)

zadjii-msft added 10 commits March 2, 2022 12:55

I think this is a fix for #32

1f6210d

bunch of todos. It also totally takes care of #12567 as well

another comment

966fa7b

cleanup, an edge case too

65638cc

wat

4388502

start working on a test

dba1e98

okay I think the test is ready

4e37693

Fix persisting attrs below the last line of the buffer too

37837ca

annother test

3e3d345

another test

de062ab

account for line rendition too

6843fc7

This comment was marked as resolved.

Sign in to view

lhecker approved these changes Mar 8, 2022

View reviewed changes

spel

e62fa78

DHowett approved these changes Mar 10, 2022

View reviewed changes

ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Mar 10, 2022

zadjii-msft commented Mar 16, 2022

View reviewed changes

build/pipelines/templates/build-console-steps.yml Show resolved Hide resolved

Update build/pipelines/templates/build-console-steps.yml

07e854f

DHowett requested changes Mar 16, 2022

View reviewed changes

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Mar 16, 2022

zadjii-msft added 2 commits March 17, 2022 14:32

Merge remote-tracking branch 'origin/main' into dev/migrie/b/32-attem…

8fa3a79

…pt-2

typo

447e309

DHowett approved these changes Mar 17, 2022

View reviewed changes

DHowett mentioned this pull request Mar 18, 2022

Clear out g.pRender on cleanup so further tests don't explode #12724

Merged

miniksa approved these changes Mar 18, 2022

View reviewed changes

Merge remote-tracking branch 'origin/main' into dev/migrie/b/32-attem…

58e6b9a

…pt-2

DHowett added AutoMerge Marked for automatic merge by the bot when requirements are met and removed AutoMerge Marked for automatic merge by the bot when requirements are met labels Mar 21, 2022

ghost merged commit 855e136 into main Mar 21, 2022

ghost deleted the dev/migrie/b/32-attempt-2 branch March 21, 2022 17:02

zadjii-msft mentioned this pull request Mar 31, 2022

Color goes default while printring tree command #12797

Closed

This was referenced Apr 19, 2022

Full-screen COLOR command no longer works right when resizing conhost and Terminal #12567

Closed

Console doesn't handle colored regions when reflowed #32

Closed

o-sdn-o mentioned this pull request Jan 16, 2026

Rendering issue with PowerShell Write-Host -BackgroundColor #19736

Open

This pull request was closed.

	// change the buffer colors then resize and everything below the past
	// change the buffer colors then resize and everything below the last

Conversation

zadjii-msft commented Mar 7, 2022 • edited by DHowett Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

THE WHITE WHALE

Validation

Uh oh!

This comment was marked as resolved.

DHowett commented Mar 9, 2022

Uh oh!

zadjii-msft commented Mar 10, 2022

Uh oh!

j4james commented Mar 10, 2022

Uh oh!

lhecker commented Mar 10, 2022

Uh oh!

DHowett commented Mar 10, 2022

Uh oh!

ghost commented Mar 10, 2022

Uh oh!

DHowett commented Mar 10, 2022

Uh oh!

zadjii-msft commented Mar 10, 2022

Uh oh!

zadjii-msft commented Mar 16, 2022

Uh oh!

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DHowett commented Mar 17, 2022

Uh oh!

DHowett commented Mar 18, 2022

Uh oh!

miniksa commented Mar 18, 2022

Uh oh!

DHowett commented Mar 21, 2022

Uh oh!

ghost commented Mar 21, 2022

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

ghost commented Apr 19, 2022

Uh oh!

ghost commented Apr 19, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

zadjii-msft commented Mar 7, 2022 •

edited by DHowett

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.