Skip to content

Commit

Permalink
Fix hashing of NewTerminalArgs (#11905)
Browse files Browse the repository at this point in the history 
00fe2b0 made the mistake of hashing the pointer of `NewTerminalArgs` instances
instead of their content. This commit calls `NewTerminalArgs::Hash` instead.

## PR Checklist
* [x] I work here
* [x] Tests added/passed

## Validation Steps Performed
* New test passes ✅
  • Loading branch information
@lhecker
lhecker authored Dec 9, 2021
1 parent 477d0b4 commit 2c7b18f
Show file tree
Hide file tree
Showing 6 changed files with 108 additions and 91 deletions.
17 changes: 9 additions & 8 deletions src/cascadia/LocalTests_SettingsModel/KeyBindingsTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,24 +39,17 @@ namespace SettingsModelLocalTests
TEST_METHOD(KeyChords);
TEST_METHOD(ManyKeysSameAction);
TEST_METHOD(LayerKeybindings);
TEST_METHOD(HashDeduplication);
TEST_METHOD(UnbindKeybindings);

TEST_METHOD(LayerScancodeKeybindings);

TEST_METHOD(TestExplicitUnbind);

TEST_METHOD(TestArbitraryArgs);
TEST_METHOD(TestSplitPaneArgs);

TEST_METHOD(TestStringOverload);

TEST_METHOD(TestSetTabColorArgs);

TEST_METHOD(TestScrollArgs);

TEST_METHOD(TestToggleCommandPaletteArgs);
TEST_METHOD(TestMoveTabArgs);

TEST_METHOD(TestGetKeyBindingForAction);
TEST_METHOD(KeybindingsWithoutVkey);
};
Expand Down Expand Up @@ -173,6 +166,14 @@ namespace SettingsModelLocalTests
VERIFY_ARE_EQUAL(2u, actionMap->_KeyMap.size());
}

void KeyBindingsTests::HashDeduplication()
{
const auto actionMap = winrt::make_self<implementation::ActionMap>();
actionMap->LayerJson(VerifyParseSucceeded(R"([ { "command": "splitPane", "keys": ["ctrl+c"] } ])"));
actionMap->LayerJson(VerifyParseSucceeded(R"([ { "command": "splitPane", "keys": ["ctrl+c"] } ])"));
VERIFY_ARE_EQUAL(1u, actionMap->_ActionMap.size());
}

void KeyBindingsTests::UnbindKeybindings()
{
const std::string bindings0String{ R"([ { "command": "copy", "keys": ["ctrl+c"] } ])" };
Expand Down
44 changes: 33 additions & 11 deletions src/cascadia/TerminalSettingsModel/ActionArgs.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -303,9 +303,14 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
copy->_ColorScheme = _ColorScheme;
return *copy;
}
size_t Hash(uint64_t hasherState) const
size_t Hash() const
{
til::hasher h;
Hash(h);
return h.finalize();
}
void Hash(til::hasher& h) const
{
til::hasher h{ gsl::narrow_cast<size_t>(hasherState) };
h.write(Commandline());
h.write(StartingDirectory());
h.write(TabTitle());
Expand All @@ -314,10 +319,27 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
h.write(Profile());
h.write(SuppressApplicationTitle());
h.write(ColorScheme());
return h.finalize();
}
};
}

template<>
struct til::hash_trait<winrt::Microsoft::Terminal::Settings::Model::NewTerminalArgs>
{
using M = winrt::Microsoft::Terminal::Settings::Model::NewTerminalArgs;
using I = winrt::Microsoft::Terminal::Settings::Model::implementation::NewTerminalArgs;

void operator()(hasher& h, const M& value) const noexcept
{
if (value)
{
winrt::get_self<I>(value)->Hash(h);
}
}
};

