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 #5834] [MERGED] Fix SGR indexed colors to distinguish Indexed256 color (and more) #26488

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

📋 Pull Request Information

Original PR: https://github.com/microsoft/terminal/pull/5834
Author: @j4james
Created: 5/10/2020
Status: ✅ Merged
Merged: 5/27/2020
Merged by: @undefined

Base: master ← Head: fix-indexed-colors

📝 Commits (9)

  • d5aabb4 Distinguish between indexed colors from the 16 color palette (which can be brightened), and the 256 color palette (which can't).
  • 414c727 Make the terminal dispatcher use the 256 color index format for SGR 38:5 and 48:5 escape sequences.
  • 6ca9015 Update unit tests to account for the updated index color formats.
  • 1814a58 Avoid directly copying the color table with memmove.
  • c06c575 Remove some unnecessary functions that made the code harder to follow.
  • a4c23a6 Combine the 16 and 256 color tables, and use string_views when passing the tables around.
  • f321051 Make the conhost dispatcher use the 256 color index format for SGR 38:5 and 48:5 escape sequences.
  • 020b0d1 Update unit tests to account for the corrected interpretation of indexed colors.
  • beeb0c9 Make the GraphicsSingleTests clearer by explicitly initialising the expected attribute before overriding the foreground index.

📊 Changes

36 files changed (+328 additions, -376 deletions)

View changed files

📝 src/buffer/out/TextAttribute.cpp (+17 -2)
📝 src/buffer/out/TextAttribute.hpp (+12 -32)
📝 src/buffer/out/TextColor.cpp (+45 -22)
📝 src/buffer/out/TextColor.h (+14 -21)
📝 src/buffer/out/ut_textbuffer/TextColorTests.cpp (+4 -4)
📝 src/cascadia/TerminalCore/ITerminalApi.hpp (+2 -0)
📝 src/cascadia/TerminalCore/Terminal.hpp (+2 -0)
📝 src/cascadia/TerminalCore/TerminalApi.cpp (+16 -0)
📝 src/cascadia/TerminalCore/TerminalDispatchGraphics.cpp (+2 -2)
📝 src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp (+1 -2)
📝 src/host/VtIo.cpp (+4 -8)
📝 src/host/conattrs.cpp (+5 -61)
📝 src/host/getset.cpp (+4 -1)
📝 src/host/screenInfo.cpp (+4 -1)
📝 src/host/settings.cpp (+35 -60)
📝 src/host/settings.hpp (+3 -6)
📝 src/host/telemetry.cpp (+2 -2)
📝 src/host/ut_host/ConptyOutputTests.cpp (+1 -2)
📝 src/host/ut_host/ScreenBufferTests.cpp (+14 -10)
📝 src/host/ut_host/TextBufferIteratorTests.cpp (+1 -1)

...and 16 more files

📄 Description

This PR introduces a new ColorType to allow us to distinguish between
SGR indexed colors from the 16 color table, the lower half of which
can be brightened, and the ISO/ITU indexed colors from the 256 color
table, which have a fixed brightness. Retaining the distinction between
these two types will enable us to forward the correct SGR sequences to
conpty when addressing issue #2661.

The other benefit of retaining the color index (which we didn't
previously do for ISO/ITU colors) is that it ensures that the colors are
updated correctly when the color scheme is changed.

References

  • This is another step towards fixing the conpty narrowing bugs in issue
    #2661.
  • This is technically a fix for issue #5384, but that won't be apparent
    until #2661 is complete.

PR Checklist

Detailed Description of the Pull Request / Additional comments

The first part of this PR was the introduction of a new ColorType in
the TextColor class. Instead of just the one IsIndex type, there is
now an IsIndex16 and an IsIndex256. IsIndex16 covers the eight
original ANSI colors set with SGR 3x and SGR 4x, as well as the
brighter aixterm variants set with SGR 9x and SGR 10x. IsIndex256
covers the 256 ISO/ITU indexed colors set with SGR 38;5 and SGR 48;5.

There are two reasons for this distinction. The first is that the ANSI
colors have the potential to be brightened by the SGR 1 bold
attribute, while the ISO/ITO color do not. The second reason is that
when forwarding an attributes through conpty, we want to try and
preserve the original SGR sequence that generated each color (to the
extent that that is possible). By having the two separate types, we can
map the IsIndex16 colors back to ANSI/aixterm values, and IsIndex256
to the ISO/ITU sequences.

In addition to the VT colors, we also have to deal with the legacy
colors set by the Windows console APIs, but we don't really need a
separate type for those. It seemed most appropriate to me to store them
as IsIndex256 colors, since it doesn't make sense to have them
brightened by the SGR 1 attribute (which is what would happen if they
were stored as IsIndex16). If a console app wanted a bright color it
would have selected one, so we shouldn't be messing with that choice.

The second part of the PR was the unification of the two color tables.
Originally we had a 16 color table for the legacy colors, and a separate
table for the 256 ISO/ITU colors. These have now been merged into one,
so color table lookups no longer need to decide which of the two tables
they should be referencing. I've also updated all the methods that took
a color table as a parameter to use a basic_string_view instead of
separate pointer and length variables, which I think makes them a lot
easier and safer to work with.

With this new architecture in place, I could now update the
AdaptDispatch SGR implementation to store the ISO/ITU indexed colors
as IsIndex256 values, where before they were mapped to RGB values
(which prevented them reflecting any color scheme changes). I could also
update the TerminalDispatch implementation to differentiate between
the two index types, so that the SGR 1 brightening would only be
applied to the ANSI colors.

I've also done a bit of code refactoring to try and minimise any direct
access to the color tables, getting rid of a lot of places that were
copying tables with memmove operations. I'm hoping this will make it
easier for us to update the code in the future if we want to reorder the
table entries (which is likely a requirement for unifying the
AdaptDispatch and TerminalDispatch implementations).

Validation Steps Performed

For testing, I've just updated the existing unit tests to account for
the API changes. The TextColorTests required an extra parameter
specifying the index type when setting an index. And the AdapterTest
and ScreenBufferTests required the use of the new SetIndexedXXX
methods in order to be explicit about the index type, instead of relying
on the TextAttribute constructor and the old SetForeground and
SetBackground methods which didn't have a way to differentiate index
types.

I've manually tested the various console APIs
(SetConsoleTextAttribute, ReadConsoleOutputAttribute, and
ReadConsoleOutput), to make sure they are still setting and reading
the attributes as well as they used to. And I've tested the
SetConsoleScreenBufferInfoEx and GetConsoleScreenBufferInfoEx APIs
to make sure they can read and write the color table correctly. I've
also tested the color table in the properties dialog, made sure it was
saved and restored from the registry correctly, and similarly saved and
restored from a shortcut link.

Note that there are still a bunch of issues with the color table APIs,
but no new problems have been introduced by the changes in this PR, as
far as I could tell.

I've also done a bunch of manual tests of OSC 4 to make sure it's
updating all the colors correctly (at least in conhost), and confirmed
that the test case in issue #1223 now works as expected.

🔄 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/5834 **Author:** [@j4james](https://github.com/j4james) **Created:** 5/10/2020 **Status:** ✅ Merged **Merged:** 5/27/2020 **Merged by:** [@undefined](undefined) **Base:** `master` ← **Head:** `fix-indexed-colors` --- ### 📝 Commits (9) - [`d5aabb4`](https://github.com/microsoft/terminal/commit/d5aabb4e837ec073bcb9cbf77fa7bdf9ae76e9f3) Distinguish between indexed colors from the 16 color palette (which can be brightened), and the 256 color palette (which can't). - [`414c727`](https://github.com/microsoft/terminal/commit/414c727d4dae2c41fcad427b5affb3216f1a3c64) Make the terminal dispatcher use the 256 color index format for SGR 38:5 and 48:5 escape sequences. - [`6ca9015`](https://github.com/microsoft/terminal/commit/6ca9015ca236c1cd114373edf37862beea1ad87a) Update unit tests to account for the updated index color formats. - [`1814a58`](https://github.com/microsoft/terminal/commit/1814a58cc93d095d79523c3f495f670e779fa30f) Avoid directly copying the color table with memmove. - [`c06c575`](https://github.com/microsoft/terminal/commit/c06c57558f606d9da6bf0299b81943a6bd38ae79) Remove some unnecessary functions that made the code harder to follow. - [`a4c23a6`](https://github.com/microsoft/terminal/commit/a4c23a602d81e54cad33d4f0e7c11cfc6670fbaf) Combine the 16 and 256 color tables, and use string_views when passing the tables around. - [`f321051`](https://github.com/microsoft/terminal/commit/f321051ba7e5264a3c1f7f8e6cb66a75e04957a0) Make the conhost dispatcher use the 256 color index format for SGR 38:5 and 48:5 escape sequences. - [`020b0d1`](https://github.com/microsoft/terminal/commit/020b0d11d2a4bdcdcaa44a3fdd6ff28e87411788) Update unit tests to account for the corrected interpretation of indexed colors. - [`beeb0c9`](https://github.com/microsoft/terminal/commit/beeb0c92de5d42b8307ae99e7316cdaf404b01d8) Make the GraphicsSingleTests clearer by explicitly initialising the expected attribute before overriding the foreground index. ### 📊 Changes **36 files changed** (+328 additions, -376 deletions) <details> <summary>View changed files</summary> 📝 `src/buffer/out/TextAttribute.cpp` (+17 -2) 📝 `src/buffer/out/TextAttribute.hpp` (+12 -32) 📝 `src/buffer/out/TextColor.cpp` (+45 -22) 📝 `src/buffer/out/TextColor.h` (+14 -21) 📝 `src/buffer/out/ut_textbuffer/TextColorTests.cpp` (+4 -4) 📝 `src/cascadia/TerminalCore/ITerminalApi.hpp` (+2 -0) 📝 `src/cascadia/TerminalCore/Terminal.hpp` (+2 -0) 📝 `src/cascadia/TerminalCore/TerminalApi.cpp` (+16 -0) 📝 `src/cascadia/TerminalCore/TerminalDispatchGraphics.cpp` (+2 -2) 📝 `src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp` (+1 -2) 📝 `src/host/VtIo.cpp` (+4 -8) 📝 `src/host/conattrs.cpp` (+5 -61) 📝 `src/host/getset.cpp` (+4 -1) 📝 `src/host/screenInfo.cpp` (+4 -1) 📝 `src/host/settings.cpp` (+35 -60) 📝 `src/host/settings.hpp` (+3 -6) 📝 `src/host/telemetry.cpp` (+2 -2) 📝 `src/host/ut_host/ConptyOutputTests.cpp` (+1 -2) 📝 `src/host/ut_host/ScreenBufferTests.cpp` (+14 -10) 📝 `src/host/ut_host/TextBufferIteratorTests.cpp` (+1 -1) _...and 16 more files_ </details> ### 📄 Description This PR introduces a new `ColorType` to allow us to distinguish between `SGR` indexed colors from the 16 color table, the lower half of which can be brightened, and the ISO/ITU indexed colors from the 256 color table, which have a fixed brightness. Retaining the distinction between these two types will enable us to forward the correct `SGR` sequences to conpty when addressing issue #2661. The other benefit of retaining the color index (which we didn't previously do for ISO/ITU colors) is that it ensures that the colors are updated correctly when the color scheme is changed. ## References * This is another step towards fixing the conpty narrowing bugs in issue #2661. * This is technically a fix for issue #5384, but that won't be apparent until #2661 is complete. ## PR Checklist * [x] Closes #1223 * [x] CLA signed. * [x] Tests added/passed * [ ] Requires documentation to be updated * [x] I've discussed this with core contributors already. ## Detailed Description of the Pull Request / Additional comments The first part of this PR was the introduction of a new `ColorType` in the `TextColor` class. Instead of just the one `IsIndex` type, there is now an `IsIndex16` and an `IsIndex256`. `IsIndex16` covers the eight original ANSI colors set with `SGR 3x` and `SGR 4x`, as well as the brighter aixterm variants set with `SGR 9x` and `SGR 10x`. `IsIndex256` covers the 256 ISO/ITU indexed colors set with `SGR 38;5` and `SGR 48;5`. There are two reasons for this distinction. The first is that the ANSI colors have the potential to be brightened by the `SGR 1` bold attribute, while the ISO/ITO color do not. The second reason is that when forwarding an attributes through conpty, we want to try and preserve the original SGR sequence that generated each color (to the extent that that is possible). By having the two separate types, we can map the `IsIndex16` colors back to ANSI/aixterm values, and `IsIndex256` to the ISO/ITU sequences. In addition to the VT colors, we also have to deal with the legacy colors set by the Windows console APIs, but we don't really need a separate type for those. It seemed most appropriate to me to store them as `IsIndex256` colors, since it doesn't make sense to have them brightened by the `SGR 1` attribute (which is what would happen if they were stored as `IsIndex16`). If a console app wanted a bright color it would have selected one, so we shouldn't be messing with that choice. The second part of the PR was the unification of the two color tables. Originally we had a 16 color table for the legacy colors, and a separate table for the 256 ISO/ITU colors. These have now been merged into one, so color table lookups no longer need to decide which of the two tables they should be referencing. I've also updated all the methods that took a color table as a parameter to use a `basic_string_view` instead of separate pointer and length variables, which I think makes them a lot easier and safer to work with. With this new architecture in place, I could now update the `AdaptDispatch` SGR implementation to store the ISO/ITU indexed colors as `IsIndex256` values, where before they were mapped to RGB values (which prevented them reflecting any color scheme changes). I could also update the `TerminalDispatch` implementation to differentiate between the two index types, so that the `SGR 1` brightening would only be applied to the ANSI colors. I've also done a bit of code refactoring to try and minimise any direct access to the color tables, getting rid of a lot of places that were copying tables with `memmove` operations. I'm hoping this will make it easier for us to update the code in the future if we want to reorder the table entries (which is likely a requirement for unifying the `AdaptDispatch` and `TerminalDispatch` implementations). ## Validation Steps Performed For testing, I've just updated the existing unit tests to account for the API changes. The `TextColorTests` required an extra parameter specifying the index type when setting an index. And the `AdapterTest` and `ScreenBufferTests` required the use of the new `SetIndexedXXX` methods in order to be explicit about the index type, instead of relying on the `TextAttribute` constructor and the old `SetForeground` and `SetBackground` methods which didn't have a way to differentiate index types. I've manually tested the various console APIs (`SetConsoleTextAttribute`, `ReadConsoleOutputAttribute`, and `ReadConsoleOutput`), to make sure they are still setting and reading the attributes as well as they used to. And I've tested the `SetConsoleScreenBufferInfoEx` and `GetConsoleScreenBufferInfoEx` APIs to make sure they can read and write the color table correctly. I've also tested the color table in the properties dialog, made sure it was saved and restored from the registry correctly, and similarly saved and restored from a shortcut link. Note that there are still a bunch of issues with the color table APIs, but no new problems have been introduced by the changes in this PR, as far as I could tell. I've also done a bunch of manual tests of `OSC 4` to make sure it's updating all the colors correctly (at least in conhost), and confirmed that the test case in issue #1223 now works as expected. --- <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:16:23 +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#26488
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?