Consider adding an entry for the default bold color in the color table #11939
Comments
j4james
added
the
Issue-Feature
ghost
added
Needs-Triage
|
This seems entirely reasonable to me |
zadjii-msft
added
Area-VT
|
This sounds nice, could even have bold use the user's accent color to fit with the Windows GUI style. On this topic, it would be nice to have the ability to set a color scheme as a dual color scheme, with one use if the current GUI apps mode is Dark, and the other if it's Light. This would make it possible to have a color scheme with a dark or light background and all other colors tuned according to it, and have it match the other apps dark/light mode. |
Hmmm... I quite like that idea. Not sure how well it would work in practice, but it would interesting to try.
This is #4066. |
|
Is this suggesting that there would be ONE color for bold? "The SGR parameters 30–37 selected the foreground color, while 40–47 selected the background. Quite a few terminals implemented "bold" (SGR code 1) as a brighter color rather than a different font, thus providing 8 additional foreground colors." (https://en.wikipedia.org/wiki/ANSI_escape_code) I thought this behavior was sufficiently common/established that a lot of software uses the bold attribute to address the upper 8 colors. It's possible now to access them via 90-97, but how many programs are going to use the older 30-37;1 sequence? |
No. This is just for dealing with the bold version of the default foreground color (i.e. SGR 39). |
zadjii-msft
removed
the
Needs-Triage
## Summary of the Pull Request This PR moves the color table and related render settings, which are common to both conhost and Windows Terminal, into a shared class that can be accessed directly from the renderer. This avoids the overhead of having to look up these properties via the `IRenderData` interface, which relies on inefficient virtual function calls. This also introduces the concept of color aliases, which determine the position in the color table that colors like the default foreground and background are stored. This allows the option of mapping them to one of the standard 16 colors, or to have their own separate table entries. ## References This is a continuation of the color table refactoring started in #11602 and #11784. The color alias functionality is a prerequisite for supporting a default bold color as proposed in #11939. The color aliases could also be a way for us to replace the PowerShell color quirk for #6807. ## PR Checklist * [x] Closes #12002 * [x] CLA signed. * [ ] Tests added/passed * [ ] Documentation updated. * [ ] Schema updated. * [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx ## Detailed Description of the Pull Request / Additional comments In addition to the color table, this new `RenderSettings` class manages the blinking state, the code for adjusting indistinguishable colors, and various boolean properties used in the color calculations. These boolean properties are now stored in a `til::enumset` so they can all be managed through a single `SetRenderMode` API, and easily extended with additional modes that we're likely to need in the future. In Windows Terminal we have an instance of `RenderSettings` stored in the `Terminal` class, and in conhost it's stored in the `Settings` class. In both cases, a reference to this class is passed to the `Renderer` constructor, so it now has direct access to that data. The renderer can then pass that reference to the render engines where it's needed in the `UpdateDrawingBrushes` method. This means the renderer no longer needs the `IRenderData` interface to access the `GetAttributeColors`, `GetCursorColor`, or `IsScreenReversed` methods, so those have now been removed. We still need access to `GetAttributeColors` in certain accessibility code, though, so I've kept that method in the `IUIAData` interface, but the implementation just forwards to the `RenderSettings` class. The implementation of the `RenderSettings::GetAttributeColors` method is loosely based on the original `Terminal` code, only the `CalculateRgbColors` call has now been incorporated directly into the code. This let us deduplicate some bits that were previously repeated in the section for adjusting indistinguishable colors. The last steps, where we calculate the alpha components, have now been split in to a separate `GetAttributeColorsWithAlpha` method, since that's typically not needed. ## Validation Steps Performed There were quite a lot changes needed in the unit tests, but they're mostly straightforward replacements of one method call with another. In the `TextAttributeTests`, where we were previously testing the `CalculateRgbColors` method, we're now running those tests though `RenderSettings::GetAttributeColors`, which incorporates the same functionality. The only complication is when testing the `IntenseIsBright` option, that needs to be set with an additional `SetRenderMode` call where previously it was just a parameter on `CalculateRgbColors`. In the `ScreenBufferTests` and `TextBufferTests`, calls to `LookupAttributeColors` have again been replaced by the `RenderSettings::GetAttributeColors` method, which serves the same purpose, and calls to `IsScreenReversed` have been replaced with an appropriate `GetRenderMode` call. In the `VtRendererTests`, all the calls to `UpdateDrawingBrushes` now just need to be passed a reference to a `RenderSettings` instance.
Ahhh, thanks for mentioning this, I just spent the better part of an hour trying to figure out why on earth my colour scheme wasn't working 🤦♀️ Setting Foreground to be the same colour as White "fixes" it, but a discrete setting for Foreground and Foreground Bright would be stellar to see at some point. |
Description of the new feature/enhancement
At the moment, we when we need to render something using the default foreground color, but with the bold attribute, we "calculate" that brighter color on the fly. Not only does this add unnecessary overhead, but it doesn't even work when the default foreground doesn't match one of the first 8 ANSI colors.
If that bold color was precalculated, though, and stored in the color table, then it's just another index lookup. It would also gives us the architecture necessary to have the bold color part of the color scheme (#5682), and support the XTerm OSC 5 sequence for changing the bold color at runtime.
Finally, I think having bold in the color table is an essential prerequisite for getting the bold color to work when the option to Automaticaly adjust lightness of indistinguishable text is enabled (see #11917).
Proposed technical implementation details (optional)
One thing to note on point 4: In the current WT implementation we always treat the default colors as unique, and store them in the separate slots at the end of the table, although in many color schemes they exactly match entries in the first 8 slots. I believe we should really be setting their index to the matching ANSI color (and the bold index then being plus 8), similar to the typical conhost mapping.
One of the advantages of that approach is that it would make it clear when those colors are intended to be the same. For example, if the default background is the same color as ANSI black, then you'd expect a black foreground on a default background to be invisible, and you wouldn't want the algorithm that adjusts the lightness of indistinguishable text to try and make those colors different.
The text was updated successfully, but these errors were encountered: