Consolidate the color palette APIs #11784

j4james · 2021-11-18T18:49:47Z

This PR merges the default colors and cursor color into the main color
table, enabling us to simplify the ConGetSet and ITerminalApi
interfaces, with just two methods required for getting and setting any
form of color palette entry.

The is a follow-up to the color table standardization in #11602, and a
another small step towards de-duplicating AdaptDispatch and
TerminalDispatch for issue #3849. It should also make it easier to
support color queries (#3718) and a configurable bold color (#5682) in
the future.

On the conhost side, default colors could originally be either indexed
positions in the 16-color table, or separate standalone RGB values. With
the new system, the default colors will always be in the color table, so
we just need to track their index positions.

To make this work, those positions need to be calculated at startup
based on the loaded registry/shortcut settings, and updated when
settings are changed (this is handled in
CalculateDefaultColorIndices). But the plus side is that it's now much
easier to lookup the default color values for rendering.

For now the default colors in Windows Terminal use hardcoded positions,
because it doesn't need indexed default colors like conhost. But in the
future I'd like to extend the index handling to both terminals, so we
can eventually support the VT525 indexed color operations.

As for the cursor color, that was previously stored in the Cursor
class, which meant that it needed to be copied around in various places
where cursors were being instantiated. Now that it's managed separately
in the color table, a lot of that code is no longer required.

Validation

Some of the unit test initialization code needed to be updated to setup
the color table and default index values as required for the new system.
There were also some adjustments needed to account for API changes, in
particular for methods that now take index values for the default colors
in place of COLORREFs. But for the most part, the essential behavior of
the tests remains unchanged.

I've also run a variety of manual tests looking at the legacy console
APIs as well as the various VT color sequences, and checking that
everything works as expected when color schemes are changed, both in
Windows Terminal and conhost, and in the latter case with both indexed
colors and RGB values.

Closes #11768

j4james · 2021-11-18T18:55:52Z

src/cascadia/UnitTests_TerminalCore/TerminalApiTest.cpp

-    VERIFY_IS_FALSE(term.SetColorTableEntry(256, 100));
    VERIFY_IS_FALSE(term.SetColorTableEntry(512, 100));


The 256 test is no longer applicable because the color table is now larger than 256, and I didn't want to leave another test case right on the edge when it's very likely to grow again. The 512 test case still covers the concept of an out-of-range value.

Long term I'd really like to get rid of these boolean error returns anyway, but that's a separate issue.

j4james · 2021-11-18T19:05:19Z

src/host/outputStream.cpp

+    // If we're setting the default foreground or background colors
+    // we need to make sure the index is correctly set as well.
+    if (tableIndex == TextColor::DEFAULT_FOREGROUND)
+    {
+        gci.SetDefaultForegroundIndex(TextColor::DEFAULT_FOREGROUND);
+    }
+    if (tableIndex == TextColor::DEFAULT_BACKGROUND)
+    {
+        gci.SetDefaultBackgroundIndex(TextColor::DEFAULT_BACKGROUND);
+    }


At some point I'm hoping this code might be moved up to the AdaptDispatch/TerminalDispatch level, since there are separate code paths for the default colors, which should be able to set these index values unconditionally. I just didn't want to overcomplicate the PR with the introduction of index APIs until we really need them.

Yeah I feel you there. There's going to be a lot more we want to share at that level going forward.

j4james · 2021-11-18T19:07:47Z

src/host/renderData.cpp

-    _defaultForeground = gci.GetDefaultForeground();
-    _defaultBackground = gci.GetDefaultBackground();


These values were just being "cached" here to optimise the LookupAttributeColors call below, but that's no longer needed now that the default index positions are essentially precalculated at startup.

j4james · 2021-11-18T19:09:36Z

src/host/screenInfo.cpp

-        altCursor.SetStyle(myCursor.GetSize(), myCursor.GetColor(), myCursor.GetType());
+        altCursor.SetStyle(myCursor.GetSize(), myCursor.GetType());


Now that the cursor color is stored in the color table, it doesn't need to be copied backwards and forwards between the alt buffer cursor and the main buffer - it's always the same everywhere.

j4james · 2021-11-18T19:15:44Z

src/host/selectionState.cpp

@@ -182,7 +181,6 @@ void Selection::_RestoreDataToCursor(Cursor& cursor) noexcept
 {
    cursor.SetSize(_ulSavedCursorSize);
    cursor.SetIsVisible(_fSavedCursorVisible);
-    cursor.SetColor(_savedCursorColor);


As with the alt buffer, there's no longer a need to save the cursor color here, because it's not going to be affected by the switch to a selection cursor.

I wonder what the purpose of this code was; it suggests that there's a different cursor during selection?

j4james · 2021-11-18T19:17:42Z

src/interactivity/win32/Clipboard.cpp

-    const auto defaultForeground = gci.GetDefaultForeground();
-    const auto defaultBackground = gci.GetDefaultBackground();
    const auto GetAttributeColors = [=, &gci](const auto& attr) {
-        return gci.LookupAttributeColors(attr, defaultForeground, defaultBackground);
+        return gci.LookupAttributeColors(attr);
    };


This is another case where we were attempting to optimise the LookupAttributeColors call, which is no longer necessary.

j4james · 2021-11-18T19:19:38Z

src/propslib/RegistrySerialization.cpp

-#define SET_FIELD_AND_SIZE(x) FIELD_OFFSET(Settings, x), RTL_FIELD_SIZE(Settings, x)
+#define SET_FIELD_AND_SIZE(x) UFIELD_OFFSET(Settings, x), RTL_FIELD_SIZE(Settings, x)


I'm not sure why this wasn't a problem before, but FIELD_OFFSET returns a LONG, which then has to be downcast to a DWORD, which the audit doesn't like. UFIELD_OFFSET returns a DWORD.

miniksa · 2021-11-23T17:57:56Z

src/cascadia/TerminalCore/ITerminalApi.hpp

-        virtual bool SetDefaultForeground(const DWORD color) noexcept = 0;
-        virtual bool SetDefaultBackground(const DWORD color) noexcept = 0;


I briefly thought "why not leave these in the header and just have them like instantly forward to the other one as a sort of windowsx.h shorthand so folks won't get too confused by having to pull a table index to adjust the defaults." but then I thought against it as it's really not that complicated to assume all colors are in the table.

miniksa

Great! I'm glad for the net negative in code as well. I think it makes sense to just have colors be all the problem of the table. Thanks as always!

DHowett

This is excellent, as always. Not a qualm in the world. 😄

Thanks for doing this!

As an aside, I was unable to figure out what the VT525 indexed color operations are, and I'm very curious 😄

DHowett · 2021-11-23T18:09:04Z

src/buffer/out/TextColor.h

+    static constexpr size_t DEFAULT_FOREGROUND = 256;
+    static constexpr size_t DEFAULT_BACKGROUND = 257;
+    static constexpr size_t CURSOR_COLOR = 258;
+    static constexpr size_t TABLE_SIZE = 259;


It's by design that TextColor can still only refer to colors 0-255 as indexed colors, right? We're not going to expand the definition of a TextColor to store default-ness as indices 256, 257?

Yeah, for now at least. I did think their might be some advantage to storing the default colors with their index already calculated (in which case we would need to increase the index range), but I'm not sure that would work correctly when the indexes are changed. I might reconsider this in the future though.

DHowett · 2021-11-23T18:17:35Z

src/host/selectionState.cpp

@@ -182,7 +181,6 @@ void Selection::_RestoreDataToCursor(Cursor& cursor) noexcept
 {
    cursor.SetSize(_ulSavedCursorSize);
    cursor.SetIsVisible(_fSavedCursorVisible);
-    cursor.SetColor(_savedCursorColor);


I wonder what the purpose of this code was; it suggests that there's a different cursor during selection?

ghost · 2021-11-23T18:28:43Z

Hello @zadjii-msft!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

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 (`@msftbot`) and give me an instruction to get started! Learn more here.

j4james · 2021-11-23T20:39:40Z

As an aside, I was unable to figure out what the VT525 indexed color operations are, and I'm very curious

The VT525 only has 16 palette entries, and the default colors are just indexed values within that set (much like the old Windows conhost). So to change one of the default colors, you'd lookup their index and change the palette entry at that position in the color table. Or if necessary, set the index to somewhere else in the table, and then update the palette entry for that new index. The sequence to set those index values is DECAC, and you can read their current values with a DECRQSS query.

Beyond that, though, they also had an "alternate color" mode, where each of the SGR renditions (bold, reverse, underline, etc) could be mapped to individual colors. So you could make bold displayed in green, and underline in red, etc. This was for compatibility with color terminals like the VT340 and VT240 which didn't support the SGR color sequences, but instead had hardcoded colors for the different SGR style renditions. On the VT525, those alternate color indexes could be set with the DECATC sequence.

And while the VT340 didn't allow you to set the index values directly, you could choose between a couple of different fixed mappings with the DECSTGLT sequence (on the VT240, that choice was made through the setup menu). I think the main reason for these mappings was to let you more closely emulate the earlier monochrome devices, which needed to map SGR renditions to different levels of brightness. In particular, the reverse attribute didn't just reverse foreground and background - it also had to alter the brightness levels to keep the text readable.

There are a few other sequences that also affect how this stuff works (e.g. DECATCUM and DECATCBM), but this is the gist of it. I've probably already told you more than you really wanted to know.😉

j4james · 2021-11-23T20:43:09Z

I wonder what the purpose of this code was; it suggests that there's a different cursor during selection?

Yeah, when you select the "Mark" edit menu (or Ctrl+M), it changes to a block cursor, and you can then set the selected area with the keyboard.

DHowett · 2021-11-23T21:12:23Z

told you more than you really wanted to know

as a self-professed sponge, I'm more than happy to know all of this and more. Thanks!

## 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.

ghost · 2022-02-03T18:23:30Z

🎉Windows Terminal Preview v1.13.10336.0 has been released which incorporates this pull request.:tada:

Handy links:

j4james added 8 commits November 18, 2021 10:55

Merge default colors into color table.

79859cf

Eliminate default color APIs in ConGetSet and ITerminalApi.

05e20f6

Standardize color table APIs in ConGetSet and ITerminalApi.

a8bad33

Use index values when referring to default colors.

96dd85f

Cache the calculation of default color indices.

6ebcb5e

Merge cursor color into color table.

7004ba7

Simplify registry queries for special colors.

a1e98f5

Silence the spelling bot.

69f2b4f

j4james commented Nov 18, 2021

View reviewed changes

miniksa reviewed Nov 23, 2021

View reviewed changes

miniksa approved these changes Nov 23, 2021

View reviewed changes

DHowett approved these changes Nov 23, 2021

View reviewed changes

zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Nov 23, 2021

ghost merged commit bb71179 into microsoft:main Nov 23, 2021

j4james mentioned this pull request Dec 7, 2021

The color scheme preview in the settings UI no longer works #11893

Closed

j4james mentioned this pull request Dec 21, 2021

Introduce a shared class for managing render settings #12002

Closed

j4james deleted the consolidate-palette-apis branch December 28, 2021 20:58

j4james mentioned this pull request Jan 10, 2022

Move the common render settings into a shared class #12127

Merged

6 tasks

ghost mentioned this pull request Feb 3, 2022

Simplify the color palette APIs #11768

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consolidate the color palette APIs #11784

Consolidate the color palette APIs #11784

j4james commented Nov 18, 2021 •

edited by DHowett

Loading

j4james Nov 18, 2021

j4james Nov 18, 2021

miniksa Nov 23, 2021

j4james Nov 18, 2021

j4james Nov 18, 2021

j4james Nov 18, 2021

DHowett Nov 23, 2021

j4james Nov 18, 2021

j4james Nov 18, 2021

miniksa Nov 23, 2021

miniksa left a comment

DHowett left a comment

DHowett Nov 23, 2021

j4james Nov 23, 2021

DHowett Nov 23, 2021

ghost commented Nov 23, 2021

j4james commented Nov 23, 2021

j4james commented Nov 23, 2021

DHowett commented Nov 23, 2021

ghost commented Feb 3, 2022

		VERIFY_IS_FALSE(term.SetColorTableEntry(256, 100));
		VERIFY_IS_FALSE(term.SetColorTableEntry(512, 100));

		_defaultForeground = gci.GetDefaultForeground();
		_defaultBackground = gci.GetDefaultBackground();

		altCursor.SetStyle(myCursor.GetSize(), myCursor.GetColor(), myCursor.GetType());
		altCursor.SetStyle(myCursor.GetSize(), myCursor.GetType());

		#define SET_FIELD_AND_SIZE(x) FIELD_OFFSET(Settings, x), RTL_FIELD_SIZE(Settings, x)
		#define SET_FIELD_AND_SIZE(x) UFIELD_OFFSET(Settings, x), RTL_FIELD_SIZE(Settings, x)

		virtual bool SetDefaultForeground(const DWORD color) noexcept = 0;
		virtual bool SetDefaultBackground(const DWORD color) noexcept = 0;

Consolidate the color palette APIs #11784

Consolidate the color palette APIs #11784

Conversation

j4james commented Nov 18, 2021 • edited by DHowett Loading

Validation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miniksa left a comment

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Nov 23, 2021

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 (@msftbot) and give me an instruction to get started! Learn more here.

j4james commented Nov 23, 2021

j4james commented Nov 23, 2021

DHowett commented Nov 23, 2021

ghost commented Feb 3, 2022

j4james commented Nov 18, 2021 •

edited by DHowett

Loading

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 (`@msftbot`) and give me an instruction to get started! Learn more here.