From 6ba3fc1b69d1549b995a575e0ab99133a61673f9 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Wed, 31 Jul 2024 13:41:55 -0700 Subject: [PATCH 01/16] Track and log changes to settings --- .../LaunchViewModel.cpp | 5 + .../AppearanceConfig.cpp | 97 +++++++++++- .../TerminalSettingsModel/AppearanceConfig.h | 4 + .../TerminalSettingsModel/FontConfig.cpp | 65 +++++++- .../TerminalSettingsModel/FontConfig.h | 6 + .../GlobalAppSettings.cpp | 145 +++++++++++++++++- .../TerminalSettingsModel/GlobalAppSettings.h | 5 +- .../TerminalSettingsModel/IInheritable.h | 21 +++ .../TerminalSettingsModel/Profile.cpp | 115 +++++++++++++- src/cascadia/TerminalSettingsModel/Profile.h | 4 +- 10 files changed, 459 insertions(+), 8 deletions(-) diff --git a/src/cascadia/TerminalSettingsEditor/LaunchViewModel.cpp b/src/cascadia/TerminalSettingsEditor/LaunchViewModel.cpp index 425d84299b9..172db128f92 100644 --- a/src/cascadia/TerminalSettingsEditor/LaunchViewModel.cpp +++ b/src/cascadia/TerminalSettingsEditor/LaunchViewModel.cpp @@ -92,6 +92,8 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation const LaunchPosition newPos{ xCoordRef, _Settings.GlobalSettings().InitialPosition().Y }; _Settings.GlobalSettings().InitialPosition(newPos); _NotifyChanges(L"LaunchParametersCurrentValue"); + + // TODO CARLOS: telemetry for InitialPos } void LaunchViewModel::InitialPosY(double yCoord) @@ -105,6 +107,8 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation const LaunchPosition newPos{ _Settings.GlobalSettings().InitialPosition().X, yCoordRef }; _Settings.GlobalSettings().InitialPosition(newPos); _NotifyChanges(L"LaunchParametersCurrentValue"); + + // TODO CARLOS: telemetry for InitialPos } void LaunchViewModel::UseDefaultLaunchPosition(bool useDefaultPosition) @@ -116,6 +120,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation InitialPosY(NAN); } _NotifyChanges(L"UseDefaultLaunchPosition", L"LaunchParametersCurrentValue", L"InitialPosX", L"InitialPosY"); + // TODO CARLOS: telemetry for InitialPos } bool LaunchViewModel::UseDefaultLaunchPosition() diff --git a/src/cascadia/TerminalSettingsModel/AppearanceConfig.cpp b/src/cascadia/TerminalSettingsModel/AppearanceConfig.cpp index 92af888c525..f88406ea3fc 100644 --- a/src/cascadia/TerminalSettingsModel/AppearanceConfig.cpp +++ b/src/cascadia/TerminalSettingsModel/AppearanceConfig.cpp @@ -90,12 +90,34 @@ Json::Value AppearanceConfig::ToJson() const void AppearanceConfig::LayerJson(const Json::Value& json) { JsonUtils::GetValueForKey(json, ForegroundKey, _Foreground); + if (_Foreground.has_value()) + { + _logSettingSet(ForegroundKey, _Foreground.value()); + } JsonUtils::GetValueForKey(json, BackgroundKey, _Background); + if (_Background.has_value()) + { + _logSettingSet(BackgroundKey, _Background.value()); + } JsonUtils::GetValueForKey(json, SelectionBackgroundKey, _SelectionBackground); + if (_SelectionBackground.has_value()) + { + _logSettingSet(SelectionBackgroundKey, _SelectionBackground.value()); + } JsonUtils::GetValueForKey(json, CursorColorKey, _CursorColor); + if (_CursorColor.has_value()) + { + _logSettingSet(CursorColorKey, _CursorColor.value()); + } JsonUtils::GetValueForKey(json, LegacyAcrylicTransparencyKey, _Opacity); JsonUtils::GetValueForKey(json, OpacityKey, _Opacity, JsonUtils::OptionalConverter{}); + if (_Opacity.has_value()) + { + _logSettingSet(OpacityKey, _Opacity.value()); + } + + // TODO CARLOS: how do we want to track this? Do we want to track the color scheme name? if (json["colorScheme"].isString()) { // to make the UI happy, set ColorSchemeName. @@ -110,7 +132,13 @@ void AppearanceConfig::LayerJson(const Json::Value& json) } #define APPEARANCE_SETTINGS_LAYER_JSON(type, name, jsonKey, ...) \ - JsonUtils::GetValueForKey(json, jsonKey, _##name); + { \ + JsonUtils::GetValueForKey(json, jsonKey, _##name); \ + if (_##name.has_value()) \ + { \ + _logSettingSet(jsonKey, _##name.value()); \ + } \ + } \ MTSM_APPEARANCE_SETTINGS(APPEARANCE_SETTINGS_LAYER_JSON) #undef APPEARANCE_SETTINGS_LAYER_JSON } @@ -156,3 +184,70 @@ winrt::hstring AppearanceConfig::ExpandedBackgroundImagePath() return winrt::hstring{ wil::ExpandEnvironmentStringsW(path.c_str()) }; } } + +winrt::hstring _convertVal(const winrt::Microsoft::Terminal::Core::CursorStyle val) +{ + OutputDebugString(L"CursorStyle\n"); + JsonUtils::ConversionTrait converter{}; + return winrt::to_hstring(converter.ToJson(val).asString()); +} + +winrt::hstring _convertVal(const winrt::Windows::UI::Xaml::Media::Stretch val) +{ + OutputDebugString(L"BackgroundImageStretchMode\n"); + JsonUtils::ConversionTrait converter{}; + return winrt::to_hstring(converter.ToJson(val).asString()); +} + +winrt::hstring _convertVal(const winrt::Microsoft::Terminal::Settings::Model::ConvergedAlignment val) +{ + OutputDebugString(L"BackgroundImageAlignment\n"); + JsonUtils::ConversionTrait converter{}; + return winrt::to_hstring(converter.ToJson(val).asString()); +} + +winrt::hstring _convertVal(const winrt::Microsoft::Terminal::Settings::Model::IntenseStyle val) +{ + OutputDebugString(L"IntenseTextStyle\n"); + JsonUtils::ConversionTrait converter{}; + return winrt::to_hstring(converter.ToJson(val).asString()); +} + +winrt::hstring _convertVal(const winrt::Microsoft::Terminal::Core::AdjustTextMode val) +{ + OutputDebugString(L"AdjustIndistinguishableColors\n"); + JsonUtils::ConversionTrait converter{}; + return winrt::to_hstring(converter.ToJson(val).asString()); +} + +winrt::hstring _convertVal(const std::optional val) +{ + // TODO CARLOS: test this one specifically + // - std::nullopt -> "null" + // - color -> something + OutputDebugString(L"optional\n"); + JsonUtils::ConversionTrait converter{}; + return winrt::to_hstring(converter.ToJson(val).asString()); +} + +winrt::hstring _convertVal(auto& val) +{ + OutputDebugString(L"auto\n"); + return winrt::to_hstring(val); +} + +void AppearanceConfig::_logSettingSet(std::string_view setting, auto& value) +{ + OutputDebugStringA(setting.data()); + OutputDebugStringA(" - "); + std::wstring val{ _convertVal(value).c_str() }; + _changeLog.insert_or_assign(setting, std::wstring_view{ val }); +} + +void AppearanceConfig::_logSettingValIfSet(const Json::Value& json, std::string_view setting) +{ + if (auto found{ json.find(&*setting.cbegin(), (&*setting.cbegin()) + setting.size()) }) + { + _changeLog.insert_or_assign(setting, std::wstring_view{ til::u8u16(found->asString()) }); + } +} diff --git a/src/cascadia/TerminalSettingsModel/AppearanceConfig.h b/src/cascadia/TerminalSettingsModel/AppearanceConfig.h index cffbdede6db..4b97942ffa8 100644 --- a/src/cascadia/TerminalSettingsModel/AppearanceConfig.h +++ b/src/cascadia/TerminalSettingsModel/AppearanceConfig.h @@ -52,5 +52,9 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation private: winrt::weak_ref _sourceProfile; + std::map _changeLog; + + void _logSettingSet(std::string_view setting, auto& value); + void _logSettingValIfSet(const Json::Value& json, std::string_view setting); }; } diff --git a/src/cascadia/TerminalSettingsModel/FontConfig.cpp b/src/cascadia/TerminalSettingsModel/FontConfig.cpp index 746ceee649d..598d93eaba1 100644 --- a/src/cascadia/TerminalSettingsModel/FontConfig.cpp +++ b/src/cascadia/TerminalSettingsModel/FontConfig.cpp @@ -83,8 +83,14 @@ void FontConfig::LayerJson(const Json::Value& json) { // A font object is defined, use that const auto fontInfoJson = json[JsonKey(FontInfoKey)]; -#define FONT_SETTINGS_LAYER_JSON(type, name, jsonKey, ...) \ - JsonUtils::GetValueForKey(fontInfoJson, jsonKey, _##name); +#define FONT_SETTINGS_LAYER_JSON(type, name, jsonKey, ...) \ + { \ + JsonUtils::GetValueForKey(fontInfoJson, jsonKey, _##name); \ + if (_##name.has_value()) \ + { \ + _logSettingSet(jsonKey, _##name.value()); \ + } \ + } MTSM_FONT_SETTINGS(FONT_SETTINGS_LAYER_JSON) #undef FONT_SETTINGS_LAYER_JSON } @@ -92,8 +98,20 @@ void FontConfig::LayerJson(const Json::Value& json) { // No font object is defined JsonUtils::GetValueForKey(json, LegacyFontFaceKey, _FontFace); + if (_FontFace.has_value()) + { + _logSettingSet(LegacyFontFaceKey, _FontFace.value()); + } JsonUtils::GetValueForKey(json, LegacyFontSizeKey, _FontSize); + if (_FontSize.has_value()) + { + _logSettingSet(LegacyFontSizeKey, _FontSize.value()); + } JsonUtils::GetValueForKey(json, LegacyFontWeightKey, _FontWeight); + if (_FontWeight.has_value()) + { + _logSettingSet(LegacyFontWeightKey, _FontWeight.value()); + } } } @@ -101,3 +119,46 @@ winrt::Microsoft::Terminal::Settings::Model::Profile FontConfig::SourceProfile() { return _sourceProfile.get(); } + +winrt::hstring _convertVal(const winrt::Windows::UI::Text::FontWeight val) +{ + OutputDebugString(L"FontWeight\n"); + JsonUtils::ConversionTrait converter{}; + return winrt::to_hstring(converter.ToJson(val).asString()); +} + +void _logMap(const winrt::Windows::Foundation::Collections::IMap& val, std::map& log) +{ + for (const auto& [mapKey, mapVal] : val) + { + log.insert_or_assign(std::wstring_view{ mapKey.c_str() }, std::to_wstring(mapVal)); + } +} + +winrt::hstring _convertVal(auto& val) +{ + OutputDebugString(L"auto\n"); + return winrt::to_hstring(val); +} + +void FontConfig::_logSettingSet(std::string_view setting, winrt::Windows::Foundation::Collections::IMap& value) +{ + OutputDebugStringA(setting.data()); + OutputDebugStringA(" - "); + if (setting == "axes") + { + _logMap(value, _changeLogAxes); + } + else if (setting == "features") + { + _logMap(value, _changeLogFeatures); + } +} + +void FontConfig::_logSettingSet(std::string_view setting, auto& value) +{ + OutputDebugStringA(setting.data()); + OutputDebugStringA(" - "); + std::wstring val{ _convertVal(value).c_str() }; + _changeLog.insert_or_assign(setting, std::wstring_view{ val }); +} diff --git a/src/cascadia/TerminalSettingsModel/FontConfig.h b/src/cascadia/TerminalSettingsModel/FontConfig.h index c249f5a1b46..8b0bb1d3f9d 100644 --- a/src/cascadia/TerminalSettingsModel/FontConfig.h +++ b/src/cascadia/TerminalSettingsModel/FontConfig.h @@ -45,5 +45,11 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation private: winrt::weak_ref _sourceProfile; + std::map _changeLog; + std::map _changeLogAxes; + std::map _changeLogFeatures; + + void _logSettingSet(std::string_view setting, winrt::Windows::Foundation::Collections::IMap& value); + void _logSettingSet(std::string_view setting, auto& value); }; } diff --git a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp index f51ece15e1b..4c8821e4e74 100644 --- a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp +++ b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp @@ -128,13 +128,23 @@ winrt::com_ptr GlobalAppSettings::FromJson(const Json::Value& void GlobalAppSettings::LayerJson(const Json::Value& json, const OriginTag origin) { JsonUtils::GetValueForKey(json, DefaultProfileKey, _UnparsedDefaultProfile); + if (_UnparsedDefaultProfile.has_value()) + { + _logSettingSet(DefaultProfileKey, _UnparsedDefaultProfile.value()); + } // GH#8076 - when adding enum values to this key, we also changed it from // "useTabSwitcher" to "tabSwitcherMode". Continue supporting // "useTabSwitcher", but prefer "tabSwitcherMode" JsonUtils::GetValueForKey(json, LegacyUseTabSwitcherModeKey, _TabSwitcherMode); #define GLOBAL_SETTINGS_LAYER_JSON(type, name, jsonKey, ...) \ - JsonUtils::GetValueForKey(json, jsonKey, _##name); + { \ + JsonUtils::GetValueForKey(json, jsonKey, _##name); \ + if (_##name.has_value()) \ + { \ + _logSettingSet(jsonKey, _##name.value()); \ + } \ + } MTSM_GLOBAL_SETTINGS(GLOBAL_SETTINGS_LAYER_JSON) #undef GLOBAL_SETTINGS_LAYER_JSON @@ -152,6 +162,10 @@ void GlobalAppSettings::LayerJson(const Json::Value& json, const OriginTag origi LayerActionsFrom(json, origin, true); JsonUtils::GetValueForKey(json, LegacyReloadEnvironmentVariablesKey, _legacyReloadEnvironmentVariables); + if (json.find(&*LegacyReloadEnvironmentVariablesKey.cbegin(), (&*LegacyReloadEnvironmentVariablesKey.cbegin()) + LegacyReloadEnvironmentVariablesKey.size())) + { + _logSettingSet(LegacyReloadEnvironmentVariablesKey, _legacyReloadEnvironmentVariables); + } } void GlobalAppSettings::LayerActionsFrom(const Json::Value& json, const OriginTag origin, const bool withKeybindings) @@ -317,3 +331,132 @@ bool GlobalAppSettings::ShouldUsePersistedLayout() const { return FirstWindowPreference() == FirstWindowPreference::PersistedWindowLayout && !IsolatedMode(); } + +winrt::hstring _convertVal(const winrt::Microsoft::Terminal::Control::GraphicsAPI val) +{ + OutputDebugString(L"GraphicsAPI\n"); + JsonUtils::ConversionTrait converter{}; + return winrt::to_hstring(converter.ToJson(val).asString()); +} + +winrt::hstring _convertVal(const winrt::Microsoft::Terminal::Control::TextMeasurement val) +{ + OutputDebugString(L"TextMeasurement\n"); + JsonUtils::ConversionTrait converter{}; + return winrt::to_hstring(converter.ToJson(val).asString()); +} + +winrt::hstring _convertVal(const winrt::Microsoft::Terminal::Settings::Model::NewTabPosition val) +{ + OutputDebugString(L"NewTabPos\n"); + JsonUtils::ConversionTrait converter{}; + return winrt::to_hstring(converter.ToJson(val).asString()); +} + +winrt::hstring _convertVal(const winrt::Microsoft::UI::Xaml::Controls::TabViewWidthMode val) +{ + OutputDebugString(L"TabViewWidthMode\n"); + JsonUtils::ConversionTrait converter{}; + return winrt::to_hstring(converter.ToJson(val).asString()); +} + +winrt::hstring _convertVal(const winrt::Microsoft::Terminal::Control::CopyFormat val) +{ + OutputDebugString(L"CopyFormat\n"); + JsonUtils::ConversionTrait converter{}; + return winrt::to_hstring(converter.ToJson(val).asString()); +} + +winrt::hstring _convertVal(const winrt::Microsoft::Terminal::Settings::Model::LaunchPosition val) +{ + OutputDebugString(L"LaunchPos\n"); + JsonUtils::ConversionTrait converter{}; + return winrt::to_hstring(converter.ToJson(val).asString()); +} + +winrt::hstring _convertVal(const winrt::Microsoft::Terminal::Settings::Model::FirstWindowPreference val) +{ + OutputDebugString(L"FirstWindowPref\n"); + JsonUtils::ConversionTrait converter{}; + return winrt::to_hstring(converter.ToJson(val).asString()); +} + +winrt::hstring _convertVal(const winrt::Microsoft::Terminal::Settings::Model::LaunchMode val) +{ + OutputDebugString(L"LaunchMode\n"); + JsonUtils::ConversionTrait converter{}; + return winrt::to_hstring(converter.ToJson(val).asString()); +} + +winrt::hstring _convertVal(const winrt::Microsoft::Terminal::Settings::Model::TabSwitcherMode val) +{ + OutputDebugString(L"TabSwitcherMode\n"); + JsonUtils::ConversionTrait converter{}; + return winrt::to_hstring(converter.ToJson(val).asString()); +} + +winrt::hstring _convertVal(const winrt::Microsoft::Terminal::Settings::Model::WindowingMode val) +{ + OutputDebugString(L"WindowingMode\n"); + JsonUtils::ConversionTrait converter{}; + return winrt::to_hstring(converter.ToJson(val).asString()); +} + +winrt::hstring _convertVal(const winrt::guid& val) +{ + OutputDebugString(L"guid\n"); + JsonUtils::ConversionTrait converter{}; + return winrt::to_hstring(converter.ToJson(val).asString()); +} + +winrt::hstring _convertVal(const winrt::Microsoft::Terminal::Settings::Model::ThemePair val) +{ + OutputDebugString(L"ThemePair\n"); + // TODO CARLOS: no conversion trait + JsonUtils::ConversionTrait converter{}; + return winrt::to_hstring(converter.ToJson(val).asString()); +} + +winrt::hstring _convertVal(const winrt::Windows::Foundation::Collections::IVector val) +{ + OutputDebugString(L"vector\n"); + + // convert and sort so we normalize results + std::vector vec; + vec.reserve(val.Size()); + val.GetMany(0, vec); + std::sort(vec.begin(), vec.end()); + + // consolidate into a single string + winrt::hstring result; + for (auto iter = vec.begin(); iter != vec.end(); iter++) + { + result = result + *iter; + if (iter + 1 != vec.end()) + { + result = result + L", "; + } + } + return result; +} + +winrt::hstring _convertVal(const winrt::Windows::Foundation::Collections::IVector /*value*/) +{ + OutputDebugString(L"vector\n"); + // TODO CARLOS: what data do we even want to collect here? + return L""; +} + +winrt::hstring _convertVal(auto& val) +{ + OutputDebugString(L"auto\n"); + return winrt::to_hstring(val); +} + +void GlobalAppSettings::_logSettingSet(std::string_view setting, auto& value) +{ + OutputDebugStringA(setting.data()); + OutputDebugStringA(" - "); + std::wstring val{ _convertVal(value).c_str() }; + _changeLog.insert_or_assign(setting, std::wstring_view{ val }); +} diff --git a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.h b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.h index 7b55b7007b5..164722d8b48 100644 --- a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.h +++ b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.h @@ -75,7 +75,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation INHERITABLE_SETTING(Model::GlobalAppSettings, hstring, UnparsedDefaultProfile, L""); #define GLOBAL_SETTINGS_INITIALIZE(type, name, jsonKey, ...) \ - INHERITABLE_SETTING(Model::GlobalAppSettings, type, name, ##__VA_ARGS__) + INHERITABLE_SETTING_WITH_LOGGING(Model::GlobalAppSettings, type, name, jsonKey, ##__VA_ARGS__) MTSM_GLOBAL_SETTINGS(GLOBAL_SETTINGS_INITIALIZE) #undef GLOBAL_SETTINGS_INITIALIZE @@ -89,9 +89,12 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation winrt::guid _defaultProfile{}; bool _legacyReloadEnvironmentVariables{ true }; winrt::com_ptr _actionMap{ winrt::make_self() }; + std::map _changeLog; std::vector _keybindingsWarnings; Windows::Foundation::Collections::IMap _colorSchemes{ winrt::single_threaded_map() }; Windows::Foundation::Collections::IMap _themes{ winrt::single_threaded_map() }; + + void _logSettingSet(std::string_view setting, auto& value); }; } diff --git a/src/cascadia/TerminalSettingsModel/IInheritable.h b/src/cascadia/TerminalSettingsModel/IInheritable.h index 091cd653a1c..ab57ff411ed 100644 --- a/src/cascadia/TerminalSettingsModel/IInheritable.h +++ b/src/cascadia/TerminalSettingsModel/IInheritable.h @@ -185,6 +185,27 @@ public: \ _##name = value; \ } +#define INHERITABLE_SETTING_WITH_LOGGING(projectedType, type, name, jsonKey, ...) \ + _BASE_INHERITABLE_SETTING(projectedType, std::optional, name, ...) \ +public: \ + /* 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 */ \ + void name(const type& value) \ + { \ + if (name() != value) \ + { \ + _logSettingSet(jsonKey, value); \ + } \ + _##name = value; \ + } + // This macro is similar to the one above, but is reserved for optional settings // like Profile.Foreground (where null is interpreted // as an acceptable value, rather than "inherit") diff --git a/src/cascadia/TerminalSettingsModel/Profile.cpp b/src/cascadia/TerminalSettingsModel/Profile.cpp index 0f568b37e53..d66d94612b8 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.cpp +++ b/src/cascadia/TerminalSettingsModel/Profile.cpp @@ -176,14 +176,30 @@ void Profile::LayerJson(const Json::Value& json) JsonUtils::GetValueForKey(json, UpdatesKey, _Updates); JsonUtils::GetValueForKey(json, GuidKey, _Guid); JsonUtils::GetValueForKey(json, HiddenKey, _Hidden); + if (_Hidden.has_value()) + { + _logSettingSet(HiddenKey, _Hidden.value()); + } JsonUtils::GetValueForKey(json, SourceKey, _Source); JsonUtils::GetValueForKey(json, IconKey, _Icon); + if (_Icon.has_value()) + { + _logSettingSet(IconKey, _Icon.value()); + } // Padding was never specified as an integer, but it was a common working mistake. // Allow it to be permissive. JsonUtils::GetValueForKey(json, PaddingKey, _Padding, JsonUtils::OptionalConverter>{}); + if (_Padding.has_value()) + { + _logSettingSet(PaddingKey, _Padding.value()); + } JsonUtils::GetValueForKey(json, TabColorKey, _TabColor); + if (_TabColor.has_value()) + { + _logSettingSet(TabColorKey, _TabColor.value()); + } // Try to load some legacy keys, to migrate them. // Done _before_ the MTSM_PROFILE_SETTINGS, which have the updated keys. @@ -191,8 +207,13 @@ void Profile::LayerJson(const Json::Value& json) JsonUtils::GetValueForKey(json, LegacyAutoMarkPromptsKey, _AutoMarkPrompts); #define PROFILE_SETTINGS_LAYER_JSON(type, name, jsonKey, ...) \ - JsonUtils::GetValueForKey(json, jsonKey, _##name); - + { \ + JsonUtils::GetValueForKey(json, jsonKey, _##name); \ + if (_##name.has_value()) \ + { \ + _logSettingSet(jsonKey, _##name.value()); \ + } \ + } \ MTSM_PROFILE_SETTINGS(PROFILE_SETTINGS_LAYER_JSON) #undef PROFILE_SETTINGS_LAYER_JSON @@ -518,3 +539,93 @@ std::wstring Profile::NormalizeCommandLine(LPCWSTR commandLine) return normalized; } + +winrt::hstring _convertVal(const winrt::Microsoft::Terminal::Control::ScrollbarState& val) +{ + OutputDebugString(L"ScrollbarState\n"); + JsonUtils::ConversionTrait converter{}; + return winrt::to_hstring(converter.ToJson(val).asString()); +} + +winrt::hstring _convertVal(const winrt::Microsoft::Terminal::Control::TextAntialiasingMode& val) +{ + OutputDebugString(L"TextAntialiasingMode\n"); + JsonUtils::ConversionTrait converter{}; + return winrt::to_hstring(converter.ToJson(val).asString()); +} + +winrt::hstring _convertVal(const winrt::Microsoft::Terminal::Settings::Model::CloseOnExitMode& val) +{ + OutputDebugString(L"CloseOnExitMode\n"); + JsonUtils::ConversionTrait converter{}; + return winrt::to_hstring(converter.ToJson(val).asString()); +} + +winrt::hstring _convertVal(const winrt::Microsoft::Terminal::Settings::Model::BellStyle& val) +{ + OutputDebugString(L"BellStyle\n"); + JsonUtils::ConversionTrait converter{}; + return winrt::to_hstring(converter.ToJson(val).asString()); +} + +winrt::hstring _convertVal(const IEnvironmentVariableMap& /*val*/) +{ + // TODO CARLOS: we probably don't want this? + // if we do, I can handle it similar to Vector + OutputDebugString(L"IEnvironmentVariableMap\n"); + return L""; +} + +winrt::hstring _convertVal(const winrt::Windows::Foundation::Collections::IVector val) +{ + OutputDebugString(L"vector\n"); + + // convert and sort so we normalize results + std::vector vec; + vec.reserve(val.Size()); + val.GetMany(0, vec); + std::sort(vec.begin(), vec.end()); + + // consolidate into a single string + winrt::hstring result; + for (auto iter = vec.begin(); iter != vec.end(); iter++) + { + result = result + *iter; + if (iter + 1 != vec.end()) + { + result = result + L", "; + } + } + return result; +} + +winrt::hstring _convertVal(const std::optional val) +{ + // TODO CARLOS: test this one specifically + // - std::nullopt -> "null" + // - color -> something + OutputDebugString(L"optional\n"); + JsonUtils::ConversionTrait converter{}; + return winrt::to_hstring(converter.ToJson(val).asString()); +} + +winrt::hstring _convertVal(const winrt::guid& val) +{ + OutputDebugString(L"guid\n"); + JsonUtils::ConversionTrait converter{}; + return winrt::to_hstring(converter.ToJson(val).asString()); +} + +winrt::hstring _convertVal(auto& val) +{ + OutputDebugString(L"auto\n"); + return winrt::to_hstring(val); +} + +void Profile::_logSettingSet(std::string_view setting, auto& value) +{ + OutputDebugStringA(setting.data()); + OutputDebugStringA(" - "); + std::wstring val{ _convertVal(value).c_str() }; + _changeLog.insert_or_assign(setting, std::wstring_view{ val }); +} diff --git a/src/cascadia/TerminalSettingsModel/Profile.h b/src/cascadia/TerminalSettingsModel/Profile.h index 6fdd5c3ed57..ddf4ab88111 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.h +++ b/src/cascadia/TerminalSettingsModel/Profile.h @@ -131,7 +131,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation public: #define PROFILE_SETTINGS_INITIALIZE(type, name, jsonKey, ...) \ - INHERITABLE_SETTING(Model::Profile, type, name, ##__VA_ARGS__) + INHERITABLE_SETTING_WITH_LOGGING(Model::Profile, type, name, jsonKey, ##__VA_ARGS__) MTSM_PROFILE_SETTINGS(PROFILE_SETTINGS_INITIALIZE) #undef PROFILE_SETTINGS_INITIALIZE @@ -140,12 +140,14 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation Model::FontConfig _FontInfo{ winrt::make(weak_ref(*this)) }; std::optional _evaluatedIcon{ std::nullopt }; + std::map _changeLog; static std::wstring EvaluateStartingDirectory(const std::wstring& directory); static guid _GenerateGuidForProfile(const std::wstring_view& name, const std::wstring_view& source) noexcept; winrt::hstring _evaluateIcon() const; + void _logSettingSet(std::string_view setting, auto& value); friend class SettingsModelUnitTests::DeserializationTests; friend class SettingsModelUnitTests::ProfileTests; From f4f6f11ac96efd064a41d2a006a40d5add6a0551 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Fri, 2 Aug 2024 05:34:27 -0700 Subject: [PATCH 02/16] log settings model objects --- src/cascadia/TerminalApp/AppLogic.cpp | 4 + .../TerminalSettingsEditor/MainPage.cpp | 1 + src/cascadia/TerminalSettingsEditor/pch.h | 7 + .../TerminalSettingsModel/ActionMap.h | 5 + .../ActionMapSerialization.cpp | 12 +- .../AppearanceConfig.cpp | 109 +++------- .../TerminalSettingsModel/AppearanceConfig.h | 7 +- .../TerminalSettingsModel/CascadiaSettings.h | 6 + .../CascadiaSettings.idl | 1 + .../CascadiaSettingsSerialization.cpp | 77 +++++++ .../TerminalSettingsModel/Command.cpp | 53 +++++ src/cascadia/TerminalSettingsModel/Command.h | 1 + .../TerminalSettingsModel/FontConfig.cpp | 85 +++----- .../TerminalSettingsModel/FontConfig.h | 9 +- .../GlobalAppSettings.cpp | 199 ++++++------------ .../TerminalSettingsModel/GlobalAppSettings.h | 7 +- .../TerminalSettingsModel/IInheritable.h | 4 +- .../TerminalSettingsModel/Profile.cpp | 131 +++--------- src/cascadia/TerminalSettingsModel/Profile.h | 7 +- src/cascadia/TerminalSettingsModel/Theme.cpp | 31 +++ src/cascadia/TerminalSettingsModel/Theme.h | 5 +- 21 files changed, 382 insertions(+), 379 deletions(-) diff --git a/src/cascadia/TerminalApp/AppLogic.cpp b/src/cascadia/TerminalApp/AppLogic.cpp index dc11b506449..5fdfd33cd87 100644 --- a/src/cascadia/TerminalApp/AppLogic.cpp +++ b/src/cascadia/TerminalApp/AppLogic.cpp @@ -432,6 +432,10 @@ namespace winrt::TerminalApp::implementation return; } } + else + { + _settings.LogSettingChanges(true); + } if (initialLoad) { diff --git a/src/cascadia/TerminalSettingsEditor/MainPage.cpp b/src/cascadia/TerminalSettingsEditor/MainPage.cpp index ea98ee83f12..76bbeacb248 100644 --- a/src/cascadia/TerminalSettingsEditor/MainPage.cpp +++ b/src/cascadia/TerminalSettingsEditor/MainPage.cpp @@ -481,6 +481,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation void MainPage::SaveButton_Click(const IInspectable& /*sender*/, const RoutedEventArgs& /*args*/) { + _settingsClone.LogSettingChanges(false); _settingsClone.WriteSettingsToDisk(); } diff --git a/src/cascadia/TerminalSettingsEditor/pch.h b/src/cascadia/TerminalSettingsEditor/pch.h index 2e9eba2f51a..fe8635709d7 100644 --- a/src/cascadia/TerminalSettingsEditor/pch.h +++ b/src/cascadia/TerminalSettingsEditor/pch.h @@ -51,6 +51,13 @@ #include #include +// Including TraceLogging essentials for the binary +#include +#include +TRACELOGGING_DECLARE_PROVIDER(g_hSettingsEditorProvider); +#include +#include + #include #include #include diff --git a/src/cascadia/TerminalSettingsModel/ActionMap.h b/src/cascadia/TerminalSettingsModel/ActionMap.h index e3ceb3f5188..af9951be475 100644 --- a/src/cascadia/TerminalSettingsModel/ActionMap.h +++ b/src/cascadia/TerminalSettingsModel/ActionMap.h @@ -73,6 +73,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation Json::Value ToJson() const; Json::Value KeyBindingsToJson() const; bool FixupsAppliedDuringLoad() const; + void LogSettingChanges(std::set& changes, std::string_view& context) const; // modification bool RebindKeys(const Control::KeyChord& oldKeys, const Control::KeyChord& newKeys); @@ -104,6 +105,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation std::vector _updateLocalSnippetCache(winrt::hstring currentWorkingDirectory); + void _logCommand(const Model::Command& cmd); + Windows::Foundation::Collections::IMap _AvailableActionsCache{ nullptr }; Windows::Foundation::Collections::IMap _NameMapCache{ nullptr }; Windows::Foundation::Collections::IMap _GlobalHotkeysCache{ nullptr }; @@ -138,6 +141,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation til::shared_mutex>> _cwdLocalSnippetsCache{}; + std::set _changeLog; + friend class SettingsModelUnitTests::KeyBindingsTests; friend class SettingsModelUnitTests::DeserializationTests; friend class SettingsModelUnitTests::TerminalSettingsTests; diff --git a/src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp b/src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp index b6216971cef..413e86eff5e 100644 --- a/src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp +++ b/src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp @@ -78,7 +78,9 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // Now check if this is a command block if (jsonBlock.isMember(JsonKey(CommandsKey)) || jsonBlock.isMember(JsonKey(ActionKey))) { - AddAction(*Command::FromJson(jsonBlock, warnings, origin), keys); + auto command = Command::FromJson(jsonBlock, warnings, origin); + command->LogSettingChanges(_changeLog); + AddAction(*command, keys); if (jsonBlock.isMember(JsonKey(KeysKey))) { @@ -156,4 +158,12 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation return keybindingsList; } + + void ActionMap::LogSettingChanges(std::set& changes, std::string_view& context) const + { + for (const auto& setting : _changeLog) + { + changes.emplace(fmt::format(FMT_COMPILE("{}.{}"), context, setting)); + } + } } diff --git a/src/cascadia/TerminalSettingsModel/AppearanceConfig.cpp b/src/cascadia/TerminalSettingsModel/AppearanceConfig.cpp index f88406ea3fc..85e206252f9 100644 --- a/src/cascadia/TerminalSettingsModel/AppearanceConfig.cpp +++ b/src/cascadia/TerminalSettingsModel/AppearanceConfig.cpp @@ -90,55 +90,42 @@ Json::Value AppearanceConfig::ToJson() const void AppearanceConfig::LayerJson(const Json::Value& json) { JsonUtils::GetValueForKey(json, ForegroundKey, _Foreground); - if (_Foreground.has_value()) - { - _logSettingSet(ForegroundKey, _Foreground.value()); - } + _logSettingIfSet(ForegroundKey, _Foreground.has_value()); + JsonUtils::GetValueForKey(json, BackgroundKey, _Background); - if (_Background.has_value()) - { - _logSettingSet(BackgroundKey, _Background.value()); - } + _logSettingIfSet(BackgroundKey, _Background.has_value()); + JsonUtils::GetValueForKey(json, SelectionBackgroundKey, _SelectionBackground); - if (_SelectionBackground.has_value()) - { - _logSettingSet(SelectionBackgroundKey, _SelectionBackground.value()); - } + _logSettingIfSet(SelectionBackgroundKey, _SelectionBackground.has_value()); + JsonUtils::GetValueForKey(json, CursorColorKey, _CursorColor); - if (_CursorColor.has_value()) - { - _logSettingSet(CursorColorKey, _CursorColor.value()); - } + _logSettingIfSet(CursorColorKey, _CursorColor.has_value()); JsonUtils::GetValueForKey(json, LegacyAcrylicTransparencyKey, _Opacity); JsonUtils::GetValueForKey(json, OpacityKey, _Opacity, JsonUtils::OptionalConverter{}); - if (_Opacity.has_value()) - { - _logSettingSet(OpacityKey, _Opacity.value()); - } + _logSettingIfSet(OpacityKey, _Opacity.has_value()); - // TODO CARLOS: how do we want to track this? Do we want to track the color scheme name? if (json["colorScheme"].isString()) { // to make the UI happy, set ColorSchemeName. JsonUtils::GetValueForKey(json, ColorSchemeKey, _DarkColorSchemeName); _LightColorSchemeName = _DarkColorSchemeName; + _logSettingSet(ColorSchemeKey); } else if (json["colorScheme"].isObject()) { // to make the UI happy, set ColorSchemeName to whatever the dark value is. JsonUtils::GetValueForKey(json["colorScheme"], "dark", _DarkColorSchemeName); JsonUtils::GetValueForKey(json["colorScheme"], "light", _LightColorSchemeName); + + _logSettingSet("colorScheme.dark"); + _logSettingSet("colorScheme.light"); } #define APPEARANCE_SETTINGS_LAYER_JSON(type, name, jsonKey, ...) \ - { \ - JsonUtils::GetValueForKey(json, jsonKey, _##name); \ - if (_##name.has_value()) \ - { \ - _logSettingSet(jsonKey, _##name.value()); \ - } \ - } \ + JsonUtils::GetValueForKey(json, jsonKey, _##name); \ + _logSettingIfSet(jsonKey, _##name.has_value()); + MTSM_APPEARANCE_SETTINGS(APPEARANCE_SETTINGS_LAYER_JSON) #undef APPEARANCE_SETTINGS_LAYER_JSON } @@ -185,69 +172,23 @@ winrt::hstring AppearanceConfig::ExpandedBackgroundImagePath() } } -winrt::hstring _convertVal(const winrt::Microsoft::Terminal::Core::CursorStyle val) -{ - OutputDebugString(L"CursorStyle\n"); - JsonUtils::ConversionTrait converter{}; - return winrt::to_hstring(converter.ToJson(val).asString()); -} - -winrt::hstring _convertVal(const winrt::Windows::UI::Xaml::Media::Stretch val) +void AppearanceConfig::_logSettingSet(std::string_view setting) { - OutputDebugString(L"BackgroundImageStretchMode\n"); - JsonUtils::ConversionTrait converter{}; - return winrt::to_hstring(converter.ToJson(val).asString()); + _changeLog.emplace(setting); } -winrt::hstring _convertVal(const winrt::Microsoft::Terminal::Settings::Model::ConvergedAlignment val) +void AppearanceConfig::_logSettingIfSet(std::string_view setting, const bool isSet) { - OutputDebugString(L"BackgroundImageAlignment\n"); - JsonUtils::ConversionTrait converter{}; - return winrt::to_hstring(converter.ToJson(val).asString()); -} - -winrt::hstring _convertVal(const winrt::Microsoft::Terminal::Settings::Model::IntenseStyle val) -{ - OutputDebugString(L"IntenseTextStyle\n"); - JsonUtils::ConversionTrait converter{}; - return winrt::to_hstring(converter.ToJson(val).asString()); -} - -winrt::hstring _convertVal(const winrt::Microsoft::Terminal::Core::AdjustTextMode val) -{ - OutputDebugString(L"AdjustIndistinguishableColors\n"); - JsonUtils::ConversionTrait converter{}; - return winrt::to_hstring(converter.ToJson(val).asString()); -} - -winrt::hstring _convertVal(const std::optional val) -{ - // TODO CARLOS: test this one specifically - // - std::nullopt -> "null" - // - color -> something - OutputDebugString(L"optional\n"); - JsonUtils::ConversionTrait converter{}; - return winrt::to_hstring(converter.ToJson(val).asString()); -} - -winrt::hstring _convertVal(auto& val) -{ - OutputDebugString(L"auto\n"); - return winrt::to_hstring(val); -} - -void AppearanceConfig::_logSettingSet(std::string_view setting, auto& value) -{ - OutputDebugStringA(setting.data()); - OutputDebugStringA(" - "); - std::wstring val{ _convertVal(value).c_str() }; - _changeLog.insert_or_assign(setting, std::wstring_view{ val }); + if (isSet) + { + _logSettingSet(setting); + } } -void AppearanceConfig::_logSettingValIfSet(const Json::Value& json, std::string_view setting) +void AppearanceConfig::LogSettingChanges(std::set& changes, std::string_view& context) const { - if (auto found{ json.find(&*setting.cbegin(), (&*setting.cbegin()) + setting.size()) }) + for (const auto& setting : _changeLog) { - _changeLog.insert_or_assign(setting, std::wstring_view{ til::u8u16(found->asString()) }); + changes.emplace(fmt::format(FMT_COMPILE("{}.{}"), context, setting)); } } diff --git a/src/cascadia/TerminalSettingsModel/AppearanceConfig.h b/src/cascadia/TerminalSettingsModel/AppearanceConfig.h index 4b97942ffa8..9d9dceb554d 100644 --- a/src/cascadia/TerminalSettingsModel/AppearanceConfig.h +++ b/src/cascadia/TerminalSettingsModel/AppearanceConfig.h @@ -31,6 +31,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation static winrt::com_ptr CopyAppearance(const AppearanceConfig* source, winrt::weak_ref sourceProfile); Json::Value ToJson() const; void LayerJson(const Json::Value& json); + void LogSettingChanges(std::set& changes, std::string_view& context) const; Model::Profile SourceProfile(); @@ -52,9 +53,9 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation private: winrt::weak_ref _sourceProfile; - std::map _changeLog; + std::set _changeLog; - void _logSettingSet(std::string_view setting, auto& value); - void _logSettingValIfSet(const Json::Value& json, std::string_view setting); + void _logSettingSet(std::string_view setting); + void _logSettingIfSet(std::string_view setting, const bool isSet); }; } diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.h b/src/cascadia/TerminalSettingsModel/CascadiaSettings.h index becc6749c9f..6fe062a96f1 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.h +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettings.h @@ -48,6 +48,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation std::unordered_map> colorSchemes; std::unordered_map colorSchemeRemappings; bool fixupsAppliedDuringLoad{ false }; + std::set themesChangeLog; void clear(); }; @@ -96,6 +97,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation void _executeGenerator(const IDynamicProfileGenerator& generator); std::unordered_set _ignoredNamespaces; + std::set themesChangeLog; // See _getNonUserOriginProfiles(). size_t _userProfileCount = 0; }; @@ -150,6 +152,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation void ExpandCommands(); + void LogSettingChanges(bool isJsonLoad) const; + private: static const std::filesystem::path& _settingsPath(); static const std::filesystem::path& _releaseSettingsPath(); @@ -173,6 +177,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation void _validateThemeExists(); void _researchOnLoad(); + std::set _logSettingChanges() const; // user settings winrt::hstring _hash; @@ -180,6 +185,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation winrt::com_ptr _baseLayerProfile = winrt::make_self(); winrt::Windows::Foundation::Collections::IObservableVector _allProfiles = winrt::single_threaded_observable_vector(); winrt::Windows::Foundation::Collections::IObservableVector _activeProfiles = winrt::single_threaded_observable_vector(); + std::set _themesChangeLog{}; // load errors winrt::Windows::Foundation::Collections::IVector _warnings = winrt::single_threaded_vector(); diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.idl b/src/cascadia/TerminalSettingsModel/CascadiaSettings.idl index 0d8c0c5b158..2fa41941d6e 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.idl +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettings.idl @@ -24,6 +24,7 @@ namespace Microsoft.Terminal.Settings.Model CascadiaSettings Copy(); void WriteSettingsToDisk(); + void LogSettingChanges(Boolean isJsonLoad); String Hash { get; }; diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp index 237b687964e..796ff418f32 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp @@ -112,6 +112,7 @@ void ParsedSettings::clear() profilesByGuid.clear(); colorSchemes.clear(); fixupsAppliedDuringLoad = false; + themesChangeLog.clear(); } // This is a convenience method used by the CascadiaSettings constructor. @@ -642,6 +643,11 @@ void SettingsLoader::_parse(const OriginTag origin, const winrt::hstring& source // versions of these themes overriding the built-in ones. continue; } + else + { + static std::string_view themesContext{ "themes" }; + theme->LogSettingChanges(settings.themesChangeLog, themesContext); + } settings.globals->AddTheme(*theme); } } @@ -1206,6 +1212,7 @@ CascadiaSettings::CascadiaSettings(SettingsLoader&& loader) : _allProfiles = winrt::single_threaded_observable_vector(std::move(allProfiles)); _activeProfiles = winrt::single_threaded_observable_vector(std::move(activeProfiles)); _warnings = winrt::single_threaded_vector(std::move(warnings)); + _themesChangeLog = std::move(loader.userSettings.themesChangeLog); _resolveDefaultProfile(); _resolveNewTabMenuProfiles(); @@ -1580,3 +1587,73 @@ void CascadiaSettings::_resolveNewTabMenuProfilesSet(const IVector CascadiaSettings::_logSettingChanges() const +{ + // aggregate setting changes + std::set changes; + static std::string_view globalContext{ "global" }; + _globals->LogSettingChanges(changes, globalContext); + + static std::string_view actionContext{ "action" }; + winrt::get_self(_globals->ActionMap())->LogSettingChanges(changes, actionContext); + + static std::string_view profileContext{ "profile" }; + for (const auto& profile : _allProfiles) + { + winrt::get_self(profile)->LogSettingChanges(changes, profileContext); + } + + static std::string_view profileDefaultsContext{ "profileDefaults" }; + _baseLayerProfile->LogSettingChanges(changes, profileDefaultsContext); + + // DO NOT CALL Theme::LogSettingChanges!! + // We already collected the changes when we loaded the JSON + for (const auto& change : _themesChangeLog) + { + changes.insert(change); + } + + return changes; +} + +void CascadiaSettings::LogSettingChanges(bool isJsonLoad) const +{ + // Only do this if we're actually being sampled + if (!TraceLoggingProviderEnabled(g_hSettingsModelProvider, 0, MICROSOFT_KEYWORD_MEASURES)) + { + return; + } + + const auto changes = _logSettingChanges(); + + // report changes + for (const auto& change : changes) + { + OutputDebugStringA(change.data()); + OutputDebugStringA("\n"); + + // A `isJsonLoad ? "JsonSettingsChanged" : "UISettingsChanged"` + // would be nice, but that apparently isn't allowed in the macro below. + // Also, there's guidance to not send too much data all in one event, + // so we'll be sending a ton of events here. + if (isJsonLoad) + { + TraceLoggingWrite(g_hSettingsModelProvider, + "JsonSettingsChanged", + TraceLoggingDescription("Event emitted when settings change"), + TraceLoggingValue(change.data()), + TraceLoggingKeyword(MICROSOFT_KEYWORD_MEASURES), + TelemetryPrivacyDataTag(PDT_ProductAndServiceUsage)); + } + else + { + TraceLoggingWrite(g_hSettingsModelProvider, + "UISettingsChanged", + TraceLoggingDescription("Event emitted when settings change"), + TraceLoggingValue(change.data()), + TraceLoggingKeyword(MICROSOFT_KEYWORD_MEASURES), + TelemetryPrivacyDataTag(PDT_ProductAndServiceUsage)); + } + } +} diff --git a/src/cascadia/TerminalSettingsModel/Command.cpp b/src/cascadia/TerminalSettingsModel/Command.cpp index ac66d188b9d..1bc362c7a89 100644 --- a/src/cascadia/TerminalSettingsModel/Command.cpp +++ b/src/cascadia/TerminalSettingsModel/Command.cpp @@ -741,4 +741,57 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation return winrt::single_threaded_vector(std::move(result)); } + + void Command::LogSettingChanges(std::set& changes) + { + if (_IterateOn != ExpandCommandType::None) + { + switch (_IterateOn) + { + case ExpandCommandType::Profiles: + changes.insert(fmt::format(FMT_COMPILE("{}.{}"), IterateOnKey, "profiles")); + break; + case ExpandCommandType::ColorSchemes: + changes.insert(fmt::format(FMT_COMPILE("{}.{}"), IterateOnKey, "schemes")); + break; + } + } + + if (!_Description.empty()) + { + changes.insert(fmt::format(FMT_COMPILE("{}"), DescriptionKey)); + } + + if (IsNestedCommand()) + { + changes.insert(fmt::format(FMT_COMPILE("{}"), CommandsKey)); + } + else + { + const auto json{ ActionAndArgs::ToJson(ActionAndArgs()) }; + if (json.isString()) + { + // covers actions w/out args + // - "command": "unbound" --> "unbound" + // - "command": "copy" --> "copy" + changes.insert(fmt::format(FMT_COMPILE("{}"), json.asString())); + } + else + { + // covers actions w/ args + // - "command": { "action": "copy", "singleLine": true } --> "copy.singleLine" + // - "command": { "action": "copy", "singleLine": true, "dismissSelection": true } --> "copy.singleLine", "copy.dismissSelection" + + std::string_view shortcutActionName{ json[JsonKey("action")].asString() }; + + auto members = json.getMemberNames(); + members.erase(std::remove_if(members.begin(), members.end(), [](const auto& member) { return member == "action"; }), members.end()); + + for (const auto& actionArg : members) + { + changes.insert(fmt::format(FMT_COMPILE("{}.{}"), shortcutActionName, actionArg)); + } + } + } + } } diff --git a/src/cascadia/TerminalSettingsModel/Command.h b/src/cascadia/TerminalSettingsModel/Command.h index 1bc92220ca3..a45e63ae0b9 100644 --- a/src/cascadia/TerminalSettingsModel/Command.h +++ b/src/cascadia/TerminalSettingsModel/Command.h @@ -61,6 +61,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation const Json::Value& json, const OriginTag origin); Json::Value ToJson() const; + void LogSettingChanges(std::set& changes); bool HasNestedCommands() const; bool IsNestedCommand() const noexcept; diff --git a/src/cascadia/TerminalSettingsModel/FontConfig.cpp b/src/cascadia/TerminalSettingsModel/FontConfig.cpp index 598d93eaba1..51abd76fd75 100644 --- a/src/cascadia/TerminalSettingsModel/FontConfig.cpp +++ b/src/cascadia/TerminalSettingsModel/FontConfig.cpp @@ -83,35 +83,25 @@ void FontConfig::LayerJson(const Json::Value& json) { // A font object is defined, use that const auto fontInfoJson = json[JsonKey(FontInfoKey)]; -#define FONT_SETTINGS_LAYER_JSON(type, name, jsonKey, ...) \ - { \ - JsonUtils::GetValueForKey(fontInfoJson, jsonKey, _##name); \ - if (_##name.has_value()) \ - { \ - _logSettingSet(jsonKey, _##name.value()); \ - } \ - } +#define FONT_SETTINGS_LAYER_JSON(type, name, jsonKey, ...) \ + JsonUtils::GetValueForKey(fontInfoJson, jsonKey, _##name); \ + _logSettingIfSet(jsonKey, _##name.has_value()); + MTSM_FONT_SETTINGS(FONT_SETTINGS_LAYER_JSON) #undef FONT_SETTINGS_LAYER_JSON } else { // No font object is defined + // Log settings as if they were a part of the font object JsonUtils::GetValueForKey(json, LegacyFontFaceKey, _FontFace); - if (_FontFace.has_value()) - { - _logSettingSet(LegacyFontFaceKey, _FontFace.value()); - } + _logSettingIfSet("face", _FontFace.has_value()); + JsonUtils::GetValueForKey(json, LegacyFontSizeKey, _FontSize); - if (_FontSize.has_value()) - { - _logSettingSet(LegacyFontSizeKey, _FontSize.value()); - } + _logSettingIfSet("size", _FontSize.has_value()); + JsonUtils::GetValueForKey(json, LegacyFontWeightKey, _FontWeight); - if (_FontWeight.has_value()) - { - _logSettingSet(LegacyFontWeightKey, _FontWeight.value()); - } + _logSettingIfSet("weight", _FontWeight.has_value()); } } @@ -120,45 +110,40 @@ winrt::Microsoft::Terminal::Settings::Model::Profile FontConfig::SourceProfile() return _sourceProfile.get(); } -winrt::hstring _convertVal(const winrt::Windows::UI::Text::FontWeight val) +void FontConfig::_logSettingSet(std::string_view setting) { - OutputDebugString(L"FontWeight\n"); - JsonUtils::ConversionTrait converter{}; - return winrt::to_hstring(converter.ToJson(val).asString()); -} - -void _logMap(const winrt::Windows::Foundation::Collections::IMap& val, std::map& log) -{ - for (const auto& [mapKey, mapVal] : val) + if (setting == "axes" && _FontAxes.has_value()) { - log.insert_or_assign(std::wstring_view{ mapKey.c_str() }, std::to_wstring(mapVal)); + for (const auto& [mapKey, _] : _FontAxes.value()) + { + _changeLog.emplace(fmt::format(FMT_COMPILE("{}.{}"), setting, til::u16u8(mapKey))); + } + } + else if (setting == "features" && _FontFeatures.has_value()) + { + for (const auto& [mapKey, _] : _FontFeatures.value()) + { + _changeLog.emplace(fmt::format(FMT_COMPILE("{}.{}"), setting, til::u16u8(mapKey))); + } + } + else + { + _changeLog.emplace(setting); } } -winrt::hstring _convertVal(auto& val) -{ - OutputDebugString(L"auto\n"); - return winrt::to_hstring(val); -} - -void FontConfig::_logSettingSet(std::string_view setting, winrt::Windows::Foundation::Collections::IMap& value) +void FontConfig::_logSettingIfSet(std::string_view setting, const bool isSet) { - OutputDebugStringA(setting.data()); - OutputDebugStringA(" - "); - if (setting == "axes") + if (isSet) { - _logMap(value, _changeLogAxes); - } - else if (setting == "features") - { - _logMap(value, _changeLogFeatures); + _logSettingSet(setting); } } -void FontConfig::_logSettingSet(std::string_view setting, auto& value) +void FontConfig::LogSettingChanges(std::set& changes, std::string_view& context) const { - OutputDebugStringA(setting.data()); - OutputDebugStringA(" - "); - std::wstring val{ _convertVal(value).c_str() }; - _changeLog.insert_or_assign(setting, std::wstring_view{ val }); + for (const auto& setting : _changeLog) + { + changes.emplace(fmt::format(FMT_COMPILE("{}.{}"), context, setting)); + } } diff --git a/src/cascadia/TerminalSettingsModel/FontConfig.h b/src/cascadia/TerminalSettingsModel/FontConfig.h index 8b0bb1d3f9d..756f64fdcf2 100644 --- a/src/cascadia/TerminalSettingsModel/FontConfig.h +++ b/src/cascadia/TerminalSettingsModel/FontConfig.h @@ -35,6 +35,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation static winrt::com_ptr CopyFontInfo(const FontConfig* source, winrt::weak_ref sourceProfile); Json::Value ToJson() const; void LayerJson(const Json::Value& json); + void LogSettingChanges(std::set& changes, std::string_view& context) const; Model::Profile SourceProfile(); @@ -45,11 +46,9 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation private: winrt::weak_ref _sourceProfile; - std::map _changeLog; - std::map _changeLogAxes; - std::map _changeLogFeatures; + std::set _changeLog; - void _logSettingSet(std::string_view setting, winrt::Windows::Foundation::Collections::IMap& value); - void _logSettingSet(std::string_view setting, auto& value); + void _logSettingSet(std::string_view setting); + void _logSettingIfSet(std::string_view setting, const bool isSet); }; } diff --git a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp index 4c8821e4e74..9da33f9fa47 100644 --- a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp +++ b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp @@ -128,23 +128,16 @@ winrt::com_ptr GlobalAppSettings::FromJson(const Json::Value& void GlobalAppSettings::LayerJson(const Json::Value& json, const OriginTag origin) { JsonUtils::GetValueForKey(json, DefaultProfileKey, _UnparsedDefaultProfile); - if (_UnparsedDefaultProfile.has_value()) - { - _logSettingSet(DefaultProfileKey, _UnparsedDefaultProfile.value()); - } + // GH#8076 - when adding enum values to this key, we also changed it from // "useTabSwitcher" to "tabSwitcherMode". Continue supporting // "useTabSwitcher", but prefer "tabSwitcherMode" JsonUtils::GetValueForKey(json, LegacyUseTabSwitcherModeKey, _TabSwitcherMode); #define GLOBAL_SETTINGS_LAYER_JSON(type, name, jsonKey, ...) \ - { \ - JsonUtils::GetValueForKey(json, jsonKey, _##name); \ - if (_##name.has_value()) \ - { \ - _logSettingSet(jsonKey, _##name.value()); \ - } \ - } + JsonUtils::GetValueForKey(json, jsonKey, _##name); \ + _logSettingIfSet(jsonKey, _##name.has_value()); + MTSM_GLOBAL_SETTINGS(GLOBAL_SETTINGS_LAYER_JSON) #undef GLOBAL_SETTINGS_LAYER_JSON @@ -161,10 +154,11 @@ void GlobalAppSettings::LayerJson(const Json::Value& json, const OriginTag origi } LayerActionsFrom(json, origin, true); + // TODO CARLOS: validate this works JsonUtils::GetValueForKey(json, LegacyReloadEnvironmentVariablesKey, _legacyReloadEnvironmentVariables); - if (json.find(&*LegacyReloadEnvironmentVariablesKey.cbegin(), (&*LegacyReloadEnvironmentVariablesKey.cbegin()) + LegacyReloadEnvironmentVariablesKey.size())) + if (json[LegacyReloadEnvironmentVariablesKey.data()]) { - _logSettingSet(LegacyReloadEnvironmentVariablesKey, _legacyReloadEnvironmentVariables); + _logSettingSet(LegacyReloadEnvironmentVariablesKey); } } @@ -332,131 +326,78 @@ bool GlobalAppSettings::ShouldUsePersistedLayout() const return FirstWindowPreference() == FirstWindowPreference::PersistedWindowLayout && !IsolatedMode(); } -winrt::hstring _convertVal(const winrt::Microsoft::Terminal::Control::GraphicsAPI val) -{ - OutputDebugString(L"GraphicsAPI\n"); - JsonUtils::ConversionTrait converter{}; - return winrt::to_hstring(converter.ToJson(val).asString()); -} - -winrt::hstring _convertVal(const winrt::Microsoft::Terminal::Control::TextMeasurement val) -{ - OutputDebugString(L"TextMeasurement\n"); - JsonUtils::ConversionTrait converter{}; - return winrt::to_hstring(converter.ToJson(val).asString()); -} - -winrt::hstring _convertVal(const winrt::Microsoft::Terminal::Settings::Model::NewTabPosition val) -{ - OutputDebugString(L"NewTabPos\n"); - JsonUtils::ConversionTrait converter{}; - return winrt::to_hstring(converter.ToJson(val).asString()); -} - -winrt::hstring _convertVal(const winrt::Microsoft::UI::Xaml::Controls::TabViewWidthMode val) -{ - OutputDebugString(L"TabViewWidthMode\n"); - JsonUtils::ConversionTrait converter{}; - return winrt::to_hstring(converter.ToJson(val).asString()); -} - -winrt::hstring _convertVal(const winrt::Microsoft::Terminal::Control::CopyFormat val) -{ - OutputDebugString(L"CopyFormat\n"); - JsonUtils::ConversionTrait converter{}; - return winrt::to_hstring(converter.ToJson(val).asString()); -} - -winrt::hstring _convertVal(const winrt::Microsoft::Terminal::Settings::Model::LaunchPosition val) -{ - OutputDebugString(L"LaunchPos\n"); - JsonUtils::ConversionTrait converter{}; - return winrt::to_hstring(converter.ToJson(val).asString()); -} - -winrt::hstring _convertVal(const winrt::Microsoft::Terminal::Settings::Model::FirstWindowPreference val) -{ - OutputDebugString(L"FirstWindowPref\n"); - JsonUtils::ConversionTrait converter{}; - return winrt::to_hstring(converter.ToJson(val).asString()); -} - -winrt::hstring _convertVal(const winrt::Microsoft::Terminal::Settings::Model::LaunchMode val) -{ - OutputDebugString(L"LaunchMode\n"); - JsonUtils::ConversionTrait converter{}; - return winrt::to_hstring(converter.ToJson(val).asString()); -} - -winrt::hstring _convertVal(const winrt::Microsoft::Terminal::Settings::Model::TabSwitcherMode val) -{ - OutputDebugString(L"TabSwitcherMode\n"); - JsonUtils::ConversionTrait converter{}; - return winrt::to_hstring(converter.ToJson(val).asString()); -} - -winrt::hstring _convertVal(const winrt::Microsoft::Terminal::Settings::Model::WindowingMode val) +void GlobalAppSettings::_logSettingSet(std::string_view setting) { - OutputDebugString(L"WindowingMode\n"); - JsonUtils::ConversionTrait converter{}; - return winrt::to_hstring(converter.ToJson(val).asString()); -} - -winrt::hstring _convertVal(const winrt::guid& val) -{ - OutputDebugString(L"guid\n"); - JsonUtils::ConversionTrait converter{}; - return winrt::to_hstring(converter.ToJson(val).asString()); -} - -winrt::hstring _convertVal(const winrt::Microsoft::Terminal::Settings::Model::ThemePair val) -{ - OutputDebugString(L"ThemePair\n"); - // TODO CARLOS: no conversion trait - JsonUtils::ConversionTrait converter{}; - return winrt::to_hstring(converter.ToJson(val).asString()); -} - -winrt::hstring _convertVal(const winrt::Windows::Foundation::Collections::IVector val) -{ - OutputDebugString(L"vector\n"); - - // convert and sort so we normalize results - std::vector vec; - vec.reserve(val.Size()); - val.GetMany(0, vec); - std::sort(vec.begin(), vec.end()); - - // consolidate into a single string - winrt::hstring result; - for (auto iter = vec.begin(); iter != vec.end(); iter++) + if (setting == "theme") { - result = result + *iter; - if (iter + 1 != vec.end()) + if (_Theme.has_value()) { - result = result + L", "; + // ThemePair always has a Dark/Light value, + // so we need to check if they were explicitly set + if (_Theme->DarkName() == _Theme->LightName()) + { + _changeLog.insert(setting); + } + else + { + _changeLog.insert(std::string_view{ fmt::format(FMT_COMPILE("{}.{}"), setting, "dark") }); + _changeLog.insert(std::string_view{ fmt::format(FMT_COMPILE("{}.{}"), setting, "light") }); + } } } - return result; -} - -winrt::hstring _convertVal(const winrt::Windows::Foundation::Collections::IVector /*value*/) -{ - OutputDebugString(L"vector\n"); - // TODO CARLOS: what data do we even want to collect here? - return L""; + else if (setting == "newTabMenu") + { + if (_NewTabMenu.has_value()) + { + for (const auto& entry : *_NewTabMenu) + { + std::string entryType; + switch (entry.Type()) + { + case NewTabMenuEntryType::Profile: + entryType = "profile"; + break; + case NewTabMenuEntryType::Separator: + entryType = "separator"; + break; + case NewTabMenuEntryType::Folder: + entryType = "folder"; + break; + case NewTabMenuEntryType::RemainingProfiles: + entryType = "remainingProfiles"; + break; + case NewTabMenuEntryType::MatchProfiles: + entryType = "matchProfiles"; + break; + case NewTabMenuEntryType::Action: + entryType = "action"; + break; + case NewTabMenuEntryType::Invalid: + // ignore invalid + continue; + } + _changeLog.insert(std::string_view{ fmt::format(FMT_COMPILE("{}.{}"), setting, entryType) }); + } + } + } + else + { + _changeLog.insert(setting); + } } -winrt::hstring _convertVal(auto& val) +void GlobalAppSettings::_logSettingIfSet(std::string_view setting, const bool isSet) { - OutputDebugString(L"auto\n"); - return winrt::to_hstring(val); + if (isSet) + { + _logSettingSet(setting); + } } -void GlobalAppSettings::_logSettingSet(std::string_view setting, auto& value) +void GlobalAppSettings::LogSettingChanges(std::set& changes, std::string_view& context) const { - OutputDebugStringA(setting.data()); - OutputDebugStringA(" - "); - std::wstring val{ _convertVal(value).c_str() }; - _changeLog.insert_or_assign(setting, std::wstring_view{ val }); + for (const auto& setting : _changeLog) + { + changes.emplace(fmt::format(FMT_COMPILE("{}.{}"), context, setting)); + } } diff --git a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.h b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.h index 164722d8b48..a866fb6e1a8 100644 --- a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.h +++ b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.h @@ -72,6 +72,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation bool LegacyReloadEnvironmentVariables() const noexcept { return _legacyReloadEnvironmentVariables; } + void LogSettingChanges(std::set& changes, std::string_view& context) const; + INHERITABLE_SETTING(Model::GlobalAppSettings, hstring, UnparsedDefaultProfile, L""); #define GLOBAL_SETTINGS_INITIALIZE(type, name, jsonKey, ...) \ @@ -89,12 +91,13 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation winrt::guid _defaultProfile{}; bool _legacyReloadEnvironmentVariables{ true }; winrt::com_ptr _actionMap{ winrt::make_self() }; - std::map _changeLog; + std::set _changeLog; std::vector _keybindingsWarnings; Windows::Foundation::Collections::IMap _colorSchemes{ winrt::single_threaded_map() }; Windows::Foundation::Collections::IMap _themes{ winrt::single_threaded_map() }; - void _logSettingSet(std::string_view setting, auto& value); + void _logSettingSet(std::string_view setting); + void _logSettingIfSet(std::string_view setting, const bool isSet); }; } diff --git a/src/cascadia/TerminalSettingsModel/IInheritable.h b/src/cascadia/TerminalSettingsModel/IInheritable.h index ab57ff411ed..1099c128bc3 100644 --- a/src/cascadia/TerminalSettingsModel/IInheritable.h +++ b/src/cascadia/TerminalSettingsModel/IInheritable.h @@ -199,9 +199,9 @@ public: /* Overwrite the user set value */ \ void name(const type& value) \ { \ - if (name() != value) \ + if (!_##name.has_value() || _##name.value() != value) \ { \ - _logSettingSet(jsonKey, value); \ + _logSettingSet(jsonKey); \ } \ _##name = value; \ } diff --git a/src/cascadia/TerminalSettingsModel/Profile.cpp b/src/cascadia/TerminalSettingsModel/Profile.cpp index d66d94612b8..3bc04b8e1de 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.cpp +++ b/src/cascadia/TerminalSettingsModel/Profile.cpp @@ -176,30 +176,19 @@ void Profile::LayerJson(const Json::Value& json) JsonUtils::GetValueForKey(json, UpdatesKey, _Updates); JsonUtils::GetValueForKey(json, GuidKey, _Guid); JsonUtils::GetValueForKey(json, HiddenKey, _Hidden); - if (_Hidden.has_value()) - { - _logSettingSet(HiddenKey, _Hidden.value()); - } + _logSettingIfSet(HiddenKey, _Hidden.has_value()); + JsonUtils::GetValueForKey(json, SourceKey, _Source); JsonUtils::GetValueForKey(json, IconKey, _Icon); - if (_Icon.has_value()) - { - _logSettingSet(IconKey, _Icon.value()); - } + _logSettingIfSet(IconKey, _Icon.has_value()); // Padding was never specified as an integer, but it was a common working mistake. // Allow it to be permissive. JsonUtils::GetValueForKey(json, PaddingKey, _Padding, JsonUtils::OptionalConverter>{}); - if (_Padding.has_value()) - { - _logSettingSet(PaddingKey, _Padding.value()); - } + _logSettingIfSet(PaddingKey, _Padding.has_value()); JsonUtils::GetValueForKey(json, TabColorKey, _TabColor); - if (_TabColor.has_value()) - { - _logSettingSet(TabColorKey, _TabColor.value()); - } + _logSettingIfSet(TabColorKey, _TabColor.has_value()); // Try to load some legacy keys, to migrate them. // Done _before_ the MTSM_PROFILE_SETTINGS, which have the updated keys. @@ -207,13 +196,9 @@ void Profile::LayerJson(const Json::Value& json) JsonUtils::GetValueForKey(json, LegacyAutoMarkPromptsKey, _AutoMarkPrompts); #define PROFILE_SETTINGS_LAYER_JSON(type, name, jsonKey, ...) \ - { \ - JsonUtils::GetValueForKey(json, jsonKey, _##name); \ - if (_##name.has_value()) \ - { \ - _logSettingSet(jsonKey, _##name.value()); \ - } \ - } \ + JsonUtils::GetValueForKey(json, jsonKey, _##name); \ + _logSettingIfSet(jsonKey, _##name.has_value()); + MTSM_PROFILE_SETTINGS(PROFILE_SETTINGS_LAYER_JSON) #undef PROFILE_SETTINGS_LAYER_JSON @@ -229,6 +214,8 @@ void Profile::LayerJson(const Json::Value& json) unfocusedAppearance->LayerJson(json[JsonKey(UnfocusedAppearanceKey)]); _UnfocusedAppearance = *unfocusedAppearance; + + _logSettingSet(UnfocusedAppearanceKey); } } @@ -540,92 +527,36 @@ std::wstring Profile::NormalizeCommandLine(LPCWSTR commandLine) return normalized; } -winrt::hstring _convertVal(const winrt::Microsoft::Terminal::Control::ScrollbarState& val) -{ - OutputDebugString(L"ScrollbarState\n"); - JsonUtils::ConversionTrait converter{}; - return winrt::to_hstring(converter.ToJson(val).asString()); -} - -winrt::hstring _convertVal(const winrt::Microsoft::Terminal::Control::TextAntialiasingMode& val) -{ - OutputDebugString(L"TextAntialiasingMode\n"); - JsonUtils::ConversionTrait converter{}; - return winrt::to_hstring(converter.ToJson(val).asString()); -} - -winrt::hstring _convertVal(const winrt::Microsoft::Terminal::Settings::Model::CloseOnExitMode& val) -{ - OutputDebugString(L"CloseOnExitMode\n"); - JsonUtils::ConversionTrait converter{}; - return winrt::to_hstring(converter.ToJson(val).asString()); -} - -winrt::hstring _convertVal(const winrt::Microsoft::Terminal::Settings::Model::BellStyle& val) +void Profile::_logSettingSet(std::string_view setting) { - OutputDebugString(L"BellStyle\n"); - JsonUtils::ConversionTrait converter{}; - return winrt::to_hstring(converter.ToJson(val).asString()); + _changeLog.insert(setting); } -winrt::hstring _convertVal(const IEnvironmentVariableMap& /*val*/) +void Profile::_logSettingIfSet(std::string_view setting, const bool isSet) { - // TODO CARLOS: we probably don't want this? - // if we do, I can handle it similar to Vector - OutputDebugString(L"IEnvironmentVariableMap\n"); - return L""; -} - -winrt::hstring _convertVal(const winrt::Windows::Foundation::Collections::IVector val) -{ - OutputDebugString(L"vector\n"); - - // convert and sort so we normalize results - std::vector vec; - vec.reserve(val.Size()); - val.GetMany(0, vec); - std::sort(vec.begin(), vec.end()); - - // consolidate into a single string - winrt::hstring result; - for (auto iter = vec.begin(); iter != vec.end(); iter++) + if (isSet) { - result = result + *iter; - if (iter + 1 != vec.end()) - { - result = result + L", "; - } + _logSettingSet(setting); } - return result; -} - -winrt::hstring _convertVal(const std::optional val) -{ - // TODO CARLOS: test this one specifically - // - std::nullopt -> "null" - // - color -> something - OutputDebugString(L"optional\n"); - JsonUtils::ConversionTrait converter{}; - return winrt::to_hstring(converter.ToJson(val).asString()); } -winrt::hstring _convertVal(const winrt::guid& val) +void Profile::LogSettingChanges(std::set& changes, std::string_view& context) const { - OutputDebugString(L"guid\n"); - JsonUtils::ConversionTrait converter{}; - return winrt::to_hstring(converter.ToJson(val).asString()); -} + for (const auto& setting : _changeLog) + { + changes.emplace(fmt::format(FMT_COMPILE("{}.{}"), context, setting)); + } -winrt::hstring _convertVal(auto& val) -{ - OutputDebugString(L"auto\n"); - return winrt::to_hstring(val); -} + std::string_view fontContext{ fmt::format(FMT_COMPILE("{}.{}"), context, FontInfoKey) }; + winrt::get_self(_FontInfo)->LogSettingChanges(changes, fontContext); -void Profile::_logSettingSet(std::string_view setting, auto& value) -{ - OutputDebugStringA(setting.data()); - OutputDebugStringA(" - "); - std::wstring val{ _convertVal(value).c_str() }; - _changeLog.insert_or_assign(setting, std::wstring_view{ val }); + // We don't want to distinguish between "profile.defaultAppearance.*" and "profile.unfocusedAppearance.*" settings, + // but we still want to aggregate all of the appearance settings from both appearances. + // Log them as "profile.appearance.*" + std::string_view appContext{ fmt::format(FMT_COMPILE("{}.{}"), context, "appearance") }; + winrt::get_self(_DefaultAppearance)->LogSettingChanges(changes, appContext); + if (_UnfocusedAppearance) + { + winrt::get_self(*_UnfocusedAppearance)->LogSettingChanges(changes, appContext); + } } diff --git a/src/cascadia/TerminalSettingsModel/Profile.h b/src/cascadia/TerminalSettingsModel/Profile.h index ddf4ab88111..0873b298352 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.h +++ b/src/cascadia/TerminalSettingsModel/Profile.h @@ -108,6 +108,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation void _FinalizeInheritance() override; + void LogSettingChanges(std::set& changes, std::string_view& context) const; + // Special fields hstring Icon() const; void Icon(const hstring& value); @@ -140,14 +142,15 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation Model::FontConfig _FontInfo{ winrt::make(weak_ref(*this)) }; std::optional _evaluatedIcon{ std::nullopt }; - std::map _changeLog; + std::set _changeLog; static std::wstring EvaluateStartingDirectory(const std::wstring& directory); static guid _GenerateGuidForProfile(const std::wstring_view& name, const std::wstring_view& source) noexcept; winrt::hstring _evaluateIcon() const; - void _logSettingSet(std::string_view setting, auto& value); + void _logSettingSet(std::string_view setting); + void _logSettingIfSet(std::string_view setting, const bool isSet); friend class SettingsModelUnitTests::DeserializationTests; friend class SettingsModelUnitTests::ProfileTests; diff --git a/src/cascadia/TerminalSettingsModel/Theme.cpp b/src/cascadia/TerminalSettingsModel/Theme.cpp index 52c283a7433..bbb92dd3423 100644 --- a/src/cascadia/TerminalSettingsModel/Theme.cpp +++ b/src/cascadia/TerminalSettingsModel/Theme.cpp @@ -292,6 +292,37 @@ winrt::com_ptr Theme::FromJson(const Json::Value& json) return result; } +void Theme::LogSettingChanges(std::set& changes, std::string_view& context) +{ +#define GET_INNER_THEME_SETTING(type, name, jsonKey, ...) \ + if (isSet) \ + changes.insert(fmt::format(FMT_COMPILE("{}.{}.{}"), context, outerJsonKey, jsonKey)); + +#define GET_OUTER_THEME_SETTING(type, name, jsonKey, ...) \ + const bool is##name##Set = _##name != nullptr; \ + std::string_view outer##name##JsonKey = jsonKey; + + MTSM_THEME_SETTINGS(GET_OUTER_THEME_SETTING) + + bool isSet = isWindowSet; + std::string_view outerJsonKey = outerWindowJsonKey; + MTSM_THEME_WINDOW_SETTINGS(GET_INNER_THEME_SETTING) + + isSet = isSettingsSet; + outerJsonKey = outerSettingsJsonKey; + MTSM_THEME_SETTINGS_SETTINGS(GET_INNER_THEME_SETTING) + + isSet = isTabRowSet; + outerJsonKey = outerTabRowJsonKey; + MTSM_THEME_TABROW_SETTINGS(GET_INNER_THEME_SETTING) + + isSet = isTabSet; + outerJsonKey = outerTabJsonKey; + MTSM_THEME_TAB_SETTINGS(GET_INNER_THEME_SETTING) +#undef GET_INNER_THEME_SETTING +#undef GET_OUTER_THEME_SETTING +} + // Method Description: // - Create a new serialized JsonObject from an instance of this class // Arguments: diff --git a/src/cascadia/TerminalSettingsModel/Theme.h b/src/cascadia/TerminalSettingsModel/Theme.h index 2c0ac139aa9..5e918a3e15c 100644 --- a/src/cascadia/TerminalSettingsModel/Theme.h +++ b/src/cascadia/TerminalSettingsModel/Theme.h @@ -79,6 +79,9 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation Json::Value ToJson(); \ \ macro(THEME_SETTINGS_INITIALIZE); \ + \ + private: \ + std::set _changeLog; \ }; THEME_OBJECT(WindowTheme, MTSM_THEME_WINDOW_SETTINGS); @@ -97,8 +100,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation hstring ToString(); static com_ptr FromJson(const Json::Value& json); - void LayerJson(const Json::Value& json); Json::Value ToJson() const; + void LogSettingChanges(std::set& changes, std::string_view& context); winrt::Windows::UI::Xaml::ElementTheme RequestedTheme() const noexcept; From 19a3decfe813ad7cc59d71a1ac90c6757500cf96 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Tue, 6 Aug 2024 16:42:39 -0700 Subject: [PATCH 03/16] fix themes --- .../CascadiaSettingsSerialization.cpp | 6 +- src/cascadia/TerminalSettingsModel/Theme.cpp | 56 ++++++++++++------- 2 files changed, 38 insertions(+), 24 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp index 796ff418f32..643a05bdfb1 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp @@ -643,7 +643,8 @@ void SettingsLoader::_parse(const OriginTag origin, const winrt::hstring& source // versions of these themes overriding the built-in ones. continue; } - else + + if (origin != OriginTag::InBox) { static std::string_view themesContext{ "themes" }; theme->LogSettingChanges(settings.themesChangeLog, themesContext); @@ -1630,9 +1631,6 @@ void CascadiaSettings::LogSettingChanges(bool isJsonLoad) const // report changes for (const auto& change : changes) { - OutputDebugStringA(change.data()); - OutputDebugStringA("\n"); - // A `isJsonLoad ? "JsonSettingsChanged" : "UISettingsChanged"` // would be nice, but that apparently isn't allowed in the macro below. // Also, there's guidance to not send too much data all in one event, diff --git a/src/cascadia/TerminalSettingsModel/Theme.cpp b/src/cascadia/TerminalSettingsModel/Theme.cpp index bbb92dd3423..98f39bd6f95 100644 --- a/src/cascadia/TerminalSettingsModel/Theme.cpp +++ b/src/cascadia/TerminalSettingsModel/Theme.cpp @@ -294,33 +294,49 @@ winrt::com_ptr Theme::FromJson(const Json::Value& json) void Theme::LogSettingChanges(std::set& changes, std::string_view& context) { -#define GET_INNER_THEME_SETTING(type, name, jsonKey, ...) \ - if (isSet) \ - changes.insert(fmt::format(FMT_COMPILE("{}.{}.{}"), context, outerJsonKey, jsonKey)); +#pragma warning(push) +#pragma warning(disable : 5103) // pasting '{' and 'winrt' does not result in a valid preprocessing token -#define GET_OUTER_THEME_SETTING(type, name, jsonKey, ...) \ - const bool is##name##Set = _##name != nullptr; \ +#define GENERATE_SET_CHECK_AND_JSON_KEYS(type, name, jsonKey, ...) \ + const bool is##name##Set = _##name != nullptr; \ std::string_view outer##name##JsonKey = jsonKey; - MTSM_THEME_SETTINGS(GET_OUTER_THEME_SETTING) + MTSM_THEME_SETTINGS(GENERATE_SET_CHECK_AND_JSON_KEYS) - bool isSet = isWindowSet; - std::string_view outerJsonKey = outerWindowJsonKey; - MTSM_THEME_WINDOW_SETTINGS(GET_INNER_THEME_SETTING) +#define LOG_IF_SET(type, name, jsonKey, ...) \ + if (obj.name() != type{##__VA_ARGS__ }) \ + changes.insert(fmt::format(FMT_COMPILE("{}.{}.{}"), context, outerJsonKey, jsonKey)); - isSet = isSettingsSet; - outerJsonKey = outerSettingsJsonKey; - MTSM_THEME_SETTINGS_SETTINGS(GET_INNER_THEME_SETTING) + if (isWindowSet) + { + const auto obj = _Window; + const auto outerJsonKey = outerWindowJsonKey; + MTSM_THEME_WINDOW_SETTINGS(LOG_IF_SET) + } - isSet = isTabRowSet; - outerJsonKey = outerTabRowJsonKey; - MTSM_THEME_TABROW_SETTINGS(GET_INNER_THEME_SETTING) + if (isSettingsSet) + { + const auto obj = _Settings; + const auto outerJsonKey = outerSettingsJsonKey; + MTSM_THEME_SETTINGS_SETTINGS(LOG_IF_SET) + } - isSet = isTabSet; - outerJsonKey = outerTabJsonKey; - MTSM_THEME_TAB_SETTINGS(GET_INNER_THEME_SETTING) -#undef GET_INNER_THEME_SETTING -#undef GET_OUTER_THEME_SETTING + if (isTabRowSet) + { + const auto obj = _TabRow; + const auto outerJsonKey = outerTabRowJsonKey; + MTSM_THEME_TABROW_SETTINGS(LOG_IF_SET) + } + + if (isTabSet) + { + const auto obj = _Tab; + const auto outerJsonKey = outerTabJsonKey; + MTSM_THEME_TAB_SETTINGS(LOG_IF_SET) + } +#undef LOG_IF_SET +#undef GENERATE_SET_CHECK_AND_JSON_KEYS +#pragma warning(pop) } // Method Description: From 34c93d10777585ebd98eaece30af9d53e0903ce7 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Tue, 6 Aug 2024 16:44:23 -0700 Subject: [PATCH 04/16] code format --- src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp index 9da33f9fa47..a78f7f2c540 100644 --- a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp +++ b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp @@ -373,7 +373,7 @@ void GlobalAppSettings::_logSettingSet(std::string_view setting) entryType = "action"; break; case NewTabMenuEntryType::Invalid: - // ignore invalid + // ignore invalid continue; } _changeLog.insert(std::string_view{ fmt::format(FMT_COMPILE("{}.{}"), setting, entryType) }); From d0ba995a318600a54bb5803f0c0f0d8f3a914030 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Tue, 6 Aug 2024 16:57:26 -0700 Subject: [PATCH 05/16] remove dead code --- src/cascadia/TerminalSettingsEditor/LaunchViewModel.cpp | 5 ----- src/cascadia/TerminalSettingsEditor/pch.h | 7 ------- src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp | 1 - 3 files changed, 13 deletions(-) diff --git a/src/cascadia/TerminalSettingsEditor/LaunchViewModel.cpp b/src/cascadia/TerminalSettingsEditor/LaunchViewModel.cpp index 172db128f92..425d84299b9 100644 --- a/src/cascadia/TerminalSettingsEditor/LaunchViewModel.cpp +++ b/src/cascadia/TerminalSettingsEditor/LaunchViewModel.cpp @@ -92,8 +92,6 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation const LaunchPosition newPos{ xCoordRef, _Settings.GlobalSettings().InitialPosition().Y }; _Settings.GlobalSettings().InitialPosition(newPos); _NotifyChanges(L"LaunchParametersCurrentValue"); - - // TODO CARLOS: telemetry for InitialPos } void LaunchViewModel::InitialPosY(double yCoord) @@ -107,8 +105,6 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation const LaunchPosition newPos{ _Settings.GlobalSettings().InitialPosition().X, yCoordRef }; _Settings.GlobalSettings().InitialPosition(newPos); _NotifyChanges(L"LaunchParametersCurrentValue"); - - // TODO CARLOS: telemetry for InitialPos } void LaunchViewModel::UseDefaultLaunchPosition(bool useDefaultPosition) @@ -120,7 +116,6 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation InitialPosY(NAN); } _NotifyChanges(L"UseDefaultLaunchPosition", L"LaunchParametersCurrentValue", L"InitialPosX", L"InitialPosY"); - // TODO CARLOS: telemetry for InitialPos } bool LaunchViewModel::UseDefaultLaunchPosition() diff --git a/src/cascadia/TerminalSettingsEditor/pch.h b/src/cascadia/TerminalSettingsEditor/pch.h index fe8635709d7..2e9eba2f51a 100644 --- a/src/cascadia/TerminalSettingsEditor/pch.h +++ b/src/cascadia/TerminalSettingsEditor/pch.h @@ -51,13 +51,6 @@ #include #include -// Including TraceLogging essentials for the binary -#include -#include -TRACELOGGING_DECLARE_PROVIDER(g_hSettingsEditorProvider); -#include -#include - #include #include #include diff --git a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp index a78f7f2c540..9e5a0d5bfa6 100644 --- a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp +++ b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp @@ -154,7 +154,6 @@ void GlobalAppSettings::LayerJson(const Json::Value& json, const OriginTag origi } LayerActionsFrom(json, origin, true); - // TODO CARLOS: validate this works JsonUtils::GetValueForKey(json, LegacyReloadEnvironmentVariablesKey, _legacyReloadEnvironmentVariables); if (json[LegacyReloadEnvironmentVariablesKey.data()]) { From 7bb991f69d98f661eb33281f36b1db5bb4f5e028 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Wed, 7 Aug 2024 10:44:45 -0700 Subject: [PATCH 06/16] use strings instead of string_view --- .../TerminalSettingsModel/ActionMap.h | 6 +-- .../ActionMapSerialization.cpp | 2 +- .../AppearanceConfig.cpp | 10 ++-- .../TerminalSettingsModel/AppearanceConfig.h | 8 ++-- .../TerminalSettingsModel/CascadiaSettings.h | 7 ++- .../CascadiaSettingsSerialization.cpp | 47 ++++++++++--------- .../TerminalSettingsModel/Command.cpp | 12 ++--- src/cascadia/TerminalSettingsModel/Command.h | 2 +- .../TerminalSettingsModel/FontConfig.cpp | 6 +-- .../TerminalSettingsModel/FontConfig.h | 8 ++-- .../GlobalAppSettings.cpp | 18 +++---- .../TerminalSettingsModel/GlobalAppSettings.h | 8 ++-- .../TerminalSettingsModel/Profile.cpp | 16 +++---- src/cascadia/TerminalSettingsModel/Profile.h | 8 ++-- src/cascadia/TerminalSettingsModel/Theme.cpp | 2 +- src/cascadia/TerminalSettingsModel/Theme.h | 5 +- 16 files changed, 81 insertions(+), 84 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/ActionMap.h b/src/cascadia/TerminalSettingsModel/ActionMap.h index af9951be475..1ac4c86d13d 100644 --- a/src/cascadia/TerminalSettingsModel/ActionMap.h +++ b/src/cascadia/TerminalSettingsModel/ActionMap.h @@ -73,7 +73,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation Json::Value ToJson() const; Json::Value KeyBindingsToJson() const; bool FixupsAppliedDuringLoad() const; - void LogSettingChanges(std::set& changes, std::string_view& context) const; + void LogSettingChanges(std::set& changes, const std::string& context) const; // modification bool RebindKeys(const Control::KeyChord& oldKeys, const Control::KeyChord& newKeys); @@ -105,8 +105,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation std::vector _updateLocalSnippetCache(winrt::hstring currentWorkingDirectory); - void _logCommand(const Model::Command& cmd); - Windows::Foundation::Collections::IMap _AvailableActionsCache{ nullptr }; Windows::Foundation::Collections::IMap _NameMapCache{ nullptr }; Windows::Foundation::Collections::IMap _GlobalHotkeysCache{ nullptr }; @@ -141,7 +139,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation til::shared_mutex>> _cwdLocalSnippetsCache{}; - std::set _changeLog; + std::set _changeLog; friend class SettingsModelUnitTests::KeyBindingsTests; friend class SettingsModelUnitTests::DeserializationTests; diff --git a/src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp b/src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp index 413e86eff5e..bac936fa3b2 100644 --- a/src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp +++ b/src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp @@ -159,7 +159,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation return keybindingsList; } - void ActionMap::LogSettingChanges(std::set& changes, std::string_view& context) const + void ActionMap::LogSettingChanges(std::set& changes, const std::string& context) const { for (const auto& setting : _changeLog) { diff --git a/src/cascadia/TerminalSettingsModel/AppearanceConfig.cpp b/src/cascadia/TerminalSettingsModel/AppearanceConfig.cpp index 85e206252f9..5da66cb97a9 100644 --- a/src/cascadia/TerminalSettingsModel/AppearanceConfig.cpp +++ b/src/cascadia/TerminalSettingsModel/AppearanceConfig.cpp @@ -110,7 +110,7 @@ void AppearanceConfig::LayerJson(const Json::Value& json) // to make the UI happy, set ColorSchemeName. JsonUtils::GetValueForKey(json, ColorSchemeKey, _DarkColorSchemeName); _LightColorSchemeName = _DarkColorSchemeName; - _logSettingSet(ColorSchemeKey); + _logSettingSet(std::string{ ColorSchemeKey }); } else if (json["colorScheme"].isObject()) { @@ -172,20 +172,20 @@ winrt::hstring AppearanceConfig::ExpandedBackgroundImagePath() } } -void AppearanceConfig::_logSettingSet(std::string_view setting) +void AppearanceConfig::_logSettingSet(const std::string& setting) { _changeLog.emplace(setting); } -void AppearanceConfig::_logSettingIfSet(std::string_view setting, const bool isSet) +void AppearanceConfig::_logSettingIfSet(const std::string_view& setting, const bool isSet) { if (isSet) { - _logSettingSet(setting); + _logSettingSet(std::string{ setting }); } } -void AppearanceConfig::LogSettingChanges(std::set& changes, std::string_view& context) const +void AppearanceConfig::LogSettingChanges(std::set& changes, const std::string& context) const { for (const auto& setting : _changeLog) { diff --git a/src/cascadia/TerminalSettingsModel/AppearanceConfig.h b/src/cascadia/TerminalSettingsModel/AppearanceConfig.h index 9d9dceb554d..03442449544 100644 --- a/src/cascadia/TerminalSettingsModel/AppearanceConfig.h +++ b/src/cascadia/TerminalSettingsModel/AppearanceConfig.h @@ -31,7 +31,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation static winrt::com_ptr CopyAppearance(const AppearanceConfig* source, winrt::weak_ref sourceProfile); Json::Value ToJson() const; void LayerJson(const Json::Value& json); - void LogSettingChanges(std::set& changes, std::string_view& context) const; + void LogSettingChanges(std::set& changes, const std::string& context) const; Model::Profile SourceProfile(); @@ -53,9 +53,9 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation private: winrt::weak_ref _sourceProfile; - std::set _changeLog; + std::set _changeLog; - void _logSettingSet(std::string_view setting); - void _logSettingIfSet(std::string_view setting, const bool isSet); + void _logSettingSet(const std::string& setting); + void _logSettingIfSet(const std::string_view& setting, const bool isSet); }; } diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.h b/src/cascadia/TerminalSettingsModel/CascadiaSettings.h index 6fe062a96f1..75eb9ee96ee 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.h +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettings.h @@ -48,7 +48,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation std::unordered_map> colorSchemes; std::unordered_map colorSchemeRemappings; bool fixupsAppliedDuringLoad{ false }; - std::set themesChangeLog; + std::set themesChangeLog; void clear(); }; @@ -97,7 +97,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation void _executeGenerator(const IDynamicProfileGenerator& generator); std::unordered_set _ignoredNamespaces; - std::set themesChangeLog; + std::set themesChangeLog; // See _getNonUserOriginProfiles(). size_t _userProfileCount = 0; }; @@ -177,7 +177,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation void _validateThemeExists(); void _researchOnLoad(); - std::set _logSettingChanges() const; // user settings winrt::hstring _hash; @@ -185,7 +184,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation winrt::com_ptr _baseLayerProfile = winrt::make_self(); winrt::Windows::Foundation::Collections::IObservableVector _allProfiles = winrt::single_threaded_observable_vector(); winrt::Windows::Foundation::Collections::IObservableVector _activeProfiles = winrt::single_threaded_observable_vector(); - std::set _themesChangeLog{}; + std::set _themesChangeLog{}; // load errors winrt::Windows::Foundation::Collections::IVector _warnings = winrt::single_threaded_vector(); diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp index 643a05bdfb1..1844872befd 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp @@ -646,7 +646,7 @@ void SettingsLoader::_parse(const OriginTag origin, const winrt::hstring& source if (origin != OriginTag::InBox) { - static std::string_view themesContext{ "themes" }; + static std::string themesContext{ "themes" }; theme->LogSettingChanges(settings.themesChangeLog, themesContext); } settings.globals->AddTheme(*theme); @@ -1589,25 +1589,35 @@ void CascadiaSettings::_resolveNewTabMenuProfilesSet(const IVector CascadiaSettings::_logSettingChanges() const +void CascadiaSettings::LogSettingChanges(bool isJsonLoad) const { +#ifndef _DEBUG + // Only do this if we're actually being sampled + if (!TraceLoggingProviderEnabled(g_hSettingsModelProvider, 0, MICROSOFT_KEYWORD_MEASURES)) + { + return; + } +#endif // !_DEBUG + // aggregate setting changes - std::set changes; - static std::string_view globalContext{ "global" }; + std::set changes; + static std::string globalContext{ "global" }; _globals->LogSettingChanges(changes, globalContext); - static std::string_view actionContext{ "action" }; + // Actions are not expected to change when loaded from the settings UI + static std::string actionContext{ "action" }; winrt::get_self(_globals->ActionMap())->LogSettingChanges(changes, actionContext); - static std::string_view profileContext{ "profile" }; + static std::string profileContext{ "profile" }; for (const auto& profile : _allProfiles) { winrt::get_self(profile)->LogSettingChanges(changes, profileContext); } - static std::string_view profileDefaultsContext{ "profileDefaults" }; + static std::string profileDefaultsContext{ "profileDefaults" }; _baseLayerProfile->LogSettingChanges(changes, profileDefaultsContext); + // Themes are not expected to change when loaded from the settings UI // DO NOT CALL Theme::LogSettingChanges!! // We already collected the changes when we loaded the JSON for (const auto& change : _themesChangeLog) @@ -1615,22 +1625,10 @@ std::set CascadiaSettings::_logSettingChanges() const changes.insert(change); } - return changes; -} - -void CascadiaSettings::LogSettingChanges(bool isJsonLoad) const -{ - // Only do this if we're actually being sampled - if (!TraceLoggingProviderEnabled(g_hSettingsModelProvider, 0, MICROSOFT_KEYWORD_MEASURES)) - { - return; - } - - const auto changes = _logSettingChanges(); - // report changes for (const auto& change : changes) { +#ifndef _DEBUG // A `isJsonLoad ? "JsonSettingsChanged" : "UISettingsChanged"` // would be nice, but that apparently isn't allowed in the macro below. // Also, there's guidance to not send too much data all in one event, @@ -1639,7 +1637,7 @@ void CascadiaSettings::LogSettingChanges(bool isJsonLoad) const { TraceLoggingWrite(g_hSettingsModelProvider, "JsonSettingsChanged", - TraceLoggingDescription("Event emitted when settings change"), + TraceLoggingDescription("Event emitted when settings.json change"), TraceLoggingValue(change.data()), TraceLoggingKeyword(MICROSOFT_KEYWORD_MEASURES), TelemetryPrivacyDataTag(PDT_ProductAndServiceUsage)); @@ -1648,10 +1646,15 @@ void CascadiaSettings::LogSettingChanges(bool isJsonLoad) const { TraceLoggingWrite(g_hSettingsModelProvider, "UISettingsChanged", - TraceLoggingDescription("Event emitted when settings change"), + TraceLoggingDescription("Event emitted when settings change via the UI"), TraceLoggingValue(change.data()), TraceLoggingKeyword(MICROSOFT_KEYWORD_MEASURES), TelemetryPrivacyDataTag(PDT_ProductAndServiceUsage)); } +#else + OutputDebugStringA(isJsonLoad ? "JsonSettingsChanged\n" : "UISettingsChanged\n"); + OutputDebugStringA(change.data()); + OutputDebugStringA("\n"); +#endif // !_DEBUG } } diff --git a/src/cascadia/TerminalSettingsModel/Command.cpp b/src/cascadia/TerminalSettingsModel/Command.cpp index 1bc362c7a89..474d0667aec 100644 --- a/src/cascadia/TerminalSettingsModel/Command.cpp +++ b/src/cascadia/TerminalSettingsModel/Command.cpp @@ -742,29 +742,29 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation return winrt::single_threaded_vector(std::move(result)); } - void Command::LogSettingChanges(std::set& changes) + void Command::LogSettingChanges(std::set& changes) { if (_IterateOn != ExpandCommandType::None) { switch (_IterateOn) { case ExpandCommandType::Profiles: - changes.insert(fmt::format(FMT_COMPILE("{}.{}"), IterateOnKey, "profiles")); + changes.insert(fmt::format(FMT_COMPILE("{}.{}"), std::string{ IterateOnKey }, "profiles")); break; case ExpandCommandType::ColorSchemes: - changes.insert(fmt::format(FMT_COMPILE("{}.{}"), IterateOnKey, "schemes")); + changes.insert(fmt::format(FMT_COMPILE("{}.{}"), std::string{ IterateOnKey }, "schemes")); break; } } if (!_Description.empty()) { - changes.insert(fmt::format(FMT_COMPILE("{}"), DescriptionKey)); + changes.insert(fmt::format(FMT_COMPILE("{}"), std::string{ DescriptionKey })); } if (IsNestedCommand()) { - changes.insert(fmt::format(FMT_COMPILE("{}"), CommandsKey)); + changes.insert(fmt::format(FMT_COMPILE("{}"), std::string{ CommandsKey })); } else { @@ -782,7 +782,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // - "command": { "action": "copy", "singleLine": true } --> "copy.singleLine" // - "command": { "action": "copy", "singleLine": true, "dismissSelection": true } --> "copy.singleLine", "copy.dismissSelection" - std::string_view shortcutActionName{ json[JsonKey("action")].asString() }; + const std::string shortcutActionName{ json[JsonKey("action")].asString() }; auto members = json.getMemberNames(); members.erase(std::remove_if(members.begin(), members.end(), [](const auto& member) { return member == "action"; }), members.end()); diff --git a/src/cascadia/TerminalSettingsModel/Command.h b/src/cascadia/TerminalSettingsModel/Command.h index a45e63ae0b9..4d372bea9cb 100644 --- a/src/cascadia/TerminalSettingsModel/Command.h +++ b/src/cascadia/TerminalSettingsModel/Command.h @@ -61,7 +61,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation const Json::Value& json, const OriginTag origin); Json::Value ToJson() const; - void LogSettingChanges(std::set& changes); + void LogSettingChanges(std::set& changes); bool HasNestedCommands() const; bool IsNestedCommand() const noexcept; diff --git a/src/cascadia/TerminalSettingsModel/FontConfig.cpp b/src/cascadia/TerminalSettingsModel/FontConfig.cpp index 51abd76fd75..e0558a78119 100644 --- a/src/cascadia/TerminalSettingsModel/FontConfig.cpp +++ b/src/cascadia/TerminalSettingsModel/FontConfig.cpp @@ -110,7 +110,7 @@ winrt::Microsoft::Terminal::Settings::Model::Profile FontConfig::SourceProfile() return _sourceProfile.get(); } -void FontConfig::_logSettingSet(std::string_view setting) +void FontConfig::_logSettingSet(const std::string& setting) { if (setting == "axes" && _FontAxes.has_value()) { @@ -132,7 +132,7 @@ void FontConfig::_logSettingSet(std::string_view setting) } } -void FontConfig::_logSettingIfSet(std::string_view setting, const bool isSet) +void FontConfig::_logSettingIfSet(const std::string& setting, const bool isSet) { if (isSet) { @@ -140,7 +140,7 @@ void FontConfig::_logSettingIfSet(std::string_view setting, const bool isSet) } } -void FontConfig::LogSettingChanges(std::set& changes, std::string_view& context) const +void FontConfig::LogSettingChanges(std::set& changes, const std::string& context) const { for (const auto& setting : _changeLog) { diff --git a/src/cascadia/TerminalSettingsModel/FontConfig.h b/src/cascadia/TerminalSettingsModel/FontConfig.h index 756f64fdcf2..73e343d97b9 100644 --- a/src/cascadia/TerminalSettingsModel/FontConfig.h +++ b/src/cascadia/TerminalSettingsModel/FontConfig.h @@ -35,7 +35,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation static winrt::com_ptr CopyFontInfo(const FontConfig* source, winrt::weak_ref sourceProfile); Json::Value ToJson() const; void LayerJson(const Json::Value& json); - void LogSettingChanges(std::set& changes, std::string_view& context) const; + void LogSettingChanges(std::set& changes, const std::string& context) const; Model::Profile SourceProfile(); @@ -46,9 +46,9 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation private: winrt::weak_ref _sourceProfile; - std::set _changeLog; + std::set _changeLog; - void _logSettingSet(std::string_view setting); - void _logSettingIfSet(std::string_view setting, const bool isSet); + void _logSettingSet(const std::string& setting); + void _logSettingIfSet(const std::string& setting, const bool isSet); }; } diff --git a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp index 9e5a0d5bfa6..8b4bfcfdfab 100644 --- a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp +++ b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp @@ -157,7 +157,7 @@ void GlobalAppSettings::LayerJson(const Json::Value& json, const OriginTag origi JsonUtils::GetValueForKey(json, LegacyReloadEnvironmentVariablesKey, _legacyReloadEnvironmentVariables); if (json[LegacyReloadEnvironmentVariablesKey.data()]) { - _logSettingSet(LegacyReloadEnvironmentVariablesKey); + _logSettingSet(std::string{ LegacyReloadEnvironmentVariablesKey }); } } @@ -325,7 +325,7 @@ bool GlobalAppSettings::ShouldUsePersistedLayout() const return FirstWindowPreference() == FirstWindowPreference::PersistedWindowLayout && !IsolatedMode(); } -void GlobalAppSettings::_logSettingSet(std::string_view setting) +void GlobalAppSettings::_logSettingSet(const std::string& setting) { if (setting == "theme") { @@ -339,8 +339,8 @@ void GlobalAppSettings::_logSettingSet(std::string_view setting) } else { - _changeLog.insert(std::string_view{ fmt::format(FMT_COMPILE("{}.{}"), setting, "dark") }); - _changeLog.insert(std::string_view{ fmt::format(FMT_COMPILE("{}.{}"), setting, "light") }); + _changeLog.emplace(fmt::format(FMT_COMPILE("{}.{}"), setting, "dark")); + _changeLog.emplace(fmt::format(FMT_COMPILE("{}.{}"), setting, "light")); } } } @@ -375,25 +375,25 @@ void GlobalAppSettings::_logSettingSet(std::string_view setting) // ignore invalid continue; } - _changeLog.insert(std::string_view{ fmt::format(FMT_COMPILE("{}.{}"), setting, entryType) }); + _changeLog.emplace(fmt::format(FMT_COMPILE("{}.{}"), setting, entryType)); } } } else { - _changeLog.insert(setting); + _changeLog.emplace(setting); } } -void GlobalAppSettings::_logSettingIfSet(std::string_view setting, const bool isSet) +void GlobalAppSettings::_logSettingIfSet(const std::string& setting, const bool isSet) { if (isSet) { - _logSettingSet(setting); + _logSettingSet(std::string{ setting }); } } -void GlobalAppSettings::LogSettingChanges(std::set& changes, std::string_view& context) const +void GlobalAppSettings::LogSettingChanges(std::set& changes, const std::string& context) const { for (const auto& setting : _changeLog) { diff --git a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.h b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.h index a866fb6e1a8..51c601836b8 100644 --- a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.h +++ b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.h @@ -72,7 +72,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation bool LegacyReloadEnvironmentVariables() const noexcept { return _legacyReloadEnvironmentVariables; } - void LogSettingChanges(std::set& changes, std::string_view& context) const; + void LogSettingChanges(std::set& changes, const std::string& context) const; INHERITABLE_SETTING(Model::GlobalAppSettings, hstring, UnparsedDefaultProfile, L""); @@ -91,13 +91,13 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation winrt::guid _defaultProfile{}; bool _legacyReloadEnvironmentVariables{ true }; winrt::com_ptr _actionMap{ winrt::make_self() }; - std::set _changeLog; + std::set _changeLog; std::vector _keybindingsWarnings; Windows::Foundation::Collections::IMap _colorSchemes{ winrt::single_threaded_map() }; Windows::Foundation::Collections::IMap _themes{ winrt::single_threaded_map() }; - void _logSettingSet(std::string_view setting); - void _logSettingIfSet(std::string_view setting, const bool isSet); + void _logSettingSet(const std::string& setting); + void _logSettingIfSet(const std::string& setting, const bool isSet); }; } diff --git a/src/cascadia/TerminalSettingsModel/Profile.cpp b/src/cascadia/TerminalSettingsModel/Profile.cpp index 3bc04b8e1de..ce840f9d34b 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.cpp +++ b/src/cascadia/TerminalSettingsModel/Profile.cpp @@ -215,7 +215,7 @@ void Profile::LayerJson(const Json::Value& json) unfocusedAppearance->LayerJson(json[JsonKey(UnfocusedAppearanceKey)]); _UnfocusedAppearance = *unfocusedAppearance; - _logSettingSet(UnfocusedAppearanceKey); + _logSettingSet(std::string{ UnfocusedAppearanceKey }); } } @@ -527,33 +527,33 @@ std::wstring Profile::NormalizeCommandLine(LPCWSTR commandLine) return normalized; } -void Profile::_logSettingSet(std::string_view setting) +void Profile::_logSettingSet(const std::string& setting) { - _changeLog.insert(setting); + _changeLog.emplace(setting); } -void Profile::_logSettingIfSet(std::string_view setting, const bool isSet) +void Profile::_logSettingIfSet(const std::string_view& setting, const bool isSet) { if (isSet) { - _logSettingSet(setting); + _logSettingSet(std::string{ setting }); } } -void Profile::LogSettingChanges(std::set& changes, std::string_view& context) const +void Profile::LogSettingChanges(std::set& changes, const std::string& context) const { for (const auto& setting : _changeLog) { changes.emplace(fmt::format(FMT_COMPILE("{}.{}"), context, setting)); } - std::string_view fontContext{ fmt::format(FMT_COMPILE("{}.{}"), context, FontInfoKey) }; + std::string fontContext{ fmt::format(FMT_COMPILE("{}.{}"), context, FontInfoKey) }; winrt::get_self(_FontInfo)->LogSettingChanges(changes, fontContext); // We don't want to distinguish between "profile.defaultAppearance.*" and "profile.unfocusedAppearance.*" settings, // but we still want to aggregate all of the appearance settings from both appearances. // Log them as "profile.appearance.*" - std::string_view appContext{ fmt::format(FMT_COMPILE("{}.{}"), context, "appearance") }; + std::string appContext{ fmt::format(FMT_COMPILE("{}.{}"), context, "appearance") }; winrt::get_self(_DefaultAppearance)->LogSettingChanges(changes, appContext); if (_UnfocusedAppearance) { diff --git a/src/cascadia/TerminalSettingsModel/Profile.h b/src/cascadia/TerminalSettingsModel/Profile.h index 0873b298352..f0e5ddee0ca 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.h +++ b/src/cascadia/TerminalSettingsModel/Profile.h @@ -108,7 +108,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation void _FinalizeInheritance() override; - void LogSettingChanges(std::set& changes, std::string_view& context) const; + void LogSettingChanges(std::set& changes, const std::string& context) const; // Special fields hstring Icon() const; @@ -142,15 +142,15 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation Model::FontConfig _FontInfo{ winrt::make(weak_ref(*this)) }; std::optional _evaluatedIcon{ std::nullopt }; - std::set _changeLog; + std::set _changeLog; static std::wstring EvaluateStartingDirectory(const std::wstring& directory); static guid _GenerateGuidForProfile(const std::wstring_view& name, const std::wstring_view& source) noexcept; winrt::hstring _evaluateIcon() const; - void _logSettingSet(std::string_view setting); - void _logSettingIfSet(std::string_view setting, const bool isSet); + void _logSettingSet(const std::string& setting); + void _logSettingIfSet(const std::string_view& setting, const bool isSet); friend class SettingsModelUnitTests::DeserializationTests; friend class SettingsModelUnitTests::ProfileTests; diff --git a/src/cascadia/TerminalSettingsModel/Theme.cpp b/src/cascadia/TerminalSettingsModel/Theme.cpp index 98f39bd6f95..2f8ce9472c2 100644 --- a/src/cascadia/TerminalSettingsModel/Theme.cpp +++ b/src/cascadia/TerminalSettingsModel/Theme.cpp @@ -292,7 +292,7 @@ winrt::com_ptr Theme::FromJson(const Json::Value& json) return result; } -void Theme::LogSettingChanges(std::set& changes, std::string_view& context) +void Theme::LogSettingChanges(std::set& changes, std::string& context) { #pragma warning(push) #pragma warning(disable : 5103) // pasting '{' and 'winrt' does not result in a valid preprocessing token diff --git a/src/cascadia/TerminalSettingsModel/Theme.h b/src/cascadia/TerminalSettingsModel/Theme.h index 5e918a3e15c..305b5fd4090 100644 --- a/src/cascadia/TerminalSettingsModel/Theme.h +++ b/src/cascadia/TerminalSettingsModel/Theme.h @@ -79,9 +79,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation Json::Value ToJson(); \ \ macro(THEME_SETTINGS_INITIALIZE); \ - \ - private: \ - std::set _changeLog; \ }; THEME_OBJECT(WindowTheme, MTSM_THEME_WINDOW_SETTINGS); @@ -101,7 +98,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation static com_ptr FromJson(const Json::Value& json); Json::Value ToJson() const; - void LogSettingChanges(std::set& changes, std::string_view& context); + void LogSettingChanges(std::set& changes, std::string& context); winrt::Windows::UI::Xaml::ElementTheme RequestedTheme() const noexcept; From b0261753996fc67844f58225b2d6ad058e49d959 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Wed, 7 Aug 2024 12:17:20 -0700 Subject: [PATCH 07/16] use string_views where possible --- src/cascadia/TerminalSettingsModel/ActionMap.h | 2 +- .../TerminalSettingsModel/ActionMapSerialization.cpp | 2 +- .../TerminalSettingsModel/AppearanceConfig.cpp | 6 +++--- .../TerminalSettingsModel/AppearanceConfig.h | 4 ++-- .../CascadiaSettingsSerialization.cpp | 2 +- src/cascadia/TerminalSettingsModel/Command.cpp | 12 ++++++------ src/cascadia/TerminalSettingsModel/FontConfig.cpp | 6 +++--- src/cascadia/TerminalSettingsModel/FontConfig.h | 6 +++--- .../TerminalSettingsModel/GlobalAppSettings.cpp | 12 ++++++------ .../TerminalSettingsModel/GlobalAppSettings.h | 6 +++--- src/cascadia/TerminalSettingsModel/Profile.cpp | 8 ++++---- src/cascadia/TerminalSettingsModel/Profile.h | 4 ++-- src/cascadia/TerminalSettingsModel/Theme.cpp | 4 ++-- src/cascadia/TerminalSettingsModel/Theme.h | 2 +- 14 files changed, 38 insertions(+), 38 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/ActionMap.h b/src/cascadia/TerminalSettingsModel/ActionMap.h index 1ac4c86d13d..8da139a30d8 100644 --- a/src/cascadia/TerminalSettingsModel/ActionMap.h +++ b/src/cascadia/TerminalSettingsModel/ActionMap.h @@ -73,7 +73,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation Json::Value ToJson() const; Json::Value KeyBindingsToJson() const; bool FixupsAppliedDuringLoad() const; - void LogSettingChanges(std::set& changes, const std::string& context) const; + void LogSettingChanges(std::set& changes, const std::string_view& context) const; // modification bool RebindKeys(const Control::KeyChord& oldKeys, const Control::KeyChord& newKeys); diff --git a/src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp b/src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp index bac936fa3b2..fe8c7028be7 100644 --- a/src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp +++ b/src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp @@ -159,7 +159,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation return keybindingsList; } - void ActionMap::LogSettingChanges(std::set& changes, const std::string& context) const + void ActionMap::LogSettingChanges(std::set& changes, const std::string_view& context) const { for (const auto& setting : _changeLog) { diff --git a/src/cascadia/TerminalSettingsModel/AppearanceConfig.cpp b/src/cascadia/TerminalSettingsModel/AppearanceConfig.cpp index 5da66cb97a9..1b180364fa4 100644 --- a/src/cascadia/TerminalSettingsModel/AppearanceConfig.cpp +++ b/src/cascadia/TerminalSettingsModel/AppearanceConfig.cpp @@ -110,7 +110,7 @@ void AppearanceConfig::LayerJson(const Json::Value& json) // to make the UI happy, set ColorSchemeName. JsonUtils::GetValueForKey(json, ColorSchemeKey, _DarkColorSchemeName); _LightColorSchemeName = _DarkColorSchemeName; - _logSettingSet(std::string{ ColorSchemeKey }); + _logSettingSet(ColorSchemeKey); } else if (json["colorScheme"].isObject()) { @@ -172,7 +172,7 @@ winrt::hstring AppearanceConfig::ExpandedBackgroundImagePath() } } -void AppearanceConfig::_logSettingSet(const std::string& setting) +void AppearanceConfig::_logSettingSet(const std::string_view& setting) { _changeLog.emplace(setting); } @@ -185,7 +185,7 @@ void AppearanceConfig::_logSettingIfSet(const std::string_view& setting, const b } } -void AppearanceConfig::LogSettingChanges(std::set& changes, const std::string& context) const +void AppearanceConfig::LogSettingChanges(std::set& changes, const std::string_view& context) const { for (const auto& setting : _changeLog) { diff --git a/src/cascadia/TerminalSettingsModel/AppearanceConfig.h b/src/cascadia/TerminalSettingsModel/AppearanceConfig.h index 03442449544..87beb7ea1b8 100644 --- a/src/cascadia/TerminalSettingsModel/AppearanceConfig.h +++ b/src/cascadia/TerminalSettingsModel/AppearanceConfig.h @@ -31,7 +31,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation static winrt::com_ptr CopyAppearance(const AppearanceConfig* source, winrt::weak_ref sourceProfile); Json::Value ToJson() const; void LayerJson(const Json::Value& json); - void LogSettingChanges(std::set& changes, const std::string& context) const; + void LogSettingChanges(std::set& changes, const std::string_view& context) const; Model::Profile SourceProfile(); @@ -55,7 +55,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation winrt::weak_ref _sourceProfile; std::set _changeLog; - void _logSettingSet(const std::string& setting); + void _logSettingSet(const std::string_view& setting); void _logSettingIfSet(const std::string_view& setting, const bool isSet); }; } diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp index 1844872befd..2e7af128747 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp @@ -1652,7 +1652,7 @@ void CascadiaSettings::LogSettingChanges(bool isJsonLoad) const TelemetryPrivacyDataTag(PDT_ProductAndServiceUsage)); } #else - OutputDebugStringA(isJsonLoad ? "JsonSettingsChanged\n" : "UISettingsChanged\n"); + OutputDebugStringA(isJsonLoad ? "JsonSettingsChanged - " : "UISettingsChanged - "); OutputDebugStringA(change.data()); OutputDebugStringA("\n"); #endif // !_DEBUG diff --git a/src/cascadia/TerminalSettingsModel/Command.cpp b/src/cascadia/TerminalSettingsModel/Command.cpp index 474d0667aec..c9a5ed82a98 100644 --- a/src/cascadia/TerminalSettingsModel/Command.cpp +++ b/src/cascadia/TerminalSettingsModel/Command.cpp @@ -749,22 +749,22 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation switch (_IterateOn) { case ExpandCommandType::Profiles: - changes.insert(fmt::format(FMT_COMPILE("{}.{}"), std::string{ IterateOnKey }, "profiles")); + changes.emplace(fmt::format(FMT_COMPILE("{}.{}"), IterateOnKey, "profiles")); break; case ExpandCommandType::ColorSchemes: - changes.insert(fmt::format(FMT_COMPILE("{}.{}"), std::string{ IterateOnKey }, "schemes")); + changes.emplace(fmt::format(FMT_COMPILE("{}.{}"), IterateOnKey, "schemes")); break; } } if (!_Description.empty()) { - changes.insert(fmt::format(FMT_COMPILE("{}"), std::string{ DescriptionKey })); + changes.emplace(fmt::format(FMT_COMPILE("{}"), DescriptionKey)); } if (IsNestedCommand()) { - changes.insert(fmt::format(FMT_COMPILE("{}"), std::string{ CommandsKey })); + changes.emplace(fmt::format(FMT_COMPILE("{}"), CommandsKey)); } else { @@ -774,7 +774,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // covers actions w/out args // - "command": "unbound" --> "unbound" // - "command": "copy" --> "copy" - changes.insert(fmt::format(FMT_COMPILE("{}"), json.asString())); + changes.emplace(fmt::format(FMT_COMPILE("{}"), json.asString())); } else { @@ -789,7 +789,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation for (const auto& actionArg : members) { - changes.insert(fmt::format(FMT_COMPILE("{}.{}"), shortcutActionName, actionArg)); + changes.emplace(fmt::format(FMT_COMPILE("{}.{}"), shortcutActionName, actionArg)); } } } diff --git a/src/cascadia/TerminalSettingsModel/FontConfig.cpp b/src/cascadia/TerminalSettingsModel/FontConfig.cpp index e0558a78119..0fe0da5439c 100644 --- a/src/cascadia/TerminalSettingsModel/FontConfig.cpp +++ b/src/cascadia/TerminalSettingsModel/FontConfig.cpp @@ -110,7 +110,7 @@ winrt::Microsoft::Terminal::Settings::Model::Profile FontConfig::SourceProfile() return _sourceProfile.get(); } -void FontConfig::_logSettingSet(const std::string& setting) +void FontConfig::_logSettingSet(const std::string_view& setting) { if (setting == "axes" && _FontAxes.has_value()) { @@ -132,7 +132,7 @@ void FontConfig::_logSettingSet(const std::string& setting) } } -void FontConfig::_logSettingIfSet(const std::string& setting, const bool isSet) +void FontConfig::_logSettingIfSet(const std::string_view& setting, const bool isSet) { if (isSet) { @@ -140,7 +140,7 @@ void FontConfig::_logSettingIfSet(const std::string& setting, const bool isSet) } } -void FontConfig::LogSettingChanges(std::set& changes, const std::string& context) const +void FontConfig::LogSettingChanges(std::set& changes, const std::string_view& context) const { for (const auto& setting : _changeLog) { diff --git a/src/cascadia/TerminalSettingsModel/FontConfig.h b/src/cascadia/TerminalSettingsModel/FontConfig.h index 73e343d97b9..99d503d56d4 100644 --- a/src/cascadia/TerminalSettingsModel/FontConfig.h +++ b/src/cascadia/TerminalSettingsModel/FontConfig.h @@ -35,7 +35,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation static winrt::com_ptr CopyFontInfo(const FontConfig* source, winrt::weak_ref sourceProfile); Json::Value ToJson() const; void LayerJson(const Json::Value& json); - void LogSettingChanges(std::set& changes, const std::string& context) const; + void LogSettingChanges(std::set& changes, const std::string_view& context) const; Model::Profile SourceProfile(); @@ -48,7 +48,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation winrt::weak_ref _sourceProfile; std::set _changeLog; - void _logSettingSet(const std::string& setting); - void _logSettingIfSet(const std::string& setting, const bool isSet); + void _logSettingSet(const std::string_view& setting); + void _logSettingIfSet(const std::string_view& setting, const bool isSet); }; } diff --git a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp index 8b4bfcfdfab..bb0343a65de 100644 --- a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp +++ b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp @@ -157,7 +157,7 @@ void GlobalAppSettings::LayerJson(const Json::Value& json, const OriginTag origi JsonUtils::GetValueForKey(json, LegacyReloadEnvironmentVariablesKey, _legacyReloadEnvironmentVariables); if (json[LegacyReloadEnvironmentVariablesKey.data()]) { - _logSettingSet(std::string{ LegacyReloadEnvironmentVariablesKey }); + _logSettingSet(LegacyReloadEnvironmentVariablesKey); } } @@ -325,7 +325,7 @@ bool GlobalAppSettings::ShouldUsePersistedLayout() const return FirstWindowPreference() == FirstWindowPreference::PersistedWindowLayout && !IsolatedMode(); } -void GlobalAppSettings::_logSettingSet(const std::string& setting) +void GlobalAppSettings::_logSettingSet(const std::string_view& setting) { if (setting == "theme") { @@ -335,7 +335,7 @@ void GlobalAppSettings::_logSettingSet(const std::string& setting) // so we need to check if they were explicitly set if (_Theme->DarkName() == _Theme->LightName()) { - _changeLog.insert(setting); + _changeLog.emplace(setting); } else { @@ -385,15 +385,15 @@ void GlobalAppSettings::_logSettingSet(const std::string& setting) } } -void GlobalAppSettings::_logSettingIfSet(const std::string& setting, const bool isSet) +void GlobalAppSettings::_logSettingIfSet(const std::string_view& setting, const bool isSet) { if (isSet) { - _logSettingSet(std::string{ setting }); + _logSettingSet(setting); } } -void GlobalAppSettings::LogSettingChanges(std::set& changes, const std::string& context) const +void GlobalAppSettings::LogSettingChanges(std::set& changes, const std::string_view& context) const { for (const auto& setting : _changeLog) { diff --git a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.h b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.h index 51c601836b8..fe60347cfcd 100644 --- a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.h +++ b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.h @@ -72,7 +72,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation bool LegacyReloadEnvironmentVariables() const noexcept { return _legacyReloadEnvironmentVariables; } - void LogSettingChanges(std::set& changes, const std::string& context) const; + void LogSettingChanges(std::set& changes, const std::string_view& context) const; INHERITABLE_SETTING(Model::GlobalAppSettings, hstring, UnparsedDefaultProfile, L""); @@ -97,7 +97,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation Windows::Foundation::Collections::IMap _colorSchemes{ winrt::single_threaded_map() }; Windows::Foundation::Collections::IMap _themes{ winrt::single_threaded_map() }; - void _logSettingSet(const std::string& setting); - void _logSettingIfSet(const std::string& setting, const bool isSet); + void _logSettingSet(const std::string_view& setting); + void _logSettingIfSet(const std::string_view& setting, const bool isSet); }; } diff --git a/src/cascadia/TerminalSettingsModel/Profile.cpp b/src/cascadia/TerminalSettingsModel/Profile.cpp index ce840f9d34b..6aab1af37bf 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.cpp +++ b/src/cascadia/TerminalSettingsModel/Profile.cpp @@ -215,7 +215,7 @@ void Profile::LayerJson(const Json::Value& json) unfocusedAppearance->LayerJson(json[JsonKey(UnfocusedAppearanceKey)]); _UnfocusedAppearance = *unfocusedAppearance; - _logSettingSet(std::string{ UnfocusedAppearanceKey }); + _logSettingSet(UnfocusedAppearanceKey); } } @@ -527,7 +527,7 @@ std::wstring Profile::NormalizeCommandLine(LPCWSTR commandLine) return normalized; } -void Profile::_logSettingSet(const std::string& setting) +void Profile::_logSettingSet(const std::string_view& setting) { _changeLog.emplace(setting); } @@ -536,11 +536,11 @@ void Profile::_logSettingIfSet(const std::string_view& setting, const bool isSet { if (isSet) { - _logSettingSet(std::string{ setting }); + _logSettingSet(setting); } } -void Profile::LogSettingChanges(std::set& changes, const std::string& context) const +void Profile::LogSettingChanges(std::set& changes, const std::string_view& context) const { for (const auto& setting : _changeLog) { diff --git a/src/cascadia/TerminalSettingsModel/Profile.h b/src/cascadia/TerminalSettingsModel/Profile.h index f0e5ddee0ca..24fbb7d1f2d 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.h +++ b/src/cascadia/TerminalSettingsModel/Profile.h @@ -108,7 +108,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation void _FinalizeInheritance() override; - void LogSettingChanges(std::set& changes, const std::string& context) const; + void LogSettingChanges(std::set& changes, const std::string_view& context) const; // Special fields hstring Icon() const; @@ -149,7 +149,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation static guid _GenerateGuidForProfile(const std::wstring_view& name, const std::wstring_view& source) noexcept; winrt::hstring _evaluateIcon() const; - void _logSettingSet(const std::string& setting); + void _logSettingSet(const std::string_view& setting); void _logSettingIfSet(const std::string_view& setting, const bool isSet); friend class SettingsModelUnitTests::DeserializationTests; diff --git a/src/cascadia/TerminalSettingsModel/Theme.cpp b/src/cascadia/TerminalSettingsModel/Theme.cpp index 2f8ce9472c2..90fd1d1a760 100644 --- a/src/cascadia/TerminalSettingsModel/Theme.cpp +++ b/src/cascadia/TerminalSettingsModel/Theme.cpp @@ -292,7 +292,7 @@ winrt::com_ptr Theme::FromJson(const Json::Value& json) return result; } -void Theme::LogSettingChanges(std::set& changes, std::string& context) +void Theme::LogSettingChanges(std::set& changes, const std::string_view& context) { #pragma warning(push) #pragma warning(disable : 5103) // pasting '{' and 'winrt' does not result in a valid preprocessing token @@ -305,7 +305,7 @@ void Theme::LogSettingChanges(std::set& changes, std::string& conte #define LOG_IF_SET(type, name, jsonKey, ...) \ if (obj.name() != type{##__VA_ARGS__ }) \ - changes.insert(fmt::format(FMT_COMPILE("{}.{}.{}"), context, outerJsonKey, jsonKey)); + changes.emplace(fmt::format(FMT_COMPILE("{}.{}.{}"), context, outerJsonKey, jsonKey)); if (isWindowSet) { diff --git a/src/cascadia/TerminalSettingsModel/Theme.h b/src/cascadia/TerminalSettingsModel/Theme.h index 305b5fd4090..e0892733ae8 100644 --- a/src/cascadia/TerminalSettingsModel/Theme.h +++ b/src/cascadia/TerminalSettingsModel/Theme.h @@ -98,7 +98,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation static com_ptr FromJson(const Json::Value& json); Json::Value ToJson() const; - void LogSettingChanges(std::set& changes, std::string& context); + void LogSettingChanges(std::set& changes, const std::string_view& context); winrt::Windows::UI::Xaml::ElementTheme RequestedTheme() const noexcept; From 8bda86e2e2bfc5d33407b387dac68276ee24f1fb Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Mon, 12 Aug 2024 15:17:10 -0700 Subject: [PATCH 08/16] exclude stuff from userDefaults.json --- .../ActionMapSerialization.cpp | 24 +++++++++++++++++++ .../GlobalAppSettings.cpp | 15 ++++++++++++ 2 files changed, 39 insertions(+) diff --git a/src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp b/src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp index fe8c7028be7..a030c1a571d 100644 --- a/src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp +++ b/src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp @@ -107,6 +107,30 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // any existing keybinding with the same keychord in this layer will get overwritten _KeyMap.insert_or_assign(keys, idJson); + + if (!_changeLog.contains(KeysKey.data())) + { + // Log "keys" field, but only if it's one that isn't in userDefaults.json + static constexpr std::array, 3> userDefaultKbds{ { { L"Terminal.CopyToClipboard", "ctrl+c" }, + { L"Terminal.PasteFromClipboard", "ctrl+v" }, + { L"Terminal.DuplicatePaneAuto", "alt+shift+d" } } }; + bool isUserDefaultKbd = false; + for (const auto& [id, kbd] : userDefaultKbds) + { + const auto keyJson{ jsonBlock.find(&*KeysKey.cbegin(), (&*KeysKey.cbegin()) + KeysKey.size()) }; + OutputDebugString(idJson.data()); + OutputDebugStringA(keyJson->asString().data()); + if (idJson == id && keyJson->asString() == kbd) + { + isUserDefaultKbd = true; + break; + } + } + if (!isUserDefaultKbd) + { + _changeLog.emplace(KeysKey); + } + } } } diff --git a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp index bb0343a65de..51cfce608d6 100644 --- a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp +++ b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp @@ -159,6 +159,21 @@ void GlobalAppSettings::LayerJson(const Json::Value& json, const OriginTag origi { _logSettingSet(LegacyReloadEnvironmentVariablesKey); } + + // Remove settings included in userDefaults + static constexpr std::array, 2> userDefaultSettings{ { { "copyOnSelect", "false" }, + { "copyFormatting", "false" } } }; + for (const auto& [setting, val] : userDefaultSettings) + { + if (const auto settingJson{ json.find(&*setting.cbegin(), (&*setting.cbegin()) + setting.size()) }) + { + if (settingJson->asString() == val) + { + // false positive! + _changeLog.erase(std::string{ setting }); + } + } + } } void GlobalAppSettings::LayerActionsFrom(const Json::Value& json, const OriginTag origin, const bool withKeybindings) From 8a17e26176638bf03d6140d25d63da3322ddbd45 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Tue, 13 Aug 2024 15:39:59 -0700 Subject: [PATCH 09/16] remove debug code; register 'keys' in _TryUpdateKeyChord --- src/cascadia/TerminalSettingsModel/ActionMap.cpp | 1 + src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp | 2 -- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/ActionMap.cpp b/src/cascadia/TerminalSettingsModel/ActionMap.cpp index 5090733342a..9ebf21c6ba8 100644 --- a/src/cascadia/TerminalSettingsModel/ActionMap.cpp +++ b/src/cascadia/TerminalSettingsModel/ActionMap.cpp @@ -570,6 +570,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation const auto action = cmd.ActionAndArgs().Action(); const auto id = action == ShortcutAction::Invalid ? hstring{} : cmd.ID(); _KeyMap.insert_or_assign(keys, id); + _changeLog.emplace(KeysKey); } // Method Description: diff --git a/src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp b/src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp index a030c1a571d..11b6fe649bd 100644 --- a/src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp +++ b/src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp @@ -118,8 +118,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation for (const auto& [id, kbd] : userDefaultKbds) { const auto keyJson{ jsonBlock.find(&*KeysKey.cbegin(), (&*KeysKey.cbegin()) + KeysKey.size()) }; - OutputDebugString(idJson.data()); - OutputDebugStringA(keyJson->asString().data()); if (idJson == id && keyJson->asString() == kbd) { isUserDefaultKbd = true; From e658ffbd332b4bdacca08d7b5d4dfb428b1e8918 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Tue, 13 Aug 2024 15:40:29 -0700 Subject: [PATCH 10/16] spell --- .github/actions/spelling/allow/allow.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/actions/spelling/allow/allow.txt b/.github/actions/spelling/allow/allow.txt index ef4f666f47d..fcdea926dfa 100644 --- a/.github/actions/spelling/allow/allow.txt +++ b/.github/actions/spelling/allow/allow.txt @@ -27,6 +27,7 @@ gje godbolt hyperlinking hyperlinks +kbd kje libfuzzer liga From 1ea1c51d726996d4bfd3079857d812d1837b54ad Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Tue, 13 Aug 2024 15:41:03 -0700 Subject: [PATCH 11/16] spell the spell --- .github/actions/spelling/allow/allow.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/actions/spelling/allow/allow.txt b/.github/actions/spelling/allow/allow.txt index fcdea926dfa..e477aadaa38 100644 --- a/.github/actions/spelling/allow/allow.txt +++ b/.github/actions/spelling/allow/allow.txt @@ -27,7 +27,7 @@ gje godbolt hyperlinking hyperlinks -kbd +Kbds kje libfuzzer liga From 65defc0efd58ae278020573dcd596b69ea15f6d5 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Wed, 21 Aug 2024 14:06:33 -0700 Subject: [PATCH 12/16] exclude false positives in profiles --- .../TerminalSettingsModel/Profile.cpp | 31 +++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/Profile.cpp b/src/cascadia/TerminalSettingsModel/Profile.cpp index 6aab1af37bf..b8dc71fe8de 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.cpp +++ b/src/cascadia/TerminalSettingsModel/Profile.cpp @@ -175,10 +175,12 @@ void Profile::LayerJson(const Json::Value& json) JsonUtils::GetValueForKey(json, NameKey, _Name); JsonUtils::GetValueForKey(json, UpdatesKey, _Updates); JsonUtils::GetValueForKey(json, GuidKey, _Guid); + + // Make sure Source is before Hidden! We use that to exclude false positives from the settings logger! + JsonUtils::GetValueForKey(json, SourceKey, _Source); JsonUtils::GetValueForKey(json, HiddenKey, _Hidden); _logSettingIfSet(HiddenKey, _Hidden.has_value()); - JsonUtils::GetValueForKey(json, SourceKey, _Source); JsonUtils::GetValueForKey(json, IconKey, _Icon); _logSettingIfSet(IconKey, _Icon.has_value()); @@ -536,7 +538,32 @@ void Profile::_logSettingIfSet(const std::string_view& setting, const bool isSet { if (isSet) { - _logSettingSet(setting); + // make sure this matches defaults.json. + static constexpr winrt::guid DEFAULT_WINDOWS_POWERSHELL_GUID{ 0x61c54bbd, 0xc2c6, 0x5271, { 0x96, 0xe7, 0x00, 0x9a, 0x87, 0xff, 0x44, 0xbf } }; + static constexpr winrt::guid DEFAULT_COMMAND_PROMPT_GUID{ 0x0caa0dad, 0x35be, 0x5f56, { 0xa8, 0xff, 0xaf, 0xce, 0xee, 0xaa, 0x61, 0x01 } }; + + // Exclude some false positives from userDefaults.json + // NOTE: we can't use the OriginTag here because it hasn't been set yet! + const bool isWinPow = _Guid.has_value() && *_Guid == DEFAULT_WINDOWS_POWERSHELL_GUID; //_Name.has_value() && til::equals_insensitive_ascii(*_Name, L"Windows PowerShell"); + const bool isCmd = _Guid.has_value() && *_Guid == DEFAULT_COMMAND_PROMPT_GUID; //_Name.has_value() && til::equals_insensitive_ascii(*_Name, L"Command Prompt"); + const bool isACS = _Name.has_value() && til::equals_insensitive_ascii(*_Name, L"Azure Cloud Shell"); + const bool isWTDynamicProfile = _Source.has_value() && til::starts_with(*_Source, L"Windows.Terminal"); + const bool settingHiddenToFalse = til::equals_insensitive_ascii(setting, HiddenKey) && _Hidden.has_value() && _Hidden == false; + const bool settingCommandlineToWinPow = til::equals_insensitive_ascii(setting, "commandline") && _Commandline.has_value() && til::equals_insensitive_ascii(*_Commandline, L"%SystemRoot%\\System32\\WindowsPowerShell\\v1.0\\powershell.exe"); + const bool settingCommandlineToCmd = til::equals_insensitive_ascii(setting, "commandline") && _Commandline.has_value() && til::equals_insensitive_ascii(*_Commandline, L"%SystemRoot%\\System32\\cmd.exe"); + // clang-format off + if ((isWinPow && (settingHiddenToFalse || settingCommandlineToWinPow)) + || (isCmd && (settingHiddenToFalse || settingCommandlineToCmd)) + || (isACS && settingHiddenToFalse) + || (isWTDynamicProfile && settingHiddenToFalse)) + { + // clang-format on + // skip + } + else + { + _logSettingSet(setting); + } } } From ff2260521be4711055fb51335d25d4666e8157d7 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Thu, 22 Aug 2024 11:20:36 -0700 Subject: [PATCH 13/16] more false positive handling (and better logic) --- .../TerminalSettingsModel/GlobalAppSettings.cpp | 8 +++++++- src/cascadia/TerminalSettingsModel/Profile.cpp | 12 ++++-------- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp index 51cfce608d6..0965a6b4006 100644 --- a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp +++ b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp @@ -404,7 +404,13 @@ void GlobalAppSettings::_logSettingIfSet(const std::string_view& setting, const { if (isSet) { - _logSettingSet(setting); + // Exclude some false positives from userDefaults.json + const bool settingCopyFormattingToDefault = til::equals_insensitive_ascii(setting, "copyFormatting") && _CopyFormatting.has_value() && _CopyFormatting.value() == static_cast(0); + const bool settingNTMtoDefault = til::equals_insensitive_ascii(setting, "newTabMenu") && _NewTabMenu.has_value() && _NewTabMenu->Size() == 1 && _NewTabMenu->GetAt(0).Type() == NewTabMenuEntryType::RemainingProfiles; + if (!settingCopyFormattingToDefault && !settingNTMtoDefault) + { + _logSettingSet(setting); + } } } diff --git a/src/cascadia/TerminalSettingsModel/Profile.cpp b/src/cascadia/TerminalSettingsModel/Profile.cpp index b8dc71fe8de..ed57474f7db 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.cpp +++ b/src/cascadia/TerminalSettingsModel/Profile.cpp @@ -552,16 +552,12 @@ void Profile::_logSettingIfSet(const std::string_view& setting, const bool isSet const bool settingCommandlineToWinPow = til::equals_insensitive_ascii(setting, "commandline") && _Commandline.has_value() && til::equals_insensitive_ascii(*_Commandline, L"%SystemRoot%\\System32\\WindowsPowerShell\\v1.0\\powershell.exe"); const bool settingCommandlineToCmd = til::equals_insensitive_ascii(setting, "commandline") && _Commandline.has_value() && til::equals_insensitive_ascii(*_Commandline, L"%SystemRoot%\\System32\\cmd.exe"); // clang-format off - if ((isWinPow && (settingHiddenToFalse || settingCommandlineToWinPow)) - || (isCmd && (settingHiddenToFalse || settingCommandlineToCmd)) - || (isACS && settingHiddenToFalse) - || (isWTDynamicProfile && settingHiddenToFalse)) + if (!(isWinPow && (settingHiddenToFalse || settingCommandlineToWinPow)) + && !(isCmd && (settingHiddenToFalse || settingCommandlineToCmd)) + && !(isACS && settingHiddenToFalse) + && !(isWTDynamicProfile && settingHiddenToFalse)) { // clang-format on - // skip - } - else - { _logSettingSet(setting); } } From f201feec97d6ee04837b8396a747c47507fc43d9 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Thu, 22 Aug 2024 11:34:24 -0700 Subject: [PATCH 14/16] appease the spell checker --- src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp index 0965a6b4006..e7f930c175a 100644 --- a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp +++ b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp @@ -406,8 +406,8 @@ void GlobalAppSettings::_logSettingIfSet(const std::string_view& setting, const { // Exclude some false positives from userDefaults.json const bool settingCopyFormattingToDefault = til::equals_insensitive_ascii(setting, "copyFormatting") && _CopyFormatting.has_value() && _CopyFormatting.value() == static_cast(0); - const bool settingNTMtoDefault = til::equals_insensitive_ascii(setting, "newTabMenu") && _NewTabMenu.has_value() && _NewTabMenu->Size() == 1 && _NewTabMenu->GetAt(0).Type() == NewTabMenuEntryType::RemainingProfiles; - if (!settingCopyFormattingToDefault && !settingNTMtoDefault) + const bool settingNTMToDefault = til::equals_insensitive_ascii(setting, "newTabMenu") && _NewTabMenu.has_value() && _NewTabMenu->Size() == 1 && _NewTabMenu->GetAt(0).Type() == NewTabMenuEntryType::RemainingProfiles; + if (!settingCopyFormattingToDefault && !settingNTMToDefault) { _logSettingSet(setting); } From 9b04a62cc8762ec7e3b7407b07d0f4aa7b78d6fc Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Thu, 22 Aug 2024 15:01:20 -0700 Subject: [PATCH 15/16] spell rite --- .github/actions/spelling/allow/allow.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/actions/spelling/allow/allow.txt b/.github/actions/spelling/allow/allow.txt index e477aadaa38..3b54d304979 100644 --- a/.github/actions/spelling/allow/allow.txt +++ b/.github/actions/spelling/allow/allow.txt @@ -44,6 +44,7 @@ mkmk mnt mru nje +NTMTo notwrapped ogonek overlined From d7b6b54379cff5346bf63a676067f5f239e7aec4 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Thu, 22 Aug 2024 17:19:37 -0700 Subject: [PATCH 16/16] feedback --- src/cascadia/TerminalSettingsModel/AppearanceConfig.cpp | 2 +- .../CascadiaSettingsSerialization.cpp | 8 ++++---- src/cascadia/TerminalSettingsModel/Command.cpp | 4 ++-- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/AppearanceConfig.cpp b/src/cascadia/TerminalSettingsModel/AppearanceConfig.cpp index 1b180364fa4..d5199c8fac9 100644 --- a/src/cascadia/TerminalSettingsModel/AppearanceConfig.cpp +++ b/src/cascadia/TerminalSettingsModel/AppearanceConfig.cpp @@ -181,7 +181,7 @@ void AppearanceConfig::_logSettingIfSet(const std::string_view& setting, const b { if (isSet) { - _logSettingSet(std::string{ setting }); + _logSettingSet(setting); } } diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp index 2e7af128747..789baf9295d 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp @@ -1601,20 +1601,20 @@ void CascadiaSettings::LogSettingChanges(bool isJsonLoad) const // aggregate setting changes std::set changes; - static std::string globalContext{ "global" }; + static constexpr std::string_view globalContext{ "global" }; _globals->LogSettingChanges(changes, globalContext); // Actions are not expected to change when loaded from the settings UI - static std::string actionContext{ "action" }; + static constexpr std::string_view actionContext{ "action" }; winrt::get_self(_globals->ActionMap())->LogSettingChanges(changes, actionContext); - static std::string profileContext{ "profile" }; + static constexpr std::string_view profileContext{ "profile" }; for (const auto& profile : _allProfiles) { winrt::get_self(profile)->LogSettingChanges(changes, profileContext); } - static std::string profileDefaultsContext{ "profileDefaults" }; + static constexpr std::string_view profileDefaultsContext{ "profileDefaults" }; _baseLayerProfile->LogSettingChanges(changes, profileDefaultsContext); // Themes are not expected to change when loaded from the settings UI diff --git a/src/cascadia/TerminalSettingsModel/Command.cpp b/src/cascadia/TerminalSettingsModel/Command.cpp index c9a5ed82a98..32e94ad3d5f 100644 --- a/src/cascadia/TerminalSettingsModel/Command.cpp +++ b/src/cascadia/TerminalSettingsModel/Command.cpp @@ -759,12 +759,12 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation if (!_Description.empty()) { - changes.emplace(fmt::format(FMT_COMPILE("{}"), DescriptionKey)); + changes.emplace(DescriptionKey); } if (IsNestedCommand()) { - changes.emplace(fmt::format(FMT_COMPILE("{}"), CommandsKey)); + changes.emplace(CommandsKey); } else {