Initialize the text buffer with the default attributes on a resize#5792
Initialize the text buffer with the default attributes on a resize#57929 commits merged intomainfrom
Conversation
ghost
added
Area-VT
DHowett-MSFT
left a comment
DHowett-MSFT
left a comment
There was a problem hiding this comment.
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?
|
I honestly don't know. I'm not sure it's well defined behavior.
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
left a comment
DHowett
left a comment
There was a problem hiding this comment.
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?
…re initialized correctly
…ore after the resize
zadjii-msft
force-pushed
the
dev/migrie/b/3848-resize-color-explosion
branch
from
dbd9df7 to
c3f9d36
Compare
| { | ||
| newTextBuffer = std::make_unique<TextBuffer>(coordNewScreenSize, | ||
| gci.GetDefaultAttributes(), | ||
| TextAttribute{}, |
There was a problem hiding this comment.
whoa. this one scares me!! the session default attributes are more important in conhost than terminal. why the change?
There was a problem hiding this comment.
oh but it defaults to {Default, Default} doesn't it
There was a problem hiding this comment.
Oh I thought this was the way to do it post-#6506
ghost
added
Needs-Author-Feedback
ghost
requested review from
PankajBhojwani,
carlos-zamora,
leonMSFT and
miniksa
|
@msftbot make sure @miniksa signs |
ghost
added
the
AutoMerge
|
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:
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". |
|
🎉 Handy links: |
4 participants
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
vimisrunning, 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::GetDefaultAttributesis, which could be any combo of thelegacy indices and the default color.
PR Checklist
Validation Steps Performed