Skip to content

Commit

Permalink
Separate runtime TerminalSettings from profile-TerminalSettings (#8602)
Browse files Browse the repository at this point in the history
<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? -->
## Summary of the Pull Request
The TerminalSettings object we create from profiles no longer gets passed into the control, instead, a child of that object gets passed into the control. Any overrides the control makes to the settings then live in the child. So, when we do a settings reload, we simply update the child's parent and the overrides will remain.

<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [ ] Closes #xxx
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [ ] Tests added/passed
* [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx
* [ ] Schema updated.
* [x] I work here

<!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed
Manual testing
  • Loading branch information
PankajBhojwani authored Feb 8, 2021
1 parent 3b7b200 commit 9047bbb
Show file tree
Hide file tree
Showing 14 changed files with 140 additions and 80 deletions.
6 changes: 3 additions & 3 deletions src/cascadia/LocalTests_TerminalApp/CommandlineTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1262,7 +1262,7 @@ namespace TerminalAppLocalTests
void CommandlineTest::TestSimpleExecuteCommandlineAction()
{
ExecuteCommandlineArgs args{ L"new-tab" };
auto actions = implementation::TerminalPage::ConvertExecuteCommandlineToActions(args);
auto actions = winrt::TerminalApp::implementation::TerminalPage::ConvertExecuteCommandlineToActions(args);
VERIFY_ARE_EQUAL(1u, actions.size());
auto actionAndArgs = actions.at(0);
VERIFY_ARE_EQUAL(ShortcutAction::NewTab, actionAndArgs.Action());
Expand All @@ -1281,7 +1281,7 @@ namespace TerminalAppLocalTests
void CommandlineTest::TestMultipleCommandExecuteCommandlineAction()
{
ExecuteCommandlineArgs args{ L"new-tab ; split-pane" };
auto actions = implementation::TerminalPage::ConvertExecuteCommandlineToActions(args);
auto actions = winrt::TerminalApp::implementation::TerminalPage::ConvertExecuteCommandlineToActions(args);
VERIFY_ARE_EQUAL(2u, actions.size());
{
auto actionAndArgs = actions.at(0);
Expand Down Expand Up @@ -1317,7 +1317,7 @@ namespace TerminalAppLocalTests
{
// -H and -V cannot be combined.
ExecuteCommandlineArgs args{ L"split-pane -H -V" };
auto actions = implementation::TerminalPage::ConvertExecuteCommandlineToActions(args);
auto actions = winrt::TerminalApp::implementation::TerminalPage::ConvertExecuteCommandlineToActions(args);
VERIFY_ARE_EQUAL(0u, actions.size());
}

Expand Down
4 changes: 2 additions & 2 deletions src/cascadia/LocalTests_TerminalApp/TabTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -899,10 +899,10 @@ namespace TerminalAppLocalTests
page->_SelectNextTab(true);
});

const auto palette = winrt::get_self<implementation::CommandPalette>(page->CommandPalette());
const auto palette = winrt::get_self<winrt::TerminalApp::implementation::CommandPalette>(page->CommandPalette());

VERIFY_ARE_EQUAL(1u, palette->_switcherStartIdx, L"Verify the index is 1 as we went right");
VERIFY_ARE_EQUAL(implementation::CommandPaletteMode::TabSwitchMode, palette->_currentMode, L"Verify we are in the tab switcher mode");
VERIFY_ARE_EQUAL(winrt::TerminalApp::implementation::CommandPaletteMode::TabSwitchMode, palette->_currentMode, L"Verify we are in the tab switcher mode");

Log::Comment(L"Verify command palette preserves MRU order of tabs");
VERIFY_ARE_EQUAL(4u, palette->_filteredActions.Size());
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalApp/AppActionHandlers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ namespace winrt::TerminalApp::implementation
{
auto controlSettings = activeControl.Settings().as<TerminalSettings>();
controlSettings->ApplyColorScheme(scheme);
activeControl.UpdateSettings(*controlSettings);
activeControl.UpdateSettings();
args.Handled(true);
}
}
Expand Down
8 changes: 7 additions & 1 deletion src/cascadia/TerminalApp/Pane.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -642,7 +642,13 @@ void Pane::UpdateSettings(const TerminalSettings& settings, const GUID& profile)
{
if (profile == _profile)
{
_control.UpdateSettings(settings);
// Update the parent of the control's settings object (and not the object itself) so
// that any overrides made by the control don't get affected by the reload
auto child = winrt::get_self<winrt::TerminalApp::implementation::TerminalSettings>(_control.Settings());
auto parent = winrt::get_self<winrt::TerminalApp::implementation::TerminalSettings>(settings);
child->ClearParents();
child->InsertParent(0, parent->get_strong());
_control.UpdateSettings();
}
}
}
Expand Down
4 changes: 3 additions & 1 deletion src/cascadia/TerminalApp/TerminalPage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -760,7 +760,9 @@ namespace winrt::TerminalApp::implementation
}
}

