Refactor ActionMap and Command to use ActionIDs #17162

PankajBhojwani · 2024-04-30T17:46:44Z

As outlined in #16816, refactor ActionMap to use the new action IDs added in #16904

Validation steps performed

Legacy style commands are parsed correctly (and rewritten to the new format)
Actions are still layered correctly and their IDs can be used to 'overwrite' actions in earlier layers
Keybindings that refer to an ID defined in another layer work correctly
User-defined actions without an ID have one generated for them (and their settings file is edited with it)
Schema updated

src/cascadia/UnitTests_SettingsModel/SerializationTests.cpp

lhecker

Mostly nits, and 1 bug.
I'm not sure I understood how all the changes fit together in the grand scheme of things. I'm not so deep into the ActionMap code. 🙈

src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp

src/cascadia/TerminalSettingsModel/ActionMap.cpp

PankajBhojwani · 2024-05-20T22:08:17Z

I'm not sure I understood how all the changes fit together in the grand scheme of things

THIS

zadjii-msft

this one is much scarier

zadjii-msft · 2024-05-29T11:34:59Z

src/cascadia/TerminalApp/TerminalPage.cpp

@@ -1023,7 +1023,8 @@ namespace winrt::TerminalApp::implementation
        // NewTab(ProfileIndex=N) action
        NewTerminalArgs newTerminalArgs{ profileIndex };
        NewTabArgs newTabArgs{ newTerminalArgs };
-        auto profileKeyChord{ _settings.ActionMap().GetKeyBindingForAction(ShortcutAction::NewTab, newTabArgs) };
+        const auto id = fmt::format(FMT_COMPILE(L"Terminal.OpenNewTabProfile{}"), profileIndex);
+        const auto profileKeyChord{ _settings.ActionMap().GetKeyBindingForAction(id) };


There 100% is a issue right now for "I rebound keys for new tab profile N, but the dropdown still has the old ones" and I'm guessing this will solve that (because people can rebind the key for the literal Terminal.OpenNewTabProfile5 rather than just making a new Command for it)

#13943 I think

Yep this fixes that! Also fixes it in the cmd palette

@PankajBhojwani should this PR close that bug?

zadjii-msft · 2024-05-29T11:40:38Z

src/cascadia/TerminalSettingsModel/userDefaults.json

-        { "command": "paste", "keys": "ctrl+v" },
-        { "command": "find", "keys": "ctrl+shift+f" },
-        { "command": { "action": "splitPane", "split": "auto", "splitMode": "duplicate" }, "keys": "alt+shift+d" }
+        { "id": "Terminal.CopySelectedText", "keys": "ctrl+c" },


Okay so, if I want to

Remove the ctrl+shift+c keybinding set in the defaults

AND add a keybinding to alt+c

how do I do that now?

Does this add a ctrl+c keybinding to Terminal.CopySelectedText, or replace the original ctrl+shift+c?

Does this add a ctrl+c keybinding to Terminal.CopySelectedText, or replace the original ctrl+shift+c?

It only adds!

Remove the ctrl+shift+c keybinding set in the defaults

Then you add {"id": null, "keys": "ctrl+shift+c"} into the keybindings array

AND add a keybinding to alt+c

Add {"id": "<id of the command>", "keys": "alt+c"} into keybindings

zadjii-msft · 2024-05-29T11:43:45Z

src/cascadia/UnitTests_SettingsModel/KeyBindingsTests.cpp

-            { "command": "copy", "keys": ["ctrl+c"] },
-            { "command": { "action": "copy", "singleLine": false }, "keys": ["ctrl+shift+c"] },
-            { "command": { "action": "copy", "singleLine": true }, "keys": ["alt+shift+c"] },
+            { "command": "copy", "id": "Test.CopyNoArgs", "keys": ["ctrl+c"] },


for my own notes: the commands in this test needed id's because the JSON was getting parsed as origin::none? if it was origin::user, they wouldn't need id's, right?

Yes, since they are Origin::None we will not generate ids for them and so the ActionMap will refuse to add them.

lhecker

1 nit left. I think it's worth addressing, but otherwise LGTM from my side.

src/cascadia/TerminalSettingsModel/ActionMap.cpp

zadjii-msft

12/17

src/cascadia/TerminalSettingsModel/ActionMap.h

src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp

src/cascadia/TerminalSettingsModel/defaults.json

zadjii-msft · 2024-05-31T16:04:54Z

src/cascadia/TerminalSettingsModel/Command.cpp

@@ -20,14 +20,6 @@ namespace winrt
    namespace WUX = Windows::UI::Xaml;
 }

-static constexpr std::string_view NameKey{ "name" };


