Fix: Allow SGR color space ID 0 to be valid#19768
Conversation
|
Oh, nevermind - you're the one who filed the original bug. Of course it seems right--your expertise is unparalleled here |
| if (!hasColorSpaceId && red <= 255 && green <= 255 && blue <= 255) | ||
|
|
||
| // Allow the color if (No ID exists OR The ID is 0) AND colors are valid | ||
| bool validId = !hasColorSpaceId || (options.at(1).value() == 0); | ||
|
|
||
| if (validId && red <= 255 && green <= 255 && blue <= 255) |
There was a problem hiding this comment.
I think it might be simpler to just replace the hasColorSpaceId definition on line 131 with something like:
const bool validColorSpace = options.at(1).value_or(0) == 0;Then you wouldn't need to define anything else here, since you could just test that variable as is.
We also need to update the comment on lines 128..130 to reflect the new behavior.
|
Hello, thanks for the suggestion! That is much cleaner. I have updated the logic to match your snippet. (I applied this edit via the GitHub web editor as my local environment is currently rebuilding. I will keep an eye on the CI checks to ensure there are no formatting issues.) |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@Vallabh-1504 thanks so much for doing this! |
| // some applications that support non-standard ODA color sequence might | ||
| // send the red value in its place. | ||
| const bool hasColorSpaceId = options.at(1).has_value(); | ||
| const bool validColorSpace = options.at(1).value_or(0) == 0; |
There was a problem hiding this comment.
This should probably also work, and technically simpler for the CPU:
| const bool validColorSpace = options.at(1).value_or(0) == 0; | |
| const bool validColorSpace = options.at(1).value() <= 0; |
This is because we store unset parameters with a value of -1. If you check the definition of value_or(0) you'll see that it actually translates to _value < 0 ? 0 : _value. In practice this difference will not matter the slightest (<1ns).
There was a problem hiding this comment.
Eh. I prefer not leaking the implementation details of VTParameter
Summary of the Pull Request
This PR fixes an issue where SGR color sequences containing a Color Space ID of 0 (e.g., \e[38:2:0:r:g:bm) were incorrectly rejected as invalid.
References and Relevant Issues
Closes #19547
Detailed Description of the Pull Request / Additional comments
The existing logic in adaptDispatchGraphics.cpp treats any non-empty Color Space ID as an invalid sequence, intended to catch malformed sequences where the Red component might be shifted.
However, according to ITU T.416, the Color Space ID parameter is defined, and a value of 0 is implementation-defined. It is widely accepted by other terminal emulators (like XTerm) as the default RGB color space. This change modifies the check to explicitly allow an ID of 0 while still rejecting other non-standard IDs, ensuring compatibility with tools that emit fully qualified T.416 sequences.
Validation Steps Performed
Verified manually in WSL/Bash using printf to send the specific SGR sequence.
Test Command: printf "\e[38:2:0:255:0:0mRED\e[m\n"
Result: The text now correctly renders in Red instead of the default foreground color.
Before:
After:
PR Checklist