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

Add ability to save input action from command line #16513

Merged
merged 21 commits into from
Jul 2, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
83e3b8c
Add ability to save input action from command line
e82eric Jan 1, 2024
dd46338
Added command line options for Save Input
e82eric Jan 1, 2024
8bec5ec
Handle key chord parse failure
e82eric Jan 2, 2024
eec4f6b
Saved Input: Use TextBlocks instead of TextBoxes for toast, exclude
e82eric Jan 10, 2024
e54b4a4
Clean up of Save Action ActionArgs
e82eric Jan 20, 2024
03f57ac
Merge branch 'main' into save_input_action
zadjii-msft Mar 18, 2024
e0b0477
format
zadjii-msft Mar 19, 2024
0ad6296
Fix for keychord not being saved
e82eric Mar 21, 2024
d4eaec3
Update so that save input toasts do not show display params for values
e82eric Mar 21, 2024
50baa7b
Update save input subcommand to x-save
e82eric Mar 21, 2024
addf247
Merge remote-tracking branch 'origin/main' into save_input_action
e82eric May 28, 2024
49a74c8
Move save input behind velocity
e82eric May 29, 2024
cf56a9a
cherry-pick into save-input PR (mostly stashing so I make sure I don'…
zadjii-msft May 28, 2024
3f15d6e
Fix for save input toast text boxes not hiding when blank
e82eric May 29, 2024
2bf8391
Updates for SaveTask for PR feedback
e82eric Jun 4, 2024
f5ceac9
Updated save task and save action to save snippet
e82eric Jun 4, 2024
d6873c1
Add validation to avoid saving snippet without a command line
e82eric Jun 4, 2024
cd13fc3
Merge remote-tracking branch 'origin/main' into save_snippet
e82eric Jun 11, 2024
8377749
Address PR feedback
e82eric Jul 1, 2024
452bfce
Merge remote-tracking branch 'origin/main' into save_snippet
e82eric Jul 1, 2024
6c79af0
format
zadjii-msft Jul 2, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 49 additions & 0 deletions src/cascadia/TerminalApp/AppActionHandlers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1234,6 +1234,55 @@ namespace winrt::TerminalApp::implementation
args.Handled(true);
}
}

void TerminalPage::_HandleSaveTask(const IInspectable& /*sender*/,
const ActionEventArgs& args)
{
if (args)
{
if (const auto& realArgs = args.ActionArgs().try_as<SaveTaskArgs>())
{
if (realArgs.Commandline().empty())
{
if (const auto termControl{ _GetActiveControl() })
{
if (termControl.HasSelection())
{
const auto selections{ termControl.SelectedText(true) };
auto selection = std::accumulate(selections.begin(), selections.end(), std::wstring());
realArgs.Commandline(selection);
Copy link
Member

Choose a reason for hiding this comment

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

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()) {
    // ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

}
}
}

winrt::Microsoft::Terminal::Control::KeyChord keyChord = nullptr;
if (!realArgs.KeyChord().empty())
{
try
{
keyChord = KeyChordSerialization::FromString(winrt::to_hstring(realArgs.KeyChord()));
_settings.GlobalSettings().ActionMap().AddSendInputAction(realArgs.Name(), realArgs.Commandline(), keyChord);
_settings.WriteSettingsToDisk();
Copy link
Member

Choose a reason for hiding this comment

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

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 🤷

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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();

ActionSaved(realArgs.Commandline(), realArgs.Name(), KeyChordSerialization::ToString(keyChord));
}
catch (const winrt::hresult_error& ex)
{
auto code = ex.code();
auto message = ex.message();
ActionSaveFailed(message);
args.Handled(true);
return;
}
}

_settings.GlobalSettings().ActionMap().AddSendInputAction(realArgs.Name(), realArgs.Commandline(), keyChord);
_settings.WriteSettingsToDisk();
ActionSaved(realArgs.Commandline(), realArgs.Name(), KeyChordSerialization::ToString(keyChord));

args.Handled(true);
}
}
}

void TerminalPage::_HandleSelectCommand(const IInspectable& /*sender*/,
const ActionEventArgs& args)
Expand Down
74 changes: 73 additions & 1 deletion src/cascadia/TerminalApp/AppCommandlineArgs.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,7 @@ void AppCommandlineArgs::_buildParser()
_buildMovePaneParser();
_buildSwapPaneParser();
_buildFocusPaneParser();
_buildSaveParser();
}

