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

Improve the legacy color conversions #6358

Merged
10 commits merged into from
Jun 8, 2020
Merged

Improve the legacy color conversions #6358

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
b405a87
Eliminate unnecessary GetLegacyAttribute calls in the console IME.
j4james May 28, 2020
65155b7
Eliminate unnecessary GetLegacyAttribute use in the color selection h…
j4james May 28, 2020
a18dc5e
Eliminate unnecessary color remapping in the popup windows.
j4james May 28, 2020
a4d5aea
Why are popup attributes returned as a pointer?
j4james May 29, 2020
b315b7e
Simplify the null fill hack in ScrollRegion to avoid using GetLegacyA…
j4james May 30, 2020
9a2174e
Update ReadOutputAttributes to use the more correct GenerateLegacyAtt…
j4james May 30, 2020
e1864e8
Reimplement the legacy attribute generation with simple table lookups.
j4james Jun 1, 2020
f8b1479
Get rid of unused TextAttribute methods.
j4james Jun 2, 2020
dbc66b4
Add RRRGGGBB to spell-check whitelist.
j4james Jun 4, 2020
be5fd3f
Update the legacy color feature test to account for the different val…
j4james Jun 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/actions/spell-check/expect/expect.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1976,6 +1976,7 @@ robomac
roundtrip
ROWSTOSCROLL
RRF
RRRGGGBB
rtcore
RTEXT
rtf
Expand Down
33 changes: 14 additions & 19 deletions src/buffer/out/TextAttribute.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,24 @@
#include "TextAttribute.hpp"
#include "../../inc/conattrs.hpp"

bool TextAttribute::IsLegacy() const noexcept
// Routine Description:
// - Returns a WORD with legacy-style attributes for this textattribute.
// Parameters:
// - defaultAttributes: the attribute values to be used for default colors.
// Return value:
// - a WORD with legacy-style attributes for this textattribute.
WORD TextAttribute::GetLegacyAttributes(const WORD defaultAttributes) const noexcept
{
return _foreground.IsLegacy() && _background.IsLegacy();
const BYTE fgIndex = _foreground.GetLegacyIndex(defaultAttributes & FG_ATTRS);
const BYTE bgIndex = _background.GetLegacyIndex((defaultAttributes & BG_ATTRS) >> 4);
const WORD metaAttrs = _wAttrLegacy & META_ATTRS;
const bool brighten = _foreground.IsIndex16() && IsBold();
return fgIndex | (bgIndex << 4) | metaAttrs | (brighten ? FOREGROUND_INTENSITY : 0);
}

bool TextAttribute::IsHighColor() const noexcept
bool TextAttribute::IsLegacy() const noexcept
{
return _foreground.IsHighColor() || _background.IsHighColor();
return _foreground.IsLegacy() && _background.IsLegacy();
}

// Arguments:
Expand Down Expand Up @@ -241,21 +251,6 @@ void TextAttribute::SetDefaultBackground() noexcept
_background = TextColor();
}

// Method Description:
// - Returns true if this attribute indicates its foreground is the "default"
// foreground. Its _rgbForeground will contain the actual value of the
// default foreground. If the default colors are ever changed, this method
// should be used to identify attributes with the default fg value, and
// update them accordingly.
// Arguments:
// - <none>
// Return Value:
// - true iff this attribute indicates it's the "default" foreground color.
bool TextAttribute::ForegroundIsDefault() const noexcept
{
return _foreground.IsDefault();
}

// Method Description:
// - Returns true if this attribute indicates its background is the "default"
// background. Its _rgbBackground will contain the actual value of the
Expand Down
35 changes: 1 addition & 34 deletions src/buffer/out/TextAttribute.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,38 +59,7 @@ class TextAttribute final
{
}

WORD GetLegacyAttributes() const noexcept
{
const BYTE fg = (_foreground.GetIndex() & FG_ATTRS);
const BYTE bg = (_background.GetIndex() << 4) & BG_ATTRS;
const WORD meta = (_wAttrLegacy & META_ATTRS);
const bool brighten = _foreground.IsIndex16() && IsBold();
return (fg | bg | meta) | (brighten ? FOREGROUND_INTENSITY : 0);
}

