From 62b2a90aef73020e8dae491f79521a1fdfd29828 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Mon, 12 Oct 2020 14:47:35 -0700 Subject: [PATCH 01/29] Make Global and Profile settings inheritable --- .../TerminalSettingsModel/CascadiaSettings.h | 3 +- .../CascadiaSettingsSerialization.cpp | 56 +++++-- .../TerminalSettingsModel/GlobalAppSettings.h | 53 +++--- .../TerminalSettingsModel/IInheritable.h | 156 ++++++++++++++++++ ...crosoft.Terminal.Settings.ModelLib.vcxproj | 1 + .../TerminalSettingsModel/Profile.cpp | 90 +++++++--- src/cascadia/TerminalSettingsModel/Profile.h | 73 ++++---- src/inc/til/coalesce.h | 47 ++++++ 8 files changed, 385 insertions(+), 94 deletions(-) create mode 100644 src/cascadia/TerminalSettingsModel/IInheritable.h diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.h b/src/cascadia/TerminalSettingsModel/CascadiaSettings.h index 410710b90b4..fe98eeb2da1 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.h +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettings.h @@ -101,10 +101,11 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation std::string _userSettingsString; Json::Value _userSettings; Json::Value _defaultSettings; - Json::Value _userDefaultProfileSettings{ Json::Value::null }; + winrt::com_ptr _userDefaultProfileSettings{ nullptr }; void _LayerOrCreateProfile(const Json::Value& profileJson); winrt::com_ptr _FindMatchingProfile(const Json::Value& profileJson); + std::optional _FindMatchingProfileIndex(const Json::Value& profileJson); void _LayerOrCreateColorScheme(const Json::Value& schemeJson); winrt::com_ptr _FindMatchingColorScheme(const Json::Value& schemeJson); void _ParseJsonString(std::string_view fileData, const bool isDefaultSettings); diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp index a3fa1c6fc3e..9de7616eb8a 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp @@ -590,6 +590,8 @@ winrt::com_ptr CascadiaSettings::FromJson(const Json::Value& j // void CascadiaSettings::LayerJson(const Json::Value& json) { + // add a new inheritance layer, and apply json values to child + _globals.attach(_globals->CreateChild()); _globals->LayerJson(json); if (auto schemes{ json[SchemesKey.data()] }) @@ -627,10 +629,14 @@ void CascadiaSettings::LayerJson(const Json::Value& json) void CascadiaSettings::_LayerOrCreateProfile(const Json::Value& profileJson) { // Layer the json on top of an existing profile, if we have one: - auto pProfile = _FindMatchingProfile(profileJson); - if (pProfile) + auto profileIndex{ _FindMatchingProfileIndex(profileJson) }; + if (profileIndex) { - pProfile->LayerJson(profileJson); + // add a new inheritance layer, and apply json values to child + auto parentProj{ _profiles.GetAt(*profileIndex) }; + auto parent{ winrt::get_self(parentProj) }; + auto childImpl{ parent->CreateChild() }; + childImpl->LayerJson(profileJson); } else { @@ -639,13 +645,13 @@ void CascadiaSettings::_LayerOrCreateProfile(const Json::Value& profileJson) // `source`. Dynamic profiles _must_ be layered on an existing profile. if (!Profile::IsDynamicProfileObject(profileJson)) { - auto profile = winrt::make_self(); + auto profile{ winrt::make_self() }; // GH#2325: If we have a set of default profile settings, apply them here. // We _won't_ have these settings yet for defaults, dynamic profiles. if (_userDefaultProfileSettings) { - profile->LayerJson(_userDefaultProfileSettings); + profile.attach(_userDefaultProfileSettings->CreateChild()); } profile->LayerJson(profileJson); @@ -667,15 +673,38 @@ void CascadiaSettings::_LayerOrCreateProfile(const Json::Value& profileJson) // profile exists. winrt::com_ptr CascadiaSettings::_FindMatchingProfile(const Json::Value& profileJson) { - for (auto profile : _profiles) + auto index{ _FindMatchingProfileIndex(profileJson) }; + if (index) { - auto profileImpl = winrt::get_self(profile); + auto profile{ _profiles.GetAt(*index) }; + auto profileImpl{ winrt::get_self(profile) }; + return profileImpl->get_strong(); + } + return nullptr; +} + +// Method Description: +// - Finds a profile from our list of profiles that matches the given json +// object. Uses Profile::ShouldBeLayered to determine if the Json::Value is a +// match or not. This method should be used to find a profile to layer the +// given settings upon. +// - Returns nullopt if no such match exists. +// Arguments: +// - json: an object which may be a partial serialization of a Profile object. +// Return Value: +// - The index for the matching Profile, iff it exists. Otherwise, nullopt. +std::optional CascadiaSettings::_FindMatchingProfileIndex(const Json::Value& profileJson) +{ + for (uint32_t i = 0; i < _profiles.Size(); ++i) + { + const auto profile{ _profiles.GetAt(i) }; + const auto profileImpl = winrt::get_self(profile); if (profileImpl->ShouldBeLayered(profileJson)) { - return profileImpl->get_strong(); + return i; } } - return nullptr; + return std::nullopt; } // Method Description: @@ -706,16 +735,17 @@ void CascadiaSettings::_ApplyDefaultsFromUserSettings() // from user settings file if (defaultSettings) { - _userDefaultProfileSettings = defaultSettings; - // Remove the `guid` member from the default settings. That'll // hyper-explode, so just don't let them do that. - _userDefaultProfileSettings.removeMember({ "guid" }); + defaultSettings.removeMember({ "guid" }); + + _userDefaultProfileSettings = winrt::make_self(); + _userDefaultProfileSettings->LayerJson(defaultSettings); for (auto profile : _profiles) { auto profileImpl = winrt::get_self(profile); - profileImpl->LayerJson(_userDefaultProfileSettings); + _userDefaultProfileSettings->ApplyTo(profileImpl); } } } diff --git a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.h b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.h index 39eb18f6549..47228bcd25c 100644 --- a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.h +++ b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.h @@ -3,7 +3,7 @@ Copyright (c) Microsoft Corporation Licensed under the MIT license. Module Name: -- CascadiaSettings.hpp +- GlobalAppSettings.h Abstract: - This class encapsulates all of the settings that are global to the app, and @@ -16,6 +16,7 @@ Author(s): #pragma once #include "GlobalAppSettings.g.h" +#include "IInheritable.h" #include "KeyMapping.h" #include "Command.h" @@ -30,7 +31,7 @@ namespace SettingsModelLocalTests namespace winrt::Microsoft::Terminal::Settings::Model::implementation { - struct GlobalAppSettings : GlobalAppSettingsT + struct GlobalAppSettings : GlobalAppSettingsT, IInheritable { public: GlobalAppSettings(); @@ -53,30 +54,30 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation guid DefaultProfile() const; hstring UnparsedDefaultProfile() const; - GETSET_PROPERTY(int32_t, InitialRows, DEFAULT_ROWS); - GETSET_PROPERTY(int32_t, InitialCols, DEFAULT_COLS); - GETSET_PROPERTY(bool, AlwaysShowTabs, true); - GETSET_PROPERTY(bool, ShowTitleInTitlebar, true); - GETSET_PROPERTY(bool, ConfirmCloseAllTabs, true); - GETSET_PROPERTY(winrt::Windows::UI::Xaml::ElementTheme, Theme, winrt::Windows::UI::Xaml::ElementTheme::Default); - GETSET_PROPERTY(winrt::Microsoft::UI::Xaml::Controls::TabViewWidthMode, TabWidthMode, winrt::Microsoft::UI::Xaml::Controls::TabViewWidthMode::Equal); - GETSET_PROPERTY(bool, ShowTabsInTitlebar, true); - GETSET_PROPERTY(hstring, WordDelimiters, DEFAULT_WORD_DELIMITERS); - GETSET_PROPERTY(bool, CopyOnSelect, false); - GETSET_PROPERTY(winrt::Microsoft::Terminal::TerminalControl::CopyFormat, CopyFormatting, 0); - GETSET_PROPERTY(bool, WarnAboutLargePaste, true); - GETSET_PROPERTY(bool, WarnAboutMultiLinePaste, true); - GETSET_PROPERTY(Model::LaunchPosition, InitialPosition, nullptr, nullptr); - GETSET_PROPERTY(Model::LaunchMode, LaunchMode, LaunchMode::DefaultMode); - GETSET_PROPERTY(bool, SnapToGridOnResize, true); - GETSET_PROPERTY(bool, ForceFullRepaintRendering, false); - GETSET_PROPERTY(bool, SoftwareRendering, false); - GETSET_PROPERTY(bool, ForceVTInput, false); - GETSET_PROPERTY(bool, DebugFeaturesEnabled); // default value set in constructor - GETSET_PROPERTY(bool, StartOnUserLogin, false); - GETSET_PROPERTY(bool, AlwaysOnTop, false); - GETSET_PROPERTY(bool, UseTabSwitcher, true); - GETSET_PROPERTY(bool, DisableAnimations, false); + GETSET_SETTING(int32_t, InitialRows, DEFAULT_ROWS); + GETSET_SETTING(int32_t, InitialCols, DEFAULT_COLS); + GETSET_SETTING(bool, AlwaysShowTabs, true); + GETSET_SETTING(bool, ShowTitleInTitlebar, true); + GETSET_SETTING(bool, ConfirmCloseAllTabs, true); + GETSET_SETTING(winrt::Windows::UI::Xaml::ElementTheme, Theme, winrt::Windows::UI::Xaml::ElementTheme::Default); + GETSET_SETTING(winrt::Microsoft::UI::Xaml::Controls::TabViewWidthMode, TabWidthMode, winrt::Microsoft::UI::Xaml::Controls::TabViewWidthMode::Equal); + GETSET_SETTING(bool, ShowTabsInTitlebar, true); + GETSET_SETTING(hstring, WordDelimiters, DEFAULT_WORD_DELIMITERS); + GETSET_SETTING(bool, CopyOnSelect, false); + GETSET_SETTING(winrt::Microsoft::Terminal::TerminalControl::CopyFormat, CopyFormatting, 0); + GETSET_SETTING(bool, WarnAboutLargePaste, true); + GETSET_SETTING(bool, WarnAboutMultiLinePaste, true); + GETSET_SETTING(Model::LaunchPosition, InitialPosition, nullptr, nullptr); + GETSET_SETTING(Model::LaunchMode, LaunchMode, LaunchMode::DefaultMode); + GETSET_SETTING(bool, SnapToGridOnResize, true); + GETSET_SETTING(bool, ForceFullRepaintRendering, false); + GETSET_SETTING(bool, SoftwareRendering, false); + GETSET_SETTING(bool, ForceVTInput, false); + GETSET_SETTING(bool, DebugFeaturesEnabled); // default value set in constructor + GETSET_SETTING(bool, StartOnUserLogin, false); + GETSET_SETTING(bool, AlwaysOnTop, false); + GETSET_SETTING(bool, UseTabSwitcher, true); + GETSET_SETTING(bool, DisableAnimations, false); private: hstring _unparsedDefaultProfile; diff --git a/src/cascadia/TerminalSettingsModel/IInheritable.h b/src/cascadia/TerminalSettingsModel/IInheritable.h new file mode 100644 index 00000000000..4a63213fa16 --- /dev/null +++ b/src/cascadia/TerminalSettingsModel/IInheritable.h @@ -0,0 +1,156 @@ +/*++ +Copyright (c) Microsoft Corporation +Licensed under the MIT license. + +Module Name: +- IInheritable.h + +Abstract: +- An interface allowing settings objects to inherit settings from a parent + +Author(s): +- Carlos Zamora - October 2020 + +--*/ +#pragma once + +namespace winrt::Microsoft::Terminal::Settings::Model::implementation +{ + template + struct IInheritable + { + public: + T* CreateChild() const + { + auto child{ winrt::make_self() }; + child->_parent.attach(const_cast(static_cast(this))); + return child.detach(); + } + + protected: + com_ptr _parent{ nullptr }; + }; + + // Nullable settings are settings that explicitly allow null to mean something + template + struct NullableSetting + { + winrt::Windows::Foundation::IReference setting{ nullptr }; + bool set{ false }; + }; +} + +// Use this macro to quickly implement both getters and the setter for an +// inheritable and observable setting property. This is similar to the GETSET_PROPERTY macro, except... +// - Has(): checks if the user explicitly set a value for this setting +// - Getter(): return the resolved value +// - Setter(): set the value directly +// - Clear(): clear the user set value +// - the setting is saved as an optional, where nullopt means +// that we must inherit the value from our parent +#define GETSET_SETTING(type, name, ...) \ +public: \ + /* Returns true if the user explicitly set the value, false otherwise*/ \ + bool Has##name() const \ + { \ + return _##name.has_value(); \ + }; \ + \ + /* Returns the resolved value for this setting */ \ + /* fallback: user set value --> inherited value --> system set value */ \ + type name() const \ + { \ + const auto val{ _get##name##Impl() }; \ + return val ? *val : type{ __VA_ARGS__ }; \ + }; \ + \ + /* Overwrite the user set value */ \ + /* Dispatch event if value changed */ \ + void name(const type& value) \ + { \ + if (_##name != value) \ + { \ + _##name = value; \ + } \ + }; \ + \ + /* Clear the user set value */ \ + /* Dispatch event if value changed */ \ + void Clear##name() \ + { \ + if (Has##name()) \ + { \ + _##name = std::nullopt; \ + } \ + }; \ + \ +private: \ + std::optional _##name{ std::nullopt }; \ + std::optional _get##name##Impl() const \ + { \ + return _##name ? \ + _##name : \ + (_parent ? \ + _parent->_get##name##Impl() : \ + type{ __VA_ARGS__ }); \ + }; + +// This macro is similar to the one above, but is reserved for optional settings +// like Profile.StartingDirectory and Profile.Foreground (where null is interpreted +// as an acceptable value, rather than "inherit") +// "type" is exposed as an IReference +#define GETSET_NULLABLE_SETTING(type, name, ...) \ +public: \ + /* Returns true if the user explicitly set the value, false otherwise*/ \ + bool Has##name() const \ + { \ + return _##name.set; \ + }; \ + \ + /* Returns the resolved value for this setting */ \ + /* fallback: user set value --> inherited value --> system set value */ \ + winrt::Windows::Foundation::IReference name() const \ + { \ + return _get##name##Impl(); \ + }; \ + \ + /* Overwrite the user set value */ \ + /* Dispatch event if value changed */ \ + void name(const winrt::Windows::Foundation::IReference& value) \ + { \ + if (!Has##name() /*value was not set*/ \ + || _##name.setting != value) /*set value is different*/ \ + { \ + _##name.setting = value; \ + _##name.set = true; \ + } \ + }; \ + \ + /* Clear the user set value */ \ + /* Dispatch event if value changed */ \ + void Clear##name() \ + { \ + if (Has##name()) \ + { \ + _##name.set = false; \ + } \ + }; \ + \ +private: \ + NullableSetting _##name{}; \ + winrt::Windows::Foundation::IReference _get##name##Impl() const \ + { \ + return Has##name() ? \ + _##name.setting : \ + (_parent ? \ + _parent->_get##name##Impl() : \ + winrt::Windows::Foundation::IReference{ __VA_ARGS__ }); \ + }; + +// Use this macro for Profile::ApplyTo +// Checks if a given setting is set, and if it is, sets it on profile +#define APPLY_OUT(setting) \ + if (Has##setting()) \ + { \ + profile->_##setting = _##setting; \ + } diff --git a/src/cascadia/TerminalSettingsModel/Microsoft.Terminal.Settings.ModelLib.vcxproj b/src/cascadia/TerminalSettingsModel/Microsoft.Terminal.Settings.ModelLib.vcxproj index 37f928fdf3f..c3ef92bc888 100644 --- a/src/cascadia/TerminalSettingsModel/Microsoft.Terminal.Settings.ModelLib.vcxproj +++ b/src/cascadia/TerminalSettingsModel/Microsoft.Terminal.Settings.ModelLib.vcxproj @@ -43,6 +43,7 @@ GlobalAppSettings.idl + diff --git a/src/cascadia/TerminalSettingsModel/Profile.cpp b/src/cascadia/TerminalSettingsModel/Profile.cpp index d3419d34255..1d21f03a6e5 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.cpp +++ b/src/cascadia/TerminalSettingsModel/Profile.cpp @@ -88,14 +88,20 @@ Json::Value Profile::GenerateStub() const stub[JsonKey(GuidKey)] = winrt::to_string(Utils::GuidToString(*_Guid)); } - stub[JsonKey(NameKey)] = winrt::to_string(_Name); + if (_Name) + { + stub[JsonKey(NameKey)] = winrt::to_string(*_Name); + } - if (!_Source.empty()) + if (_Source && !_Source.value().empty()) { - stub[JsonKey(SourceKey)] = winrt::to_string(_Source); + stub[JsonKey(SourceKey)] = winrt::to_string(*_Source); } - stub[JsonKey(HiddenKey)] = _Hidden; + if (_Hidden) + { + stub[JsonKey(HiddenKey)] = *_Hidden; + } return stub; } @@ -151,12 +157,12 @@ bool Profile::ShouldBeLayered(const Json::Value& json) const // For profiles with a `source`, also check the `source` property. bool sourceMatches = false; - if (!_Source.empty()) + if (_Source && !_Source.value().empty()) { if (otherHadSource) { // If we have a source and the other has a source, compare them! - sourceMatches = *otherSource == _Source; + sourceMatches = *otherSource == *_Source; } else { @@ -164,9 +170,9 @@ bool Profile::ShouldBeLayered(const Json::Value& json) const // `this` is a dynamic profile with a source, and our _source is one // of the legacy DPG namespaces. We're looking to see if the other // json object has the same guid, but _no_ "source" - if (_Source == WslGeneratorNamespace || - _Source == AzureGeneratorNamespace || - _Source == PowershellCoreGeneratorNamespace) + if (*_Source == WslGeneratorNamespace || + *_Source == AzureGeneratorNamespace || + *_Source == PowershellCoreGeneratorNamespace) { sourceMatches = true; } @@ -201,10 +207,10 @@ void Profile::LayerJson(const Json::Value& json) JsonUtils::GetValueForKey(json, HiddenKey, _Hidden); // Core Settings - JsonUtils::GetValueForKey(json, ForegroundKey, _Foreground); - JsonUtils::GetValueForKey(json, BackgroundKey, _Background); - JsonUtils::GetValueForKey(json, SelectionBackgroundKey, _SelectionBackground); - JsonUtils::GetValueForKey(json, CursorColorKey, _CursorColor); + _Foreground.set = JsonUtils::GetValueForKey(json, ForegroundKey, _Foreground.setting); + _Background.set = JsonUtils::GetValueForKey(json, BackgroundKey, _Background.setting); + _SelectionBackground.set = JsonUtils::GetValueForKey(json, SelectionBackgroundKey, _SelectionBackground.setting); + _CursorColor.set = JsonUtils::GetValueForKey(json, CursorColorKey, _CursorColor.setting); JsonUtils::GetValueForKey(json, ColorSchemeKey, _ColorSchemeName); // TODO:MSFT:20642297 - Use a sentinel value (-1) for "Infinite scrollback" @@ -240,7 +246,7 @@ void Profile::LayerJson(const Json::Value& json) JsonUtils::GetValueForKey(json, RetroTerminalEffectKey, _RetroTerminalEffect); JsonUtils::GetValueForKey(json, AntialiasingModeKey, _AntialiasingMode); - JsonUtils::GetValueForKey(json, TabColorKey, _TabColor); + _TabColor.set = JsonUtils::GetValueForKey(json, TabColorKey, _TabColor.setting); } // Method Description: @@ -250,16 +256,18 @@ void Profile::LayerJson(const Json::Value& json) // - This profile's expanded background image path / the empty string. winrt::hstring Profile::ExpandedBackgroundImagePath() const { - if (_BackgroundImagePath.empty()) + const auto path{ BackgroundImagePath() }; + if (path.empty()) { - return _BackgroundImagePath; + return path; } - return winrt::hstring{ wil::ExpandEnvironmentStringsW(_BackgroundImagePath.c_str()) }; + return winrt::hstring{ wil::ExpandEnvironmentStringsW(path.c_str()) }; } winrt::hstring Profile::EvaluatedStartingDirectory() const { - return winrt::hstring{ Profile::EvaluateStartingDirectory(_StartingDirectory.c_str()) }; + const auto path{ StartingDirectory() }; + return winrt::hstring{ Profile::EvaluateStartingDirectory(path.c_str()) }; } // Method Description: @@ -303,7 +311,7 @@ void Profile::GenerateGuidIfNecessary() noexcept { // Always use the name to generate the temporary GUID. That way, across // reloads, we'll generate the same static GUID. - _Guid = Profile::_GenerateGuidForProfile(_Name, _Source); + _Guid = Profile::_GenerateGuidForProfile(Name(), Source()); TraceLoggingWrite( g_hSettingsModelProvider, @@ -328,6 +336,50 @@ bool Profile::IsDynamicProfileObject(const Json::Value& json) return !source.isNull(); } +// Function Description: +// - Apply each of the current Profile's settings values onto the given Profile, if it is set +// Arguments: +// - profile: the Profile to be updated +// Return Value: +// - +void Profile::ApplyTo(Profile* profile) const +{ + APPLY_OUT(Name); + APPLY_OUT(Source); + APPLY_OUT(Hidden); + APPLY_OUT(Icon); + APPLY_OUT(CloseOnExit); + APPLY_OUT(TabTitle); + APPLY_OUT(TabColor); + APPLY_OUT(SuppressApplicationTitle); + APPLY_OUT(UseAcrylic); + APPLY_OUT(AcrylicOpacity); + APPLY_OUT(ScrollState); + APPLY_OUT(FontFace); + APPLY_OUT(FontSize); + APPLY_OUT(FontWeight); + APPLY_OUT(Padding); + APPLY_OUT(Commandline); + APPLY_OUT(StartingDirectory); + APPLY_OUT(BackgroundImagePath); + APPLY_OUT(BackgroundImageOpacity); + APPLY_OUT(BackgroundImageStretchMode); + APPLY_OUT(AntialiasingMode); + APPLY_OUT(RetroTerminalEffect); + APPLY_OUT(ForceFullRepaintRendering); + APPLY_OUT(SoftwareRendering); + APPLY_OUT(ColorSchemeName); + APPLY_OUT(Foreground); + APPLY_OUT(Background); + APPLY_OUT(SelectionBackground); + APPLY_OUT(CursorColor); + APPLY_OUT(HistorySize); + APPLY_OUT(SnapOnInput); + APPLY_OUT(AltGrAliasing); + APPLY_OUT(CursorShape); + APPLY_OUT(CursorHeight); +} + // Function Description: // - Generates a unique guid for a profile, given the name. For an given name, will always return the same GUID. // Arguments: diff --git a/src/cascadia/TerminalSettingsModel/Profile.h b/src/cascadia/TerminalSettingsModel/Profile.h index eec8d1dda9a..ce4b8315696 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.h +++ b/src/cascadia/TerminalSettingsModel/Profile.h @@ -16,6 +16,7 @@ Author(s): #pragma once #include "Profile.g.h" +#include "IInheritable.h" #include "../inc/cppwinrt_utils.h" #include "JsonUtils.h" @@ -39,7 +40,7 @@ constexpr GUID RUNTIME_GENERATED_PROFILE_NAMESPACE_GUID = { 0xf65ddb7e, 0x706b, namespace winrt::Microsoft::Terminal::Settings::Model::implementation { - struct Profile : ProfileT + struct Profile : ProfileT, IInheritable { public: Profile(); @@ -50,6 +51,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation bool ShouldBeLayered(const Json::Value& json) const; void LayerJson(const Json::Value& json); static bool IsDynamicProfileObject(const Json::Value& json); + void ApplyTo(Profile* profile) const; hstring EvaluatedStartingDirectory() const; hstring ExpandedBackgroundImagePath() const; @@ -70,50 +72,51 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation const Windows::UI::Xaml::VerticalAlignment BackgroundImageVerticalAlignment() const noexcept; void BackgroundImageVerticalAlignment(const Windows::UI::Xaml::VerticalAlignment& value) noexcept; - GETSET_PROPERTY(hstring, Name, L"Default"); - GETSET_PROPERTY(hstring, Source); - GETSET_PROPERTY(bool, Hidden, false); + GETSET_SETTING(hstring, Name, L"Default"); + GETSET_SETTING(hstring, Source); + GETSET_SETTING(bool, Hidden, false); - GETSET_PROPERTY(hstring, Icon); + GETSET_SETTING(hstring, Icon); - GETSET_PROPERTY(CloseOnExitMode, CloseOnExit, CloseOnExitMode::Graceful); - GETSET_PROPERTY(hstring, TabTitle); - GETSET_PROPERTY(Windows::Foundation::IReference, TabColor); - GETSET_PROPERTY(bool, SuppressApplicationTitle, false); + GETSET_SETTING(CloseOnExitMode, CloseOnExit, CloseOnExitMode::Graceful); + GETSET_SETTING(hstring, TabTitle); + GETSET_NULLABLE_SETTING(Windows::UI::Color, TabColor, nullptr); + GETSET_SETTING(bool, SuppressApplicationTitle, false); - GETSET_PROPERTY(bool, UseAcrylic, false); - GETSET_PROPERTY(double, AcrylicOpacity, 0.5); - GETSET_PROPERTY(Microsoft::Terminal::TerminalControl::ScrollbarState, ScrollState, Microsoft::Terminal::TerminalControl::ScrollbarState::Visible); + GETSET_SETTING(bool, UseAcrylic, false); + GETSET_SETTING(double, AcrylicOpacity, 0.5); + GETSET_SETTING(Microsoft::Terminal::TerminalControl::ScrollbarState, ScrollState, Microsoft::Terminal::TerminalControl::ScrollbarState::Visible); - GETSET_PROPERTY(hstring, FontFace, DEFAULT_FONT_FACE); - GETSET_PROPERTY(int32_t, FontSize, DEFAULT_FONT_SIZE); - GETSET_PROPERTY(Windows::UI::Text::FontWeight, FontWeight, DEFAULT_FONT_WEIGHT); - GETSET_PROPERTY(hstring, Padding, DEFAULT_PADDING); + GETSET_SETTING(hstring, FontFace, DEFAULT_FONT_FACE); + GETSET_SETTING(int32_t, FontSize, DEFAULT_FONT_SIZE); + GETSET_SETTING(Windows::UI::Text::FontWeight, FontWeight, DEFAULT_FONT_WEIGHT); + GETSET_SETTING(hstring, Padding, DEFAULT_PADDING); - GETSET_PROPERTY(hstring, Commandline, L"cmd.exe"); - GETSET_PROPERTY(hstring, StartingDirectory); + GETSET_SETTING(hstring, Commandline, L"cmd.exe"); + GETSET_SETTING(hstring, StartingDirectory); - GETSET_PROPERTY(hstring, BackgroundImagePath); - GETSET_PROPERTY(double, BackgroundImageOpacity, 1.0); - GETSET_PROPERTY(Windows::UI::Xaml::Media::Stretch, BackgroundImageStretchMode, Windows::UI::Xaml::Media::Stretch::Fill); + GETSET_SETTING(hstring, BackgroundImagePath); + GETSET_SETTING(double, BackgroundImageOpacity, 1.0); + GETSET_SETTING(Windows::UI::Xaml::Media::Stretch, BackgroundImageStretchMode, Windows::UI::Xaml::Media::Stretch::Fill); - GETSET_PROPERTY(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_SETTING(Microsoft::Terminal::TerminalControl::TextAntialiasingMode, AntialiasingMode, Microsoft::Terminal::TerminalControl::TextAntialiasingMode::Grayscale); + GETSET_SETTING(bool, RetroTerminalEffect, false); + GETSET_SETTING(bool, ForceFullRepaintRendering, false); + GETSET_SETTING(bool, SoftwareRendering, false); - GETSET_PROPERTY(hstring, ColorSchemeName, L"Campbell"); - GETSET_PROPERTY(Windows::Foundation::IReference, Foreground); - GETSET_PROPERTY(Windows::Foundation::IReference, Background); - GETSET_PROPERTY(Windows::Foundation::IReference, SelectionBackground); - GETSET_PROPERTY(Windows::Foundation::IReference, CursorColor); + GETSET_SETTING(hstring, ColorSchemeName, L"Campbell"); - GETSET_PROPERTY(int32_t, HistorySize, DEFAULT_HISTORY_SIZE); - GETSET_PROPERTY(bool, SnapOnInput, true); - GETSET_PROPERTY(bool, AltGrAliasing, true); + GETSET_NULLABLE_SETTING(Windows::UI::Color, Foreground, nullptr); + GETSET_NULLABLE_SETTING(Windows::UI::Color, Background, nullptr); + GETSET_NULLABLE_SETTING(Windows::UI::Color, SelectionBackground, nullptr); + GETSET_NULLABLE_SETTING(Windows::UI::Color, CursorColor, nullptr); - GETSET_PROPERTY(Microsoft::Terminal::TerminalControl::CursorStyle, CursorShape, Microsoft::Terminal::TerminalControl::CursorStyle::Bar); - GETSET_PROPERTY(uint32_t, CursorHeight, DEFAULT_CURSOR_HEIGHT); + GETSET_SETTING(int32_t, HistorySize, DEFAULT_HISTORY_SIZE); + GETSET_SETTING(bool, SnapOnInput, true); + GETSET_SETTING(bool, AltGrAliasing, true); + + GETSET_SETTING(Microsoft::Terminal::TerminalControl::CursorStyle, CursorShape, Microsoft::Terminal::TerminalControl::CursorStyle::Bar); + GETSET_SETTING(uint32_t, CursorHeight, DEFAULT_CURSOR_HEIGHT); private: std::optional _Guid{ std::nullopt }; diff --git a/src/inc/til/coalesce.h b/src/inc/til/coalesce.h index 24153869a67..aacbbf52751 100644 --- a/src/inc/til/coalesce.h +++ b/src/inc/til/coalesce.h @@ -3,8 +3,20 @@ #pragma once +namespace winrt +{ + // If we don't use winrt, nobody will include the ConversionTraits for winrt stuff. + // If nobody includes it, these forward declarations will suffice. + namespace Windows::Foundation + { + template + struct IReference; + } +} + namespace til { +#pragma region coalesce_value // Method Description: // - Base case provided to handle the last argument to coalesce_value() template @@ -22,6 +34,15 @@ namespace til return T{}; } + // Method Description: + // - Base case provided to throw an assertion if you call coalesce_value(opt, opt, opt) + template + T coalesce_value(const winrt::Windows::Foundation::IReference& base) + { + static_assert(false, "coalesce_value must be passed a base non-optional value to be used if all optionals are empty"); + return T{}; + } + // Method Description: // - Returns the value from the first populated optional, or a base value if none were populated. template @@ -34,6 +55,16 @@ namespace til return t1.value_or(coalesce_value(std::forward(t2)...)); } + // Method Description: + // - Returns the value from the first populated IReference, or a base value if none were populated. + template + T coalesce_value(const winrt::Windows::Foundation::IReference& t1, Ts&&... t2) + { + return t1 ? t1.Value() : coalesce_value(std::forward(t2)...); + } +#pragma endregion + +#pragma region coalesce // Method Description: // - Base case provided to handle the last argument to coalesce_value() template @@ -42,6 +73,14 @@ namespace til return base; } + // Method Description: + // - Base case provided to handle the last argument to coalesce_value() + template + winrt::Windows::Foundation::IReference coalesce(const winrt::Windows::Foundation::IReference& base) + { + return base; + } + // Method Description: // - Base case provided to handle the last argument to coalesce_value(..., nullopt) template @@ -58,4 +97,12 @@ namespace til return t1.has_value() ? t1 : coalesce(std::forward(t2)...); } + // Method Description: + // - Returns the value from the first populated IReference, or the last one (if none of the previous had a value) + template + winrt::Windows::Foundation::IReference coalesce(const winrt::Windows::Foundation::IReference& t1, Ts&&... t2) + { + return t1 ? t1 : coalesce(std::forward(t2)...); + } +#pragma endregion } From 570afa27e7057bc262ef34cdc08f68406495b98e Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Thu, 15 Oct 2020 13:44:58 -0700 Subject: [PATCH 02/29] Update IDL. Fix Generate Stub. More inheritance --- .../actions/spell-check/dictionary/apis.txt | 1 + .../GlobalAppSettings.cpp | 47 ++++++++- .../TerminalSettingsModel/GlobalAppSettings.h | 10 +- .../GlobalAppSettings.idl | 75 ++++++++++++++- .../TerminalSettingsModel/Profile.cpp | 52 ++++++---- src/cascadia/TerminalSettingsModel/Profile.h | 14 ++- .../TerminalSettingsModel/Profile.idl | 96 +++++++++++++++++++ 7 files changed, 265 insertions(+), 30 deletions(-) diff --git a/.github/actions/spell-check/dictionary/apis.txt b/.github/actions/spell-check/dictionary/apis.txt index 2ce6c20c31f..976b33bba73 100644 --- a/.github/actions/spell-check/dictionary/apis.txt +++ b/.github/actions/spell-check/dictionary/apis.txt @@ -21,6 +21,7 @@ ICustom IDialog IDirect IExplorer +IInheritable IMap IObject IStorage diff --git a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp index b3eb1ad0e8d..2c9cba33a34 100644 --- a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp +++ b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp @@ -55,7 +55,7 @@ static constexpr bool debugFeaturesDefault{ false }; GlobalAppSettings::GlobalAppSettings() : _keymap{ winrt::make_self() }, _keybindingsWarnings{}, - _unparsedDefaultProfile{}, + _validDefaultProfile{ false }, _defaultProfile{}, _DebugFeaturesEnabled{ debugFeaturesDefault } { @@ -68,23 +68,57 @@ winrt::Windows::Foundation::Collections::IMapView GlobalAppSettings::_getUnparsedDefaultProfileImpl() const +{ + return _UnparsedDefaultProfile ? + _UnparsedDefaultProfile : + (_parent ? + _parent->_getUnparsedDefaultProfileImpl() : + std::nullopt); } +#pragma endregion winrt::Microsoft::Terminal::Settings::Model::KeyMapping GlobalAppSettings::KeyMap() const noexcept { @@ -106,7 +140,10 @@ winrt::com_ptr GlobalAppSettings::FromJson(const Json::Value& void GlobalAppSettings::LayerJson(const Json::Value& json) { - JsonUtils::GetValueForKey(json, DefaultProfileKey, _unparsedDefaultProfile); + // _validDefaultProfile keeps track of whether we've verified that DefaultProfile points to something + // CascadiaSettings::_ResolveDefaultProfile performs a validation and updates DefaultProfile() with the + // resolved value, then making it valid. + _validDefaultProfile = !JsonUtils::GetValueForKey(json, DefaultProfileKey, _UnparsedDefaultProfile); JsonUtils::GetValueForKey(json, AlwaysShowTabsKey, _AlwaysShowTabs); diff --git a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.h b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.h index 47228bcd25c..ed371525d00 100644 --- a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.h +++ b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.h @@ -52,7 +52,10 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // by higher layers in the app. void DefaultProfile(const guid& defaultProfile) noexcept; guid DefaultProfile() const; - hstring UnparsedDefaultProfile() const; + bool HasUnparsedDefaultProfile() const; + winrt::hstring UnparsedDefaultProfile() const; + void UnparsedDefaultProfile(const hstring& value); + void ClearUnparsedDefaultProfile(); GETSET_SETTING(int32_t, InitialRows, DEFAULT_ROWS); GETSET_SETTING(int32_t, InitialCols, DEFAULT_COLS); @@ -80,8 +83,9 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation GETSET_SETTING(bool, DisableAnimations, false); private: - hstring _unparsedDefaultProfile; guid _defaultProfile; + std::optional _UnparsedDefaultProfile{ std::nullopt }; + bool _validDefaultProfile; com_ptr _keymap; std::vector _keybindingsWarnings; @@ -89,6 +93,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation Windows::Foundation::Collections::IMap _colorSchemes; Windows::Foundation::Collections::IMap _commands; + std::optional _getUnparsedDefaultProfileImpl() const; + friend class SettingsModelLocalTests::DeserializationTests; friend class SettingsModelLocalTests::ColorSchemeTests; }; diff --git a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.idl b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.idl index bf2a8ce43a0..af7b515eb55 100644 --- a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.idl +++ b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.idl @@ -26,31 +26,104 @@ namespace Microsoft.Terminal.Settings.Model [default_interface] runtimeclass GlobalAppSettings { Guid DefaultProfile; - String UnparsedDefaultProfile(); + Boolean HasUnparsedDefaultProfile(); + void ClearUnparsedDefaultProfile(); + String UnparsedDefaultProfile; + Boolean HasInitialRows(); + void ClearInitialRows(); Int32 InitialRows; + + Boolean HasInitialCols(); + void ClearInitialCols(); Int32 InitialCols; + + Boolean HasAlwaysShowTabs(); + void ClearAlwaysShowTabs(); Boolean AlwaysShowTabs; + + Boolean HasShowTitleInTitlebar(); + void ClearShowTitleInTitlebar(); Boolean ShowTitleInTitlebar; + + Boolean HasConfirmCloseAllTabs(); + void ClearConfirmCloseAllTabs(); Boolean ConfirmCloseAllTabs; + + Boolean HasTheme(); + void ClearTheme(); Windows.UI.Xaml.ElementTheme Theme; + + Boolean HasTabWidthMode(); + void ClearTabWidthMode(); Microsoft.UI.Xaml.Controls.TabViewWidthMode TabWidthMode; + + Boolean HasShowTabsInTitlebar(); + void ClearShowTabsInTitlebar(); Boolean ShowTabsInTitlebar; + + Boolean HasWordDelimiters(); + void ClearWordDelimiters(); String WordDelimiters; + + Boolean HasCopyOnSelect(); + void ClearCopyOnSelect(); Boolean CopyOnSelect; + + Boolean HasCopyFormatting(); + void ClearCopyFormatting(); Microsoft.Terminal.TerminalControl.CopyFormat CopyFormatting; + + Boolean HasWarnAboutLargePaste(); + void ClearWarnAboutLargePaste(); Boolean WarnAboutLargePaste; + + Boolean HasWarnAboutMultiLinePaste(); + void ClearWarnAboutMultiLinePaste(); Boolean WarnAboutMultiLinePaste; + + Boolean HasInitialPosition(); + void ClearInitialPosition(); LaunchPosition InitialPosition; + + Boolean HasLaunchMode(); + void ClearLaunchMode(); LaunchMode LaunchMode; + + Boolean HasSnapToGridOnResize(); + void ClearSnapToGridOnResize(); Boolean SnapToGridOnResize; + + Boolean HasForceFullRepaintRendering(); + void ClearForceFullRepaintRendering(); Boolean ForceFullRepaintRendering; + + Boolean HasSoftwareRendering(); + void ClearSoftwareRendering(); Boolean SoftwareRendering; + + Boolean HasForceVTInput(); + void ClearForceVTInput(); Boolean ForceVTInput; + + Boolean HasDebugFeaturesEnabled(); + void ClearDebugFeaturesEnabled(); Boolean DebugFeaturesEnabled; + + Boolean HasStartOnUserLogin(); + void ClearStartOnUserLogin(); Boolean StartOnUserLogin; + + Boolean HasAlwaysOnTop(); + void ClearAlwaysOnTop(); Boolean AlwaysOnTop; + + Boolean HasUseTabSwitcher(); + void ClearUseTabSwitcher(); Boolean UseTabSwitcher; + + Boolean HasDisableAnimations(); + void ClearDisableAnimations(); Boolean DisableAnimations; Windows.Foundation.Collections.IMapView ColorSchemes(); diff --git a/src/cascadia/TerminalSettingsModel/Profile.cpp b/src/cascadia/TerminalSettingsModel/Profile.cpp index 1d21f03a6e5..fe9c0e5f1f2 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.cpp +++ b/src/cascadia/TerminalSettingsModel/Profile.cpp @@ -88,20 +88,15 @@ Json::Value Profile::GenerateStub() const stub[JsonKey(GuidKey)] = winrt::to_string(Utils::GuidToString(*_Guid)); } - if (_Name) - { - stub[JsonKey(NameKey)] = winrt::to_string(*_Name); - } + stub[JsonKey(NameKey)] = winrt::to_string(Name()); - if (_Source && !_Source.value().empty()) + const auto source{ Source() }; + if (!source.empty()) { - stub[JsonKey(SourceKey)] = winrt::to_string(*_Source); + stub[JsonKey(SourceKey)] = winrt::to_string(source); } - if (_Hidden) - { - stub[JsonKey(HiddenKey)] = *_Hidden; - } + stub[JsonKey(HiddenKey)] = Hidden(); return stub; } @@ -157,12 +152,13 @@ bool Profile::ShouldBeLayered(const Json::Value& json) const // For profiles with a `source`, also check the `source` property. bool sourceMatches = false; - if (_Source && !_Source.value().empty()) + const auto mySource{ Source() }; + if (!mySource.empty()) { if (otherHadSource) { // If we have a source and the other has a source, compare them! - sourceMatches = *otherSource == *_Source; + sourceMatches = *otherSource == mySource; } else { @@ -170,9 +166,9 @@ bool Profile::ShouldBeLayered(const Json::Value& json) const // `this` is a dynamic profile with a source, and our _source is one // of the legacy DPG namespaces. We're looking to see if the other // json object has the same guid, but _no_ "source" - if (*_Source == WslGeneratorNamespace || - *_Source == AzureGeneratorNamespace || - *_Source == PowershellCoreGeneratorNamespace) + if (mySource == WslGeneratorNamespace || + mySource == AzureGeneratorNamespace || + mySource == PowershellCoreGeneratorNamespace) { sourceMatches = true; } @@ -421,25 +417,43 @@ winrt::guid Profile::GetGuidOrGenerateForJson(const Json::Value& json) noexcept return Profile::_GenerateGuidForProfile(name, source); } +#pragma region BackgroundImageAlignment +bool Profile::HasBackgroundImageAlignment() const noexcept +{ + return _BackgroundImageAlignment.has_value(); +} + +void Profile::ClearBackgroundImageAlignment() noexcept +{ + _BackgroundImageAlignment = std::nullopt; +} + const HorizontalAlignment Profile::BackgroundImageHorizontalAlignment() const noexcept { - return std::get(_BackgroundImageAlignment); + return std::get(*_getBackgroundImageAlignmentImpl()); } void Profile::BackgroundImageHorizontalAlignment(const HorizontalAlignment& value) noexcept { - std::get(_BackgroundImageAlignment) = value; + if (!HasBackgroundImageAlignment() || std::get(*_BackgroundImageAlignment) != value) + { + std::get(*_BackgroundImageAlignment) = value; + } } const VerticalAlignment Profile::BackgroundImageVerticalAlignment() const noexcept { - return std::get(_BackgroundImageAlignment); + return std::get(*_getBackgroundImageAlignmentImpl()); } void Profile::BackgroundImageVerticalAlignment(const VerticalAlignment& value) noexcept { - std::get(_BackgroundImageAlignment) = value; + if (!HasBackgroundImageAlignment() || std::get(*_BackgroundImageAlignment) != value) + { + std::get(*_BackgroundImageAlignment) = value; + } } +#pragma endregion bool Profile::HasGuid() const noexcept { diff --git a/src/cascadia/TerminalSettingsModel/Profile.h b/src/cascadia/TerminalSettingsModel/Profile.h index ce4b8315696..095cdc26308 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.h +++ b/src/cascadia/TerminalSettingsModel/Profile.h @@ -67,6 +67,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation void ConnectionType(const winrt::guid& conType) noexcept; // BackgroundImageAlignment is 1 setting saved as 2 separate values + bool HasBackgroundImageAlignment() const noexcept; + void ClearBackgroundImageAlignment() noexcept; const Windows::UI::Xaml::HorizontalAlignment BackgroundImageHorizontalAlignment() const noexcept; void BackgroundImageHorizontalAlignment(const Windows::UI::Xaml::HorizontalAlignment& value) noexcept; const Windows::UI::Xaml::VerticalAlignment BackgroundImageVerticalAlignment() const noexcept; @@ -121,9 +123,15 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation private: std::optional _Guid{ std::nullopt }; std::optional _ConnectionType{ std::nullopt }; - std::tuple _BackgroundImageAlignment{ - Windows::UI::Xaml::HorizontalAlignment::Center, - Windows::UI::Xaml::VerticalAlignment::Center + std::optional> _BackgroundImageAlignment{ std::nullopt }; + std::optional> _getBackgroundImageAlignmentImpl() const + { + return _BackgroundImageAlignment ? + _BackgroundImageAlignment : + (_parent ? + _parent->_getBackgroundImageAlignmentImpl() : + std::tuple{ Windows::UI::Xaml::HorizontalAlignment::Center, + Windows::UI::Xaml::VerticalAlignment::Center }); }; static std::wstring EvaluateStartingDirectory(const std::wstring& directory); diff --git a/src/cascadia/TerminalSettingsModel/Profile.idl b/src/cascadia/TerminalSettingsModel/Profile.idl index a81c95bfa59..73c62d4a76c 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.idl +++ b/src/cascadia/TerminalSettingsModel/Profile.idl @@ -14,57 +14,153 @@ namespace Microsoft.Terminal.Settings.Model Profile(); Profile(Guid guid); + Boolean HasName(); + void ClearName(); String Name; + Boolean HasGuid(); Guid Guid; + + Boolean HasSource(); + void ClearSource(); String Source; + Boolean HasConnectionType(); Guid ConnectionType; + + Boolean HasHidden(); + void ClearHidden(); Boolean Hidden; + Boolean HasIcon(); + void ClearIcon(); String Icon; + Boolean HasCloseOnExit(); + void ClearCloseOnExit(); CloseOnExitMode CloseOnExit; + + Boolean HasTabTitle(); + void ClearTabTitle(); String TabTitle; + + Boolean HasTabColor(); + void ClearTabColor(); Windows.Foundation.IReference TabColor; + + Boolean HasSuppressApplicationTitle(); + void ClearSuppressApplicationTitle(); Boolean SuppressApplicationTitle; + Boolean HasUseAcrylic(); + void ClearUseAcrylic(); Boolean UseAcrylic; + + Boolean HasAcrylicOpacity(); + void ClearAcrylicOpacity(); Double AcrylicOpacity; + + Boolean HasScrollState(); + void ClearScrollState(); Microsoft.Terminal.TerminalControl.ScrollbarState ScrollState; + Boolean HasFontFace(); + void ClearFontFace(); String FontFace; + + Boolean HasFontSize(); + void ClearFontSize(); Int32 FontSize; + + Boolean HasFontWeight(); + void ClearFontWeight(); Windows.UI.Text.FontWeight FontWeight; + + Boolean HasPadding(); + void ClearPadding(); String Padding; + Boolean HasCommandline(); + void ClearCommandline(); String Commandline; + + Boolean HasStartingDirectory(); + void ClearStartingDirectory(); String StartingDirectory; String EvaluatedStartingDirectory { get; }; + Boolean HasBackgroundImagePath(); + void ClearBackgroundImagePath(); String BackgroundImagePath; String ExpandedBackgroundImagePath { get; }; + + Boolean HasBackgroundImageOpacity(); + void ClearBackgroundImageOpacity(); Double BackgroundImageOpacity; + + Boolean HasBackgroundImageStretchMode(); + void ClearBackgroundImageStretchMode(); Windows.UI.Xaml.Media.Stretch BackgroundImageStretchMode; + + Boolean HasBackgroundImageAlignment(); + void ClearBackgroundImageAlignment(); Windows.UI.Xaml.HorizontalAlignment BackgroundImageHorizontalAlignment; Windows.UI.Xaml.VerticalAlignment BackgroundImageVerticalAlignment; + Boolean HasAntialiasingMode(); + void ClearAntialiasingMode(); Microsoft.Terminal.TerminalControl.TextAntialiasingMode AntialiasingMode; + + Boolean HasRetroTerminalEffect(); + void ClearRetroTerminalEffect(); Boolean RetroTerminalEffect; + + Boolean HasForceFullRepaintRendering(); + void ClearForceFullRepaintRendering(); Boolean ForceFullRepaintRendering; + + Boolean HasSoftwareRendering(); + void ClearSoftwareRendering(); Boolean SoftwareRendering; + Boolean HasColorSchemeName(); + void ClearColorSchemeName(); String ColorSchemeName; + + Boolean HasForeground(); + void ClearForeground(); Windows.Foundation.IReference Foreground; + + Boolean HasBackground(); + void ClearBackground(); Windows.Foundation.IReference Background; + + Boolean HasSelectionBackground(); + void ClearSelectionBackground(); Windows.Foundation.IReference SelectionBackground; + + Boolean HasCursorColor(); + void ClearCursorColor(); Windows.Foundation.IReference CursorColor; + Boolean HasHistorySize(); + void ClearHistorySize(); Int32 HistorySize; + + Boolean HasSnapOnInput(); + void ClearSnapOnInput(); Boolean SnapOnInput; + + Boolean HasAltGrAliasing(); + void ClearAltGrAliasing(); Boolean AltGrAliasing; + Boolean HasCursorShape(); + void ClearCursorShape(); Microsoft.Terminal.TerminalControl.CursorStyle CursorShape; + + Boolean HasCursorHeight(); + void ClearCursorHeight(); UInt32 CursorHeight; } } From 374ad8074598a1aced8e6496f0382153ba0f10d2 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Thu, 15 Oct 2020 15:08:33 -0700 Subject: [PATCH 03/29] address PR Comments; inherit schemes/actions --- .../CascadiaSettingsSerialization.cpp | 7 ++- .../GlobalAppSettings.cpp | 24 +++++++++ .../TerminalSettingsModel/GlobalAppSettings.h | 1 + .../TerminalSettingsModel/IInheritable.h | 49 ++++++++++--------- 4 files changed, 57 insertions(+), 24 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp index 9de7616eb8a..0c50cdf1f50 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp @@ -591,7 +591,7 @@ winrt::com_ptr CascadiaSettings::FromJson(const Json::Value& j void CascadiaSettings::LayerJson(const Json::Value& json) { // add a new inheritance layer, and apply json values to child - _globals.attach(_globals->CreateChild()); + _globals = _globals->CreateChild(); _globals->LayerJson(json); if (auto schemes{ json[SchemesKey.data()] }) @@ -637,6 +637,9 @@ void CascadiaSettings::_LayerOrCreateProfile(const Json::Value& profileJson) auto parent{ winrt::get_self(parentProj) }; auto childImpl{ parent->CreateChild() }; childImpl->LayerJson(profileJson); + + // replace parent in _profiles with child + _profiles.SetAt(*profileIndex, *childImpl); } else { @@ -651,7 +654,7 @@ void CascadiaSettings::_LayerOrCreateProfile(const Json::Value& profileJson) // We _won't_ have these settings yet for defaults, dynamic profiles. if (_userDefaultProfileSettings) { - profile.attach(_userDefaultProfileSettings->CreateChild()); + profile = _userDefaultProfileSettings->CreateChild(); } profile->LayerJson(profileJson); diff --git a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp index 2c9cba33a34..6f933e5aa2a 100644 --- a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp +++ b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp @@ -63,6 +63,30 @@ GlobalAppSettings::GlobalAppSettings() : _colorSchemes = winrt::single_threaded_map(); } +// Method Description: +// - Copies any extraneous data from the parent before completing a CreateChild call +// Arguments: +// - +// Return Value: +// - +void GlobalAppSettings::_FinalizeInheritance() +{ + // TODO CARLOS: these should be "Copy", but we're just directly linking them while 7877 merges + _keymap = _parent->_keymap; + std::for_each(_parent->_keybindingsWarnings.begin(), _parent->_keybindingsWarnings.end(), [this](auto& warning) { _keybindingsWarnings.push_back(warning); }); + for (auto kv : _parent->_colorSchemes) + { + const auto schemeImpl{ winrt::get_self(kv.Value()) }; + _colorSchemes.Insert(kv.Key(), *schemeImpl); + } + + for (auto kv : _parent->_commands) + { + const auto commandImpl{ winrt::get_self(kv.Value()) }; + _commands.Insert(kv.Key(), *commandImpl); + } +} + winrt::Windows::Foundation::Collections::IMapView GlobalAppSettings::ColorSchemes() noexcept { return _colorSchemes.GetView(); diff --git a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.h b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.h index ed371525d00..2a98163cfbf 100644 --- a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.h +++ b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.h @@ -35,6 +35,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation { public: GlobalAppSettings(); + void _FinalizeInheritance() override; Windows::Foundation::Collections::IMapView ColorSchemes() noexcept; void AddColorScheme(const Model::ColorScheme& scheme); diff --git a/src/cascadia/TerminalSettingsModel/IInheritable.h b/src/cascadia/TerminalSettingsModel/IInheritable.h index 4a63213fa16..e7f36e826da 100644 --- a/src/cascadia/TerminalSettingsModel/IInheritable.h +++ b/src/cascadia/TerminalSettingsModel/IInheritable.h @@ -20,18 +20,34 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation struct IInheritable { public: - T* CreateChild() const + // Method Description: + // - Create a new instance of T, but set its parent to this instance + // Arguments: + // - + // Return Value: + // - a new instance of T with this instance set as its parent + com_ptr CreateChild() const { auto child{ winrt::make_self() }; - child->_parent.attach(const_cast(static_cast(this))); - return child.detach(); + winrt::copy_from_abi(child->_parent, const_cast(static_cast(this))); + child->_FinalizeInheritance(); + return child; } protected: com_ptr _parent{ nullptr }; + + // Method Description: + // - Actions to be performed after a child was created. Generally used to set + // any extraneous data from the parent into the child. + // Arguments: + // - + // Return Value: + // - + virtual void _FinalizeInheritance() {} }; - // Nullable settings are settings that explicitly allow null to mean something + // This is like std::optional, but we can use it in inheritance to determine whether the user explicitly cleared it template struct NullableSetting { @@ -65,23 +81,15 @@ public: \ }; \ \ /* Overwrite the user set value */ \ - /* Dispatch event if value changed */ \ void name(const type& value) \ { \ - if (_##name != value) \ - { \ - _##name = value; \ - } \ + _##name = value; \ }; \ \ /* Clear the user set value */ \ - /* Dispatch event if value changed */ \ void Clear##name() \ { \ - if (Has##name()) \ - { \ - _##name = std::nullopt; \ - } \ + _##name = std::nullopt; \ }; \ \ private: \ @@ -92,7 +100,7 @@ private: \ _##name : \ (_parent ? \ _parent->_get##name##Impl() : \ - type{ __VA_ARGS__ }); \ + std::nullopt); \ }; // This macro is similar to the one above, but is reserved for optional settings @@ -130,10 +138,7 @@ public: /* Dispatch event if value changed */ \ void Clear##name() \ { \ - if (Has##name()) \ - { \ - _##name.set = false; \ - } \ + _##name.set = false; \ }; \ \ private: \ @@ -149,8 +154,8 @@ private: // Use this macro for Profile::ApplyTo // Checks if a given setting is set, and if it is, sets it on profile -#define APPLY_OUT(setting) \ - if (Has##setting()) \ - { \ +#define APPLY_OUT(setting) \ + if (Has##setting()) \ + { \ profile->_##setting = _##setting; \ } From ae97064bc754ec027ce99e4240cf285c123392f2 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Thu, 15 Oct 2020 17:10:19 -0700 Subject: [PATCH 04/29] fix BI Alignment bug --- src/cascadia/TerminalSettingsModel/Profile.cpp | 7 +++++-- src/cascadia/TerminalSettingsModel/Profile.h | 3 +-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/Profile.cpp b/src/cascadia/TerminalSettingsModel/Profile.cpp index fe9c0e5f1f2..1cc517d49f1 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.cpp +++ b/src/cascadia/TerminalSettingsModel/Profile.cpp @@ -360,6 +360,7 @@ void Profile::ApplyTo(Profile* profile) const APPLY_OUT(BackgroundImagePath); APPLY_OUT(BackgroundImageOpacity); APPLY_OUT(BackgroundImageStretchMode); + APPLY_OUT(BackgroundImageAlignment); APPLY_OUT(AntialiasingMode); APPLY_OUT(RetroTerminalEffect); APPLY_OUT(ForceFullRepaintRendering); @@ -430,7 +431,8 @@ void Profile::ClearBackgroundImageAlignment() noexcept const HorizontalAlignment Profile::BackgroundImageHorizontalAlignment() const noexcept { - return std::get(*_getBackgroundImageAlignmentImpl()); + const auto val{ _getBackgroundImageAlignmentImpl() }; + return val ? std::get(*val) : HorizontalAlignment::Center; } void Profile::BackgroundImageHorizontalAlignment(const HorizontalAlignment& value) noexcept @@ -443,7 +445,8 @@ void Profile::BackgroundImageHorizontalAlignment(const HorizontalAlignment& valu const VerticalAlignment Profile::BackgroundImageVerticalAlignment() const noexcept { - return std::get(*_getBackgroundImageAlignmentImpl()); + const auto val{ _getBackgroundImageAlignmentImpl() }; + return val ? std::get(*val) : VerticalAlignment::Center; } void Profile::BackgroundImageVerticalAlignment(const VerticalAlignment& value) noexcept diff --git a/src/cascadia/TerminalSettingsModel/Profile.h b/src/cascadia/TerminalSettingsModel/Profile.h index 095cdc26308..11e8a37cd08 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.h +++ b/src/cascadia/TerminalSettingsModel/Profile.h @@ -130,8 +130,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation _BackgroundImageAlignment : (_parent ? _parent->_getBackgroundImageAlignmentImpl() : - std::tuple{ Windows::UI::Xaml::HorizontalAlignment::Center, - Windows::UI::Xaml::VerticalAlignment::Center }); + std::nullopt); }; static std::wstring EvaluateStartingDirectory(const std::wstring& directory); From d1ddab5e513ffcfe8359c6da91b44b9e6a335666 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Fri, 16 Oct 2020 16:36:16 -0700 Subject: [PATCH 05/29] fix StartingDirectory --- src/cascadia/TerminalApp/TerminalSettings.cpp | 5 +- .../AzureCloudShellGenerator.cpp | 3 +- .../TerminalSettingsModel/IInheritable.h | 8 +++ .../PowershellCoreProfileGenerator.cpp | 3 +- .../TerminalSettingsModel/Profile.cpp | 54 ++++++++++++++++++- src/cascadia/TerminalSettingsModel/Profile.h | 18 ++++++- .../TerminalSettingsModel/Profile.idl | 2 +- .../WslDistroGenerator.cpp | 3 +- 8 files changed, 85 insertions(+), 11 deletions(-) diff --git a/src/cascadia/TerminalApp/TerminalSettings.cpp b/src/cascadia/TerminalApp/TerminalSettings.cpp index cee6b82d041..662079d7dcb 100644 --- a/src/cascadia/TerminalApp/TerminalSettings.cpp +++ b/src/cascadia/TerminalApp/TerminalSettings.cpp @@ -94,10 +94,7 @@ namespace winrt::TerminalApp::implementation _Commandline = profile.Commandline(); - if (!profile.StartingDirectory().empty()) - { - _StartingDirectory = profile.EvaluatedStartingDirectory(); - } + _StartingDirectory = profile.EvaluatedStartingDirectory(); // GH#2373: Use the tabTitle as the starting title if it exists, otherwise // use the profile name diff --git a/src/cascadia/TerminalSettingsModel/AzureCloudShellGenerator.cpp b/src/cascadia/TerminalSettingsModel/AzureCloudShellGenerator.cpp index 3435d795591..23a8e7520c0 100644 --- a/src/cascadia/TerminalSettingsModel/AzureCloudShellGenerator.cpp +++ b/src/cascadia/TerminalSettingsModel/AzureCloudShellGenerator.cpp @@ -35,7 +35,8 @@ std::vector AzureCloudShellGenerator::GenerateProfiles() { auto azureCloudShellProfile{ CreateDefaultProfile(L"Azure Cloud Shell") }; azureCloudShellProfile.Commandline(L"Azure"); - azureCloudShellProfile.StartingDirectory(DEFAULT_STARTING_DIRECTORY); + const auto startDir{ winrt::Windows::Foundation::PropertyValue::CreateString(DEFAULT_STARTING_DIRECTORY) }; + azureCloudShellProfile.StartingDirectory(startDir); azureCloudShellProfile.ColorSchemeName(L"Vintage"); azureCloudShellProfile.ConnectionType(AzureConnection::ConnectionType()); profiles.emplace_back(azureCloudShellProfile); diff --git a/src/cascadia/TerminalSettingsModel/IInheritable.h b/src/cascadia/TerminalSettingsModel/IInheritable.h index e7f36e826da..90ec752f936 100644 --- a/src/cascadia/TerminalSettingsModel/IInheritable.h +++ b/src/cascadia/TerminalSettingsModel/IInheritable.h @@ -54,6 +54,14 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation winrt::Windows::Foundation::IReference setting{ nullptr }; bool set{ false }; }; + + // hstrings cannot be exposed in IDLs as nullable + // We can get around that by treating it as an IInspectable + struct NullableString + { + winrt::Windows::Foundation::IInspectable setting{ nullptr }; + bool set{ false }; + }; } // Use this macro to quickly implement both getters and the setter for an diff --git a/src/cascadia/TerminalSettingsModel/PowershellCoreProfileGenerator.cpp b/src/cascadia/TerminalSettingsModel/PowershellCoreProfileGenerator.cpp index f882a929056..71650c29f3d 100644 --- a/src/cascadia/TerminalSettingsModel/PowershellCoreProfileGenerator.cpp +++ b/src/cascadia/TerminalSettingsModel/PowershellCoreProfileGenerator.cpp @@ -310,7 +310,8 @@ std::vector PowershellCoreProfileGenerator::GenerateProfiles() const auto name = psI.Name(); auto profile{ CreateDefaultProfile(name) }; profile.Commandline(psI.executablePath.wstring()); - profile.StartingDirectory(DEFAULT_STARTING_DIRECTORY); + const auto startDir{ winrt::Windows::Foundation::PropertyValue::CreateString(DEFAULT_STARTING_DIRECTORY) }; + profile.StartingDirectory(startDir); profile.ColorSchemeName(L"Campbell"); profile.Icon(WI_IsFlagSet(psI.flags, PowerShellFlags::Preview) ? POWERSHELL_PREVIEW_ICON : POWERSHELL_ICON); diff --git a/src/cascadia/TerminalSettingsModel/Profile.cpp b/src/cascadia/TerminalSettingsModel/Profile.cpp index 34f45afc61c..3314ab4b9cc 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.cpp +++ b/src/cascadia/TerminalSettingsModel/Profile.cpp @@ -280,7 +280,13 @@ void Profile::LayerJson(const Json::Value& json) JsonUtils::GetValueForKey(json, PaddingKey, _Padding, JsonUtils::PermissiveStringConverter{}); JsonUtils::GetValueForKey(json, ScrollbarStateKey, _ScrollState); - JsonUtils::GetValueForKey(json, StartingDirectoryKey, _StartingDirectory); + + // StartingDirectory is nullable. But WinRT does not allow IReference to be exposed in the IDL. + // Exposing it as an IInspectable is a workaround for this issue. + std::optional startDir{ std::nullopt }; + _StartingDirectory.set = JsonUtils::GetValueForKey(json, StartingDirectoryKey, startDir); + _StartingDirectory.setting = startDir ? PropertyValue::CreateString(*startDir) : nullptr; + JsonUtils::GetValueForKey(json, IconKey, _Icon); JsonUtils::GetValueForKey(json, BackgroundImageKey, _BackgroundImagePath); JsonUtils::GetValueForKey(json, BackgroundImageOpacityKey, _BackgroundImageOpacity); @@ -333,7 +339,17 @@ winrt::hstring Profile::ExpandedBackgroundImagePath() const winrt::hstring Profile::EvaluatedStartingDirectory() const { const auto path{ StartingDirectory() }; - return winrt::hstring{ Profile::EvaluateStartingDirectory(path.c_str()) }; + const auto pathString{ path.as>() }; + if (pathString == nullptr) + { + // Translate explicit null to empty string + // This is interpreted on the other end as "current directory for process" + return L""; + } + else + { + return winrt::hstring{ Profile::EvaluateStartingDirectory(pathString.Value().c_str()) }; + } } // Method Description: @@ -560,3 +576,37 @@ void Profile::ConnectionType(const winrt::guid& conType) noexcept { _ConnectionType = conType; } + +#pragma region StartingDirectory +bool Profile::HasStartingDirectory() const +{ + return _StartingDirectory.set; +}; + +winrt::Windows::Foundation::IInspectable Profile::StartingDirectory() const +{ + return _getStartingDirectoryImpl(); +}; + +void Profile::StartingDirectory(const winrt::Windows::Foundation::IInspectable& value) +{ + // Set the value if... + // - the value is not set already + // - both values are not null + // - both values are not the same string + auto myVal{ _StartingDirectory.setting.as>() }; + auto otherVal{ value.as>() }; + if (!HasStartingDirectory() || + !myVal == !otherVal || + myVal.Value() == otherVal.Value()) + { + _StartingDirectory.setting = value; + _StartingDirectory.set = true; + } +} + +void Profile::ClearStartingDirectory() +{ + _StartingDirectory.set = false; +}; +#pragma endregion diff --git a/src/cascadia/TerminalSettingsModel/Profile.h b/src/cascadia/TerminalSettingsModel/Profile.h index 2169027d3ce..a59a271da76 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.h +++ b/src/cascadia/TerminalSettingsModel/Profile.h @@ -75,6 +75,13 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation const Windows::UI::Xaml::VerticalAlignment BackgroundImageVerticalAlignment() const noexcept; void BackgroundImageVerticalAlignment(const Windows::UI::Xaml::VerticalAlignment& value) noexcept; + // StartingDirectory is a nullable hstring. The IDL does not allow + // hstring to be nullable. So we must expose it as an IInspectable + bool HasStartingDirectory() const; + winrt::Windows::Foundation::IInspectable StartingDirectory() const; + void StartingDirectory(const winrt::Windows::Foundation::IInspectable& value); + void ClearStartingDirectory(); + GETSET_SETTING(hstring, Name, L"Default"); GETSET_SETTING(hstring, Source); GETSET_SETTING(bool, Hidden, false); @@ -96,7 +103,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation GETSET_SETTING(hstring, Padding, DEFAULT_PADDING); GETSET_SETTING(hstring, Commandline, L"cmd.exe"); - GETSET_SETTING(hstring, StartingDirectory); GETSET_SETTING(hstring, BackgroundImagePath); GETSET_SETTING(double, BackgroundImageOpacity, 1.0); @@ -136,6 +142,16 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation std::nullopt); }; + NullableString _StartingDirectory{}; + winrt::Windows::Foundation::IInspectable _getStartingDirectoryImpl() const + { + return HasStartingDirectory() ? + _StartingDirectory.setting : + (_parent ? + _parent->_getStartingDirectoryImpl() : + nullptr); + }; + static std::wstring EvaluateStartingDirectory(const std::wstring& directory); static guid _GenerateGuidForProfile(const hstring& name, const hstring& source) noexcept; diff --git a/src/cascadia/TerminalSettingsModel/Profile.idl b/src/cascadia/TerminalSettingsModel/Profile.idl index ce228214e85..5ff5c9dd217 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.idl +++ b/src/cascadia/TerminalSettingsModel/Profile.idl @@ -92,7 +92,7 @@ namespace Microsoft.Terminal.Settings.Model Boolean HasStartingDirectory(); void ClearStartingDirectory(); - String StartingDirectory; + IInspectable StartingDirectory; String EvaluatedStartingDirectory { get; }; Boolean HasBackgroundImagePath(); diff --git a/src/cascadia/TerminalSettingsModel/WslDistroGenerator.cpp b/src/cascadia/TerminalSettingsModel/WslDistroGenerator.cpp index 3cd4b7b0d95..e7d9727b4d6 100644 --- a/src/cascadia/TerminalSettingsModel/WslDistroGenerator.cpp +++ b/src/cascadia/TerminalSettingsModel/WslDistroGenerator.cpp @@ -128,7 +128,8 @@ std::vector WslDistroGenerator::GenerateProfiles() WSLDistro.Commandline(L"wsl.exe -d " + distName); WSLDistro.ColorSchemeName(L"Campbell"); - WSLDistro.StartingDirectory(DEFAULT_STARTING_DIRECTORY); + const auto startDir{ winrt::Windows::Foundation::PropertyValue::CreateString(DEFAULT_STARTING_DIRECTORY) }; + WSLDistro.StartingDirectory(startDir); WSLDistro.Icon(L"ms-appx:///ProfileIcons/{9acb9455-ca41-5af7-950f-6bca1bc9722f}.png"); profiles.emplace_back(WSLDistro); } From 2866b4e559736229b963aed6d3c47447e6c12e15 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Mon, 19 Oct 2020 20:10:34 -0700 Subject: [PATCH 06/29] Introduce Multi-Parent Inheritance (LOGIC ERROR) --- .../CascadiaSettingsSerialization.cpp | 21 ++- .../GlobalAppSettings.cpp | 56 +++++-- .../TerminalSettingsModel/IInheritable.h | 146 ++++++++++++------ .../TerminalSettingsModel/Profile.cpp | 10 +- src/cascadia/TerminalSettingsModel/Profile.h | 46 ++++-- 5 files changed, 199 insertions(+), 80 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp index 0c50cdf1f50..85342c3bf9b 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp @@ -638,6 +638,12 @@ void CascadiaSettings::_LayerOrCreateProfile(const Json::Value& profileJson) auto childImpl{ parent->CreateChild() }; childImpl->LayerJson(profileJson); + if (_userDefaultProfileSettings) + { + // Add profile.defaults as the _first_ parent to the child + childImpl->InsertParent(0, _userDefaultProfileSettings); + } + // replace parent in _profiles with child _profiles.SetAt(*profileIndex, *childImpl); } @@ -745,10 +751,19 @@ void CascadiaSettings::_ApplyDefaultsFromUserSettings() _userDefaultProfileSettings = winrt::make_self(); _userDefaultProfileSettings->LayerJson(defaultSettings); - for (auto profile : _profiles) + const auto numOfProfiles{ _profiles.Size() }; + for (uint32_t profileIndex = 0; profileIndex < numOfProfiles; ++profileIndex) { - auto profileImpl = winrt::get_self(profile); - _userDefaultProfileSettings->ApplyTo(profileImpl); + // create a child, so we inherit from the defaults.json layer + auto parentProj{ _profiles.GetAt(profileIndex) }; + auto parentImpl{ winrt::get_self(parentProj) }; + auto childImpl{ parentImpl->CreateChild() }; + + // Add profile.defaults as the _first_ parent to the child + childImpl->InsertParent(0, _userDefaultProfileSettings); + + // replace parent in _profiles with child + _profiles.SetAt(profileIndex, *childImpl); } } } diff --git a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp index 12738e2040c..befb87c2db0 100644 --- a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp +++ b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp @@ -73,18 +73,24 @@ void GlobalAppSettings::_FinalizeInheritance() { // TODO CARLOS: Crash invoking IActionAndArgs->Copy //_keymap = _parent->_keymap->Copy(); - _keymap = _parent->_keymap; - std::copy(_parent->_keybindingsWarnings.begin(), _parent->_keybindingsWarnings.end(), std::back_inserter(_keybindingsWarnings)); - for (auto kv : _parent->_colorSchemes) - { - const auto schemeImpl{ winrt::get_self(kv.Value()) }; - _colorSchemes.Insert(kv.Key(), *schemeImpl->Copy()); - } - for (auto kv : _parent->_commands) + // Globals only ever has 1 parent + FAIL_FAST_IF(_parents.size() > 1); + for (auto parent : _parents) { - const auto commandImpl{ winrt::get_self(kv.Value()) }; - _commands.Insert(kv.Key(), *commandImpl->Copy()); + _keymap = parent->_keymap; + std::copy(parent->_keybindingsWarnings.begin(), parent->_keybindingsWarnings.end(), std::back_inserter(_keybindingsWarnings)); + for (auto kv : parent->_colorSchemes) + { + const auto schemeImpl{ winrt::get_self(kv.Value()) }; + _colorSchemes.Insert(kv.Key(), *schemeImpl->Copy()); + } + + for (auto kv : parent->_commands) + { + const auto commandImpl{ winrt::get_self(kv.Value()) }; + _commands.Insert(kv.Key(), *commandImpl->Copy()); + } } } @@ -134,6 +140,13 @@ winrt::com_ptr GlobalAppSettings::Copy() const const auto commandImpl{ winrt::get_self(kv.Value()) }; globals->_commands.Insert(kv.Key(), *commandImpl->Copy()); } + + // Globals only ever has 1 parent + FAIL_FAST_IF(_parents.size() > 1); + for (auto parent : _parents) + { + globals->InsertParent(parent->Copy()); + } return globals; } @@ -186,11 +199,24 @@ void GlobalAppSettings::ClearUnparsedDefaultProfile() std::optional GlobalAppSettings::_getUnparsedDefaultProfileImpl() const { - return _UnparsedDefaultProfile ? - _UnparsedDefaultProfile : - (_parent ? - _parent->_getUnparsedDefaultProfileImpl() : - std::nullopt); + /*return user set value*/ + if (_UnparsedDefaultProfile) + { + return _UnparsedDefaultProfile; + } + + /*user set value was not set*/ + /*iterate through parents to find a value*/ + for (auto parent : _parents) + { + if (auto val{ parent->_getUnparsedDefaultProfileImpl() }) + { + return val; + } + } + + /*no value was found*/ + return std::nullopt; } #pragma endregion diff --git a/src/cascadia/TerminalSettingsModel/IInheritable.h b/src/cascadia/TerminalSettingsModel/IInheritable.h index 90ec752f936..dca0bf4bb57 100644 --- a/src/cascadia/TerminalSettingsModel/IInheritable.h +++ b/src/cascadia/TerminalSettingsModel/IInheritable.h @@ -29,13 +29,32 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation com_ptr CreateChild() const { auto child{ winrt::make_self() }; - winrt::copy_from_abi(child->_parent, const_cast(static_cast(this))); + + com_ptr parent; + winrt::copy_from_abi(parent, const_cast(static_cast(this))); + child->InsertParent(parent); + child->_FinalizeInheritance(); return child; } + void InsertParent(com_ptr parent) + { + _parents.push_back(parent); + } + + void InsertParent(size_t index, com_ptr parent) + { + auto pos{ _parents.begin() }; + for (size_t i=0; i _parent{ nullptr }; + std::vector> _parents{}; // Method Description: // - Actions to be performed after a child was created. Generally used to set @@ -104,60 +123,87 @@ private: \ std::optional _##name{ std::nullopt }; \ std::optional _get##name##Impl() const \ { \ - return _##name ? \ - _##name : \ - (_parent ? \ - _parent->_get##name##Impl() : \ - std::nullopt); \ + /*return user set value*/ \ + if (_##name) \ + { \ + return _##name; \ + } \ + \ + /*user set value was not set*/ \ + /*iterate through parents to find a value*/ \ + for (auto parent : _parents) \ + { \ + if (auto val{ parent->_get##name##Impl() }) \ + { \ + return val; \ + } \ + } \ + \ + /*no value was found*/ \ + return std::nullopt; \ }; // This macro is similar to the one above, but is reserved for optional settings // like Profile.StartingDirectory and Profile.Foreground (where null is interpreted // as an acceptable value, rather than "inherit") // "type" is exposed as an IReference -#define GETSET_NULLABLE_SETTING(type, name, ...) \ -public: \ - /* Returns true if the user explicitly set the value, false otherwise*/ \ - bool Has##name() const \ - { \ - return _##name.set; \ - }; \ - \ - /* Returns the resolved value for this setting */ \ - /* fallback: user set value --> inherited value --> system set value */ \ - winrt::Windows::Foundation::IReference name() const \ - { \ - return _get##name##Impl(); \ - }; \ - \ - /* Overwrite the user set value */ \ - /* Dispatch event if value changed */ \ - void name(const winrt::Windows::Foundation::IReference& value) \ - { \ - if (!Has##name() /*value was not set*/ \ - || _##name.setting != value) /*set value is different*/ \ - { \ - _##name.setting = value; \ - _##name.set = true; \ - } \ - }; \ - \ - /* Clear the user set value */ \ - /* Dispatch event if value changed */ \ - void Clear##name() \ - { \ - _##name.set = false; \ - }; \ - \ -private: \ - NullableSetting _##name{}; \ - winrt::Windows::Foundation::IReference _get##name##Impl() const \ - { \ - return Has##name() ? \ - _##name.setting : \ - (_parent ? \ - _parent->_get##name##Impl() : \ - winrt::Windows::Foundation::IReference{ __VA_ARGS__ }); \ +#define GETSET_NULLABLE_SETTING(type, name, ...) \ +public: \ + /* Returns true if the user explicitly set the value, false otherwise*/ \ + bool Has##name() const \ + { \ + return _##name.set; \ + }; \ + \ + /* Returns the resolved value for this setting */ \ + /* fallback: user set value --> inherited value --> system set value */ \ + winrt::Windows::Foundation::IReference name() const \ + { \ + const auto val{ _get##name##Impl() }; \ + return val.set ? val.setting : winrt::Windows::Foundation::IReference{ __VA_ARGS__ }; \ + }; \ + \ + /* Overwrite the user set value */ \ + /* Dispatch event if value changed */ \ + void name(const winrt::Windows::Foundation::IReference& value) \ + { \ + if (!Has##name() /*value was not set*/ \ + || _##name.setting != value) /*set value is different*/ \ + { \ + _##name.setting = value; \ + _##name.set = true; \ + } \ + }; \ + \ + /* Clear the user set value */ \ + /* Dispatch event if value changed */ \ + void Clear##name() \ + { \ + _##name.set = false; \ + }; \ + \ +private: \ + NullableSetting _##name{}; \ + NullableSetting _get##name##Impl() const \ + { \ + /*return user set value*/ \ + if (Has##name()) \ + { \ + return _##name; \ + } \ + \ + /*user set value was not set*/ \ + /*iterate through parents to find a value*/ \ + for (auto parent : _parents) \ + { \ + auto val{ parent->_get##name##Impl() }; \ + if (val.set) \ + { \ + return val; \ + } \ + } \ + /*no value was found*/ \ + return { nullptr, false }; \ }; // Use this macro for Profile::ApplyTo diff --git a/src/cascadia/TerminalSettingsModel/Profile.cpp b/src/cascadia/TerminalSettingsModel/Profile.cpp index 3314ab4b9cc..44f364a8c0a 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.cpp +++ b/src/cascadia/TerminalSettingsModel/Profile.cpp @@ -111,6 +111,13 @@ winrt::com_ptr Profile::Copy() const profile->_ConnectionType = _ConnectionType; profile->_BackgroundImageAlignment = _BackgroundImageAlignment; profile->_BellStyle = _BellStyle; + + // TODO CARLOS: copy parents instead of parent + //if (_parent) + //{ + // profile->_parent = _parent->Copy(); + //} + return profile; } @@ -585,7 +592,8 @@ bool Profile::HasStartingDirectory() const winrt::Windows::Foundation::IInspectable Profile::StartingDirectory() const { - return _getStartingDirectoryImpl(); + const auto val{ _getStartingDirectoryImpl() }; + return val.set ? val.setting : nullptr; }; void Profile::StartingDirectory(const winrt::Windows::Foundation::IInspectable& value) diff --git a/src/cascadia/TerminalSettingsModel/Profile.h b/src/cascadia/TerminalSettingsModel/Profile.h index a59a271da76..08889d0c265 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.h +++ b/src/cascadia/TerminalSettingsModel/Profile.h @@ -135,21 +135,45 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation std::optional> _BackgroundImageAlignment{ std::nullopt }; std::optional> _getBackgroundImageAlignmentImpl() const { - return _BackgroundImageAlignment ? - _BackgroundImageAlignment : - (_parent ? - _parent->_getBackgroundImageAlignmentImpl() : - std::nullopt); + /*return user set value*/ + if (_BackgroundImageAlignment) + { + return _BackgroundImageAlignment; + } + + /*user set value was not set*/ /*iterate through parents to find a value*/ + for (auto parent : _parents) + { + if (auto val{ parent->_getBackgroundImageAlignmentImpl() }) + { + return val; + } + } + + /*no value was found*/ + return std::nullopt; }; NullableString _StartingDirectory{}; - winrt::Windows::Foundation::IInspectable _getStartingDirectoryImpl() const + NullableString _getStartingDirectoryImpl() const { - return HasStartingDirectory() ? - _StartingDirectory.setting : - (_parent ? - _parent->_getStartingDirectoryImpl() : - nullptr); + /*return user set value*/ + if (HasStartingDirectory()) + { + return _StartingDirectory; + } + + /*user set value was not set*/ + /*iterate through parents to find a value*/ + for (auto parent : _parents) + { + auto val{ parent->_getStartingDirectoryImpl() }; + if (val.set) + { + return val; + } + } /*no value was found*/ + return { nullptr, false }; }; static std::wstring EvaluateStartingDirectory(const std::wstring& directory); From d74f2364117d13c6160553e133679a2d894454f8 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Mon, 19 Oct 2020 21:37:19 -0700 Subject: [PATCH 07/29] fix logic error (needed see-thru guid) --- .../CascadiaSettingsSerialization.cpp | 6 ----- .../TerminalSettingsModel/Profile.cpp | 23 ++++--------------- src/cascadia/TerminalSettingsModel/Profile.h | 6 +---- 3 files changed, 5 insertions(+), 30 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp index 85342c3bf9b..da2c94622a4 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp @@ -638,12 +638,6 @@ void CascadiaSettings::_LayerOrCreateProfile(const Json::Value& profileJson) auto childImpl{ parent->CreateChild() }; childImpl->LayerJson(profileJson); - if (_userDefaultProfileSettings) - { - // Add profile.defaults as the _first_ parent to the child - childImpl->InsertParent(0, _userDefaultProfileSettings); - } - // replace parent in _profiles with child _profiles.SetAt(*profileIndex, *childImpl); } diff --git a/src/cascadia/TerminalSettingsModel/Profile.cpp b/src/cascadia/TerminalSettingsModel/Profile.cpp index 44f364a8c0a..373cb19163c 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.cpp +++ b/src/cascadia/TerminalSettingsModel/Profile.cpp @@ -178,7 +178,9 @@ winrt::com_ptr>(json, GuidKey) }) { - if (otherGuid.value() != *_Guid) + if (otherGuid.value() != myGuid) { return false; } @@ -552,23 +554,6 @@ void Profile::BackgroundImageVerticalAlignment(const VerticalAlignment& value) n } #pragma endregion -bool Profile::HasGuid() const noexcept -{ - return _Guid.has_value(); -} - -winrt::guid Profile::Guid() const -{ - // This can throw if we never had our guid set to a legitimate value. - THROW_HR_IF_MSG(E_FAIL, !_Guid.has_value(), "Profile._guid always expected to have a value"); - return *_Guid; -} - -void Profile::Guid(const winrt::guid& guid) noexcept -{ - _Guid = guid; -} - bool Profile::HasConnectionType() const noexcept { return _ConnectionType.has_value(); diff --git a/src/cascadia/TerminalSettingsModel/Profile.h b/src/cascadia/TerminalSettingsModel/Profile.h index 08889d0c265..002d1e11819 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.h +++ b/src/cascadia/TerminalSettingsModel/Profile.h @@ -59,10 +59,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation void GenerateGuidIfNecessary() noexcept; static guid GetGuidOrGenerateForJson(const Json::Value& json) noexcept; - bool HasGuid() const noexcept; - winrt::guid Guid() const; - void Guid(const winrt::guid& guid) noexcept; - bool HasConnectionType() const noexcept; winrt::guid ConnectionType() const noexcept; void ConnectionType(const winrt::guid& conType) noexcept; @@ -82,6 +78,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation void StartingDirectory(const winrt::Windows::Foundation::IInspectable& value); void ClearStartingDirectory(); + GETSET_SETTING(guid, Guid); GETSET_SETTING(hstring, Name, L"Default"); GETSET_SETTING(hstring, Source); GETSET_SETTING(bool, Hidden, false); @@ -130,7 +127,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation GETSET_SETTING(Model::BellStyle, BellStyle, BellStyle::Audible); private: - std::optional _Guid{ std::nullopt }; std::optional _ConnectionType{ std::nullopt }; std::optional> _BackgroundImageAlignment{ std::nullopt }; std::optional> _getBackgroundImageAlignmentImpl() const From 9685108034894abaa7482f4790081001ab4a0497 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Tue, 20 Oct 2020 13:16:32 -0700 Subject: [PATCH 08/29] startingDirectory is just a setting --- .../AzureCloudShellGenerator.cpp | 3 +- .../TerminalSettingsModel/IInheritable.h | 8 --- .../PowershellCoreProfileGenerator.cpp | 3 +- .../TerminalSettingsModel/Profile.cpp | 56 ++----------------- src/cascadia/TerminalSettingsModel/Profile.h | 30 +--------- .../TerminalSettingsModel/Profile.idl | 2 +- .../WslDistroGenerator.cpp | 3 +- 7 files changed, 10 insertions(+), 95 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/AzureCloudShellGenerator.cpp b/src/cascadia/TerminalSettingsModel/AzureCloudShellGenerator.cpp index 23a8e7520c0..3435d795591 100644 --- a/src/cascadia/TerminalSettingsModel/AzureCloudShellGenerator.cpp +++ b/src/cascadia/TerminalSettingsModel/AzureCloudShellGenerator.cpp @@ -35,8 +35,7 @@ std::vector AzureCloudShellGenerator::GenerateProfiles() { auto azureCloudShellProfile{ CreateDefaultProfile(L"Azure Cloud Shell") }; azureCloudShellProfile.Commandline(L"Azure"); - const auto startDir{ winrt::Windows::Foundation::PropertyValue::CreateString(DEFAULT_STARTING_DIRECTORY) }; - azureCloudShellProfile.StartingDirectory(startDir); + azureCloudShellProfile.StartingDirectory(DEFAULT_STARTING_DIRECTORY); azureCloudShellProfile.ColorSchemeName(L"Vintage"); azureCloudShellProfile.ConnectionType(AzureConnection::ConnectionType()); profiles.emplace_back(azureCloudShellProfile); diff --git a/src/cascadia/TerminalSettingsModel/IInheritable.h b/src/cascadia/TerminalSettingsModel/IInheritable.h index dca0bf4bb57..3f26bbc237e 100644 --- a/src/cascadia/TerminalSettingsModel/IInheritable.h +++ b/src/cascadia/TerminalSettingsModel/IInheritable.h @@ -73,14 +73,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation winrt::Windows::Foundation::IReference setting{ nullptr }; bool set{ false }; }; - - // hstrings cannot be exposed in IDLs as nullable - // We can get around that by treating it as an IInspectable - struct NullableString - { - winrt::Windows::Foundation::IInspectable setting{ nullptr }; - bool set{ false }; - }; } // Use this macro to quickly implement both getters and the setter for an diff --git a/src/cascadia/TerminalSettingsModel/PowershellCoreProfileGenerator.cpp b/src/cascadia/TerminalSettingsModel/PowershellCoreProfileGenerator.cpp index 71650c29f3d..f882a929056 100644 --- a/src/cascadia/TerminalSettingsModel/PowershellCoreProfileGenerator.cpp +++ b/src/cascadia/TerminalSettingsModel/PowershellCoreProfileGenerator.cpp @@ -310,8 +310,7 @@ std::vector PowershellCoreProfileGenerator::GenerateProfiles() const auto name = psI.Name(); auto profile{ CreateDefaultProfile(name) }; profile.Commandline(psI.executablePath.wstring()); - const auto startDir{ winrt::Windows::Foundation::PropertyValue::CreateString(DEFAULT_STARTING_DIRECTORY) }; - profile.StartingDirectory(startDir); + profile.StartingDirectory(DEFAULT_STARTING_DIRECTORY); profile.ColorSchemeName(L"Campbell"); profile.Icon(WI_IsFlagSet(psI.flags, PowerShellFlags::Preview) ? POWERSHELL_PREVIEW_ICON : POWERSHELL_ICON); diff --git a/src/cascadia/TerminalSettingsModel/Profile.cpp b/src/cascadia/TerminalSettingsModel/Profile.cpp index 373cb19163c..3ef466d864e 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.cpp +++ b/src/cascadia/TerminalSettingsModel/Profile.cpp @@ -289,12 +289,7 @@ void Profile::LayerJson(const Json::Value& json) JsonUtils::GetValueForKey(json, PaddingKey, _Padding, JsonUtils::PermissiveStringConverter{}); JsonUtils::GetValueForKey(json, ScrollbarStateKey, _ScrollState); - - // StartingDirectory is nullable. But WinRT does not allow IReference to be exposed in the IDL. - // Exposing it as an IInspectable is a workaround for this issue. - std::optional startDir{ std::nullopt }; - _StartingDirectory.set = JsonUtils::GetValueForKey(json, StartingDirectoryKey, startDir); - _StartingDirectory.setting = startDir ? PropertyValue::CreateString(*startDir) : nullptr; + JsonUtils::GetValueForKey(json, StartingDirectoryKey, _StartingDirectory); JsonUtils::GetValueForKey(json, IconKey, _Icon); JsonUtils::GetValueForKey(json, BackgroundImageKey, _BackgroundImagePath); @@ -347,18 +342,12 @@ winrt::hstring Profile::ExpandedBackgroundImagePath() const winrt::hstring Profile::EvaluatedStartingDirectory() const { - const auto path{ StartingDirectory() }; - const auto pathString{ path.as>() }; - if (pathString == nullptr) - { - // Translate explicit null to empty string - // This is interpreted on the other end as "current directory for process" - return L""; - } - else + if (_StartingDirectory) { - return winrt::hstring{ Profile::EvaluateStartingDirectory(pathString.Value().c_str()) }; + return winrt::hstring{ Profile::EvaluateStartingDirectory(_StartingDirectory->c_str()) }; } + // treated as "inherit directory from parent process" + return L""; } // Method Description: @@ -568,38 +557,3 @@ void Profile::ConnectionType(const winrt::guid& conType) noexcept { _ConnectionType = conType; } - -#pragma region StartingDirectory -bool Profile::HasStartingDirectory() const -{ - return _StartingDirectory.set; -}; - -winrt::Windows::Foundation::IInspectable Profile::StartingDirectory() const -{ - const auto val{ _getStartingDirectoryImpl() }; - return val.set ? val.setting : nullptr; -}; - -void Profile::StartingDirectory(const winrt::Windows::Foundation::IInspectable& value) -{ - // Set the value if... - // - the value is not set already - // - both values are not null - // - both values are not the same string - auto myVal{ _StartingDirectory.setting.as>() }; - auto otherVal{ value.as>() }; - if (!HasStartingDirectory() || - !myVal == !otherVal || - myVal.Value() == otherVal.Value()) - { - _StartingDirectory.setting = value; - _StartingDirectory.set = true; - } -} - -void Profile::ClearStartingDirectory() -{ - _StartingDirectory.set = false; -}; -#pragma endregion diff --git a/src/cascadia/TerminalSettingsModel/Profile.h b/src/cascadia/TerminalSettingsModel/Profile.h index 002d1e11819..eac64a940a5 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.h +++ b/src/cascadia/TerminalSettingsModel/Profile.h @@ -71,13 +71,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation const Windows::UI::Xaml::VerticalAlignment BackgroundImageVerticalAlignment() const noexcept; void BackgroundImageVerticalAlignment(const Windows::UI::Xaml::VerticalAlignment& value) noexcept; - // StartingDirectory is a nullable hstring. The IDL does not allow - // hstring to be nullable. So we must expose it as an IInspectable - bool HasStartingDirectory() const; - winrt::Windows::Foundation::IInspectable StartingDirectory() const; - void StartingDirectory(const winrt::Windows::Foundation::IInspectable& value); - void ClearStartingDirectory(); - GETSET_SETTING(guid, Guid); GETSET_SETTING(hstring, Name, L"Default"); GETSET_SETTING(hstring, Source); @@ -100,6 +93,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation GETSET_SETTING(hstring, Padding, DEFAULT_PADDING); GETSET_SETTING(hstring, Commandline, L"cmd.exe"); + GETSET_SETTING(hstring, StartingDirectory); GETSET_SETTING(hstring, BackgroundImagePath); GETSET_SETTING(double, BackgroundImageOpacity, 1.0); @@ -150,28 +144,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation return std::nullopt; }; - NullableString _StartingDirectory{}; - NullableString _getStartingDirectoryImpl() const - { - /*return user set value*/ - if (HasStartingDirectory()) - { - return _StartingDirectory; - } - - /*user set value was not set*/ - /*iterate through parents to find a value*/ - for (auto parent : _parents) - { - auto val{ parent->_getStartingDirectoryImpl() }; - if (val.set) - { - return val; - } - } /*no value was found*/ - return { nullptr, false }; - }; - static std::wstring EvaluateStartingDirectory(const std::wstring& directory); static guid _GenerateGuidForProfile(const hstring& name, const hstring& source) noexcept; diff --git a/src/cascadia/TerminalSettingsModel/Profile.idl b/src/cascadia/TerminalSettingsModel/Profile.idl index 5ff5c9dd217..ce228214e85 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.idl +++ b/src/cascadia/TerminalSettingsModel/Profile.idl @@ -92,7 +92,7 @@ namespace Microsoft.Terminal.Settings.Model Boolean HasStartingDirectory(); void ClearStartingDirectory(); - IInspectable StartingDirectory; + String StartingDirectory; String EvaluatedStartingDirectory { get; }; Boolean HasBackgroundImagePath(); diff --git a/src/cascadia/TerminalSettingsModel/WslDistroGenerator.cpp b/src/cascadia/TerminalSettingsModel/WslDistroGenerator.cpp index e7d9727b4d6..3cd4b7b0d95 100644 --- a/src/cascadia/TerminalSettingsModel/WslDistroGenerator.cpp +++ b/src/cascadia/TerminalSettingsModel/WslDistroGenerator.cpp @@ -128,8 +128,7 @@ std::vector WslDistroGenerator::GenerateProfiles() WSLDistro.Commandline(L"wsl.exe -d " + distName); WSLDistro.ColorSchemeName(L"Campbell"); - const auto startDir{ winrt::Windows::Foundation::PropertyValue::CreateString(DEFAULT_STARTING_DIRECTORY) }; - WSLDistro.StartingDirectory(startDir); + WSLDistro.StartingDirectory(DEFAULT_STARTING_DIRECTORY); WSLDistro.Icon(L"ms-appx:///ProfileIcons/{9acb9455-ca41-5af7-950f-6bca1bc9722f}.png"); profiles.emplace_back(WSLDistro); } From 298636bf5a6e42b5ee70f7ffc2b5630ec05adaef Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Tue, 20 Oct 2020 14:09:17 -0700 Subject: [PATCH 09/29] fix up some tests --- .../DeserializationTests.cpp | 15 +++---- .../LocalTests_SettingsModel/ProfileTests.cpp | 42 ++++++++++--------- .../CascadiaSettingsSerialization.cpp | 2 +- 3 files changed, 29 insertions(+), 30 deletions(-) diff --git a/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp b/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp index 9a1ed2fb482..b6d48310db8 100644 --- a/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp +++ b/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp @@ -1355,6 +1355,7 @@ namespace SettingsModelLocalTests const winrt::guid guid1{ ::Microsoft::Console::Utils::GuidFromString(L"{6239a42c-6666-49a3-80bd-e8fdd045185c}") }; const winrt::guid guid2{ ::Microsoft::Console::Utils::GuidFromString(L"{2C4DE342-38B7-51CF-B940-2309A097F518}") }; const winrt::guid fakeGuid{ ::Microsoft::Console::Utils::GuidFromString(L"{FFFFFFFF-FFFF-FFFF-FFFF-FFFFFFFFFFFF}") }; + const winrt::guid autogeneratedGuid{}; const std::optional badGuid{}; VerifyParseSucceeded(settings0String); @@ -1366,7 +1367,7 @@ namespace SettingsModelLocalTests VERIFY_ARE_EQUAL(guid0, settings->_GetProfileGuidByName(name0)); VERIFY_ARE_EQUAL(guid1, settings->_GetProfileGuidByName(name1)); VERIFY_ARE_EQUAL(guid2, settings->_GetProfileGuidByName(name2)); - VERIFY_ARE_EQUAL(badGuid, settings->_GetProfileGuidByName(name3)); + VERIFY_ARE_EQUAL(autogeneratedGuid, settings->_GetProfileGuidByName(name3)); VERIFY_ARE_EQUAL(badGuid, settings->_GetProfileGuidByName(badName)); auto prof0{ settings->FindProfile(guid0) }; @@ -1521,9 +1522,9 @@ namespace SettingsModelLocalTests { auto settings = winrt::make_self(false); settings->_ParseJsonString(settings0String, false); - VERIFY_IS_TRUE(settings->_userDefaultProfileSettings == Json::Value::null); + VERIFY_IS_NULL(settings->_userDefaultProfileSettings); settings->_ApplyDefaultsFromUserSettings(); - VERIFY_IS_FALSE(settings->_userDefaultProfileSettings == Json::Value::null); + VERIFY_IS_NOT_NULL(settings->_userDefaultProfileSettings); settings->LayerJson(settings->_userSettings); VERIFY_ARE_EQUAL(guid1String, settings->_globals->UnparsedDefaultProfile()); @@ -1573,9 +1574,9 @@ namespace SettingsModelLocalTests VERIFY_ARE_EQUAL(2u, settings->_profiles.Size()); settings->_ParseJsonString(settings0String, false); - VERIFY_IS_TRUE(settings->_userDefaultProfileSettings == Json::Value::null); + VERIFY_IS_NULL(settings->_userDefaultProfileSettings); settings->_ApplyDefaultsFromUserSettings(); - VERIFY_IS_FALSE(settings->_userDefaultProfileSettings == Json::Value::null); + VERIFY_IS_NOT_NULL(settings->_userDefaultProfileSettings); Log::Comment(NoThrowString().Format( L"Ensure that cmd and powershell don't get their GUIDs overwritten")); @@ -1692,10 +1693,6 @@ namespace SettingsModelLocalTests VERIFY_ARE_EQUAL(L"Terminal.App.UnitTest.1", settings->_profiles.GetAt(1).Source()); VERIFY_ARE_EQUAL(L"Terminal.App.UnitTest.1", settings->_profiles.GetAt(2).Source()); - VERIFY_IS_TRUE(settings->_profiles.GetAt(0).HasGuid()); - VERIFY_IS_TRUE(settings->_profiles.GetAt(1).HasGuid()); - VERIFY_IS_TRUE(settings->_profiles.GetAt(2).HasGuid()); - VERIFY_ARE_EQUAL(guid1, settings->_profiles.GetAt(0).Guid()); VERIFY_ARE_EQUAL(guid1, settings->_profiles.GetAt(1).Guid()); VERIFY_ARE_EQUAL(guid2, settings->_profiles.GetAt(2).Guid()); diff --git a/src/cascadia/LocalTests_SettingsModel/ProfileTests.cpp b/src/cascadia/LocalTests_SettingsModel/ProfileTests.cpp index 03d2e7c0826..5ad42162a60 100644 --- a/src/cascadia/LocalTests_SettingsModel/ProfileTests.cpp +++ b/src/cascadia/LocalTests_SettingsModel/ProfileTests.cpp @@ -132,39 +132,41 @@ namespace SettingsModelLocalTests Log::Comment(NoThrowString().Format( L"Layering profile1 on top of profile0")); - profile0->LayerJson(profile1Json); + auto profile1{ profile0->CreateChild() }; + profile1->LayerJson(profile1Json); - VERIFY_IS_NOT_NULL(profile0->Foreground()); - VERIFY_ARE_EQUAL(til::color(2, 2, 2), til::color{ profile0->Foreground().Value() }); + VERIFY_IS_NOT_NULL(profile1->Foreground()); + VERIFY_ARE_EQUAL(til::color(2, 2, 2), til::color{ profile1->Foreground().Value() }); - VERIFY_IS_NOT_NULL(profile0->Background()); - VERIFY_ARE_EQUAL(til::color(1, 1, 1), til::color{ profile0->Background().Value() }); + VERIFY_IS_NOT_NULL(profile1->Background()); + VERIFY_ARE_EQUAL(til::color(1, 1, 1), til::color{ profile1->Background().Value() }); - VERIFY_IS_NOT_NULL(profile0->Background()); - VERIFY_ARE_EQUAL(til::color(1, 1, 1), til::color{ profile0->Background().Value() }); + VERIFY_IS_NOT_NULL(profile1->Background()); + VERIFY_ARE_EQUAL(til::color(1, 1, 1), til::color{ profile1->Background().Value() }); - VERIFY_ARE_EQUAL(L"profile1", profile0->Name()); + VERIFY_ARE_EQUAL(L"profile1", profile1->Name()); - VERIFY_IS_FALSE(profile0->StartingDirectory().empty()); - VERIFY_ARE_EQUAL(L"C:/", profile0->StartingDirectory()); + VERIFY_IS_FALSE(profile1->StartingDirectory().empty()); + VERIFY_ARE_EQUAL(L"C:/", profile1->StartingDirectory()); Log::Comment(NoThrowString().Format( L"Layering profile2 on top of (profile0+profile1)")); - profile0->LayerJson(profile2Json); + auto profile2{ profile1->CreateChild() }; + profile2->LayerJson(profile2Json); - VERIFY_IS_NOT_NULL(profile0->Foreground()); - VERIFY_ARE_EQUAL(til::color(3, 3, 3), til::color{ profile0->Foreground().Value() }); + VERIFY_IS_NOT_NULL(profile2->Foreground()); + VERIFY_ARE_EQUAL(til::color(3, 3, 3), til::color{ profile2->Foreground().Value() }); - VERIFY_IS_NOT_NULL(profile0->Background()); - VERIFY_ARE_EQUAL(til::color(1, 1, 1), til::color{ profile0->Background().Value() }); + VERIFY_IS_NOT_NULL(profile2->Background()); + VERIFY_ARE_EQUAL(til::color(1, 1, 1), til::color{ profile2->Background().Value() }); - VERIFY_IS_NOT_NULL(profile0->SelectionBackground()); - VERIFY_ARE_EQUAL(til::color(2, 2, 2), til::color{ profile0->SelectionBackground().Value() }); + VERIFY_IS_NOT_NULL(profile2->SelectionBackground()); + VERIFY_ARE_EQUAL(til::color(2, 2, 2), til::color{ profile2->SelectionBackground().Value() }); - VERIFY_ARE_EQUAL(L"profile2", profile0->Name()); + VERIFY_ARE_EQUAL(L"profile2", profile2->Name()); - VERIFY_IS_FALSE(profile0->StartingDirectory().empty()); - VERIFY_ARE_EQUAL(L"C:/", profile0->StartingDirectory()); + VERIFY_IS_FALSE(profile2->StartingDirectory().empty()); + VERIFY_ARE_EQUAL(L"C:/", profile2->StartingDirectory()); } void ProfileTests::LayerProfileIcon() diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp index da2c94622a4..8eb5ffab5f9 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp @@ -511,7 +511,7 @@ bool CascadiaSettings::_AppendDynamicProfilesToUserSettings() for (const auto& profile : _profiles) { - if (!profile.HasGuid()) + if (profile.Guid() == winrt::guid{}) { // If the profile doesn't have a guid, it's a name-only profile. // During validation, we'll generate a GUID for the profile, but From 49b98b38fb6d46db907c80e7bf149629b2fc22bc Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Tue, 20 Oct 2020 15:33:24 -0700 Subject: [PATCH 10/29] polish --- .../TerminalSettingsModel/ActionAndArgs.cpp | 2 +- .../CascadiaSettingsSerialization.cpp | 4 +- .../GlobalAppSettings.cpp | 10 +--- .../TerminalSettingsModel/IInheritable.h | 14 +---- .../TerminalSettingsModel/Profile.cpp | 55 ++----------------- src/cascadia/TerminalSettingsModel/Profile.h | 1 - 6 files changed, 13 insertions(+), 73 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/ActionAndArgs.cpp b/src/cascadia/TerminalSettingsModel/ActionAndArgs.cpp index 2cf9c7febbc..14dedc0a24f 100644 --- a/src/cascadia/TerminalSettingsModel/ActionAndArgs.cpp +++ b/src/cascadia/TerminalSettingsModel/ActionAndArgs.cpp @@ -244,7 +244,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation { auto copy{ winrt::make_self() }; copy->_Action = _Action; - copy->_Args = _Args.Copy(); + copy->_Args = _Args ? _Args.Copy() : IActionArgs{ nullptr }; return copy; } diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp index 8eb5ffab5f9..07cc4be28d3 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp @@ -650,11 +650,11 @@ void CascadiaSettings::_LayerOrCreateProfile(const Json::Value& profileJson) { auto profile{ winrt::make_self() }; - // GH#2325: If we have a set of default profile settings, apply them here. + // GH#2325: If we have a set of default profile settings, set that as my parent. // We _won't_ have these settings yet for defaults, dynamic profiles. if (_userDefaultProfileSettings) { - profile = _userDefaultProfileSettings->CreateChild(); + profile->InsertParent(0,_userDefaultProfileSettings); } profile->LayerJson(profileJson); diff --git a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp index befb87c2db0..4754de16ec7 100644 --- a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp +++ b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp @@ -71,14 +71,11 @@ GlobalAppSettings::GlobalAppSettings() : // - void GlobalAppSettings::_FinalizeInheritance() { - // TODO CARLOS: Crash invoking IActionAndArgs->Copy - //_keymap = _parent->_keymap->Copy(); - // Globals only ever has 1 parent FAIL_FAST_IF(_parents.size() > 1); for (auto parent : _parents) { - _keymap = parent->_keymap; + _keymap = parent->_keymap->Copy(); std::copy(parent->_keybindingsWarnings.begin(), parent->_keybindingsWarnings.end(), std::back_inserter(_keybindingsWarnings)); for (auto kv : parent->_colorSchemes) { @@ -123,10 +120,9 @@ winrt::com_ptr GlobalAppSettings::Copy() const globals->_DisableAnimations = _DisableAnimations; globals->_UnparsedDefaultProfile = _UnparsedDefaultProfile; + globals->_validDefaultProfile = _validDefaultProfile; globals->_defaultProfile = _defaultProfile; - // TODO CARLOS: Crash invoking IActionAndArgs->Copy - //globals->_keymap = _keymap->Copy(); - globals->_keymap = _keymap; + globals->_keymap = _keymap->Copy(); std::copy(_keybindingsWarnings.begin(), _keybindingsWarnings.end(), std::back_inserter(globals->_keybindingsWarnings)); for (auto kv : _colorSchemes) diff --git a/src/cascadia/TerminalSettingsModel/IInheritable.h b/src/cascadia/TerminalSettingsModel/IInheritable.h index 3f26bbc237e..34d0632418e 100644 --- a/src/cascadia/TerminalSettingsModel/IInheritable.h +++ b/src/cascadia/TerminalSettingsModel/IInheritable.h @@ -45,11 +45,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation void InsertParent(size_t index, com_ptr parent) { - auto pos{ _parents.begin() }; - for (size_t i=0; i_##setting = _##setting; \ - } diff --git a/src/cascadia/TerminalSettingsModel/Profile.cpp b/src/cascadia/TerminalSettingsModel/Profile.cpp index 3ef466d864e..bad80d7e288 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.cpp +++ b/src/cascadia/TerminalSettingsModel/Profile.cpp @@ -72,7 +72,9 @@ Profile::Profile(guid guid) : winrt::com_ptr Profile::Copy() const { + // copy the private members auto profile{ winrt::make_self() }; + profile->_Guid = _Guid; profile->_Name = _Name; profile->_Source = _Source; profile->_Hidden = _Hidden; @@ -107,11 +109,12 @@ winrt::com_ptr Profile::Copy() const profile->_AltGrAliasing = _AltGrAliasing; profile->_CursorShape = _CursorShape; profile->_CursorHeight = _CursorHeight; - profile->_Guid = _Guid; - profile->_ConnectionType = _ConnectionType; - profile->_BackgroundImageAlignment = _BackgroundImageAlignment; profile->_BellStyle = _BellStyle; + // copy settings that did not use the GETSET macro + profile->_BackgroundImageAlignment = _BackgroundImageAlignment; + profile->_ConnectionType = _ConnectionType; + // TODO CARLOS: copy parents instead of parent //if (_parent) //{ @@ -416,52 +419,6 @@ bool Profile::IsDynamicProfileObject(const Json::Value& json) return !source.isNull(); } -// Function Description: -// - Apply each of the current Profile's settings values onto the given Profile, if it is set -// Arguments: -// - profile: the Profile to be updated -// Return Value: -// - -void Profile::ApplyTo(Profile* profile) const -{ - APPLY_OUT(Name); - APPLY_OUT(Source); - APPLY_OUT(Hidden); - APPLY_OUT(Icon); - APPLY_OUT(CloseOnExit); - APPLY_OUT(TabTitle); - APPLY_OUT(TabColor); - APPLY_OUT(SuppressApplicationTitle); - APPLY_OUT(UseAcrylic); - APPLY_OUT(AcrylicOpacity); - APPLY_OUT(ScrollState); - APPLY_OUT(FontFace); - APPLY_OUT(FontSize); - APPLY_OUT(FontWeight); - APPLY_OUT(Padding); - APPLY_OUT(Commandline); - APPLY_OUT(StartingDirectory); - APPLY_OUT(BackgroundImagePath); - APPLY_OUT(BackgroundImageOpacity); - APPLY_OUT(BackgroundImageStretchMode); - APPLY_OUT(BackgroundImageAlignment); - APPLY_OUT(AntialiasingMode); - APPLY_OUT(RetroTerminalEffect); - APPLY_OUT(ForceFullRepaintRendering); - APPLY_OUT(SoftwareRendering); - APPLY_OUT(ColorSchemeName); - APPLY_OUT(Foreground); - APPLY_OUT(Background); - APPLY_OUT(SelectionBackground); - APPLY_OUT(CursorColor); - APPLY_OUT(HistorySize); - APPLY_OUT(SnapOnInput); - APPLY_OUT(AltGrAliasing); - APPLY_OUT(CursorShape); - APPLY_OUT(CursorHeight); - APPLY_OUT(BellStyle); -} - // Function Description: // - Generates a unique guid for a profile, given the name. For an given name, will always return the same GUID. // Arguments: diff --git a/src/cascadia/TerminalSettingsModel/Profile.h b/src/cascadia/TerminalSettingsModel/Profile.h index eac64a940a5..abebe0b1789 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.h +++ b/src/cascadia/TerminalSettingsModel/Profile.h @@ -52,7 +52,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation bool ShouldBeLayered(const Json::Value& json) const; void LayerJson(const Json::Value& json); static bool IsDynamicProfileObject(const Json::Value& json); - void ApplyTo(Profile* profile) const; hstring EvaluatedStartingDirectory() const; hstring ExpandedBackgroundImagePath() const; From e026f325df4bb468a80d58a621d0e6a62eda7be8 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Tue, 20 Oct 2020 17:59:39 -0700 Subject: [PATCH 11/29] first draft of CloneInheritanceGraph --- .../DeserializationTests.cpp | 65 +++++++++ .../CascadiaSettings.cpp | 25 +++- .../TerminalSettingsModel/IInheritable.h | 5 + .../TerminalSettingsModel/Profile.cpp | 136 ++++++++++++------ src/cascadia/TerminalSettingsModel/Profile.h | 3 + 5 files changed, 184 insertions(+), 50 deletions(-) diff --git a/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp b/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp index b6d48310db8..9222abefc8b 100644 --- a/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp +++ b/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp @@ -81,6 +81,7 @@ namespace SettingsModelLocalTests TEST_METHOD(TestRebindNestedCommand); TEST_METHOD(TestCopy); + TEST_METHOD(TestCloneInheritanceTree); TEST_CLASS_SETUP(ClassSetup) { @@ -2457,4 +2458,68 @@ namespace SettingsModelLocalTests copyImpl->_globals->WordDelimiters(L"changed value"); VERIFY_ARE_NOT_EQUAL(settings->_globals->WordDelimiters(), copyImpl->_globals->WordDelimiters()); } + + void DeserializationTests::TestCloneInheritanceTree() + { + const std::string settingsJson{ R"( + { + "defaultProfile": "{61c54bbd-1111-5271-96e7-009a87ff44bf}", + "profiles": + { + "defaults": { + "name": "PROFILE DEFAULTS", + }, + "list": [ + { + "guid": "{61c54bbd-1111-5271-96e7-009a87ff44bf}", + "name": "CMD" + }, + { + "guid": "{61c54bbd-2222-5271-96e7-009a87ff44bf}", + "name": "PowerShell" + }, + { + "guid": "{61c54bbd-3333-5271-96e7-009a87ff44bf}" + } + ] + } + })" }; + + VerifyParseSucceeded(settingsJson); + + auto settings{ winrt::make_self() }; + settings->_ParseJsonString(settingsJson, false); + settings->LayerJson(settings->_userSettings); + settings->_ValidateSettings(); + + DebugBreak(); + const auto copy{ settings->Copy() }; + const auto copyImpl{ winrt::get_self(copy) }; + + // test globals + VERIFY_ARE_EQUAL(settings->_globals->DefaultProfile(), copyImpl->_globals->DefaultProfile()); + + // test profiles + VERIFY_ARE_EQUAL(settings->_profiles.Size(), copyImpl->_profiles.Size()); + VERIFY_ARE_EQUAL(settings->_profiles.GetAt(0).Name(), copyImpl->_profiles.GetAt(0).Name()); + VERIFY_ARE_EQUAL(settings->_profiles.GetAt(1).Name(), copyImpl->_profiles.GetAt(1).Name()); + VERIFY_ARE_EQUAL(settings->_profiles.GetAt(2).Name(), copyImpl->_profiles.GetAt(2).Name()); + VERIFY_ARE_EQUAL(settings->_userDefaultProfileSettings->Name(), copyImpl->_userDefaultProfileSettings->Name()); + + // Modifying profile.defaults should... + VERIFY_ARE_EQUAL(settings->_userDefaultProfileSettings->HasName(), copyImpl->_userDefaultProfileSettings->HasName()); + copyImpl->_userDefaultProfileSettings->Name(L"changed value"); + + // keep the same name for the first two profiles + VERIFY_ARE_EQUAL(settings->_profiles.Size(), copyImpl->_profiles.Size()); + VERIFY_ARE_EQUAL(settings->_profiles.GetAt(0).Name(), copyImpl->_profiles.GetAt(0).Name()); + VERIFY_ARE_EQUAL(settings->_profiles.GetAt(1).Name(), copyImpl->_profiles.GetAt(1).Name()); + + // change the name for the one that inherited it from profile.defaults + VERIFY_ARE_NOT_EQUAL(settings->_profiles.GetAt(2).Name(), copyImpl->_profiles.GetAt(2).Name()); + + // profile.defaults should be different between the two graphs + VERIFY_ARE_NOT_EQUAL(settings->_userDefaultProfileSettings->HasName(), copyImpl->_userDefaultProfileSettings->HasName()); + VERIFY_ARE_NOT_EQUAL(settings->_userDefaultProfileSettings->Name(), copyImpl->_userDefaultProfileSettings->Name()); + } } diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp index 0110c4babdf..7ff18d8447e 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp @@ -71,11 +71,6 @@ winrt::Microsoft::Terminal::Settings::Model::CascadiaSettings CascadiaSettings:: // dynamic profile generators added by default auto settings{ winrt::make_self() }; settings->_globals = _globals->Copy(); - for (const auto profile : _profiles) - { - auto profImpl{ winrt::get_self(profile) }; - settings->_profiles.Append(*profImpl->Copy()); - } for (auto warning : _warnings) { settings->_warnings.Append(warning); @@ -86,6 +81,26 @@ winrt::Microsoft::Terminal::Settings::Model::CascadiaSettings CascadiaSettings:: settings->_userSettings = _userSettings; settings->_defaultSettings = _defaultSettings; settings->_userDefaultProfileSettings = _userDefaultProfileSettings; + + // Our profiles inheritance graph doesn't have a formal root. + // However, if we create a dummy Profile, and set _profiles as the parent, + // we now have a root. So we'll do just that, and + auto dummyRootSource{ winrt::make_self() }; + for (auto profile : _profiles) + { + winrt::com_ptr profileImpl; + profileImpl.copy_from(winrt::get_self(profile)); + dummyRootSource->InsertParent(profileImpl); + } + auto dummyRootClone{ winrt::make_self() }; + Profile::CloneInheritanceGraph(dummyRootSource, dummyRootClone); + + auto cloneParents{ dummyRootClone->ExportParents() }; + for (auto profile : cloneParents) + { + settings->_profiles.Append(*profile); + } + return *settings; } diff --git a/src/cascadia/TerminalSettingsModel/IInheritable.h b/src/cascadia/TerminalSettingsModel/IInheritable.h index 34d0632418e..4f61564b92d 100644 --- a/src/cascadia/TerminalSettingsModel/IInheritable.h +++ b/src/cascadia/TerminalSettingsModel/IInheritable.h @@ -49,6 +49,11 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation _parents.insert(pos, parent); } + std::vector> ExportParents() + { + return _parents; + } + protected: std::vector> _parents{}; diff --git a/src/cascadia/TerminalSettingsModel/Profile.cpp b/src/cascadia/TerminalSettingsModel/Profile.cpp index bad80d7e288..67eb691e187 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.cpp +++ b/src/cascadia/TerminalSettingsModel/Profile.cpp @@ -70,60 +70,106 @@ Profile::Profile(guid guid) : { } -winrt::com_ptr Profile::Copy() const +winrt::com_ptr Profile::_CopyMembers(winrt::com_ptr source) { // copy the private members auto profile{ winrt::make_self() }; - profile->_Guid = _Guid; - profile->_Name = _Name; - profile->_Source = _Source; - profile->_Hidden = _Hidden; - profile->_Icon = _Icon; - profile->_CloseOnExit = _CloseOnExit; - profile->_TabTitle = _TabTitle; - profile->_TabColor = _TabColor; - profile->_SuppressApplicationTitle = _SuppressApplicationTitle; - profile->_UseAcrylic = _UseAcrylic; - profile->_AcrylicOpacity = _AcrylicOpacity; - profile->_ScrollState = _ScrollState; - profile->_FontFace = _FontFace; - profile->_FontSize = _FontSize; - profile->_FontWeight = _FontWeight; - profile->_Padding = _Padding; - profile->_Commandline = _Commandline; - profile->_StartingDirectory = _StartingDirectory; - profile->_BackgroundImagePath = _BackgroundImagePath; - profile->_BackgroundImageOpacity = _BackgroundImageOpacity; - profile->_BackgroundImageStretchMode = _BackgroundImageStretchMode; - profile->_AntialiasingMode = _AntialiasingMode; - profile->_RetroTerminalEffect = _RetroTerminalEffect; - profile->_ForceFullRepaintRendering = _ForceFullRepaintRendering; - profile->_SoftwareRendering = _SoftwareRendering; - profile->_ColorSchemeName = _ColorSchemeName; - profile->_Foreground = _Foreground; - profile->_Background = _Background; - profile->_SelectionBackground = _SelectionBackground; - profile->_CursorColor = _CursorColor; - profile->_HistorySize = _HistorySize; - profile->_SnapOnInput = _SnapOnInput; - profile->_AltGrAliasing = _AltGrAliasing; - profile->_CursorShape = _CursorShape; - profile->_CursorHeight = _CursorHeight; - profile->_BellStyle = _BellStyle; + profile->_Guid = source->_Guid; + profile->_Name = source->_Name; + profile->_Source = source->_Source; + profile->_Hidden = source->_Hidden; + profile->_Icon = source->_Icon; + profile->_CloseOnExit = source->_CloseOnExit; + profile->_TabTitle = source->_TabTitle; + profile->_TabColor = source->_TabColor; + profile->_SuppressApplicationTitle = source->_SuppressApplicationTitle; + profile->_UseAcrylic = source->_UseAcrylic; + profile->_AcrylicOpacity = source->_AcrylicOpacity; + profile->_ScrollState = source->_ScrollState; + profile->_FontFace = source->_FontFace; + profile->_FontSize = source->_FontSize; + profile->_FontWeight = source->_FontWeight; + profile->_Padding = source->_Padding; + profile->_Commandline = source->_Commandline; + profile->_StartingDirectory = source->_StartingDirectory; + profile->_BackgroundImagePath = source->_BackgroundImagePath; + profile->_BackgroundImageOpacity = source->_BackgroundImageOpacity; + profile->_BackgroundImageStretchMode = source->_BackgroundImageStretchMode; + profile->_AntialiasingMode = source->_AntialiasingMode; + profile->_RetroTerminalEffect = source->_RetroTerminalEffect; + profile->_ForceFullRepaintRendering = source->_ForceFullRepaintRendering; + profile->_SoftwareRendering = source->_SoftwareRendering; + profile->_ColorSchemeName = source->_ColorSchemeName; + profile->_Foreground = source->_Foreground; + profile->_Background = source->_Background; + profile->_SelectionBackground = source->_SelectionBackground; + profile->_CursorColor = source->_CursorColor; + profile->_HistorySize = source->_HistorySize; + profile->_SnapOnInput = source->_SnapOnInput; + profile->_AltGrAliasing = source->_AltGrAliasing; + profile->_CursorShape = source->_CursorShape; + profile->_CursorHeight = source->_CursorHeight; + profile->_BellStyle = source->_BellStyle; // copy settings that did not use the GETSET macro - profile->_BackgroundImageAlignment = _BackgroundImageAlignment; - profile->_ConnectionType = _ConnectionType; - - // TODO CARLOS: copy parents instead of parent - //if (_parent) - //{ - // profile->_parent = _parent->Copy(); - //} + profile->_BackgroundImageAlignment = source->_BackgroundImageAlignment; + profile->_ConnectionType = source->_ConnectionType; return profile; } +// Method Description: +// - Creates a copy of the inheritance graph by performing a depth-first traversal recursively. +// Profiles are recorded as visited via the "visited" parameter. +// Unvisited Profiles are copied into the "cloneGraph" parameter, then marked as visited. +// Arguments: +// - sourceGraph - the graph of Profile's we're cloning +// - cloneGraph - the clone of sourceGraph that is being constructed +// - visited - a map of which Profiles have been visited, and, if so, a reference to the Profile's clone +// Return Value: +// - a clone in both inheritance structure and Profile values of sourceGraph +winrt::com_ptr Profile::CloneInheritanceGraph(winrt::com_ptr sourceGraph, winrt::com_ptr cloneGraph, std::unordered_map> visited) +{ + // If this is an unexplored Profile + // and we have parents... + if (visited.find(sourceGraph.get()) == visited.end() && !sourceGraph->_parents.empty()) + { + // iterate through all of our parents to copy them + for (auto sourceParent : sourceGraph->_parents) + { + // If we visited this Profile already... + auto kv{ visited.find(sourceParent.get()) }; + if (visited.find(sourceParent.get()) != visited.end()) + { + // add this Profile's clone as a parent + cloneGraph->InsertParent(kv->second); + } + else + { + // We have not visited this Profile yet, + // copy contents of sourceParent to clone + winrt::com_ptr clone{ sourceParent->Copy() }; + + // add the new copy to the cloneGraph + cloneGraph->InsertParent(clone); + + // copy the sub-graph at "clone" + CloneInheritanceGraph(sourceParent, clone, visited); + + // mark clone as "visited" + // save it to the map in case somebody else references it + visited[sourceParent.get()] = clone; + } + + // if clone is empty, this is the end of the inheritance line + // otherwise, it is populated with the contents of its parent + } + } + + // we have no more to explore down this path. + return cloneGraph; +} + // Method Description: // - Generates a Json::Value which is a "stub" of this profile. This stub will // have enough information that it could be layered with this profile. diff --git a/src/cascadia/TerminalSettingsModel/Profile.h b/src/cascadia/TerminalSettingsModel/Profile.h index abebe0b1789..82a98e35de2 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.h +++ b/src/cascadia/TerminalSettingsModel/Profile.h @@ -46,6 +46,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation Profile(); Profile(guid guid); com_ptr Copy() const; + static com_ptr CloneInheritanceGraph(com_ptr oldProfile, com_ptr newProfile, std::unordered_map> visited = {}); Json::Value GenerateStub() const; static com_ptr FromJson(const Json::Value& json); @@ -147,6 +148,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation static guid _GenerateGuidForProfile(const hstring& name, const hstring& source) noexcept; + static com_ptr _CopyMembers(com_ptr source); + friend class TerminalAppLocalTests::SettingsTests; friend class TerminalAppLocalTests::ProfileTests; friend class TerminalAppUnitTests::JsonTests; From dd8a2cbd9cc493888e4bba45f656af4c9d9a8f50 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Tue, 20 Oct 2020 18:17:13 -0700 Subject: [PATCH 12/29] fix inheritance linker error; optimize inheritance tree --- .../CascadiaSettingsSerialization.cpp | 11 +++++------ src/cascadia/TerminalSettingsModel/Profile.cpp | 2 +- src/cascadia/TerminalSettingsModel/Profile.h | 1 - 3 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp index 07cc4be28d3..74245b3261c 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp @@ -632,14 +632,13 @@ void CascadiaSettings::_LayerOrCreateProfile(const Json::Value& profileJson) auto profileIndex{ _FindMatchingProfileIndex(profileJson) }; if (profileIndex) { - // add a new inheritance layer, and apply json values to child auto parentProj{ _profiles.GetAt(*profileIndex) }; auto parent{ winrt::get_self(parentProj) }; - auto childImpl{ parent->CreateChild() }; - childImpl->LayerJson(profileJson); - // replace parent in _profiles with child - _profiles.SetAt(*profileIndex, *childImpl); + // We don't actually need to CreateChild() here. + // When we loaded Profile.Defaults, we created an empty child already. + // So this just populates the empty child + parent->LayerJson(profileJson); } else { @@ -654,7 +653,7 @@ void CascadiaSettings::_LayerOrCreateProfile(const Json::Value& profileJson) // We _won't_ have these settings yet for defaults, dynamic profiles. if (_userDefaultProfileSettings) { - profile->InsertParent(0,_userDefaultProfileSettings); + profile->InsertParent(0, _userDefaultProfileSettings); } profile->LayerJson(profileJson); diff --git a/src/cascadia/TerminalSettingsModel/Profile.cpp b/src/cascadia/TerminalSettingsModel/Profile.cpp index 67eb691e187..42c4797960a 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.cpp +++ b/src/cascadia/TerminalSettingsModel/Profile.cpp @@ -148,7 +148,7 @@ winrt::com_ptr Profile::CloneInheritanceGraph(winrt::com_ptr s { // We have not visited this Profile yet, // copy contents of sourceParent to clone - winrt::com_ptr clone{ sourceParent->Copy() }; + winrt::com_ptr clone{ _CopyMembers(sourceParent) }; // add the new copy to the cloneGraph cloneGraph->InsertParent(clone); diff --git a/src/cascadia/TerminalSettingsModel/Profile.h b/src/cascadia/TerminalSettingsModel/Profile.h index 82a98e35de2..d9d2cb0d27c 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.h +++ b/src/cascadia/TerminalSettingsModel/Profile.h @@ -45,7 +45,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation public: Profile(); Profile(guid guid); - com_ptr Copy() const; static com_ptr CloneInheritanceGraph(com_ptr oldProfile, com_ptr newProfile, std::unordered_map> visited = {}); Json::Value GenerateStub() const; From 07351bb72da33d0308a18c4cc9573aa200742f50 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Wed, 21 Oct 2020 12:08:23 -0700 Subject: [PATCH 13/29] fix inheritance copy --- .../DeserializationTests.cpp | 10 +++--- .../CascadiaSettings.cpp | 36 +++++++++++++++---- .../TerminalSettingsModel/CascadiaSettings.h | 1 + .../CascadiaSettingsSerialization.cpp | 20 ++++++++--- .../TerminalSettingsModel/Profile.cpp | 15 ++++---- src/cascadia/TerminalSettingsModel/Profile.h | 21 ++++++----- 6 files changed, 72 insertions(+), 31 deletions(-) diff --git a/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp b/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp index 9222abefc8b..1b23a5b4741 100644 --- a/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp +++ b/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp @@ -2467,7 +2467,7 @@ namespace SettingsModelLocalTests "profiles": { "defaults": { - "name": "PROFILE DEFAULTS", + "name": "PROFILE DEFAULTS" }, "list": [ { @@ -2489,10 +2489,10 @@ namespace SettingsModelLocalTests auto settings{ winrt::make_self() }; settings->_ParseJsonString(settingsJson, false); + settings->_ApplyDefaultsFromUserSettings(); settings->LayerJson(settings->_userSettings); settings->_ValidateSettings(); - DebugBreak(); const auto copy{ settings->Copy() }; const auto copyImpl{ winrt::get_self(copy) }; @@ -2510,16 +2510,16 @@ namespace SettingsModelLocalTests VERIFY_ARE_EQUAL(settings->_userDefaultProfileSettings->HasName(), copyImpl->_userDefaultProfileSettings->HasName()); copyImpl->_userDefaultProfileSettings->Name(L"changed value"); - // keep the same name for the first two profiles + // ...keep the same name for the first two profiles VERIFY_ARE_EQUAL(settings->_profiles.Size(), copyImpl->_profiles.Size()); VERIFY_ARE_EQUAL(settings->_profiles.GetAt(0).Name(), copyImpl->_profiles.GetAt(0).Name()); VERIFY_ARE_EQUAL(settings->_profiles.GetAt(1).Name(), copyImpl->_profiles.GetAt(1).Name()); - // change the name for the one that inherited it from profile.defaults + // ...but change the name for the one that inherited it from profile.defaults VERIFY_ARE_NOT_EQUAL(settings->_profiles.GetAt(2).Name(), copyImpl->_profiles.GetAt(2).Name()); // profile.defaults should be different between the two graphs - VERIFY_ARE_NOT_EQUAL(settings->_userDefaultProfileSettings->HasName(), copyImpl->_userDefaultProfileSettings->HasName()); + VERIFY_ARE_EQUAL(settings->_userDefaultProfileSettings->HasName(), copyImpl->_userDefaultProfileSettings->HasName()); VERIFY_ARE_NOT_EQUAL(settings->_userDefaultProfileSettings->Name(), copyImpl->_userDefaultProfileSettings->Name()); } } diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp index 7ff18d8447e..5ead0afb9bd 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp @@ -80,11 +80,24 @@ winrt::Microsoft::Terminal::Settings::Model::CascadiaSettings CascadiaSettings:: settings->_userSettingsString = _userSettingsString; settings->_userSettings = _userSettings; settings->_defaultSettings = _defaultSettings; - settings->_userDefaultProfileSettings = _userDefaultProfileSettings; + _CopyProfileInheritanceTree(settings.get()); + + return *settings; +} + +// Method Description: +// - Copies the inheritance tree for profiles and hooks them up to a clone CascadiaSettings +// Arguments: +// - cloneSettings: the CascadiaSettings we're copying the inheritance tree to +// Return Value: +// - +void CascadiaSettings::_CopyProfileInheritanceTree(CascadiaSettings* cloneSettings) const +{ // Our profiles inheritance graph doesn't have a formal root. // However, if we create a dummy Profile, and set _profiles as the parent, - // we now have a root. So we'll do just that, and + // we now have a root. So we'll do just that, then copy the inheritance graph + // from the dummyRoot. auto dummyRootSource{ winrt::make_self() }; for (auto profile : _profiles) { @@ -92,16 +105,27 @@ winrt::Microsoft::Terminal::Settings::Model::CascadiaSettings CascadiaSettings:: profileImpl.copy_from(winrt::get_self(profile)); dummyRootSource->InsertParent(profileImpl); } + auto dummyRootClone{ winrt::make_self() }; - Profile::CloneInheritanceGraph(dummyRootSource, dummyRootClone); + std::unordered_map> visited{}; + + if (_userDefaultProfileSettings) + { + // profile.defaults must be saved to CascadiaSettings + // So let's do that manually first, and add that to visited + cloneSettings->_userDefaultProfileSettings = Profile::CopySettings(_userDefaultProfileSettings); + visited[_userDefaultProfileSettings.get()] = cloneSettings->_userDefaultProfileSettings; + } + + Profile::CloneInheritanceGraph(dummyRootSource, dummyRootClone, visited); + // All of the parents of the dummy root clone are _profiles. + // Export the parents and add them to the settings clone. auto cloneParents{ dummyRootClone->ExportParents() }; for (auto profile : cloneParents) { - settings->_profiles.Append(*profile); + cloneSettings->_profiles.Append(*profile); } - - return *settings; } // Method Description: diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.h b/src/cascadia/TerminalSettingsModel/CascadiaSettings.h index 3f38752a04c..d61cf3ad54f 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.h +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettings.h @@ -113,6 +113,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation bool _PrependSchemaDirective(); bool _AppendDynamicProfilesToUserSettings(); std::string _ApplyFirstRunChangesToSettingsTemplate(std::string_view settingsTemplate) const; + void _CopyProfileInheritanceTree(CascadiaSettings* cloneSettings) const; void _ApplyDefaultsFromUserSettings(); diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp index 74245b3261c..8b7da717678 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp @@ -635,10 +635,22 @@ void CascadiaSettings::_LayerOrCreateProfile(const Json::Value& profileJson) auto parentProj{ _profiles.GetAt(*profileIndex) }; auto parent{ winrt::get_self(parentProj) }; - // We don't actually need to CreateChild() here. - // When we loaded Profile.Defaults, we created an empty child already. - // So this just populates the empty child - parent->LayerJson(profileJson); + if (_userDefaultProfileSettings) + { + // We don't actually need to CreateChild() here. + // When we loaded Profile.Defaults, we created an empty child already. + // So this just populates the empty child + parent->LayerJson(profileJson); + } + else + { + // otherwise, add a new inheritance layer + auto childImpl{ parent->CreateChild() }; + childImpl->LayerJson(profileJson); + + // replace parent in _profiles with child + _profiles.SetAt(*profileIndex, *childImpl); + } } else { diff --git a/src/cascadia/TerminalSettingsModel/Profile.cpp b/src/cascadia/TerminalSettingsModel/Profile.cpp index 42c4797960a..05b85f42d9e 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.cpp +++ b/src/cascadia/TerminalSettingsModel/Profile.cpp @@ -70,7 +70,7 @@ Profile::Profile(guid guid) : { } -winrt::com_ptr Profile::_CopyMembers(winrt::com_ptr source) +winrt::com_ptr Profile::CopySettings(winrt::com_ptr source) { // copy the private members auto profile{ winrt::make_self() }; @@ -128,7 +128,7 @@ winrt::com_ptr Profile::_CopyMembers(winrt::com_ptr source) // - visited - a map of which Profiles have been visited, and, if so, a reference to the Profile's clone // Return Value: // - a clone in both inheritance structure and Profile values of sourceGraph -winrt::com_ptr Profile::CloneInheritanceGraph(winrt::com_ptr sourceGraph, winrt::com_ptr cloneGraph, std::unordered_map> visited) +winrt::com_ptr Profile::CloneInheritanceGraph(winrt::com_ptr sourceGraph, winrt::com_ptr cloneGraph, std::unordered_map>& visited) { // If this is an unexplored Profile // and we have parents... @@ -139,7 +139,7 @@ winrt::com_ptr Profile::CloneInheritanceGraph(winrt::com_ptr s { // If we visited this Profile already... auto kv{ visited.find(sourceParent.get()) }; - if (visited.find(sourceParent.get()) != visited.end()) + if (kv != visited.end()) { // add this Profile's clone as a parent cloneGraph->InsertParent(kv->second); @@ -148,7 +148,7 @@ winrt::com_ptr Profile::CloneInheritanceGraph(winrt::com_ptr s { // We have not visited this Profile yet, // copy contents of sourceParent to clone - winrt::com_ptr clone{ _CopyMembers(sourceParent) }; + winrt::com_ptr clone{ CopySettings(sourceParent) }; // add the new copy to the cloneGraph cloneGraph->InsertParent(clone); @@ -391,12 +391,13 @@ winrt::hstring Profile::ExpandedBackgroundImagePath() const winrt::hstring Profile::EvaluatedStartingDirectory() const { - if (_StartingDirectory) + auto path{ StartingDirectory() }; + if (!path.empty()) { - return winrt::hstring{ Profile::EvaluateStartingDirectory(_StartingDirectory->c_str()) }; + return winrt::hstring{ Profile::EvaluateStartingDirectory(path.c_str()) }; } // treated as "inherit directory from parent process" - return L""; + return path; } // Method Description: diff --git a/src/cascadia/TerminalSettingsModel/Profile.h b/src/cascadia/TerminalSettingsModel/Profile.h index d9d2cb0d27c..60dc83ad485 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.h +++ b/src/cascadia/TerminalSettingsModel/Profile.h @@ -23,15 +23,17 @@ Author(s): #include // fwdecl unittest classes -namespace TerminalAppLocalTests +namespace SettingsModelLocalTests { - class SettingsTests; + class DeserializationTests; class ProfileTests; + class ColorSchemeTests; + class KeyBindingsTests; }; namespace TerminalAppUnitTests { - class JsonTests; class DynamicProfileTests; + class JsonTests; }; // GUID used for generating GUIDs at runtime, for profiles that did not have a @@ -45,7 +47,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation public: Profile(); Profile(guid guid); - static com_ptr CloneInheritanceGraph(com_ptr oldProfile, com_ptr newProfile, std::unordered_map> visited = {}); + static com_ptr CloneInheritanceGraph(com_ptr oldProfile, com_ptr newProfile, std::unordered_map>& visited); + static com_ptr CopySettings(com_ptr source); Json::Value GenerateStub() const; static com_ptr FromJson(const Json::Value& json); @@ -147,12 +150,12 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation static guid _GenerateGuidForProfile(const hstring& name, const hstring& source) noexcept; - static com_ptr _CopyMembers(com_ptr source); - - friend class TerminalAppLocalTests::SettingsTests; - friend class TerminalAppLocalTests::ProfileTests; - friend class TerminalAppUnitTests::JsonTests; + friend class SettingsModelLocalTests::DeserializationTests; + friend class SettingsModelLocalTests::ProfileTests; + friend class SettingsModelLocalTests::ColorSchemeTests; + friend class SettingsModelLocalTests::KeyBindingsTests; friend class TerminalAppUnitTests::DynamicProfileTests; + friend class TerminalAppUnitTests::JsonTests; }; } From 6d3498030139bcc52df3a9b26d1b15fadd90a687 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Wed, 21 Oct 2020 13:59:41 -0700 Subject: [PATCH 14/29] fix remaining PR comments --- .../TerminalSettingsModel/IInheritable.h | 129 ++++++++++-------- .../TerminalSettingsModel/Profile.cpp | 12 +- src/inc/til/coalesce.h | 48 ------- 3 files changed, 79 insertions(+), 110 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/IInheritable.h b/src/cascadia/TerminalSettingsModel/IInheritable.h index 4f61564b92d..940f54051e1 100644 --- a/src/cascadia/TerminalSettingsModel/IInheritable.h +++ b/src/cascadia/TerminalSettingsModel/IInheritable.h @@ -71,13 +71,13 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation template struct NullableSetting { - winrt::Windows::Foundation::IReference setting{ nullptr }; + std::optional setting{ std::nullopt }; bool set{ false }; }; } // Use this macro to quickly implement both getters and the setter for an -// inheritable and observable setting property. This is similar to the GETSET_PROPERTY macro, except... +// inheritable setting property. This is similar to the GETSET_PROPERTY macro, except... // - Has(): checks if the user explicitly set a value for this setting // - Getter(): return the resolved value // - Setter(): set the value directly @@ -137,64 +137,73 @@ private: \ }; // This macro is similar to the one above, but is reserved for optional settings -// like Profile.StartingDirectory and Profile.Foreground (where null is interpreted +// like Profile.Foreground (where null is interpreted // as an acceptable value, rather than "inherit") // "type" is exposed as an IReference -#define GETSET_NULLABLE_SETTING(type, name, ...) \ -public: \ - /* Returns true if the user explicitly set the value, false otherwise*/ \ - bool Has##name() const \ - { \ - return _##name.set; \ - }; \ - \ - /* Returns the resolved value for this setting */ \ - /* fallback: user set value --> inherited value --> system set value */ \ - winrt::Windows::Foundation::IReference name() const \ - { \ - const auto val{ _get##name##Impl() }; \ - return val.set ? val.setting : winrt::Windows::Foundation::IReference{ __VA_ARGS__ }; \ - }; \ - \ - /* Overwrite the user set value */ \ - /* Dispatch event if value changed */ \ - void name(const winrt::Windows::Foundation::IReference& value) \ - { \ - if (!Has##name() /*value was not set*/ \ - || _##name.setting != value) /*set value is different*/ \ - { \ - _##name.setting = value; \ - _##name.set = true; \ - } \ - }; \ - \ - /* Clear the user set value */ \ - /* Dispatch event if value changed */ \ - void Clear##name() \ - { \ - _##name.set = false; \ - }; \ - \ -private: \ - NullableSetting _##name{}; \ - NullableSetting _get##name##Impl() const \ - { \ - /*return user set value*/ \ - if (Has##name()) \ - { \ - return _##name; \ - } \ - \ - /*user set value was not set*/ \ - /*iterate through parents to find a value*/ \ - for (auto parent : _parents) \ - { \ - auto val{ parent->_get##name##Impl() }; \ - if (val.set) \ - { \ - return val; \ - } \ - } \ - /*no value was found*/ \ - return { nullptr, false }; \ +#define GETSET_NULLABLE_SETTING(type, name, ...) \ +public: \ + /* Returns true if the user explicitly set the value, false otherwise*/ \ + bool Has##name() const \ + { \ + return _##name.set; \ + }; \ + \ + /* Returns the resolved value for this setting */ \ + /* fallback: user set value --> inherited value --> system set value */ \ + winrt::Windows::Foundation::IReference name() const \ + { \ + const auto val{ _get##name##Impl() }; \ + if (val.set) \ + { \ + if (val.setting) \ + { \ + return *val.setting; \ + } \ + return nullptr; \ + } \ + return winrt::Windows::Foundation::IReference{ __VA_ARGS__ }; \ + }; \ + \ + /* Overwrite the user set value */ \ + void name(const winrt::Windows::Foundation::IReference& value) \ + { \ + if (value) /*set value is different*/ \ + { \ + _##name.setting = value.Value(); \ + } \ + else \ + { \ + _##name.setting = std::nullopt; \ + } \ + _##name.set = true; \ + }; \ + \ + /* Clear the user set value */ \ + void Clear##name() \ + { \ + _##name.set = false; \ + }; \ + \ +private: \ + NullableSetting _##name{}; \ + NullableSetting _get##name##Impl() const \ + { \ + /*return user set value*/ \ + if (Has##name()) \ + { \ + return _##name; \ + } \ + \ + /*user set value was not set*/ \ + /*iterate through parents to find a value*/ \ + for (auto parent : _parents) \ + { \ + auto val{ parent->_get##name##Impl() }; \ + if (val.set) \ + { \ + return val; \ + } \ + } \ + /*no value was found*/ \ + return { std::nullopt, false }; \ }; diff --git a/src/cascadia/TerminalSettingsModel/Profile.cpp b/src/cascadia/TerminalSettingsModel/Profile.cpp index 05b85f42d9e..d94e70eb02b 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.cpp +++ b/src/cascadia/TerminalSettingsModel/Profile.cpp @@ -526,10 +526,14 @@ const HorizontalAlignment Profile::BackgroundImageHorizontalAlignment() const no void Profile::BackgroundImageHorizontalAlignment(const HorizontalAlignment& value) noexcept { - if (!HasBackgroundImageAlignment() || std::get(*_BackgroundImageAlignment) != value) + if (HasBackgroundImageAlignment()) { std::get(*_BackgroundImageAlignment) = value; } + else + { + _BackgroundImageAlignment = { value, VerticalAlignment::Center }; + } } const VerticalAlignment Profile::BackgroundImageVerticalAlignment() const noexcept @@ -540,10 +544,14 @@ const VerticalAlignment Profile::BackgroundImageVerticalAlignment() const noexce void Profile::BackgroundImageVerticalAlignment(const VerticalAlignment& value) noexcept { - if (!HasBackgroundImageAlignment() || std::get(*_BackgroundImageAlignment) != value) + if (HasBackgroundImageAlignment()) { std::get(*_BackgroundImageAlignment) = value; } + else + { + _BackgroundImageAlignment = { HorizontalAlignment::Center, value }; + } } #pragma endregion diff --git a/src/inc/til/coalesce.h b/src/inc/til/coalesce.h index aacbbf52751..f14564dd30c 100644 --- a/src/inc/til/coalesce.h +++ b/src/inc/til/coalesce.h @@ -3,20 +3,8 @@ #pragma once -namespace winrt -{ - // If we don't use winrt, nobody will include the ConversionTraits for winrt stuff. - // If nobody includes it, these forward declarations will suffice. - namespace Windows::Foundation - { - template - struct IReference; - } -} - namespace til { -#pragma region coalesce_value // Method Description: // - Base case provided to handle the last argument to coalesce_value() template @@ -34,15 +22,6 @@ namespace til return T{}; } - // Method Description: - // - Base case provided to throw an assertion if you call coalesce_value(opt, opt, opt) - template - T coalesce_value(const winrt::Windows::Foundation::IReference& base) - { - static_assert(false, "coalesce_value must be passed a base non-optional value to be used if all optionals are empty"); - return T{}; - } - // Method Description: // - Returns the value from the first populated optional, or a base value if none were populated. template @@ -55,16 +34,6 @@ namespace til return t1.value_or(coalesce_value(std::forward(t2)...)); } - // Method Description: - // - Returns the value from the first populated IReference, or a base value if none were populated. - template - T coalesce_value(const winrt::Windows::Foundation::IReference& t1, Ts&&... t2) - { - return t1 ? t1.Value() : coalesce_value(std::forward(t2)...); - } -#pragma endregion - -#pragma region coalesce // Method Description: // - Base case provided to handle the last argument to coalesce_value() template @@ -73,14 +42,6 @@ namespace til return base; } - // Method Description: - // - Base case provided to handle the last argument to coalesce_value() - template - winrt::Windows::Foundation::IReference coalesce(const winrt::Windows::Foundation::IReference& base) - { - return base; - } - // Method Description: // - Base case provided to handle the last argument to coalesce_value(..., nullopt) template @@ -96,13 +57,4 @@ namespace til { return t1.has_value() ? t1 : coalesce(std::forward(t2)...); } - - // Method Description: - // - Returns the value from the first populated IReference, or the last one (if none of the previous had a value) - template - winrt::Windows::Foundation::IReference coalesce(const winrt::Windows::Foundation::IReference& t1, Ts&&... t2) - { - return t1 ? t1 : coalesce(std::forward(t2)...); - } -#pragma endregion } From 6e1849bac9b95a777b1db3d1f46499d6e92bfb31 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Wed, 21 Oct 2020 14:21:18 -0700 Subject: [PATCH 15/29] fix startingDirectory --- src/cascadia/TerminalSettingsModel/Profile.cpp | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/cascadia/TerminalSettingsModel/Profile.cpp b/src/cascadia/TerminalSettingsModel/Profile.cpp index d94e70eb02b..e8ed277394f 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.cpp +++ b/src/cascadia/TerminalSettingsModel/Profile.cpp @@ -338,7 +338,15 @@ void Profile::LayerJson(const Json::Value& json) JsonUtils::GetValueForKey(json, PaddingKey, _Padding, JsonUtils::PermissiveStringConverter{}); JsonUtils::GetValueForKey(json, ScrollbarStateKey, _ScrollState); - JsonUtils::GetValueForKey(json, StartingDirectoryKey, _StartingDirectory); + + // StartingDirectory is "nullable". But we represent a null starting directory as the empty string + // When null is set in the JSON, we empty initialize startDir (empty string), and set StartingDirectory to that + // Without this, we're accidentally setting StartingDirectory to nullopt instead. + hstring startDir; + if (JsonUtils::GetValueForKey(json, StartingDirectoryKey, startDir)) + { + _StartingDirectory = startDir; + } JsonUtils::GetValueForKey(json, IconKey, _Icon); JsonUtils::GetValueForKey(json, BackgroundImageKey, _BackgroundImagePath); From 80e234b76111f72ed2250200e0d4fe044a3113b2 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Wed, 21 Oct 2020 14:59:33 -0700 Subject: [PATCH 16/29] polish --- .../TerminalSettingsModel/CascadiaSettings.cpp | 10 +++++----- src/cascadia/TerminalSettingsModel/CascadiaSettings.h | 2 +- src/cascadia/TerminalSettingsModel/IInheritable.h | 11 ++++++++++- src/cascadia/TerminalSettingsModel/Profile.cpp | 5 +---- 4 files changed, 17 insertions(+), 11 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp index 5ead0afb9bd..bbf53ad2732 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp @@ -81,7 +81,7 @@ winrt::Microsoft::Terminal::Settings::Model::CascadiaSettings CascadiaSettings:: settings->_userSettings = _userSettings; settings->_defaultSettings = _defaultSettings; - _CopyProfileInheritanceTree(settings.get()); + _CopyProfileInheritanceTree(settings); return *settings; } @@ -92,14 +92,14 @@ winrt::Microsoft::Terminal::Settings::Model::CascadiaSettings CascadiaSettings:: // - cloneSettings: the CascadiaSettings we're copying the inheritance tree to // Return Value: // - -void CascadiaSettings::_CopyProfileInheritanceTree(CascadiaSettings* cloneSettings) const +void CascadiaSettings::_CopyProfileInheritanceTree(winrt::com_ptr& cloneSettings) const { // Our profiles inheritance graph doesn't have a formal root. // However, if we create a dummy Profile, and set _profiles as the parent, // we now have a root. So we'll do just that, then copy the inheritance graph // from the dummyRoot. auto dummyRootSource{ winrt::make_self() }; - for (auto profile : _profiles) + for (const auto& profile : _profiles) { winrt::com_ptr profileImpl; profileImpl.copy_from(winrt::get_self(profile)); @@ -121,8 +121,8 @@ void CascadiaSettings::_CopyProfileInheritanceTree(CascadiaSettings* cloneSettin // All of the parents of the dummy root clone are _profiles. // Export the parents and add them to the settings clone. - auto cloneParents{ dummyRootClone->ExportParents() }; - for (auto profile : cloneParents) + const auto cloneParents{ dummyRootClone->ExportParents() }; + for (const auto& profile : cloneParents) { cloneSettings->_profiles.Append(*profile); } diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.h b/src/cascadia/TerminalSettingsModel/CascadiaSettings.h index d61cf3ad54f..05910e73669 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.h +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettings.h @@ -113,7 +113,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation bool _PrependSchemaDirective(); bool _AppendDynamicProfilesToUserSettings(); std::string _ApplyFirstRunChangesToSettingsTemplate(std::string_view settingsTemplate) const; - void _CopyProfileInheritanceTree(CascadiaSettings* cloneSettings) const; + void _CopyProfileInheritanceTree(com_ptr& cloneSettings) const; void _ApplyDefaultsFromUserSettings(); diff --git a/src/cascadia/TerminalSettingsModel/IInheritable.h b/src/cascadia/TerminalSettingsModel/IInheritable.h index 940f54051e1..76cc061fb62 100644 --- a/src/cascadia/TerminalSettingsModel/IInheritable.h +++ b/src/cascadia/TerminalSettingsModel/IInheritable.h @@ -30,6 +30,9 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation { auto child{ winrt::make_self() }; + // set "this" as the parent. + // However, "this" is an IInheritable, so we need to cast it as T (the impl winrt type) + // to pass ownership over to the com_ptr. com_ptr parent; winrt::copy_from_abi(parent, const_cast(static_cast(this))); child->InsertParent(parent); @@ -49,7 +52,13 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation _parents.insert(pos, parent); } - std::vector> ExportParents() + // Method Description: + // - Exports list of parents as an immutable view. + // Arguments: + // - + // Return Value: + // - + const std::vector> ExportParents() { return _parents; } diff --git a/src/cascadia/TerminalSettingsModel/Profile.cpp b/src/cascadia/TerminalSettingsModel/Profile.cpp index e8ed277394f..d825ecca1a2 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.cpp +++ b/src/cascadia/TerminalSettingsModel/Profile.cpp @@ -135,7 +135,7 @@ winrt::com_ptr Profile::CloneInheritanceGraph(winrt::com_ptr s if (visited.find(sourceGraph.get()) == visited.end() && !sourceGraph->_parents.empty()) { // iterate through all of our parents to copy them - for (auto sourceParent : sourceGraph->_parents) + for (const auto& sourceParent : sourceGraph->_parents) { // If we visited this Profile already... auto kv{ visited.find(sourceParent.get()) }; @@ -160,9 +160,6 @@ winrt::com_ptr Profile::CloneInheritanceGraph(winrt::com_ptr s // save it to the map in case somebody else references it visited[sourceParent.get()] = clone; } - - // if clone is empty, this is the end of the inheritance line - // otherwise, it is populated with the contents of its parent } } From 7150e52a14f4c613f30a93c10cce2f52c09ec636 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Wed, 21 Oct 2020 16:44:10 -0700 Subject: [PATCH 17/29] move guid generation to getter --- .../LocalTests_SettingsModel/ProfileTests.cpp | 2 +- .../CascadiaSettingsSerialization.cpp | 14 -------- .../TerminalSettingsModel/Profile.cpp | 35 +++++++------------ src/cascadia/TerminalSettingsModel/Profile.h | 2 +- 4 files changed, 15 insertions(+), 38 deletions(-) diff --git a/src/cascadia/LocalTests_SettingsModel/ProfileTests.cpp b/src/cascadia/LocalTests_SettingsModel/ProfileTests.cpp index 5ad42162a60..aa203759537 100644 --- a/src/cascadia/LocalTests_SettingsModel/ProfileTests.cpp +++ b/src/cascadia/LocalTests_SettingsModel/ProfileTests.cpp @@ -87,7 +87,7 @@ namespace SettingsModelLocalTests // A profile _can_ be layered with itself, though what's the point? VERIFY_IS_FALSE(profile3->ShouldBeLayered(profile1Json)); VERIFY_IS_FALSE(profile3->ShouldBeLayered(profile2Json)); - VERIFY_IS_FALSE(profile3->ShouldBeLayered(profile3Json)); + VERIFY_IS_TRUE(profile3->ShouldBeLayered(profile3Json)); } void ProfileTests::LayerProfileProperties() diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp index 8b7da717678..57658dd827d 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp @@ -511,20 +511,6 @@ bool CascadiaSettings::_AppendDynamicProfilesToUserSettings() for (const auto& profile : _profiles) { - if (profile.Guid() == winrt::guid{}) - { - // If the profile doesn't have a guid, it's a name-only profile. - // During validation, we'll generate a GUID for the profile, but - // validation occurs after this. We should ignore these types of - // profiles. - // If a dynamic profile was generated _without_ a GUID, we also - // don't want it serialized here. The first check in - // Profile::ShouldBeLayered checks that the profile has a guid. For a - // dynamic profile without a GUID, that'll _never_ be true, so it - // would be impossible to be layered. - continue; - } - // Skip profiles that are in the user settings or the default settings. if (isInJsonObj(profile, _userSettings) || isInJsonObj(profile, _defaultSettings)) { diff --git a/src/cascadia/TerminalSettingsModel/Profile.cpp b/src/cascadia/TerminalSettingsModel/Profile.cpp index d825ecca1a2..ffac8903804 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.cpp +++ b/src/cascadia/TerminalSettingsModel/Profile.cpp @@ -183,10 +183,7 @@ Json::Value Profile::GenerateStub() const Json::Value stub; ///// Profile-specific settings ///// - if (_Guid.has_value()) - { - stub[JsonKey(GuidKey)] = winrt::to_string(Utils::GuidToString(*_Guid)); - } + stub[JsonKey(GuidKey)] = winrt::to_string(Utils::GuidToString(Guid())); stub[JsonKey(NameKey)] = winrt::to_string(Name()); @@ -224,40 +221,34 @@ winrt::com_ptr>(json, GuidKey) }) + const auto otherGuid{ JsonUtils::GetValueForKey>(json, GuidKey) }; + const auto otherSource{ JsonUtils::GetValueForKey>(json, SourceKey) }; + if (otherGuid) { - if (otherGuid.value() != myGuid) + if (otherGuid.value() != Guid()) { return false; } } else { - // If the other json object didn't have a GUID, we definitely don't want - // to layer. We technically might have the same name, and would - // auto-generate the same guid, but they should be treated as different - // profiles. - return false; + // If the other json object didn't have a GUID, + // check if we auto-generate the same guid using the name and source. + const auto otherName{ JsonUtils::GetValueForKey>(json, NameKey) }; + if (Guid() != _GenerateGuidForProfile(otherName ? *otherName : L"Default", otherSource ? *otherSource : L"")) + { + return false; + } } - std::optional otherSource; - bool otherHadSource = JsonUtils::GetValueForKey(json, SourceKey, otherSource); - // For profiles with a `source`, also check the `source` property. bool sourceMatches = false; const auto mySource{ Source() }; if (!mySource.empty()) { - if (otherHadSource) + if (otherSource.has_value()) { // If we have a source and the other has a source, compare them! sourceMatches = *otherSource == mySource; diff --git a/src/cascadia/TerminalSettingsModel/Profile.h b/src/cascadia/TerminalSettingsModel/Profile.h index 60dc83ad485..4476df1725b 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.h +++ b/src/cascadia/TerminalSettingsModel/Profile.h @@ -73,7 +73,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation const Windows::UI::Xaml::VerticalAlignment BackgroundImageVerticalAlignment() const noexcept; void BackgroundImageVerticalAlignment(const Windows::UI::Xaml::VerticalAlignment& value) noexcept; - GETSET_SETTING(guid, Guid); + GETSET_SETTING(guid, Guid, _GenerateGuidForProfile(Name(), Source())); GETSET_SETTING(hstring, Name, L"Default"); GETSET_SETTING(hstring, Source); GETSET_SETTING(bool, Hidden, false); From 0a806fdf92516207a626e4900b33396347f3f4aa Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Wed, 21 Oct 2020 17:13:14 -0700 Subject: [PATCH 18/29] address more of Dustin's comments --- src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp | 4 ++-- src/cascadia/TerminalSettingsModel/IInheritable.h | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp index bbf53ad2732..4cd053f86bc 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp @@ -120,8 +120,8 @@ void CascadiaSettings::_CopyProfileInheritanceTree(winrt::com_ptrExportParents() }; + // Get the parents and add them to the settings clone. + const auto cloneParents{ dummyRootClone->Parents() }; for (const auto& profile : cloneParents) { cloneSettings->_profiles.Append(*profile); diff --git a/src/cascadia/TerminalSettingsModel/IInheritable.h b/src/cascadia/TerminalSettingsModel/IInheritable.h index 76cc061fb62..c086e3dfcfa 100644 --- a/src/cascadia/TerminalSettingsModel/IInheritable.h +++ b/src/cascadia/TerminalSettingsModel/IInheritable.h @@ -53,12 +53,12 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation } // Method Description: - // - Exports list of parents as an immutable view. + // - Exports list of parents as an view. // Arguments: // - // Return Value: // - - const std::vector> ExportParents() + const std::vector>& Parents() { return _parents; } From 401efb891784acaaedef8955f9ee31138589da12 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Wed, 21 Oct 2020 17:22:32 -0700 Subject: [PATCH 19/29] adios comment --- src/cascadia/TerminalSettingsModel/IInheritable.h | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/IInheritable.h b/src/cascadia/TerminalSettingsModel/IInheritable.h index c086e3dfcfa..c8c1ee1e4fd 100644 --- a/src/cascadia/TerminalSettingsModel/IInheritable.h +++ b/src/cascadia/TerminalSettingsModel/IInheritable.h @@ -52,12 +52,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation _parents.insert(pos, parent); } - // Method Description: - // - Exports list of parents as an view. - // Arguments: - // - - // Return Value: - // - const std::vector>& Parents() { return _parents; From 12926338452ab2699c58a6730e884a0f17fb3823 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Wed, 21 Oct 2020 17:31:20 -0700 Subject: [PATCH 20/29] fix failing tests --- .../LocalTests_SettingsModel/DeserializationTests.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp b/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp index 1b23a5b4741..ff4a27e0ca4 100644 --- a/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp +++ b/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp @@ -832,7 +832,7 @@ namespace SettingsModelLocalTests const auto serialized0Profile = profile0->GenerateStub(); const auto profile1 = implementation::Profile::FromJson(serialized0Profile); VERIFY_IS_FALSE(profile0->HasGuid()); - VERIFY_IS_FALSE(profile1->HasGuid()); + VERIFY_IS_TRUE(profile1->HasGuid()); auto settings = winrt::make_self(); settings->_profiles.Append(*profile1); @@ -1356,7 +1356,7 @@ namespace SettingsModelLocalTests const winrt::guid guid1{ ::Microsoft::Console::Utils::GuidFromString(L"{6239a42c-6666-49a3-80bd-e8fdd045185c}") }; const winrt::guid guid2{ ::Microsoft::Console::Utils::GuidFromString(L"{2C4DE342-38B7-51CF-B940-2309A097F518}") }; const winrt::guid fakeGuid{ ::Microsoft::Console::Utils::GuidFromString(L"{FFFFFFFF-FFFF-FFFF-FFFF-FFFFFFFFFFFF}") }; - const winrt::guid autogeneratedGuid{}; + const winrt::guid autogeneratedGuid{ implementation::Profile::_GenerateGuidForProfile(name3, L"") }; const std::optional badGuid{}; VerifyParseSucceeded(settings0String); From 5c594f39a8b460af8742313e9b67bb642998c3c1 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Thu, 22 Oct 2020 15:46:22 -0700 Subject: [PATCH 21/29] fix tests --- src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp b/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp index ff4a27e0ca4..aaaa3fa9cef 100644 --- a/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp +++ b/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp @@ -2254,6 +2254,7 @@ namespace SettingsModelLocalTests settings->_ParseJsonString(settings1Json, false); settings->LayerJson(settings->_userSettings); settings->_ValidateSettings(); + commands = settings->_globals->Commands(); _logCommandNames(commands); VERIFY_ARE_EQUAL(0u, settings->_warnings.Size()); VERIFY_ARE_EQUAL(0u, commands.Size()); @@ -2348,6 +2349,7 @@ namespace SettingsModelLocalTests settings->_ParseJsonString(settings1Json, false); settings->LayerJson(settings->_userSettings); settings->_ValidateSettings(); + commands = settings->_globals->Commands(); _logCommandNames(commands); VERIFY_ARE_EQUAL(0u, settings->_warnings.Size()); VERIFY_ARE_EQUAL(1u, commands.Size()); From 01e2c49d7e7db37b3958be79cf305eaa676459f9 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Fri, 23 Oct 2020 13:53:00 -0700 Subject: [PATCH 22/29] address Zadji PR comments --- .../GlobalAppSettings.cpp | 20 ++++++------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp index 4754de16ec7..24d75d8b708 100644 --- a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp +++ b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp @@ -75,19 +75,10 @@ void GlobalAppSettings::_FinalizeInheritance() FAIL_FAST_IF(_parents.size() > 1); for (auto parent : _parents) { - _keymap = parent->_keymap->Copy(); - std::copy(parent->_keybindingsWarnings.begin(), parent->_keybindingsWarnings.end(), std::back_inserter(_keybindingsWarnings)); - for (auto kv : parent->_colorSchemes) - { - const auto schemeImpl{ winrt::get_self(kv.Value()) }; - _colorSchemes.Insert(kv.Key(), *schemeImpl->Copy()); - } - - for (auto kv : parent->_commands) - { - const auto commandImpl{ winrt::get_self(kv.Value()) }; - _commands.Insert(kv.Key(), *commandImpl->Copy()); - } + _keymap = std::move(parent->_keymap); + _keybindingsWarnings = std::move(parent->_keybindingsWarnings); + _colorSchemes = std::move(parent->_colorSchemes); + _commands = std::move(parent->_commands); } } @@ -239,7 +230,8 @@ void GlobalAppSettings::LayerJson(const Json::Value& json) // _validDefaultProfile keeps track of whether we've verified that DefaultProfile points to something // CascadiaSettings::_ResolveDefaultProfile performs a validation and updates DefaultProfile() with the // resolved value, then making it valid. - _validDefaultProfile = !JsonUtils::GetValueForKey(json, DefaultProfileKey, _UnparsedDefaultProfile); + _validDefaultProfile = false; + JsonUtils::GetValueForKey(json, DefaultProfileKey, _UnparsedDefaultProfile); JsonUtils::GetValueForKey(json, AlwaysShowTabsKey, _AlwaysShowTabs); From e0e39438c6947b7452bdadad54cc78fd2a04e310 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Fri, 23 Oct 2020 14:02:00 -0700 Subject: [PATCH 23/29] make ConnectionType Inheritable --- src/cascadia/TerminalSettingsModel/Profile.cpp | 15 --------------- src/cascadia/TerminalSettingsModel/Profile.h | 6 +----- 2 files changed, 1 insertion(+), 20 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/Profile.cpp b/src/cascadia/TerminalSettingsModel/Profile.cpp index ffac8903804..8c1295e9ed9 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.cpp +++ b/src/cascadia/TerminalSettingsModel/Profile.cpp @@ -550,18 +550,3 @@ void Profile::BackgroundImageVerticalAlignment(const VerticalAlignment& value) n } } #pragma endregion - -bool Profile::HasConnectionType() const noexcept -{ - return _ConnectionType.has_value(); -} - -winrt::guid Profile::ConnectionType() const noexcept -{ - return *_ConnectionType; -} - -void Profile::ConnectionType(const winrt::guid& conType) noexcept -{ - _ConnectionType = conType; -} diff --git a/src/cascadia/TerminalSettingsModel/Profile.h b/src/cascadia/TerminalSettingsModel/Profile.h index 4476df1725b..4a42c4108bf 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.h +++ b/src/cascadia/TerminalSettingsModel/Profile.h @@ -61,10 +61,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation void GenerateGuidIfNecessary() noexcept; static guid GetGuidOrGenerateForJson(const Json::Value& json) noexcept; - bool HasConnectionType() const noexcept; - winrt::guid ConnectionType() const noexcept; - void ConnectionType(const winrt::guid& conType) noexcept; - // BackgroundImageAlignment is 1 setting saved as 2 separate values bool HasBackgroundImageAlignment() const noexcept; void ClearBackgroundImageAlignment() noexcept; @@ -77,6 +73,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation GETSET_SETTING(hstring, Name, L"Default"); GETSET_SETTING(hstring, Source); GETSET_SETTING(bool, Hidden, false); + GETSET_SETTING(guid, ConnectionType); GETSET_SETTING(hstring, Icon); @@ -123,7 +120,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation GETSET_SETTING(Model::BellStyle, BellStyle, BellStyle::Audible); private: - std::optional _ConnectionType{ std::nullopt }; std::optional> _BackgroundImageAlignment{ std::nullopt }; std::optional> _getBackgroundImageAlignmentImpl() const { From b4ec4daacb71094250ac0a818428b8dcceac5bb4 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Fri, 23 Oct 2020 14:06:39 -0700 Subject: [PATCH 24/29] identify connectiontype-ed profiles --- src/cascadia/TerminalApp/TerminalPage.cpp | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index 2b9be96ef29..a41e67d8b33 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -752,17 +752,10 @@ namespace winrt::TerminalApp::implementation TerminalConnection::ITerminalConnection connection{ nullptr }; - winrt::guid connectionType{}; + winrt::guid connectionType = profile.ConnectionType(); winrt::guid sessionGuid{}; - const auto hasConnectionType = profile.HasConnectionType(); - if (hasConnectionType) - { - connectionType = profile.ConnectionType(); - } - - if (hasConnectionType && - connectionType == TerminalConnection::AzureConnection::ConnectionType() && + if (connectionType == TerminalConnection::AzureConnection::ConnectionType() && TerminalConnection::AzureConnection::IsAzureConnectionAvailable()) { // TODO GH#4661: Replace this with directly using the AzCon when our VT is better From 2716fa027f08505ef84a14b5faf0a520991c90d0 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Mon, 26 Oct 2020 14:09:26 -0700 Subject: [PATCH 25/29] fix defaultProfile --- .../DeserializationTests.cpp | 2 +- .../LocalTests_TerminalApp/SettingsTests.cpp | 1 + .../AzureCloudShellGenerator.cpp | 2 +- .../DefaultProfileUtils.cpp | 7 +++--- .../DefaultProfileUtils.h | 6 +---- .../GlobalAppSettings.cpp | 23 +++++++++++++------ .../PowershellCoreProfileGenerator.cpp | 2 +- .../TerminalSettingsModel/Profile.cpp | 10 ++++---- src/cascadia/TerminalSettingsModel/Profile.h | 5 ++-- .../WslDistroGenerator.cpp | 2 +- 10 files changed, 33 insertions(+), 27 deletions(-) diff --git a/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp b/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp index aaaa3fa9cef..dc6fe07841a 100644 --- a/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp +++ b/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp @@ -1356,7 +1356,7 @@ namespace SettingsModelLocalTests const winrt::guid guid1{ ::Microsoft::Console::Utils::GuidFromString(L"{6239a42c-6666-49a3-80bd-e8fdd045185c}") }; const winrt::guid guid2{ ::Microsoft::Console::Utils::GuidFromString(L"{2C4DE342-38B7-51CF-B940-2309A097F518}") }; const winrt::guid fakeGuid{ ::Microsoft::Console::Utils::GuidFromString(L"{FFFFFFFF-FFFF-FFFF-FFFF-FFFFFFFFFFFF}") }; - const winrt::guid autogeneratedGuid{ implementation::Profile::_GenerateGuidForProfile(name3, L"") }; + const winrt::guid autogeneratedGuid{ implementation::Profile::GenerateGuidForProfile(name3, L"") }; const std::optional badGuid{}; VerifyParseSucceeded(settings0String); diff --git a/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp b/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp index af9cda0776e..c3c225b9f0b 100644 --- a/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp +++ b/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp @@ -499,6 +499,7 @@ namespace TerminalAppLocalTests const std::string settings0String{ R"( { + "defaultProfile": "profile5", "profiles": [ { "name" : "profile0", diff --git a/src/cascadia/TerminalSettingsModel/AzureCloudShellGenerator.cpp b/src/cascadia/TerminalSettingsModel/AzureCloudShellGenerator.cpp index 3435d795591..51298d1dd45 100644 --- a/src/cascadia/TerminalSettingsModel/AzureCloudShellGenerator.cpp +++ b/src/cascadia/TerminalSettingsModel/AzureCloudShellGenerator.cpp @@ -33,7 +33,7 @@ std::vector AzureCloudShellGenerator::GenerateProfiles() if (AzureConnection::IsAzureConnectionAvailable()) { - auto azureCloudShellProfile{ CreateDefaultProfile(L"Azure Cloud Shell") }; + auto azureCloudShellProfile{ CreateDefaultProfile(L"Azure Cloud Shell", GetNamespace()) }; azureCloudShellProfile.Commandline(L"Azure"); azureCloudShellProfile.StartingDirectory(DEFAULT_STARTING_DIRECTORY); azureCloudShellProfile.ColorSchemeName(L"Vintage"); diff --git a/src/cascadia/TerminalSettingsModel/DefaultProfileUtils.cpp b/src/cascadia/TerminalSettingsModel/DefaultProfileUtils.cpp index 32ba251c74e..950c2601ab8 100644 --- a/src/cascadia/TerminalSettingsModel/DefaultProfileUtils.cpp +++ b/src/cascadia/TerminalSettingsModel/DefaultProfileUtils.cpp @@ -13,12 +13,13 @@ static constexpr std::wstring_view PACKAGED_PROFILE_ICON_EXTENSION{ L".png" }; // guid and name. // Arguments: // - name: the name of the new profile. +// - source: the source of the new profile. // Return Value: // - A Profile, ready to be filled in -winrt::Microsoft::Terminal::Settings::Model::Profile CreateDefaultProfile(const std::wstring_view name) +winrt::Microsoft::Terminal::Settings::Model::Profile CreateDefaultProfile(const std::wstring_view name, const std::wstring_view source) { - const winrt::guid profileGuid{ Microsoft::Console::Utils::CreateV5Uuid(TERMINAL_PROFILE_NAMESPACE_GUID, - gsl::as_bytes(gsl::make_span(name))) }; + const winrt::guid profileGuid{ winrt::Microsoft::Terminal::Settings::Model::implementation::Profile::GenerateGuidForProfile(winrt::hstring{ name }, winrt::hstring{ source }) }; + auto newProfile = winrt::make(profileGuid); newProfile.Name(name); diff --git a/src/cascadia/TerminalSettingsModel/DefaultProfileUtils.h b/src/cascadia/TerminalSettingsModel/DefaultProfileUtils.h index 28b6c53cd7e..b4d1f030086 100644 --- a/src/cascadia/TerminalSettingsModel/DefaultProfileUtils.h +++ b/src/cascadia/TerminalSettingsModel/DefaultProfileUtils.h @@ -16,8 +16,4 @@ Author(s): #include "Profile.h" -// {2bde4a90-d05f-401c-9492-e40884ead1d8} -// uuidv5 properties: name format is UTF-16LE bytes -static constexpr GUID TERMINAL_PROFILE_NAMESPACE_GUID = { 0x2bde4a90, 0xd05f, 0x401c, { 0x94, 0x92, 0xe4, 0x8, 0x84, 0xea, 0xd1, 0xd8 } }; - -winrt::Microsoft::Terminal::Settings::Model::Profile CreateDefaultProfile(const std::wstring_view name); +winrt::Microsoft::Terminal::Settings::Model::Profile CreateDefaultProfile(const std::wstring_view name, const std::wstring_view source); diff --git a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp index 24d75d8b708..03bda082c89 100644 --- a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp +++ b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp @@ -113,19 +113,28 @@ winrt::com_ptr GlobalAppSettings::Copy() const globals->_UnparsedDefaultProfile = _UnparsedDefaultProfile; globals->_validDefaultProfile = _validDefaultProfile; globals->_defaultProfile = _defaultProfile; - globals->_keymap = _keymap->Copy(); + if (_keymap) + { + globals->_keymap = _keymap->Copy(); + } std::copy(_keybindingsWarnings.begin(), _keybindingsWarnings.end(), std::back_inserter(globals->_keybindingsWarnings)); - for (auto kv : _colorSchemes) + if (_colorSchemes) { - const auto schemeImpl{ winrt::get_self(kv.Value()) }; - globals->_colorSchemes.Insert(kv.Key(), *schemeImpl->Copy()); + for (auto kv : _colorSchemes) + { + const auto schemeImpl{ winrt::get_self(kv.Value()) }; + globals->_colorSchemes.Insert(kv.Key(), *schemeImpl->Copy()); + } } - for (auto kv : _commands) + if (_commands) { - const auto commandImpl{ winrt::get_self(kv.Value()) }; - globals->_commands.Insert(kv.Key(), *commandImpl->Copy()); + for (auto kv : _commands) + { + const auto commandImpl{ winrt::get_self(kv.Value()) }; + globals->_commands.Insert(kv.Key(), *commandImpl->Copy()); + } } // Globals only ever has 1 parent diff --git a/src/cascadia/TerminalSettingsModel/PowershellCoreProfileGenerator.cpp b/src/cascadia/TerminalSettingsModel/PowershellCoreProfileGenerator.cpp index f882a929056..890b9a6c90f 100644 --- a/src/cascadia/TerminalSettingsModel/PowershellCoreProfileGenerator.cpp +++ b/src/cascadia/TerminalSettingsModel/PowershellCoreProfileGenerator.cpp @@ -308,7 +308,7 @@ std::vector PowershellCoreProfileGenerator::GenerateProfiles() for (const auto& psI : psInstances) { const auto name = psI.Name(); - auto profile{ CreateDefaultProfile(name) }; + auto profile{ CreateDefaultProfile(name, GetNamespace()) }; profile.Commandline(psI.executablePath.wstring()); profile.StartingDirectory(DEFAULT_STARTING_DIRECTORY); profile.ColorSchemeName(L"Campbell"); diff --git a/src/cascadia/TerminalSettingsModel/Profile.cpp b/src/cascadia/TerminalSettingsModel/Profile.cpp index 8c1295e9ed9..80d8de3cc0f 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.cpp +++ b/src/cascadia/TerminalSettingsModel/Profile.cpp @@ -237,7 +237,7 @@ bool Profile::ShouldBeLayered(const Json::Value& json) const // If the other json object didn't have a GUID, // check if we auto-generate the same guid using the name and source. const auto otherName{ JsonUtils::GetValueForKey>(json, NameKey) }; - if (Guid() != _GenerateGuidForProfile(otherName ? *otherName : L"Default", otherSource ? *otherSource : L"")) + if (Guid() != GenerateGuidForProfile(otherName ? *otherName : L"Default", otherSource ? *otherSource : L"")) { return false; } @@ -433,11 +433,11 @@ std::wstring Profile::EvaluateStartingDirectory(const std::wstring& directory) // will _not_ change the profile's GUID. void Profile::GenerateGuidIfNecessary() noexcept { - if (!_Guid.has_value()) + if (!_getGuidImpl().has_value()) { // Always use the name to generate the temporary GUID. That way, across // reloads, we'll generate the same static GUID. - _Guid = Profile::_GenerateGuidForProfile(Name(), Source()); + _Guid = Profile::GenerateGuidForProfile(Name(), Source()); TraceLoggingWrite( g_hSettingsModelProvider, @@ -468,7 +468,7 @@ bool Profile::IsDynamicProfileObject(const Json::Value& json) // - name: The name to generate a unique GUID from // Return Value: // - a uuidv5 GUID generated from the given name. -winrt::guid Profile::_GenerateGuidForProfile(const hstring& name, const hstring& source) noexcept +winrt::guid Profile::GenerateGuidForProfile(const hstring& name, const hstring& source) noexcept { // If we have a _source, then we can from a dynamic profile generator. Use // our source to build the namespace guid, instead of using the default GUID. @@ -500,7 +500,7 @@ winrt::guid Profile::GetGuidOrGenerateForJson(const Json::Value& json) noexcept const auto name{ JsonUtils::GetValueForKey(json, NameKey) }; const auto source{ JsonUtils::GetValueForKey(json, SourceKey) }; - return Profile::_GenerateGuidForProfile(name, source); + return Profile::GenerateGuidForProfile(name, source); } #pragma region BackgroundImageAlignment diff --git a/src/cascadia/TerminalSettingsModel/Profile.h b/src/cascadia/TerminalSettingsModel/Profile.h index 4a42c4108bf..8f19f8e5d5a 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.h +++ b/src/cascadia/TerminalSettingsModel/Profile.h @@ -60,6 +60,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation hstring ExpandedBackgroundImagePath() const; void GenerateGuidIfNecessary() noexcept; static guid GetGuidOrGenerateForJson(const Json::Value& json) noexcept; + static guid GenerateGuidForProfile(const hstring& name, const hstring& source) noexcept; // BackgroundImageAlignment is 1 setting saved as 2 separate values bool HasBackgroundImageAlignment() const noexcept; @@ -69,7 +70,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation const Windows::UI::Xaml::VerticalAlignment BackgroundImageVerticalAlignment() const noexcept; void BackgroundImageVerticalAlignment(const Windows::UI::Xaml::VerticalAlignment& value) noexcept; - GETSET_SETTING(guid, Guid, _GenerateGuidForProfile(Name(), Source())); + GETSET_SETTING(guid, Guid, GenerateGuidForProfile(Name(), Source())); GETSET_SETTING(hstring, Name, L"Default"); GETSET_SETTING(hstring, Source); GETSET_SETTING(bool, Hidden, false); @@ -144,8 +145,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation static std::wstring EvaluateStartingDirectory(const std::wstring& directory); - static guid _GenerateGuidForProfile(const hstring& name, const hstring& source) noexcept; - friend class SettingsModelLocalTests::DeserializationTests; friend class SettingsModelLocalTests::ProfileTests; friend class SettingsModelLocalTests::ColorSchemeTests; diff --git a/src/cascadia/TerminalSettingsModel/WslDistroGenerator.cpp b/src/cascadia/TerminalSettingsModel/WslDistroGenerator.cpp index 3cd4b7b0d95..f9518889058 100644 --- a/src/cascadia/TerminalSettingsModel/WslDistroGenerator.cpp +++ b/src/cascadia/TerminalSettingsModel/WslDistroGenerator.cpp @@ -124,7 +124,7 @@ std::vector WslDistroGenerator::GenerateProfiles() { distName.resize(firstChar); } - auto WSLDistro{ CreateDefaultProfile(distName) }; + auto WSLDistro{ CreateDefaultProfile(distName, GetNamespace()) }; WSLDistro.Commandline(L"wsl.exe -d " + distName); WSLDistro.ColorSchemeName(L"Campbell"); From 36211ba5186efb80332f9a28e3be753404c5e444 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Mon, 26 Oct 2020 14:50:17 -0700 Subject: [PATCH 26/29] add optimization: don't inherit PD if it's empty --- .../DeserializationTests.cpp | 58 +++++++++++++++++++ .../CascadiaSettingsSerialization.cpp | 2 +- 2 files changed, 59 insertions(+), 1 deletion(-) diff --git a/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp b/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp index dc6fe07841a..c45469a6736 100644 --- a/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp +++ b/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp @@ -2523,5 +2523,63 @@ namespace SettingsModelLocalTests // profile.defaults should be different between the two graphs VERIFY_ARE_EQUAL(settings->_userDefaultProfileSettings->HasName(), copyImpl->_userDefaultProfileSettings->HasName()); VERIFY_ARE_NOT_EQUAL(settings->_userDefaultProfileSettings->Name(), copyImpl->_userDefaultProfileSettings->Name()); + + Log::Comment(L"Test empty profiles.defaults"); + const std::string emptyPDJson{ R"( + { + "defaultProfile": "{61c54bbd-1111-5271-96e7-009a87ff44bf}", + "profiles": + { + "defaults": { + }, + "list": [ + { + "guid": "{61c54bbd-2222-5271-96e7-009a87ff44bf}", + "name": "PowerShell" + } + ] + } + })" }; + + const std::string missingPDJson{ R"( + { + "defaultProfile": "{61c54bbd-1111-5271-96e7-009a87ff44bf}", + "profiles": + [ + { + "guid": "{61c54bbd-2222-5271-96e7-009a87ff44bf}", + "name": "PowerShell" + } + ] + })" }; + + auto verifyEmptyPD = [this](const std::string json) { + VerifyParseSucceeded(json); + + auto settings{ winrt::make_self() }; + settings->_ParseJsonString(json, false); + settings->_ApplyDefaultsFromUserSettings(); + settings->LayerJson(settings->_userSettings); + settings->_ValidateSettings(); + + const auto copy{ settings->Copy() }; + const auto copyImpl{ winrt::get_self(copy) }; + + // test optimization: if we don't have profiles.defaults, don't add it to the tree + VERIFY_IS_NULL(settings->_userDefaultProfileSettings); + VERIFY_ARE_EQUAL(settings->_userDefaultProfileSettings, copyImpl->_userDefaultProfileSettings); + + VERIFY_ARE_EQUAL(settings->Profiles().Size(), 1u); + VERIFY_ARE_EQUAL(settings->Profiles().Size(), copyImpl->Profiles().Size()); + + // so we should only have one parent, instead of two + auto srcProfile{ winrt::get_self(settings->Profiles().GetAt(0)) }; + auto copyProfile{ winrt::get_self(copyImpl->Profiles().GetAt(0)) }; + VERIFY_ARE_EQUAL(srcProfile->Parents().size(), 0u); + VERIFY_ARE_EQUAL(srcProfile->Parents().size(), copyProfile->Parents().size()); + }; + + verifyEmptyPD(emptyPDJson); + verifyEmptyPD(missingPDJson); } } diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp index 57658dd827d..4ce8101d339 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp @@ -725,7 +725,7 @@ void CascadiaSettings::_ApplyDefaultsFromUserSettings() auto defaultSettings{ Json::Value::null }; if (const auto profiles{ _userSettings[JsonKey(ProfilesKey)] }) { - if (profiles.isObject()) + if (profiles.isObject() && !profiles[JsonKey(DefaultSettingsKey)].empty()) { defaultSettings = profiles[JsonKey(DefaultSettingsKey)]; } From b55856ef587cf77b8884d67232975fed13781653 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Mon, 26 Oct 2020 15:36:13 -0700 Subject: [PATCH 27/29] dynamic generator guids should have their own namespace --- .../LocalTests_SettingsModel/DeserializationTests.cpp | 2 +- .../TerminalSettingsModel/AzureCloudShellGenerator.cpp | 2 +- .../TerminalSettingsModel/DefaultProfileUtils.cpp | 6 +++--- src/cascadia/TerminalSettingsModel/DefaultProfileUtils.h | 9 ++++++++- .../PowershellCoreProfileGenerator.cpp | 2 +- src/cascadia/TerminalSettingsModel/Profile.cpp | 8 ++++---- src/cascadia/TerminalSettingsModel/Profile.h | 5 +++-- .../TerminalSettingsModel/WslDistroGenerator.cpp | 2 +- 8 files changed, 22 insertions(+), 14 deletions(-) diff --git a/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp b/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp index c45469a6736..a8a9b488806 100644 --- a/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp +++ b/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp @@ -1356,7 +1356,7 @@ namespace SettingsModelLocalTests const winrt::guid guid1{ ::Microsoft::Console::Utils::GuidFromString(L"{6239a42c-6666-49a3-80bd-e8fdd045185c}") }; const winrt::guid guid2{ ::Microsoft::Console::Utils::GuidFromString(L"{2C4DE342-38B7-51CF-B940-2309A097F518}") }; const winrt::guid fakeGuid{ ::Microsoft::Console::Utils::GuidFromString(L"{FFFFFFFF-FFFF-FFFF-FFFF-FFFFFFFFFFFF}") }; - const winrt::guid autogeneratedGuid{ implementation::Profile::GenerateGuidForProfile(name3, L"") }; + const winrt::guid autogeneratedGuid{ implementation::Profile::_GenerateGuidForProfile(name3, L"") }; const std::optional badGuid{}; VerifyParseSucceeded(settings0String); diff --git a/src/cascadia/TerminalSettingsModel/AzureCloudShellGenerator.cpp b/src/cascadia/TerminalSettingsModel/AzureCloudShellGenerator.cpp index 51298d1dd45..3435d795591 100644 --- a/src/cascadia/TerminalSettingsModel/AzureCloudShellGenerator.cpp +++ b/src/cascadia/TerminalSettingsModel/AzureCloudShellGenerator.cpp @@ -33,7 +33,7 @@ std::vector AzureCloudShellGenerator::GenerateProfiles() if (AzureConnection::IsAzureConnectionAvailable()) { - auto azureCloudShellProfile{ CreateDefaultProfile(L"Azure Cloud Shell", GetNamespace()) }; + auto azureCloudShellProfile{ CreateDefaultProfile(L"Azure Cloud Shell") }; azureCloudShellProfile.Commandline(L"Azure"); azureCloudShellProfile.StartingDirectory(DEFAULT_STARTING_DIRECTORY); azureCloudShellProfile.ColorSchemeName(L"Vintage"); diff --git a/src/cascadia/TerminalSettingsModel/DefaultProfileUtils.cpp b/src/cascadia/TerminalSettingsModel/DefaultProfileUtils.cpp index 950c2601ab8..08c2c67cfbd 100644 --- a/src/cascadia/TerminalSettingsModel/DefaultProfileUtils.cpp +++ b/src/cascadia/TerminalSettingsModel/DefaultProfileUtils.cpp @@ -13,12 +13,12 @@ static constexpr std::wstring_view PACKAGED_PROFILE_ICON_EXTENSION{ L".png" }; // guid and name. // Arguments: // - name: the name of the new profile. -// - source: the source of the new profile. // Return Value: // - A Profile, ready to be filled in -winrt::Microsoft::Terminal::Settings::Model::Profile CreateDefaultProfile(const std::wstring_view name, const std::wstring_view source) +winrt::Microsoft::Terminal::Settings::Model::Profile CreateDefaultProfile(const std::wstring_view name) { - const winrt::guid profileGuid{ winrt::Microsoft::Terminal::Settings::Model::implementation::Profile::GenerateGuidForProfile(winrt::hstring{ name }, winrt::hstring{ source }) }; + const winrt::guid profileGuid{ Microsoft::Console::Utils::CreateV5Uuid(TERMINAL_PROFILE_NAMESPACE_GUID, + gsl::as_bytes(gsl::make_span(name))) }; auto newProfile = winrt::make(profileGuid); newProfile.Name(name); diff --git a/src/cascadia/TerminalSettingsModel/DefaultProfileUtils.h b/src/cascadia/TerminalSettingsModel/DefaultProfileUtils.h index b4d1f030086..5a401d1202d 100644 --- a/src/cascadia/TerminalSettingsModel/DefaultProfileUtils.h +++ b/src/cascadia/TerminalSettingsModel/DefaultProfileUtils.h @@ -16,4 +16,11 @@ Author(s): #include "Profile.h" -winrt::Microsoft::Terminal::Settings::Model::Profile CreateDefaultProfile(const std::wstring_view name, const std::wstring_view source); +// !!! LOAD-BEARING +// If you change or delete this GUID, all dynamic profiles +// will become disconnected from user settings. +// {2bde4a90-d05f-401c-9492-e40884ead1d8} +// uuidv5 properties: name format is UTF-16LE bytes +static constexpr GUID TERMINAL_PROFILE_NAMESPACE_GUID = { 0x2bde4a90, 0xd05f, 0x401c, { 0x94, 0x92, 0xe4, 0x8, 0x84, 0xea, 0xd1, 0xd8 } }; + +winrt::Microsoft::Terminal::Settings::Model::Profile CreateDefaultProfile(const std::wstring_view name); diff --git a/src/cascadia/TerminalSettingsModel/PowershellCoreProfileGenerator.cpp b/src/cascadia/TerminalSettingsModel/PowershellCoreProfileGenerator.cpp index 890b9a6c90f..f882a929056 100644 --- a/src/cascadia/TerminalSettingsModel/PowershellCoreProfileGenerator.cpp +++ b/src/cascadia/TerminalSettingsModel/PowershellCoreProfileGenerator.cpp @@ -308,7 +308,7 @@ std::vector PowershellCoreProfileGenerator::GenerateProfiles() for (const auto& psI : psInstances) { const auto name = psI.Name(); - auto profile{ CreateDefaultProfile(name, GetNamespace()) }; + auto profile{ CreateDefaultProfile(name) }; profile.Commandline(psI.executablePath.wstring()); profile.StartingDirectory(DEFAULT_STARTING_DIRECTORY); profile.ColorSchemeName(L"Campbell"); diff --git a/src/cascadia/TerminalSettingsModel/Profile.cpp b/src/cascadia/TerminalSettingsModel/Profile.cpp index 80d8de3cc0f..9fab528a217 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.cpp +++ b/src/cascadia/TerminalSettingsModel/Profile.cpp @@ -237,7 +237,7 @@ bool Profile::ShouldBeLayered(const Json::Value& json) const // If the other json object didn't have a GUID, // check if we auto-generate the same guid using the name and source. const auto otherName{ JsonUtils::GetValueForKey>(json, NameKey) }; - if (Guid() != GenerateGuidForProfile(otherName ? *otherName : L"Default", otherSource ? *otherSource : L"")) + if (Guid() != _GenerateGuidForProfile(otherName ? *otherName : L"Default", otherSource ? *otherSource : L"")) { return false; } @@ -437,7 +437,7 @@ void Profile::GenerateGuidIfNecessary() noexcept { // Always use the name to generate the temporary GUID. That way, across // reloads, we'll generate the same static GUID. - _Guid = Profile::GenerateGuidForProfile(Name(), Source()); + _Guid = Profile::_GenerateGuidForProfile(Name(), Source()); TraceLoggingWrite( g_hSettingsModelProvider, @@ -468,7 +468,7 @@ bool Profile::IsDynamicProfileObject(const Json::Value& json) // - name: The name to generate a unique GUID from // Return Value: // - a uuidv5 GUID generated from the given name. -winrt::guid Profile::GenerateGuidForProfile(const hstring& name, const hstring& source) noexcept +winrt::guid Profile::_GenerateGuidForProfile(const hstring& name, const hstring& source) noexcept { // If we have a _source, then we can from a dynamic profile generator. Use // our source to build the namespace guid, instead of using the default GUID. @@ -500,7 +500,7 @@ winrt::guid Profile::GetGuidOrGenerateForJson(const Json::Value& json) noexcept const auto name{ JsonUtils::GetValueForKey(json, NameKey) }; const auto source{ JsonUtils::GetValueForKey(json, SourceKey) }; - return Profile::GenerateGuidForProfile(name, source); + return Profile::_GenerateGuidForProfile(name, source); } #pragma region BackgroundImageAlignment diff --git a/src/cascadia/TerminalSettingsModel/Profile.h b/src/cascadia/TerminalSettingsModel/Profile.h index 8f19f8e5d5a..4a42c4108bf 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.h +++ b/src/cascadia/TerminalSettingsModel/Profile.h @@ -60,7 +60,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation hstring ExpandedBackgroundImagePath() const; void GenerateGuidIfNecessary() noexcept; static guid GetGuidOrGenerateForJson(const Json::Value& json) noexcept; - static guid GenerateGuidForProfile(const hstring& name, const hstring& source) noexcept; // BackgroundImageAlignment is 1 setting saved as 2 separate values bool HasBackgroundImageAlignment() const noexcept; @@ -70,7 +69,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation const Windows::UI::Xaml::VerticalAlignment BackgroundImageVerticalAlignment() const noexcept; void BackgroundImageVerticalAlignment(const Windows::UI::Xaml::VerticalAlignment& value) noexcept; - GETSET_SETTING(guid, Guid, GenerateGuidForProfile(Name(), Source())); + GETSET_SETTING(guid, Guid, _GenerateGuidForProfile(Name(), Source())); GETSET_SETTING(hstring, Name, L"Default"); GETSET_SETTING(hstring, Source); GETSET_SETTING(bool, Hidden, false); @@ -145,6 +144,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation static std::wstring EvaluateStartingDirectory(const std::wstring& directory); + static guid _GenerateGuidForProfile(const hstring& name, const hstring& source) noexcept; + friend class SettingsModelLocalTests::DeserializationTests; friend class SettingsModelLocalTests::ProfileTests; friend class SettingsModelLocalTests::ColorSchemeTests; diff --git a/src/cascadia/TerminalSettingsModel/WslDistroGenerator.cpp b/src/cascadia/TerminalSettingsModel/WslDistroGenerator.cpp index f9518889058..3cd4b7b0d95 100644 --- a/src/cascadia/TerminalSettingsModel/WslDistroGenerator.cpp +++ b/src/cascadia/TerminalSettingsModel/WslDistroGenerator.cpp @@ -124,7 +124,7 @@ std::vector WslDistroGenerator::GenerateProfiles() { distName.resize(firstChar); } - auto WSLDistro{ CreateDefaultProfile(distName, GetNamespace()) }; + auto WSLDistro{ CreateDefaultProfile(distName) }; WSLDistro.Commandline(L"wsl.exe -d " + distName); WSLDistro.ColorSchemeName(L"Campbell"); From 6f55ae5a0c6bacc2b0e6bba40a400300dcc15d66 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Mon, 26 Oct 2020 15:50:14 -0700 Subject: [PATCH 28/29] code format --- src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp b/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp index a8a9b488806..9f878d2300d 100644 --- a/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp +++ b/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp @@ -2573,7 +2573,7 @@ namespace SettingsModelLocalTests VERIFY_ARE_EQUAL(settings->Profiles().Size(), copyImpl->Profiles().Size()); // so we should only have one parent, instead of two - auto srcProfile{ winrt::get_self(settings->Profiles().GetAt(0)) }; + auto srcProfile{ winrt::get_self(settings->Profiles().GetAt(0)) }; auto copyProfile{ winrt::get_self(copyImpl->Profiles().GetAt(0)) }; VERIFY_ARE_EQUAL(srcProfile->Parents().size(), 0u); VERIFY_ARE_EQUAL(srcProfile->Parents().size(), copyProfile->Parents().size()); From 4cf3b1ca2eceb4fcc6e22813d4d7e89bde39f5a1 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Tue, 27 Oct 2020 09:32:31 -0700 Subject: [PATCH 29/29] remove a few comments --- src/cascadia/TerminalSettingsModel/Profile.cpp | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/Profile.cpp b/src/cascadia/TerminalSettingsModel/Profile.cpp index 9fab528a217..d7404b98f1b 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.cpp +++ b/src/cascadia/TerminalSettingsModel/Profile.cpp @@ -72,7 +72,6 @@ Profile::Profile(guid guid) : winrt::com_ptr Profile::CopySettings(winrt::com_ptr source) { - // copy the private members auto profile{ winrt::make_self() }; profile->_Guid = source->_Guid; profile->_Name = source->_Name; @@ -110,8 +109,6 @@ winrt::com_ptr Profile::CopySettings(winrt::com_ptr source) profile->_CursorShape = source->_CursorShape; profile->_CursorHeight = source->_CursorHeight; profile->_BellStyle = source->_BellStyle; - - // copy settings that did not use the GETSET macro profile->_BackgroundImageAlignment = source->_BackgroundImageAlignment; profile->_ConnectionType = source->_ConnectionType;