// Method Description:
Expand Down Expand Up @@ -537,6 +538,76 @@ void AppCommandlineArgs::_buildFocusPaneParser()
setupSubcommand(_focusPaneShort);
}

void AppCommandlineArgs::_buildSaveParser()
{
_saveCommand = _app.add_subcommand("save", RS_A(L"SaveActionDesc"));

auto setupSubcommand = [this](auto* subcommand) {
subcommand->add_option("--name,-n", _saveInputName, RS_A(L"SaveActionArgDesc"));
subcommand->add_option("--keychord,-k", _keyChordOption, RS_A(L"KeyChordArgDesc"));
subcommand->add_option("command,", _commandline, RS_A(L"CmdCommandArgDesc"));
subcommand->positionals_at_end(true);

// When ParseCommand is called, if this subcommand was provided, this
// callback function will be triggered on the same thread. We can be sure
// that `this` will still be safe - this function just lets us know this
// command was parsed.
subcommand->callback([&, this]() {
// 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,
Copy link
Member

Choose a reason for hiding this comment

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

a bit of copypasta here 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

// as it might clear those options while finding the commandline
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to suggestion inside of commit 2bf8391

SaveTaskArgs args{};

if (!_commandline.empty())
{
std::ostringstream cmdlineBuffer;

for (const auto& arg : _commandline)
{
if (cmdlineBuffer.tellp() != 0)
{
// If there's already something in here, prepend a space
cmdlineBuffer << ' ';
}

if (arg.find(" ") != std::string::npos)
{
cmdlineBuffer << '"' << arg << '"';
}
else
{
cmdlineBuffer << arg;
}
}

args.Commandline(winrt::to_hstring(cmdlineBuffer.str()));
}

if (!_keyChordOption.empty())
{
args.KeyChord(winrt::to_hstring(_keyChordOption));
}

if (!_saveInputName.empty())
{
winrt::hstring hString = winrt::to_hstring(_saveInputName);
args.Name(hString);
}
else
{
args.Name(args.GenerateName());
Copy link
Member

Choose a reason for hiding this comment

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

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

}

saveAction.Args(args);
_startupActions.push_back(saveAction);
});
};

setupSubcommand(_saveCommand);
}

// Method Description:
// - Add the `NewTerminalArgs` parameters to the given subcommand. This enables
// that subcommand to support all the properties in a NewTerminalArgs.
Expand Down Expand Up @@ -700,7 +771,8 @@ bool AppCommandlineArgs::_noCommandsProvided()
*_focusPaneCommand ||
*_focusPaneShort ||
*_newPaneShort.subcommand ||
*_newPaneCommand.subcommand);
*_newPaneCommand.subcommand ||
*_saveCommand);
}

// Method Description:
Expand Down
4 changes: 4 additions & 0 deletions src/cascadia/TerminalApp/AppCommandlineArgs.h
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ class TerminalApp::AppCommandlineArgs final
CLI::App* _swapPaneCommand;
CLI::App* _focusPaneCommand;
CLI::App* _focusPaneShort;
CLI::App* _saveCommand;

// Are you adding a new sub-command? Make sure to update _noCommandsProvided!

Expand Down Expand Up @@ -121,6 +122,8 @@ class TerminalApp::AppCommandlineArgs final
bool _focusPrevTab{ false };

int _focusPaneTarget{ -1 };
std::string _saveInputName;
std::string _keyChordOption;
// Are you adding more args here? Make sure to reset them in _resetStateToDefault

