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

Allow OSC 17 to set the selection background color #17742

Merged
merged 6 commits into from
Aug 22, 2024
Merged

Allow OSC 17 to set the selection background color #17742

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
687b6e6
Push selection color down into core (as a color table entry)
DHowett Aug 16, 2024
48d8a3e
vt: add support for OSC 17 "set highlight color"
DHowett Aug 16, 2024
018bbf8
Reserve SELECTION_FOREGROUND and keep fg/bg in the right order
DHowett Aug 20, 2024
d203dcb
Unreserve selection foreground for now.
DHowett Aug 21, 2024
0998431
Make sure _p stays in sync with _api
DHowett Aug 22, 2024
c1ed74a
Fix build and audit
DHowett Aug 22, 2024
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
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 @@ -82,7 +82,8 @@ struct TextColor
static constexpr size_t FRAME_FOREGROUND = 263;
static constexpr size_t FRAME_BACKGROUND = 264;
static constexpr size_t CURSOR_COLOR = 265;
static constexpr size_t TABLE_SIZE = 266;
static constexpr size_t SELECTION_BACKGROUND = 266;
static constexpr size_t TABLE_SIZE = 267;
Comment on lines +85 to +86
Copy link
Collaborator

Choose a reason for hiding this comment

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

If there's a chance we may one day support a selection foreground color like Xterm does, it may be a good idea to reserve that slot now, so we've always got our foreground and background colors paired in the same order.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, I love that. That ship may have sailed for the cursor color, however... as i have always wanted to add cursor foreground.

Since we didn't have palette report support before (unreleased) 1.221, is there still time to make changes in our reserved color indices?

Footnotes

  1. under the assumption that we want them stable now that people can back up and restore them :) ↩

Copy link
Collaborator

Choose a reason for hiding this comment

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

That ship may have sailed for the cursor color, however... as i have always wanted to add cursor foreground.

Cursor color is a bit of a special case, because it can be thought of as both foreground and background depending on the style. When it's the block style, you think of the cursor color as a background attribute, and when it's the underline or bar style it's more like a foreground attribute.

But if you consider the way cursors were originally implemented, it makes the most sense as a foreground attribute, because the block cursor was really just implemented as a reverse attribute toggle. So if your default text is white on black, and your cursor color is say green, then an underline cursor is going to be green on black, and a block cursor is just green on black reversed. In both case the black background is retained, and the foreground white is replaced with green.

That said, if you wanted to introduce a second cursor color, referring to the two as foreground and background is going to be confusing no matter which way you look at it, and maybe it would be easiest to refer to the second color and something generic like "secondary cursor color".

Since we didn't have palette report support before (unreleased) 1.22, is there still time to make changes in our reserved color indices?

I don't think we want to mess with the default text and frame indices, because even though we couldn't query them, we've already been telling people they can set the colors at those specific positions. The cursor color is probably less of an issue, because I don't think we've publicaly mentioned its value anywhere. But as I explained above, I don't think we necessarily should be changing that anywav - just reserve a slot for a "secondary cursor color" if you think we're going to need that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, that makes way more sense. Thanks for explaining.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you not still want to reserve a spot for a SECONDARY_CURSOR_COLOR before the new selection color entries, or have you decided we aren't likely to need that?

Copy link
Member Author

Choose a reason for hiding this comment

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

Eh, I was going to leave it. But you're right, I might as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Leonard's position is more (paraphrased by me) "If these aren't a committed public API, we should be able to renumber them at our leisure later." I have not fully digested how I feel about Terminal Preview and DECCTR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I'm OK with that too.


constexpr TextColor() noexcept :
_meta{ ColorType::IsDefault },
Expand Down
5 changes: 0 additions & 5 deletions src/cascadia/TerminalControl/ControlCore.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,9 +349,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation
const auto viewInPixels = Viewport::FromDimensions({ 0, 0 }, windowSize);
LOG_IF_FAILED(_renderEngine->SetWindowSize({ viewInPixels.Width(), viewInPixels.Height() }));

// Update AtlasEngine's SelectionBackground
_renderEngine->SetSelectionBackground(til::color{ _settings->SelectionBackground() });

