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

Add support for resetting the color scheme with RIS #17879

Merged
merged 2 commits into from
Sep 16, 2024
Merged

Add support for resetting the color scheme with RIS #17879

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1175d7d
Add support for resetting the color table.
j4james Sep 8, 2024
9a297b9
Add some unit tests.
j4james Sep 8, 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: 3 additions & 0 deletions src/cascadia/TerminalControl/HwndTerminal.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -891,6 +891,9 @@ void _stdcall TerminalSetTheme(void* terminal, TerminalTheme theme, LPCWSTR font
renderSettings.SetColorTableEntry(tableIndex, gsl::at(theme.ColorTable, tableIndex));
}

// Save these values as the new default render settings.
renderSettings.SaveDefaultSettings();

publicTerminal->_terminal->SetCursorStyle(static_cast<Microsoft::Console::VirtualTerminal::DispatchTypes::CursorStyle>(theme.CursorStyle));

publicTerminal->_desiredFont = { fontFamily, 0, DEFAULT_FONT_WEIGHT, static_cast<float>(fontSize), CP_UTF8 };
Expand Down
3 changes: 3 additions & 0 deletions src/cascadia/TerminalCore/Terminal.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,9 @@ void Terminal::UpdateSettings(ICoreSettings settings)
GetRenderSettings().SetColorTableEntry(TextColor::FRAME_BACKGROUND, til::color{ settings.TabColor().Value() });
}

// Save the changes made above and in UpdateAppearance as the new default render settings.
GetRenderSettings().SaveDefaultSettings();
Comment on lines +104 to +105
Copy link
Member

Choose a reason for hiding this comment

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

If the code above calls SetColorTableEntry, won't this overwrite the modified colors set by the terminal application (as if a RIS was received)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah. That's typically what you want if you're actually using the settings to change the color scheme, but not so much when you're editing an unrelated setting. In fact the colors can even end up being reset when when you haven't edited the settings at all! This is a known problem that is tracked in issue #11522.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah. That's typically what you want if you're actually using the settings to change the color scheme

I'm somewhat surprised about that - I expected the opposite. But that answered my question. It was the only concern I had, and otherwise the PR looks great. :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just to be clear, I don't feel strongly about it either way, but that's the way it's always worked, and it seemed to me a reasonable behavior. If you open up a shell with a particular color scheme, decide you don't like it, so you change the scheme in the settings, wouldn't you expect that shell to update straight away?

Of course if you were busy using an application that set its own palette, having it overwritten would be annoying, but it's much less likely you'd be trying to switch your color scheme in that situation (this assumes that #11522 is fixed, so changing other settings wouldn't trigger a color scheme refresh).

Either way, if we want to reconsider that behavior, I think it's best raised as a separate issue.


if (!_startingTabColor && settings.StartingTabColor())
{
_startingTabColor = settings.StartingTabColor().Value();
Expand Down
7 changes: 7 additions & 0 deletions src/host/settings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,8 @@ void Settings::Validate()
TextAttribute::SetLegacyDefaultAttributes(_wFillAttribute);
// And calculate the position of the default colors in the color table.
CalculateDefaultColorIndices();
// We can also then save these values as the default render settings.
SaveDefaultRenderSettings();

FAIL_FAST_IF(!(_dwWindowSize.X > 0));
FAIL_FAST_IF(!(_dwWindowSize.Y > 0));
Expand Down Expand Up @@ -755,6 +757,11 @@ void Settings::CalculateDefaultColorIndices() noexcept
_renderSettings.SetColorAliasIndex(ColorAlias::DefaultBackground, backgroundAlias);
}

void Settings::SaveDefaultRenderSettings() noexcept
{
_renderSettings.SaveDefaultSettings();
}

bool Settings::IsTerminalScrolling() const noexcept
{
return _TerminalScrolling;
Expand Down
1 change: 1 addition & 0 deletions src/host/settings.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,7 @@ class Settings
void SetInterceptCopyPaste(const bool interceptCopyPaste) noexcept;

void CalculateDefaultColorIndices() noexcept;
void SaveDefaultRenderSettings() noexcept;

bool IsTerminalScrolling() const noexcept;
void SetTerminalScrolling(const bool terminalScrollingEnabled) noexcept;
Expand Down
17 changes: 17 additions & 0 deletions src/host/ut_host/ScreenBufferTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

#include "../interactivity/inc/ServiceLocator.hpp"
#include "../../inc/conattrs.hpp"
#include "../../types/inc/colorTable.hpp"
#include "../../types/inc/Viewport.hpp"

#include "../../inc/TestUtils.h"
Expand Down Expand Up @@ -2070,6 +2071,15 @@ void ScreenBufferTests::VtRestoreColorTableReport()
// Blue component is clamped at 100%, so 150% interpreted as 100%
stateMachine.ProcessString(L"\033P2$p14;2;0;0;150\033\\");
VERIFY_ARE_EQUAL(RGB(0, 0, 255), gci.GetColorTableEntry(14));

Log::Comment(L"RIS restores initial Campbell color scheme");

stateMachine.ProcessString(L"\033c");
for (auto i = 0; i < 16; i++)
{
const COLORREF expectedColor = Microsoft::Console::Utils::CampbellColorTable()[i];
VERIFY_ARE_EQUAL(expectedColor, gci.GetColorTableEntry(i));
}
}

void ScreenBufferTests::ResizeTraditionalDoesNotDoubleFreeAttrRows()
Expand Down Expand Up @@ -3352,6 +3362,13 @@ void ScreenBufferTests::AssignColorAliases()
stateMachine.ProcessString(L"\033[2;34;56,|");
VERIFY_ARE_EQUAL(34u, renderSettings.GetColorAliasIndex(ColorAlias::FrameForeground));
VERIFY_ARE_EQUAL(56u, renderSettings.GetColorAliasIndex(ColorAlias::FrameBackground));

Log::Comment(L"Test RIS restores initial color assignments");
stateMachine.ProcessString(L"\033c");
VERIFY_ARE_EQUAL(defaultFg, renderSettings.GetColorAliasIndex(ColorAlias::DefaultForeground));
VERIFY_ARE_EQUAL(defaultBg, renderSettings.GetColorAliasIndex(ColorAlias::DefaultBackground));
VERIFY_ARE_EQUAL(frameFg, renderSettings.GetColorAliasIndex(ColorAlias::FrameForeground));
VERIFY_ARE_EQUAL(frameBg, renderSettings.GetColorAliasIndex(ColorAlias::FrameBackground));
}

