Skip to content

Commit

Permalink
Display these two warning cases
Browse files Browse the repository at this point in the history
  * When the keybindings are missing required args (#3522)
  * When more than one "keys" aws specified (#4239)
  • Loading branch information
zadjii-msft committed Feb 28, 2020
1 parent 2e56284 commit 45dc345
Show file tree
Hide file tree
Showing 10 changed files with 89 additions and 12 deletions.
9 changes: 8 additions & 1 deletion src/cascadia/TerminalApp/ActionArgs.h
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,14 @@ namespace winrt::TerminalApp::implementation
{
args->_Direction = ParseDirection(directionString.asString());
}
return { *args, {} };
if (args->_Direction == TerminalApp::Direction::None)
{
return { nullptr, { ::TerminalApp::SettingsLoadWarnings::MissingRequiredParameter } };
}
else
{
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::Settings::KeyModifiers modifiers);

// Defined in AppKeyBindingsSerialization.cpp
void 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
22 changes: 19 additions & 3 deletions src/cascadia/TerminalApp/AppKeyBindingsSerialization.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -431,8 +431,10 @@ static ShortcutAction GetActionFromString(const std::string_view actionString)
// `"unbound"`, then we'll clear the keybinding from the existing keybindings.
// Arguments:
// - json: and array of JsonObject's to deserialize into our _keyShortcuts mapping.
void winrt::TerminalApp::implementation::AppKeyBindings::LayerJson(const Json::Value& json)
std::vector<::TerminalApp::SettingsLoadWarnings> winrt::TerminalApp::implementation::AppKeyBindings::LayerJson(const Json::Value& json)
{
std::vector<::TerminalApp::SettingsLoadWarnings> warnings;

for (const auto& value : json)
{
if (!value.isObject())
Expand All @@ -447,6 +449,12 @@ void winrt::TerminalApp::implementation::AppKeyBindings::LayerJson(const Json::V
{
const auto validString = keys.isString();
const auto validArray = keys.isArray() && keys.size() == 1;

if (keys.isArray() && keys.size() > 1)
{
warnings.push_back(::TerminalApp::SettingsLoadWarnings::TooManyKeysForChord);
}

if (!validString && !validArray)
{
continue;
Expand Down Expand Up @@ -490,16 +498,22 @@ void winrt::TerminalApp::implementation::AppKeyBindings::LayerJson(const Json::V
// does, we'll try to deserialize any "args" that were provided with
// the binding.
IActionArgs args{ nullptr };
std::vector<::TerminalApp::SettingsLoadWarnings> warnings;
std::vector<::TerminalApp::SettingsLoadWarnings> parseWarnings;
const auto deserializersIter = argParsers.find(action);
if (deserializersIter != argParsers.end())
{
auto pfn = deserializersIter->second;
if (pfn)
{
std::tie(args, warnings) = pfn(argsVal);
std::tie(args, parseWarnings) = pfn(argsVal);
}
}
warnings.insert(warnings.end(), parseWarnings.begin(), parseWarnings.end());

if (args == nullptr)
{
continue;
}

// Try parsing the chord
try
Expand Down Expand Up @@ -528,4 +542,6 @@ void winrt::TerminalApp::implementation::AppKeyBindings::LayerJson(const Json::V
}
}
}

return warnings;
}
9 changes: 6 additions & 3 deletions src/cascadia/TerminalApp/AppLogic.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,17 @@ namespace winrt
// !!! IMPORTANT !!!
// Make sure that these keys are in the same order as the
// SettingsLoadWarnings/Errors enum is!
static const std::array<std::wstring_view, 5> settingsLoadWarningsLabels {
static const std::array<std::wstring_view, static_cast<uint32_t>(SettingsLoadWarnings::WARNINGS_SIZE)> settingsLoadWarningsLabels {
USES_RESOURCE(L"MissingDefaultProfileText"),
USES_RESOURCE(L"DuplicateProfileText"),
USES_RESOURCE(L"UnknownColorSchemeText"),
USES_RESOURCE(L"InvalidBackgroundImage"),
USES_RESOURCE(L"InvalidIcon")
USES_RESOURCE(L"InvalidIcon"),
USES_RESOURCE(L"AtLeastOneKeybindingWarning"),
USES_RESOURCE(L"TooManyKeysForChord"),
USES_RESOURCE(L"MissingRequiredParameter")
};
static const std::array<std::wstring_view, 2> settingsLoadErrorsLabels {
static const std::array<std::wstring_view, static_cast<uint32_t>(SettingsLoadErrors::ERRORS_SIZE)> settingsLoadErrorsLabels {
USES_RESOURCE(L"NoProfilesText"),
USES_RESOURCE(L"AllProfilesHiddenText")
};
Expand Down
23 changes: 23 additions & 0 deletions src/cascadia/TerminalApp/CascadiaSettings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,7 @@ void CascadiaSettings::_ValidateSettings()
// TODO:GH#3522 With variable args to keybindings, it's possible that a user
// set a keybinding without all the required args for an action. Display a
// warning if an action didn't have a required arg.
_ValidateKeybindings();
}

// Method Description:
Expand Down Expand Up @@ -651,3 +652,25 @@ GUID CascadiaSettings::_GetProfileForIndex(std::optional<int> index) const
}
return profileGuid;
}

// Method Description:
// - 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:
// - <none>
// Return Value:
// - <none>
// - Appends a SettingsLoadWarnings::InvalidBackgroundImage to our list of warnings if
// we find any invalid background images.
// - Appends a SettingsLoadWarnings::InvalidIconImage to our list of warnings if
// we find any invalid icon images.
void CascadiaSettings::_ValidateKeybindings()
{
auto keybindingWarnings = _globals.GetKeybindingsWarnings();

if (!keybindingWarnings.empty())
{
_warnings.push_back(::TerminalApp::SettingsLoadWarnings::AtLeastOneKeybindingWarning);
_warnings.insert(_warnings.end(), keybindingWarnings.begin(), keybindingWarnings.end());
}
}
1 change: 1 addition & 0 deletions src/cascadia/TerminalApp/CascadiaSettings.h
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ class TerminalApp::CascadiaSettings final
void _RemoveHiddenProfiles();
void _ValidateAllSchemesExist();
void _ValidateMediaResources();
void _ValidateKeybindings();

friend class TerminalAppLocalTests::SettingsTests;
friend class TerminalAppLocalTests::ProfileTests;
Expand Down
9 changes: 8 additions & 1 deletion src/cascadia/TerminalApp/GlobalAppSettings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ static constexpr std::wstring_view SystemThemeValue{ L"system" };

GlobalAppSettings::GlobalAppSettings() :
_keybindings{ winrt::make_self<winrt::TerminalApp::implementation::AppKeyBindings>() },
_keybindingsWarnings{},
_colorSchemes{},
_defaultProfile{},
_alwaysShowTabs{ true },
Expand Down Expand Up @@ -330,7 +331,8 @@ void GlobalAppSettings::LayerJson(const Json::Value& json)

if (auto keybindings{ json[JsonKey(KeybindingsKey)] })
{
_keybindings->LayerJson(keybindings);
auto warnings = _keybindings->LayerJson(keybindings);
_keybindingsWarnings.insert(_keybindingsWarnings.end(), warnings.begin(), warnings.end());
}

if (auto snapToGridOnResize{ json[JsonKey(SnapToGridOnResizeKey)] })
Expand Down Expand Up @@ -537,3 +539,8 @@ void GlobalAppSettings::AddColorScheme(ColorScheme scheme)
std::wstring name{ scheme.GetName() };
_colorSchemes[name] = std::move(scheme);
}

std::vector<TerminalApp::SettingsLoadWarnings> GlobalAppSettings::GetKeybindingsWarnings() const
{
return _keybindingsWarnings;
}
3 changes: 3 additions & 0 deletions src/cascadia/TerminalApp/GlobalAppSettings.h
Original file line number Diff line number Diff line change
Expand Up @@ -83,11 +83,14 @@ class TerminalApp::GlobalAppSettings final

void ApplyToSettings(winrt::Microsoft::Terminal::Settings::TerminalSettings& settings) const noexcept;

std::vector<TerminalApp::SettingsLoadWarnings> GetKeybindingsWarnings() const;

GETSET_PROPERTY(bool, SnapToGridOnResize, true);

private:
GUID _defaultProfile;
winrt::com_ptr<winrt::TerminalApp::implementation::AppKeyBindings> _keybindings;
std::vector<::TerminalApp::SettingsLoadWarnings> _keybindingsWarnings;

std::unordered_map<std::wstring, ColorScheme> _colorSchemes;

Expand Down
14 changes: 13 additions & 1 deletion src/cascadia/TerminalApp/Resources/Resources.language-en.resw
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,18 @@ Temporarily using the Windows Terminal default settings.
<data name="InvalidIcon" xml:space="preserve">
<value>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.</value>
</data>
<data name="AtLeastOneKeybindingWarning" xml:space="preserve">
<value>Warnings were found while parsing your keybindings:
</value>
</data>
<data name="TooManyKeysForChord" xml:space="preserve">
<value> Found a keybinding with too many strings for the "keys" array. There should only be one string value in the "keys" array.
</value>
</data>
<data name="MissingRequiredParameter" xml:space="preserve">
<value> Found a keybinding that was missing a required parameter value. This keybinding will be ignored.
</value>
</data>
<data name="CmdCommandArgDesc" xml:space="preserve">
<value>An optional command, with arguments, to be spawned in the new tab or pane</value>
</data>
Expand Down Expand Up @@ -256,4 +268,4 @@ Temporarily using the Windows Terminal default settings.
<data name="CmdStartingDirArgDesc" xml:space="preserve">
<value>Open in the given directory instead of the profile's set startingDirectory</value>
</data>
</root>
</root>
9 changes: 7 additions & 2 deletions src/cascadia/TerminalApp/TerminalWarnings.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,15 +25,20 @@ namespace TerminalApp
DuplicateProfile = 1,
UnknownColorScheme = 2,
InvalidBackgroundImage = 3,
InvalidIcon = 4
InvalidIcon = 4,
AtLeastOneKeybindingWarning = 5,
TooManyKeysForChord = 6,
MissingRequiredParameter = 7,
WARNINGS_SIZE // IMPORTANT: This MUST be the last value in this enum. It's an unused placeholder.
};

// SettingsLoadWarnings are scenarios where the settings had invalid state
// that we could not recover from.
enum class SettingsLoadErrors : uint32_t
{
NoProfiles = 0,
AllProfilesHidden = 1
AllProfilesHidden = 1,
ERRORS_SIZE // IMPORTANT: This MUST be the last value in this enum. It's an unused placeholder.
};

// This is a helper class to wrap up a SettingsLoadErrors into a proper
Expand Down

0 comments on commit 45dc345

Please sign in to comment.