From 71cd42cc1aef3d86c951d8534f811778a3833677 Mon Sep 17 00:00:00 2001 From: Michael Kitzan Date: Sun, 12 Jan 2020 19:46:23 -0800 Subject: [PATCH 1/6] Initial fix to bad background image crash --- src/cascadia/TerminalApp/AppLogic.cpp | 5 +- src/cascadia/TerminalApp/CascadiaSettings.cpp | 36 +++++++++++ src/cascadia/TerminalApp/CascadiaSettings.h | 1 + src/cascadia/TerminalApp/Profile.cpp | 8 +++ src/cascadia/TerminalApp/Profile.h | 1 + .../Resources/en-US/Resources.resw | 59 ++++++++++--------- src/cascadia/TerminalApp/TerminalWarnings.h | 3 +- 7 files changed, 82 insertions(+), 31 deletions(-) diff --git a/src/cascadia/TerminalApp/AppLogic.cpp b/src/cascadia/TerminalApp/AppLogic.cpp index bb43bca3e54..6a4bf475aff 100644 --- a/src/cascadia/TerminalApp/AppLogic.cpp +++ b/src/cascadia/TerminalApp/AppLogic.cpp @@ -27,10 +27,11 @@ namespace winrt // !!! IMPORTANT !!! // Make sure that these keys are in the same order as the // SettingsLoadWarnings/Errors enum is! -static const std::array settingsLoadWarningsLabels { +static const std::array settingsLoadWarningsLabels { USES_RESOURCE(L"MissingDefaultProfileText"), USES_RESOURCE(L"DuplicateProfileText"), - USES_RESOURCE(L"UnknownColorSchemeText") + USES_RESOURCE(L"UnknownColorSchemeText"), + USES_RESOURCE(L"InvalidBackgroundImage") }; static const std::array settingsLoadErrorsLabels { USES_RESOURCE(L"NoProfilesText"), diff --git a/src/cascadia/TerminalApp/CascadiaSettings.cpp b/src/cascadia/TerminalApp/CascadiaSettings.cpp index f520ec517bf..fbc623a5f10 100644 --- a/src/cascadia/TerminalApp/CascadiaSettings.cpp +++ b/src/cascadia/TerminalApp/CascadiaSettings.cpp @@ -198,6 +198,9 @@ void CascadiaSettings::_ValidateSettings() // just use the hardcoded defaults _ValidateAllSchemesExist(); + // Ensure all profile's with specified background images have valid files + _ValidateBackgroundImage(); + // TODO:GH#2548 ensure there's at least one key bound. Display a warning if // there's _NO_ keys bound to any actions. That's highly irregular, and // likely an indication of an error somehow. @@ -424,6 +427,39 @@ void CascadiaSettings::_ValidateAllSchemesExist() } } +// Method Description: +// - Ensures that all specified background images are valid files existing on the local machine. +// This does not verify that the background image file is encoded as an image. +// Arguments: +// - +// Return Value: +// - +// - Appends a SettingsLoadWarnings::InvalidBackgroundImage to our list of warnings if +// we find any invalid background images. +void CascadiaSettings::_ValidateBackgroundImage() +{ + bool invalidImage{ false }; + + for (auto& profile : _profiles) + { + if (profile.HasBackgroundImage()) + { + auto imagePath{ profile.GetExpandedBackgroundImagePath() }; + + if (INVALID_FILE_ATTRIBUTES == GetFileAttributes(imagePath.c_str())) + { + profile.ResetBackgroundImagePath(); + invalidImage = true; + } + } + } + + if (invalidImage) + { + _warnings.push_back(::TerminalApp::SettingsLoadWarnings::InvalidBackgroundImage); + } +} + // Method Description: // - Create a TerminalSettings object for the provided newTerminalArgs. We'll // use the newTerminalArgs to look up the profile that should be used to diff --git a/src/cascadia/TerminalApp/CascadiaSettings.h b/src/cascadia/TerminalApp/CascadiaSettings.h index 9c5daecb980..e62094f36d7 100644 --- a/src/cascadia/TerminalApp/CascadiaSettings.h +++ b/src/cascadia/TerminalApp/CascadiaSettings.h @@ -115,6 +115,7 @@ class TerminalApp::CascadiaSettings final void _ReorderProfilesToMatchUserSettingsOrder(); void _RemoveHiddenProfiles(); void _ValidateAllSchemesExist(); + void _ValidateBackgroundImage(); friend class TerminalAppLocalTests::SettingsTests; friend class TerminalAppLocalTests::ProfileTests; diff --git a/src/cascadia/TerminalApp/Profile.cpp b/src/cascadia/TerminalApp/Profile.cpp index d4b2170f4cd..09117b21519 100644 --- a/src/cascadia/TerminalApp/Profile.cpp +++ b/src/cascadia/TerminalApp/Profile.cpp @@ -880,6 +880,14 @@ winrt::hstring Profile::GetExpandedBackgroundImagePath() const return result; } +// Method Description: +// - Resets the std::optional holding the background image file path string. +// HasBackgroundImage() will return false after the execution of this function. +void Profile::ResetBackgroundImagePath() +{ + _backgroundImage.reset(); +} + // Method Description: // - Returns the name of this profile. // Arguments: diff --git a/src/cascadia/TerminalApp/Profile.h b/src/cascadia/TerminalApp/Profile.h index 6f0e0a1f50a..eab46365544 100644 --- a/src/cascadia/TerminalApp/Profile.h +++ b/src/cascadia/TerminalApp/Profile.h @@ -92,6 +92,7 @@ class TerminalApp::Profile final bool HasBackgroundImage() const noexcept; winrt::hstring GetExpandedBackgroundImagePath() const; + void ResetBackgroundImagePath(); CloseOnExitMode GetCloseOnExitMode() const noexcept; bool GetSuppressApplicationTitle() const noexcept; diff --git a/src/cascadia/TerminalApp/Resources/en-US/Resources.resw b/src/cascadia/TerminalApp/Resources/en-US/Resources.resw index 3296a65b221..51b115d6539 100644 --- a/src/cascadia/TerminalApp/Resources/en-US/Resources.resw +++ b/src/cascadia/TerminalApp/Resources/en-US/Resources.resw @@ -1,17 +1,17 @@  - @@ -210,4 +210,7 @@ Temporarily using the Windows Terminal default settings. Do you want to close all tabs? - + + Found a profile with an invalid "backgroundImage". Defaulting that profile to have no background image. Make sure that when setting a "backgroundImage", the value is a valid file path to an image. + + \ No newline at end of file diff --git a/src/cascadia/TerminalApp/TerminalWarnings.h b/src/cascadia/TerminalApp/TerminalWarnings.h index 19d6fdcd665..55dee3e2bf2 100644 --- a/src/cascadia/TerminalApp/TerminalWarnings.h +++ b/src/cascadia/TerminalApp/TerminalWarnings.h @@ -23,7 +23,8 @@ namespace TerminalApp { MissingDefaultProfile = 0, DuplicateProfile = 1, - UnknownColorScheme = 2 + UnknownColorScheme = 2, + InvalidBackgroundImage = 3 }; // SettingsLoadWarnings are scenarios where the settings had invalid state From 20ca96fe3c4b24b37ed45dea980a9f71263f15bf Mon Sep 17 00:00:00 2001 From: Michael Kitzan Date: Sun, 12 Jan 2020 20:05:04 -0800 Subject: [PATCH 2/6] Small change to function name --- src/cascadia/TerminalApp/CascadiaSettings.cpp | 4 ++-- src/cascadia/TerminalApp/CascadiaSettings.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/cascadia/TerminalApp/CascadiaSettings.cpp b/src/cascadia/TerminalApp/CascadiaSettings.cpp index fbc623a5f10..88169b19733 100644 --- a/src/cascadia/TerminalApp/CascadiaSettings.cpp +++ b/src/cascadia/TerminalApp/CascadiaSettings.cpp @@ -199,7 +199,7 @@ void CascadiaSettings::_ValidateSettings() _ValidateAllSchemesExist(); // Ensure all profile's with specified background images have valid files - _ValidateBackgroundImage(); + _ValidateBackgroundImages(); // TODO:GH#2548 ensure there's at least one key bound. Display a warning if // there's _NO_ keys bound to any actions. That's highly irregular, and @@ -436,7 +436,7 @@ void CascadiaSettings::_ValidateAllSchemesExist() // - // - Appends a SettingsLoadWarnings::InvalidBackgroundImage to our list of warnings if // we find any invalid background images. -void CascadiaSettings::_ValidateBackgroundImage() +void CascadiaSettings::_ValidateBackgroundImages() { bool invalidImage{ false }; diff --git a/src/cascadia/TerminalApp/CascadiaSettings.h b/src/cascadia/TerminalApp/CascadiaSettings.h index e62094f36d7..873c0353ce7 100644 --- a/src/cascadia/TerminalApp/CascadiaSettings.h +++ b/src/cascadia/TerminalApp/CascadiaSettings.h @@ -115,7 +115,7 @@ class TerminalApp::CascadiaSettings final void _ReorderProfilesToMatchUserSettingsOrder(); void _RemoveHiddenProfiles(); void _ValidateAllSchemesExist(); - void _ValidateBackgroundImage(); + void _ValidateBackgroundImages(); friend class TerminalAppLocalTests::SettingsTests; friend class TerminalAppLocalTests::ProfileTests; From bc5215e855cba338e7e4d04fadb6b7c465828c40 Mon Sep 17 00:00:00 2001 From: Michael Kitzan Date: Mon, 13 Jan 2020 10:58:34 -0800 Subject: [PATCH 3/6] Validates URI paths This seems like a good method of validating resource paths. --- src/cascadia/TerminalApp/CascadiaSettings.cpp | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/cascadia/TerminalApp/CascadiaSettings.cpp b/src/cascadia/TerminalApp/CascadiaSettings.cpp index 88169b19733..fdcaaacc576 100644 --- a/src/cascadia/TerminalApp/CascadiaSettings.cpp +++ b/src/cascadia/TerminalApp/CascadiaSettings.cpp @@ -444,9 +444,13 @@ void CascadiaSettings::_ValidateBackgroundImages() { if (profile.HasBackgroundImage()) { - auto imagePath{ profile.GetExpandedBackgroundImagePath() }; - - if (INVALID_FILE_ATTRIBUTES == GetFileAttributes(imagePath.c_str())) + // Attempt to convert the path to a URI, the ctor will throw if it's invalid/unparseable. + // This covers file paths on the machine, app data, URLs, and other resource paths. + try + { + winrt::Windows::Foundation::Uri imagePath{ profile.GetExpandedBackgroundImagePath() }; + } + catch (...) { profile.ResetBackgroundImagePath(); invalidImage = true; From 109868c3caf5a37e5df4ea7e9ea91ff7c6175adf Mon Sep 17 00:00:00 2001 From: Michael Kitzan Date: Mon, 13 Jan 2020 16:32:19 -0800 Subject: [PATCH 4/6] Removed autoformatting changes --- .../Resources/en-US/Resources.resw | 56 +++++++++---------- 1 file changed, 28 insertions(+), 28 deletions(-) diff --git a/src/cascadia/TerminalApp/Resources/en-US/Resources.resw b/src/cascadia/TerminalApp/Resources/en-US/Resources.resw index 51b115d6539..a5e309d84a6 100644 --- a/src/cascadia/TerminalApp/Resources/en-US/Resources.resw +++ b/src/cascadia/TerminalApp/Resources/en-US/Resources.resw @@ -1,17 +1,17 @@  - @@ -213,4 +213,4 @@ Temporarily using the Windows Terminal default settings. Found a profile with an invalid "backgroundImage". Defaulting that profile to have no background image. Make sure that when setting a "backgroundImage", the value is a valid file path to an image. - \ No newline at end of file + From ea3858c89d115e7d30268ba4fce2ee82adf7db7b Mon Sep 17 00:00:00 2001 From: Michael Kitzan Date: Tue, 14 Jan 2020 10:48:31 -0800 Subject: [PATCH 5/6] Validate icon paths --- src/cascadia/TerminalApp/AppLogic.cpp | 5 ++- src/cascadia/TerminalApp/CascadiaSettings.cpp | 38 +++++++++++++++---- src/cascadia/TerminalApp/CascadiaSettings.h | 2 +- src/cascadia/TerminalApp/Profile.cpp | 8 ++++ src/cascadia/TerminalApp/Profile.h | 1 + .../Resources/en-US/Resources.resw | 3 ++ src/cascadia/TerminalApp/TerminalWarnings.h | 3 +- 7 files changed, 48 insertions(+), 12 deletions(-) diff --git a/src/cascadia/TerminalApp/AppLogic.cpp b/src/cascadia/TerminalApp/AppLogic.cpp index 6a4bf475aff..173ff5ab2cf 100644 --- a/src/cascadia/TerminalApp/AppLogic.cpp +++ b/src/cascadia/TerminalApp/AppLogic.cpp @@ -27,11 +27,12 @@ namespace winrt // !!! IMPORTANT !!! // Make sure that these keys are in the same order as the // SettingsLoadWarnings/Errors enum is! -static const std::array settingsLoadWarningsLabels { +static const std::array settingsLoadWarningsLabels { USES_RESOURCE(L"MissingDefaultProfileText"), USES_RESOURCE(L"DuplicateProfileText"), USES_RESOURCE(L"UnknownColorSchemeText"), - USES_RESOURCE(L"InvalidBackgroundImage") + USES_RESOURCE(L"InvalidBackgroundImage"), + USES_RESOURCE(L"InvalidIcon") }; static const std::array settingsLoadErrorsLabels { USES_RESOURCE(L"NoProfilesText"), diff --git a/src/cascadia/TerminalApp/CascadiaSettings.cpp b/src/cascadia/TerminalApp/CascadiaSettings.cpp index fdcaaacc576..b6ff076404d 100644 --- a/src/cascadia/TerminalApp/CascadiaSettings.cpp +++ b/src/cascadia/TerminalApp/CascadiaSettings.cpp @@ -198,8 +198,9 @@ void CascadiaSettings::_ValidateSettings() // just use the hardcoded defaults _ValidateAllSchemesExist(); - // Ensure all profile's with specified background images have valid files - _ValidateBackgroundImages(); + // Ensure all profile's with specified images resources have valid file path. + // This validates icons and background images. + _ValidateMediaResources(); // TODO:GH#2548 ensure there's at least one key bound. Display a warning if // there's _NO_ keys bound to any actions. That's highly irregular, and @@ -428,17 +429,20 @@ void CascadiaSettings::_ValidateAllSchemesExist() } // Method Description: -// - Ensures that all specified background images are valid files existing on the local machine. -// This does not verify that the background image file is encoded as an image. +// - Ensures that all specified images resources (icons and background images) are valid URIs. +// This does not verify that the icon or background image files are encoded as an image. // Arguments: // - // Return Value: // - // - Appends a SettingsLoadWarnings::InvalidBackgroundImage to our list of warnings if // we find any invalid background images. -void CascadiaSettings::_ValidateBackgroundImages() +// - Appends a SettingsLoadWarnings::InvalidIconImage to our list of warnings if +// we find any invalid icon images. +void CascadiaSettings::_ValidateMediaResources() { - bool invalidImage{ false }; + bool invalidBackground{ false }; + bool invalidIcon{ false }; for (auto& profile : _profiles) { @@ -453,15 +457,33 @@ void CascadiaSettings::_ValidateBackgroundImages() catch (...) { profile.ResetBackgroundImagePath(); - invalidImage = true; + invalidBackground = true; + } + } + + if (profile.HasIcon()) + { + try + { + winrt::Windows::Foundation::Uri imagePath{ profile.GetExpandedIconPath() }; + } + catch (...) + { + profile.ResetIconPath(); + invalidIcon = true; } } } - if (invalidImage) + if (invalidBackground) { _warnings.push_back(::TerminalApp::SettingsLoadWarnings::InvalidBackgroundImage); } + + if (invalidIcon) + { + _warnings.push_back(::TerminalApp::SettingsLoadWarnings::InvalidIcon); + } } // Method Description: diff --git a/src/cascadia/TerminalApp/CascadiaSettings.h b/src/cascadia/TerminalApp/CascadiaSettings.h index 873c0353ce7..50fa261a187 100644 --- a/src/cascadia/TerminalApp/CascadiaSettings.h +++ b/src/cascadia/TerminalApp/CascadiaSettings.h @@ -115,7 +115,7 @@ class TerminalApp::CascadiaSettings final void _ReorderProfilesToMatchUserSettingsOrder(); void _RemoveHiddenProfiles(); void _ValidateAllSchemesExist(); - void _ValidateBackgroundImages(); + void _ValidateMediaResources(); friend class TerminalAppLocalTests::SettingsTests; friend class TerminalAppLocalTests::ProfileTests; diff --git a/src/cascadia/TerminalApp/Profile.cpp b/src/cascadia/TerminalApp/Profile.cpp index 09117b21519..9fba138aa70 100644 --- a/src/cascadia/TerminalApp/Profile.cpp +++ b/src/cascadia/TerminalApp/Profile.cpp @@ -847,6 +847,14 @@ void Profile::SetIconPath(std::wstring_view path) _icon.emplace(path); } +// Method Description: +// - Resets the std::optional holding the icon file path string. +// HasIcon() will return false after the execution of this function. +void Profile::ResetIconPath() +{ + _icon.reset(); +} + // Method Description: // - Returns this profile's icon path, if one is set. Otherwise returns the // empty string. This method will expand any environment variables in the diff --git a/src/cascadia/TerminalApp/Profile.h b/src/cascadia/TerminalApp/Profile.h index eab46365544..19979eee307 100644 --- a/src/cascadia/TerminalApp/Profile.h +++ b/src/cascadia/TerminalApp/Profile.h @@ -89,6 +89,7 @@ class TerminalApp::Profile final bool HasIcon() const noexcept; winrt::hstring GetExpandedIconPath() const; void SetIconPath(std::wstring_view path); + void ResetIconPath(); bool HasBackgroundImage() const noexcept; winrt::hstring GetExpandedBackgroundImagePath() const; diff --git a/src/cascadia/TerminalApp/Resources/en-US/Resources.resw b/src/cascadia/TerminalApp/Resources/en-US/Resources.resw index a5e309d84a6..d8e5fa3a09e 100644 --- a/src/cascadia/TerminalApp/Resources/en-US/Resources.resw +++ b/src/cascadia/TerminalApp/Resources/en-US/Resources.resw @@ -213,4 +213,7 @@ Temporarily using the Windows Terminal default settings. Found a profile with an invalid "backgroundImage". Defaulting that profile to have no background image. Make sure that when setting a "backgroundImage", the value is a valid file path to an image. + + Found a profile with an invalid "icon". Defaulting that profile to have no icon. Make sure that when setting an "icon", the value is a valid file path to an image. + diff --git a/src/cascadia/TerminalApp/TerminalWarnings.h b/src/cascadia/TerminalApp/TerminalWarnings.h index 55dee3e2bf2..35db814d999 100644 --- a/src/cascadia/TerminalApp/TerminalWarnings.h +++ b/src/cascadia/TerminalApp/TerminalWarnings.h @@ -24,7 +24,8 @@ namespace TerminalApp MissingDefaultProfile = 0, DuplicateProfile = 1, UnknownColorScheme = 2, - InvalidBackgroundImage = 3 + InvalidBackgroundImage = 3, + InvalidIcon = 4 }; // SettingsLoadWarnings are scenarios where the settings had invalid state From 7bff2330d012253936c94f0620a964fcd992982b Mon Sep 17 00:00:00 2001 From: Michael Kitzan Date: Tue, 14 Jan 2020 10:50:49 -0800 Subject: [PATCH 6/6] Remove trailing space in comment --- src/cascadia/TerminalApp/CascadiaSettings.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cascadia/TerminalApp/CascadiaSettings.cpp b/src/cascadia/TerminalApp/CascadiaSettings.cpp index b6ff076404d..d51cff25532 100644 --- a/src/cascadia/TerminalApp/CascadiaSettings.cpp +++ b/src/cascadia/TerminalApp/CascadiaSettings.cpp @@ -429,7 +429,7 @@ void CascadiaSettings::_ValidateAllSchemesExist() } // Method Description: -// - Ensures that all specified images resources (icons and background images) are valid URIs. +// - Ensures that all specified images resources (icons and background images) are valid URIs. // This does not verify that the icon or background image files are encoded as an image. // Arguments: // -