Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Simplify the color palette APIs #11768

Closed
j4james opened this issue Nov 16, 2021 · 1 comment · Fixed by #11784
Closed

Simplify the color palette APIs #11768

j4james opened this issue Nov 16, 2021 · 1 comment · Fixed by #11784
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Product-Conhost For issues in the Console codebase Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.

Comments

@j4james
Copy link
Collaborator

j4james commented Nov 16, 2021

Description of the new feature/enhancement

At the moment, the ConGetSet and ITerminalApi interfaces have a bunch of different methods for setting color palette entries. There's a SetColorTableEntry, a SetCursorColor, a SetDefaultForeground, and a SetDefaultBackground. If we want to support the OSC color queries (#3718), we're going to need to add equivalent getter methods for each of these colors. And at some point we'll probably want add support for things like the selection colors, and bold color (#5682), each of which will also require setter and getter methods.

My proposal is we simplify this to just two methods: SetColorTableEntry and GetColorTableEntry, which can then be used to access any of the configurable color values, and not just the 256-color table.

Proposed technical implementation details (optional)

The way this would work, is we'd extend the 256-color table with additional entries on the end for the special colors, and define a few constants to reference those entries.

So instead of calling a unique method like SetDefaultForeground(color), you'd just be doing something like SetColorTableEntry(TextColor::DEFAULT_FOREGROUJD, color).

One of the advantages of this approach, is that it simplifies the interpretation of the default colors in conhost. Right now a default color can either be an index in the color table, or a completely separate value. With this new approach it'll always be somewhere in the color table - you just need to store the actual index.

And note that support for index-based default colors is something that we'll need for VT525 emulation at some point, so in the long term it won't just be a conhost thing.

I also have a sneaking suspicion that this might help us get rid of the PowerShell color quirk (#6807), or at least simplify it, but I can't promise that.
@j4james j4james added the Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. label Nov 16, 2021
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Nov 16, 2021
@DHowett DHowett added Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. Product-Conhost For issues in the Console codebase labels Nov 16, 2021
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Nov 16, 2021
@j4james j4james mentioned this issue Nov 18, 2021
Merged
@ghost ghost added the In-PR This issue has a related PR label Nov 18, 2021
@ghost ghost closed this as completed in #11784 Nov 23, 2021
@ghost ghost removed the In-PR This issue has a related PR label Nov 23, 2021
ghost pushed a commit that referenced this issue Nov 23, 2021
@j4james
Consolidate the color palette APIs (#11784)
bb71179 
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
@ghost ghost added the Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. label Nov 23, 2021
@ghost
Copy link

ghost commented Feb 3, 2022

🎉This issue was addressed in #11784, which has now been successfully released as Windows Terminal Preview v1.13.10336.0.:tada:

Handy links:

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Product-Conhost For issues in the Console codebase Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Consolidate the color palette APIs j4james/terminal
2 participants
@DHowett @j4james