Skip to content

Commit

Permalink
Add serialization error handling to settings projection layer (#7576)
Browse files Browse the repository at this point in the history
Now that CascadiaSettings is a WinRT object, we need to update the error
handling a bit. Making it a WinRT object limits our errors to be
hresults. So we moved all the error handling down a layer to when we
load the settings object.

- Warnings encountered during validation are saved to `Warnings()`.
- Errors encountered during validation are saved to `GetLoadingError()`.
- Deserialization errors (mainly from JsonUtils) are saved to
  `GetDeserializationErrorMessage()`.

## References
#7141 - CascadiaSettings is a settings object
#885 - this makes ripping out CascadiaSettings into
     TerminalSettingsModel much easier

## Validation Steps Performed
* [x] Tests passed
- [x] Deployment succeeded
   - tested with invalid JSON (deserialization error)
   - tested with missing DefaultProfile (validation error)
  • Loading branch information
carlos-zamora authored Sep 11, 2020
1 parent 1377dbc commit 892cf05
Show file tree
Hide file tree
Showing 19 changed files with 358 additions and 297 deletions.
144 changes: 72 additions & 72 deletions src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp

Large diffs are not rendered by default.

6 changes: 3 additions & 3 deletions src/cascadia/TerminalApp/ActionAndArgs.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ namespace winrt::TerminalApp::implementation
{ UnboundKey, ShortcutAction::Invalid },
};

using ParseResult = std::tuple<IActionArgs, std::vector<::TerminalApp::SettingsLoadWarnings>>;
using ParseResult = std::tuple<IActionArgs, std::vector<TerminalApp::SettingsLoadWarnings>>;
using ParseActionFunction = std::function<ParseResult(const Json::Value&)>;

