Fix conpty not emitting colored spaces on the VERY FIRST frame #13665
Conversation
ghost
added
Area-Rendering
This comment has been minimized.
This comment has been minimized.
| _clearedAllThisFrame || | ||
| (_newBottomLine && printingBottomLine && bgMatched)); | ||
| const auto cchActual = removeSpaces ? nonSpaceLength : cchLine; | ||
| static const TextAttribute defaultAttrs{}; |
There was a problem hiding this comment.
It has been so long that I can't recall whether I wrote this
| const auto cchActual = removeSpaces ? nonSpaceLength : cchLine; | ||
| static const TextAttribute defaultAttrs{}; | ||
| const bool removeSpaces = !lineWrapped && (useEraseChar // we determined earlier that ECH is optimal | ||
| || (_clearedAllThisFrame && _lastTextAttributes == defaultAttrs) // OR we cleared the last frame to the default attributes (specifically) |
There was a problem hiding this comment.
I don't think I ever recommended this on account of its fragility, and I meant to ask @j4james if this seemed ... uh, reasonable?
Sorry, I'm still a bit brain-fogged
There was a problem hiding this comment.
I don't know know the conpty code very well, so I'm unsure if it's guaranteed that _clearedAllThisFrame would mean the screen was cleared with the default attributes. Is that correct? It's not possible for conpty to inherit colors from somewhere in the first frame, like maybe when nesting conpty sessions?
But if you're sure the screen was cleared with default attributes, and we're now writing out text with default attributes, then I think that would imply it's safe to remove spaces.
There was a problem hiding this comment.
Looks like the only place _clearedAllThisFrame = true is set is in the first paint of StartPaint, when yea, the screen was cleared with the defaults. I don't think nesting conpty would change that - even if you did color 46, wsl, cmd.exe in a Terminal cmd instance, that second conpty would spawn with default attrs.
That sounds good enough to me!
|
awesome. now just spellbot it! |
|
Should the test case include a frame with white space characters? For example a terminal prompt containing a /n |
zadjii-msft
added
the
AutoMerge
|
Hello @zadjii-msft! Because this pull request has the 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 (
|
ghost
merged commit This bug arose from a "race condition" in the first frame handling of conpty. We'd try to optimize out spaces if we've cleared the entire frame (which always happens on the first frame). However, doing that even for colored spaces meant that things like powerline prompts could be emitted to conhost during the first frame, and we'd optimize the spaces out. That's silly. This is hard to repro naturally, but this comment has another repro I used #8341 (comment) Modified to facilitate simpler testing, it looks like: #### Before  #### After  * [x] Closes #8341 * [x] Tests added Co-authored by @DHowett (cherry picked from commit 210a98e) Service-Card-Id: 84836597 Service-Version: 1.15
|
🎉 Handy links: |
ghost
mentioned this pull request
|
🎉 Handy links: |
No longer needed per microsoft/terminal#13665
…soft#13665) This bug arose from a "race condition" in the first frame handling of conpty. We'd try to optimize out spaces if we've cleared the entire frame (which always happens on the first frame). However, doing that even for colored spaces meant that things like powerline prompts could be emitted to conhost during the first frame, and we'd optimize the spaces out. That's silly. This is hard to repro naturally, but this comment has another repro I used microsoft#8341 (comment) Modified to facilitate simpler testing, it looks like:   * [x] Closes microsoft#8341 * [x] Tests added Co-authored by @DHowett (cherry picked from commit 210a98e) Service-Card-Id: 84836596 Service-Version: 1.14
This bug arose from a "race condition" in the first frame handling of conpty. We'd try to optimize out spaces if we've cleared the entire frame (which always happens on the first frame). However, doing that even for colored spaces meant that things like powerline prompts could be emitted to conhost during the first frame, and we'd optimize the spaces out. That's silly.
This is hard to repro naturally, but this comment has another repro I used
#8341 (comment)
Modified to facilitate simpler testing, it looks like:
Before
After
Co-authored by @DHowett