Standardize the color table order #11602
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghost
added
Area-Server
j4james
force-pushed
the
ansi-color-order
branch
2 times, most recently
from
356570b
to
853197c
Compare
j4james
force-pushed
the
ansi-color-order
branch
from
853197c
to
04f6ab2
Compare
|
In case anyone is waiting on this, I think it's ready for review now. I know it's quite a big change, but I think it's going to be important for future development. And I've done my best to make sure it doesn't break backwards compatibility, and doesn't have a negative impact on performance. |
zadjii-msft
approved these changes
Nov 1, 2021
ghost
requested review from
miniksa,
carlos-zamora,
DHowett,
leonMSFT,
PankajBhojwani and
lhecker
miniksa
approved these changes
Nov 4, 2021
| @@ -312,7 +312,7 @@ void TextAttributeTests::TestBoldAsBright() | |||
|
|
|||
| Log::Comment(L"When set to a bright color, and bold, 'bold is bright' changes nothing"); | |||
| attr.SetBold(true); | |||
| attr.SetIndexedForeground(8); | |||
| attr.SetIndexedForeground(TextColor::BRIGHT_BLACK); | |||
There was a problem hiding this comment.
Oh this is so much more sensible.
| @@ -96,6 +113,16 @@ struct TextColor | |||
|
|
|||
| COLORREF GetRGB() const noexcept; | |||
|
|
|||
| static constexpr BYTE TransposeLegacyIndex(const size_t index) | |||
| { | |||
| // When converting a 16-color index in the legacy Windows order to or | |||
| @@ -89,7 +89,6 @@ SOURCES = \ | |||
| ..\utf8ToWideCharParser.cpp \ | |||
| ..\conareainfo.cpp \ | |||
| ..\conimeinfo.cpp \ | |||
| ..\conattrs.cpp \ | |||
There was a problem hiding this comment.
Came by to double check this and you had it covered!
| const auto GetAttributeColors = std::bind(&CONSOLE_INFORMATION::LookupAttributeColors, &gci, std::placeholders::_1); | ||
| const auto defaultForeground = gci.GetDefaultForeground(); | ||
| const auto defaultBackground = gci.GetDefaultBackground(); | ||
| const auto GetAttributeColors = [=, &gci](const auto& attr) { |
There was a problem hiding this comment.
Thank you for torching the std::bind
Comment on lines
+224
to
+225
| const auto prefix = WI_IsFlagSet(index, FOREGROUND_INTENSITY) ? (fIsForeground ? 90 : 100) : (fIsForeground ? 30 : 40); | ||
| return _WriteFormatted(FMT_COMPILE("\x1b[{}m"), prefix + (index & 7)); |
miniksa
added
the
AutoMerge
|
Hello @miniksa! Because this pull request has the 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 (
|
ghost
pushed a commit
that referenced
this pull request
Nov 23, 2021
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
6 tasks
ghost
pushed a commit
that referenced
this pull request
Jan 13, 2022
## 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.
|
🎉 Handy links: |
ghost
mentioned this pull request
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary of the Pull Request
In the original implementation, we used two different orderings for the color tables. The WT color table used ANSI order, while the conhost color table used a Windows-specific order. This PR standardizes on the ANSI color order everywhere, so the usage of indexed colors is consistent across both parts of the code base, which will hopefully allow more of the code to be shared one day.
References
This is another small step towards de-duplicating
AdaptDispatchandTerminalDispatchfor issue #3849, and is essentially a followup to the SGR dispatch refactoring in PR #6728.PR Checklist
Detailed Description of the Pull Request / Additional comments
Conhost still needs to deal with legacy attributes using Windows color order, so those values now need to be transposed to ANSI colors order when creating a
TextAttributeobject. This is done with a simple mapping table, which also handles the translation of the default color entries, so it's actually slightly faster than the original code.And when converting
TextAttributevalues back to legacy console attributes, we were already using a mapping table to handle the narrowing of 256-color values down to 16 colors, so we just needed to adjust that table to account for the translation from ANSI to Windows, and then could make use of the same table for both 256-color and 16-color values.There are also a few places in conhost that read from or write to the color tables, and those now need to transpose the index values. I've addressed this by creating separate
SetLegacyColorTableEntryandGetLegacyColorTableEntrymethods in theSettingsclass which take care of the mapping, so it's now clearer in which cases the code is dealing with legacy values, and which are ANSI values.These methods are used in the
SetConsoleScreenBufferInfoExandGetConsoleScreenBufferInfoExAPIs, as well as a few place where color preferences are handled (the registry, shortcut links, and the properties dialog), none of which are particularly sensitive to performance. However, we also use the legacy table when looking up the default colors for rendering (which happens a lot), so I've refactored that code so the default color calculations now only occur once per frame.The plus side of all of this is that the VT code doesn't need to do the index translation anymore, so we can finally get rid of all the calls to
XTermToWindowsIndex, and we no longer need a separate color table initialization method for conhost, so I was able to merge a number of color initialization methods into one. We also no longer need to translate from legacy values to ANSI when generating VT sequences for conpty.The one exception to that is the 16-color VT renderer, which uses the
TextColor::GetLegacyIndexmethod to approximate 16-color equivalents for RGB and 256-color values. Since that method returns a legacy index, it still needs to be translated to ANSI before it can be used in a VT sequence. But this should be no worse than it was before.One more special case is conhost's secret Color Selection feature. That uses
Ctrl+Number andAlt+Number key sequences to highlight parts of the buffer, and the mapping from number to color is based on the Windows color order. So that mapping now needs to be transposed, but that's also not performance sensitive.The only thing that I haven't bothered to update is the trace logging code in the
Telemetryclass, which logs the first 16 entries in the color table. Those entries are now going to be in a different order, but I didn't think that would be of great concern to anyone.Validation Steps Performed
A lot of unit tests needed to be updated to use ANSI color constants when setting indexed colors, where before they might have been expecting values in Windows order. But this replaced a wild mix of different constants, sometimes having to use bit shifting, as well as values mapped with
XTermToWindowsIndex, so I think the tests are a whole lot clearer now. Only a few cases have been left with literal numbers where that seemed more appropriate.In addition to getting the unit tests working, I've also manually tested the behaviour of all the console APIs which I thought could be affected by these changes, and confirmed that they produced the same results in the new code as they did in the original implementation.
This includes:
WriteConsoleOutputReadConsoleOutputSetConsoleTextAttributewithWriteConsoleOutputCharacterFillConsoleOutputAttributeandFillConsoleOutputCharacterScrollConsoleScreenBufferGetConsoleScreenBufferInfoGetConsoleScreenBufferInfoExSetConsoleScreenBufferInfoExI've also manually tested changing colors via the console properties menu, the registry, and shortcut links, including setting default colors and popup colors. And I've tested that the "Quirks Mode" is still working as expected in PowerShell.
In terms of performance, I wrote a little test app that filled a 80x9999 buffer with random color combinations using
WriteConsoleOutput, which I figured was likely to be the most performance sensitive call, and I think it now actually performs slightly better than the original implementation.I've also tested similar code - just filling the visible window - with SGR VT sequences of various types, and the performance seems about the same as it was before.