Skip to content

Commit

Permalink
Add support for ITU's T.416 - ODA SGR (38/48) sequence (#15729)
Browse files Browse the repository at this point in the history
This PR adds support for **ITU's T.416 - ODA SGR (38/48)** colour
sequence, which makes use of colon instead of semi-colon as a parameter
separator.

- We use semi-colons as the only parameter separator while sending SGR
color sequences to a ConPTY client. This is to keep backward
compatibility.
- In response to `DECRQSS` query, we have decided to use colons, as the
major usecase for such queries are feature detection (whether client
supports ODA colours), and tracking the original separator may add too
much complexity to the codebase.

## Validation Steps Performed

- Made sure that we are always sending semi-colon separated parameters
regardless of whether the original sequence used colons.
- Made sure that we are always using colons as the parameter separator
in a `DECRQSS` response.
- Added new tests!

Closes #15706
  • Loading branch information
tusharsnx committed Jul 21, 2023
1 parent 7fba298 commit 3de496d
Show file tree
Hide file tree
Showing 5 changed files with 218 additions and 14 deletions.
5 changes: 4 additions & 1 deletion src/terminal/adapter/DispatchTypes.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,8 @@ namespace Microsoft::Console::VirtualTerminal

constexpr VTParameter at(const size_t index) const noexcept
{
return til::at(_subParams, index);
// If the index is out of range, we return a sub parameter with no value.
return index < _subParams.size() ? til::at(_subParams, index) : defaultParameter;
}

VTSubParameters subspan(const size_t offset, const size_t count) const noexcept
Expand All @@ -191,6 +192,8 @@ namespace Microsoft::Console::VirtualTerminal
}

private:
static constexpr VTParameter defaultParameter{};

std::span<const VTParameter> _subParams;
};

Expand Down
4 changes: 2 additions & 2 deletions src/terminal/adapter/adaptDispatch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4117,14 +4117,14 @@ void AdaptDispatch::_ReportSGRSetting() const
else if (color.IsIndex256())
{
const auto index = color.GetIndex();
fmt::format_to(std::back_inserter(response), FMT_COMPILE(L";{};5;{}"), base + 8, index);
fmt::format_to(std::back_inserter(response), FMT_COMPILE(L";{}:5:{}"), base + 8, index);
}
else if (color.IsRgb())
{
const auto r = GetRValue(color.GetRGB());
const auto g = GetGValue(color.GetRGB());
const auto b = GetBValue(color.GetRGB());
fmt::format_to(std::back_inserter(response), FMT_COMPILE(L";{};2;{};{};{}"), base + 8, r, g, b);
fmt::format_to(std::back_inserter(response), FMT_COMPILE(L";{}:2::{}:{}:{}"), base + 8, r, g, b);
}
};
addColor(30, attr.GetForeground());
Expand Down
9 changes: 6 additions & 3 deletions src/terminal/adapter/adaptDispatch.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -297,12 +297,15 @@ namespace Microsoft::Console::VirtualTerminal
size_t _SetRgbColorsHelper(const VTParameters options,
TextAttribute& attr,
const bool isForeground) noexcept;
void _SetRgbColorsHelperFromSubParams(const VTParameter colorItem,
const VTSubParameters options,
TextAttribute& attr) noexcept;
size_t _ApplyGraphicsOption(const VTParameters options,
const size_t optionIndex,
TextAttribute& attr) noexcept;
void _ApplyGraphicsOptionSubParam(const VTParameter option,
const VTSubParameters subParams,
TextAttribute& attr) noexcept;
void _ApplyGraphicsOptionWithSubParams(const VTParameter option,
const VTSubParameters subParams,
TextAttribute& attr) noexcept;
void _ApplyGraphicsOptions(const VTParameters options,
TextAttribute& attr) noexcept;