const Commandline* _currentCommandline{ nullptr };
Expand All @@ -139,6 +142,7 @@ class TerminalApp::AppCommandlineArgs final
winrt::Microsoft::Terminal::Settings::Model::NewTerminalArgs _getNewTerminalArgs(NewTerminalSubcommand& subcommand);
void _addNewTerminalArgs(NewTerminalSubcommand& subcommand);
void _buildParser();
void _buildSaveParser();
void _buildNewTabParser();
void _buildSplitPaneParser();
void _buildFocusTabParser();
Expand Down
9 changes: 9 additions & 0 deletions src/cascadia/TerminalApp/Resources/en-US/Resources.resw
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,15 @@
<data name="CmdCommandArgDesc" xml:space="preserve">
<value>An optional command, with arguments, to be spawned in the new tab or pane</value>
</data>
<data name="SaveActionDesc" xml:space="preserve">
<value>Save command line as input action</value>
</data>
<data name="SaveActionArgDesc" xml:space="preserve">
<value>An optional argument</value>
</data>
<data name="KeyChordArgDesc" xml:space="preserve">
<value>An optional argument</value>
</data>
<data name="CmdFocusTabDesc" xml:space="preserve">
<value>Move focus to another tab</value>
</data>
Expand Down
9 changes: 4 additions & 5 deletions src/cascadia/TerminalApp/TerminalAppLib.vcxproj
Original file line number Diff line number Diff line change
Expand Up @@ -343,7 +343,9 @@
</ItemGroup>
<!-- ========================= Misc Files ======================== -->
<ItemGroup>
<PRIResource Include="Resources\en-US\Resources.resw" />
<PRIResource Include="Resources\en-US\Resources.resw">
<SubType>Designer</SubType>
</PRIResource>
<PRIResource Include="Resources\en-US\ContextMenu.resw" />
<OCResourceDirectory Include="Resources" />
</ItemGroup>
Expand Down Expand Up @@ -383,7 +385,6 @@
<Private>true</Private>
<ReferenceOutputAssembly>false</ReferenceOutputAssembly>
</ProjectReference>

</ItemGroup>
<PropertyGroup>
<!-- This is a hack to get the ARM64 CI build working. See
Expand Down Expand Up @@ -449,10 +450,8 @@
</ItemDefinitionGroup>
<!-- ========================= Globals ======================== -->
<Import Project="$(OpenConsoleDir)src\cppwinrt.build.post.props" />

<!-- This -must- go after cppwinrt.build.post.props because that includes many VS-provided props including appcontainer.common.props, which stomps on what cppwinrt.targets did. -->
<Import Project="$(OpenConsoleDir)src\common.nugetversions.targets" />

<!--
By default, the PRI file will contain resource paths beginning with the
project name. Since we enabled XBF embedding, this *also* includes App.xbf.
Expand All @@ -473,4 +472,4 @@
</ItemGroup>
</Target>
<Import Project="$(SolutionDir)build\rules\CollectWildcardResources.targets" />
</Project>
</Project>
59 changes: 59 additions & 0 deletions src/cascadia/TerminalApp/TerminalPage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4205,6 +4205,65 @@ namespace winrt::TerminalApp::implementation
}
}

winrt::fire_and_forget TerminalPage::ActionSaved(winrt::hstring input, winrt::hstring name, winrt::hstring keyChord)
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

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

Choose a reason for hiding this comment

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

Does this code get called from a background thread?

Copy link
Contributor Author

@e82eric e82eric Jul 1, 2024

Choose a reason for hiding this comment

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

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.

{
// If we haven't ever loaded the TeachingTip, then do so now and
// create the toast for it.
if (page->_actionSavedToast == nullptr)
{
if (auto tip{ page->FindName(L"ActionSavedToast").try_as<MUX::Controls::TeachingTip>() })
{
page->_actionSavedToast = std::make_shared<Toast>(tip);
// Make sure to use the weak ref when setting up this
// callback.
tip.Closed({ page->get_weak(), &TerminalPage::_FocusActiveControl });
}
}
_UpdateTeachingTipTheme(ActionSavedToast().try_as<winrt::Windows::UI::Xaml::FrameworkElement>());
ActionSavedNameText().Text(L"Name: " + name);
ActionSavedKeyChordText().Text(L"Key Chord: " + keyChord);
ActionSavedCommandLineText().Text(L"Input: " + input);

if (page->_actionSavedToast != nullptr)
{
page->_actionSavedToast->Open();
}
}
}

winrt::fire_and_forget TerminalPage::ActionSaveFailed(winrt::hstring message)
{
auto weakThis{ get_weak() };
co_await wil::resume_foreground(Dispatcher());
if (auto page{ weakThis.get() })
{
// If we haven't ever loaded the TeachingTip, then do so now and
// create the toast for it.
if (page->_actionSaveFailedToast == nullptr)
{
if (auto tip{ page->FindName(L"ActionSaveFailedToast").try_as<MUX::Controls::TeachingTip>() })
{
page->_actionSaveFailedToast = std::make_shared<Toast>(tip);
// Make sure to use the weak ref when setting up this
// callback.
tip.Closed({ page->get_weak(), &TerminalPage::_FocusActiveControl });
}
}
_UpdateTeachingTipTheme(ActionSaveFailedToast().try_as<winrt::Windows::UI::Xaml::FrameworkElement>());

ActionSaveFailedMessage().Text(message);

if (page->_actionSaveFailedToast != nullptr)
{
page->_actionSaveFailedToast->Open();
}
}
}

