Add ability to save input action from command line #16513

e82eric · 2024-01-01T17:53:25Z

Hi wanted to make an attempt at 12857. This still needs work but I think the initial version is ready to be reviewed.

Summary of the Pull Request

Mostly copied from: 6f5b9fb...1cde67a

Save to disk
If command line is empty use selection
Show toast
No UI. Trying out the different options now.

PR Checklist

Closes Ability to save selected text as a sendInput action #12857
Tests added/passed
Documentation updated
- If checked, please file a pull request on our docs repo and link it here: #xxx
Schema updated (if necessary)

e82eric · 2024-01-01T17:57:05Z

src/cascadia/TerminalSettingsModel/ActionMap.cpp

@@ -857,6 +857,18 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
        AddAction(*cmd);
    }

+    void ActionMap::AddSendInputAction(winrt::hstring name, winrt::hstring input)


I tried to add this in TerminalPage::_HandleSaveTask, the build errors from calling winrt::make(input) from that project defeated me.

- Save to disk - If command line is empty use selection - Show toast

e82eric · 2024-01-02T06:31:58Z

zadjii-msft

Just a drive by review for now:

structurally, this looks like it hits in all the right places
I like the confirmation toast. Great idea.
Testing with the commandline mode of the command palette was a clever idea here, really liked that
I'm excited about this and definitely need to make some additional time for this review soon

zadjii-msft · 2024-01-09T20:58:43Z

src/cascadia/TerminalApp/TerminalPage.xaml

+                         IsLightDismissEnabled="True">
+            <mux:TeachingTip.Content>
+                <StackPanel Orientation="Vertical">
+                    <TextBox x:Name="ActionSavedNameText" Text="" />


These may need to be TextBlock's, instead of TextBoxes. Blocks are just for displaying text, while the Box is supposed to be for input. (super intuitive, I know 🙃)

zadjii-msft · 2024-01-09T21:03:00Z

src/cascadia/TerminalSettingsModel/ActionArgs.cpp

