Repository: microsoft/terminal
File: src/terminal/adapter/adaptDispatchGraphics.cpp (C++)

Code before:
//     - RGB sequences then take 4 MORE options to designate the ColorSpaceID, R, G, B parts
//       of the color.
//     - Xterm index will use the option that follows to use a color from the
//       preset 256 color xterm color table.
// Arguments:
// - colorItem - One of FG(38) and BG(48), indicating which color we're setting.
// - options - An array of options that will be used to generate the RGB color
// - attr - The attribute that will be updated with the parsed color.
// Return Value:
// - <none>
void AdaptDispatch::_SetRgbColorsHelperAlt(const VTParameter colorItem,
                                           const VTSubParameters options,
                                           TextAttribute& attr) noexcept
{
    // This should be called for applying FG and BG colors only.
    assert(colorItem == GraphicsOptions::ForegroundExtended ||
           colorItem == GraphicsOptions::BackgroundExtended);

    const bool isForeground = (colorItem == GraphicsOptions::ForegroundExtended);
    const DispatchTypes::GraphicsOptions typeOpt = options.at(0);

    if (typeOpt == DispatchTypes::GraphicsOptions::RGBColorOrFaint)
    {
        // sub params are in the order:
        // :2:<color-space-id>:<r>:<g>:<b>
        // we skip color-space-id, because we don't support it.
        const size_t red = options.at(2).value_or(0);
        const size_t green = options.at(3).value_or(0);
        const size_t blue = options.at(4).value_or(0);
        // ensure that each value fits in a byte
        if (red <= 255 && green <= 255 && blue <= 255)
        {
            const auto rgbColor = RGB(red, green, blue);
            attr.SetColor(rgbColor, isForeground);
        }
    }
    else if (typeOpt == DispatchTypes::GraphicsOptions::BlinkOrXterm256Index)
    {
        // sub params are in the order:
        // :5:<n>
        // where 'n' is the index into the xterm color table.
        const size_t tableIndex = options.at(1).value_or(0);
        if (tableIndex <= 255)
        {
            const auto adjustedIndex = gsl::narrow_cast<BYTE>(tableIndex);
            if (isForeground)
            {
                attr.SetIndexedForeground256(adjustedIndex);
            }
            else
            {

Review comment (bug):
I'm wondering whether maybe we should check the color space parameter here, and if it's anything other than "default", then we just don't apply that color.

The reason why I say that is that some terminals initially supported these sequences _without_ the color space parameter (in fact some probably still do). So if we receive a sequence like `2:12:34:56`, it's more likely to be an attempt to set RGB(12,34,56) rather than RGB(34,56,0).

If you're feeling very enthusiastic, we could actually support both forms (so both `2:12:34:56` and `2::12:34:56` produce the same result), but I'd be quite happy to just ignore anything with an "invalid" color space, and hope that discourages apps from using that format. I just don't want us to interpret those sequences as the _wrong_ color.

Code after:

// Routine Description:
// - Helper to parse extended graphics options, which start with 38 (FG) or 48 (BG)
//   - These options are followed by either a 2 (RGB) or 5 (xterm index):
//     - RGB sequences then take 4 MORE options to designate the ColorSpaceID, R, G, B parts
//       of the color.
//     - Xterm index will use the option that follows to use a color from the
//       preset 256 color xterm color table.
// Arguments:
// - colorItem - One of FG(38) and BG(48), indicating which color we're setting.
// - options - An array of options that will be used to generate the RGB color
// - attr - The attribute that will be updated with the parsed color.
// Return Value:
// - <none>
void AdaptDispatch::_SetRgbColorsHelperFromSubParams(const VTParameter colorItem,
                                                     const VTSubParameters options,
                                                     TextAttribute& attr) noexcept
{
    // This should be called for applying FG and BG colors only.
    assert(colorItem == GraphicsOptions::ForegroundExtended ||
           colorItem == GraphicsOptions::BackgroundExtended);

    const bool isForeground = (colorItem == GraphicsOptions::ForegroundExtended);
    const DispatchTypes::GraphicsOptions typeOpt = options.at(0);

    if (typeOpt == DispatchTypes::GraphicsOptions::RGBColorOrFaint)
    {
        // sub params are in the order:
        // :2:<color-space-id>:<r>:<g>:<b>

        // We treat a color as invalid, if it has a color space ID, as some
        // applications that support non-standard ODA color sequence may send
        // the red value in its place.
        const bool hasColorSpaceId = options.at(1).has_value();

        // Skip color-space-id.
        const size_t red = options.at(2).value_or(0);
        const size_t green = options.at(3).value_or(0);
        const size_t blue = options.at(4).value_or(0);

        // We only apply the color if the R, G, B values fit within a byte.
        // This is to match XTerm's and VTE's behavior.
        if (!hasColorSpaceId && red <= 255 && green <= 255 && blue <= 255)
        {
            const auto rgbColor = RGB(red, green, blue);
            attr.SetColor(rgbColor, isForeground);
        }
    }
    else if (typeOpt == DispatchTypes::GraphicsOptions::BlinkOrXterm256Index)
    {
        // sub params are in the order: