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

Fix combining wt args and "wt new-tab" args in implicit context #8315

Merged
merged 1 commit into from
Nov 18, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
53 changes: 53 additions & 0 deletions src/cascadia/LocalTests_TerminalApp/CommandlineTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ namespace TerminalAppLocalTests
TEST_METHOD(TestMultipleCommandExecuteCommandlineAction);
TEST_METHOD(TestInvalidExecuteCommandlineAction);
TEST_METHOD(TestLaunchMode);
TEST_METHOD(TestLaunchModeWithNoCommand);

private:
void _buildCommandlinesHelper(AppCommandlineArgs& appArgs,
Expand Down Expand Up @@ -1224,4 +1225,56 @@ namespace TerminalAppLocalTests
VERIFY_ARE_EQUAL(appArgs.GetLaunchMode().value(), LaunchMode::MaximizedFocusMode);
}
}

void CommandlineTest::TestLaunchModeWithNoCommand()
{
{
Log::Comment(NoThrowString().Format(L"Pass a launch mode and profile"));

AppCommandlineArgs appArgs{};
std::vector<const wchar_t*> rawCommands{ L"wt.exe", L"-M", L"--profile", L"cmd" };
_buildCommandlinesHelper(appArgs, 1u, rawCommands);

VERIFY_IS_TRUE(appArgs.GetLaunchMode().has_value());
VERIFY_ARE_EQUAL(appArgs.GetLaunchMode().value(), LaunchMode::MaximizedMode);
VERIFY_ARE_EQUAL(1u, appArgs._startupActions.size());

auto actionAndArgs = appArgs._startupActions.at(0);
VERIFY_ARE_EQUAL(ShortcutAction::NewTab, actionAndArgs.Action());
VERIFY_IS_NOT_NULL(actionAndArgs.Args());
auto myArgs = actionAndArgs.Args().try_as<NewTabArgs>();
VERIFY_IS_NOT_NULL(myArgs);
VERIFY_IS_NOT_NULL(myArgs.TerminalArgs());
VERIFY_IS_TRUE(myArgs.TerminalArgs().Commandline().empty());
VERIFY_IS_TRUE(myArgs.TerminalArgs().StartingDirectory().empty());
VERIFY_IS_TRUE(myArgs.TerminalArgs().TabTitle().empty());
VERIFY_IS_TRUE(myArgs.TerminalArgs().ProfileIndex() == nullptr);
VERIFY_IS_FALSE(myArgs.TerminalArgs().Profile().empty());
VERIFY_ARE_EQUAL(L"cmd", myArgs.TerminalArgs().Profile());
}
{
Log::Comment(NoThrowString().Format(L"Pass a launch mode and command line"));

AppCommandlineArgs appArgs{};
std::vector<const wchar_t*> rawCommands{ L"wt.exe", L"-M", L"powershell.exe" };
_buildCommandlinesHelper(appArgs, 1u, rawCommands);

VERIFY_IS_TRUE(appArgs.GetLaunchMode().has_value());
VERIFY_ARE_EQUAL(appArgs.GetLaunchMode().value(), LaunchMode::MaximizedMode);
VERIFY_ARE_EQUAL(1u, appArgs._startupActions.size());

auto actionAndArgs = appArgs._startupActions.at(0);
VERIFY_ARE_EQUAL(ShortcutAction::NewTab, actionAndArgs.Action());
VERIFY_IS_NOT_NULL(actionAndArgs.Args());
auto myArgs = actionAndArgs.Args().try_as<NewTabArgs>();
VERIFY_IS_NOT_NULL(myArgs);
VERIFY_IS_NOT_NULL(myArgs.TerminalArgs());
VERIFY_IS_FALSE(myArgs.TerminalArgs().Commandline().empty());
VERIFY_IS_TRUE(myArgs.TerminalArgs().StartingDirectory().empty());
VERIFY_IS_TRUE(myArgs.TerminalArgs().TabTitle().empty());
VERIFY_IS_TRUE(myArgs.TerminalArgs().ProfileIndex() == nullptr);
VERIFY_IS_TRUE(myArgs.TerminalArgs().Profile().empty());
VERIFY_ARE_EQUAL(L"powershell.exe", myArgs.TerminalArgs().Commandline());
}
}
}
54 changes: 21 additions & 33 deletions src/cascadia/TerminalApp/AppCommandlineArgs.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -69,50 +69,29 @@ int AppCommandlineArgs::ParseCommand(const Commandline& command)
{
throw CLI::CallForHelp();
}
// Clear the parser's internal state
_app.clear();

// attempt to parse the commandline
// attempt to parse the commandline prefix of the form [options][subcommand]
_app.parse(args);
auto remainingParams = _app.remaining_size();

// If we parsed the commandline, and _no_ subcommands were provided, try
// parsing again as a "new-tab" command.

// parse the remaining suffix as a "new-tab" command.
if (_noCommandsProvided())
{
_newTabCommand.subcommand->clear();
_newTabCommand.subcommand->parse(args);
remainingParams = _newTabCommand.subcommand->remaining_size();
}

// if after parsing the prefix and (optionally) the implicit tab subcommand
// we still have unparsed parameters we need to fail
if (remainingParams > 0)
{
throw CLI::ExtrasError(args);
Copy link
Member

Choose a reason for hiding this comment

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

who catches this? Does it end up down on line 92 as a ParseError?

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. It is caught there as a parse error. Implemented this to allow future handling. I can handle it immediately if you prefer.

}
}
catch (const CLI::CallForHelp& e)
Copy link
Member

Choose a reason for hiding this comment

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

are we losing support for CallForHelp?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From my understanding the CallForHelp was here only to prevent implicit subcommand parsing (the second catch clause). Now we don't have this catch clause, thus we can handle all exceptions the same way by calling _handleExit, that will still show the relevant UI.

{
return _handleExit(_app, e);
}
catch (const CLI::ParseError& e)
{
// If we parsed the commandline, and _no_ subcommands were provided, try
// parsing again as a "new-tab" command.
if (_noCommandsProvided())
{
try
{
// CLI11 mutated the original vector the first time it tried to
// parse the args. Reconstruct it the way CLI11 wants here.
// "See above for why it's begin() + 1"
std::vector<std::string> args{ command.Args().begin() + 1, command.Args().end() };
std::reverse(args.begin(), args.end());
_newTabCommand.subcommand->clear();
_newTabCommand.subcommand->parse(args);
}
catch (const CLI::ParseError& e)
{
return _handleExit(*_newTabCommand.subcommand, e);
}
}
else
{
return _handleExit(_app, e);
}
return _handleExit(_app, e);
}
return 0;
}
Expand Down Expand Up @@ -157,6 +136,15 @@ int AppCommandlineArgs::_handleExit(const CLI::App& command, const CLI::Error& e
// - <none>
void AppCommandlineArgs::_buildParser()
{
// We define or parser as a prefix command, to support "implicit new tab subcommand" scenario.
// In this scenario we will try to parse the prefix that contains parameters like launch mode,
// but will not encounter an explicit command.
// Instead we will encounter an argument that doesn't belong to the prefix indicating the prefix is over.
// Then we will try to parse the remaining arguments as a new tab subcommand.
// E.g., for "wt.exe -M -d c:/", we will use -M for the launch mode, but once we will encounter -d
// we will know that the prefix is over and try to handle the suffix as a new tab subcommand
_app.prefix_command();

// -v,--version: Displays version info
auto versionCallback = [this](int64_t /*count*/) {
// Set our message to display the application name and the current version.
Expand Down