namespace winrt::Microsoft::Terminal::Settings::Model::implementation
{
// New Tabs, Panes, and Windows all use NewTerminalArgs, which is more
// complicated and doesn't play nice with the macro. So those we'll still
// have to define manually.
Expand Down Expand Up @@ -363,9 +385,9 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
copy->_TerminalArgs = _TerminalArgs.Copy();
return *copy;
}
size_t Hash(uint64_t hasherState) const
size_t Hash() const
{
til::hasher h{ gsl::narrow_cast<size_t>(hasherState) };
til::hasher h;
h.write(TerminalArgs());
return h.finalize();
}
Expand Down Expand Up @@ -449,9 +471,9 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
copy->_SplitSize = _SplitSize;
return *copy;
}
size_t Hash(uint64_t hasherState) const
size_t Hash() const
{
til::hasher h{ gsl::narrow_cast<size_t>(hasherState) };
til::hasher h;
h.write(SplitDirection());
h.write(TerminalArgs());
h.write(SplitMode());
Expand Down Expand Up @@ -501,9 +523,9 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
copy->_TerminalArgs = _TerminalArgs.Copy();
return *copy;
}
size_t Hash(uint64_t hasherState) const
size_t Hash() const
{
til::hasher h{ gsl::narrow_cast<size_t>(hasherState) };
til::hasher h;
h.write(TerminalArgs());
return h.finalize();
}
Expand Down Expand Up @@ -622,9 +644,9 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
copy->_Actions = _Actions;
return *copy;
}
size_t Hash(uint64_t hasherState) const
size_t Hash() const
{
til::hasher h{ gsl::narrow_cast<size_t>(hasherState) };
til::hasher h;
h.write(winrt::get_abi(_Actions));
return h.finalize();
}
Expand Down
4 changes: 2 additions & 2 deletions src/cascadia/TerminalSettingsModel/ActionArgs.idl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ namespace Microsoft.Terminal.Settings.Model
Boolean Equals(IActionArgs other);
String GenerateName();
IActionArgs Copy();
UInt64 Hash(UInt64 hasher);
UInt64 Hash();
};

interface IActionEventArgs
Expand Down Expand Up @@ -128,7 +128,7 @@ namespace Microsoft.Terminal.Settings.Model
Boolean Equals(NewTerminalArgs other);
String GenerateName();
String ToCommandline();
UInt64 Hash(UInt64 hasher);
UInt64 Hash();
};

[default_interface] runtimeclass ActionEventArgs : IActionEventArgs
Expand Down
98 changes: 49 additions & 49 deletions src/cascadia/TerminalSettingsModel/ActionArgsMagic.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,53 +111,53 @@ struct InitListPlaceholder
// * NewTerminalArgs has a ToCommandline method it needs to additionally declare.
// * GlobalSummonArgs has the QuakeModeFromJson helper

