Ensure equality when hashing default args and no args in actions (#10341)

## Summary of the Pull Request
#10297 found a bug in `ActionMap` where the `ToggleCommandPalette` key chord could not be found using `GetKeyBindingForAction`.

This was caused by the following:
- `AddAction`: when adding the action, the `ActionAndArgs` is basically added as `{ToggleCommandPalette, ToggleCommandLineArgs{}}` (Note the default ctor used for the action args)
- `GetKeyBindingForAction`: we're searching for an `ActionAndArgs` structured as `{ToggleCommandPalette, nullptr}`
- Since these are _technically_ two different actions, we are unable to find it.

This issue was fixed by making the `Hash(ActionAndArgs)` function smarter! If the `ActionAndArgs` has no args, but the `ShortcutAction` _supports_ args, generate the args using the default ctor.

By making `Hash()` smarter, everybody benefits from this logic! We can basically now enforce that `ActionAndArgs{ <X>, nullptr } == ActionAndArgs{ <X>, <default_ctor> }`.

## Validation Steps Performed
- Added a test.
- Tested this on #10297's branch and this does fix the bug
This commit is contained in:
Carlos Zamora 2021-06-16 15:43:29 -07:00 committed by GitHub
parent 1ae6e3b772
commit b3b648496e
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
3 changed files with 39 additions and 2 deletions

View file

@ -647,10 +647,12 @@ namespace SettingsModelLocalTests
const std::string bindings0String{ R"([ { "command": "closeWindow", "keys": "ctrl+a" } ])" };
const std::string bindings1String{ R"([ { "command": { "action": "copy", "singleLine": true }, "keys": "ctrl+b" } ])" };
const std::string bindings2String{ R"([ { "command": { "action": "newTab", "index": 0 }, "keys": "ctrl+c" } ])" };
const std::string bindings3String{ R"([ { "command": "commandPalette", "keys": "ctrl+shift+p" } ])" };
const auto bindings0Json = VerifyParseSucceeded(bindings0String);
const auto bindings1Json = VerifyParseSucceeded(bindings1String);
const auto bindings2Json = VerifyParseSucceeded(bindings2String);
const auto bindings3Json = VerifyParseSucceeded(bindings3String);
auto VerifyKeyChordEquality = [](const KeyChord& expected, const KeyChord& actual) {
if (expected)
@ -699,5 +701,13 @@ namespace SettingsModelLocalTests
const auto& kbd{ actionMap->GetKeyBindingForAction(ShortcutAction::NewTab, *args) };
VerifyKeyChordEquality({ KeyModifiers::Ctrl, static_cast<int32_t>('C') }, kbd);
}
{
Log::Comment(L"command with hidden args");
actionMap->LayerJson(bindings3Json);
VERIFY_ARE_EQUAL(4u, actionMap->_KeyMap.size());
const auto& kbd{ actionMap->GetKeyBindingForAction(ShortcutAction::ToggleCommandPalette) };
VerifyKeyChordEquality({ KeyModifiers::Ctrl | KeyModifiers::Shift, static_cast<int32_t>('P') }, kbd);
}
}
}

View file

@ -68,6 +68,15 @@ constexpr size_t Microsoft::Terminal::Settings::Model::HashUtils::HashProperty(c
return gsl::narrow_cast<size_t>(args.Hash());
}
// Retrieves the hash value for an empty-constructed object.
template<typename T>
static size_t EmptyHash()
{
// cache the value of the empty hash
static const size_t cachedHash = winrt::make_self<T>()->Hash();
return cachedHash;
}
namespace winrt::Microsoft::Terminal::Settings::Model::implementation
{
using namespace ::Microsoft::Terminal::Settings::Model;

View file

@ -2,6 +2,7 @@
// Licensed under the MIT license.
#include "pch.h"
#include "AllShortcutActions.h"
#include "ActionMap.h"
#include "ActionMap.g.cpp"
@ -18,12 +19,29 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
size_t hashedArgs{};
if (const auto& args{ actionAndArgs.Args() })
{
// Args are defined, so hash them
hashedArgs = gsl::narrow_cast<size_t>(args.Hash());
}
else
{
std::hash<IActionArgs> argsHash;
hashedArgs = argsHash(nullptr);
// Args are not defined.
// Check if the ShortcutAction supports args.
switch (actionAndArgs.Action())
{
#define ON_ALL_ACTIONS_WITH_ARGS(action) \
case ShortcutAction::action: \
/* If it does, hash the default values for the args.*/ \
hashedArgs = EmptyHash<implementation::action##Args>(); \
break;
ALL_SHORTCUT_ACTIONS_WITH_ARGS
#undef ON_ALL_ACTIONS_WITH_ARGS
default:
{
// Otherwise, hash nullptr.
std::hash<IActionArgs> argsHash;
hashedArgs = argsHash(nullptr);
}
}
}
return hashedAction ^ hashedArgs;
}