Introduce a shared class for managing render settings #12002
Labels
Issue-Feature
Complex enough to require an in depth planning process and actual budgeted, scheduled work.
Needs-Tag-Fix
Doesn't match tag requirements
Needs-Triage
It's a new issue that the core contributor team needs to triage at the next triage meeting
Resolution-Fix-Committed
Fix is checked in, but it might be 3-4 weeks until a release.
Comments
j4james
added
the
Issue-Feature
ghost
added
Needs-Triage
6 tasks
ghost
pushed a commit
that referenced
this issue
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.
ghost
added
Resolution-Fix-Committed
|
🎉This issue was addressed in #12127, which has now been successfully released as Handy links: |
This issue was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Description of the new feature/enhancement
This is the final stage of the color table refactoring that I started in #11602 and #11784. What I would like to do now is move the color table and related render settings into a new class that can be shared between conhost and Windows Terminal. This would deduplicate some code that is used in both places, and hopefully make things easier to extend in the future.
An additional benefit is that we can pass a reference to this class into the renderer, so the render engines have direct access to this information, instead of having to lookup colors and render modes via the
IRenderDatainterface. Considering the overhead of the virtual function calls, I'm hoping this should make things a little more efficient.Proposed technical implementation details (optional)
As I mentioned above, we would have a new class for maintaining what I refer to as
RenderSettings. Initially this would hold the color table, and the properties needed to calculate the colors for a givenTextAttribute(i.e. things like_screenReversed,_intenseIsBright,_adjustIndistinguishableColors,_blinkingState). Over time we can probably add more to it though.In Windows Terminal you'd have an instance of this class in the
Terminalclass, and in conhost it would be in theSettingsclass. In both cases, a reference to that instance would be passed in to theRendererclass when it's constructed. The renderer in turn can pass that on to the render engines when required.I'd also like to take this opportunity to introduce the concept of "render modes", and have the boolean properties like
_screenReversedand_intenseIsBrightstored in atil::enumset. This would make it easier to manage them via the VT mode sequences, and allow for the addition of more modes in the future without introducing a lot of boilerplate code.The other concept I'd like to introduce is that of "color aliases", which would manage the mapping of special colors like "default foreground" to index positions in the color table. For now this is only needed for the default foreground and background, but in time I'd like to add more (e.g. the bold color proposed in #11939) so it would be good to get a framework in place that's easy to extend.
The text was updated successfully, but these errors were encountered: