From 53c98610b4ed7a2ba608ba3784de08d378ff04d7 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Mon, 27 Apr 2020 17:20:18 -0500 Subject: [PATCH] Add a warning about using the `globals` property (#5597) This property was deprecated in 0.11. We probably should have also added a warning message to help the community figure out that this property is gone and won't work anymore. This PR adds that warning. * I'm not going to list the enormous number of duped threads _wait yes I am_ * #5581 * #5547 * #5555 * #5557 * #5573 * #5532 * #5527 * #5535 * #5510 * #5511 * #5512 * #5513 * #5516 * #5515 * #5521 * This literally isn't even all of them * [x] Also mainly related to #5458 * [x] I work here * [x] Tests added/passed (cherry picked from commit d6cae40d26d2f4ba249d3960e8478c04681c8e1e) --- .../LocalTests_TerminalApp/SettingsTests.cpp | 38 +++++++++++ src/cascadia/TerminalApp/AppLogic.cpp | 24 ++++++- src/cascadia/TerminalApp/CascadiaSettings.cpp | 21 ++++++ src/cascadia/TerminalApp/CascadiaSettings.h | 1 + .../Resources/en-US/Resources.resw | 65 +++++++++++-------- src/cascadia/TerminalApp/TerminalWarnings.h | 1 + 6 files changed, 122 insertions(+), 28 deletions(-) diff --git a/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp b/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp index ba0115842de..4bc51dbcd26 100644 --- a/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp +++ b/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp @@ -76,6 +76,8 @@ namespace TerminalAppLocalTests TEST_METHOD(ValidateKeybindingsWarnings); + TEST_METHOD(ValidateLegacyGlobalsWarning); + TEST_CLASS_SETUP(ClassSetup) { InitializeJsonReader(); @@ -2225,4 +2227,40 @@ namespace TerminalAppLocalTests VERIFY_ARE_EQUAL(::TerminalApp::SettingsLoadWarnings::MissingRequiredParameter, settings->_warnings.at(2)); VERIFY_ARE_EQUAL(::TerminalApp::SettingsLoadWarnings::MissingRequiredParameter, settings->_warnings.at(3)); } + + void SettingsTests::ValidateLegacyGlobalsWarning() + { + const std::string badSettings{ R"( + { + "globals": {}, + "defaultProfile": "{6239a42c-2222-49a3-80bd-e8fdd045185c}", + "profiles": [ + { + "name" : "profile0", + "guid": "{6239a42c-2222-49a3-80bd-e8fdd045185c}" + }, + { + "name" : "profile1", + "guid": "{6239a42c-3333-49a3-80bd-e8fdd045185c}" + } + ], + "keybindings": [] + })" }; + + // Create the default settings + CascadiaSettings settings; + settings._ParseJsonString(DefaultJson, true); + settings.LayerJson(settings._defaultSettings); + + settings._ValidateNoGlobalsKey(); + VERIFY_ARE_EQUAL(0u, settings._warnings.size()); + + // Now layer on the user's settings + settings._ParseJsonString(badSettings, false); + settings.LayerJson(settings._userSettings); + + settings._ValidateNoGlobalsKey(); + VERIFY_ARE_EQUAL(1u, settings._warnings.size()); + VERIFY_ARE_EQUAL(::TerminalApp::SettingsLoadWarnings::LegacyGlobalsProperty, settings._warnings.at(0)); + } } diff --git a/src/cascadia/TerminalApp/AppLogic.cpp b/src/cascadia/TerminalApp/AppLogic.cpp index 316b2459000..4444271a6b6 100644 --- a/src/cascadia/TerminalApp/AppLogic.cpp +++ b/src/cascadia/TerminalApp/AppLogic.cpp @@ -35,7 +35,8 @@ static const std::array(SettingsLoadWar USES_RESOURCE(L"InvalidIcon"), USES_RESOURCE(L"AtLeastOneKeybindingWarning"), USES_RESOURCE(L"TooManyKeysForChord"), - USES_RESOURCE(L"MissingRequiredParameter") + USES_RESOURCE(L"MissingRequiredParameter"), + USES_RESOURCE(L"LegacyGlobalsProperty") }; static const std::array(SettingsLoadErrors::ERRORS_SIZE)> settingsLoadErrorsLabels { USES_RESOURCE(L"NoProfilesText"), @@ -391,6 +392,27 @@ namespace winrt::TerminalApp::implementation if (!warningText.empty()) { warningsTextBlock.Inlines().Append(_BuildErrorRun(warningText, ::winrt::Windows::UI::Xaml::Application::Current().as<::winrt::TerminalApp::App>().Resources())); + + // The "LegacyGlobalsProperty" warning is special - it has a URL + // that goes with it. So we need to manually construct a + // Hyperlink and insert it along with the warning text. + if (warning == SettingsLoadWarnings::LegacyGlobalsProperty) + { + // Add the URL here too + const auto legacyGlobalsLinkLabel = RS_(L"LegacyGlobalsPropertyHrefLabel"); + const auto legacyGlobalsLinkUriValue = RS_(L"LegacyGlobalsPropertyHrefUrl"); + + winrt::Windows::UI::Xaml::Documents::Run legacyGlobalsLinkText; + winrt::Windows::UI::Xaml::Documents::Hyperlink legacyGlobalsLink; + winrt::Windows::Foundation::Uri legacyGlobalsLinkUri{ legacyGlobalsLinkUriValue }; + + legacyGlobalsLinkText.Text(legacyGlobalsLinkLabel); + legacyGlobalsLink.NavigateUri(legacyGlobalsLinkUri); + legacyGlobalsLink.Inlines().Append(legacyGlobalsLinkText); + + warningsTextBlock.Inlines().Append(legacyGlobalsLink); + } + warningsTextBlock.Inlines().Append(Documents::LineBreak{}); } } diff --git a/src/cascadia/TerminalApp/CascadiaSettings.cpp b/src/cascadia/TerminalApp/CascadiaSettings.cpp index 46fc2f08071..767aaa9f546 100644 --- a/src/cascadia/TerminalApp/CascadiaSettings.cpp +++ b/src/cascadia/TerminalApp/CascadiaSettings.cpp @@ -211,6 +211,8 @@ void CascadiaSettings::_ValidateSettings() // warning if an action didn't have a required arg. // This will also catch other keybinding warnings, like from GH#4239 _ValidateKeybindings(); + + _ValidateNoGlobalsKey(); } // Method Description: @@ -674,6 +676,25 @@ void CascadiaSettings::_ValidateKeybindings() } } +// Method Description: +// - Checks for the presence of the legacy "globals" key in the user's +// settings.json. If this key is present, then they've probably got a pre-0.11 +// settings file that won't work as expected anymore. We should warn them +// about that. +// Arguments: +// - +// Return Value: +// - +// - Appends a SettingsLoadWarnings::LegacyGlobalsProperty to our list of warnings if +// we find any invalid background images. +void CascadiaSettings::_ValidateNoGlobalsKey() +{ + if (auto oldGlobalsProperty{ _userSettings["globals"] }) + { + _warnings.push_back(::TerminalApp::SettingsLoadWarnings::LegacyGlobalsProperty); + } +} + // Method Description // - Replaces known tokens DEFAULT_PROFILE, PRODUCT and VERSION in the settings template // with their expected values. DEFAULT_PROFILE is updated to match PowerShell Core's GUID diff --git a/src/cascadia/TerminalApp/CascadiaSettings.h b/src/cascadia/TerminalApp/CascadiaSettings.h index 7d84176f718..31ce8ab8456 100644 --- a/src/cascadia/TerminalApp/CascadiaSettings.h +++ b/src/cascadia/TerminalApp/CascadiaSettings.h @@ -119,6 +119,7 @@ class TerminalApp::CascadiaSettings final void _ValidateAllSchemesExist(); void _ValidateMediaResources(); void _ValidateKeybindings(); + void _ValidateNoGlobalsKey(); friend class TerminalAppLocalTests::SettingsTests; friend class TerminalAppLocalTests::ProfileTests; diff --git a/src/cascadia/TerminalApp/Resources/en-US/Resources.resw b/src/cascadia/TerminalApp/Resources/en-US/Resources.resw index c31ad6448a5..c8fa25218ba 100644 --- a/src/cascadia/TerminalApp/Resources/en-US/Resources.resw +++ b/src/cascadia/TerminalApp/Resources/en-US/Resources.resw @@ -1,17 +1,17 @@  - @@ -187,6 +187,17 @@ • Found a keybinding that was missing a required parameter value. This keybinding will be ignored. {Locked="•"} This glyph is a bullet, used in a bulleted list. + + The "globals" property is deprecated - your settings might need updating. + {Locked="\"globals\""} + + + https://go.microsoft.com/fwlink/?linkid=2128258 + {Locked}This is a FWLink, so it will be localized with the fwlink tool + + + For more info, see this web page. + An optional command, with arguments, to be spawned in the new tab or pane diff --git a/src/cascadia/TerminalApp/TerminalWarnings.h b/src/cascadia/TerminalApp/TerminalWarnings.h index f8b13c88522..287acb369af 100644 --- a/src/cascadia/TerminalApp/TerminalWarnings.h +++ b/src/cascadia/TerminalApp/TerminalWarnings.h @@ -29,6 +29,7 @@ namespace TerminalApp AtLeastOneKeybindingWarning = 5, TooManyKeysForChord = 6, MissingRequiredParameter = 7, + LegacyGlobalsProperty = 8, WARNINGS_SIZE // IMPORTANT: This MUST be the last value in this enum. It's an unused placeholder. };