const auto vp = _renderEngine->GetViewportInCharacters(viewInPixels);
const auto width = vp.Width();
const auto height = vp.Height();
Expand Down Expand Up @@ -900,7 +897,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation
if (_renderEngine)
{
// Update AtlasEngine settings under the lock
_renderEngine->SetSelectionBackground(til::color{ newAppearance->SelectionBackground() });
_renderEngine->SetRetroTerminalEffect(newAppearance->RetroTerminalEffect());
_renderEngine->SetPixelShaderPath(newAppearance->PixelShaderPath());
_renderEngine->SetPixelShaderImagePath(newAppearance->PixelShaderImagePath());
Expand Down Expand Up @@ -2412,7 +2408,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation

const auto lock = _terminal->LockForWriting();
_terminal->ApplyScheme(scheme);
_renderEngine->SetSelectionBackground(til::color{ _settings->SelectionBackground() });
_renderer->TriggerRedrawAll(true);
}

Expand Down
3 changes: 1 addition & 2 deletions src/cascadia/TerminalControl/HwndTerminal.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -881,8 +881,7 @@ void _stdcall TerminalSetTheme(void* terminal, TerminalTheme theme, LPCWSTR font
auto& renderSettings = publicTerminal->_terminal->GetRenderSettings();
renderSettings.SetColorTableEntry(TextColor::DEFAULT_FOREGROUND, theme.DefaultForeground);
renderSettings.SetColorTableEntry(TextColor::DEFAULT_BACKGROUND, theme.DefaultBackground);

publicTerminal->_renderEngine->SetSelectionBackground(theme.DefaultSelectionBackground);
renderSettings.SetColorTableEntry(TextColor::SELECTION_BACKGROUND, theme.DefaultSelectionBackground);

// Set the font colors
for (size_t tableIndex = 0; tableIndex < 16; tableIndex++)
Expand Down
1 change: 0 additions & 1 deletion src/cascadia/TerminalControl/IControlAppearance.idl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ namespace Microsoft.Terminal.Control
{
interface IControlAppearance requires Microsoft.Terminal.Core.ICoreAppearance
{
Microsoft.Terminal.Core.Color SelectionBackground { get; };
String BackgroundImage { get; };
Single BackgroundImageOpacity { get; };
Windows.UI.Xaml.Media.Stretch BackgroundImageStretchMode { get; };
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalCore/ICoreAppearance.idl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ namespace Microsoft.Terminal.Core
{
Microsoft.Terminal.Core.Color DefaultForeground;
Microsoft.Terminal.Core.Color DefaultBackground;
Microsoft.Terminal.Core.Color SelectionBackground;
Microsoft.Terminal.Core.Color GetColorTableEntry(Int32 index);
Microsoft.Terminal.Core.Color CursorColor;
CursorStyle CursorShape;
Expand Down
5 changes: 4 additions & 1 deletion src/cascadia/TerminalCore/Terminal.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,8 @@ void Terminal::UpdateAppearance(const ICoreAppearance& appearance)
renderSettings.SetColorAlias(ColorAlias::DefaultForeground, TextColor::DEFAULT_FOREGROUND, newForegroundColor);
const til::color newCursorColor{ appearance.CursorColor() };
renderSettings.SetColorTableEntry(TextColor::CURSOR_COLOR, newCursorColor);
const til::color newSelectionColor{ appearance.SelectionBackground() };
renderSettings.SetColorTableEntry(TextColor::SELECTION_BACKGROUND, newSelectionColor);

for (auto i = 0; i < 16; i++)
{
Expand Down Expand Up @@ -1275,8 +1277,8 @@ Scheme Terminal::GetColorScheme() const
s.Foreground = til::color{ renderSettings.GetColorAlias(ColorAlias::DefaultForeground) };
s.Background = til::color{ renderSettings.GetColorAlias(ColorAlias::DefaultBackground) };

// SelectionBackground is stored in the ControlAppearance
s.CursorColor = til::color{ renderSettings.GetColorTableEntry(TextColor::CURSOR_COLOR) };
s.SelectionBackground = til::color{ renderSettings.GetColorTableEntry(TextColor::SELECTION_BACKGROUND) };

s.Black = til::color{ renderSettings.GetColorTableEntry(TextColor::DARK_BLACK) };
s.Red = til::color{ renderSettings.GetColorTableEntry(TextColor::DARK_RED) };
Expand Down Expand Up @@ -1322,6 +1324,7 @@ void Terminal::ApplyScheme(const Scheme& colorScheme)
renderSettings.SetColorTableEntry(TextColor::BRIGHT_WHITE, til::color{ colorScheme.BrightWhite });

renderSettings.SetColorTableEntry(TextColor::CURSOR_COLOR, til::color{ colorScheme.CursorColor });
renderSettings.SetColorTableEntry(TextColor::SELECTION_BACKGROUND, til::color{ colorScheme.SelectionBackground });

// Tell the control that the scrollbar has somehow changed. Used as a
// workaround to force the control to redraw any scrollbar marks whose color
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/inc/ControlProperties.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,14 @@
X(til::color, CursorColor, DEFAULT_CURSOR_COLOR) \
X(winrt::Microsoft::Terminal::Core::CursorStyle, CursorShape, winrt::Microsoft::Terminal::Core::CursorStyle::Vintage) \
X(uint32_t, CursorHeight, DEFAULT_CURSOR_HEIGHT) \
X(til::color, SelectionBackground, DEFAULT_FOREGROUND) \
X(bool, IntenseIsBold) \
X(bool, IntenseIsBright, true) \
X(winrt::Microsoft::Terminal::Core::AdjustTextMode, AdjustIndistinguishableColors, winrt::Microsoft::Terminal::Core::AdjustTextMode::Never)

// --------------------------- Control Appearance ---------------------------
// All of these settings are defined in IControlAppearance.
#define CONTROL_APPEARANCE_SETTINGS(X) \
X(til::color, SelectionBackground, DEFAULT_FOREGROUND) \
X(float, Opacity, 1.0f) \
X(bool, UseAcrylic, false) \
X(winrt::hstring, BackgroundImage) \
Expand Down
13 changes: 0 additions & 13 deletions src/renderer/atlas/AtlasEngine.api.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
#include "Backend.h"
#include "../../buffer/out/textBuffer.hpp"
#include "../base/FontCache.h"
#include "../../types/inc/ColorFix.hpp"

// #### NOTE ####
// If you see any code in here that contains "_r." you might be seeing a race condition.
Expand Down Expand Up @@ -425,18 +424,6 @@ void AtlasEngine::SetRetroTerminalEffect(bool enable) noexcept
}
}

void AtlasEngine::SetSelectionBackground(const COLORREF color) noexcept
{
const u32 selectionColor = (color & 0xffffff) | 0xff000000;
if (_api.s->misc->selectionColor != selectionColor)
{
auto misc = _api.s.write()->misc.write();
misc->selectionColor = selectionColor;
// Select a black or white foreground based on the perceptual lightness of the background.
misc->selectionForeground = ColorFix::GetLuminosity(selectionColor) < 0.5f ? 0xffffffff : 0xff000000;
}
}

void AtlasEngine::SetSoftwareRendering(bool enable) noexcept
{
if (_api.s->target->useWARP != enable)
Expand Down
17 changes: 16 additions & 1 deletion src/renderer/atlas/AtlasEngine.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,21 @@ CATCH_RETURN()
}

_api.selectionSpans = til::point_span_subspan_within_rect(info.selectionSpans, dr);

const u32 newSelectionColor{ static_cast<COLORREF>(info.selectionBackground) | 0xff000000 };
if (_api.s->misc->selectionColor != newSelectionColor)
{
auto misc = _api.s.write()->misc.write();
Copy link
Member

Choose a reason for hiding this comment

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

Copying from _api.s to _p.s happens in StartPaint(). Similar to AtlasEngine::PaintCursor you need to update both structs simultaneously to keep their generation count in sync.

You can test that this by setting a breakpoint into AtlasEngine::_handleSettingsUpdate. Then and emit an OSC 17. If done correctly, AtlasEngine::_handleSettingsUpdate should not get called.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh no, this is interesting. It didn't cause a problem because I didn't understand your architecture.

I put the selection colors on _api, and I read them out of _api. I didn't even know (remember?) that they would also be on _p.

Should PaintBufferLineHelper be reading them out of _p instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

I made the call to read them out of _p, because that's the RenderingPayload.

misc->selectionColor = newSelectionColor;
// Select a black or white foreground based on the perceptual lightness of the background.
misc->selectionForeground = ColorFix::GetLuminosity(newSelectionColor) < 0.5f ? 0xffffffff : 0xff000000;

// We copied the selection colors into _p during StartPaint, which happened just before PrepareRenderInfo
// This keeps their generations in sync.
auto pm = _p.s.write()->misc.write();
pm->selectionColor = misc->selectionColor;
pm->selectionForeground = misc->selectionForeground;
}
}

return S_OK;
Expand Down Expand Up @@ -502,7 +517,7 @@ try
// Apply the highlighting colors to the highlighted cells
RETURN_IF_FAILED(_drawHighlighted(_api.searchHighlights, y, x, columnEnd, highlightFg, highlightBg));
RETURN_IF_FAILED(_drawHighlighted(_api.searchHighlightFocused, y, x, columnEnd, highlightFocusFg, highlightFocusBg));
RETURN_IF_FAILED(_drawHighlighted(_api.selectionSpans, y, x, columnEnd, _api.s->misc->selectionForeground, _api.s->misc->selectionColor));
RETURN_IF_FAILED(_drawHighlighted(_api.selectionSpans, y, x, columnEnd, _p.s->misc->selectionForeground, _p.s->misc->selectionColor));

_api.lastPaintBufferLineCoord = { x, y };
return S_OK;
Expand Down
1 change: 0 additions & 1 deletion src/renderer/atlas/AtlasEngine.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,6 @@ namespace Microsoft::Console::Render::Atlas
void SetPixelShaderPath(std::wstring_view value) noexcept;
void SetPixelShaderImagePath(std::wstring_view value) noexcept;
void SetRetroTerminalEffect(bool enable) noexcept;
void SetSelectionBackground(COLORREF color) noexcept;
void SetSoftwareRendering(bool enable) noexcept;
void SetDisablePartialInvalidation(bool enable) noexcept;
void SetGraphicsAPI(GraphicsAPI graphicsAPI) noexcept;
Expand Down
1 change: 1 addition & 0 deletions src/renderer/base/RenderSettings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ RenderSettings::RenderSettings() noexcept
SetColorTableEntry(TextColor::FRAME_FOREGROUND, INVALID_COLOR);
SetColorTableEntry(TextColor::FRAME_BACKGROUND, INVALID_COLOR);
SetColorTableEntry(TextColor::CURSOR_COLOR, INVALID_COLOR);
SetColorTableEntry(TextColor::SELECTION_BACKGROUND, INVALID_COLOR);

SetColorAliasIndex(ColorAlias::DefaultForeground, TextColor::DARK_WHITE);
SetColorAliasIndex(ColorAlias::DefaultBackground, TextColor::DARK_BLACK);
Expand Down
1 change: 1 addition & 0 deletions src/renderer/base/renderer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1276,6 +1276,7 @@ void Renderer::_PaintCursor(_In_ IRenderEngine* const pEngine)
info.searchHighlights = _pData->GetSearchHighlights();
info.searchHighlightFocused = _pData->GetSearchHighlightFocused();
info.selectionSpans = _pData->GetSelectionSpans();
info.selectionBackground = _renderSettings.GetColorTableEntry(TextColor::SELECTION_BACKGROUND);
return pEngine->PrepareRenderInfo(std::move(info));
}

Expand Down
1 change: 1 addition & 0 deletions src/renderer/inc/IRenderEngine.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ namespace Microsoft::Console::Render
std::span<const til::point_span> searchHighlights;
const til::point_span* searchHighlightFocused;
std::span<const til::point_span> selectionSpans;
til::color selectionBackground;
};