#define ACTION_ARG_BODY(className, argsMacro) \
className() = default; \
className( \
argsMacro(CTOR_PARAMS) InitListPlaceholder = {}) : \
argsMacro(CTOR_INIT) _placeholder{} {}; \
argsMacro(DECLARE_ARGS); \
\
private: \
InitListPlaceholder _placeholder; \
\
public: \
hstring GenerateName() const; \
bool Equals(const IActionArgs& other) \
{ \
auto otherAsUs = other.try_as<className>(); \
if (otherAsUs) \
{ \
return true argsMacro(EQUALS_ARGS); \
} \
return false; \
}; \
static FromJsonResult FromJson(const Json::Value& json) \
{ \
auto args = winrt::make_self<className>(); \
argsMacro(FROM_JSON_ARGS); \
return { *args, {} }; \
} \
static Json::Value ToJson(const IActionArgs& val) \
{ \
if (!val) \
{ \
return {}; \
} \
Json::Value json{ Json::ValueType::objectValue }; \
const auto args{ get_self<className>(val) }; \
argsMacro(TO_JSON_ARGS); \
return json; \
} \
IActionArgs Copy() const \
{ \
auto copy{ winrt::make_self<className>() }; \
argsMacro(COPY_ARGS); \
return *copy; \
} \
size_t Hash(uint64_t hasherState) const \
{ \
til::hasher h{ gsl::narrow_cast<size_t>(hasherState) }; \
argsMacro(HASH_ARGS); \
return h.finalize(); \
#define ACTION_ARG_BODY(className, argsMacro) \
className() = default; \
className( \
argsMacro(CTOR_PARAMS) InitListPlaceholder = {}) : \
argsMacro(CTOR_INIT) _placeholder{} {}; \
argsMacro(DECLARE_ARGS); \
\
private: \
InitListPlaceholder _placeholder; \
\
public: \
hstring GenerateName() const; \
bool Equals(const IActionArgs& other) \
{ \
auto otherAsUs = other.try_as<className>(); \
if (otherAsUs) \
{ \
return true argsMacro(EQUALS_ARGS); \
} \
return false; \
}; \
static FromJsonResult FromJson(const Json::Value& json) \
{ \
auto args = winrt::make_self<className>(); \
argsMacro(FROM_JSON_ARGS); \
return { *args, {} }; \
} \
static Json::Value ToJson(const IActionArgs& val) \
{ \
if (!val) \
{ \
return {}; \
} \
Json::Value json{ Json::ValueType::objectValue }; \
const auto args{ get_self<className>(val) }; \
argsMacro(TO_JSON_ARGS); \
return json; \
} \
IActionArgs Copy() const \
{ \
auto copy{ winrt::make_self<className>() }; \
argsMacro(COPY_ARGS); \
return *copy; \
} \
size_t Hash() const \
{ \
til::hasher h; \
argsMacro(HASH_ARGS); \
return h.finalize(); \
}
27 changes: 15 additions & 12 deletions src/cascadia/TerminalSettingsModel/ActionMap.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,30 +15,33 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
{
static InternalActionID Hash(const Model::ActionAndArgs& actionAndArgs)
{
static constexpr auto initialHash = til::hasher{}.finalize();
const auto action = actionAndArgs.Action();
til::hasher hasher;

// action will be hashed last.
// This allows us to first seed a til::hasher
// with the return value of IActionArgs::Hash().
const auto action = actionAndArgs.Action();

if (const auto args = actionAndArgs.Args())
{
hasher = til::hasher{ gsl::narrow_cast<size_t>(args.Hash(initialHash)) };
hasher = til::hasher{ gsl::narrow_cast<size_t>(args.Hash()) };
}
else
{
size_t hash = 0;

// Args are not defined.
// Check if the ShortcutAction supports args.
switch (actionAndArgs.Action())
switch (action)
{
#define ON_ALL_ACTIONS_WITH_ARGS(action) \
case ShortcutAction::action: \
{ \
/* If it does, hash the default values for the args. */ \
static const auto cachedHash = gsl::narrow_cast<size_t>( \
winrt::make_self<implementation::action##Args>()->Hash(initialHash)); \
hash = cachedHash; \
break; \
#define ON_ALL_ACTIONS_WITH_ARGS(action) \
case ShortcutAction::action: \
{ \
/* If it does, hash the default values for the args. */ \
static const auto cachedHash = gsl::narrow_cast<size_t>( \
winrt::make_self<implementation::action##Args>()->Hash()); \
hash = cachedHash; \
break; \
}
ALL_SHORTCUT_ACTIONS_WITH_ARGS
#undef ON_ALL_ACTIONS_WITH_ARGS
Expand Down
9 changes: 0 additions & 9 deletions src/cascadia/TerminalSettingsModel/HashUtils.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,15 +33,6 @@ namespace til
}
};

template<>
struct hash_trait<winrt::Microsoft::Terminal::Settings::Model::NewTerminalArgs>
{
void operator()(hasher& h, const winrt::Microsoft::Terminal::Settings::Model::NewTerminalArgs& value) const noexcept
{
h.write(winrt::get_abi(value));
}
};

template<>
struct hash_trait<winrt::hstring>
{
Expand Down

0 comments on commit 2c7b18f

Please sign in to comment.