// Method Description:
// - Called when an attempt to rename the window has failed. This will open
// the toast displaying a message to the user that the attempt to rename
Expand Down
4 changes: 4 additions & 0 deletions src/cascadia/TerminalApp/TerminalPage.h
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,8 @@ namespace winrt::TerminalApp::implementation
winrt::hstring KeyboardServiceDisabledText();

winrt::fire_and_forget IdentifyWindow();
winrt::fire_and_forget ActionSaved(winrt::hstring input, winrt::hstring name, winrt::hstring keyChord);
winrt::fire_and_forget ActionSaveFailed(winrt::hstring message);
winrt::fire_and_forget RenameFailed();
winrt::fire_and_forget ShowTerminalWorkingDirectory();

Expand Down Expand Up @@ -260,6 +262,8 @@ namespace winrt::TerminalApp::implementation
bool _isEmbeddingInboundListener{ false };

std::shared_ptr<Toast> _windowIdToast{ nullptr };
std::shared_ptr<Toast> _actionSavedToast{ nullptr };
std::shared_ptr<Toast> _actionSaveFailedToast{ nullptr };
std::shared_ptr<Toast> _windowRenameFailedToast{ nullptr };
std::shared_ptr<Toast> _windowCwdToast{ nullptr };

Expand Down
23 changes: 23 additions & 0 deletions src/cascadia/TerminalApp/TerminalPage.xaml
Original file line number Diff line number Diff line change
Expand Up @@ -219,5 +219,28 @@
Title="{x:Bind WindowProperties.VirtualWorkingDirectory, Mode=OneWay}"
x:Load="False"
IsLightDismissEnabled="True" />

<mux:TeachingTip x:Name="ActionSavedToast"
x:Uid="ActionSavedToast"
Title="Action Saved"
x:Load="False"
IsLightDismissEnabled="True">
<mux:TeachingTip.Content>
<StackPanel Orientation="Vertical">
<TextBox x:Name="ActionSavedNameText" Text="" />
Copy link
Member

Choose a reason for hiding this comment

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

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

<TextBox x:Name="ActionSavedKeyChordText" Text="" />
<TextBox x:Name="ActionSavedCommandLineText" Text="" />
</StackPanel>
</mux:TeachingTip.Content>
</mux:TeachingTip>
<mux:TeachingTip x:Name="ActionSaveFailedToast"
x:Uid="ActionSaveFailedToast"
Title="Action Save Failed"
x:Load="False"
IsLightDismissEnabled="True">
<mux:TeachingTip.Content>
<TextBox x:Name="ActionSaveFailedMessage" Text="" />
</mux:TeachingTip.Content>
</mux:TeachingTip>
</Grid>
</Page>
2 changes: 2 additions & 0 deletions src/cascadia/TerminalSettingsModel/ActionAndArgs.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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" };
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

static constexpr std::string_view SuggestionsKey{ "showSuggestions" };
static constexpr std::string_view ToggleFocusModeKey{ "toggleFocusMode" };
static constexpr std::string_view SetFocusModeKey{ "setFocusMode" };
Expand Down Expand Up @@ -388,6 +389,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
{ ShortcutAction::TabSearch, RS_(L"TabSearchCommandKey") },
{ ShortcutAction::ToggleAlwaysOnTop, RS_(L"ToggleAlwaysOnTopCommandKey") },
{ ShortcutAction::ToggleCommandPalette, MustGenerate },
{ ShortcutAction::SaveTask, MustGenerate },
{ ShortcutAction::Suggestions, MustGenerate },
{ ShortcutAction::ToggleFocusMode, RS_(L"ToggleFocusModeCommandKey") },
{ ShortcutAction::SetFocusMode, MustGenerate },
Expand Down
8 changes: 8 additions & 0 deletions src/cascadia/TerminalSettingsModel/ActionArgs.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
#include "ScrollToMarkArgs.g.cpp"
#include "AddMarkArgs.g.cpp"
#include "FindMatchArgs.g.cpp"
#include "SaveTaskArgs.g.cpp"
#include "ToggleCommandPaletteArgs.g.cpp"
#include "SuggestionsArgs.g.cpp"
#include "NewWindowArgs.g.cpp"
Expand Down Expand Up @@ -939,6 +940,13 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
}
}

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

};
}

static winrt::hstring _FormatColorString(const Control::SelectionColor& selectionColor)
{
if (!selectionColor)
Expand Down
Loading
Loading