Skip to content

Commit

Permalink
Switch the Cascadia projects to til::color where it's easily possible…
Browse files Browse the repository at this point in the history 
… to do so (#5847)

This pull request moves swaths of Cascadia to use `til::color` for color
interop. There are still some places where we use `COLORREF`, such as in
the ABI boundaries between WinRT components.

I've also added two more til::color helpers - `with_alpha`, which takes
an existing color and sets its alpha component, and a
`Windows::UI::Color` convertor pair.

Future direction might include a `TerminalSettings::Color` type at the
idl boundary so we can finally stop using UInt32s (!) for color.

## Validation Steps Performed
Tested certain fragile areas:
* [x] setting the background with OSC 11
* [x] setting the background when acrylic is in use (which requires
  low-alpha)
  • Loading branch information
@DHowett
DHowett authored May 15, 2020
1 parent 6394e5d commit b46d393
Show file tree
Hide file tree
Showing 18 changed files with 143 additions and 102 deletions.
22 changes: 11 additions & 11 deletions src/cascadia/TerminalApp/ColorScheme.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ ColorScheme::ColorScheme() :
{
}

ColorScheme::ColorScheme(std::wstring name, COLORREF defaultFg, COLORREF defaultBg, COLORREF cursorColor) :
ColorScheme::ColorScheme(std::wstring name, til::color defaultFg, til::color defaultBg, til::color cursorColor) :
_schemeName{ name },
_table{},
_defaultForeground{ defaultFg },
Expand All @@ -70,15 +70,15 @@ ColorScheme::~ColorScheme()
// - <none>
void ColorScheme::ApplyScheme(TerminalSettings terminalSettings) const
{
terminalSettings.DefaultForeground(_defaultForeground);
terminalSettings.DefaultBackground(_defaultBackground);
terminalSettings.SelectionBackground(_selectionBackground);
terminalSettings.CursorColor(_cursorColor);
terminalSettings.DefaultForeground(static_cast<COLORREF>(_defaultForeground));
terminalSettings.DefaultBackground(static_cast<COLORREF>(_defaultBackground));
terminalSettings.SelectionBackground(static_cast<COLORREF>(_selectionBackground));
terminalSettings.CursorColor(static_cast<COLORREF>(_cursorColor));

auto const tableCount = gsl::narrow_cast<int>(_table.size());
for (int i = 0; i < tableCount; i++)
{
terminalSettings.SetColorTableEntry(i, _table[i]);
terminalSettings.SetColorTableEntry(i, static_cast<COLORREF>(_table[i]));
}
}

Expand Down Expand Up @@ -167,27 +167,27 @@ std::wstring_view ColorScheme::GetName() const noexcept
return { _schemeName };
}

std::array<COLORREF, COLOR_TABLE_SIZE>& ColorScheme::GetTable() noexcept
std::array<til::color, COLOR_TABLE_SIZE>& ColorScheme::GetTable() noexcept
{
return _table;
}

COLORREF ColorScheme::GetForeground() const noexcept
til::color ColorScheme::GetForeground() const noexcept
{
return _defaultForeground;
}

COLORREF ColorScheme::GetBackground() const noexcept
til::color ColorScheme::GetBackground() const noexcept
{
return _defaultBackground;
}

COLORREF ColorScheme::GetSelectionBackground() const noexcept
til::color ColorScheme::GetSelectionBackground() const noexcept
{
return _selectionBackground;
}