void ScreenBufferTests::DeleteCharsNearEndOfLine()
Expand Down
2 changes: 2 additions & 0 deletions src/interactivity/win32/menu.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -576,6 +576,8 @@ void Menu::s_PropertiesUpdate(PCONSOLE_STATE_INFO pStateInfo)
TextAttribute::SetLegacyDefaultAttributes(pStateInfo->ScreenAttributes);
// And recalculate the position of the default colors in the color table.
gci.CalculateDefaultColorIndices();
// Then save these values as the new default render settings.
gci.SaveDefaultRenderSettings();

// Set the screen info's default text attributes to defaults -
ScreenInfo.SetDefaultAttributes({}, TextAttribute{ gci.GetPopupFillAttribute() });
Expand Down
23 changes: 23 additions & 0 deletions src/renderer/base/RenderSettings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,29 @@ RenderSettings::RenderSettings() noexcept
SetColorAliasIndex(ColorAlias::DefaultBackground, TextColor::DARK_BLACK);
SetColorAliasIndex(ColorAlias::FrameForeground, TextColor::FRAME_FOREGROUND);
SetColorAliasIndex(ColorAlias::FrameBackground, TextColor::FRAME_BACKGROUND);

SaveDefaultSettings();
}

// Routine Description:
// - Saves the current color table and color aliases as the default values, so
// we can later restore them when a hard reset (RIS) is requested.
void RenderSettings::SaveDefaultSettings() noexcept
{
_defaultColorTable = _colorTable;
_defaultColorAliasIndices = _colorAliasIndices;
}

// Routine Description:
// - Resets the render settings to their default values. which is typically
// what they were set to at startup.
void RenderSettings::RestoreDefaultSettings() noexcept
{
_colorTable = _defaultColorTable;
_colorAliasIndices = _defaultColorAliasIndices;
// For now, DECSCNM is the only render mode we need to reset. The others are
// all user preferences that can't be changed programmatically.
_renderMode.reset(Mode::ScreenReversed);
}

// Routine Description:
Expand Down
4 changes: 4 additions & 0 deletions src/renderer/inc/RenderSettings.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ namespace Microsoft::Console::Render
};

RenderSettings() noexcept;
void SaveDefaultSettings() noexcept;
void RestoreDefaultSettings() noexcept;
void SetRenderMode(const Mode mode, const bool enabled) noexcept;
bool GetRenderMode(const Mode mode) const noexcept;
const std::array<COLORREF, TextColor::TABLE_SIZE>& GetColorTable() const noexcept;
Expand All @@ -48,6 +50,8 @@ namespace Microsoft::Console::Render
til::enumset<Mode> _renderMode{ Mode::BlinkAllowed, Mode::IntenseIsBright };
std::array<COLORREF, TextColor::TABLE_SIZE> _colorTable;
std::array<size_t, static_cast<size_t>(ColorAlias::ENUM_COUNT)> _colorAliasIndices;
std::array<COLORREF, TextColor::TABLE_SIZE> _defaultColorTable;
std::array<size_t, static_cast<size_t>(ColorAlias::ENUM_COUNT)> _defaultColorAliasIndices;
size_t _blinkCycle = 0;
mutable bool _blinkIsInUse = false;
bool _blinkShouldBeFaint = false;
Expand Down
9 changes: 7 additions & 2 deletions src/terminal/adapter/adaptDispatch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3019,8 +3019,13 @@ void AdaptDispatch::HardReset()
EraseInDisplay(DispatchTypes::EraseType::All);
EraseInDisplay(DispatchTypes::EraseType::Scrollback);

// Set the DECSCNM screen mode back to normal.
_renderSettings.SetRenderMode(RenderSettings::Mode::ScreenReversed, false);
// Set the color table and render modes back to their initial startup values.
_renderSettings.RestoreDefaultSettings();
// Let the renderer know that the background and frame colors may have changed.
if (_renderer)
{
_renderer->TriggerRedrawAll(true, true);
}

// Cursor to 1,1 - the Soft Reset guarantees this is absolute
CursorPosition(1, 1);
Expand Down
Loading