Introduce ActionMap to Terminal Settings Model #9621

carlos-zamora · 2021-03-25T22:04:46Z

Summary of the Pull Request

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.

carlos-zamora

~~As described in the PR Description, I can't run the local tests for the settings model, for some reason. Here's the error I get:~~

3>Microsoft.Terminal.Settings.Model.Lib.lib(init.obj) : error LNK2005: DllMain already defined in MSVCRTD.lib(dll_dllmain_stub.obj)

~~Anybody got any ideas?~~

EDIT: sometimes I can get it to work and sometimes I can't. I managed to verify that most of the local tests passed. Made one more change afterwards that removed an unnecessary warning during deserialization. I suspect that was the cause of a lot of test failures so I think this is good to go (just need to verify that once the tests work again.)

EDIT: tests are fixed

src/cascadia/TerminalSettingsModel/KeyChordSerialization.cpp

src/cascadia/TerminalSettingsModel/Command.cpp

carlos-zamora · 2021-03-25T22:13:40Z

src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp

@@ -248,57 +248,6 @@ winrt::Microsoft::Terminal::Settings::Model::CascadiaSettings CascadiaSettings::
        // If this throws, the app will catch it and use the default settings
        resultPtr->_ValidateSettings();

-        // GH 3855 - Gathering Data on custom profiles to inform better defaults


Covered this with Dustin. This was a quick-n-easy way to gather data on the key bindings. Since we removed KeyMapping, we don't actually have a way to serialize our key bindings (yet).

Makes more sense to just outright remove this, then we leverage the settings model to gather data better later (if desired).

src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp

zadjii-msft

Alright, so I'm at 42/52. Probably 30 of those were fully just renaming variables, but I am making progress on this. I'll keep at it.

Maybe for the Action().Action() I've got three equally bad ideas:

command.Action().Type()
command.ActionAndArgs().Action()
command.Action().ShortcutAction()

src/cascadia/TerminalSettingsEditor/KeyChordConverter.cpp

src/cascadia/TerminalSettingsEditor/Actions.xaml

src/cascadia/TerminalSettingsModel/ActionArgs.h

src/cascadia/TerminalApp/ActionPaletteItem.cpp

src/cascadia/TerminalSettingsModel/ActionAndArgs.cpp

src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp

src/cascadia/TerminalSettingsModel/KeyChordSerialization.cpp

src/cascadia/TerminalSettingsModel/ActionMap.idl

zadjii-msft

Okay a bunch of stuff still needs to be done here, but I've read it all! Almost easier to just read it all like it's entirely new code, rather than with the diff. Bunch of comments below.

src/cascadia/TerminalSettingsModel/Command.cpp

zadjii-msft · 2021-04-13T17:14:07Z

src/cascadia/TerminalSettingsModel/Command.cpp

