Skip to content

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

Merged
9 commits merged into
mainfrom
dev/migrie/b/3848-resize-color-explosion
Apr 21, 2021
Merged

9 commits merged into
mainfrom
dev/migrie/b/3848-resize-color-explosion

Conversation

@zadjii-msft

@zadjii-msft Mike Griese (zadjii-msft) commented May 7, 2020 •

Copy link
Copy Markdown
Member

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

Validation Steps Performed

  • ran tests

@ghost 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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

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?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Comment thread src/cascadia/TerminalCore/Terminal.cpp Resolved
@zadjii-msft
Mike Griese (zadjii-msft) force-pushed the dev/migrie/b/3848-resize-color-explosion branch from dbd9df7 to c3f9d36 Compare April 20, 2021 21:24
Comment thread src/host/screenInfo.cpp
{
newTextBuffer = std::make_unique<TextBuffer>(coordNewScreenSize,
gci.GetDefaultAttributes(),
TextAttribute{},

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ohhhhhhhhh okay

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

disc

@ghost 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

Copy link
Copy Markdown
Member

@msftbot make sure Michael Niksa (@miniksa) signs

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

Copy link
Copy Markdown

Hello Dustin L. Howett (@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:

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 Michael Niksa (miniksa) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. OK. It seems fair.

@ghost
ghost merged commit 913cf4b into main Apr 21, 2021
@ghost
ghost deleted the dev/migrie/b/3848-resize-color-explosion branch April 21, 2021 21:34
@ghost

Copy link
Copy Markdown

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

Handy links:

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-VT Virtual Terminal sequence support AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Second It's a PR that needs another sign-off Priority-2 A description (P2) Product-Terminal The new Windows Terminal.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Weird blue selection after closing neovim (WSL)

4 participants