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).

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...

1. construct a Command for each of the commands in the JSON
2. 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".

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:

1. 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)
2. 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.

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.

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).

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.