TermControl term{ settings, connection };
// Give term control a child of the settings so that any overrides go in the child
// This way, when we do a settings reload we just update the parent and the overrides remain
TermControl term{ *(winrt::get_self<TerminalSettings>(settings)->CreateChild()), connection };

auto newTabImpl = winrt::make_self<TerminalTab>(profileGuid, term);

Expand Down
50 changes: 47 additions & 3 deletions src/cascadia/TerminalApp/TerminalSettings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -226,13 +226,57 @@ namespace winrt::TerminalApp::implementation
_CursorColor = til::color{ scheme.CursorColor() };

const auto table = scheme.Table();
std::transform(table.cbegin(), table.cend(), _colorTable.begin(), [](auto&& color) {
std::array<uint32_t, COLOR_TABLE_SIZE> colorTable{};
std::transform(table.cbegin(), table.cend(), colorTable.begin(), [](auto&& color) {
return static_cast<uint32_t>(til::color{ color });
});
ColorTable(colorTable);
}

uint32_t TerminalSettings::GetColorTableEntry(int32_t index) const noexcept
uint32_t TerminalSettings::GetColorTableEntry(int32_t index) noexcept
{
return _colorTable.at(index);
return ColorTable().at(index);
}

void TerminalSettings::ColorTable(std::array<uint32_t, 16> colors)
{
_ColorTable = colors;
}

std::array<uint32_t, COLOR_TABLE_SIZE> TerminalSettings::ColorTable()
{
auto span = _getColorTableImpl();
std::array<uint32_t, COLOR_TABLE_SIZE> colorTable{};
if (span.size() > 0)
{
std::transform(span.begin(), span.end(), colorTable.begin(), [](auto&& color) {
return static_cast<uint32_t>(til::color{ color });
});
}
else
{
const auto campbellSpan = CampbellColorTable();
std::transform(campbellSpan.begin(), campbellSpan.end(), colorTable.begin(), [](auto&& color) {
return static_cast<uint32_t>(til::color{ color });
});
}
return colorTable;
}

gsl::span<uint32_t> TerminalSettings::_getColorTableImpl()
{
if (_ColorTable.has_value())
{
return gsl::make_span(*_ColorTable);
}
for (auto&& parent : _parents)
{
auto parentSpan = parent->_getColorTableImpl();
if (parentSpan.size() > 0)
{
return parentSpan;
}
}
return {};
}
}
117 changes: 58 additions & 59 deletions src/cascadia/TerminalApp/TerminalSettings.h
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,14 @@ Author(s):
#pragma once

#include "TerminalSettings.g.h"
#include "../TerminalSettingsModel/IInheritable.h"
#include "../inc/cppwinrt_utils.h"
#include "../../types/inc/colorTable.hpp"
#include <DefaultSettings.h>
#include <conattrs.hpp>

using namespace Microsoft::Console::Utils;