// Method Description:
// - Returns a WORD with legacy-style attributes for this textattribute.
// If either the foreground or background of this textattribute is not
// a legacy attribute, then instead use the provided default index as
// the value for that component.
// Arguments:
// - defaultFgIndex: the BYTE to use as the index for the foreground, should
// the foreground not be a legacy style attribute.
// - defaultBgIndex: the BYTE to use as the index for the background, should
// the background not be a legacy style attribute.
// Return Value:
// - a WORD with legacy-style attributes for this textattribute.
WORD GetLegacyAttributes(const BYTE defaultFgIndex,
const BYTE defaultBgIndex) const noexcept
{
const BYTE fgIndex = _foreground.IsLegacy() ? _foreground.GetIndex() : defaultFgIndex;
const BYTE bgIndex = _background.IsLegacy() ? _background.GetIndex() : defaultBgIndex;
const BYTE fg = (fgIndex & FG_ATTRS);
const BYTE bg = (bgIndex << 4) & BG_ATTRS;
const WORD meta = (_wAttrLegacy & META_ATTRS);
const bool brighten = _foreground.IsIndex16() && IsBold();
return (fg | bg | meta) | (brighten ? FOREGROUND_INTENSITY : 0);
}
WORD GetLegacyAttributes(const WORD defaultAttributes = 0x07) const noexcept;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note to self and future reviewers: we don't have a constant for this, we just use 0x07 everywhere. Not worth making one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK.


COLORREF CalculateRgbForeground(std::basic_string_view<COLORREF> colorTable,
COLORREF defaultFgColor,
Expand Down Expand Up @@ -119,7 +88,6 @@ class TextAttribute final
friend constexpr bool operator!=(const WORD& legacyAttr, const TextAttribute& attr) noexcept;

bool IsLegacy() const noexcept;
bool IsHighColor() const noexcept;
bool IsBold() const noexcept;
bool IsItalic() const noexcept;
bool IsBlinking() const noexcept;
Expand Down Expand Up @@ -149,7 +117,6 @@ class TextAttribute final
void SetDefaultForeground() noexcept;
void SetDefaultBackground() noexcept;

bool ForegroundIsDefault() const noexcept;
bool BackgroundIsDefault() const noexcept;

void SetStandardErase() noexcept;
Expand Down
82 changes: 77 additions & 5 deletions src/buffer/out/TextColor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,57 @@
#include "precomp.h"
#include "TextColor.h"

// clang-format off

// A table mapping 8-bit RGB colors, in the form RRRGGGBB,
// down to one of the 16 colors in the legacy palette.
constexpr std::array<BYTE, 256> CompressedRgbToIndex16 = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is clever as heck. I love it.

0, 1, 1, 9, 0, 0, 1, 1, 2, 1, 1, 1, 2, 8, 1, 9,
2, 2, 3, 3, 2, 2, 11, 3, 10, 10, 11, 11, 10, 10, 10, 11,
0, 5, 1, 1, 0, 0, 1, 1, 8, 1, 1, 1, 2, 8, 1, 9,
2, 2, 3, 3, 2, 2, 11, 3, 10, 10, 10, 11, 10, 10, 10, 11,
5, 5, 5, 1, 4, 5, 1, 1, 8, 8, 1, 9, 2, 8, 9, 9,
2, 2, 3, 3, 2, 2, 11, 3, 10, 10, 11, 11, 10, 10, 10, 11,
4, 5, 5, 1, 4, 5, 5, 1, 8, 5, 5, 1, 8, 8, 9, 9,
2, 2, 8, 9, 10, 2, 11, 3, 10, 10, 11, 11, 10, 10, 10, 11,
4, 13, 5, 5, 4, 13, 5, 5, 4, 13, 13, 13, 6, 8, 13, 9,
6, 8, 8, 9, 10, 10, 11, 3, 10, 10, 11, 11, 10, 10, 10, 11,
4, 13, 13, 13, 4, 13, 13, 13, 4, 12, 13, 13, 6, 12, 13, 13,
6, 6, 8, 9, 6, 6, 7, 7, 10, 14, 14, 7, 10, 10, 14, 11,
4, 12, 13, 13, 4, 12, 13, 13, 4, 12, 13, 13, 6, 12, 12, 13,
6, 6, 12, 7, 6, 6, 7, 7, 6, 14, 14, 7, 14, 14, 14, 15,
12, 12, 13, 13, 12, 12, 13, 13, 12, 12, 12, 13, 12, 12, 12, 13,
6, 12, 12, 7, 6, 6, 7, 7, 6, 14, 14, 7, 14, 14, 14, 15
};

