Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor ActionMap and Command to use ActionIDs #17162

Merged
merged 84 commits into from
Jun 4, 2024

Conversation

PankajBhojwani
Copy link
Contributor

@PankajBhojwani PankajBhojwani commented Apr 30, 2024

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

Refs #16816
Closes #17133

Copy link
Member

@lhecker lhecker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/ActionMap.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsModel/ActionMap.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsModel/ActionMap.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsModel/ActionMap.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsModel/ActionMap.cpp Outdated Show resolved Hide resolved
@PankajBhojwani
Copy link
Contributor Author

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

THIS

Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one is much scarier

@@ -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) };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#13943 I think

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PankajBhojwani should this PR close that bug?

{ "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" },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@PankajBhojwani PankajBhojwani May 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{ "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"] },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@lhecker lhecker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/cascadia/TerminalSettingsModel/ActionMap.cpp Outdated Show resolved Hide resolved
@zadjii-msft zadjii-msft added this to the Terminal v1.22 milestone May 31, 2024
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

12/17

src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsModel/defaults.json Outdated Show resolved Hide resolved
@@ -20,14 +20,6 @@ namespace winrt
namespace WUX = Windows::UI::Xaml;
}

static constexpr std::string_view NameKey{ "name" };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

_fixUpsAppliedDuringLoad = true;
}

if (origin == OriginTag::User && !jsonBlock.isMember(JsonKey(IDKey)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)


// 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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// 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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree. look at all that nesting

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// { "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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// 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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is hilarious to me

@@ -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") };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh i really like how clean this feels now

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

6/17 one block so far - reviewing still

src/cascadia/TerminalSettingsModel/ActionMap.h Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsModel/ActionMap.h Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsModel/ActionMap.h Outdated Show resolved Hide resolved
@microsoft-github-policy-service 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
src/cascadia/TerminalSettingsModel/Command.h Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsModel/defaults.json Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsModel/ActionMap.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsModel/ActionMap.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsModel/ActionMap.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsModel/ActionMap.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsModel/ActionMap.cpp Outdated Show resolved Hide resolved
@microsoft-github-policy-service 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
@DHowett DHowett added this pull request to the merge queue Jun 3, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Jun 3, 2024
@DHowett DHowett added this pull request to the merge queue Jun 4, 2024
Merged via the queue into main with commit ece0c04 Jun 4, 2024
20 checks passed
@DHowett DHowett deleted the dev/pabhoj/action_refactor branch June 4, 2024 01:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pre-seeded actions from userDefaults.json should be removed after adding IDs
4 participants