enum class GridLines
Expand Down
2 changes: 1 addition & 1 deletion src/terminal/adapter/adaptDispatch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ static constexpr std::array<XtermResourceColorTableEntry, 10> XtermResourceColor
/* 14 */ { -1, -1 },
/* 15 */ { -1, -1 },
/* 16 */ { -1, -1 },
/* 17 */ { -1, -1 },
/* 17 */ { TextColor::SELECTION_BACKGROUND, -1 },
/* 18 */ { -1, -1 },
/* 19 */ { -1, -1 },
} };
Expand Down
1 change: 1 addition & 0 deletions src/terminal/parser/OutputStateMachineEngine.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -804,6 +804,7 @@ bool OutputStateMachineEngine::ActionOscDispatch(const size_t parameter, const s
case OscActionCodes::SetForegroundColor:
case OscActionCodes::SetBackgroundColor:
case OscActionCodes::SetCursorColor:
case OscActionCodes::SetHighlightColor:
Copy link
Member Author

Choose a reason for hiding this comment

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

this is xterm's name for selection color

{
std::vector<DWORD> colors;
success = _GetOscSetColor(string, colors);
Expand Down
1 change: 1 addition & 0 deletions src/terminal/parser/OutputStateMachineEngine.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,7 @@ namespace Microsoft::Console::VirtualTerminal
SetForegroundColor = 10,
SetBackgroundColor = 11,
SetCursorColor = 12,
SetHighlightColor = 17,
DECSWT_SetWindowTitle = 21,
SetClipboard = 52,
ResetForegroundColor = 110, // Not implemented
Expand Down
Loading