Expand Down
97 changes: 91 additions & 6 deletions src/terminal/adapter/adaptDispatchGraphics.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@ size_t AdaptDispatch::_SetRgbColorsHelper(const VTParameters options,
const size_t red = options.at(1).value_or(0);
const size_t green = options.at(2).value_or(0);
const size_t blue = options.at(3).value_or(0);
// ensure that each value fits in a byte
// 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 (red <= 255 && green <= 255 && blue <= 255)
{
const auto rgbColor = RGB(red, green, blue);
Expand All @@ -46,6 +47,9 @@ size_t AdaptDispatch::_SetRgbColorsHelper(const VTParameters options,
{
optionsConsumed = 2;
const size_t tableIndex = options.at(1).value_or(0);

// We only apply the color if the index value fit within a byte.
// This is to match XTerm's and VTE's behavior.
if (tableIndex <= 255)
{
const auto adjustedIndex = gsl::narrow_cast<BYTE>(tableIndex);
Expand All @@ -62,6 +66,77 @@ size_t AdaptDispatch::_SetRgbColorsHelper(const VTParameters options,
return optionsConsumed;
}

// 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:
// :5:<n>
// where 'n' is the index into the xterm color table.
const size_t tableIndex = options.at(1).value_or(0);

// We only apply the color if the index value fit within a byte.
// This is to match XTerm's and VTE's behavior.
if (tableIndex <= 255)
{
const auto adjustedIndex = gsl::narrow_cast<BYTE>(tableIndex);
if (isForeground)
{
attr.SetIndexedForeground256(adjustedIndex);
}
else
{
attr.SetIndexedBackground256(adjustedIndex);
}
}
}
}

// Routine Description:
// - Helper to apply a single graphic rendition option to an attribute.
// - Calls appropriate helper to apply the option with sub parameters when necessary.
Expand All @@ -80,7 +155,7 @@ size_t AdaptDispatch::_ApplyGraphicsOption(const VTParameters options,
if (options.hasSubParamsFor(optionIndex))
{
const auto subParams = options.subParamsFor(optionIndex);
_ApplyGraphicsOptionSubParam(opt, subParams, attr);
_ApplyGraphicsOptionWithSubParams(opt, subParams, attr);
return 1;
}

Expand Down Expand Up @@ -260,20 +335,30 @@ size_t AdaptDispatch::_ApplyGraphicsOption(const VTParameters options,
}

// Routine Description:
// - This is a no-op until we have something meaningful to do with sub parameters.
// - Helper to apply a single graphic rendition option with sub parameters to an attribute.
// Arguments:
// - option - An option to apply.
// - subParams - Sub parameters associated with the option.
// - attr - The attribute that will be updated with the applied option.
// Return Value:
// - <None>
void AdaptDispatch::_ApplyGraphicsOptionSubParam(const VTParameter /* option */,
const VTSubParameters /* subParam */,
TextAttribute& /* attr */) noexcept
void AdaptDispatch::_ApplyGraphicsOptionWithSubParams(const VTParameter option,
const VTSubParameters subParams,
TextAttribute& attr) noexcept
{
// here, we apply our "best effort" rule, while handling sub params if we don't
// recognise the parameter substring (parameter and it's sub parameters) then
// we should just skip over them.
switch (option)
{
case ForegroundExtended:
case BackgroundExtended:
_SetRgbColorsHelperFromSubParams(option, subParams, attr);
break;
default:
/* do nothing */
break;
}
}

// Routine Description:
Expand Down
117 changes: 115 additions & 2 deletions src/terminal/adapter/ut_adapter/adapterTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1699,7 +1699,7 @@ class AdapterTest
attribute.SetIndexedBackground256(45);
_testGetSet->_textBuffer->SetCurrentAttributes(attribute);
requestSetting(L"m");
_testGetSet->ValidateInputEvent(L"\033P1$r0;38;5;123;48;5;45m\033\\");
_testGetSet->ValidateInputEvent(L"\033P1$r0;38:5:123;48:5:45m\033\\");

Log::Comment(L"Requesting SGR attributes (ITU RGB colors).");
_testGetSet->PrepData();
Expand All @@ -1708,7 +1708,7 @@ class AdapterTest
attribute.SetBackground(RGB(65, 43, 21));
_testGetSet->_textBuffer->SetCurrentAttributes(attribute);
requestSetting(L"m");
_testGetSet->ValidateInputEvent(L"\033P1$r0;38;2;12;34;56;48;2;65;43;21m\033\\");
_testGetSet->ValidateInputEvent(L"\033P1$r0;38:2::12:34:56;48:2::65:43:21m\033\\");

Log::Comment(L"Requesting DECSCA attributes (unprotected).");
_testGetSet->PrepData();
Expand Down Expand Up @@ -2464,6 +2464,119 @@ class AdapterTest
rgOptions[4] = 123;
_testGetSet->_expectedAttribute.SetForeground(RGB(0, 0, 123));
VERIFY_IS_TRUE(_pDispatch->SetGraphicsRendition({ rgOptions, 5 }));

Log::Comment(L"Test 6: Ignore Rgb color when R, G or B is out of range (>255)");
_testGetSet->PrepData(); // default color from here is gray on black, FOREGROUND_BLUE | FOREGROUND_GREEN | FOREGROUND_RED
rgOptions[0] = DispatchTypes::GraphicsOptions::ForegroundExtended;
rgOptions[1] = DispatchTypes::GraphicsOptions::RGBColorOrFaint;
rgOptions[2] = 283;
rgOptions[3] = 182;
rgOptions[4] = 123;
// expect no change
_testGetSet->_expectedAttribute = TextAttribute{ FOREGROUND_BLUE | FOREGROUND_GREEN | FOREGROUND_RED };
VERIFY_IS_TRUE(_pDispatch->SetGraphicsRendition({ rgOptions, 5 }));

Log::Comment(L"Test 7: Ignore indexed color when index is out of range (>255)");
_testGetSet->PrepData(); // default color from here is gray on black, FOREGROUND_BLUE | FOREGROUND_GREEN | FOREGROUND_RED
rgOptions[0] = DispatchTypes::GraphicsOptions::ForegroundExtended;
rgOptions[1] = DispatchTypes::GraphicsOptions::BlinkOrXterm256Index;
rgOptions[2] = 283;
// expect no change
_testGetSet->_expectedAttribute = TextAttribute{ FOREGROUND_BLUE | FOREGROUND_GREEN | FOREGROUND_RED };
VERIFY_IS_TRUE(_pDispatch->SetGraphicsRendition({ rgOptions, 3 }));
}

TEST_METHOD(XtermExtendedSubParameterColorTest)
{
Log::Comment(L"Starting test...");

_testGetSet->PrepData(); // default color from here is gray on black, FOREGROUND_BLUE | FOREGROUND_GREEN | FOREGROUND_RED

VTParameter rgOptions[1];
VTParameter rgSubParamOpts[16];
std::pair<BYTE, BYTE> subParamRanges[1];

_testGetSet->_expectedAttribute = _testGetSet->_textBuffer->GetCurrentAttributes();

Log::Comment(L"Test 1: Change Indexed Foreground with missing index sub parameter");
rgOptions[0] = DispatchTypes::GraphicsOptions::ForegroundExtended;
rgSubParamOpts[0] = DispatchTypes::GraphicsOptions::BlinkOrXterm256Index;
subParamRanges[0] = { (BYTE)0, (BYTE)1 };
_testGetSet->_expectedAttribute.SetIndexedForeground256(TextColor::DARK_BLACK);
VERIFY_IS_TRUE(_pDispatch->SetGraphicsRendition({ rgOptions, rgSubParamOpts, subParamRanges }));

Log::Comment(L"Test 2: Change Indexed Background with default index sub parameter");
rgOptions[0] = DispatchTypes::GraphicsOptions::BackgroundExtended;
rgSubParamOpts[0] = DispatchTypes::GraphicsOptions::BlinkOrXterm256Index;
rgSubParamOpts[1] = {};
subParamRanges[0] = { (BYTE)0, (BYTE)2 };
_testGetSet->_expectedAttribute.SetIndexedBackground256(TextColor::DARK_BLACK);
VERIFY_IS_TRUE(_pDispatch->SetGraphicsRendition({ rgOptions, rgSubParamOpts, subParamRanges }));

Log::Comment(L"Test 3: Change RGB Foreground with all RGB sub parameters missing");
rgOptions[0] = DispatchTypes::GraphicsOptions::ForegroundExtended;
rgSubParamOpts[0] = DispatchTypes::GraphicsOptions::RGBColorOrFaint;
subParamRanges[0] = { (BYTE)0, (BYTE)1 };
_testGetSet->_expectedAttribute.SetForeground(RGB(0, 0, 0));
VERIFY_IS_TRUE(_pDispatch->SetGraphicsRendition({ rgOptions, rgSubParamOpts, subParamRanges }));

Log::Comment(L"Test 4: Change RGB Background with some missing RGB sub parameters");
rgOptions[0] = DispatchTypes::GraphicsOptions::BackgroundExtended;
rgSubParamOpts[0] = DispatchTypes::GraphicsOptions::RGBColorOrFaint;
rgSubParamOpts[1] = {}; // color-space-id
rgSubParamOpts[2] = 123;
subParamRanges[0] = { (BYTE)0, (BYTE)3 };
_testGetSet->_expectedAttribute.SetBackground(RGB(123, 0, 0));
VERIFY_IS_TRUE(_pDispatch->SetGraphicsRendition({ rgOptions, rgSubParamOpts, subParamRanges }));

Log::Comment(L"Test 5: Change RGB Foreground with some default RGB sub parameters");
rgOptions[0] = DispatchTypes::GraphicsOptions::ForegroundExtended;
rgSubParamOpts[0] = DispatchTypes::GraphicsOptions::RGBColorOrFaint;
rgSubParamOpts[1] = {}; // color-space-id
rgSubParamOpts[2] = {};
rgSubParamOpts[3] = {};
rgSubParamOpts[4] = 123;
subParamRanges[0] = { (BYTE)0, (BYTE)5 };
_testGetSet->_expectedAttribute.SetForeground(RGB(0, 0, 123));
VERIFY_IS_TRUE(_pDispatch->SetGraphicsRendition({ rgOptions, rgSubParamOpts, subParamRanges }));

Log::Comment(L"Test 6: Ignore color when ColorSpaceID is not empty");
_testGetSet->PrepData(); // default color from here is gray on black, FOREGROUND_BLUE | FOREGROUND_GREEN | FOREGROUND_RED
rgOptions[0] = DispatchTypes::GraphicsOptions::ForegroundExtended;
rgSubParamOpts[0] = DispatchTypes::GraphicsOptions::RGBColorOrFaint;
rgSubParamOpts[1] = 7; // color-space-id
rgSubParamOpts[2] = 182;
rgSubParamOpts[3] = 182;
rgSubParamOpts[4] = 123;
subParamRanges[0] = { (BYTE)0, (BYTE)5 };
// expect no change
_testGetSet->_expectedAttribute = TextAttribute{ FOREGROUND_BLUE | FOREGROUND_GREEN | FOREGROUND_RED };
VERIFY_IS_TRUE(_pDispatch->SetGraphicsRendition({ rgOptions, rgSubParamOpts, subParamRanges }));

Log::Comment(L"Test 7: Ignore Rgb color when R, G or B is out of range (>255)");
_testGetSet->PrepData(); // default color from here is gray on black, FOREGROUND_BLUE | FOREGROUND_GREEN | FOREGROUND_RED
rgOptions[0] = DispatchTypes::GraphicsOptions::BackgroundExtended;
rgSubParamOpts[0] = DispatchTypes::GraphicsOptions::RGBColorOrFaint;
rgSubParamOpts[1] = {}; // color-space-id
// Ensure r, g and b set a color that is different from current color.
// Otherwise, the test will pass even if the color is not ignored.
rgSubParamOpts[2] = 128;
rgSubParamOpts[3] = 283;
rgSubParamOpts[4] = 155;
subParamRanges[0] = { (BYTE)0, (BYTE)5 };
// expect no change
_testGetSet->_expectedAttribute = TextAttribute{ FOREGROUND_BLUE | FOREGROUND_GREEN | FOREGROUND_RED };
VERIFY_IS_TRUE(_pDispatch->SetGraphicsRendition({ rgOptions, rgSubParamOpts, subParamRanges }));

Log::Comment(L"Test 8: Ignore indexed color when index is out of range (>255)");
_testGetSet->PrepData(); // default color from here is gray on black, FOREGROUND_BLUE | FOREGROUND_GREEN | FOREGROUND_RED
rgOptions[0] = DispatchTypes::GraphicsOptions::ForegroundExtended;
rgSubParamOpts[0] = DispatchTypes::GraphicsOptions::BlinkOrXterm256Index;
rgSubParamOpts[1] = 283;
subParamRanges[0] = { (BYTE)0, (BYTE)2 };
// expect no change
_testGetSet->_expectedAttribute = TextAttribute{ FOREGROUND_BLUE | FOREGROUND_GREEN | FOREGROUND_RED };
VERIFY_IS_TRUE(_pDispatch->SetGraphicsRendition({ rgOptions, rgSubParamOpts, subParamRanges }));
}

TEST_METHOD(SetColorTableValue)
Expand Down

0 comments on commit 3de496d

Please sign in to comment.