diff --git a/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp b/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp index 82bcfbe611f..4a70386e33d 100644 --- a/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp +++ b/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp @@ -4,11 +4,13 @@ #include "precomp.h" #include "../TerminalApp/ColorScheme.h" +#include "../TerminalApp/CascadiaSettings.h" using namespace Microsoft::Console; using namespace TerminalApp; using namespace WEX::Logging; using namespace WEX::TestExecution; +using namespace WEX::Common; namespace TerminalAppLocalTests { @@ -29,8 +31,31 @@ namespace TerminalAppLocalTests END_TEST_CLASS() TEST_METHOD(TryCreateWinRTType); + TEST_METHOD(ValidateProfilesExist); + TEST_METHOD(ValidateDefaultProfileExists); + TEST_METHOD(ValidateDuplicateProfiles); + TEST_METHOD(ValidateManyWarnings); + + TEST_CLASS_SETUP(ClassSetup) + { + reader = std::unique_ptr(Json::CharReaderBuilder::CharReaderBuilder().newCharReader()); + return true; + } + Json::Value VerifyParseSucceeded(std::string content); + + private: + std::unique_ptr reader; }; + Json::Value SettingsTests::VerifyParseSucceeded(std::string content) + { + Json::Value root; + std::string errs; + const bool parseResult = reader->parse(content.c_str(), content.c_str() + content.size(), &root, &errs); + VERIFY_IS_TRUE(parseResult, winrt::to_hstring(errs).c_str()); + return root; + } + void SettingsTests::TryCreateWinRTType() { winrt::Microsoft::Terminal::Settings::TerminalSettings settings{}; @@ -41,4 +66,303 @@ namespace TerminalAppLocalTests VERIFY_ARE_NOT_EQUAL(oldFontSize, newFontSize); } + void SettingsTests::ValidateProfilesExist() + { + const std::string settingsWithProfiles{ R"( + { + "profiles": [ + { + "name" : "profile0" + } + ] + })" }; + + const std::string settingsWithoutProfiles{ R"( + { + "defaultProfile": "{6239a42c-1de4-49a3-80bd-e8fdd045185c}" + })" }; + + const std::string settingsWithEmptyProfiles{ R"( + { + "profiles": [] + })" }; + + { + // Case 1: Good settings + const auto settingsObject = VerifyParseSucceeded(settingsWithProfiles); + auto settings = CascadiaSettings::FromJson(settingsObject); + settings->_ValidateProfilesExist(); + } + { + // Case 2: Bad settings + const auto settingsObject = VerifyParseSucceeded(settingsWithoutProfiles); + auto settings = CascadiaSettings::FromJson(settingsObject); + bool caughtExpectedException = false; + try + { + settings->_ValidateProfilesExist(); + } + catch (const ::TerminalApp::SettingsException& ex) + { + VERIFY_IS_TRUE(ex.Error() == ::TerminalApp::SettingsLoadErrors::NoProfiles); + caughtExpectedException = true; + } + VERIFY_IS_TRUE(caughtExpectedException); + } + { + // Case 3: Bad settings + const auto settingsObject = VerifyParseSucceeded(settingsWithEmptyProfiles); + auto settings = CascadiaSettings::FromJson(settingsObject); + bool caughtExpectedException = false; + try + { + settings->_ValidateProfilesExist(); + } + catch (const ::TerminalApp::SettingsException& ex) + { + VERIFY_IS_TRUE(ex.Error() == ::TerminalApp::SettingsLoadErrors::NoProfiles); + caughtExpectedException = true; + } + VERIFY_IS_TRUE(caughtExpectedException); + } + } + + void SettingsTests::ValidateDefaultProfileExists() + { + const std::string goodProfiles{ R"( + { + "globals": { + "defaultProfile": "{6239a42c-1111-49a3-80bd-e8fdd045185c}" + }, + "profiles": [ + { + "name" : "profile0", + "guid": "{6239a42c-1111-49a3-80bd-e8fdd045185c}" + }, + { + "name" : "profile0", + "guid": "{6239a42c-2222-49a3-80bd-e8fdd045185c}" + } + ] + })" }; + + const std::string badProfiles{ R"( + { + "globals": { + "defaultProfile": "{6239a42c-1111-49a3-80bd-e8fdd045185c}" + }, + "profiles": [ + { + "name" : "profile0", + "guid": "{6239a42c-3333-49a3-80bd-e8fdd045185c}" + }, + { + "name" : "profile1", + "guid": "{6239a42c-4444-49a3-80bd-e8fdd045185c}" + } + ] + })" }; + + const std::string noDefaultAtAll{ R"( + { + "globals": { + "alwaysShowTabs": true + }, + "profiles": [ + { + "name" : "profile0", + "guid": "{6239a42c-5555-49a3-80bd-e8fdd045185c}" + }, + { + "name" : "profile1", + "guid": "{6239a42c-6666-49a3-80bd-e8fdd045185c}" + } + ] + })" }; + + { + // Case 1: Good settings + Log::Comment(NoThrowString().Format( + L"Testing a pair of profiles with unique guids, and the defaultProfile is one of those guids")); + const auto settingsObject = VerifyParseSucceeded(goodProfiles); + auto settings = CascadiaSettings::FromJson(settingsObject); + settings->_ValidateDefaultProfileExists(); + VERIFY_ARE_EQUAL(static_cast(0), settings->_warnings.size()); + VERIFY_ARE_EQUAL(static_cast(2), settings->_profiles.size()); + VERIFY_ARE_EQUAL(settings->_globals.GetDefaultProfile(), settings->_profiles.at(0).GetGuid()); + } + { + // Case 2: Bad settings + Log::Comment(NoThrowString().Format( + L"Testing a pair of profiles with unique guids, but the defaultProfile is NOT one of those guids")); + const auto settingsObject = VerifyParseSucceeded(badProfiles); + auto settings = CascadiaSettings::FromJson(settingsObject); + settings->_ValidateDefaultProfileExists(); + VERIFY_ARE_EQUAL(static_cast(1), settings->_warnings.size()); + VERIFY_ARE_EQUAL(::TerminalApp::SettingsLoadWarnings::MissingDefaultProfile, settings->_warnings.at(0)); + + VERIFY_ARE_EQUAL(static_cast(2), settings->_profiles.size()); + VERIFY_ARE_EQUAL(settings->_globals.GetDefaultProfile(), settings->_profiles.at(0).GetGuid()); + } + { + // Case 2: Bad settings + Log::Comment(NoThrowString().Format( + L"Testing a pair of profiles with unique guids, and no defaultProfile at all")); + const auto settingsObject = VerifyParseSucceeded(badProfiles); + auto settings = CascadiaSettings::FromJson(settingsObject); + settings->_ValidateDefaultProfileExists(); + VERIFY_ARE_EQUAL(static_cast(1), settings->_warnings.size()); + VERIFY_ARE_EQUAL(::TerminalApp::SettingsLoadWarnings::MissingDefaultProfile, settings->_warnings.at(0)); + + VERIFY_ARE_EQUAL(static_cast(2), settings->_profiles.size()); + VERIFY_ARE_EQUAL(settings->_globals.GetDefaultProfile(), settings->_profiles.at(0).GetGuid()); + } + } + + void SettingsTests::ValidateDuplicateProfiles() + { + const std::string goodProfiles{ R"( + { + "profiles": [ + { + "name" : "profile0", + "guid": "{6239a42c-1111-49a3-80bd-e8fdd045185c}" + }, + { + "name" : "profile0", + "guid": "{6239a42c-2222-49a3-80bd-e8fdd045185c}" + } + ] + })" }; + + const std::string badProfiles{ R"( + { + "profiles": [ + { + "name" : "profile0", + "guid": "{6239a42c-3333-49a3-80bd-e8fdd045185c}" + }, + { + "name" : "profile1", + "guid": "{6239a42c-3333-49a3-80bd-e8fdd045185c}" + } + ] + })" }; + + const std::string veryBadProfiles{ R"( + { + "profiles": [ + { + "name" : "profile0", + "guid": "{6239a42c-4444-49a3-80bd-e8fdd045185c}" + }, + { + "name" : "profile1", + "guid": "{6239a42c-5555-49a3-80bd-e8fdd045185c}" + }, + { + "name" : "profile2", + "guid": "{6239a42c-4444-49a3-80bd-e8fdd045185c}" + }, + { + "name" : "profile3", + "guid": "{6239a42c-4444-49a3-80bd-e8fdd045185c}" + }, + { + "name" : "profile4", + "guid": "{6239a42c-6666-49a3-80bd-e8fdd045185c}" + }, + { + "name" : "profile5", + "guid": "{6239a42c-5555-49a3-80bd-e8fdd045185c}" + }, + { + "name" : "profile6", + "guid": "{6239a42c-7777-49a3-80bd-e8fdd045185c}" + } + ] + })" }; + + { + // Case 1: Good settings + Log::Comment(NoThrowString().Format( + L"Testing a pair of profiles with unique guids")); + const auto settingsObject = VerifyParseSucceeded(goodProfiles); + auto settings = CascadiaSettings::FromJson(settingsObject); + settings->_ValidateNoDuplicateProfiles(); + VERIFY_ARE_EQUAL(static_cast(0), settings->_warnings.size()); + VERIFY_ARE_EQUAL(static_cast(2), settings->_profiles.size()); + } + { + // Case 2: Bad settings + Log::Comment(NoThrowString().Format( + L"Testing a pair of profiles with the same guid")); + const auto settingsObject = VerifyParseSucceeded(badProfiles); + auto settings = CascadiaSettings::FromJson(settingsObject); + + settings->_ValidateNoDuplicateProfiles(); + + VERIFY_ARE_EQUAL(static_cast(1), settings->_warnings.size()); + VERIFY_ARE_EQUAL(::TerminalApp::SettingsLoadWarnings::DuplicateProfile, settings->_warnings.at(0)); + + VERIFY_ARE_EQUAL(static_cast(1), settings->_profiles.size()); + VERIFY_ARE_EQUAL(L"profile0", settings->_profiles.at(0).GetName()); + } + { + // Case 3: Very bad settings + Log::Comment(NoThrowString().Format( + L"Testing a set of profiles, many of which with duplicated guids")); + const auto settingsObject = VerifyParseSucceeded(veryBadProfiles); + auto settings = CascadiaSettings::FromJson(settingsObject); + settings->_ValidateNoDuplicateProfiles(); + VERIFY_ARE_EQUAL(static_cast(1), settings->_warnings.size()); + VERIFY_ARE_EQUAL(::TerminalApp::SettingsLoadWarnings::DuplicateProfile, settings->_warnings.at(0)); + + VERIFY_ARE_EQUAL(static_cast(4), settings->_profiles.size()); + VERIFY_ARE_EQUAL(L"profile0", settings->_profiles.at(0).GetName()); + VERIFY_ARE_EQUAL(L"profile1", settings->_profiles.at(1).GetName()); + VERIFY_ARE_EQUAL(L"profile4", settings->_profiles.at(2).GetName()); + VERIFY_ARE_EQUAL(L"profile6", settings->_profiles.at(3).GetName()); + } + } + + void SettingsTests::ValidateManyWarnings() + { + const std::string badProfiles{ R"( + { + "globals": { + "defaultProfile": "{6239a42c-1111-49a3-80bd-e8fdd045185c}" + }, + "profiles": [ + { + "name" : "profile0", + "guid": "{6239a42c-2222-49a3-80bd-e8fdd045185c}" + }, + { + "name" : "profile1", + "guid": "{6239a42c-3333-49a3-80bd-e8fdd045185c}" + }, + { + "name" : "profile2", + "guid": "{6239a42c-2222-49a3-80bd-e8fdd045185c}" + } + ] + })" }; + + // Case 2: Bad settings + Log::Comment(NoThrowString().Format( + L"Testing a pair of profiles with the same guid")); + const auto settingsObject = VerifyParseSucceeded(badProfiles); + auto settings = CascadiaSettings::FromJson(settingsObject); + + settings->_ValidateSettings(); + + VERIFY_ARE_EQUAL(static_cast(2), settings->_warnings.size()); + VERIFY_ARE_EQUAL(::TerminalApp::SettingsLoadWarnings::DuplicateProfile, settings->_warnings.at(0)); + VERIFY_ARE_EQUAL(::TerminalApp::SettingsLoadWarnings::MissingDefaultProfile, settings->_warnings.at(1)); + + VERIFY_ARE_EQUAL(static_cast(2), settings->_profiles.size()); + VERIFY_ARE_EQUAL(settings->_globals.GetDefaultProfile(), settings->_profiles.at(0).GetGuid()); + } + } diff --git a/src/cascadia/TerminalApp/App.cpp b/src/cascadia/TerminalApp/App.cpp index f56421e205a..fdc8f795da7 100644 --- a/src/cascadia/TerminalApp/App.cpp +++ b/src/cascadia/TerminalApp/App.cpp @@ -26,6 +26,94 @@ namespace winrt using IInspectable = Windows::Foundation::IInspectable; } +// clang-format off +// !!! IMPORTANT !!! +// Make sure that these keys are in the same order as the +// SettingsLoadWarnings/Errors enum is! +static const std::array settingsLoadWarningsLabels { + L"MissingDefaultProfileText", + L"DuplicateProfileText" +}; +static const std::array settingsLoadErrorsLabels { + L"NoProfilesText" +}; +// clang-format on + +// Function Description: +// - General-purpose helper for looking up a localized string for a +// warning/error. First will look for the given key in the provided map of +// keys->strings, where the values in the map are ResourceKeys. If it finds +// one, it will lookup the localized string from that ResourceKey. +// - If it does not find a key, it'll return an empty string +// Arguments: +// - key: the value to use to look for a resource key in the given map +// - map: A map of keys->Resource keys. +// - loader: the ScopedResourceLoader to use to look up the localized string. +// Return Value: +// - the localized string for the given type, if it exists. +template +static winrt::hstring _GetMessageText(uint32_t index, std::array keys, ScopedResourceLoader loader) +{ + if (index < keys.size()) + { + return loader.GetLocalizedString(keys.at(index)); + } + return {}; +} + +// Function Description: +// - Gets the text from our ResourceDictionary for the given +// SettingsLoadWarning. If there is no such text, we'll return nullptr. +// - The warning should have an entry in settingsLoadWarningsLabels. +// Arguments: +// - warning: the SettingsLoadWarnings value to get the localized text for. +// - loader: the ScopedResourceLoader to use to look up the localized string. +// Return Value: +// - localized text for the given warning +static winrt::hstring _GetWarningText(::TerminalApp::SettingsLoadWarnings warning, ScopedResourceLoader loader) +{ + return _GetMessageText(static_cast(warning), settingsLoadWarningsLabels, loader); +} + +// Function Description: +// - Gets the text from our ResourceDictionary for the given +// SettingsLoadError. If there is no such text, we'll return nullptr. +// - The warning should have an entry in settingsLoadErrorsLabels. +// Arguments: +// - error: the SettingsLoadErrors value to get the localized text for. +// - loader: the ScopedResourceLoader to use to look up the localized string. +// Return Value: +// - localized text for the given error +static winrt::hstring _GetErrorText(::TerminalApp::SettingsLoadErrors error, ScopedResourceLoader loader) +{ + return _GetMessageText(static_cast(error), settingsLoadErrorsLabels, loader); +} + +// Function Description: +// - Creates a Run of text to display an error message. The text is yellow or +// red for dark/light theme, respectively. +// Arguments: +// - text: The text of the error message. +// - resources: The application's resource loader. +// Return Value: +// - The fully styled text run. +static Documents::Run _BuildErrorRun(const winrt::hstring& text, const ResourceDictionary& resources) +{ + Documents::Run textRun; + textRun.Text(text); + + // Color the text red (light theme) or yellow (dark theme) based on the system theme + winrt::IInspectable key = winrt::box_value(L"ErrorTextBrush"); + if (resources.HasKey(key)) + { + winrt::IInspectable g = resources.Lookup(key); + auto brush = g.try_as(); + textRun.Foreground(brush); + } + + return textRun; +} + namespace winrt::TerminalApp::implementation { App::App() : @@ -177,6 +265,81 @@ namespace winrt::TerminalApp::implementation _ShowDialog(winrt::box_value(title), winrt::box_value(message), buttonText); } + // Method Description: + // - Displays a dialog for errors found while loading or validating the + // settings. Uses the resources under the provided title and content keys + // as the title and first content of the dialog, then also displays a + // message for whatever exception was found while validating the settings. + // - Only one dialog can be visible at a time. If another dialog is visible + // when this is called, nothing happens. See _ShowDialog for details + // Arguments: + // - titleKey: The key to use to lookup the title text from our resources. + // - contentKey: The key to use to lookup the content text from our resources. + void App::_ShowLoadErrorsDialog(const winrt::hstring& titleKey, + const winrt::hstring& contentKey) + { + auto title = _resourceLoader.GetLocalizedString(titleKey); + auto buttonText = _resourceLoader.GetLocalizedString(L"Ok"); + + Controls::TextBlock warningsTextBlock; + // Make sure you can copy-paste + warningsTextBlock.IsTextSelectionEnabled(true); + // Make sure the lines of text wrap + warningsTextBlock.TextWrapping(TextWrapping::Wrap); + + winrt::Windows::UI::Xaml::Documents::Run errorRun; + const auto errorLabel = _resourceLoader.GetLocalizedString(contentKey); + errorRun.Text(errorLabel); + warningsTextBlock.Inlines().Append(errorRun); + + if (FAILED(_settingsLoadedResult)) + { + if (!_settingsLoadExceptionText.empty()) + { + warningsTextBlock.Inlines().Append(_BuildErrorRun(_settingsLoadExceptionText, Resources())); + } + } + + // Add a note that we're using the default settings in this case. + winrt::Windows::UI::Xaml::Documents::Run usingDefaultsRun; + const auto usingDefaultsText = _resourceLoader.GetLocalizedString(L"UsingDefaultSettingsText"); + usingDefaultsRun.Text(usingDefaultsText); + warningsTextBlock.Inlines().Append(usingDefaultsRun); + + _ShowDialog(winrt::box_value(title), warningsTextBlock, buttonText); + } + + // Method Description: + // - Displays a dialog for warnings found while loading or validating the + // settings. Displays messages for whatever warnings were found while + // validating the settings. + // - Only one dialog can be visible at a time. If another dialog is visible + // when this is called, nothing happens. See _ShowDialog for details + void App::_ShowLoadWarningsDialog() + { + auto title = _resourceLoader.GetLocalizedString(L"SettingsValidateErrorTitle"); + auto buttonText = _resourceLoader.GetLocalizedString(L"Ok"); + + Controls::TextBlock warningsTextBlock; + // Make sure you can copy-paste + warningsTextBlock.IsTextSelectionEnabled(true); + // Make sure the lines of text wrap + warningsTextBlock.TextWrapping(TextWrapping::Wrap); + + const auto& warnings = _settings->GetWarnings(); + for (const auto& warning : warnings) + { + // Try looking up the warning message key for each warning. + const auto warningText = _GetWarningText(warning, _resourceLoader); + if (!warningText.empty()) + { + warningsTextBlock.Inlines().Append(_BuildErrorRun(warningText, Resources())); + } + } + + _ShowDialog(winrt::box_value(title), warningsTextBlock, buttonText); + } + // Method Description: // - Show a dialog with "About" information. Displays the app's Display // Name, version, getting started link, documentation link, and release @@ -261,7 +424,11 @@ namespace winrt::TerminalApp::implementation { const winrt::hstring titleKey = L"InitialJsonParseErrorTitle"; const winrt::hstring textKey = L"InitialJsonParseErrorText"; - _ShowOkDialog(titleKey, textKey); + _ShowLoadErrorsDialog(titleKey, textKey); + } + else if (_settingsLoadedResult == S_FALSE) + { + _ShowLoadWarningsDialog(); } } @@ -314,7 +481,8 @@ namespace winrt::TerminalApp::implementation auto keyBindings = _settings->GetKeybindings(); const GUID defaultProfileGuid = _settings->GlobalSettings().GetDefaultProfile(); - auto const profileCount = gsl::narrow_cast(_settings->GetProfiles().size()); // the number of profiles should not change in the loop for this to work + // the number of profiles should not change in the loop for this to work + auto const profileCount = gsl::narrow_cast(_settings->GetProfiles().size()); for (int profileIndex = 0; profileIndex < profileCount; profileIndex++) { const auto& profile = _settings->GetProfiles()[profileIndex]; @@ -324,7 +492,8 @@ namespace winrt::TerminalApp::implementation if (profileIndex < 9) { // enum value for ShortcutAction::NewTabProfileX; 0==NewTabProfile0 - auto profileKeyChord = keyBindings.GetKeyBinding(static_cast(profileIndex + static_cast(ShortcutAction::NewTabProfile0))); + const auto action = static_cast(profileIndex + static_cast(ShortcutAction::NewTabProfile0)); + auto profileKeyChord = keyBindings.GetKeyBinding(action); // make sure we find one to display if (profileKeyChord) @@ -512,13 +681,20 @@ namespace winrt::TerminalApp::implementation { auto newSettings = CascadiaSettings::LoadAll(saveOnLoad); _settings = std::move(newSettings); - hr = S_OK; + const auto& warnings = _settings->GetWarnings(); + hr = warnings.size() == 0 ? S_OK : S_FALSE; } catch (const winrt::hresult_error& e) { hr = e.code(); + _settingsLoadExceptionText = e.message(); LOG_HR(hr); } + catch (const ::TerminalApp::SettingsException& ex) + { + hr = E_INVALIDARG; + _settingsLoadExceptionText = _GetErrorText(ex.Error(), _resourceLoader); + } catch (...) { hr = wil::ResultFromCaughtException(); @@ -631,11 +807,17 @@ namespace winrt::TerminalApp::implementation _root.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [this]() { const winrt::hstring titleKey = L"ReloadJsonParseErrorTitle"; const winrt::hstring textKey = L"ReloadJsonParseErrorText"; - _ShowOkDialog(titleKey, textKey); + _ShowLoadErrorsDialog(titleKey, textKey); }); return; } + else if (_settingsLoadedResult == S_FALSE) + { + _root.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [this]() { + _ShowLoadWarningsDialog(); + }); + } // Here, we successfully reloaded the settings, and created a new // TerminalSettings object. @@ -1385,7 +1567,8 @@ namespace winrt::TerminalApp::implementation // Takes into account a special case for an error condition for a comma // Arguments: // - MenuFlyoutItem that will be displayed, and a KeyChord to map an accelerator - void App::_SetAcceleratorForMenuItem(Windows::UI::Xaml::Controls::MenuFlyoutItem& menuItem, const winrt::Microsoft::Terminal::Settings::KeyChord& keyChord) + void App::_SetAcceleratorForMenuItem(Controls::MenuFlyoutItem& menuItem, + const winrt::Microsoft::Terminal::Settings::KeyChord& keyChord) { #ifdef DEP_MICROSOFT_UI_XAML_708_FIXED // work around https://github.com/microsoft/microsoft-ui-xaml/issues/708 in case of VK_OEM_COMMA @@ -1426,7 +1609,8 @@ namespace winrt::TerminalApp::implementation // - the terminal settings // Return value: // - the desired connection - TerminalConnection::ITerminalConnection App::_CreateConnectionFromSettings(GUID profileGuid, winrt::Microsoft::Terminal::Settings::TerminalSettings settings) + TerminalConnection::ITerminalConnection App::_CreateConnectionFromSettings(GUID profileGuid, + winrt::Microsoft::Terminal::Settings::TerminalSettings settings) { const auto* const profile = _settings->FindProfile(profileGuid); TerminalConnection::ITerminalConnection connection{ nullptr }; @@ -1437,9 +1621,12 @@ namespace winrt::TerminalApp::implementation connectionType = profile->GetConnectionType(); } - if (profile->HasConnectionType() && profile->GetConnectionType() == AzureConnectionType && TerminalConnection::AzureConnection::IsAzureConnectionAvailable()) + if (profile->HasConnectionType() && + profile->GetConnectionType() == AzureConnectionType && + TerminalConnection::AzureConnection::IsAzureConnectionAvailable()) { - connection = TerminalConnection::AzureConnection(settings.InitialRows(), settings.InitialCols()); + connection = TerminalConnection::AzureConnection(settings.InitialRows(), + settings.InitialCols()); } else { @@ -1484,6 +1671,6 @@ namespace winrt::TerminalApp::implementation // These macros will define them both for you. DEFINE_EVENT(App, TitleChanged, _titleChangeHandlers, TerminalControl::TitleChangedEventArgs); DEFINE_EVENT(App, LastTabClosed, _lastTabClosedHandlers, winrt::TerminalApp::LastTabClosedEventArgs); - DEFINE_EVENT_WITH_TYPED_EVENT_HANDLER(App, SetTitleBarContent, _setTitleBarContentHandlers, TerminalApp::App, winrt::Windows::UI::Xaml::UIElement); - DEFINE_EVENT_WITH_TYPED_EVENT_HANDLER(App, RequestedThemeChanged, _requestedThemeChangedHandlers, TerminalApp::App, winrt::Windows::UI::Xaml::ElementTheme); + DEFINE_EVENT_WITH_TYPED_EVENT_HANDLER(App, SetTitleBarContent, _setTitleBarContentHandlers, TerminalApp::App, UIElement); + DEFINE_EVENT_WITH_TYPED_EVENT_HANDLER(App, RequestedThemeChanged, _requestedThemeChangedHandlers, TerminalApp::App, ElementTheme); } diff --git a/src/cascadia/TerminalApp/App.h b/src/cascadia/TerminalApp/App.h index 0d52350357d..3d34f9656c3 100644 --- a/src/cascadia/TerminalApp/App.h +++ b/src/cascadia/TerminalApp/App.h @@ -63,6 +63,7 @@ namespace winrt::TerminalApp::implementation std::unique_ptr<::TerminalApp::CascadiaSettings> _settings; HRESULT _settingsLoadedResult; + winrt::hstring _settingsLoadExceptionText{}; bool _loadedInitialSettings; std::shared_mutex _dialogLock; @@ -80,6 +81,8 @@ namespace winrt::TerminalApp::implementation const winrt::hstring& closeButtonText); void _ShowOkDialog(const winrt::hstring& titleKey, const winrt::hstring& contentKey); void _ShowAboutDialog(); + void _ShowLoadWarningsDialog(); + void _ShowLoadErrorsDialog(const winrt::hstring& titleKey, const winrt::hstring& contentKey); [[nodiscard]] HRESULT _TryLoadSettings(const bool saveOnLoad) noexcept; void _LoadSettings(); diff --git a/src/cascadia/TerminalApp/App.xaml b/src/cascadia/TerminalApp/App.xaml index 4cb4d0eea66..7b5ce85ed39 100644 --- a/src/cascadia/TerminalApp/App.xaml +++ b/src/cascadia/TerminalApp/App.xaml @@ -44,6 +44,11 @@ the MIT License. See LICENSE in the project root for license information. --> + + + diff --git a/src/cascadia/TerminalApp/CascadiaSettings.cpp b/src/cascadia/TerminalApp/CascadiaSettings.cpp index 2f0602d3475..6fe28a9b4f5 100644 --- a/src/cascadia/TerminalApp/CascadiaSettings.cpp +++ b/src/cascadia/TerminalApp/CascadiaSettings.cpp @@ -643,3 +643,132 @@ Profile CascadiaSettings::_CreateDefaultProfile(const std::wstring_view name) return newProfile; } + +// Method Description: +// - Gets our list of warnings we found during loading. These are things that we +// knew were bad when we called `_ValidateSettings` last. +// Return Value: +// - a reference to our list of warnings. +std::vector& CascadiaSettings::GetWarnings() +{ + return _warnings; +} + +// Method Description: +// - Attempts to validate this settings structure. If there are critical errors +// found, they'll be thrown as a SettingsLoadError. Non-critical errors, such +// as not finding the default profile, will only result in an error. We'll add +// all these warnings to our list of warnings, and the application can chose +// to display these to the user. +// Arguments: +// - +// Return Value: +// - +void CascadiaSettings::_ValidateSettings() +{ + _warnings.clear(); + + // Make sure to check that profiles exists at all first and foremost: + _ValidateProfilesExist(); + + // Then do some validation on the profiles. The order of these does not + // terribly matter. + _ValidateNoDuplicateProfiles(); + _ValidateDefaultProfileExists(); +} + +// Method Description: +// - Checks if the settings contain profiles at all. As we'll need to have some +// profiles at all, we'll throw an error if there aren't any profiles. +void CascadiaSettings::_ValidateProfilesExist() +{ + const bool hasProfiles = !_profiles.empty(); + if (!hasProfiles) + { + // Throw an exception. This is an invalid state, and we want the app to + // be able to gracefully use the default settings. + + // We can't add the warning to the list of warnings here, because this + // object is not going to be returned at any point. + + throw ::TerminalApp::SettingsException(::TerminalApp::SettingsLoadErrors::NoProfiles); + } +} + +// Method Description: +// - Checks if the "globals.defaultProfile" is set to one of the profiles we +// actually have. If the value is unset, or the value is set to something that +// doesn't exist in the list of profiles, we'll arbitrarily pick the first +// profile to use temporarily as the default. +// - Appends a SettingsLoadWarnings::MissingDefaultProfile to our list of +// warnings if we failed to find the default. +void CascadiaSettings::_ValidateDefaultProfileExists() +{ + const auto defaultProfileGuid = GlobalSettings().GetDefaultProfile(); + const bool nullDefaultProfile = defaultProfileGuid == GUID{}; + bool defaultProfileNotInProfiles = true; + for (const auto& profile : _profiles) + { + if (profile.GetGuid() == defaultProfileGuid) + { + defaultProfileNotInProfiles = false; + break; + } + } + + if (nullDefaultProfile || defaultProfileNotInProfiles) + { + _warnings.push_back(::TerminalApp::SettingsLoadWarnings::MissingDefaultProfile); + // Use the first profile as the new default + + // _temporarily_ set the default profile to the first profile. Because + // we're adding a warning, this settings change won't be re-serialized. + GlobalSettings().SetDefaultProfile(_profiles[0].GetGuid()); + } +} + +// Method Description: +// - Checks to make sure there aren't any duplicate profiles in the list of +// profiles. If so, we'll remove the subsequent entries (temporarily), as they +// won't be accessible anyways. +// - Appends a SettingsLoadWarnings::DuplicateProfile to our list of warnings if +// we find any such duplicate. +void CascadiaSettings::_ValidateNoDuplicateProfiles() +{ + bool foundDupe = false; + + std::vector indiciesToDelete{}; + + // Helper to establish an ordering on guids + struct GuidEquality + { + bool operator()(const GUID& lhs, const GUID& rhs) const + { + return memcmp(&lhs, &rhs, sizeof(rhs)) < 0; + } + }; + std::set uniqueGuids{}; + + // Try collecting all the unique guids. If we ever encounter a guid that's + // already in the set, then we need to delete that profile. + for (int i = 0; i < _profiles.size(); i++) + { + if (!uniqueGuids.insert(_profiles.at(i).GetGuid()).second) + { + foundDupe = true; + indiciesToDelete.push_back(i); + } + } + + // Remove all the duplicates we've marked + // Walk backwards, so we don't accidentally shift any of the elements + for (auto iter = indiciesToDelete.rbegin(); iter != indiciesToDelete.rend(); iter++) + { + _profiles.erase(_profiles.begin() + *iter); + } + + if (foundDupe) + { + _warnings.push_back(::TerminalApp::SettingsLoadWarnings::DuplicateProfile); + } +} diff --git a/src/cascadia/TerminalApp/CascadiaSettings.h b/src/cascadia/TerminalApp/CascadiaSettings.h index 6cb941dfea2..6334705bdc4 100644 --- a/src/cascadia/TerminalApp/CascadiaSettings.h +++ b/src/cascadia/TerminalApp/CascadiaSettings.h @@ -3,7 +3,7 @@ Copyright (c) Microsoft Corporation Licensed under the MIT license. Module Name: -- CascadiaSettings.hpp +- CascadiaSettings.h Abstract: - This class acts as the container for all app settings. It's composed of two @@ -18,10 +18,17 @@ Author(s): #pragma once #include #include "GlobalAppSettings.h" +#include "TerminalWarnings.h" #include "Profile.h" static constexpr GUID AzureConnectionType = { 0xd9fcfdfa, 0xa479, 0x412c, { 0x83, 0xb7, 0xc5, 0x64, 0xe, 0x61, 0xcd, 0x62 } }; +// fwdecl unittest classes +namespace TerminalAppLocalTests +{ + class SettingsTests; +} + namespace TerminalApp { class CascadiaSettings; @@ -53,9 +60,12 @@ class TerminalApp::CascadiaSettings final void CreateDefaults(); + std::vector& GetWarnings(); + private: GlobalAppSettings _globals; std::vector _profiles; + std::vector _warnings{}; void _CreateDefaultKeybindings(); void _CreateDefaultSchemes(); @@ -65,8 +75,15 @@ class TerminalApp::CascadiaSettings final static void _WriteSettings(const std::string_view content); static std::optional _ReadSettings(); + void _ValidateSettings(); + void _ValidateProfilesExist(); + void _ValidateDefaultProfileExists(); + void _ValidateNoDuplicateProfiles(); + static bool _isPowerShellCoreInstalledInPath(const std::wstring_view programFileEnv, std::filesystem::path& cmdline); static bool _isPowerShellCoreInstalled(std::filesystem::path& cmdline); static void _AppendWslProfiles(std::vector& profileStorage); static Profile _CreateDefaultProfile(const std::wstring_view name); + + friend class TerminalAppLocalTests::SettingsTests; }; diff --git a/src/cascadia/TerminalApp/CascadiaSettingsSerialization.cpp b/src/cascadia/TerminalApp/CascadiaSettingsSerialization.cpp index 40e81ada29a..dfaf68aa05c 100644 --- a/src/cascadia/TerminalApp/CascadiaSettingsSerialization.cpp +++ b/src/cascadia/TerminalApp/CascadiaSettingsSerialization.cpp @@ -41,7 +41,10 @@ std::unique_ptr CascadiaSettings::LoadAll(const bool saveOnLoa std::optional fileData = _ReadSettings(); const bool foundFile = fileData.has_value(); - if (foundFile) + // Make sure the file isn't totally empty. If it is, we'll treat the file + // like it doesn't exist at all. + const bool fileHasData = foundFile && !fileData.value().empty(); + if (foundFile && fileHasData) { const auto actualData = fileData.value(); @@ -59,18 +62,20 @@ std::unique_ptr CascadiaSettings::LoadAll(const bool saveOnLoa // `parse` will return false if it fails. if (!reader->parse(actualDataStart, actualData.c_str() + actualData.size(), &root, &errs)) { - // TODO:GH#990 display this exception text to the user, in a - // copy-pasteable way. + // This will be caught by App::_TryLoadSettings, who will display + // the text to the user. throw winrt::hresult_error(WEB_E_INVALID_JSON_STRING, winrt::to_hstring(errs)); } resultPtr = FromJson(root); - if (resultPtr->GlobalSettings().GetDefaultProfile() == GUID{}) - { - throw winrt::hresult_invalid_argument(); - } + // If this throws, the app will catch it and use the default settings (temporarily) + resultPtr->_ValidateSettings(); + + const bool foundWarnings = resultPtr->_warnings.size() > 0; - if (saveOnLoad) + // Don't save on load if there were warnings - we tried to gracefully + // handle them. + if (saveOnLoad && !foundWarnings) { // Logically compare the json we've parsed from the file to what // we'd serialize at runtime. If the values are different, then diff --git a/src/cascadia/TerminalApp/Resources/en-US/Resources.resw b/src/cascadia/TerminalApp/Resources/en-US/Resources.resw index 8173dcac8e8..c461da5b42b 100644 --- a/src/cascadia/TerminalApp/Resources/en-US/Resources.resw +++ b/src/cascadia/TerminalApp/Resources/en-US/Resources.resw @@ -1,17 +1,17 @@  - @@ -118,17 +118,39 @@ System.Resources.ResXResourceWriter, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089 - Settings could not be loaded from file - temporarily using the default settings. Check for syntax errors, including trailing commas. + Settings could not be loaded from file. Check for syntax errors, including trailing commas. + + + + Could not find your default profile in your list of profiles - using the first profile. Check to make sure the defaultProfile matches the GUID of one of your profiles. + + + + Found multiple profiles with the same GUID in your settings file - ignoring duplicates. Make sure each profile's GUID is unique. + + + + No profiles were found in your settings. + + + + Settings could not be reloaded from file. Check for syntax errors, including trailing commas. + + + + +Temporarily using the Windows Terminal default settings. + Failed to load settings + + Encountered errors while loading user settings + Ok - - Settings could not be reloaded from file. Check for syntax errors, including trailing commas. - Failed to reload settings @@ -171,4 +193,4 @@ Settings - \ No newline at end of file + diff --git a/src/cascadia/TerminalApp/TerminalWarnings.h b/src/cascadia/TerminalApp/TerminalWarnings.h new file mode 100644 index 00000000000..b926f1c1ab7 --- /dev/null +++ b/src/cascadia/TerminalApp/TerminalWarnings.h @@ -0,0 +1,57 @@ +/*++ +Copyright (c) Microsoft Corporation +Licensed under the MIT license. + +Module Name: +- TerminalWarnings.h + +Abstract: +- This file contains definitions for warnings, errors and exceptions used by the + Windows Terminal + +Author(s): +- Mike Griese - August 2019 + +--*/ +#pragma once + +namespace TerminalApp +{ + // SettingsLoadWarnings are scenarios where the settings contained + // information we knew was invalid, but we could recover from. + enum class SettingsLoadWarnings : uint32_t + { + MissingDefaultProfile = 0, + DuplicateProfile = 1 + }; + + // SettingsLoadWarnings are scenarios where the settings had invalid state + // that we could not recover from. + enum class SettingsLoadErrors : uint32_t + { + NoProfiles = 0 + }; + + // This is a helper class to wrap up a SettingsLoadErrors into a proper + // exception type. + class SettingsException : public std::runtime_error + { + public: + SettingsException(const SettingsLoadErrors& error) : + std::runtime_error{ nullptr }, + _error{ error } {}; + + // We don't use the what() method - we want to be able to display + // localizable error messages. Catchers of this exception should use + // _GetErrorText (in App.cpp) to get the localized exception string. + const char* what() const override + { + return "Exception while loading or validating Terminal settings"; + }; + + SettingsLoadErrors Error() const noexcept { return _error; }; + + private: + const SettingsLoadErrors _error; + }; +}; diff --git a/src/cascadia/TerminalApp/lib/TerminalAppLib.vcxproj b/src/cascadia/TerminalApp/lib/TerminalAppLib.vcxproj index 2c608df6d6b..80c61d203fb 100644 --- a/src/cascadia/TerminalApp/lib/TerminalAppLib.vcxproj +++ b/src/cascadia/TerminalApp/lib/TerminalAppLib.vcxproj @@ -68,6 +68,7 @@ + diff --git a/src/inc/LibraryIncludes.h b/src/inc/LibraryIncludes.h index eb4ae8db0b7..d50e2e16394 100644 --- a/src/inc/LibraryIncludes.h +++ b/src/inc/LibraryIncludes.h @@ -43,6 +43,7 @@ #include #include #include +#include // WIL #include