📝 moved to the header, so that these can be used in ActionMapSerialization

zadjii-msft · 2024-05-31T16:15:44Z

src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp

+                        _fixUpsAppliedDuringLoad = true;
+                    }
+
+                    if (origin == OriginTag::User && !jsonBlock.isMember(JsonKey(IDKey)))


📝 do we enforce ID's present for fragment actions now?

Yep! This is as specced (docs will need to be updated as well, I've got a todo item for that once these have been reviewed so I can do it all at once)

zadjii-msft · 2024-05-31T16:29:34Z

src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp

+
+        // if these keys are already bound to some command,
+        // we need to update that command to erase these keys as we are about to overwrite them
+        if (const auto foundCommand = _GetActionByKeyChordInternal(keys); foundCommand && *foundCommand)


i'm not gonna overindex on this, cause #17215 is right there and gets rid of this whole thing

zadjii-msft · 2024-05-31T17:23:42Z

src/cascadia/TerminalSettingsModel/ActionMap.cpp

-            // This command has a name, check if it's new.
-            if (newName != maskingCmd.Name())
+            // only add to the _ActionMap if there is an ID
+            if (auto cmdID = cmd.ID(); !cmdID.empty() && cmd.ActionAndArgs().Action() != ShortcutAction::Invalid)


nit: may be cleaner to read if we do a early return for cmdID.empty() || cmd.ActionAndArgs().Action() == ShortcutAction::Invalid instead of another level of indenting

agree. look at all that nesting

This was actually just a mistake - was checking for Invalid twice! Removed the excess check and this should be much more readable now. Also we cannot early return on cmdID.empty() because if it's a user command we should generate an ID for it.

zadjii-msft · 2024-05-31T17:29:44Z

src/cascadia/TerminalSettingsModel/ActionMap.cpp

-        //   { "command": "copy",  "keys": "ctrl+c" }       --> register "ctrl+c" (section above)
-        //   { "command": "copy",  "keys": "ctrl+shift+c" } --> also register "ctrl+shift+c" to the same Command (oldCmd)
-        if (oldCmd)
+        if (const auto foundCommand = _GetActionByKeyChordInternal(keys); foundCommand && *foundCommand)


📝 this block is gone in #17215 (the next follow up PR)

zadjii-msft · 2024-05-31T17:31:49Z

src/cascadia/TerminalSettingsModel/ActionMap.cpp

+        // in this case, we do _not_ want to use the id they provided, we want to use an empty id
+        // (empty id in the _KeyMap indicates the keychord was explicitly unbound)
+        const auto action = cmd.ActionAndArgs().Action();
+        const auto id = action == ShortcutAction::Invalid ? hstring{} : cmd.ID();


huh okay so if we unbind a key, then it's still in the keymap, but bound to the action ID "", which then doesn't resolve to an actual action

Yep! So previously an unbound key was bound to an actual Command object with action type Invalid, now the keybinding is just bound to an empty ID

zadjii-msft · 2024-05-31T17:36:56Z

src/cascadia/TerminalSettingsModel/ActionMap.cpp

@@ -431,7 +450,9 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
        // invalidate caches
        _NameMapCache = nullptr;
        _GlobalHotkeysCache = nullptr;
-        _KeyBindingMapCache = nullptr;
+        _CumulativeKeyMapCache = nullptr;


🔖 read from here down, need to read the top again

zadjii-msft · 2024-05-31T18:34:44Z

src/cascadia/UnitTests_SettingsModel/SerializationTests.cpp

+                    "name": "foo",
+                    "icon": "myCoolIconPath.png",
+                    "command": { "action": "sendInput", "input": "just some input" },
+                    "id": "User.sendInput.)" SEND_INPUT_ARCH_SPECIFIC_ACTION_HASH R"("


this is hilarious to me

DHowett · 2024-05-31T18:47:36Z

src/cascadia/TerminalApp/TerminalPage.cpp

@@ -826,7 +826,7 @@ namespace winrt::TerminalApp::implementation
            newTabFlyout.Items().Append(settingsItem);

            auto actionMap = _settings.ActionMap();
-            const auto settingsKeyChord{ actionMap.GetKeyBindingForAction(ShortcutAction::OpenSettings, OpenSettingsArgs{ SettingsTarget::SettingsUI }) };
+            const auto settingsKeyChord{ actionMap.GetKeyBindingForAction(L"Terminal.OpenSettingsUI") };


oh i really like how clean this feels now

DHowett

6/17 one block so far - reviewing still

src/cascadia/TerminalSettingsModel/ActionMap.h

src/cascadia/UnitTests_SettingsModel/SerializationTests.cpp

src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp

src/cascadia/TerminalSettingsModel/Command.h

src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp

src/cascadia/TerminalSettingsModel/defaults.json

src/cascadia/TerminalSettingsModel/ActionMap.cpp

PankajBhojwani added 30 commits March 5, 2024 11:35

add origin tag

90627b3

update calls in tests

9dff28f

fix tests

8bcbd0b

ah one of the tests uses this

052d063

generated

8cc82de

inbox makes more sense

642d0ab

default ids

66fe08f

conflict

2bb1b6c

merge origin

be193b2

generate IDs for user commands

db528c9

nits

7c907fe

spacing

b43191d

line

2093660

string of numbers is unsightly but it works

6c32539

update comment

eccd87f

move id generation to fixupusersettings

44510dc

this way is better

10d1fc8

even better, also get the ID from json

71bf90f

move this

d57c7a1

fix test

5c2307c

add tests

9fc6972

excess line

dca7df5

change tests

dd25ed7

Everytime

6e293a5

defaults conflict

af2d22f

first round of nits

bdf42c2

truncate and hex, debug assert

12f3aa9

null check

aa49212

fmt is smart

5ee630e

fmt_compile, fix test

360b92e

PankajBhojwani added 2 commits May 2, 2024 18:11

some new tests

80fc299

another test

ebc03e9

github-advanced-security bot found potential problems May 3, 2024

View reviewed changes

src/cascadia/UnitTests_SettingsModel/SerializationTests.cpp Fixed Show fixed Hide fixed

This comment has been minimized.

Sign in to view

PankajBhojwani added 6 commits May 2, 2024 19:06

nits

ccf1cc9

schema

7793c5c

move this to header

abef25d

schema conflict

4d35c14

generate here instead

3e31bda

delete user actions that are identical to inbox actions

14d83b5

PankajBhojwani mentioned this pull request May 8, 2024

Remove command's knowledge of its keys #17215

Open

4 tasks

lhecker reviewed May 17, 2024

View reviewed changes

leonard comments

6c6dd46

zadjii-msft reviewed May 29, 2024

View reviewed changes

lhecker approved these changes May 29, 2024

View reviewed changes

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

eraseif

b88a8c5

lhecker approved these changes May 30, 2024

View reviewed changes

zadjii-msft added this to the Terminal v1.22 milestone May 31, 2024

zadjii-msft reviewed May 31, 2024

View reviewed changes

zadjii-msft approved these changes May 31, 2024

View reviewed changes

DHowett reviewed May 31, 2024

View reviewed changes

DHowett requested changes May 31, 2024

View reviewed changes

microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label May 31, 2024

DHowett requested changes May 31, 2024

View reviewed changes

microsoft-github-policy-service bot removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label May 31, 2024

PankajBhojwani added 4 commits May 31, 2024 16:32

nits n fixes

9703815

1 more test

a80316d

x86 hash

625753c

fix loops

406312f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor ActionMap and Command to use ActionIDs #17162

Refactor ActionMap and Command to use ActionIDs #17162

PankajBhojwani commented Apr 30, 2024 •

edited by DHowett

This comment has been minimized.

lhecker left a comment

PankajBhojwani commented May 20, 2024

zadjii-msft left a comment

zadjii-msft May 29, 2024

zadjii-msft May 29, 2024

PankajBhojwani May 29, 2024

DHowett May 31, 2024

zadjii-msft May 29, 2024

PankajBhojwani May 29, 2024 •

edited

zadjii-msft May 29, 2024

PankajBhojwani May 30, 2024

lhecker left a comment

zadjii-msft left a comment

zadjii-msft May 31, 2024

zadjii-msft May 31, 2024

PankajBhojwani May 31, 2024

zadjii-msft May 31, 2024

zadjii-msft May 31, 2024

DHowett May 31, 2024

PankajBhojwani May 31, 2024

zadjii-msft May 31, 2024

zadjii-msft May 31, 2024

PankajBhojwani May 31, 2024

zadjii-msft May 31, 2024

zadjii-msft May 31, 2024

DHowett May 31, 2024

DHowett left a comment

Refactor ActionMap and Command to use ActionIDs #17162

Are you sure you want to change the base?

Refactor ActionMap and Command to use ActionIDs #17162

Conversation

PankajBhojwani commented Apr 30, 2024 • edited by DHowett

Validation steps performed

This comment has been minimized.

lhecker left a comment

Choose a reason for hiding this comment

PankajBhojwani commented May 20, 2024

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

PankajBhojwani May 29, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker 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

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

PankajBhojwani commented Apr 30, 2024 •

edited by DHowett

PankajBhojwani May 29, 2024 •

edited