+    winrt::hstring SaveTaskArgs::GenerateName() const
+    {
+        return winrt::hstring{
+            fmt::format(L"Save Task commandline:{}, name: {}, keyChord {}", Commandline(), Name(), KeyChord())


nit: we may want to omit the arg from the generated name, if the value of the arg is just empty. Like, in your gif, it always shows "keychord ," even when the keys weren't provided yet

Updated, let me know if that looks ok.

For the toast, I have it showing None when the keyChord is null, is it better to hide the TextBlocks from the toast's content?

Right now if the same name is used I think it will overwrite the existing action, does it make sense to have a --force arg, and fail if there is a existing action for the name and no force arg?

blank arges from generated name

zadjii-msft

More drive-by comments. We discussed possibly holding this from 1.20 (which is gonna be a smaller, more stablization-focused release), and then merge it in for 1.21.

Sorry that we've been slow here. Hopefully in a few weeks, we'll be able to better start moving code again 😄

zadjii-msft · 2024-01-19T21:23:38Z

src/cascadia/TerminalApp/AppCommandlineArgs.cpp

+            }
+            else
+            {
+                args.Name(args.GenerateName());


This right here might not be right. In the case that you don't pass a --name explicitly, this ends up setting the SaveTask::Name to something like "Save task commandline: git status". Then later, we'll build the SendInput action, and set it's name to "Save task commandline: git status".

The json for this will end up like

{ "command": { "action": "sendInput", "input": "git status" }, "name": "Save Task commandline: git status" },

I'm pretty sure we'd want to just leave that .Name empty

zadjii-msft · 2024-01-19T21:50:58Z

src/cascadia/TerminalSettingsModel/ActionArgs.cpp

+    {
+        std::wstring formattedString;
+
+        if (!Name().empty() && !KeyChord().empty())


You may want to copy the pattern in SplitPaneArgs::GenerateName, which uses a stringstream, and then just adds each successive parameter to the stream (if the param is provided)

zadjii-msft · 2024-01-19T21:51:34Z

src/cascadia/TerminalSettingsModel/ActionArgs.cpp

+
+        if (!Name().empty() && !KeyChord().empty())
+        {
+            formattedString = fmt::format(L"Save Task commandline: {}, name: {}, keyChord {}", Commandline(), Name(), KeyChord());


"Save Task" will almost definitely need to be in the resources.resw, and loaded with RS_ (like the other actions)

zadjii-msft · 2024-01-19T21:55:52Z

src/cascadia/TerminalApp/TerminalPage.xaml

+                         IsLightDismissEnabled="True">
+            <mux:TeachingTip.Content>
+                <StackPanel Orientation="Vertical" HorizontalAlignment="Stretch">
+                    <TextBlock x:Name="ActionSavedNameText" Text="" />


Playing with this locally, there's still like, a faint background to these text blocks. I can't quite tell why - maybe there's a weird interaction with the acrylic of the Toast. Any chance you're seeing that? Maybe it's just me 😅

I tried a few different settings and wasn't able to notice anything. I am not able to see depths at all, so I may just be missing it.

e82eric · 2024-01-20T16:32:20Z

More drive-by comments. We discussed possibly holding this from 1.20 (which is gonna be a smaller, more stablization-focused release), and then merge it in for 1.21.

Sorry that we've been slow here. Hopefully in a few weeks, we'll be able to better start moving code again 😄

Sounds good! No rush on my end.

zadjii-msft · 2024-03-18T21:39:07Z

Notes for team discussion later this week:

Overall, I think I'm fine with the ideas in this PR. Namely:
- the wt save subcommand to save a commandline as a sendInput action (aka, a "task")
- the saveTask action
- the toasts
There are some annoying things that I think probably need to get done before we can ship this, and I'm volunteering to finish them in post. Namely:
- wt save should default to the current window. I kinda did something similar here: a13e81d#diff-dfd6c47961e6a1b340dcf4fcb82921004045b3269597521aeba822af41eb6e21R557-R561
- The toast should use x:Uid's and resources for localizable strings. I can handle that.
- The toast probably shouldn't display params that weren't filled. e.g. wt save foo shouldn only display
  
  Task saved
  commandline: Foo

But, this also lines up well with other work that's in progress (#13445 (comment)), so I'd like to get this in and keep iterating.

zadjii-msft · 2024-03-19T21:59:59Z

Discussion feedback:

Stick behind velocity
change subcommand to x-save, ala MIME types to indicate "this subcommand is experimental"

e82eric · 2024-03-20T12:34:45Z

I think the key chord parsing got broken with the upstream merge. I think I can take a look tonight.

e82eric · 2024-03-21T05:30:51Z

I think the key chord parsing got broken with the upstream merge. I think I can take a look tonight.

It looks like I had broken that here. Should be fixed now.

terminal/src/cascadia/TerminalApp/AppActionHandlers.cpp

Line 1280 in eec4f6b

ActionSaved(realArgs.Commandline(), realArgs.Name(), L"None");

that are not filled in

e82eric · 2024-03-21T06:14:51Z

Notes for team discussion later this week:

Overall, I think I'm fine with the ideas in this PR. Namely:

the wt save subcommand to save a commandline as a sendInput action (aka, a "task")

the saveTask action

the toasts

There are some annoying things that I think probably need to get done before we can ship this, and I'm volunteering to finish them in post. Namely:

wt save should default to the current window. I kinda did something similar here: a13e81d#diff-dfd6c47961e6a1b340dcf4fcb82921004045b3269597521aeba822af41eb6e21R557-R561

The toast should use x:Uid's and resources for localizable strings. I can handle that.

The toast probably shouldn't display params that weren't filled. e.g. wt save foo shouldn only display

Task saved
commandline: Foo

But, this also lines up well with other work that's in progress (#13445 (comment)), so I'd like to get this in and keep iterating.

Is this along the line of what you were thinking for not displaying unfilled display params? d4eaec3

Wanted to try to address this one but wasn't sure what the behavior was. Is there a way that I can reproduce it? "should default to _the current window"

e82eric · 2024-03-21T06:24:52Z

Discussion feedback:

Stick behind velocity

change subcommand to x-save, ala MIME types to indicate "this subcommand is experimental"

for x-save. is this along the lines of what you were thinking? 50baa7b

For the first, what does velocity mean in this context?

zadjii-msft · 2024-05-28T14:46:35Z

Hey sorry about not replying earlier. I was... delayed... getting ready for Build.

BUT THAT'S DONE NOW. NOW IT'S TIME FOR FUN.

I'm just gonna quick leave this link here: dev/migrie/save-input-patches so that I don't lose it.

I was prototyping some other fixes that would make the XAML binding less painful. Admittedly, that branched off probably two or three months ago, so I'm sure it's stale.

for x-save. is this along the lines of what you were thinking? 50baa7b

Exactly that! That's perfect.

For the first, what does velocity mean in this context?

"Velocity" is our hodgepodge'd way of controlling which features go into which branches. It's pretty much all in features.xml. (you already figured this out in e82eric@89a4e14). We just call it that still since that's what it's called in Windows internally.

I'm gonna start looping back on this now, cause this is a pretty important part of the story I think we can tell with #17329. Thanks for all the patience!

zadjii-msft

Honestly, my only concerns are over some comments? Yea that's not worth blocking over. (Things I mark with 📝 are meant as notes to myself while reviewing, which may be helpful for other reviewers)

The saving settings on the UI thread thing isn't great, but we should fix that for both the settings UI and this at once. That's not on you.

I'll come through after this and figure out how to yeet this action into the current terminal window using the Monarch. I'm not gonna ask you to try and wrap your head around the fuckiness that is our cross-window communication 😅

Thanks for all your patience here!

zadjii-msft · 2024-05-28T20:06:52Z

src/cascadia/TerminalSettingsModel/ActionAndArgs.cpp

@@ -52,6 +52,7 @@ static constexpr std::string_view SwitchToTabKey{ "switchToTab" };
 static constexpr std::string_view TabSearchKey{ "tabSearch" };
 static constexpr std::string_view ToggleAlwaysOnTopKey{ "toggleAlwaysOnTop" };
 static constexpr std::string_view ToggleCommandPaletteKey{ "commandPalette" };
+static constexpr std::string_view SaveTaskKey{ "saveTask" };


Okay this is super annoying but I think we collectively decided to change the name of this feature to "snippets" to better align with VS tools. We only decided that in like the last month, so that's on me to actually come through and fix

Made an attempt at this in the commit below. Not totally sure I got the names right. Let me know if I should add this commit to the PR (totally understand if you want to update it yourself).

e82eric@f5ceac9

Testing this I realized that I have an issue where you can save a snippet without a command line and a validation warning about the incorrect configs pops up after. I think this commit would fix it (It may need to be localized)

e82eric@d6873c1

zadjii-msft · 2024-06-03T10:27:16Z

src/cascadia/TerminalSettingsModel/ActionMap.cpp

+        {
+            cmd->Name(name);
+        }
+        cmd->RegisterKey(keys);


📝: this feels like it'll change after ~#17215, but that shouldn't be too bad

@e82eric FYI I believe that PR did merge now, so this one conflicts now. Let me know if you heed help with the conflict

It should be updated now. I ran into 2 questions through.

I copied the last 2 lines from ActionMap::RegisterKeyBinding instead of just calling ActionMap::RegisterKeyBinding to get the name to show up.

void ActionMap::AddSendInputAction(winrt::hstring name, winrt::hstring input, const Control::KeyChord keys) { auto newAction = winrt::make<ActionAndArgs>(); newAction.Action(ShortcutAction::SendInput); auto sendInputArgs = winrt::make<SendInputArgs>(input); newAction.Args(sendInputArgs); auto cmd{ make_self<Command>() }; cmd->ActionAndArgs(newAction); if (!name.empty()) { cmd->Name(name); } cmd->GenerateID(); AddAction(*cmd, keys); }

I keep wanting to move the above code to TerminalPage::_HandleSaveSnippet but I can't get winrt::make<ActionAndArgs>() to work. Should I keep trying to get that to work, or is it ok to have the one off method in ActionMap?

zadjii-msft · 2024-06-03T11:07:28Z

src/cascadia/TerminalApp/TerminalPage.xaml

+                            Orientation="Vertical">
+                    <TextBlock x:Name="ActionSavedNameText"
+                            Visibility="{x:Bind mtu:Converters.StringNotEmptyToVisibility(SavedActionName), Mode=OneWay}">
+                        <Run Text="Name: " />


📝: I think I'm fine with these not being localized, since they're literal params to the action args, and we've got precedent for that in the cmdpal already

zadjii-msft · 2024-06-03T11:09:50Z

src/cascadia/TerminalApp/AppActionHandlers.cpp

+    void TerminalPage::_HandleSaveTask(const IInspectable& /*sender*/,
+                                       const ActionEventArgs& args)
+    {
+        if (Feature_SaveTask::IsEnabled())


IMO I'd just do an early-return if the feature isn't enabled, but I'm not sure if that messes with the compiler's ability to just optimize the entire rest of the function out

Updated to early return. (Is there a way to tell if the compiler will struggle with this?)

zadjii-msft · 2024-06-03T11:10:42Z

src/cascadia/TerminalApp/AppActionHandlers.cpp

+    void TerminalPage::_HandleSaveTask(const IInspectable& /*sender*/,
+                                       const ActionEventArgs& args)
+    {
+        if (Feature_SaveTask::IsEnabled())


Oh also:

Suggested change

if (Feature_SaveTask::IsEnabled())

if constexpr (Feature_SaveTask::IsEnabled())

zadjii-msft · 2024-06-03T11:17:52Z

src/cascadia/TerminalApp/AppActionHandlers.cpp

+                            keyChord = KeyChordSerialization::FromString(winrt::to_hstring(realArgs.KeyChord()));
+                        }
+                        _settings.GlobalSettings().ActionMap().AddSendInputAction(realArgs.Name(), realArgs.Commandline(), keyChord);
+                        _settings.WriteSettingsToDisk();


❌ WriteSettingsToDisk writes the file on this thread. We definitely can't be doing this on the UI thread.

You might be able to just do a

auto saveSettings = [settings=_settings]() -> winrt::fire_and_forget { co_await winrt::resume_background(); settings.WriteSettingsToDisk(); }; saveSettings();

to basically make a lambda that does the fire_and_forget thing, but then also just call it immediately. I haven't tested it, so I'm sure there's spooky coroutine reasons that's a bad idea, but 🤷

Also now that I'm looking at it, the Settings UI itself saves the settings on the UI thread! Egads! I don't think it's worth blocking you over this if we can't even do it right

I gave this a try just so I could get a better understanding of how the fire_and_forget stuff works.

It looked like it worked, the settings were saved to the file, and I was able to see it switch from the window thread to the background thread in the debugger when it did the write to disk.

In the debugger I started getting a AV. Adding a strong ref to settings seemed to prevent that.

auto saveSettings = [settings = _settings]() -> winrt::fire_and_forget { const auto strongSettings = settings; co_await winrt::resume_background(); strongSettings.WriteSettingsToDisk(); }; saveSettings();

zadjii-msft · 2024-06-03T11:23:53Z

src/cascadia/TerminalApp/AppCommandlineArgs.cpp

+            // Build the NewTab action from the values we've parsed on the commandline.
+            ActionAndArgs saveAction{};
+            saveAction.Action(ShortcutAction::SaveTask);
+            // _getNewTerminalArgs MUST be called before parsing any other options,


a bit of copypasta here 😅

Updated to be slightly more generic. // Build the action from the values we've parsed on the commandline.

Does this make sense or should I just remove it?

zadjii-msft · 2024-06-03T11:25:38Z

src/cascadia/TerminalApp/AppCommandlineArgs.cpp

+            // _getNewTerminalArgs MUST be called before parsing any other options,
+            // as it might clear those options while finding the commandline


Suggested change

// _getNewTerminalArgs MUST be called before parsing any other options,

// as it might clear those options while finding the commandline

// First, parse out the commandline in the same way that

// _getNewTerminalArgs does it

Updated to suggestion inside of commit 2bf8391

lhecker · 2024-07-01T12:18:13Z

src/cascadia/TerminalApp/AppActionHandlers.cpp

+                        {
+                            const auto selections{ termControl.SelectedText(true) };
+                            const auto selection = std::accumulate(selections.begin(), selections.end(), std::wstring());
+                            realArgs.Commandline(selection);


This shouldn't modify the realArgs.Commandline() but instead extract the hstring and then modify the local reference. Like this:

auto commandline = realArgs.Commandline(); if (commandline.empty() { // ... commandline = selection; } if (commandline.empty()) { // ... }

lhecker · 2024-07-01T12:21:06Z

src/cascadia/TerminalApp/TerminalPage.cpp

@@ -4184,6 +4184,66 @@ namespace winrt::TerminalApp::implementation
        }
    }

+    winrt::fire_and_forget TerminalPage::ActionSaved(winrt::hstring input, winrt::hstring name, winrt::hstring keyChord)


Why put these two functions here instead of having them next to the related code inside AppActionHandlers.cpp? I feel like it should either be all here or all there.

lhecker · 2024-07-01T12:21:35Z

src/cascadia/TerminalApp/TerminalPage.cpp

+        auto weakThis{ get_weak() };
+        co_await wil::resume_foreground(Dispatcher());
+        if (auto page{ weakThis.get() })


Does this code get called from a background thread?

you are right, It was not running on a background thread, before and after the co_await it was on the Window thread. Updated to just use void and removed the co_await.

In the spec review, we agreed these didn't really need to be saved to the user's own settings file. This removes parsing and saving for the `experimental.saveSnippet` action, but we still have the action _internally_. This is powered by a new x-macro for "INTERNAL_" actions. Follow-up from #16513.

e82eric commented Jan 1, 2024

View reviewed changes

Add ability to save input action from command line

83e3b8c

- Save to disk - If command line is empty use selection - Show toast

e82eric force-pushed the save_input_action branch from 7cc2dea to 83e3b8c Compare January 1, 2024 18:23

Added command line options for Save Input

dd46338

e82eric force-pushed the save_input_action branch 2 times, most recently from 4ac3c7a to 5d99e7b Compare January 2, 2024 06:12

Handle key chord parse failure

8bec5ec

e82eric force-pushed the save_input_action branch from 5d99e7b to 8bec5ec Compare January 2, 2024 12:23

zadjii-msft reviewed Jan 9, 2024

View reviewed changes

Saved Input: Use TextBlocks instead of TextBoxes for toast, exclude

eec4f6b

blank arges from generated name

zadjii-msft mentioned this pull request Jan 19, 2024

Suggestions UI ship list #15845

Open

zadjii-msft reviewed Jan 19, 2024

View reviewed changes

zadjii-msft added this to the Terminal 1.21 milestone Jan 19, 2024

Clean up of Save Action ActionArgs

e54b4a4

Merge branch 'main' into save_input_action

03f57ac

zadjii-msft added the Needs-Discussion Something that requires a team discussion before we can proceed label Mar 18, 2024

format

e0b0477

Fix for keychord not being saved

0ad6296

Update so that save input toasts do not show display params for values

d4eaec3

that are not filled in

Update save input subcommand to x-save

50baa7b

zadjii-msft removed this from the Terminal v1.21 milestone May 28, 2024

Fix for save input toast text boxes not hiding when blank

3f15d6e

e82eric force-pushed the save_input_action branch from 4e85136 to 3f15d6e Compare May 29, 2024 07:09

zadjii-msft mentioned this pull request Jun 1, 2024

[megathread] Snippets #13000

Open

zadjii-msft approved these changes Jun 3, 2024

View reviewed changes

zadjii-msft removed their assignment Jun 3, 2024

microsoft-github-policy-service bot added Issue-Task It's a feature request, but it doesn't really need a major design. Area-Settings Issues related to settings and customizability, for console or terminal Product-Terminal The new Windows Terminal. labels Jun 3, 2024

zadjii-msft added a commit that referenced this pull request Jun 3, 2024

PRE-MERGE #16513 Add ability to save input action from command line

5460324

e82eric added 3 commits June 3, 2024 20:07

Updates for SaveTask for PR feedback

2bf8391

Updated save task and save action to save snippet

f5ceac9

Add validation to avoid saving snippet without a command line

d6873c1

This was referenced Jun 4, 2024

1.22 We broke hot reload entirely #17364

Closed

1.22: wt save should save the command in the current window, if at all possible #17366

Closed

Add a spec for "Snippets" #17329

Merged

Merge remote-tracking branch 'origin/main' into save_snippet

cd13fc3

e82eric force-pushed the save_input_action branch from c9d3e5a to cd13fc3 Compare June 11, 2024 06:13

lhecker approved these changes Jul 1, 2024

View reviewed changes

e82eric added 2 commits July 1, 2024 09:21

Address PR feedback

8377749

Merge remote-tracking branch 'origin/main' into save_snippet

452bfce

lhecker approved these changes Jul 2, 2024

View reviewed changes

format

6c79af0

zadjii-msft enabled auto-merge July 2, 2024 10:45

lhecker approved these changes Jul 2, 2024

View reviewed changes

zadjii-msft added this pull request to the merge queue Jul 2, 2024

Merged via the queue into microsoft:main with commit d051f70 Jul 2, 2024
17 checks passed

zadjii-msft mentioned this pull request Jul 9, 2024

Stop parsing saveSnippets actions in json #17535

Merged

	if (Feature_SaveTask::IsEnabled())
	if constexpr (Feature_SaveTask::IsEnabled())

		// _getNewTerminalArgs MUST be called before parsing any other options,
		// as it might clear those options while finding the commandline

Add ability to save input action from command line #16513

Add ability to save input action from command line #16513

Conversation

e82eric commented Jan 1, 2024 • edited by zadjii-msft Loading

Summary of the Pull Request

PR Checklist

e82eric Jan 1, 2024 • edited Loading

Choose a reason for hiding this comment

e82eric commented Jan 2, 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

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

e82eric commented Jan 20, 2024

zadjii-msft commented Mar 18, 2024

zadjii-msft commented Mar 19, 2024

e82eric commented Mar 20, 2024

e82eric commented Mar 21, 2024 • edited Loading

e82eric commented Mar 21, 2024 • edited Loading

e82eric commented Mar 21, 2024 • edited Loading

zadjii-msft commented May 28, 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

e82eric Jun 11, 2024 • edited Loading

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

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

e82eric Jul 1, 2024 • edited Loading

Choose a reason for hiding this comment

e82eric commented Jan 1, 2024 •

edited by zadjii-msft

Loading

e82eric Jan 1, 2024 •

edited

Loading

e82eric commented Mar 21, 2024 •

edited

Loading

e82eric commented Mar 21, 2024 •

edited

Loading

e82eric commented Mar 21, 2024 •

edited

Loading

e82eric Jun 11, 2024 •

edited

Loading

e82eric Jul 1, 2024 •

edited

Loading