COLORREF ColorScheme::GetCursorColor() const noexcept
til::color ColorScheme::GetCursorColor() const noexcept
{
return _cursorColor;
}
Expand Down
22 changes: 11 additions & 11 deletions src/cascadia/TerminalApp/ColorScheme.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ class TerminalApp::ColorScheme
{
public:
ColorScheme();
ColorScheme(std::wstring name, COLORREF defaultFg, COLORREF defaultBg, COLORREF cursorColor);
ColorScheme(std::wstring name, til::color defaultFg, til::color defaultBg, til::color cursorColor);
~ColorScheme();

void ApplyScheme(winrt::Microsoft::Terminal::Settings::TerminalSettings terminalSettings) const;
Expand All @@ -45,21 +45,21 @@ class TerminalApp::ColorScheme
void LayerJson(const Json::Value& json);

std::wstring_view GetName() const noexcept;
std::array<COLORREF, COLOR_TABLE_SIZE>& GetTable() noexcept;
COLORREF GetForeground() const noexcept;
COLORREF GetBackground() const noexcept;
COLORREF GetSelectionBackground() const noexcept;
COLORREF GetCursorColor() const noexcept;
std::array<til::color, COLOR_TABLE_SIZE>& GetTable() noexcept;
til::color GetForeground() const noexcept;
til::color GetBackground() const noexcept;
til::color GetSelectionBackground() const noexcept;
til::color GetCursorColor() const noexcept;

static std::optional<std::wstring> GetNameFromJson(const Json::Value& json);

private:
std::wstring _schemeName;
std::array<COLORREF, COLOR_TABLE_SIZE> _table;
COLORREF _defaultForeground;
COLORREF _defaultBackground;
COLORREF _selectionBackground;
COLORREF _cursorColor;
std::array<til::color, COLOR_TABLE_SIZE> _table;
til::color _defaultForeground;
til::color _defaultBackground;
til::color _selectionBackground;
til::color _cursorColor;

friend class TerminalAppLocalTests::SettingsTests;
friend class TerminalAppLocalTests::ColorSchemeTests;
Expand Down
4 changes: 2 additions & 2 deletions src/cascadia/TerminalApp/JsonUtils.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@

void TerminalApp::JsonUtils::GetOptionalColor(const Json::Value& json,
std::string_view key,
std::optional<uint32_t>& target)
std::optional<til::color>& target)
{
const auto conversionFn = [](const Json::Value& value) -> uint32_t {
const auto conversionFn = [](const Json::Value& value) -> til::color {
return ::Microsoft::Console::Utils::ColorFromHexString(value.asString());
};
GetOptionalValue(json,
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalApp/JsonUtils.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ namespace TerminalApp::JsonUtils
{
void GetOptionalColor(const Json::Value& json,
std::string_view key,
std::optional<uint32_t>& target);
std::optional<til::color>& target);

void GetOptionalString(const Json::Value& json,
std::string_view key,
Expand Down
6 changes: 3 additions & 3 deletions src/cascadia/TerminalApp/Profile.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -551,17 +551,17 @@ void Profile::SetUseAcrylic(bool useAcrylic) noexcept
_useAcrylic = useAcrylic;
}

void Profile::SetDefaultForeground(COLORREF defaultForeground) noexcept
void Profile::SetDefaultForeground(til::color defaultForeground) noexcept
{
_defaultForeground = defaultForeground;
}

void Profile::SetDefaultBackground(COLORREF defaultBackground) noexcept
void Profile::SetDefaultBackground(til::color defaultBackground) noexcept
{
_defaultBackground = defaultBackground;
}

void Profile::SetSelectionBackground(COLORREF selectionBackground) noexcept
void Profile::SetSelectionBackground(til::color selectionBackground) noexcept
{
_selectionBackground = selectionBackground;
}
Expand Down
14 changes: 7 additions & 7 deletions src/cascadia/TerminalApp/Profile.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,9 +79,9 @@ class TerminalApp::Profile final
void SetStartingDirectory(std::wstring startingDirectory) noexcept;
void SetName(const std::wstring_view name) noexcept;
void SetUseAcrylic(bool useAcrylic) noexcept;
void SetDefaultForeground(COLORREF defaultForeground) noexcept;
void SetDefaultBackground(COLORREF defaultBackground) noexcept;
void SetSelectionBackground(COLORREF selectionBackground) noexcept;
void SetDefaultForeground(til::color defaultForeground) noexcept;
void SetDefaultBackground(til::color defaultBackground) noexcept;
void SetSelectionBackground(til::color selectionBackground) noexcept;
void SetCloseOnExitMode(CloseOnExitMode mode) noexcept;
void SetConnectionType(GUID connectionType) noexcept;

Expand Down Expand Up @@ -137,10 +137,10 @@ class TerminalApp::Profile final
// If this is set, then our colors should come from the associated color scheme
std::optional<std::wstring> _schemeName;

std::optional<uint32_t> _defaultForeground;
std::optional<uint32_t> _defaultBackground;
std::optional<uint32_t> _selectionBackground;
std::optional<uint32_t> _cursorColor;
std::optional<til::color> _defaultForeground;
std::optional<til::color> _defaultBackground;
std::optional<til::color> _selectionBackground;
std::optional<til::color> _cursorColor;
std::optional<std::wstring> _tabTitle;
bool _suppressApplicationTitle;
int32_t _historySize;
Expand Down
28 changes: 10 additions & 18 deletions src/cascadia/TerminalControl/TermControl.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
{
_InitializeBackgroundBrush();

uint32_t bg = _settings.DefaultBackground();
COLORREF bg = _settings.DefaultBackground();
_BackgroundColorChanged(bg);

// Apply padding as swapChainPanel's margin
Expand All @@ -274,7 +274,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation

// set TSF Foreground
Media::SolidColorBrush foregroundBrush{};
foregroundBrush.Color(ColorRefToColor(_settings.DefaultForeground()));
foregroundBrush.Color(static_cast<til::color>(_settings.DefaultForeground()));
TSFInputControl().Foreground(foregroundBrush);
TSFInputControl().Margin(newMargin);

Expand Down Expand Up @@ -405,37 +405,29 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
// - color: The background color to use as a uint32 (aka DWORD COLORREF)
// Return Value:
// - <none>
winrt::fire_and_forget TermControl::_BackgroundColorChanged(const uint32_t color)
winrt::fire_and_forget TermControl::_BackgroundColorChanged(const COLORREF color)
{
til::color newBgColor{ color };

auto weakThis{ get_weak() };

co_await winrt::resume_foreground(Dispatcher());

if (auto control{ weakThis.get() })
{
const auto R = GetRValue(color);
const auto G = GetGValue(color);
const auto B = GetBValue(color);

winrt::Windows::UI::Color bgColor{};
bgColor.R = R;
bgColor.G = G;
bgColor.B = B;
bgColor.A = 255;

if (auto acrylic = RootGrid().Background().try_as<Media::AcrylicBrush>())
{
acrylic.FallbackColor(bgColor);
acrylic.TintColor(bgColor);
acrylic.FallbackColor(newBgColor);
acrylic.TintColor(newBgColor);
}
else if (auto solidColor = RootGrid().Background().try_as<Media::SolidColorBrush>())
{
solidColor.Color(bgColor);
solidColor.Color(newBgColor);
}

// Set the default background as transparent to prevent the
// DX layer from overwriting the background image or acrylic effect
_settings.DefaultBackground(ARGB(0, R, G, B));
_settings.DefaultBackground(static_cast<COLORREF>(newBgColor.with_alpha(0)));
}
}

Expand Down Expand Up @@ -1309,7 +1301,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
{
_settings.UseAcrylic(false);
_InitializeBackgroundBrush();
uint32_t bg = _settings.DefaultBackground();
COLORREF bg = _settings.DefaultBackground();
_BackgroundColorChanged(bg);
}
else
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalControl/TermControl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation

void _ApplyUISettings();
void _InitializeBackgroundBrush();
winrt::fire_and_forget _BackgroundColorChanged(const uint32_t color);
winrt::fire_and_forget _BackgroundColorChanged(const COLORREF color);
bool _InitializeTerminal();
void _UpdateFont(const bool initialUpdate = false);
void _SetFontSize(int fontSize);
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalCore/Terminal.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -875,7 +875,7 @@ void Terminal::SetCursorPositionChangedCallback(std::function<void()> pfn) noexc
// - Allows setting a callback for when the background color is changed
// Arguments:
// - pfn: a function callback that takes a uint32 (DWORD COLORREF) color in the format 0x00BBGGRR
void Terminal::SetBackgroundCallback(std::function<void(const uint32_t)> pfn) noexcept
void Terminal::SetBackgroundCallback(std::function<void(const COLORREF)> pfn) noexcept
{
_pfnBackgroundColorChanged.swap(pfn);
}
Expand Down
4 changes: 2 additions & 2 deletions src/cascadia/TerminalCore/Terminal.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ class Microsoft::Terminal::Core::Terminal final :
void SetTitleChangedCallback(std::function<void(const std::wstring_view&)> pfn) noexcept;
void SetScrollPositionChangedCallback(std::function<void(const int, const int, const int)> pfn) noexcept;
void SetCursorPositionChangedCallback(std::function<void()> pfn) noexcept;
void SetBackgroundCallback(std::function<void(const uint32_t)> pfn) noexcept;
void SetBackgroundCallback(std::function<void(const COLORREF)> pfn) noexcept;

void SetCursorOn(const bool isOn);
bool IsCursorBlinkingAllowed() const noexcept;
Expand All @@ -196,7 +196,7 @@ class Microsoft::Terminal::Core::Terminal final :
std::function<void(std::wstring&)> _pfnWriteInput;
std::function<void(const std::wstring_view&)> _pfnTitleChanged;
std::function<void(const int, const int, const int)> _pfnScrollPositionChanged;
std::function<void(const uint32_t)> _pfnBackgroundColorChanged;
std::function<void(const COLORREF)> _pfnBackgroundColorChanged;
std::function<void()> _pfnCursorPositionChanged;

std::unique_ptr<::Microsoft::Console::VirtualTerminal::StateMachine> _stateMachine;
Expand Down
16 changes: 0 additions & 16 deletions src/cascadia/WinRTUtils/inc/Utils.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,22 +3,6 @@

#pragma once

// Method Description:
// - Converts a COLORREF to Color
// Arguments:
// - colorref: COLORREF to convert to Color
// Return Value:
// - Color containing the RGB values from colorref
inline winrt::Windows::UI::Color ColorRefToColor(const COLORREF& colorref)
{
winrt::Windows::UI::Color color;
color.A = 255;
color.R = GetRValue(colorref);
color.G = GetGValue(colorref);
color.B = GetBValue(colorref);
return color;
}

// Method Description:
// - Scales a Rect based on a scale factor
// Arguments:
Expand Down
9 changes: 4 additions & 5 deletions src/cascadia/WindowsTerminal/NonClientIslandWindow.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ static constexpr int AutohideTaskbarSize = 2;

NonClientIslandWindow::NonClientIslandWindow(const ElementTheme& requestedTheme) noexcept :
IslandWindow{},
_backgroundBrushColor{ RGB(0, 0, 0) },
_backgroundBrushColor{ 0, 0, 0 },
_theme{ requestedTheme },
_isMaximized{ false }
{
Expand Down Expand Up @@ -735,13 +735,12 @@ void NonClientIslandWindow::_UpdateFrameMargins() const noexcept

const auto backgroundBrush = _titlebar.Background();
const auto backgroundSolidBrush = backgroundBrush.as<Media::SolidColorBrush>();
const auto backgroundColor = backgroundSolidBrush.Color();
const auto color = RGB(backgroundColor.R, backgroundColor.G, backgroundColor.B);
const til::color backgroundColor = backgroundSolidBrush.Color();

if (!_backgroundBrush || color != _backgroundBrushColor)
if (!_backgroundBrush || backgroundColor != _backgroundBrushColor)
{
// Create brush for titlebar color.
_backgroundBrush = wil::unique_hbrush(CreateSolidBrush(color));
_backgroundBrush = wil::unique_hbrush(CreateSolidBrush(backgroundColor));
}

// To hide the original title bar, we have to paint on top of it with
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/WindowsTerminal/NonClientIslandWindow.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ class NonClientIslandWindow : public IslandWindow
winrt::Windows::UI::Xaml::UIElement _clientContent{ nullptr };

wil::unique_hbrush _backgroundBrush;
COLORREF _backgroundBrushColor;
til::color _backgroundBrushColor;

winrt::Windows::UI::Xaml::Controls::Border _dragBar{ nullptr };
wil::unique_hwnd _dragBarWindow;
Expand Down
2 changes: 0 additions & 2 deletions src/inc/DefaultSettings.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@ constexpr COLORREF DEFAULT_FOREGROUND_WITH_ALPHA = OPACITY_OPAQUE | DEFAULT_FORE
constexpr COLORREF DEFAULT_BACKGROUND = COLOR_BLACK;
constexpr COLORREF DEFAULT_BACKGROUND_WITH_ALPHA = OPACITY_OPAQUE | DEFAULT_BACKGROUND;

constexpr COLORREF POWERSHELL_BLUE = RGB(1, 36, 86);

constexpr short DEFAULT_HISTORY_SIZE = 9001;

#pragma warning(push)
Expand Down
Loading

1 comment on commit b46d393

@github-actions
Copy link

Choose a reason for hiding this comment

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

New misspellings found, please review:

  • adb
To accept these changes, run the following commands 
remove_obsolete_words=$(mktemp)
echo '#!/usr/bin/perl -ni
my $re=join "|", qw('"
bitfield
Emojis
HREF
memcpying
OUTPATHROOT
storageitems
textblock
usr
vpack
"');
next if /^($re)(?:$| .*)/;
print;' > $remove_obsolete_words
chmod +x $remove_obsolete_words
for file in .github/actions/spell-check/whitelist/alphabet.txt .github/actions/spell-check/whitelist/web.txt .github/actions/spell-check/whitelist/whitelist.txt; do $remove_obsolete_words $file; done
rm $remove_obsolete_words
(
echo "
adb
"
) | sort -u -f | perl -ne 'next unless /./; print' > new_whitelist.txt && mv new_whitelist.txt '.github/actions/spell-check/whitelist/b46d39306124363a4104afb74fa9b6657b1d17d0.txt'
✏️ Contributor please read this
  • If the items listed above are names, please add them to .github/actions/spell-check/dictionary/names.txt.
  • If they're APIs, you can add them to a file in .github/actions/spell-check/dictionary/.
  • If they're just things you're using, please add them to an appropriate file in .github/actions/spell-check/whitelist/.
  • If you need to use a specific token in one place and it shouldn't generally be used, you can
    add an item in an appropriate file in .github/actions/spell-check/patterns/.

See the README.md in each directory for more information.

⚠️ Reviewers

At present, the action that triggered this message will not show its ❌ in this PR unless the branch is within this repository.
Thus, you should make sure that this comment has been addressed before encouraging the merge bot to merge this PR.

Please sign in to comment.