-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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
Changes from 3 commits
83e3b8c
dd46338
8bec5ec
eec4f6b
e54b4a4
03f57ac
e0b0477
0ad6296
d4eaec3
50baa7b
addf247
49a74c8
cf56a9a
3f15d6e
2bf8391
f5ceac9
d6873c1
cd13fc3
8377749
452bfce
6c79af0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
} | ||
} | ||
} | ||
|
||
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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❌ 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 🤷 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
|
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -209,6 +209,7 @@ void AppCommandlineArgs::_buildParser() | |||||||||
_buildMovePaneParser(); | ||||||||||
_buildSwapPaneParser(); | ||||||||||
_buildFocusPaneParser(); | ||||||||||
_buildSaveParser(); | ||||||||||
} | ||||||||||
|
||||||||||
// Method Description: | ||||||||||
|
@@ -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, | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. a bit of copypasta here 😅 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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()); | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 The json for this will end up like
I'm pretty sure we'd want to just leave that |
||||||||||
} | ||||||||||
|
||||||||||
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. | ||||||||||
|
@@ -700,7 +771,8 @@ bool AppCommandlineArgs::_noCommandsProvided() | |||||||||
*_focusPaneCommand || | ||||||||||
*_focusPaneShort || | ||||||||||
*_newPaneShort.subcommand || | ||||||||||
*_newPaneCommand.subcommand); | ||||||||||
*_newPaneCommand.subcommand || | ||||||||||
*_saveCommand); | ||||||||||
} | ||||||||||
|
||||||||||
// Method Description: | ||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4205,6 +4205,65 @@ namespace winrt::TerminalApp::implementation | |
} | ||
} | ||
|
||
winrt::fire_and_forget TerminalPage::ActionSaved(winrt::hstring input, winrt::hstring name, winrt::hstring keyChord) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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() }) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this code get called from a background thread? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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="" /> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These may need to be |
||
<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> |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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" }; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) |
||
static constexpr std::string_view SuggestionsKey{ "showSuggestions" }; | ||
static constexpr std::string_view ToggleFocusModeKey{ "toggleFocusMode" }; | ||
static constexpr std::string_view SetFocusModeKey{ "setFocusMode" }; | ||
|
@@ -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 }, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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" | ||
|
@@ -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()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
|
There was a problem hiding this comment.
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:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated