Fix the ConPTY extended attributes optimization #13661
Conversation
ghost
added
Area-Rendering
|
Whoops, I might want to revert e476682. I started from there, but that might not actually be relevant to this bug. |
This comment has been minimized.
This comment has been minimized.
…-also-maybe-13229
This reverts commit e476682.
src/buffer/out/TextAttribute.hpp
Outdated
| !IsAnyGridLineEnabled() || | ||
| GetHyperlinkId() != 0 || | ||
| IsReverseVideo(); |
There was a problem hiding this comment.
BTW !IsAnyGridLineEnabled() || IsReverseVideo() is the same as _wAttrLegacy != 0. The wLegacyAttr & META_ATTRS in the constructor strips away legacy color attributes and the following WI_ClearAllFlags(_wAttrLegacy, COMMON_LVB_SBCSDBCS); strips DBCS attributes. All attributes that are left are all those that you're testing here.
| @@ -161,6 +164,13 @@ class TextAttribute final | |||
| { | |||
| return WI_IsAnyFlagSet(_wAttrLegacy, COMMON_LVB_GRID_HORIZONTAL | COMMON_LVB_GRID_LVERTICAL | COMMON_LVB_GRID_RVERTICAL | COMMON_LVB_UNDERSCORE); | |||
| } | |||
| constexpr bool HasAnyExtendedAttributes() const noexcept | |||
| { | |||
| return GetExtendedAttributes() != ExtendedAttributes::Normal || | |||
There was a problem hiding this comment.
(fwiw: since GetExtendedAttributes doesn't do any translation or synthesis on its own, why should we use it instead of just using _extendedAttrs directly?)
| @@ -424,10 +424,15 @@ using namespace Microsoft::Console::Types; | |||
| // the inbox telnet client doesn't understand the Erase Character sequence, | |||
| // and it uses xterm-ascii. This ensures that xterm and -256color consumers | |||
| // get the enhancements, and telnet isn't broken. | |||
| // | |||
| // GH#13229: ECH and EL don't fill the space with "meta" attributes like | |||
There was a problem hiding this comment.
I mean, don't we still have the "background" version of this problem? \e[40m x theres a lot of spaces here, github deleted them \e[m doesn't require any extended attributes.
There was a problem hiding this comment.
more tests you got it
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 ... which should have never worked in the first place Quick filing a PR for review. This is the bulk of the actual code changes. Figured it was best to review the conpty changes sooner than later and I can add tests in the morning. Test cases: ``` printf "\e[7m test \e[m\n" ``` ``` printf "\e[7m"; printf ' %.0s' $(seq 1 $COLUMNS); printf "\e[m\n" ``` After:  Closes #13229 Definitely fixes: * [x] #13643 * [x] PowerShell/PowerShell#17812 (cherry picked from commit ffe9a0f) Service-Card-Id: 84736231 Service-Version: 1.15
|
🎉 Handy links: |
ghost
mentioned this pull request
|
🎉 Handy links: |
... which should have never worked in the first place Quick filing a PR for review. This is the bulk of the actual code changes. Figured it was best to review the conpty changes sooner than later and I can add tests in the morning. Test cases: ``` printf "\e[7m test \e[m\n" ``` ``` printf "\e[7m"; printf ' %.0s' $(seq 1 $COLUMNS); printf "\e[m\n" ``` After:  Closes microsoft#13229 Definitely fixes: * [x] microsoft#13643 * [x] PowerShell/PowerShell#17812 (cherry picked from commit ffe9a0f) Service-Card-Id: 84736231 Service-Version: 1.15 (cherry picked from commit fff3372) Service-Card-Id: 84736230 Service-Version: 1.14
... which should have never worked in the first place
Quick filing a PR for review. This is the bulk of the actual code changes. Figured it was best to review the conpty changes sooner than later and I can add tests in the morning.
Test cases:
After:
Closes #13229
Definitely fixes: