Skip to content

Commit

Permalink
Ensure equality when hashing default args and no args in actions (#10341
Browse files Browse the repository at this point in the history
)

## 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

(cherry picked from commit b3b6484)
  • Loading branch information
carlos-zamora authored and DHowett committed Jul 7, 2021
1 parent febbf08 commit ee05855
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 2 deletions.
10 changes: 10 additions & 0 deletions src/cascadia/LocalTests_SettingsModel/KeyBindingsTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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);
}
}
}
9 changes: 9 additions & 0 deletions src/cascadia/TerminalSettingsModel/ActionArgs.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
22 changes: 20 additions & 2 deletions src/cascadia/TerminalSettingsModel/ActionMap.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the MIT license.

#include "pch.h"
#include "AllShortcutActions.h"
#include "ActionMap.h"

#include "ActionMap.g.cpp"
Expand All @@ -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;
}
Expand Down

0 comments on commit ee05855

Please sign in to comment.