// A table mapping indexed colors from the 256-color palette,
// down to one of the 16 colors in the legacy palette.
constexpr std::array<BYTE, 256> Index256ToIndex16 = {
0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15,
0, 1, 1, 1, 9, 9, 2, 1, 1, 1, 1, 1, 2, 2, 3, 3,
3, 3, 2, 2, 11, 11, 3, 3, 10, 10, 11, 11, 11, 11, 10, 10,
10, 10, 11, 11, 5, 5, 5, 5, 1, 1, 8, 8, 1, 1, 9, 9,
2, 2, 3, 3, 3, 3, 2, 2, 11, 11, 3, 3, 10, 10, 11, 11,
11, 11, 10, 10, 10, 10, 11, 11, 4, 13, 5, 5, 5, 5, 4, 13,
13, 13, 13, 13, 6, 8, 8, 8, 9, 9, 10, 10, 11, 11, 3, 3,
10, 10, 11, 11, 11, 11, 10, 10, 10, 10, 11, 11, 4, 13, 13, 13,
13, 13, 4, 12, 13, 13, 13, 13, 6, 6, 8, 8, 9, 9, 6, 6,
7, 7, 7, 7, 10, 14, 14, 14, 7, 7, 10, 10, 14, 14, 11, 11,
4, 12, 13, 13, 13, 13, 4, 12, 13, 13, 13, 13, 6, 6, 12, 12,
7, 7, 6, 6, 7, 7, 7, 7, 6, 14, 14, 14, 7, 7, 14, 14,
14, 14, 15, 15, 12, 12, 13, 13, 13, 13, 12, 12, 12, 12, 13, 13,
6, 12, 12, 12, 7, 7, 6, 6, 7, 7, 7, 7, 6, 14, 14, 14,
7, 7, 14, 14, 14, 14, 15, 15, 0, 0, 0, 0, 0, 0, 8, 8,
8, 8, 8, 8, 8, 8, 8, 8, 7, 7, 7, 7, 7, 7, 15, 15
};

// clang-format on

bool TextColor::IsLegacy() const noexcept
{
return IsIndex16() || (IsIndex256() && _index < 16);
}

bool TextColor::IsHighColor() const noexcept
{
return IsRgb() || (IsIndex256() && _index >= 16);
}

bool TextColor::IsIndex16() const noexcept
{
return _meta == ColorType::IsIndex16;
Expand Down Expand Up @@ -135,6 +176,37 @@ COLORREF TextColor::GetColor(std::basic_string_view<COLORREF> colorTable,
}
}

// Method Description:
// - Return a legacy index value that best approximates this color.
// Arguments:
// - defaultIndex: The index to use for a default color.
// Return Value:
// - an index into the 16-color table
BYTE TextColor::GetLegacyIndex(const BYTE defaultIndex) const noexcept
{
if (IsDefault())
{
return defaultIndex;
}
else if (IsIndex16())
{
return GetIndex();
}
else if (IsIndex256())
{
return Index256ToIndex16.at(GetIndex());
}
else
{
// We compress the RGB down to an 8-bit value and use that to
// lookup a representative 16-color index from a hard-coded table.
const BYTE compressedRgb = (_red & 0b11100000) +
((_green >> 3) & 0b00011100) +
((_blue >> 6) & 0b00000011);
return CompressedRgbToIndex16.at(compressedRgb);
}
}

// Method Description:
// - Return a COLORREF containing our stored value. Will return garbage if this
//attribute is not a RGB attribute.
Expand Down
3 changes: 2 additions & 1 deletion src/buffer/out/TextColor.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,6 @@ struct TextColor
friend constexpr bool operator!=(const TextColor& a, const TextColor& b) noexcept;

bool IsLegacy() const noexcept;
bool IsHighColor() const noexcept;
bool IsIndex16() const noexcept;
bool IsIndex256() const noexcept;
bool IsDefault() const noexcept;
Expand All @@ -92,6 +91,8 @@ struct TextColor
const COLORREF defaultColor,
const bool brighten) const noexcept;

BYTE GetLegacyIndex(const BYTE defaultIndex) const noexcept;

