Move the common render settings into a shared class #12127

j4james · 2022-01-10T00:30:56Z

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

Closes Introduce a shared class for managing render settings #12002
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.

miniksa

Awesome step forward. I admit that IRenderData was probably not my best idea ever, but oh well. Thanks as always @j4james.

miniksa · 2022-01-11T17:38:13Z

src/renderer/base/RenderSettings.cpp

+    SetColorAliasIndex(ColorAlias::DefaultForeground, TextColor::DARK_WHITE);
+    SetColorAliasIndex(ColorAlias::DefaultBackground, TextColor::DARK_BLACK);


I really love this idea!

color aliases are clever as. Thanks

miniksa · 2022-01-12T17:02:08Z

src/host/registry.cpp

@@ -250,17 +250,14 @@ void Registry::LoadFromRegistry(_In_ PCWSTR const pwszConsoleTitle)

    // Now load complex properties
    // Some properties shouldn't be filled by the registry if a copy already exists from the process start information.
-    DWORD dwValue;
+    const auto loadDWORD = [=](const auto valueName) {


Well I love it. But I do wonder if we should just be making it into a first-class function or template citizen instead of just a lambda. It sure cleans up the below calls.

Yeah, I did consider this, but pulling it out into a first-class function means you need to pass additional parameters that you get for free with the lambda, so it's not quite as clean. I do think perhaps there's potential for a nicer registry encapsulation class here that could clean up more of the code, but I didn't want to get too sidetracked in this PR, and the lambda at least doesn't make things a whole lot worse.

I almost broke down and worked on better-encapsulating the registry code some time a few years back when I first saw this file . . . so yeah, I get that for sure :D

miniksa · 2022-01-12T17:03:05Z

src/host/registry.cpp

    {
-        _pSettings->SetCodePage(dwValue);
+        _pSettings->SetCodePage(codePage.value());


I was going to say "what about the exception" but I guess the if has got you covered on that as it will test false. Nice.

miniksa · 2022-01-12T17:04:55Z

src/propslib/RegistrySerialization.cpp

@@ -61,10 +61,7 @@ const RegistrySerialization::_RegPropertyMap RegistrySerialization::s_PropertyMa
    { _RegPropertyType::Boolean,        CONSOLE_REGISTRY_INTERCEPTCOPYPASTE,            SET_FIELD_AND_SIZE(_fInterceptCopyPaste)         },
    { _RegPropertyType::Boolean,        CONSOLE_REGISTRY_TERMINALSCROLLING,             SET_FIELD_AND_SIZE(_TerminalScrolling)           },
    { _RegPropertyType::Dword,          CONSOLE_REGISTRY_USEDX,                         SET_FIELD_AND_SIZE(_fUseDx)                      },
-    { _RegPropertyType::Boolean,        CONSOLE_REGISTRY_COPYCOLOR,                     SET_FIELD_AND_SIZE(_fCopyColor)                  },
-    { _RegPropertyType::Dword,          CONSOLE_REGISTRY_DEFAULTFOREGROUND,             SET_FIELD_AND_SIZE(_colorTable[TextColor::DEFAULT_FOREGROUND]) },


Perhaps leave a comment that some of these have disappeared as the model has changed as I think this table was a semi-definitive list of what we were messing with in terms of values under HKCU\Console

OK, I've added a comment at the bottom of the list noting all the settings that are not included. There was already also a comment at the top noting that the list is incomplete.

lhecker

I have slightly mixed feelings about this PR. I can't quite put my finger on it, but I feel like there's some way to make our entire color handling a lot simpler and involve less classes. We have quite a lot of code to represent colors and metadata don't we? 😅

However this PR seems like the absolutely right approach anyways because it simplifies and "unifies" a ton of code. Nice! I left some thoughts you can consider before we merge this later.

src/buffer/out/TextColor.h

src/cascadia/TerminalCore/Terminal.cpp

src/renderer/base/RenderSettings.cpp

j4james · 2022-01-13T15:24:42Z

I have slightly mixed feelings about this PR. I can't quite put my finger on it, but I feel like there's some way to make our entire color handling a lot simpler and involve less classes.

Yeah, I feel the same way. I know this is far from perfect but I'm hoping it's at least moving us in the right direction, and we'll still be able to improve things over time.

I should add that my initial plan was to try add these properties directly to the renderer, so there wouldn't have been a need for another class, but that ended up being unworkable without essentially rewriting everything everywhere. So this was a bit of a compromise, which is not as neat as I would liked, but still achieved most of my goals.

j4james · 2022-01-13T17:42:11Z

FYI, I've merged my local branch with main to resolve the conflicts, but there are some tests that are failing now, and I'm still trying to establish the cause.

j4james · 2022-01-13T19:39:37Z

I think the merge is good. Test failures were the result of a previous commit. I've raised another issue for that (#12158).

ghost · 2022-01-13T21:09:14Z

Hello @lhecker!

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.

DHowett · 2022-01-13T21:50:30Z

src/renderer/base/RenderSettings.cpp

+//   color pair to the adjusted foreground for that color pair
+void RenderSettings::MakeAdjustedColorArray() noexcept
+{
+    // The color table has 16 colors, but the adjusted color table needs to be 18


FWIW I believe that we now calculate this for everyone, even if the feature isn't being used -- that might have a cost in conhost that we don't want to take on, right?

No, right now it's only being called from Terminal.cpp (here and here).

There is some slight overhead in the GetAttributeColors method testing the DistinguishableColors mode (see here), which won't be set in conhost, but I'm still hopeful we can get rid of that whole branch at some point.

I've tested this PR locally before approving it. I found no regressions. Binary size (without PGO) only increased ever so slightly (~500 Byte).

DHowett

(Post-merge review: I love it! I'm v. happy with this. Thank you!)

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

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

Handy links:

j4james added 12 commits January 9, 2022 23:47

Introduce a class for managing runtime render settings.

6d9c1d0

Use RenderSettings where possible in the renderer.

1006686

Integrate the RenderSettings into conhost.

474e35c

Update conhost unit tests.

98c01b3

Incorporate the color adjustment algorithm into RenderSettings.

dc1c9a9

Integrate the RenderSettings into Windows Terminal.

8a53a7d

Remove unused IRenderData methods.

72514c0

Remove unused BlinkingState class.

fbe4bae

Replace SetScreenMode in ConGetSet and ITerminalApi.

44ed8ed

Convert the "intense is bold" option into a mode.

2dbb6ee

Set the default color aliases in the dispatch class.

046ee30

Remove unused CalculateRgbColors method.

5c858fc

ghost added the Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. label Jan 10, 2022

Address audit issues in ColorFix class.

8c0b5c4

j4james marked this pull request as ready for review January 11, 2022 14:31

miniksa approved these changes Jan 12, 2022

View reviewed changes

miniksa assigned lhecker Jan 12, 2022

lhecker approved these changes Jan 13, 2022

View reviewed changes

src/buffer/out/TextColor.h Outdated Show resolved Hide resolved

src/cascadia/TerminalCore/Terminal.cpp Show resolved Hide resolved

src/renderer/base/RenderSettings.cpp Show resolved Hide resolved

j4james added 2 commits January 13, 2022 14:17

Make a note of the registry serialization special cases.

bbd5bac

Add an ENUM_COUNT for the ColorAlias enum.

b91196a

Merge branch 'main' into render-settings

a814203

zadjii-msft assigned j4james and unassigned lhecker Jan 13, 2022

lhecker added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jan 13, 2022

ghost merged commit 62c95b5 into microsoft:main Jan 13, 2022

DHowett reviewed Jan 13, 2022

View reviewed changes

j4james mentioned this pull request Jan 13, 2022

Disable being able to set the automatic adjustment of indistinguishable text #12160

Merged

6 tasks

j4james deleted the render-settings branch January 27, 2022 00:42

ghost mentioned this pull request Feb 3, 2022

Introduce a shared class for managing render settings #12002

Closed

j4james mentioned this pull request Nov 9, 2023

SGR 8 doesn't work when acrylic is enabled #11919

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move the common render settings into a shared class #12127

Move the common render settings into a shared class #12127

j4james commented Jan 10, 2022 •

edited

Loading

miniksa left a comment

miniksa Jan 11, 2022

DHowett Jan 13, 2022

miniksa Jan 12, 2022

j4james Jan 13, 2022

DHowett Jan 13, 2022

miniksa Jan 12, 2022

miniksa Jan 12, 2022

j4james Jan 13, 2022

lhecker left a comment

j4james commented Jan 13, 2022 •

edited

Loading

j4james commented Jan 13, 2022

j4james commented Jan 13, 2022

ghost commented Jan 13, 2022

DHowett Jan 13, 2022

j4james Jan 13, 2022

lhecker Jan 13, 2022

DHowett left a comment

ghost commented Feb 3, 2022

		SetColorAliasIndex(ColorAlias::DefaultForeground, TextColor::DARK_WHITE);
		SetColorAliasIndex(ColorAlias::DefaultBackground, TextColor::DARK_BLACK);

Move the common render settings into a shared class #12127

Move the common render settings into a shared class #12127

Conversation

j4james commented Jan 10, 2022 • edited Loading

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

miniksa 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

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

lhecker left a comment

Choose a reason for hiding this comment

j4james commented Jan 13, 2022 • edited Loading

j4james commented Jan 13, 2022

j4james commented Jan 13, 2022

ghost commented Jan 13, 2022

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.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

ghost commented Feb 3, 2022

j4james commented Jan 10, 2022 •

edited

Loading

j4james commented Jan 13, 2022 •

edited

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.