Skip to content

Don't remove spaces when printing a new bottom line with a background color#5550

Merged
7 commits merged intomasterfrom
dev/migrie/b/5502-background-colors-after-circling
Apr 30, 2020
Merged

Don't remove spaces when printing a new bottom line with a background color#5550
7 commits merged intomasterfrom
dev/migrie/b/5502-background-colors-after-circling

Conversation

@zadjii-msft
Copy link
Member

@zadjii-msft zadjii-msft commented Apr 24, 2020 •
edited by DHowett-MSFT
Loading

Turns out we're still being a bit too aggressive when removing spaces.
If there are spaces at the end of the first run painted to a bottom
line, and the bottom line was a different color than the previous,
then we can't trim those spaces off the string. We still need to emit
those to make sure the terminal has colored spaces in it as well.

References

  • there's like 80 PRs in the last month for this function

PR Checklist

Validation Steps

  • ran the tests
  • checked that vtpipeterm still worked
  • Checked that the bug was fixed in the Terminal

@zadjii-msft zadjii-msft added Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Product-Conpty For console issues specifically related to conpty labels Apr 24, 2020
DHowett-MSFT
DHowett-MSFT reviewed Apr 24, 2020
View reviewed changes
Copy link
Contributor

@DHowett-MSFT DHowett-MSFT left a comment

Choose a reason for hiding this comment

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

I'm not sure I understand why we're hitting the "optimize spaces" case between two other characters.

Also, do you think it would be possible to save the whole TextAttribute and use the HasIdenticalVisualRepresentationForBlankSpace function? It takes care of cases like global and local inverse (which James pointed out) which I think could be a ... problem .. here

nevermind, it won't be a problem here because we're using the COLORREFs and we've lost text attribute info by this point (#2661)

Caching more state always scares me :)

@zadjii-msft
Copy link
Member Author

zadjii-msft commented Apr 27, 2020

So in this case, the row we're printing is separated into two runs:

  • "    #         "
    • This run is in green-on-blue
  • "#             "...
    • This run is in the default attrs

It's that first run's 9 spaces that we tried to optimize with the ECH.

I could certainly see us needing to cache more info, but maybe that's just something we should handle in #2661 when we have all the info in one nice blob

DHowett-MSFT
DHowett-MSFT approved these changes Apr 29, 2020
View reviewed changes
Copy link
Contributor

@DHowett-MSFT DHowett-MSFT left a comment

Choose a reason for hiding this comment

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

I don't love it, but it works and that's more important than ideology.

@zadjii-msft zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Apr 29, 2020
@ghost ghost requested review from carlos-zamora, leonMSFT and miniksa April 29, 2020 20:19
miniksa
miniksa approved these changes Apr 29, 2020
View reviewed changes
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.

This spaghetti is killing me, but if it works, I have to agree with Dustin and just let it go. At least there are more tests than we had two months ago before this started so we might have some hope of making this less gross in the future.

@DHowett-MSFT DHowett-MSFT added the AutoMerge Marked for automatic merge by the bot when requirements are met label Apr 29, 2020
@ghost ghost added the Issue-Bug It either shouldn't be doing this or needs an investigation. label Apr 29, 2020
@ghost
Copy link

ghost commented Apr 29, 2020

Hello @DHowett-MSFT!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@DHowett-MSFT
Copy link
Contributor

DHowett-MSFT commented Apr 30, 2020

Wow -- merging master actually broke the test (???) Sorry :(

@ghost ghost merged commit 4286877 into master Apr 30, 2020
@ghost ghost deleted the dev/migrie/b/5502-background-colors-after-circling branch April 30, 2020 15:01
@ghost
Copy link

ghost commented May 5, 2020

🎉Windows Terminal Release Candidate v0.11.1251.0 (1.0rc1) has been released which incorporates this pull request.:tada:

Handy links:

@ghost ghost mentioned this pull request May 5, 2020
Closed
@zadjii-msft zadjii-msft mentioned this pull request May 12, 2020
Open
31 tasks
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@miniksa miniksa miniksa approved these changes

@carlos-zamora carlos-zamora Awaiting requested review from carlos-zamora

@leonMSFT leonMSFT Awaiting requested review from leonMSFT

+1 more reviewer

@DHowett-MSFT DHowett-MSFT DHowett-MSFT approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues 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 Product-Conpty For console issues specifically related to conpty

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

New 0.11 VT colors bug, background color isn't applied to " " (space) cells

4 participants

@zadjii-msft @DHowett-MSFT @miniksa @DHowett