+
+    hstring Command::Name() const noexcept
+    {
+        if (_name.has_value())


okay so in the past hasn't there been some weirdness with optional<string>s? Don't we usually use string.empty() to indicate that a string property doesn't have a value set, rather than an optional<string>? Is there a reason we're doing the optional here?

Yes! So this goes back to the problem of IReference<hstring> cannot be exposed in the IDL.

The current architecture is...

construct a Command for each of the commands in the JSON

register that Command in our ActionMap
a. If it exists, update the existing command
b. If it's new, just add it in like normal

The problem is, not all commands in the JSON have a "name". And if the "name" is missing, we want to interpret that as "just use the name we were already using". Like so...

// Scenario 1 // "foo" invokes the "copy" command across 3 key bindings ctrl+a/b/c { "name": "foo", "command": "copy", "keys": "ctrl+a" }, { "command": "copy", "keys": "ctrl+b" }, { "command": "copy", "keys": "ctrl+c" }, // Scenario 2 // the "paste" command has 3 key bindings ctrl+x/y/z // the "paste" command is _NOT_ in the command palette { "name": "bar", "command": "paste", "keys": "ctrl+x" }, { "command": "paste", "keys": "ctrl+y" }, { "name": null, "command": "paste", "keys": "ctrl+z" },

Storing _name as an optional<string> allows us to distinguish the two scenarios above.

TL;DR: we need to distinguish between an "inherited" name and no name again

The same thing can be said for "icon".

src/cascadia/TerminalSettingsModel/Command.cpp

src/cascadia/TerminalSettingsModel/Command.h

src/cascadia/TerminalSettingsModel/ActionMap.h

src/cascadia/TerminalSettingsModel/ActionMap.cpp

zadjii-msft · 2021-04-13T19:50:46Z

src/cascadia/TerminalSettingsModel/ActionMap.cpp

+        // Add NestedCommands to NameMap _after_ we handle our parents.
+        // This allows us to override whatever our parents tell us.
+        for (const auto& [name, cmd] : _NestedCommands)
+        {
+            nameMap.insert_or_assign(name, cmd);
+        }


This seems weird - why isn't this above the parents check with a similar if (visitedActionIDs.find() == end()) check?

Yeah, this one's trippy. Nested commands are special in that we have no way to check for equality. Thus, we can't add them to visitedActionIDs. Instead, we do the following:

In AddAction, remove/update _NestedCommands if there is a collision (i.e. copy action was explicitly set to have the same name as a nested command)

In _PopulateNameMap...
a. add nested commands last so that we override any collisions from our parents

Using this method, we make sure that we do not modify our parents. This lets you see the state of our ActionMap after loading a specific settings file as opposed to the whole thing.

src/cascadia/TerminalSettingsModel/ActionMap.cpp

zadjii-msft · 2021-04-13T20:15:38Z

src/cascadia/TerminalSettingsModel/ActionMap.cpp

+            {
+                // This command has a name, check if it's new.
+                const auto newName{ cmd.Name() };
+                if (newName != oldCmd.Name())


So to be sure - this is comparing newName (which definitely is a given name, because HasName will be false for a generated name) with the current name for this command - which might be a generated name (if oldCmd doesn't have a name explicitly set)?

If I do something like

{ "command": { "action": "newTab" } }, { "command": { "action": "newTab" }, "name": "foo" }, { "command": { "action": "newTab" }, "name": "bar" },

will I end up with one command in the palette, or 3?

One: "bar". ActionMap will store the command as if it was:

{ "command": { "action": "newTab" }, "name": "bar" },

This will be super useful when I start working on serialization because we won't need to write any of the older names that got overwritten.

carlos-zamora · 2021-04-15T18:00:44Z

So close! I just have ONE failing test: DeserializationTests::TestRebindNestedCommand.

Fixed! I'm a bit annoyed by the fix but it works and it's not bad, tbh. Basically, I split up _PopulateNameMap into two functions:

_PopulateNameMapWithNestedCommands:
- top-down approach. We're just adding the nested commands starting from the root and moving down until we get to our current layer.
_PopulateNameMapWithStandardCommands:
- bottom-up approach. Start from the current layer, update a set of visited action IDs, then visit our parents and add any actions that we're missing.
- This is so that we don't have to do a dance of figuring out the old name of a command when we have to update it (because we'd have to remove the old name, and add the new name).

An important assumption is that in the current layer, there is no overlap in names between the names of _NestedCommands and standard commands. We enforce this assumption via ActionMap::AddAction.

By taking a two-step approach to populating the name map, we are able to handle collisions across multiple layers.

carlos-zamora · 2021-04-16T01:10:25Z

New problem: new tab dropdown doesn't display key chords.

Fixed! Had to move HashProperty into its own file HashUtils.h and make them all inline (Leonard suggested constexpr as a way to make them inline explicitly). This gave me more space to create specializations for IReference, Color, and other types. Doing so allows me to make sure that we always get a consistent hash from two structs composed of the same data.

While fixing this, I ran into a problem with a deserialization test. This PR introduces new behavior for actions, but I think this is the correct behavior. It is as follows:

{ "name": "foo", "command": "copy" },
{ "name": "bar", "command": "copy" }

	Before PR	After PR
NameMap keys	`foo`, `bar`	`bar`

I believe this is the desired behavior because every command should only have one name.

Using this behavior, we can fix #7441. In fact, #7441 needs this. Consider the following:

{ "name": "foo", "command": "copy" },
{ "name": "bar", "command": "copy" },
{ "name": null, "command": "copy" },

	Before PR	After PR
NameMap keys	`foo`, `bar`	<none>

The value of a null name comes from being able to find the names that map to the command, and remove them from the name map. Thus, we can leverage internal action IDs (aka hashing) to accomplish this.

src/cascadia/TerminalSettingsEditor/Actions.h

src/cascadia/TerminalSettingsModel/ActionMap.cpp

src/cascadia/TerminalSettingsModel/Command.cpp

src/cascadia/TerminalSettingsModel/ActionMap.h

DHowett

OKAY i reviewed ActionMap.cpp

DHowett · 2021-05-04T21:53:55Z

src/cascadia/TerminalSettingsModel/ActionMap.h

+{
+    typedef size_t InternalActionID;
+
+    struct KeyChordHash


qq: why is this its own struct and not an std::hash specialization?

This was stolen from KeyMapping. We use it when we declare a key map:

std::unordered_map<Control::KeyChord, InternalActionID, KeyChordHash, KeyChordEquality> _KeyMap;

I think we ran into a problem before with CppWinRT already using std::hash specializations, so idk if that would work (at least right now).

DHowett · 2021-05-04T21:54:28Z

src/cascadia/TerminalSettingsModel/ActionMap.h

+        }
+    };
+
+    struct KeyChordEquality


qq: why is this its own struct and not an operator== in the global namespace (or anywhere we can use ADL -- so, inside this namespace is fine)?

Same as above, this was stolen from KeyMapping and is used in the same declarations for key map.

This one makes more sense to just have as a global operator though, so I'll do that.

Ugh, I tried just that, but then all the key bindings were broken. It seems like these unordered_maps need KeyChordEquality.

the unordered map should be using std::equals or std::is_equal which will use operator== from the type unless it's overridden in the template args.

Like, this should work: (writing code in godbolt)

okay, this works. it actually requires the operator to be in the leaf namespace. i could have sworn it should work in the parent namespace.

#include <unordered_map> namespace A::B::C { struct Thing { int a, b; }; // it apparently has to go here inline bool operator==(const C::Thing& lhs, const C::Thing& rhs) { return lhs.a==rhs.a && lhs.b==rhs.b; } } namespace A::B { struct ThingHasher { size_t operator()(const C::Thing& lhs) const { return 7; } }; // okay, i thought you could put it here and because A::B::C is inside A::B it would find it. } int fleh() { std::unordered_map<A::B::C::Thing, int, A::B::ThingHasher> augh; augh[A::B::C::Thing{1,2}] = 3; augh[A::B::C::Thing{3,4}] = 7; return augh.find(A::B::C::Thing{1,2}) == augh.end(); }

Ah, ADL only uses the leafmost namespace for a lot of things. So yes: if you introduce an operator== into the MS.Terminal.Control namespace in your header file it will get used automatically

broken = not recognized. Like, "ctrl+shift+space" wouldn't be detected.

llllet's just leave it. we can fix it in post.

src/cascadia/TerminalSettingsModel/ActionMap.h

src/cascadia/TerminalSettingsModel/ActionMap.cpp

DHowett

okay I think i understand the bulk of it

src/cascadia/TerminalSettingsModel/ActionMap.cpp

src/cascadia/TerminalSettingsModel/ActionMap.h

src/cascadia/TerminalSettingsModel/ActionMap.cpp

DHowett

I'm comfortable with this. Thanks so much for all the comments!

## Summary of the Pull Request This PR lays the foundation for a new Actions page in the Settings UI as designed in #6900. The Actions page now leverages the `ActionMap` to display all of the key bindings and allow the user to modify the associated key chord or delete the key binding entirely. ## References #9621 - ActionMap #9926 - ActionMap serialization #9428 - ActionMap Spec #6900 - Actions page #9427 - Actions page design doc ## Detailed Description of the Pull Request / Additional comments ### Settings Model Changes - `Command::Copy()` now copies the `ActionAndArgs` - `ActionMap::RebindKeys()` handles changing the key chord of a key binding. If a conflict occurs, the conflicting key chord is overwritten. - `ActionMap::DeleteKeyBinding()` "deletes" a key binding by binding "unbound" to the given key chord. - `ActionMap::KeyBindings()` presents another view (similar to `NameMap`) of the `ActionMap`. It specifically presents a map of key chords to commands. It is generated similar to how `NameMap` is generated. ### Editor Changes - `Actions.xaml` is mainly split into two parts: - `ListView` (as before) holds the list of key bindings. We _could_ explore the idea of an items repeater, but the `ListView` seems to provide some niceties with regards to navigating the list via the key board (though none are selectable). - `DataTemplate` is used to represent each key binding inside the `ListView`. This is tricky because it is bound to a `KeyBindingViewModel` which must provide _all_ context necessary to modify the UI and the settings model. We cannot use names to target UI elements inside this template, so we must make the view model smart and force updates to the UI via changes in the view model. - `KeyBindingViewModel` is a view model object that controls the UI and the settings model. There are a number of TODOs in Actions.cpp will be long-term follow-ups and would be nice to have. This includes... - a binary search by name on `Actions::KeyBindingList` - presenting an error when the provided key chord is invalid. ## Demo ![Actions Page Demo](https://user-images.githubusercontent.com/11050425/116034988-131d1b80-a619-11eb-8df2-c7e57c6fad86.gif)

## Summary of the Pull Request This PR builds on the `ActionMap` PR (#6900) by leveraging `ActionMap` to serialize actions. From the top down, the process is now as follows: - `CascadiaSettings`: remove the hack of copying whatever we had for actions before. - `GlobalAppSettings`: serialize the `ActionMap` to `"actions": []` - `ActionMap`: iterate over the internal `_ActionMap` (list of actions) and serialize each `Command` - `Command`: **THIS IS WHERE THE MAGIC HAPPENS!** For _each_ key mapping, serialize an action. Only the first one needs to include the name and icon. - `ActionAndArgs`: Find the relevant `IActionArgs` parser and serialize the `ActionAndArgs`. - `ActionArgs`: **ANNOYING CHANGE** Serialize any args that are set. We _need_ each setting to be saved as a `std::optional`. As with inheritance, this allows us to distinguish an explicit setting to the default value (sometimes `null`) vs an implicit "give me the default value". This allows us to serialize only the relevant details of each action, rather than writing _all_ of the args. ## References - #8100: Inheritance/Layering for lists - This tracks layering and better serialization for color schemes _and_ actions. This PR resolves half of that issue. The next step is to apply the concepts used in this PR (and #9621) to solve the similar problem for color schemes. - #6900: Actions page ## Validation Steps Performed Tests added!

ghost · 2021-05-25T19:10:54Z

🎉Windows Terminal Preview v1.9.1445.0 has been released which incorporates this pull request.:tada:

Handy links:

This comment has been minimized.

Sign in to view

carlos-zamora commented Mar 25, 2021

View reviewed changes

carlos-zamora force-pushed the dev/cazamor/tsm/action-map branch from 54f17b8 to b71b595 Compare March 30, 2021 00:05

This comment has been minimized.

Sign in to view

carlos-zamora force-pushed the dev/cazamor/tsm/action-map branch from b71b595 to b9c2ac0 Compare April 9, 2021 21:14

This comment has been minimized.

Sign in to view

carlos-zamora changed the base branch from dev/cazamor/tsm/upgrade-command to main April 13, 2021 00:32

carlos-zamora mentioned this pull request Apr 13, 2021

Give Command ownership over KeyChord #9543

Closed

carlos-zamora force-pushed the dev/cazamor/tsm/action-map branch from 70cd021 to c9b8294 Compare April 13, 2021 00:35

This comment has been minimized.

Sign in to view

carlos-zamora marked this pull request as ready for review April 13, 2021 00:43

carlos-zamora requested review from DHowett and zadjii-msft April 13, 2021 00:43

zadjii-msft reviewed Apr 13, 2021

View reviewed changes

zadjii-msft requested changes Apr 13, 2021

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 13, 2021

zadjii-msft assigned carlos-zamora Apr 13, 2021

zadjii-msft added Area-Settings Issues related to settings and customizability, for console or terminal Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Apr 13, 2021

zadjii-msft added this to the Terminal v1.9 milestone Apr 13, 2021

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 13, 2021

This comment has been minimized.

Sign in to view

zadjii-msft self-assigned this Apr 15, 2021

This comment has been minimized.

Sign in to view

carlos-zamora force-pushed the dev/cazamor/tsm/action-map branch from 41135af to efe5bd6 Compare May 3, 2021 22:39

lhecker approved these changes May 4, 2021

View reviewed changes

src/cascadia/TerminalSettingsEditor/Actions.h Show resolved Hide resolved

src/cascadia/TerminalSettingsModel/ActionMap.cpp Outdated Show resolved Hide resolved

src/cascadia/TerminalSettingsModel/Command.cpp Outdated Show resolved Hide resolved

apply Leonard's feedback

e3b0f48

DHowett reviewed May 4, 2021

View reviewed changes

src/cascadia/TerminalSettingsModel/ActionMap.h Outdated Show resolved Hide resolved

DHowett requested changes May 4, 2021

View reviewed changes

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels May 4, 2021

carlos-zamora added 2 commits May 4, 2021 18:04

address DHowett comments

28479dd

Merge branch 'main' into dev/cazamor/tsm/action-map

28ae1d9

DHowett requested changes May 5, 2021

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label May 5, 2021

moar DHowett comments

ad42c2d

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label May 5, 2021

DHowett reviewed May 5, 2021

View reviewed changes

src/cascadia/TerminalSettingsModel/ActionMap.cpp Outdated Show resolved Hide resolved

carlos-zamora added 2 commits May 4, 2021 21:18

find & replace the wrong one :(

900226a

Merge branch 'main' into dev/cazamor/tsm/action-map

24ada24

DHowett approved these changes May 5, 2021

View reviewed changes

carlos-zamora unassigned DHowett May 5, 2021

carlos-zamora merged commit 22fd06e into main May 5, 2021

carlos-zamora deleted the dev/cazamor/tsm/action-map branch May 5, 2021 04:50

carlos-zamora restored the dev/cazamor/tsm/action-map branch May 5, 2021 04:50

carlos-zamora deleted the dev/cazamor/tsm/action-map branch May 5, 2021 04:50

This was referenced May 12, 2021

Implement inheritance/layering behavior for settings lists/maps #8100

Open

Epic: Introduce Actions page to the Settings UI #6900

Open

ghost mentioned this pull request May 25, 2021

NewTab won't leave the Command Palette #7441

Closed

carlos-zamora mentioned this pull request Jun 4, 2021

Replace feedback button with command palette button in dropdown #10297

Merged

1 task

zadjii-msft mentioned this pull request Apr 20, 2023

Enable WINRT_LEAN_AND_MEAN #15215

Merged

Introduce ActionMap to Terminal Settings Model #9621

Introduce ActionMap to Terminal Settings Model #9621

Conversation

carlos-zamora commented Mar 25, 2021 • edited Loading

Summary of the Pull Request

References

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

This comment has been minimized.

carlos-zamora left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

zadjii-msft left a comment

Choose a reason for hiding this comment

zadjii-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

carlos-zamora commented Apr 15, 2021

This comment has been minimized.

carlos-zamora commented Apr 16, 2021

DHowett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

ghost commented May 25, 2021

carlos-zamora commented Mar 25, 2021 •

edited

Loading

carlos-zamora left a comment •

edited

Loading