// This is a map of ShortcutAction->function<IActionArgs(Json::Value)>. It holds
Expand Down Expand Up @@ -169,7 +169,7 @@ namespace winrt::TerminalApp::implementation
// - a deserialized ActionAndArgs corresponding to the values in json, or
// null if we failed to deserialize an action.
winrt::com_ptr<ActionAndArgs> ActionAndArgs::FromJson(const Json::Value& json,
std::vector<::TerminalApp::SettingsLoadWarnings>& warnings)
std::vector<TerminalApp::SettingsLoadWarnings>& warnings)
{
// Invalid is our placeholder that the action was not parsed.
ShortcutAction action = ShortcutAction::Invalid;
Expand Down Expand Up @@ -208,7 +208,7 @@ namespace winrt::TerminalApp::implementation
// does, we'll try to deserialize any "args" that were provided with
// the binding.
IActionArgs args{ nullptr };
std::vector<::TerminalApp::SettingsLoadWarnings> parseWarnings;
std::vector<TerminalApp::SettingsLoadWarnings> parseWarnings;
const auto deserializersIter = argParsers.find(action);
if (deserializersIter != argParsers.end())
{
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalApp/ActionAndArgs.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ namespace winrt::TerminalApp::implementation
{
static const std::map<std::string_view, ShortcutAction, std::less<>> ActionKeyNamesMap;
static winrt::com_ptr<ActionAndArgs> FromJson(const Json::Value& json,
std::vector<::TerminalApp::SettingsLoadWarnings>& warnings);
std::vector<TerminalApp::SettingsLoadWarnings>& warnings);

ActionAndArgs() = default;
hstring GenerateName() const;
Expand Down
12 changes: 6 additions & 6 deletions src/cascadia/TerminalApp/ActionArgs.h
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
namespace winrt::TerminalApp::implementation
{
using namespace ::TerminalApp;
using FromJsonResult = std::tuple<winrt::TerminalApp::IActionArgs, std::vector<::TerminalApp::SettingsLoadWarnings>>;
using FromJsonResult = std::tuple<winrt::TerminalApp::IActionArgs, std::vector<TerminalApp::SettingsLoadWarnings>>;

struct ActionEventArgs : public ActionEventArgsT<ActionEventArgs>
{
Expand Down Expand Up @@ -202,7 +202,7 @@ namespace winrt::TerminalApp::implementation
JsonUtils::GetValueForKey(json, DirectionKey, args->_Direction);
if (args->_Direction == TerminalApp::Direction::None)
{
return { nullptr, { ::TerminalApp::SettingsLoadWarnings::MissingRequiredParameter } };
return { nullptr, { TerminalApp::SettingsLoadWarnings::MissingRequiredParameter } };
}
else
{
Expand Down Expand Up @@ -237,7 +237,7 @@ namespace winrt::TerminalApp::implementation
JsonUtils::GetValueForKey(json, DirectionKey, args->_Direction);
if (args->_Direction == TerminalApp::Direction::None)
{
return { nullptr, { ::TerminalApp::SettingsLoadWarnings::MissingRequiredParameter } };
return { nullptr, { TerminalApp::SettingsLoadWarnings::MissingRequiredParameter } };
}
else
{
Expand Down Expand Up @@ -299,7 +299,7 @@ namespace winrt::TerminalApp::implementation
JsonUtils::GetValueForKey(json, InputKey, args->_Input);
if (args->_Input.empty())
{
return { nullptr, { ::TerminalApp::SettingsLoadWarnings::MissingRequiredParameter } };
return { nullptr, { TerminalApp::SettingsLoadWarnings::MissingRequiredParameter } };
}
return { *args, {} };
}
Expand Down Expand Up @@ -395,7 +395,7 @@ namespace winrt::TerminalApp::implementation
JsonUtils::GetValueForKey(json, NameKey, args->_SchemeName);
if (args->_SchemeName.empty())
{
return { nullptr, { ::TerminalApp::SettingsLoadWarnings::MissingRequiredParameter } };
return { nullptr, { TerminalApp::SettingsLoadWarnings::MissingRequiredParameter } };
}
return { *args, {} };
}
Expand Down Expand Up @@ -486,7 +486,7 @@ namespace winrt::TerminalApp::implementation
JsonUtils::GetValueForKey(json, CommandlineKey, args->_Commandline);
if (args->_Commandline.empty())
{
return { nullptr, { ::TerminalApp::SettingsLoadWarnings::MissingRequiredParameter } };
return { nullptr, { TerminalApp::SettingsLoadWarnings::MissingRequiredParameter } };
}
return { *args, {} };
}
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalApp/AppKeyBindings.h
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ namespace winrt::TerminalApp::implementation
static Windows::System::VirtualKeyModifiers ConvertVKModifiers(winrt::Microsoft::Terminal::TerminalControl::KeyModifiers modifiers);

// Defined in AppKeyBindingsSerialization.cpp
std::vector<::TerminalApp::SettingsLoadWarnings> LayerJson(const Json::Value& json);
std::vector<TerminalApp::SettingsLoadWarnings> LayerJson(const Json::Value& json);
Json::Value ToJson();

void SetDispatch(const winrt::TerminalApp::ShortcutActionDispatch& dispatch);
Expand Down
6 changes: 3 additions & 3 deletions src/cascadia/TerminalApp/AppKeyBindingsSerialization.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -93,13 +93,13 @@ Json::Value winrt::TerminalApp::implementation::AppKeyBindings::ToJson()
// `"unbound"`, then we'll clear the keybinding from the existing keybindings.
// Arguments:
// - json: an array of Json::Value's to deserialize into our _keyShortcuts mapping.
std::vector<::TerminalApp::SettingsLoadWarnings> winrt::TerminalApp::implementation::AppKeyBindings::LayerJson(const Json::Value& json)
std::vector<SettingsLoadWarnings> winrt::TerminalApp::implementation::AppKeyBindings::LayerJson(const Json::Value& json)
{
// It's possible that the user provided keybindings have some warnings in
// them - problems that we should alert the user to, but we can recover
// from. Most of these warnings cannot be detected later in the Validate
// settings phase, so we'll collect them now.
std::vector<::TerminalApp::SettingsLoadWarnings> warnings;
std::vector<SettingsLoadWarnings> warnings;

for (const auto& value : json)
{
Expand All @@ -121,7 +121,7 @@ std::vector<::TerminalApp::SettingsLoadWarnings> winrt::TerminalApp::implementat
// TODO: GH#1334 - remove this check.
if (keys.isArray() && keys.size() > 1)
{
warnings.push_back(::TerminalApp::SettingsLoadWarnings::TooManyKeysForChord);
warnings.push_back(SettingsLoadWarnings::TooManyKeysForChord);
}

if (!validString && !validArray)
Expand Down
37 changes: 17 additions & 20 deletions src/cascadia/TerminalApp/AppLogic.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ static const winrt::hstring StartupTaskName = L"StartTerminalOnLoginTask";
// !!! IMPORTANT !!!
// Make sure that these keys are in the same order as the
// SettingsLoadWarnings/Errors enum is!
static const std::array<std::wstring_view, static_cast<uint32_t>(SettingsLoadWarnings::WARNINGS_SIZE)> settingsLoadWarningsLabels {
static const std::array<std::wstring_view, static_cast<uint32_t>(winrt::TerminalApp::SettingsLoadWarnings::WARNINGS_SIZE)> settingsLoadWarningsLabels {
USES_RESOURCE(L"MissingDefaultProfileText"),
USES_RESOURCE(L"DuplicateProfileText"),
USES_RESOURCE(L"UnknownColorSchemeText"),
Expand All @@ -41,7 +41,7 @@ static const std::array<std::wstring_view, static_cast<uint32_t>(SettingsLoadWar
USES_RESOURCE(L"LegacyGlobalsProperty"),
USES_RESOURCE(L"FailedToParseCommandJson")
};
static const std::array<std::wstring_view, static_cast<uint32_t>(SettingsLoadErrors::ERRORS_SIZE)> settingsLoadErrorsLabels {
static const std::array<std::wstring_view, static_cast<uint32_t>(winrt::TerminalApp::SettingsLoadErrors::ERRORS_SIZE)> settingsLoadErrorsLabels {
USES_RESOURCE(L"NoProfilesText"),
USES_RESOURCE(L"AllProfilesHiddenText")
};
Expand Down Expand Up @@ -76,7 +76,7 @@ static winrt::hstring _GetMessageText(uint32_t index, std::array<std::wstring_vi
// - warning: the SettingsLoadWarnings value to get the localized text for.
// Return Value:
// - localized text for the given warning
static winrt::hstring _GetWarningText(::TerminalApp::SettingsLoadWarnings warning)
static winrt::hstring _GetWarningText(winrt::TerminalApp::SettingsLoadWarnings warning)
{
return _GetMessageText(static_cast<uint32_t>(warning), settingsLoadWarningsLabels);
}
Expand All @@ -89,7 +89,7 @@ static winrt::hstring _GetWarningText(::TerminalApp::SettingsLoadWarnings warnin
// - error: the SettingsLoadErrors value to get the localized text for.
// Return Value:
// - localized text for the given error
static winrt::hstring _GetErrorText(::TerminalApp::SettingsLoadErrors error)
static winrt::hstring _GetErrorText(winrt::TerminalApp::SettingsLoadErrors error)
{
return _GetMessageText(static_cast<uint32_t>(error), settingsLoadErrorsLabels);
}
Expand Down Expand Up @@ -411,8 +411,7 @@ namespace winrt::TerminalApp::implementation
// Make sure the lines of text wrap
warningsTextBlock.TextWrapping(TextWrapping::Wrap);

const auto settingsImpl{ winrt::get_self<implementation::CascadiaSettings>(_settings) };
const auto& warnings = settingsImpl->GetWarnings();
const auto warnings = _settings.Warnings();
for (const auto& warning : warnings)
{
// Try looking up the warning message key for each warning.
Expand Down Expand Up @@ -631,27 +630,25 @@ namespace winrt::TerminalApp::implementation
auto newSettings = _isUwp ? CascadiaSettings::LoadUniversal() : CascadiaSettings::LoadAll();
_settings = newSettings;

const auto settingsImpl{ winrt::get_self<implementation::CascadiaSettings>(_settings) };
const auto& warnings = settingsImpl->GetWarnings();
hr = warnings.size() == 0 ? S_OK : S_FALSE;
if (_settings.GetLoadingError())
{
_settingsLoadExceptionText = _GetErrorText(_settings.GetLoadingError().Value());
return E_INVALIDARG;
}
else if (!_settings.GetSerializationErrorMessage().empty())
{
_settingsLoadExceptionText = _settings.GetSerializationErrorMessage();
return E_INVALIDARG;
}

hr = _settings.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());
}
catch (const ::TerminalApp::SettingsTypedDeserializationException& e)
{
hr = E_INVALIDARG;
std::string_view what{ e.what() };
_settingsLoadExceptionText = til::u8u16(what);
}
catch (...)
{
hr = wil::ResultFromCaughtException();
Expand Down
43 changes: 29 additions & 14 deletions src/cascadia/TerminalApp/CascadiaSettings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,9 @@ CascadiaSettings::CascadiaSettings() :
// - addDynamicProfiles: if true, we'll add the built-in DPGs.
CascadiaSettings::CascadiaSettings(const bool addDynamicProfiles) :
_globals{ winrt::make_self<implementation::GlobalAppSettings>() },
_profiles{ winrt::single_threaded_observable_vector<TerminalApp::Profile>() }
_profiles{ winrt::single_threaded_observable_vector<TerminalApp::Profile>() },
_warnings{ winrt::single_threaded_vector<SettingsLoadWarnings>() },
_deserializationErrorMessage{ L"" }
{
if (addDynamicProfiles)
{
Expand Down Expand Up @@ -119,9 +121,19 @@ winrt::TerminalApp::GlobalAppSettings CascadiaSettings::GlobalSettings()
// knew were bad when we called `_ValidateSettings` last.
// Return Value:
// - a reference to our list of warnings.
std::vector<TerminalApp::SettingsLoadWarnings>& CascadiaSettings::GetWarnings()
IVectorView<winrt::TerminalApp::SettingsLoadWarnings> CascadiaSettings::Warnings()
{
return _warnings;
return _warnings.GetView();
}

winrt::Windows::Foundation::IReference<winrt::TerminalApp::SettingsLoadErrors> CascadiaSettings::GetLoadingError()
{
return _loadError;
}

winrt::hstring CascadiaSettings::GetSerializationErrorMessage()
{
return _deserializationErrorMessage;
}

// Method Description:
Expand All @@ -136,7 +148,7 @@ std::vector<TerminalApp::SettingsLoadWarnings>& CascadiaSettings::GetWarnings()
// - <none>
void CascadiaSettings::_ValidateSettings()
{
_warnings.clear();
_warnings.Clear();

// Make sure to check that profiles exists at all first and foremost:
_ValidateProfilesExist();
Expand Down Expand Up @@ -198,7 +210,7 @@ void CascadiaSettings::_ValidateProfilesExist()
// 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);
throw SettingsException(TerminalApp::SettingsLoadErrors::NoProfiles);
}
}

Expand Down Expand Up @@ -252,7 +264,7 @@ void CascadiaSettings::_ValidateDefaultProfileExists()

if (nullDefaultProfile || defaultProfileNotInProfiles)
{
_warnings.push_back(::TerminalApp::SettingsLoadWarnings::MissingDefaultProfile);
_warnings.Append(TerminalApp::SettingsLoadWarnings::MissingDefaultProfile);
// Use the first profile as the new default

// _temporarily_ set the default profile to the first profile. Because
Expand Down Expand Up @@ -295,7 +307,7 @@ void CascadiaSettings::_ValidateNoDuplicateProfiles()

if (foundDupe)
{
_warnings.push_back(::TerminalApp::SettingsLoadWarnings::DuplicateProfile);
_warnings.Append(TerminalApp::SettingsLoadWarnings::DuplicateProfile);
}
}

Expand Down Expand Up @@ -384,7 +396,7 @@ void CascadiaSettings::_RemoveHiddenProfiles()
{
// Throw an exception. This is an invalid state, and we want the app to
// be able to gracefully use the default settings.
throw ::TerminalApp::SettingsException(::TerminalApp::SettingsLoadErrors::AllProfilesHidden);
throw SettingsException(TerminalApp::SettingsLoadErrors::AllProfilesHidden);
}
}

Expand Down Expand Up @@ -413,7 +425,7 @@ void CascadiaSettings::_ValidateAllSchemesExist()

if (foundInvalidScheme)
{
_warnings.push_back(::TerminalApp::SettingsLoadWarnings::UnknownColorScheme);
_warnings.Append(SettingsLoadWarnings::UnknownColorScheme);
}
}

Expand Down Expand Up @@ -468,12 +480,12 @@ void CascadiaSettings::_ValidateMediaResources()

if (invalidBackground)
{
_warnings.push_back(::TerminalApp::SettingsLoadWarnings::InvalidBackgroundImage);
_warnings.Append(TerminalApp::SettingsLoadWarnings::InvalidBackgroundImage);
}

if (invalidIcon)
{
_warnings.push_back(::TerminalApp::SettingsLoadWarnings::InvalidIcon);
_warnings.Append(TerminalApp::SettingsLoadWarnings::InvalidIcon);
}
}

Expand Down Expand Up @@ -659,8 +671,11 @@ void CascadiaSettings::_ValidateKeybindings()

if (!keybindingWarnings.empty())
{
_warnings.push_back(::TerminalApp::SettingsLoadWarnings::AtLeastOneKeybindingWarning);
_warnings.insert(_warnings.end(), keybindingWarnings.begin(), keybindingWarnings.end());
_warnings.Append(TerminalApp::SettingsLoadWarnings::AtLeastOneKeybindingWarning);
for (auto warning : keybindingWarnings)
{
_warnings.Append(warning);
}
}
}

Expand All @@ -679,7 +694,7 @@ void CascadiaSettings::_ValidateNoGlobalsKey()
{
if (auto oldGlobalsProperty{ _userSettings["globals"] })
{
_warnings.push_back(::TerminalApp::SettingsLoadWarnings::LegacyGlobalsProperty);
_warnings.Append(TerminalApp::SettingsLoadWarnings::LegacyGlobalsProperty);
}
}

Expand Down
8 changes: 6 additions & 2 deletions src/cascadia/TerminalApp/CascadiaSettings.h
Original file line number Diff line number Diff line change
Expand Up @@ -84,14 +84,18 @@ namespace winrt::TerminalApp::implementation
TerminalApp::Profile FindProfile(guid profileGuid) const noexcept;
TerminalApp::ColorScheme GetColorSchemeForProfile(const guid profileGuid) const;

std::vector<::TerminalApp::SettingsLoadWarnings>& GetWarnings();
Windows::Foundation::Collections::IVectorView<SettingsLoadWarnings> Warnings();
Windows::Foundation::IReference<SettingsLoadErrors> GetLoadingError();
hstring GetSerializationErrorMessage();

bool ApplyColorScheme(Microsoft::Terminal::TerminalControl::IControlSettings settings, hstring schemeName);

private:
com_ptr<GlobalAppSettings> _globals;
Windows::Foundation::Collections::IObservableVector<TerminalApp::Profile> _profiles;
std::vector<::TerminalApp::SettingsLoadWarnings> _warnings;
Windows::Foundation::Collections::IVector<TerminalApp::SettingsLoadWarnings> _warnings;
Windows::Foundation::IReference<SettingsLoadErrors> _loadError;
hstring _deserializationErrorMessage;

std::vector<std::unique_ptr<::TerminalApp::IDynamicProfileGenerator>> _profileGenerators;

Expand Down
5 changes: 5 additions & 0 deletions src/cascadia/TerminalApp/CascadiaSettings.idl
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

import "GlobalAppSettings.idl";
import "Profile.idl";
import "TerminalWarnings.idl";

namespace TerminalApp
{
Expand All @@ -17,6 +18,10 @@ namespace TerminalApp

AppKeyBindings Keybindings { get; };

Windows.Foundation.Collections.IVectorView<SettingsLoadWarnings> Warnings { get; };
Windows.Foundation.IReference<SettingsLoadErrors> GetLoadingError { get; };
String GetSerializationErrorMessage { get; };

Profile FindProfile(Guid profileGuid);
ColorScheme GetColorSchemeForProfile(Guid profileGuid);
}
Expand Down
Loading

0 comments on commit 892cf05

Please sign in to comment.