maxmustermann/terminal
1
0
Fork 0
Code Issues Pull requests Projects Releases Wiki Activity

Make ActionMap compatible with ScanCode-only KeyChords (#10945)

Browse source 
This commit partially reverts d465a47 and introduces an alternative approach by adding Hash and Equals methods to the KeyChords class. Those methods will now favor any existing Vkeys over ScanCodes.

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

## Validation Steps Performed

* Added a new test, which is ✔️
* Various standard commands still work ✔️
* Hash() returns the same value for all KeyChords that are Equals() ✔️
This commit is contained in:
Leonard Hecker 2021-08-20 02:21:33 +02:00 committed by GitHub
parent 1678b58dde
commit 70d44c84c8
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
7 changed files with 117 additions and 103 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

12
src/cascadia/LocalTests_SettingsModel/KeyBindingsTests.cpp
View file

@ -58,6 +58,7 @@ namespace SettingsModelLocalTests
TEST_METHOD(TestMoveTabArgs);
TEST_METHOD(TestGetKeyBindingForAction);
TEST_METHOD(KeybindingsWithoutVkey);
TEST_CLASS_SETUP(ClassSetup)
{
@ -788,4 +789,15 @@ namespace SettingsModelLocalTests
actionMap->LayerJson(bindings2Json);
VERIFY_ARE_EQUAL(2u, actionMap->_KeyMap.size());
}
void KeyBindingsTests::KeybindingsWithoutVkey()
{
const auto json = VerifyParseSucceeded(R"!([{"command": "quakeMode", "keys":"shift+sc(255)"}])!");
const auto actionMap = winrt::make_self<implementation::ActionMap>();
actionMap->LayerJson(json);
const auto action = actionMap->GetActionByKeyChord({ VirtualKeyModifiers::Shift, 0, 255 });
VERIFY_IS_NOT_NULL(action);
}
}

56
src/cascadia/TerminalControl/KeyChord.cpp
View file

@ -38,19 +38,67 @@ namespace winrt::Microsoft::Terminal::Control::implementation
{
_vkey = MapVirtualKeyW(scanCode, MAPVK_VSC_TO_VK_EX);
}
assert(_vkey || _scanCode);
}
VirtualKeyModifiers KeyChord::Modifiers() noexcept
uint64_t KeyChord::Hash() const noexcept
{
// Two KeyChords are equal if they have the same modifiers and either identical
// Vkey or ScanCode, with Vkey being preferred. See KeyChord::Equals().
// This forces us to _either_ hash _vkey or _scanCode.
//
// Additionally the hash value with _vkey==123 and _scanCode==123 must be different.
// --> Taint hashes of KeyChord without _vkey.
auto h = static_cast<uint64_t>(_modifiers) << 32;
h |= _vkey ? _vkey : (_scanCode | 0xBABE0000);
// I didn't like how std::hash uses the byte-wise FNV1a for integers.
// So I built my own std::hash with murmurhash3.
h ^= h >> 33;
h *= UINT64_C(0xff51afd7ed558ccd);
h ^= h >> 33;
h *= UINT64_C(0xc4ceb9fe1a85ec53);
h ^= h >> 33;
return h;
}
bool KeyChord::Equals(const Control::KeyChord& other) const noexcept
{
// Two KeyChords are equal if they have the same modifiers and either identical
// Vkey or ScanCode, with Vkey being preferred. Vkey is preferred because:
// ActionMap needs to identify KeyChords which should "layer" (overwrite) each other.
// For instance win+sc(41) and win+` both specify the same KeyChord on an US keyboard layout
// from the perspective of a user. Either of the two should correctly overwrite the other.
//
// Two problems exist here:
// * Since a value of 0 indicates that the Vkey/ScanCode isn't set, we cannot use == to compare them.
// Otherwise we return true, even if the Vkey/ScanCode isn't set on both sides.
// * Whenever Equals() returns true, the Hash() value _must_ be identical.
// For instance the code below ensures the preference of Vkey over ScanCode by:
// this->_vkey || other->_vkey ? ...vkey... : ...scanCode...
// We cannot use "&&", even if it would be technically more correct, as this would mean the
// return value of this function would be dependent on the existence of a Vkey in "other".
// But Hash() has no "other" argument to consider when deciding if its Vkey or ScanCode should be hashed.
//
// Bitwise operators are used because MSVC doesn't support compiling
// boolean operators into bitwise ones at the time of writing.
const auto otherSelf = winrt::get_self<KeyChord>(other);
return _modifiers == otherSelf->_modifiers && ((_vkey | otherSelf->_vkey) ? _vkey == otherSelf->_vkey : _scanCode == otherSelf->_scanCode);
}
VirtualKeyModifiers KeyChord::Modifiers() const noexcept
{
return _modifiers;
}
void KeyChord::Modifiers(VirtualKeyModifiers const& value) noexcept
void KeyChord::Modifiers(const VirtualKeyModifiers value) noexcept
{
_modifiers = value;
}
int32_t KeyChord::Vkey() noexcept
int32_t KeyChord::Vkey() const noexcept
{
return _vkey;
}
@ -60,7 +108,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
_vkey = value;
}
int32_t KeyChord::ScanCode() noexcept
int32_t KeyChord::ScanCode() const noexcept
{
return _scanCode;
}

