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

[zadjii-msft]

Open cmd window, set the color to 2f (green background, white text); observe the color change
Resize the window, the color changes back to black, with the "text" portion of the window still left in 2f.
This behavior can be repeated every time a resize happens.

This is the external half of MSFT:34761400

Yep, okay this is weird in conhost now. I bet this was caused by fixing #3848.

Since this one's a partner bug, we've gotta get this sorted out in this OS release.

[zadjii-msft]

#5792

yes, yes it does

[zadjii-msft]

for linking: #3848

Options

• Pretty sure we can't gate this behavior on being in VT mode, b/c I think cmd.exe itself runs in VT mode (just, disables it for child processes). Check anyways?
• We could target the fix from Initialize the text buffer with the default attributes on a resize #5792 to only happen when exiting the alt buffer. That wouldn't fix Weird blue selection after closing neovim (WSL) #3848 (comment) though
• Something more clever. Is this sorta like Console doesn't handle colored regions when reflowed #32? Like, maybe we need to be able to see that the empty spaces at the end of the line were filled with attributes that aren't the default attributes and reflow them as well

[zadjii-msft]

So, this is totally low key #32. It was just never a problem before, because when you'd do color 2f before, we'd start the buffer off with the attributes set to 2f, so you wouldn't notice that the colors didn't get extended to the end of the buffer. Like, if you did

wsl printf "\x1b[2Jfoo\x1b[5b\nbar\x1b[5b\nbaz\x1b[5b"

color 2f

wsl printf "\x1b[s\x1b[2;2H\x1b[41m[ FOO ]\x1b[u"

<resize window>

I bet it would have done the same thing with the red FOO line in conhost before this change. (need to find an old conhost to confirm)

That being said, people love their color commands so I guess we gotta sort this out now.

• ❌ vb_release:
• ❌ rs5_release:
• ✅ rs4_release:

Look, this has been busted since RS5. That's like, since October 2018. I wasn't the murderer (this time)!

[j4james]

Yeah, I was going to say I thought this was probably the same thing as #32. Will be great if we can get that fixed.

[zadjii-msft]

Will be great if we can get that fixed.

it's my white whale 😆

[zadjii-msft]

Yo @j4james since you've probably spent more time in the buffer than anyone else recently, would you mind taking a quick look at https://github.com/microsoft/terminal/compare/dev/migrie/b/32-attempt-2 ? I feel like I have to be missing something obvious. Like, this bug (#32) dates back to Windows 10, there's no way fixing it should be that easy. It's not perfect:

• it's not very performant (we're already in reflow here so this is unlikely to be the worst of our worries)
• It will still extend the attrs of the last column to the end of the row when reflowed, which can still result in some artifacts, but much less aggressive than before
  • we may be able to mitigate that by inserting a TextAttribute{} "bookend" to the end of the attr row, when we encounter the end of the old row.
• If you have a bunch of colored spaces, then resize smaller, then resize bigger, they'll be gone (again, no worse than today)

I just feel like I must have missed something obvious. It couldn't have been that easy

[j4james]

The first thing I noticed is that it only works as far as the last non-blank line. For example, if you do printf "\e[42m\e[2J"; sleep 10, and then resizing while it's sleeping, the whole screen goes back to black. Is that an expected limitation?

Then there were a couple of things in the code that looked wrong to me, which I suspect might fail in certain edge cases. One example being the line widths not taking line rendition into account - I'd expect us to be using the TextBuffer GetLineWidth method to determine the max width, and in the case of the old buffer, that value should already be in cOldColsTotal.

And why are we using the min of the old width and new width for the source column range? Maybe I'm missing something, but that doesn't make sense to me. I would have thought the newAttrColumn only cared about the new width and the copyAttrCol only cared about the old width?

Then, not related to this fix per se, but I noticed that the active attributes are being reset when you resize. I suspect this is because the two lines below are in the wrong order (the old attributes are restored into the buffer that's about to be replaced):

terminal/src/host/screenInfo.cpp

Lines 1474 to 1476 in 71c7556

	_textBuffer->SetCurrentAttributes(oldPrimaryAttributes);
	_textBuffer.swap(newTextBuffer);

I should also note that I've definitely had some other things looking wrong when messing about resizing randomly, but I need to experiment some more to get reproducible test cases.

[zadjii-msft]

The first thing I noticed is that it only works as far as the last non-blank line. For example, if you do printf "\e[42m\e[2J"; sleep 10, and then resizing while it's sleeping, the whole screen goes back to black. Is that an expected limitation?

In that branch, yea. That's one of the TODOs that definitely needs to be done 😅

Then there were a couple of things in the code that looked wrong to me, which I suspect might fail in certain edge cases. One example being the line widths not taking line rendition into account - I'd expect us to be using the TextBuffer GetLineWidth method to determine the max width, and in the case of the old buffer, that value should already be in cOldColsTotal.

See this is exactly why I asked for your help - I totally forgot about that feature entirely. That can be fixed. Thanks!

And why are we using the min of the old width and new width for the source column range? Maybe I'm missing something, but that doesn't make sense to me. I would have thought the newAttrColumn only cared about the new width and the copyAttrCol only cared about the old width?

Yea, that's an artifact from the first iteration of the fix I forgot to remove. Whoops.

This was a great sanity check! I'll patch those up and see if I can't get some tests written too. If we can confidently say that the fix is "not perfect, but definitely no worse than before", that's good enough for me frankly.

---

Then, not related to this fix per se, but I noticed that the active attributes are being reset when you resize. I suspect this is because the two lines below are in the wrong order (the old attributes are restored into the buffer that's about to be replaced):

holy shit you're right, that's been there since forever. I wonder if that's a bug floating around on the repo somewhere.

[ghost]

🎉This issue was addressed in #12637, which has now been successfully released as Windows Terminal v1.12.1098.:tada:

Handy links:

• Release Notes
• Store Download

[ghost]

🎉This issue was addressed in #12637, which has now been successfully released as Windows Terminal Preview v1.13.1098.:tada:

Handy links:

• Release Notes
• Store Download