Initialize the text buffer with the default attributes on a resize #5792

zadjii-msft · 2020-05-07T16:59:17Z

When we resize the text buffer, initialize the buffer with the
_default_¹ attributes, not the current ones. If we use the current
attributes, then we can get into scenarios where something like vim is
running, and left the attributes set to something other than the
defaults, and when we resized the buffer, we'd fill it up with color, as
opposed to whatever the default would be.

This PR instead initializes the buffers with the default colors. It also
makes sure to set the active attributes of the newly created buffers
back to whatever the current attributes of the old buffer were.

[1]: For the Terminal, the default attributes are "default on default".
For conhost, the default attributes are whatever the result of
Settings::GetDefaultAttributes is, which could be any combo of the
legacy indices and the default color.

PR Checklist

Closes Weird blue selection after closing neovim (WSL) #3848
I work here
Tests added/passed
[n/a] Requires documentation to be updated

Validation Steps Performed

ran tests

DHowett-MSFT

I'm actually a bit worried about this -- considering that a resize "reveals new lines" below the prompt, those lines should be subject to the same attribute continuation rules... am I off base?

zadjii-msft · 2020-05-08T21:43:09Z

I honestly don't know. I'm not sure it's well defined behavior.

gnome-terminal definitely doesn't let you "reveal new lines" when you increase the height of the buffer - the buffer's always at the bottom, resizing just moves lines from scrollback into the buffer, so that's not really an issue for them.

IMO we should act like those lines don't exist until text gets to them. Then whenever text is output and we eventually do get to that line, we'll initialize it with whatever the new attributes should currently be for those lines. (IIRC, we already do that).

Maybe selfhost this (after 1.0) so we can see how it feels?

DHowett

Okay. I'm a little scared, but this is probably correct. Resize should initialize to the default and then be filled up with the current attributes. Yes.

This isn't going to bother cmd's color command (in conhost), right?

src/cascadia/TerminalCore/Terminal.cpp

…re initialized correctly

…ore after the resize

DHowett · 2021-04-20T21:42:49Z

src/host/screenInfo.cpp

@@ -1422,7 +1421,7 @@ bool SCREEN_INFORMATION::IsMaximizedY() const
    try
    {
        newTextBuffer = std::make_unique<TextBuffer>(coordNewScreenSize,
-                                                     gci.GetDefaultAttributes(),
+                                                     TextAttribute{},


whoa. this one scares me!! the session default attributes are more important in conhost than terminal. why the change?

oh but it defaults to {Default, Default} doesn't it

Oh I thought this was the way to do it post-#6506

ohhhhhhhhh okay

DHowett

disc

DHowett · 2021-04-20T23:50:17Z

@msftbot make sure @miniksa signs

ghost · 2021-04-20T23:50:21Z

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".

miniksa

Yeah. OK. It seems fair.

ghost · 2021-05-25T19:06:51Z

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

Handy links:

ghost added Area-VT Virtual Terminal sequence support Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Terminal The new Windows Terminal. labels May 7, 2020

DHowett-MSFT reviewed May 7, 2020

View reviewed changes

microsoft-github-updates bot changed the base branch from master to main October 22, 2020 00:27

zadjii-msft mentioned this pull request Apr 20, 2021

Weird blue selection after closing neovim (WSL) #3848

Closed

DHowett approved these changes Apr 20, 2021

View reviewed changes

src/cascadia/TerminalCore/Terminal.cpp Outdated Show resolved Hide resolved

zadjii-msft added 9 commits April 20, 2021 15:17

clean this test up substantially

0177498

I believe this is the fix for this bug

ee92c26

This should really be in the previous commit as well.

72e0a55

This is another test case for checking the Terminal buffer contents a…

f32d1e8

…re initialized correctly

Make sure to stash the current attributes before the resize, and rest…

1d067cc

…ore after the resize

I believe this is needed here as well.

e945fb3

Some test cleanup for review

ea66461

This test passes without any resizing involved

46d6d68

update for merge with main

c3f9d36

zadjii-msft force-pushed the dev/migrie/b/3848-resize-color-explosion branch from dbd9df7 to c3f9d36 Compare April 20, 2021 21:24

DHowett reviewed Apr 20, 2021

View reviewed changes

DHowett requested changes Apr 20, 2021

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 Apr 20, 2021

DHowett approved these changes Apr 20, 2021

View reviewed changes

DHowett added the Needs-Second It's a PR that needs another sign-off label Apr 20, 2021

ghost requested review from miniksa, carlos-zamora, leonMSFT and PankajBhojwani April 20, 2021 23:49

ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Apr 20, 2021

miniksa approved these changes Apr 21, 2021

View reviewed changes

ghost merged commit 913cf4b into main Apr 21, 2021

ghost deleted the dev/migrie/b/3848-resize-color-explosion branch April 21, 2021 21:34

zadjii-msft mentioned this pull request Aug 23, 2021

Console background color has strange behavior when changed from a program #8144

Closed

zadjii-msft mentioned this pull request Dec 2, 2021

CMD color Not Working with Scroll #11867

Closed

zadjii-msft mentioned this pull request Mar 2, 2022

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

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initialize the text buffer with the default attributes on a resize #5792

Initialize the text buffer with the default attributes on a resize #5792

zadjii-msft commented May 7, 2020 •

edited by DHowett

Loading

DHowett-MSFT left a comment

zadjii-msft commented May 8, 2020

DHowett left a comment

DHowett Apr 20, 2021

DHowett Apr 20, 2021

zadjii-msft Apr 20, 2021

DHowett Apr 20, 2021

DHowett left a comment

DHowett commented Apr 20, 2021

ghost commented Apr 20, 2021

miniksa left a comment

ghost commented May 25, 2021

Initialize the text buffer with the default attributes on a resize #5792

Initialize the text buffer with the default attributes on a resize #5792

Conversation

zadjii-msft commented May 7, 2020 • edited by DHowett Loading

PR Checklist

Validation Steps Performed

DHowett-MSFT left a comment

Choose a reason for hiding this comment

zadjii-msft commented May 8, 2020

DHowett left a comment

Choose a reason for hiding this comment

DHowett Apr 20, 2021

Choose a reason for hiding this comment

DHowett Apr 20, 2021

Choose a reason for hiding this comment

zadjii-msft Apr 20, 2021

Choose a reason for hiding this comment

DHowett Apr 20, 2021

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

DHowett commented Apr 20, 2021

ghost commented Apr 20, 2021

miniksa left a comment

Choose a reason for hiding this comment

ghost commented May 25, 2021

zadjii-msft commented May 7, 2020 •

edited by DHowett

Loading