Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix hashing of NewTerminalArgs #11905

Merged
1 commit merged into from
Dec 9, 2021
Merged

Fix hashing of NewTerminalArgs #11905

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 9 additions & 8 deletions src/cascadia/LocalTests_SettingsModel/KeyBindingsTests.cpp
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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