// fwdecl unittest classes
namespace TerminalAppLocalTests
{
Expand All @@ -27,7 +31,7 @@ namespace TerminalAppLocalTests

namespace winrt::TerminalApp::implementation
{
struct TerminalSettings : TerminalSettingsT<TerminalSettings>
struct TerminalSettings : TerminalSettingsT<TerminalSettings>, winrt::Microsoft::Terminal::Settings::Model::implementation::IInheritable<TerminalSettings>
{
TerminalSettings() = default;
TerminalSettings(const Microsoft::Terminal::Settings::Model::CascadiaSettings& appSettings,
Expand All @@ -40,35 +44,31 @@ namespace winrt::TerminalApp::implementation

void ApplyColorScheme(const Microsoft::Terminal::Settings::Model::ColorScheme& scheme);

// TECHNICALLY, the hstring copy assignment can throw, but the GETSET_PROPERTY
// macro defines the operator as `noexcept`. We're not really worried about it,
// because the only time it will throw is when we're out of memory, and then
// we've got much worse problems. So just suppress that warning for now.
#pragma warning(push)
#pragma warning(disable : 26447)
// --------------------------- Core Settings ---------------------------
// All of these settings are defined in ICoreSettings.

// GetColorTableEntry needs to be implemented manually, to get a
// particular value from the array.
uint32_t GetColorTableEntry(int32_t index) const noexcept;

GETSET_PROPERTY(uint32_t, DefaultForeground, DEFAULT_FOREGROUND_WITH_ALPHA);
GETSET_PROPERTY(uint32_t, DefaultBackground, DEFAULT_BACKGROUND_WITH_ALPHA);
GETSET_PROPERTY(uint32_t, SelectionBackground, DEFAULT_FOREGROUND);
GETSET_PROPERTY(int32_t, HistorySize, DEFAULT_HISTORY_SIZE);
GETSET_PROPERTY(int32_t, InitialRows, 30);
GETSET_PROPERTY(int32_t, InitialCols, 80);

GETSET_PROPERTY(bool, SnapOnInput, true);
GETSET_PROPERTY(bool, AltGrAliasing, true);
GETSET_PROPERTY(uint32_t, CursorColor, DEFAULT_CURSOR_COLOR);
GETSET_PROPERTY(Microsoft::Terminal::TerminalControl::CursorStyle, CursorShape, Microsoft::Terminal::TerminalControl::CursorStyle::Vintage);
GETSET_PROPERTY(uint32_t, CursorHeight, DEFAULT_CURSOR_HEIGHT);
GETSET_PROPERTY(hstring, WordDelimiters, DEFAULT_WORD_DELIMITERS);
GETSET_PROPERTY(bool, CopyOnSelect, false);

GETSET_PROPERTY(Windows::Foundation::IReference<uint32_t>, TabColor, nullptr);
uint32_t GetColorTableEntry(int32_t index) noexcept;
void ColorTable(std::array<uint32_t, 16> colors);
std::array<uint32_t, 16> ColorTable();

GETSET_SETTING(uint32_t, DefaultForeground, DEFAULT_FOREGROUND_WITH_ALPHA);
GETSET_SETTING(uint32_t, DefaultBackground, DEFAULT_BACKGROUND_WITH_ALPHA);
GETSET_SETTING(uint32_t, SelectionBackground, DEFAULT_FOREGROUND);
GETSET_SETTING(int32_t, HistorySize, DEFAULT_HISTORY_SIZE);
GETSET_SETTING(int32_t, InitialRows, 30);
GETSET_SETTING(int32_t, InitialCols, 80);

GETSET_SETTING(bool, SnapOnInput, true);
GETSET_SETTING(bool, AltGrAliasing, true);
GETSET_SETTING(uint32_t, CursorColor, DEFAULT_CURSOR_COLOR);
GETSET_SETTING(Microsoft::Terminal::TerminalControl::CursorStyle, CursorShape, Microsoft::Terminal::TerminalControl::CursorStyle::Vintage);
GETSET_SETTING(uint32_t, CursorHeight, DEFAULT_CURSOR_HEIGHT);
GETSET_SETTING(hstring, WordDelimiters, DEFAULT_WORD_DELIMITERS);
GETSET_SETTING(bool, CopyOnSelect, false);

GETSET_SETTING(Windows::Foundation::IReference<uint32_t>, TabColor, nullptr);

// When set, StartingTabColor allows to create a terminal with a "sticky" tab color.
// This color is prioritized above the TabColor (that is usually initialized based on profile settings).
Expand All @@ -78,55 +78,54 @@ namespace winrt::TerminalApp::implementation
// TODO: to ensure that this property is not populated during settings reload,
// we should consider moving this property to a separate interface,
// passed to the terminal only upon creation.
GETSET_PROPERTY(Windows::Foundation::IReference<uint32_t>, StartingTabColor, nullptr);
GETSET_SETTING(Windows::Foundation::IReference<uint32_t>, StartingTabColor, nullptr);

// ------------------------ End of Core Settings -----------------------

GETSET_PROPERTY(hstring, ProfileName);
GETSET_PROPERTY(bool, UseAcrylic, false);
GETSET_PROPERTY(double, TintOpacity, 0.5);
GETSET_PROPERTY(hstring, Padding, DEFAULT_PADDING);
GETSET_PROPERTY(hstring, FontFace, DEFAULT_FONT_FACE);
GETSET_PROPERTY(int32_t, FontSize, DEFAULT_FONT_SIZE);
GETSET_SETTING(hstring, ProfileName);
GETSET_SETTING(bool, UseAcrylic, false);
GETSET_SETTING(double, TintOpacity, 0.5);
GETSET_SETTING(hstring, Padding, DEFAULT_PADDING);
GETSET_SETTING(hstring, FontFace, DEFAULT_FONT_FACE);
GETSET_SETTING(int32_t, FontSize, DEFAULT_FONT_SIZE);

GETSET_PROPERTY(winrt::Windows::UI::Text::FontWeight, FontWeight);
GETSET_SETTING(winrt::Windows::UI::Text::FontWeight, FontWeight);

GETSET_PROPERTY(hstring, BackgroundImage);
GETSET_PROPERTY(double, BackgroundImageOpacity, 1.0);
GETSET_SETTING(hstring, BackgroundImage);
GETSET_SETTING(double, BackgroundImageOpacity, 1.0);

GETSET_PROPERTY(winrt::Windows::UI::Xaml::Media::Stretch,
BackgroundImageStretchMode,
winrt::Windows::UI::Xaml::Media::Stretch::UniformToFill);
GETSET_PROPERTY(winrt::Windows::UI::Xaml::HorizontalAlignment,
BackgroundImageHorizontalAlignment,
winrt::Windows::UI::Xaml::HorizontalAlignment::Center);
GETSET_PROPERTY(winrt::Windows::UI::Xaml::VerticalAlignment,
BackgroundImageVerticalAlignment,
winrt::Windows::UI::Xaml::VerticalAlignment::Center);
GETSET_SETTING(winrt::Windows::UI::Xaml::Media::Stretch,
BackgroundImageStretchMode,
winrt::Windows::UI::Xaml::Media::Stretch::UniformToFill);
GETSET_SETTING(winrt::Windows::UI::Xaml::HorizontalAlignment,
BackgroundImageHorizontalAlignment,
winrt::Windows::UI::Xaml::HorizontalAlignment::Center);
GETSET_SETTING(winrt::Windows::UI::Xaml::VerticalAlignment,
BackgroundImageVerticalAlignment,
winrt::Windows::UI::Xaml::VerticalAlignment::Center);

GETSET_PROPERTY(Microsoft::Terminal::TerminalControl::IKeyBindings, KeyBindings, nullptr);
GETSET_SETTING(Microsoft::Terminal::TerminalControl::IKeyBindings, KeyBindings, nullptr);

GETSET_PROPERTY(hstring, Commandline);
GETSET_PROPERTY(hstring, StartingDirectory);
GETSET_PROPERTY(hstring, StartingTitle);
GETSET_PROPERTY(bool, SuppressApplicationTitle);
GETSET_PROPERTY(hstring, EnvironmentVariables);
GETSET_SETTING(hstring, Commandline);
GETSET_SETTING(hstring, StartingDirectory);
GETSET_SETTING(hstring, StartingTitle);
GETSET_SETTING(bool, SuppressApplicationTitle);
GETSET_SETTING(hstring, EnvironmentVariables);

GETSET_PROPERTY(Microsoft::Terminal::TerminalControl::ScrollbarState, ScrollState, Microsoft::Terminal::TerminalControl::ScrollbarState::Visible);
GETSET_SETTING(Microsoft::Terminal::TerminalControl::ScrollbarState, ScrollState, Microsoft::Terminal::TerminalControl::ScrollbarState::Visible);

GETSET_PROPERTY(Microsoft::Terminal::TerminalControl::TextAntialiasingMode, AntialiasingMode, Microsoft::Terminal::TerminalControl::TextAntialiasingMode::Grayscale);
GETSET_SETTING(Microsoft::Terminal::TerminalControl::TextAntialiasingMode, AntialiasingMode, Microsoft::Terminal::TerminalControl::TextAntialiasingMode::Grayscale);

GETSET_PROPERTY(bool, RetroTerminalEffect, false);
GETSET_PROPERTY(bool, ForceFullRepaintRendering, false);
GETSET_PROPERTY(bool, SoftwareRendering, false);
GETSET_PROPERTY(bool, ForceVTInput, false);
GETSET_SETTING(bool, RetroTerminalEffect, false);
GETSET_SETTING(bool, ForceFullRepaintRendering, false);
GETSET_SETTING(bool, SoftwareRendering, false);
GETSET_SETTING(bool, ForceVTInput, false);

GETSET_PROPERTY(hstring, PixelShaderPath);
#pragma warning(pop)

private:
std::array<uint32_t, COLOR_TABLE_SIZE> _colorTable{};

std::optional<std::array<uint32_t, COLOR_TABLE_SIZE>> _ColorTable;
gsl::span<uint32_t> _getColorTableImpl();
void _ApplyProfileSettings(const Microsoft::Terminal::Settings::Model::Profile& profile, const Windows::Foundation::Collections::IMapView<hstring, Microsoft::Terminal::Settings::Model::ColorScheme>& schemes);
void _ApplyGlobalSettings(const Microsoft::Terminal::Settings::Model::GlobalAppSettings& globalSettings) noexcept;

Expand Down
9 changes: 2 additions & 7 deletions src/cascadia/TerminalControl/TermControl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -278,12 +278,11 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
// Method Description:
// - Given new settings for this profile, applies the settings to the current terminal.
// Arguments:
// - newSettings: New settings values for the profile in this terminal.
// - <none>
// Return Value:
// - <none>
winrt::fire_and_forget TermControl::UpdateSettings(IControlSettings newSettings)
winrt::fire_and_forget TermControl::UpdateSettings()
{
_settings = newSettings;
auto weakThis{ get_weak() };

// Dispatch a call to the UI thread to apply the new settings to the
Expand Down Expand Up @@ -560,10 +559,6 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
{
solidColor.Color(newBgColor);
}

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

Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalControl/TermControl.h
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
{
TermControl(IControlSettings settings, TerminalConnection::ITerminalConnection connection);

winrt::fire_and_forget UpdateSettings(IControlSettings newSettings);
winrt::fire_and_forget UpdateSettings();

hstring Title();
hstring GetProfileName() const;
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalControl/TermControl.idl
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ namespace Microsoft.Terminal.TerminalControl

static Windows.Foundation.Size GetProposedDimensions(Microsoft.Terminal.TerminalControl.IControlSettings settings, UInt32 dpi);

void UpdateSettings(Microsoft.Terminal.TerminalControl.IControlSettings newSettings);
void UpdateSettings();

Microsoft.Terminal.TerminalControl.IControlSettings Settings { get; };

Expand Down
5 changes: 4 additions & 1 deletion src/cascadia/TerminalCore/Terminal.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -112,8 +112,11 @@ void Terminal::CreateFromSettings(ICoreSettings settings,
// - settings: an ICoreSettings with new settings values for us to use.
void Terminal::UpdateSettings(ICoreSettings settings)
{
// Set the default background as transparent to prevent the
// DX layer from overwriting the background image or acrylic effect
til::color newBackgroundColor{ static_cast<COLORREF>(settings.DefaultBackground()) };
_defaultBg = newBackgroundColor.with_alpha(0);
_defaultFg = settings.DefaultForeground();
_defaultBg = settings.DefaultBackground();

CursorType cursorShape = CursorType::VerticalBar;
switch (settings.CursorShape())
Expand Down
5 changes: 5 additions & 0 deletions src/cascadia/TerminalSettingsModel/IInheritable.h
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,11 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
return child;
}

void ClearParents()
{
_parents.clear();
}

void InsertParent(com_ptr<T> parent)
{
_parents.push_back(parent);
Expand Down
5 changes: 5 additions & 0 deletions src/types/colorTable.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -475,6 +475,11 @@ void Utils::InitializeCampbellColorTable(const gsl::span<til::color> table)
std::copy(campbellColorTable.begin(), campbellColorTable.end(), table.begin());
}

gsl::span<const til::color> Utils::CampbellColorTable()
{
return gsl::make_span(campbellColorTable);
}

// Function Description:
// - Fill the first 16 entries of a given color table with the Campbell color
// scheme, in the Windows BGR order.
Expand Down
1 change: 1 addition & 0 deletions src/types/inc/colorTable.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ namespace Microsoft::Console::Utils
void InitializeCampbellColorTableForConhost(const gsl::span<COLORREF> table);
void SwapANSIColorOrderForConhost(const gsl::span<COLORREF> table);
void Initialize256ColorTable(const gsl::span<COLORREF> table);
gsl::span<const til::color> CampbellColorTable();

std::optional<til::color> ColorFromXOrgAppColorName(const std::wstring_view wstr) noexcept;

Expand Down

0 comments on commit 9047bbb

Please sign in to comment.