Skip to content

Commit

Permalink
Introduce ActionMap to Terminal Settings Model (#9621)
Browse files Browse the repository at this point in the history
This entirely removes `KeyMapping` from the settings model, and builds on the work done in #9543 to consolidate all actions (key bindings and commands) into a unified data structure (`ActionMap`).

## References
#9428 - Spec
#6900 - Actions page

Closes #7441

## Detailed Description of the Pull Request / Additional comments
The important thing here is to remember that we're shifting our philosophy of how to interact/represent actions. Prior to this, the actions arrays in the JSON would be deserialized twice: once for key bindings, and again for commands. By thinking of every entry in the relevant JSON as a `Command`, we can remove a lot of the context switching between working with a key binding vs a command palette item.

#9543 allows us to make that shift. Given the work in that PR, we can now deserialize all of the relevant information from each JSON action item. This allows us to simplify `ActionMap::FromJson` to simply iterate over each JSON action item, deserialize it, and add it to our `ActionMap`.

Internally, our `ActionMap` operates as discussed in #9428 by maintaining a `_KeyMap` that points to an action ID, and using that action ID to retrieve the `Command` from the `_ActionMap`. Adding actions to the `ActionMap` automatically accounts for name/key-chord collisions. A `NameMap` can be constructed when requested; this is for the Command Palette.

Querying the `ActionMap` is fairly straightforward. Helper functions were needed to be able to distinguish an explicit unbinding vs the command not being found in the current layer. Internally, we store explicitly unbound names/key-chords as `ShortcutAction::Invalid` commands. However, we return `nullptr` when a query points to an unbound command. This is done to hide this complexity away from any caller.

The command palette still needs special handling for nested and iterable commands. Thankfully, the expansion of iterable commands is performed on an `IMapView`, so we can just expose `NameMap` as a consolidation of `ActionMap`'s `NameMap` with its parents. The same can be said for exposing key chords in nested commands.

## Validation Steps Performed

All local tests pass.
  • Loading branch information
carlos-zamora committed May 5, 2021
1 parent 5713cd2 commit 22fd06e
Show file tree
Hide file tree
Showing 58 changed files with 2,021 additions and 1,112 deletions.
2 changes: 2 additions & 0 deletions .github/actions/spelling/dictionary/dictionary.txt
Original file line number Diff line number Diff line change
Expand Up @@ -345242,6 +345242,8 @@ resequester
resequestration
reserate
reserene
reserialize
reserializes
reserialization
reserpine
reserpinized
Expand Down
126 changes: 63 additions & 63 deletions src/cascadia/LocalTests_SettingsModel/CommandTests.cpp

Large diffs are not rendered by default.

292 changes: 214 additions & 78 deletions src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp

Large diffs are not rendered by default.

308 changes: 187 additions & 121 deletions src/cascadia/LocalTests_SettingsModel/KeyBindingsTests.cpp

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,13 @@
</ClCompile>
<Link>
<AdditionalDependencies>onecoreuap.lib;%(AdditionalDependencies)</AdditionalDependencies>
<!--
SettingsModelLib contains a DllMain that we need to force the use of.
If you don't have this, then you'll see an error like
"(init.obj) : error LNK2005: DllMain already defined in MSVCRTD.lib(dll_dllmain_stub.obj)"
-->
<AdditionalOptions Condition="'$(Platform)'=='Win32'">/INCLUDE:_DllMain@12</AdditionalOptions>
<AdditionalOptions Condition="'$(Platform)'!='Win32'">/INCLUDE:DllMain</AdditionalOptions>
</Link>
</ItemDefinitionGroup>

Expand Down
29 changes: 15 additions & 14 deletions src/cascadia/LocalTests_SettingsModel/TerminalSettingsTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -103,17 +103,18 @@ namespace SettingsModelLocalTests

CascadiaSettings settings{ til::u8u16(settingsJson) };

auto keymap = settings.GlobalSettings().KeyMap();
auto actionMap = settings.GlobalSettings().ActionMap();
VERIFY_ARE_EQUAL(3u, settings.ActiveProfiles().Size());

const auto profile2Guid = settings.ActiveProfiles().GetAt(2).Guid();
VERIFY_ARE_NOT_EQUAL(winrt::guid{}, profile2Guid);

VERIFY_ARE_EQUAL(12u, keymap.Size());
const auto& actionMapImpl{ winrt::get_self<implementation::ActionMap>(actionMap) };
VERIFY_ARE_EQUAL(12u, actionMapImpl->_KeyMap.size());

{
KeyChord kc{ true, false, false, static_cast<int32_t>('A') };
auto actionAndArgs = ::TestUtils::GetActionAndArgs(keymap, kc);
auto actionAndArgs = ::TestUtils::GetActionAndArgs(actionMap, kc);
VERIFY_ARE_EQUAL(ShortcutAction::SplitPane, actionAndArgs.Action());
const auto& realArgs = actionAndArgs.Args().try_as<SplitPaneArgs>();
VERIFY_IS_NOT_NULL(realArgs);
Expand All @@ -134,7 +135,7 @@ namespace SettingsModelLocalTests
}
{
KeyChord kc{ true, false, false, static_cast<int32_t>('B') };
auto actionAndArgs = ::TestUtils::GetActionAndArgs(keymap, kc);
auto actionAndArgs = ::TestUtils::GetActionAndArgs(actionMap, kc);
VERIFY_ARE_EQUAL(ShortcutAction::SplitPane, actionAndArgs.Action());
const auto& realArgs = actionAndArgs.Args().try_as<SplitPaneArgs>();
VERIFY_IS_NOT_NULL(realArgs);
Expand All @@ -156,7 +157,7 @@ namespace SettingsModelLocalTests
}
{
KeyChord kc{ true, false, false, static_cast<int32_t>('C') };
auto actionAndArgs = ::TestUtils::GetActionAndArgs(keymap, kc);
auto actionAndArgs = ::TestUtils::GetActionAndArgs(actionMap, kc);
VERIFY_ARE_EQUAL(ShortcutAction::SplitPane, actionAndArgs.Action());
const auto& realArgs = actionAndArgs.Args().try_as<SplitPaneArgs>();
VERIFY_IS_NOT_NULL(realArgs);
Expand All @@ -178,7 +179,7 @@ namespace SettingsModelLocalTests
}
{
KeyChord kc{ true, false, false, static_cast<int32_t>('D') };
auto actionAndArgs = ::TestUtils::GetActionAndArgs(keymap, kc);
auto actionAndArgs = ::TestUtils::GetActionAndArgs(actionMap, kc);
VERIFY_ARE_EQUAL(ShortcutAction::SplitPane, actionAndArgs.Action());
const auto& realArgs = actionAndArgs.Args().try_as<SplitPaneArgs>();
VERIFY_IS_NOT_NULL(realArgs);
Expand All @@ -200,7 +201,7 @@ namespace SettingsModelLocalTests
}
{
KeyChord kc{ true, false, false, static_cast<int32_t>('E') };
auto actionAndArgs = ::TestUtils::GetActionAndArgs(keymap, kc);
auto actionAndArgs = ::TestUtils::GetActionAndArgs(actionMap, kc);
VERIFY_ARE_EQUAL(ShortcutAction::SplitPane, actionAndArgs.Action());
const auto& realArgs = actionAndArgs.Args().try_as<SplitPaneArgs>();
VERIFY_IS_NOT_NULL(realArgs);
Expand All @@ -222,7 +223,7 @@ namespace SettingsModelLocalTests
}
{
KeyChord kc{ true, false, false, static_cast<int32_t>('F') };
auto actionAndArgs = ::TestUtils::GetActionAndArgs(keymap, kc);
auto actionAndArgs = ::TestUtils::GetActionAndArgs(actionMap, kc);
VERIFY_ARE_EQUAL(ShortcutAction::SplitPane, actionAndArgs.Action());
const auto& realArgs = actionAndArgs.Args().try_as<SplitPaneArgs>();
VERIFY_IS_NOT_NULL(realArgs);
Expand All @@ -245,7 +246,7 @@ namespace SettingsModelLocalTests
}
{
KeyChord kc{ true, false, false, static_cast<int32_t>('G') };
auto actionAndArgs = ::TestUtils::GetActionAndArgs(keymap, kc);
auto actionAndArgs = ::TestUtils::GetActionAndArgs(actionMap, kc);
VERIFY_ARE_EQUAL(ShortcutAction::NewTab, actionAndArgs.Action());
const auto& realArgs = actionAndArgs.Args().try_as<NewTabArgs>();
VERIFY_IS_NOT_NULL(realArgs);
Expand All @@ -265,7 +266,7 @@ namespace SettingsModelLocalTests
}
{
KeyChord kc{ true, false, false, static_cast<int32_t>('H') };
auto actionAndArgs = ::TestUtils::GetActionAndArgs(keymap, kc);
auto actionAndArgs = ::TestUtils::GetActionAndArgs(actionMap, kc);
VERIFY_ARE_EQUAL(ShortcutAction::NewTab, actionAndArgs.Action());
const auto& realArgs = actionAndArgs.Args().try_as<NewTabArgs>();
VERIFY_IS_NOT_NULL(realArgs);
Expand All @@ -287,7 +288,7 @@ namespace SettingsModelLocalTests
}
{
KeyChord kc{ true, false, false, static_cast<int32_t>('I') };
auto actionAndArgs = ::TestUtils::GetActionAndArgs(keymap, kc);
auto actionAndArgs = ::TestUtils::GetActionAndArgs(actionMap, kc);
VERIFY_ARE_EQUAL(ShortcutAction::NewTab, actionAndArgs.Action());
const auto& realArgs = actionAndArgs.Args().try_as<NewTabArgs>();
VERIFY_IS_NOT_NULL(realArgs);
Expand All @@ -310,7 +311,7 @@ namespace SettingsModelLocalTests
}
{
KeyChord kc{ true, false, false, static_cast<int32_t>('J') };
auto actionAndArgs = ::TestUtils::GetActionAndArgs(keymap, kc);
auto actionAndArgs = ::TestUtils::GetActionAndArgs(actionMap, kc);
VERIFY_ARE_EQUAL(ShortcutAction::NewTab, actionAndArgs.Action());
const auto& realArgs = actionAndArgs.Args().try_as<NewTabArgs>();
VERIFY_IS_NOT_NULL(realArgs);
Expand All @@ -332,7 +333,7 @@ namespace SettingsModelLocalTests
}
{
KeyChord kc{ true, false, false, static_cast<int32_t>('K') };
auto actionAndArgs = ::TestUtils::GetActionAndArgs(keymap, kc);
auto actionAndArgs = ::TestUtils::GetActionAndArgs(actionMap, kc);
VERIFY_ARE_EQUAL(ShortcutAction::NewTab, actionAndArgs.Action());
const auto& realArgs = actionAndArgs.Args().try_as<NewTabArgs>();
VERIFY_IS_NOT_NULL(realArgs);
Expand All @@ -355,7 +356,7 @@ namespace SettingsModelLocalTests
}
{
KeyChord kc{ true, false, false, static_cast<int32_t>('L') };
auto actionAndArgs = ::TestUtils::GetActionAndArgs(keymap, kc);
auto actionAndArgs = ::TestUtils::GetActionAndArgs(actionMap, kc);
VERIFY_ARE_EQUAL(ShortcutAction::NewTab, actionAndArgs.Action());
const auto& realArgs = actionAndArgs.Args().try_as<NewTabArgs>();
VERIFY_IS_NOT_NULL(realArgs);
Expand Down
10 changes: 5 additions & 5 deletions src/cascadia/LocalTests_SettingsModel/TestUtils.h
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,11 @@ class TestUtils
// - This is a helper to retrieve the ActionAndArgs from the keybindings
// for a given chord.
// Arguments:
// - keymap: The AppKeyBindings to lookup the ActionAndArgs from.
// - actionMap: The ActionMap to lookup the ActionAndArgs from.
// - kc: The key chord to look up the bound ActionAndArgs for.
// Return Value:
// - The ActionAndArgs bound to the given key, or nullptr if nothing is bound to it.
static const winrt::Microsoft::Terminal::Settings::Model::ActionAndArgs GetActionAndArgs(const winrt::Microsoft::Terminal::Settings::Model::KeyMapping& keymap,
static const winrt::Microsoft::Terminal::Settings::Model::ActionAndArgs GetActionAndArgs(const winrt::Microsoft::Terminal::Settings::Model::ActionMap& actionMap,
const winrt::Microsoft::Terminal::Control::KeyChord& kc)
{
std::wstring buffer{ L"" };
Expand All @@ -42,8 +42,8 @@ class TestUtils
buffer += static_cast<wchar_t>(MapVirtualKeyW(kc.Vkey(), MAPVK_VK_TO_CHAR));
WEX::Logging::Log::Comment(WEX::Common::NoThrowString().Format(L"Looking for key:%s", buffer.c_str()));

const auto action = keymap.TryLookup(kc);
VERIFY_IS_NOT_NULL(action, L"Expected to find an action bound to the given KeyChord");
return action;
const auto cmd = actionMap.GetActionByKeyChord(kc);
VERIFY_IS_NOT_NULL(cmd, L"Expected to find an action bound to the given KeyChord");
return cmd.ActionAndArgs();
};
};
Loading

0 comments on commit 22fd06e

Please sign in to comment.