Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace GetDefaultBrushColors with hardcoded default attributes #11982

Merged
1 commit merged into from
Jan 3, 2022

Conversation

j4james
Copy link
Collaborator

@j4james j4james commented Dec 16, 2021

The IRenderData::GetDefaultBrushColors method was intended to return
the default attributes from which the renderer would calculate the
default background color. It should always have been returning a default
TextAttribute object, but the conhost RenderData implementation was
mistakenly returning the active attributes instead. This resulted in
margin areas being filled with the wrong color. To correct that, this PR
simply replaces all usage of GetDefaultBrushColors with hardcoded
default attributes.

Validation Steps Performed

I've manually checked the test case described in issue #11976 and
confirmed that the conhost margin areas are now correctly filled with
the default background color.

Closes #11976

@zadjii-msft zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Jan 3, 2022
@ghost ghost added Priority-3 A description (P3) Product-Conhost For issues in the Console codebase labels Jan 3, 2022
@DHowett
Copy link
Member

DHowett commented Jan 3, 2022

@msftbot make sure @miniksa signs off

@ghost ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jan 3, 2022
@ghost
Copy link

ghost commented Jan 3, 2022

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

Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

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

That makes sense. Nice catch, @j4james. Thank you!

@miniksa
Copy link
Member

miniksa commented Jan 3, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ghost ghost merged commit ff7f569 into microsoft:main Jan 3, 2022
@j4james j4james deleted the nuke-getdefaultbrushcolors branch January 8, 2022 17:13
@ghost
Copy link

ghost commented Feb 3, 2022

🎉Windows Terminal Preview v1.13.10336.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
AutoMerge Marked for automatic merge by the bot when requirements are met Needs-Second It's a PR that needs another sign-off Priority-3 A description (P3) Product-Conhost For issues in the Console codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Conhost window padding is filled with the wrong color
4 participants