Add support for querying the DECCTR color table report #17708
Conversation
src/types/utils.cpp
Outdated
| // This calculation is based on the RGB to HSL algorithm described in | ||
| // Wikipedia: https://en.wikipedia.org/wiki/HSL_and_HSV#From_RGB | ||
| // We start by calculating the maximum and minimum component values. | ||
| const auto maxComp = std::max(std::max(red, green), blue); |
There was a problem hiding this comment.
huh, TIL that there's an initializer list version of max that returns the max-of-N (I looked it up when I was wondering if there was a shorter way to say this)
There was a problem hiding this comment.
Cool. I didn't know you could do that. I'm happy to make that change.
There was a problem hiding this comment.
FYI: In areas where performance is crucial, I recommend against using that overload as it depends on the quality of the STL implementation and compiler, which in this particular case is not great. Microsoft's STL chooses to use vectorized operations for this which cannot be inlined. This forces MSVC to allocate all arguments on the stack. Disabling the vectorization fixes that issue, but still causes MSVC to allocate the array on the stack anyway, as it has always struggled with optimizing function arguments from stack- to register-passing.
There was a problem hiding this comment.
Btw, I did actually check the generated assembly for the initializer list version before making this change, and it looked identical to me.
| // RGB(255,255,255) -> RGB(100%,100%,100%) | ||
| expectedResponse += L"15;2;100;100;100/"; | ||
| // Remaining slots are black, i.e. RGB(0%,0%,0%) | ||
| for (size_t i = 16; i < TextColor::TABLE_SIZE; i++) |
There was a problem hiding this comment.
for TABLE_SIZE: is it correct that we will emit the FRAME_BACKGROUND et al? Are those indices stable across terminal emulators?
There was a problem hiding this comment.
what if they are set to INVALID_COLOR?
There was a problem hiding this comment.
Of the terminals that support this, there is one which is a genuine terminal emulator, which only returns 16 colors (since that is what you would get on the original hardware terminals). The others that I know of return 256 colors, which includes the color cube and gray scale entries that are common to most modern terminals.
I wanted to include our entire color table, even though the indices aren't standard, because you shouldn't really need to care about that - it just provides a way to save and restore the entire set. However, I think the INVALID_COLOR entries are going to be a problem, because they're not going to restore correctly, and that could break things.
But maybe we can just drop any colors from the report that are INVALID_COLOR, and that way there's no risk of corrupting them with a simple save and restore that doesn't change anything. We won't then be able to restore any INVALID_COLOR entries that have been changed, but that's no worse than if we only reported the first 256.
|
Thanks! Excellent work as always, James |
Summary of the Pull Request
This PR introduces the framework for the
DECRQTSRsequence which isused to query terminal state reports. But for now I've just implemented
the
DECCTRcolor table report, which provides a way for applicationsto query the terminal's color scheme.
References and Relevant Issues
This is the counterpart to the the
DECRSTSsequence, which is used torestore a color table report. That was implemented in PR #13139, but it
only became practical to report the color table once conpty passthrough
was added in PR #17510.
Detailed Description of the Pull Request / Additional comments
This sequence has the option of reporting the colors as either HLS or
RGB, but in both cases the resolution is lower than 24 bits, so the
colors won't necessarily round-trip exactly when saving and restoring.
The HLS model in particular can accumulate rounding errors over time.
Validation Steps Performed
I've added a basic unit test that confirms the colors are reported as
expected for both color models. The color values in these tests were
obtained from color reports on a real VT525 terminal.
PR Checklist