11
src/cascadia/TerminalControl/KeyChord.h
View file

@ -13,11 +13,14 @@ namespace winrt::Microsoft::Terminal::Control::implementation
KeyChord(const winrt::Windows::System::VirtualKeyModifiers modifiers, int32_t vkey, int32_t scanCode) noexcept;
KeyChord(bool ctrl, bool alt, bool shift, bool win, int32_t vkey, int32_t scanCode) noexcept;
winrt::Windows::System::VirtualKeyModifiers Modifiers() noexcept;
void Modifiers(winrt::Windows::System::VirtualKeyModifiers const& value) noexcept;
int32_t Vkey() noexcept;
uint64_t Hash() const noexcept;
bool Equals(const Control::KeyChord& other) const noexcept;
winrt::Windows::System::VirtualKeyModifiers Modifiers() const noexcept;
void Modifiers(const winrt::Windows::System::VirtualKeyModifiers value) noexcept;
int32_t Vkey() const noexcept;
void Vkey(int32_t value) noexcept;
int32_t ScanCode() noexcept;
int32_t ScanCode() const noexcept;
void ScanCode(int32_t value) noexcept;
private:

3
src/cascadia/TerminalControl/KeyChord.idl
View file

@ -10,6 +10,9 @@ namespace Microsoft.Terminal.Control
KeyChord(Windows.System.VirtualKeyModifiers modifiers, Int32 vkey, Int32 scanCode);
KeyChord(Boolean ctrl, Boolean alt, Boolean shift, Boolean win, Int32 vkey, Int32 scanCode);
UInt64 Hash();
Boolean Equals(KeyChord other);
Windows.System.VirtualKeyModifiers Modifiers;
Int32 Vkey;
Int32 ScanCode;

56
src/cascadia/TerminalSettingsModel/ActionMap.cpp
View file

