Should we mention layering here too? Because the layering more applies to the container of objects than it does the individual Actions. Like, when you have

{ "keys": "ctrl+a", "command": "copy"}
{ "keys": "ctrl+a", "command": "paste"}

then we'll need to first set the KeyChordText of the copy action to "ctrl+a", but then when we go to replace copy in the keymap with paste, we'll need to clear that copy's KeyChordText when we replace it with paste

Added a layering section for the defaults.json and settings.json layering.

In an event like you described above where both occur in the same layer, my plan is to keep "copy" in the action list, but actually have "paste" be bound to ctrl+a. Serialization-wise, we'll only output:

{ "command": "copy"}
{ "keys": "ctrl+a", "command": "paste"}

I think this is what our current actions model does, because we just iterate over all of our actions and add them to the command palette independently of their key chord.

Wait could you elaborate more on how we get into the "remove an action" state? Like, removing the keys from an action, I get why we'd do that. I get why we'd set the name of an action to null. But why are we replacing an action with Unbound? Like, what's the user story here?

We talked a bit about this in the spec meeting, and I've updated the spec to kinda cover this a bit better. Let me know if it's still not clear though :)

So to be clear - "update the conflicting one" means something like "If a subsequent command has the same Keys as an existing one, we'll remove the Keys from the existing one, then update the _KeyMap to point at the subsequent command (instead of the original)"

For Names, we don't really need to remove the Name from the old command in the _NameMap, right?

I removed _NameMap from the spec so resolving.

NameMap is still in the spec in the public interface for ActionMap..?

Yeah, we still need a NameMap exposed to the projected type for the Command Palette. There's no (easy) way around that because the command palette needs to expand the iterable commands over in TermApp. So the process looks something like this:

• TermApp requests ActionMap::NameMap
• we generate the NameMap (as described in the spec)
• TermApp expands any iterable commands
• TermApp stores this expanded NameMap and uses it whenever the Command Palette requests it

Internally (to ActionMap), we won't be updating a _NameMap as we add more actions. It's just not worth it because we only want each action to appear once (and the _ActionMap already keeps track of that).

do nested commands have a special hash function that ensures that they will not collide with a hash generated from an Action+ActionAndArgs

or do all nested commands hash to the same thing

Added a section in the spec. The gist of it is...

• No, nested commands can only be identified by their given name. Thus, we cannot add them to the internal _ActionMap. Instead, we add them to their own map<hstring, Command> _NestedCommands.
• Handling collisions:
  • as actions get added to the ActionMap, add nested commands to their own _NestedCommands map.
  • If a newly added Command conflicts with a name in _NestedCommands, resolve in favor of the new Command by removing the conflicting nested command from _NestedCommands.

I'm still worried about this.

Consider that you have the following nested/repeated commands:

Split Pane...
   Windows PowerShell

New Tab...
   Windows PowerShell

those have the same name. They are very much different commands. We're not keying anything off their names, right? Well, except the Name Map which will immediately and horribly explode when you do that.

I think there seems to be some confusion from my word choice. When I say "nested commands", I always mean the root. So in your example, there are 2 nested commands:

• "split pane..."
• "new tab..."

Part 1: AddAction the nested command

AddAction receives these commands and does the following:

• is it a nested command?
  • Yes! Insert (or assign) "" to _NestedCommands
• Done!

Part 2: AddAction a conflicting command

Later, let's say we get a new Command x who's name is "Split Pane...". We have two paths here:

1. x is a nested command:
   • update _NestedCommands such that "Split Pane..." maps to x
2. x is a standard command:
   • remove Split Pane... from _NestedCommands
   • register x with _ActionMap

NOTE: We don't care what is inside of these nested commands. We just care about the root.

Part 3: NameMap generation with our parents

Now we get to part 3 of our story: dealing with parents. We don't really care about our parents until we have to generate the NameMap.

1. add all of the nested actions
   • Due to part 2, we can assume that we have a disjoint set of names between standard and nested actions. This means that all names in _NestedCommands are new to those from the standard actions.
   • If we start from our parents' nested actions, then work our way down to the current layer, we can create a NameMap filled with just nested commands. We don't know if any of them will persist, but that's ok, because then we move on to step 2...
2. add all of the standard/consolidated actions
   • ANY collisions are just straight-up assigned to these actions. The only collisions we can expect are the ones from our ancestors' nested commands intersecting with our standard actions. If that happens, we have the right to prioritize our standard actions.