constexpr BYTE GetIndex() const noexcept
{
return _index;
Expand Down
17 changes: 0 additions & 17 deletions src/host/cmdline.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,23 +26,6 @@

#pragma hdrstop
using Microsoft::Console::Interactivity::ServiceLocator;
// Routine Description:
// - This routine is called when the user changes the screen/popup colors.
// - It goes through the popup structures and changes the saved contents to reflect the new screen/popup colors.
void CommandLine::UpdatePopups(const TextAttribute& NewAttributes,
const TextAttribute& NewPopupAttributes,
const TextAttribute& OldAttributes,
const TextAttribute& OldPopupAttributes)
{
for (auto& popup : _popups)
{
try
{
popup->UpdateStoredColors(NewAttributes, NewPopupAttributes, OldAttributes, OldPopupAttributes);
}
CATCH_LOG();
}
}

// Routine Description:
// - This routine validates a string buffer and returns the pointers of where the strings start within the buffer.
Expand Down
5 changes: 0 additions & 5 deletions src/host/cmdline.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,11 +84,6 @@ class CommandLine
bool HasPopup() const noexcept;
Popup& GetPopup();

void UpdatePopups(const TextAttribute& NewAttributes,
const TextAttribute& NewPopupAttributes,
const TextAttribute& OldAttributes,
const TextAttribute& OldPopupAttributes);

void EndCurrentPopup();
void EndAllPopups();

Expand Down
8 changes: 4 additions & 4 deletions src/host/conareainfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ ConversionAreaBufferInfo::ConversionAreaBufferInfo(const COORD coordBufferSize)

ConversionAreaInfo::ConversionAreaInfo(const COORD bufferSize,
const COORD windowSize,
const CHAR_INFO fill,
const CHAR_INFO popupFill,
const TextAttribute& fill,
const TextAttribute& popupFill,
const FontInfo fontInfo) :
_caInfo{ bufferSize },
_isHidden{ true },
Expand All @@ -37,8 +37,8 @@ ConversionAreaInfo::ConversionAreaInfo(const COORD bufferSize,
THROW_IF_NTSTATUS_FAILED(SCREEN_INFORMATION::CreateInstance(windowSize,
fontInfo,
bufferSize,
TextAttribute{ fill.Attributes },
TextAttribute{ popupFill.Attributes },
fill,
popupFill,
0,
&pNewScreen));

Expand Down
4 changes: 2 additions & 2 deletions src/host/conareainfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,8 @@ class ConversionAreaInfo final
public:
ConversionAreaInfo(const COORD bufferSize,
const COORD windowSize,
const CHAR_INFO fill,
const CHAR_INFO popupFill,
const TextAttribute& fill,
const TextAttribute& popupFill,
const FontInfo fontInfo);
~ConversionAreaInfo() = default;
ConversionAreaInfo(const ConversionAreaInfo&) = delete;
Expand Down
6 changes: 2 additions & 4 deletions src/host/conimeinfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,11 +146,9 @@ void ConsoleImeInfo::ClearAllAreas()

const COORD windowSize = gci.GetActiveOutputBuffer().GetViewport().Dimensions();

CHAR_INFO fill;
fill.Attributes = gci.GetActiveOutputBuffer().GetAttributes().GetLegacyAttributes();
const TextAttribute fill = gci.GetActiveOutputBuffer().GetAttributes();

CHAR_INFO popupFill;
popupFill.Attributes = gci.GetActiveOutputBuffer().GetPopupAttributes()->GetLegacyAttributes();
const TextAttribute popupFill = gci.GetActiveOutputBuffer().GetPopupAttributes();

const FontInfo& fontInfo = gci.GetActiveOutputBuffer().GetCurrentFont();

Expand Down
20 changes: 10 additions & 10 deletions src/host/ft_host/API_RgbColorTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,21 +320,21 @@ BOOL Validate256GridToLegacy(COORD cursorPosInitial)

// Verify some other locations in the table, that will be RGB->Legacy conversions.
VERIFY_ARE_EQUAL(GetGridAttrs(1, 1, rOutputBuffer, actualWriteSize), MakeAttribute(0x1, 0x1));
VERIFY_ARE_EQUAL(GetGridAttrs(2, 1, rOutputBuffer, actualWriteSize), MakeAttribute(0xB, 0xB));
VERIFY_ARE_EQUAL(GetGridAttrs(3, 3, rOutputBuffer, actualWriteSize), MakeAttribute(0xB, 0xB));
VERIFY_ARE_EQUAL(GetGridAttrs(2, 2, rOutputBuffer, actualWriteSize), MakeAttribute(0x2, 0x2));
VERIFY_ARE_EQUAL(GetGridAttrs(2, 3, rOutputBuffer, actualWriteSize), MakeAttribute(0x3, 0x3));
VERIFY_ARE_EQUAL(GetGridAttrs(3, 4, rOutputBuffer, actualWriteSize), MakeAttribute(0x4, 0x4));
VERIFY_ARE_EQUAL(GetGridAttrs(2, 1, rOutputBuffer, actualWriteSize), MakeAttribute(0x3, 0x3));
VERIFY_ARE_EQUAL(GetGridAttrs(5, 8, rOutputBuffer, actualWriteSize), MakeAttribute(0x4, 0x4));
VERIFY_ARE_EQUAL(GetGridAttrs(3, 5, rOutputBuffer, actualWriteSize), MakeAttribute(0x5, 0x5));
VERIFY_ARE_EQUAL(GetGridAttrs(4, 5, rOutputBuffer, actualWriteSize), MakeAttribute(0x9, 0x9));
VERIFY_ARE_EQUAL(GetGridAttrs(4, 6, rOutputBuffer, actualWriteSize), MakeAttribute(0x6, 0x6));
VERIFY_ARE_EQUAL(GetGridAttrs(4, 7, rOutputBuffer, actualWriteSize), MakeAttribute(0x7, 0x7));
VERIFY_ARE_EQUAL(GetGridAttrs(6, 8, rOutputBuffer, actualWriteSize), MakeAttribute(0x9, 0x9));
VERIFY_ARE_EQUAL(GetGridAttrs(8, 8, rOutputBuffer, actualWriteSize), MakeAttribute(0x6, 0x6));
VERIFY_ARE_EQUAL(GetGridAttrs(9, 8, rOutputBuffer, actualWriteSize), MakeAttribute(0x7, 0x7));
VERIFY_ARE_EQUAL(GetGridAttrs(3, 11, rOutputBuffer, actualWriteSize), MakeAttribute(0x8, 0x8));
VERIFY_ARE_EQUAL(GetGridAttrs(3, 12, rOutputBuffer, actualWriteSize), MakeAttribute(0x1, 0x1));
VERIFY_ARE_EQUAL(GetGridAttrs(4, 12, rOutputBuffer, actualWriteSize), MakeAttribute(0xA, 0xA));
VERIFY_ARE_EQUAL(GetGridAttrs(5, 12, rOutputBuffer, actualWriteSize), MakeAttribute(0xD, 0xD));
VERIFY_ARE_EQUAL(GetGridAttrs(10, 12, rOutputBuffer, actualWriteSize), MakeAttribute(0xE, 0xE));
VERIFY_ARE_EQUAL(GetGridAttrs(10, 13, rOutputBuffer, actualWriteSize), MakeAttribute(0xC, 0xC));
VERIFY_ARE_EQUAL(GetGridAttrs(11, 13, rOutputBuffer, actualWriteSize), MakeAttribute(0xF, 0xF));
VERIFY_ARE_EQUAL(GetGridAttrs(8, 1, rOutputBuffer, actualWriteSize), MakeAttribute(0xD, 0xD));
VERIFY_ARE_EQUAL(GetGridAttrs(12, 0, rOutputBuffer, actualWriteSize), MakeAttribute(0xE, 0xE));
VERIFY_ARE_EQUAL(GetGridAttrs(12, 4, rOutputBuffer, actualWriteSize), MakeAttribute(0xC, 0xC));
VERIFY_ARE_EQUAL(GetGridAttrs(12, 3, rOutputBuffer, actualWriteSize), MakeAttribute(0xF, 0xF));

// Greyscale ramp
VERIFY_ARE_EQUAL(GetGridAttrs(14, 8, rOutputBuffer, actualWriteSize), MakeAttribute(0x0, 0x0));
Expand Down
9 changes: 6 additions & 3 deletions src/host/output.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,16 +148,19 @@ std::vector<WORD> ReadOutputAttributes(const SCREEN_INFORMATION& screenInfo,
// While we haven't read enough cells yet and the iterator is still valid (hasn't reached end of buffer)
while (amountRead < amountToRead && it)
{
const auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
const auto legacyAttributes = gci.GenerateLegacyAttributes(it->TextAttr());

// If the first thing we read is trailing, pad with a space.
// OR If the last thing we read is leading, pad with a space.
if ((amountRead == 0 && it->DbcsAttr().IsTrailing()) ||
(amountRead == (amountToRead - 1) && it->DbcsAttr().IsLeading()))
{
retVal.push_back(it->TextAttr().GetLegacyAttributes());
retVal.push_back(legacyAttributes);
}
else
{
retVal.push_back(it->TextAttr().GetLegacyAttributes() | it->DbcsAttr().GeneratePublicApiAttributeFormat());
retVal.push_back(legacyAttributes | it->DbcsAttr().GeneratePublicApiAttributeFormat());
}

amountRead++;
Expand Down Expand Up @@ -384,7 +387,7 @@ void ScrollRegion(SCREEN_INFORMATION& screenInfo,

// However, if the character is null and we were given a null attribute (represented as legacy 0),
// then we'll just fill with spaces and whatever the buffer's default colors are.
if (fillCharGiven == UNICODE_NULL && fillAttrsGiven.IsLegacy() && fillAttrsGiven.GetLegacyAttributes() == 0)
if (fillCharGiven == UNICODE_NULL && fillAttrsGiven == TextAttribute{ 0 })
{
fillData = OutputCellIterator(UNICODE_SPACE, screenInfo.GetAttributes());
}
Expand Down
Loading