@ -124,7 +124,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
ALL_SHORTCUT_ACTIONS
#undef ON_ALL_ACTIONS
_AvailableActionsCache = single_threaded_map<hstring, Model::ActionAndArgs>(std::move(availableActions));
_AvailableActionsCache = single_threaded_map(std::move(availableActions));
}
return _AvailableActionsCache.GetView();
}
@ -173,7 +173,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
_PopulateNameMapWithSpecialCommands(nameMap);
_PopulateNameMapWithStandardCommands(nameMap);
_NameMapCache = single_threaded_map<hstring, Model::Command>(std::move(nameMap));
_NameMapCache = single_threaded_map(std::move(nameMap));
}
return _NameMapCache.GetView();
}
@ -281,17 +281,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
{
if (!_GlobalHotkeysCache)
{
std::unordered_map<Control::KeyChord, Model::Command, KeyChordHash, KeyChordEquality> globalHotkeys;
for (const auto& [keys, cmd] : KeyBindings())
{
// Only populate GlobalHotkeys with actions whose
// ShortcutAction is GlobalSummon or QuakeMode
if (cmd.ActionAndArgs().Action() == ShortcutAction::GlobalSummon || cmd.ActionAndArgs().Action() == ShortcutAction::QuakeMode)
{
globalHotkeys.emplace(keys, cmd);
}
}
_GlobalHotkeysCache = single_threaded_map<Control::KeyChord, Model::Command>(std::move(globalHotkeys));
_RefreshKeyBindingCaches();
}
return _GlobalHotkeysCache.GetView();
}
@ -300,16 +290,33 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
{
if (!_KeyBindingMapCache)
{
// populate _KeyBindingMapCache
std::unordered_map<KeyChord, Model::Command, KeyChordHash, KeyChordEquality> keyBindingsMap;
std::unordered_set<ActionMapKeyChord> unboundKeys;
_PopulateKeyBindingMapWithStandardCommands(keyBindingsMap, unboundKeys);
_KeyBindingMapCache = single_threaded_map<KeyChord, Model::Command>(std::move(keyBindingsMap));
_RefreshKeyBindingCaches();
}
return _KeyBindingMapCache.GetView();
}
void ActionMap::_RefreshKeyBindingCaches()
{
std::unordered_map<KeyChord, Model::Command, KeyChordHash, KeyChordEquality> keyBindingsMap;
std::unordered_map<KeyChord, Model::Command, KeyChordHash, KeyChordEquality> globalHotkeys;
std::unordered_set<Control::KeyChord, KeyChordHash, KeyChordEquality> unboundKeys;
_PopulateKeyBindingMapWithStandardCommands(keyBindingsMap, unboundKeys);
for (const auto& [keys, cmd] : keyBindingsMap)
{
// Only populate GlobalHotkeys with actions whose
// ShortcutAction is GlobalSummon or QuakeMode
if (cmd.ActionAndArgs().Action() == ShortcutAction::GlobalSummon || cmd.ActionAndArgs().Action() == ShortcutAction::QuakeMode)
{
globalHotkeys.emplace(keys, cmd);
}
}
_KeyBindingMapCache = single_threaded_map(std::move(keyBindingsMap));
_GlobalHotkeysCache = single_threaded_map(std::move(globalHotkeys));
}
// Method Description:
// - Populates the provided keyBindingsMap with all of our actions and our parents actions
// while omitting the key bindings that were already added before.
@ -317,7 +324,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
// Arguments:
// - keyBindingsMap: the keyBindingsMap we're populating. This maps the key chord of a command to the command itself.
// - unboundKeys: a set of keys that are explicitly unbound
void ActionMap::_PopulateKeyBindingMapWithStandardCommands(std::unordered_map<KeyChord, Model::Command, KeyChordHash, KeyChordEquality>& keyBindingsMap, std::unordered_set<ActionMapKeyChord>& unboundKeys) const
void ActionMap::_PopulateKeyBindingMapWithStandardCommands(std::unordered_map<Control::KeyChord, Model::Command, KeyChordHash, KeyChordEquality>& keyBindingsMap, std::unordered_set<Control::KeyChord, KeyChordHash, KeyChordEquality>& unboundKeys) const
{
// Update KeyBindingsMap with our current layer
for (const auto& [keys, actionID] : _KeyMap)
@ -710,15 +717,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
// - the command with the given key chord
// - nullptr if the key chord is explicitly unbound
// - nullopt if it was not bound in this layer
std::optional<Model::Command> ActionMap::_GetActionByKeyChordInternal(const ActionMapKeyChord keys) const
std::optional<Model::Command> ActionMap::_GetActionByKeyChordInternal(const Control::KeyChord& keys) const
{
// KeyChord's constructor ensures that Modifiers() & Vkey() is a valid value at a minimum.
// This allows ActionMap to identify KeyChords which should "layer" (overwrite) each other.
// For instance win+sc(41) and win+` both specify the same KeyChord on an US keyboard layout
// from the perspective of a user. Either of the two should correctly overwrite the other.
// As such we need to pretend as if ScanCode doesn't exist.
assert(keys.vkey != 0);
// Check the current layer
if (const auto actionIDPair = _KeyMap.find(keys); actionIDPair != _KeyMap.end())
{

69
src/cascadia/TerminalSettingsModel/ActionMap.h
View file

@ -28,77 +28,23 @@ namespace SettingsModelLocalTests
class TerminalSettingsTests;
}
namespace winrt::Microsoft::Terminal::Settings::Model::implementation
{
union ActionMapKeyChord
{
uint16_t value = 0;
struct
{
uint8_t modifiers;
uint8_t vkey;
};
constexpr ActionMapKeyChord() = default;
ActionMapKeyChord(const Control::KeyChord& keys) :
modifiers(gsl::narrow_cast<uint8_t>(keys.Modifiers())),
vkey(gsl::narrow_cast<uint8_t>(keys.Vkey()))
{
}
constexpr bool operator==(ActionMapKeyChord other) const noexcept
{
return value == other.value;
}
};
}
template<>
struct std::hash<winrt::Microsoft::Terminal::Settings::Model::implementation::ActionMapKeyChord>
{
constexpr size_t operator()(winrt::Microsoft::Terminal::Settings::Model::implementation::ActionMapKeyChord keys) const noexcept
{
// I didn't like how std::hash uses the byte-wise FNV1a for integers.
// So I built my own std::hash with murmurhash3.
#if SIZE_MAX == UINT32_MAX
size_t h = keys.value;
h ^= h >> 16;
h *= UINT32_C(0x85ebca6b);
h ^= h >> 13;
h *= UINT32_C(0xc2b2ae35);
h ^= h >> 16;
return h;
#elif SIZE_MAX == UINT64_MAX
size_t h = keys.value;
h ^= h >> 33;
h *= UINT64_C(0xff51afd7ed558ccd);
h ^= h >> 33;
h *= UINT64_C(0xc4ceb9fe1a85ec53);
h ^= h >> 33;
return h;
#else
return std::hash<uint16_t>{}(keys.value);
#endif
}
};
namespace winrt::Microsoft::Terminal::Settings::Model::implementation
{
using InternalActionID = size_t;
struct KeyChordHash
{
std::size_t operator()(const Control::KeyChord& key) const
inline std::size_t operator()(const Control::KeyChord& key) const
{
return ::Microsoft::Terminal::Settings::Model::HashUtils::HashProperty(key.Modifiers(), key.Vkey());
return static_cast<size_t>(key.Hash());
}
};
struct KeyChordEquality
{
bool operator()(const Control::KeyChord& lhs, const Control::KeyChord& rhs) const
inline bool operator()(const Control::KeyChord& lhs, const Control::KeyChord& rhs) const
{
return lhs.Modifiers() == rhs.Modifiers() && lhs.Vkey() == rhs.Vkey();
return lhs.Equals(rhs);
}
};
@ -132,12 +78,13 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
private:
std::optional<Model::Command> _GetActionByID(const InternalActionID actionID) const;
std::optional<Model::Command> _GetActionByKeyChordInternal(const ActionMapKeyChord keys) const;
std::optional<Model::Command> _GetActionByKeyChordInternal(const Control::KeyChord& keys) const;
void _RefreshKeyBindingCaches();
void _PopulateAvailableActionsWithStandardCommands(std::unordered_map<hstring, Model::ActionAndArgs>& availableActions, std::unordered_set<InternalActionID>& visitedActionIDs) const;
void _PopulateNameMapWithSpecialCommands(std::unordered_map<hstring, Model::Command>& nameMap) const;
void _PopulateNameMapWithStandardCommands(std::unordered_map<hstring, Model::Command>& nameMap) const;
void _PopulateKeyBindingMapWithStandardCommands(std::unordered_map<Control::KeyChord, Model::Command, KeyChordHash, KeyChordEquality>& keyBindingsMap, std::unordered_set<ActionMapKeyChord>& unboundKeys) const;
void _PopulateKeyBindingMapWithStandardCommands(std::unordered_map<Control::KeyChord, Model::Command, KeyChordHash, KeyChordEquality>& keyBindingsMap, std::unordered_set<Control::KeyChord, KeyChordHash, KeyChordEquality>& unboundKeys) const;
std::vector<Model::Command> _GetCumulativeActions() const noexcept;
void _TryUpdateActionMap(const Model::Command& cmd, Model::Command& oldCmd, Model::Command& consolidatedCmd);
@ -151,7 +98,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
std::unordered_map<winrt::hstring, Model::Command> _NestedCommands;
std::vector<Model::Command> _IterableCommands;
std::unordered_map<ActionMapKeyChord, InternalActionID> _KeyMap;
std::unordered_map<Control::KeyChord, InternalActionID, KeyChordHash, KeyChordEquality> _KeyMap;
std::unordered_map<InternalActionID, Model::Command> _ActionMap;
// Masking Actions:

13
src/cascadia/ut_app/JsonTests.cpp
View file

@ -36,26 +36,27 @@ namespace TerminalAppUnitTests
return true;
}
Json::Value VerifyParseSucceeded(std::string content);
void VerifyParseFailed(std::string content);
Json::Value VerifyParseSucceeded(std::string_view content);
void VerifyParseFailed(std::string_view content);
private:
Json::StreamWriterBuilder _builder;
};
Json::Value JsonTests::VerifyParseSucceeded(std::string content)
Json::Value JsonTests::VerifyParseSucceeded(std::string_view content)
{
Json::Value root;
std::string errs;
const bool parseResult = _reader->parse(content.c_str(), content.c_str() + content.size(), &root, &errs);
const bool parseResult = _reader->parse(content.data(), content.data() + content.size(), &root, &errs);
VERIFY_IS_TRUE(parseResult, winrt::to_hstring(errs).c_str());
return root;
}
void JsonTests::VerifyParseFailed(std::string content)
void JsonTests::VerifyParseFailed(std::string_view content)
{
Json::Value root;
std::string errs;
const bool parseResult = _reader->parse(content.c_str(), content.c_str() + content.size(), &root, &errs);
const bool parseResult = _reader->parse(content.data(), content.data() + content.size(), &root, &errs);
VERIFY_IS_FALSE(parseResult);
}