starred/terminal
1
0
Fork 0
mirror of https://github.com/microsoft/terminal.git synced 2026-02-03 21:25:34 +00:00
Code Issues 19.0k Packages Projects Releases 20 Wiki Activity

[PR #6506] [MERGED] Improve the propagation of color attributes over ConPTY #26713

New Issue
Open
opened 2026-01-31 09:17:42 +00:00 by claunia · 0 comments
claunia commented 2026-01-31 09:17:42 +00:00
Owner
Copy Link

📋 Pull Request Information

Original PR: https://github.com/microsoft/terminal/pull/6506
Author: @j4james
Created: 6/15/2020
Status: ✅ Merged
Merged: 7/1/2020
Merged by: @DHowett

Base: master ← Head: fix-conpty-narrowing

📝 Commits (10+)

  • aee0e75 Pass the full text attributes through to the renderer's UpdateDrawingBrushes method.
  • 8ae6147 Pass the IRenderData to UpdateDrawingBrushes instead of the COLORREFs.
  • e27d39b Update VtRendererTests to account for changes to the renderer interface.
  • 626fca4 Reimplement the Xterm256Engine UpdateDrawingBrushes method preserving the attribute color format.
  • 6f73dd7 Simplify and consolidate the meta/extended attribute updates.
  • c111b7a Reimplement the 16-color UpdateDrawingBrushes method to avoid doing palette searches.
  • 1c31b4f Add support for reverse video in the 16-color Xterm engine and correct the order attributes are applied.
  • fb3afaf Update unit tests to match the new behavior of the VT render engines.
  • 5cc251f Cleanup FindTableIndex methods that are no longer needed.
  • 8b7dbf4 Remove the ColorProvider interface that's no longer needed.

📊 Changes

35 files changed (+614 additions, -796 deletions)

View changed files

📝 src/buffer/out/TextAttribute.cpp (+21 -1)
📝 src/buffer/out/TextAttribute.hpp (+4 -0)
📝 src/buffer/out/TextColor.cpp (+7 -2)
📝 src/buffer/out/TextColor.h (+4 -3)
📝 src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp (+1 -3)
📝 src/host/VtIo.cpp (+1 -7)
📝 src/host/conattrs.cpp (+0 -117)
📝 src/host/server.h (+1 -4)
📝 src/host/ut_host/ConptyOutputTests.cpp (+1 -3)
📝 src/host/ut_host/VtIoTests.cpp (+6 -54)
📝 src/host/ut_host/VtRendererTests.cpp (+302 -135)
➖ src/inc/IDefaultColorProvider.hpp (+0 -28)
📝 src/inc/conattrs.hpp (+0 -7)
📝 src/inc/test/CommonState.hpp (+1 -1)
📝 src/interactivity/onecore/BgfxEngine.cpp (+3 -5)
📝 src/interactivity/onecore/BgfxEngine.hpp (+2 -4)
📝 src/interactivity/win32/menu.cpp (+1 -2)
📝 src/renderer/base/renderer.cpp (+1 -8)
📝 src/renderer/dx/DxRenderer.cpp (+8 -9)
📝 src/renderer/dx/DxRenderer.hpp (+3 -5)

...and 15 more files

📄 Description

Summary of the Pull Request

This PR reimplements the VT rendering engines to do a better job of preserving the original color types when propagating attributes over ConPTY. For the 16-color renderers it provides better support for default colors and improves the efficiency of the color narrowing conversions. It also fixes problems with the ordering of character renditions that could result in attributes being dropped.

References

  • This is only a partial fix for #293, but I suspect the remaining cases are unfixable.

PR Checklist

Detailed Description of the Pull Request / Additional comments

Originally the base renderer would calculate the RGB color values and legacy/extended attributes up front, passing that data on to the active engine's UpdateDrawingBrushes method. With this new implementation, the renderer now just passes through the original TextAttribute along with an IRenderData interface, and leaves it to the engines to extract the information they need.

The GDI and DirectX engines now have to lookup the RGB colors themselves (via simple IRenderData calls), but have no need for the other attributes. The VT engines extract the information that they need from the TextAttribute, instead of having to reverse engineer it from COLORREFs.

The process for the 256-color Xterm engine starts with a check for default colors. If both foreground and background are default, it outputs a SGR 0 reset, and clears the _lastTextAttribute completely to make sure any reset state is reapplied. With that out the way, the foreground and background are updated (if changed) in one of 4 ways. They can either be a default value (SGR 39 and 49), a 16-color index (using ANSI or AIX sequences), a 256-color index, or a 24-bit RGB value (both using SGR 38 and 48 sequences).

Then once the colors are accounted for, there is a separate step that handles the character rendition attributes (bold, italics, underline, etc.) This step must come after the color sequences, in case a SGR reset is required, which would otherwise have cleared any character rendition attributes if it came last (which is what happened in the original implementation).

The process for the 16-color engines is a little different. The target client in this case (Windows telnet) is incapable of setting default colors individually, so we need to output an SGR 0 reset if either color has changed to default. With that out the way, we use the TextColor::GetLegacyIndex method to obtain an approximate 16-color index for each color, and apply the bold attribute by brightening the foreground index (setting bit 8) if the color type permits that.

However, since Windows telnet only supports the 8 basic ANSI colors, the best we can do for bright colors is to output an SGR 1 attribute to get a bright foreground. There is nothing we can do about a bright background, so after that we just have to drop the high bit from the colors. If the resulting index values have changed from what they were before, we then output ANSI 8-color SGR sequences to update them.

As with the 256-color engine, there is also a final step to handle the character rendition attributes. But in this case, the only supported attributes are underline and reversed video.

Since the VT engines no longer depend on the active color table and default color values, there was quite a lot of code that could now be removed. This included the IDefaultColorProvider interface and implementations, the Find(Nearest)TableIndex functions, and also the associated HLS conversion and difference calculations.

Validation Steps Performed

Other than simple API parameter changes, the majority of updates required in the unit tests were to correct assumptions about the way the colors should be rendered, which were the source of the narrowing bugs this PR was trying to fix. Like passing white on black to the UpdateDrawingBrushes API, and expecting it to output the default SGR 0 sequence, or passing an RGB color and expecting an indexed SGR sequence.

In addition to that, I've added some VT renderer tests to make sure the rendition attributes (bold, underline, etc) are correctly retained when a default color update causes an SGR 0 sequence to be generated (the source of bug #3076). And I've extended the VT renderer color tests (both 256-color and 16-color) to make sure we're covering all of the different color types (default, RGB, and both forms of indexed colors).

I've also tried to manually verify that all of the test cases in the linked bug reports (and their associated duplicates) are now fixed when this PR is applied.

🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.

## 📋 Pull Request Information **Original PR:** https://github.com/microsoft/terminal/pull/6506 **Author:** [@j4james](https://github.com/j4james) **Created:** 6/15/2020 **Status:** ✅ Merged **Merged:** 7/1/2020 **Merged by:** [@DHowett](https://github.com/DHowett) **Base:** `master` ← **Head:** `fix-conpty-narrowing` --- ### 📝 Commits (10+) - [`aee0e75`](https://github.com/microsoft/terminal/commit/aee0e7531a6b0aebc6d2a4476aa0fb1df2c3744a) Pass the full text attributes through to the renderer's UpdateDrawingBrushes method. - [`8ae6147`](https://github.com/microsoft/terminal/commit/8ae61470e6980e14c406fe43d24436e2ed0999fe) Pass the IRenderData to UpdateDrawingBrushes instead of the COLORREFs. - [`e27d39b`](https://github.com/microsoft/terminal/commit/e27d39bcb097d1dde9f8ce3a9cdbe027480f10c4) Update VtRendererTests to account for changes to the renderer interface. - [`626fca4`](https://github.com/microsoft/terminal/commit/626fca4bda980241de714a9dfdd722d4d33171ea) Reimplement the Xterm256Engine UpdateDrawingBrushes method preserving the attribute color format. - [`6f73dd7`](https://github.com/microsoft/terminal/commit/6f73dd7641497d56df4a140db91f32489cff187d) Simplify and consolidate the meta/extended attribute updates. - [`c111b7a`](https://github.com/microsoft/terminal/commit/c111b7a1429d4c5a32b074a8efc0c5b04023fed1) Reimplement the 16-color UpdateDrawingBrushes method to avoid doing palette searches. - [`1c31b4f`](https://github.com/microsoft/terminal/commit/1c31b4fc55cb2957ea33c8d49a92478b8ede58c4) Add support for reverse video in the 16-color Xterm engine and correct the order attributes are applied. - [`fb3afaf`](https://github.com/microsoft/terminal/commit/fb3afaf16c476512d0c0fa19af9840e682bebcd9) Update unit tests to match the new behavior of the VT render engines. - [`5cc251f`](https://github.com/microsoft/terminal/commit/5cc251fb74ccf978710afa6abb6074437370178c) Cleanup FindTableIndex methods that are no longer needed. - [`8b7dbf4`](https://github.com/microsoft/terminal/commit/8b7dbf44dd90c6c99b7bbd1d3e460c79ea30e53a) Remove the ColorProvider interface that's no longer needed. ### 📊 Changes **35 files changed** (+614 additions, -796 deletions) <details> <summary>View changed files</summary> 📝 `src/buffer/out/TextAttribute.cpp` (+21 -1) 📝 `src/buffer/out/TextAttribute.hpp` (+4 -0) 📝 `src/buffer/out/TextColor.cpp` (+7 -2) 📝 `src/buffer/out/TextColor.h` (+4 -3) 📝 `src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp` (+1 -3) 📝 `src/host/VtIo.cpp` (+1 -7) 📝 `src/host/conattrs.cpp` (+0 -117) 📝 `src/host/server.h` (+1 -4) 📝 `src/host/ut_host/ConptyOutputTests.cpp` (+1 -3) 📝 `src/host/ut_host/VtIoTests.cpp` (+6 -54) 📝 `src/host/ut_host/VtRendererTests.cpp` (+302 -135) ➖ `src/inc/IDefaultColorProvider.hpp` (+0 -28) 📝 `src/inc/conattrs.hpp` (+0 -7) 📝 `src/inc/test/CommonState.hpp` (+1 -1) 📝 `src/interactivity/onecore/BgfxEngine.cpp` (+3 -5) 📝 `src/interactivity/onecore/BgfxEngine.hpp` (+2 -4) 📝 `src/interactivity/win32/menu.cpp` (+1 -2) 📝 `src/renderer/base/renderer.cpp` (+1 -8) 📝 `src/renderer/dx/DxRenderer.cpp` (+8 -9) 📝 `src/renderer/dx/DxRenderer.hpp` (+3 -5) _...and 15 more files_ </details> ### 📄 Description ## Summary of the Pull Request This PR reimplements the VT rendering engines to do a better job of preserving the original color types when propagating attributes over ConPTY. For the 16-color renderers it provides better support for default colors and improves the efficiency of the color narrowing conversions. It also fixes problems with the ordering of character renditions that could result in attributes being dropped. ## References * This is only a partial fix for #293, but I suspect the remaining cases are unfixable. ## PR Checklist * [x] Closes #2661 * [x] Closes #3076 * [x] Closes #3717 * [x] Closes #5384 * [x] Closes #5864 * [x] CLA signed. * [x] Tests added/passed * [ ] Requires documentation to be updated * [x] I've discussed this with core contributors already. Issue number where discussion took place: #2661 ## Detailed Description of the Pull Request / Additional comments Originally the base renderer would calculate the RGB color values and legacy/extended attributes up front, passing that data on to the active engine's `UpdateDrawingBrushes` method. With this new implementation, the renderer now just passes through the original `TextAttribute` along with an `IRenderData` interface, and leaves it to the engines to extract the information they need. The GDI and DirectX engines now have to lookup the RGB colors themselves (via simple `IRenderData` calls), but have no need for the other attributes. The VT engines extract the information that they need from the `TextAttribute`, instead of having to reverse engineer it from `COLORREF`s. The process for the 256-color Xterm engine starts with a check for default colors. If both foreground and background are default, it outputs a SGR 0 reset, and clears the `_lastTextAttribute` completely to make sure any reset state is reapplied. With that out the way, the foreground and background are updated (if changed) in one of 4 ways. They can either be a default value (SGR 39 and 49), a 16-color index (using ANSI or AIX sequences), a 256-color index, or a 24-bit RGB value (both using SGR 38 and 48 sequences). Then once the colors are accounted for, there is a separate step that handles the character rendition attributes (bold, italics, underline, etc.) This step must come _after_ the color sequences, in case a SGR reset is required, which would otherwise have cleared any character rendition attributes if it came last (which is what happened in the original implementation). The process for the 16-color engines is a little different. The target client in this case (Windows telnet) is incapable of setting default colors individually, so we need to output an SGR 0 reset if _either_ color has changed to default. With that out the way, we use the `TextColor::GetLegacyIndex` method to obtain an approximate 16-color index for each color, and apply the bold attribute by brightening the foreground index (setting bit 8) if the color type permits that. However, since Windows telnet only supports the 8 basic ANSI colors, the best we can do for bright colors is to output an SGR 1 attribute to get a bright foreground. There is nothing we can do about a bright background, so after that we just have to drop the high bit from the colors. If the resulting index values have changed from what they were before, we then output ANSI 8-color SGR sequences to update them. As with the 256-color engine, there is also a final step to handle the character rendition attributes. But in this case, the only supported attributes are underline and reversed video. Since the VT engines no longer depend on the active color table and default color values, there was quite a lot of code that could now be removed. This included the `IDefaultColorProvider` interface and implementations, the `Find(Nearest)TableIndex` functions, and also the associated HLS conversion and difference calculations. ## Validation Steps Performed Other than simple API parameter changes, the majority of updates required in the unit tests were to correct assumptions about the way the colors should be rendered, which were the source of the narrowing bugs this PR was trying to fix. Like passing white on black to the `UpdateDrawingBrushes` API, and expecting it to output the default `SGR 0` sequence, or passing an RGB color and expecting an indexed SGR sequence. In addition to that, I've added some VT renderer tests to make sure the rendition attributes (bold, underline, etc) are correctly retained when a default color update causes an `SGR 0` sequence to be generated (the source of bug #3076). And I've extended the VT renderer color tests (both 256-color and 16-color) to make sure we're covering all of the different color types (default, RGB, and both forms of indexed colors). I've also tried to manually verify that all of the test cases in the linked bug reports (and their associated duplicates) are now fixed when this PR is applied. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
claunia added the pull-request label 2026-01-31 09:17:42 +00:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
⛺ Reserved
A11yCO
A11yMAS
A11ySev1
A11ySev2
A11ySev3
A11yTTValidated
A11yUsable
A11yVoiceAccess
A11yWCAG
Area-Accessibility
Area-AtlasEngine
Area-AzureShell
Area-Build
Area-Build
Area-Chat
Area-CmdPal
Area-CodeHealth
Area-Commandline
Area-CookedRead
Area-DefApp
Area-Extensibility
Area-Fonts
Area-GroupPolicy
Area-i18n
Area-Input
Area-Interaction
Area-Interop
Area-Localization
Area-Output
Area-Performance
Area-Portable
Area-Quality
Area-Remoting
Area-Rendering
Area-Schema
Area-Server
Area-Settings
Area-SettingsUI
Area-ShellExtension
Area-ShellExtension
Area-ShellExtension
Area-Suggestions
Area-Suggestions
Area-TerminalConnection
Area-TerminalControl
Area-Theming
Area-UserInterface
Area-VT
Area-Windowing
Area-WPFControl
AutoMerge
Blocking-Ingestion
Culprit-Centennial
Culprit-WinUI
Disability-All
Disability-Blind
Disability-LowVision
Disability-Mobility
External-Blocked-WinUI3
Fixed
Gathering-Data
good first issue
HCL-E+D
HCL-WindowsTerminal
Help Wanted
Impact-Compatibility
Impact-Compliance
Impact-Correctness
Impact-Visual
In-PR
InclusionBacklog
InclusionBacklog-Windows TerminalWin32
InclusionCommitted-202206
Issue-Bug
Issue-Docs
Issue-Feature
Issue-Feature
Issue-Question
Issue-Samples
Issue-Scenario
Issue-Task
Needs-Attention
Needs-Author-Feedback
Needs-Bisect
Needs-Discussion
Needs-Repro
Needs-Tag-Fix
Needs-Tag-Fix
Needs-Triage
No-Recent-Activity
Priority-0
Priority-1
Priority-2
Priority-3
Product-Cmd.exe
Product-Colortool
Product-Colortool
Product-Colortool
Product-Conhost
Product-Conpty
Product-Meta
Product-Powershell
Product-Terminal
Product-WSL
pull-request
Mirrored from GitHub Pull Request
 Resolution-Answered
Resolution-By-Design
Resolution-Duplicate
Resolution-External
Resolution-Fix-Available
Resolution-Fix-Committed
Resolution-No-Repro
Resolution-Won't-Fix
Severity-Blocking
Severity-Crash
Severity-DataLoss
spam
this-will-be-a-breaking-change
Tracking-External
WindowsTerminal_Win32
Work-Item
zAskModeBug
zInbox-Bug
No Label pull-request
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
claunia
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: starred/terminal#26713
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?