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.

Sign up for GitHub

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

Move commandline->title promotion into TerminalSettings #11029

Merged
1 commit merged into from
Aug 24, 2021
Merged

Move commandline->title promotion into TerminalSettings #11029

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
83 changes: 83 additions & 0 deletions src/cascadia/LocalTests_SettingsModel/TerminalSettingsTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ namespace SettingsModelLocalTests

TEST_METHOD(TestLayerProfileOnColorScheme);

TEST_METHOD(TestCommandlineToTitlePromotion);

TEST_CLASS_SETUP(ClassSetup)
{
return true;
Expand Down Expand Up @@ -554,4 +556,85 @@ namespace SettingsModelLocalTests
VERIFY_ARE_EQUAL(ARGB(0, 0x45, 0x67, 0x89), terminalSettings4->CursorColor()); // from profile (no color scheme)
VERIFY_ARE_EQUAL(DEFAULT_CURSOR_COLOR, terminalSettings5->CursorColor()); // default
}

void TerminalSettingsTests::TestCommandlineToTitlePromotion()
{
const std::string settingsJson{ R"(
{
"defaultProfile": "{6239a42c-0000-49a3-80bd-e8fdd045185c}",
"profiles": { "list": [
{
"name": "profile0",
"guid": "{6239a42c-0000-49a3-80bd-e8fdd045185c}",
"historySize": 1,
"commandline": "cmd.exe"
},
],
"defaults": {
"historySize": 29
} }
})" };

const winrt::guid guid0{ ::Microsoft::Console::Utils::GuidFromString(L"{6239a42c-0000-49a3-80bd-e8fdd045185c}") };

CascadiaSettings settings{ til::u8u16(settingsJson) };

{ // just a profile (profile wins)
NewTerminalArgs args{};
args.Profile(L"profile0");
const auto settingsStruct{ TerminalSettings::CreateWithNewTerminalArgs(settings, args, nullptr) };
VERIFY_ARE_EQUAL(L"profile0", settingsStruct.DefaultSettings().StartingTitle());
}
{ // profile and command line -> no promotion (profile wins)
NewTerminalArgs args{};
args.Profile(L"profile0");
args.Commandline(L"foo.exe");
const auto settingsStruct{ TerminalSettings::CreateWithNewTerminalArgs(settings, args, nullptr) };
VERIFY_ARE_EQUAL(L"profile0", settingsStruct.DefaultSettings().StartingTitle());
}
{ // just a title -> it is propagated
NewTerminalArgs args{};
args.TabTitle(L"Analog Kid");
const auto settingsStruct{ TerminalSettings::CreateWithNewTerminalArgs(settings, args, nullptr) };
VERIFY_ARE_EQUAL(L"Analog Kid", settingsStruct.DefaultSettings().StartingTitle());
}
{ // title and command line -> no promotion
NewTerminalArgs args{};
args.TabTitle(L"Digital Man");
args.Commandline(L"foo.exe");
const auto settingsStruct{ TerminalSettings::CreateWithNewTerminalArgs(settings, args, nullptr) };
VERIFY_ARE_EQUAL(L"Digital Man", settingsStruct.DefaultSettings().StartingTitle());
}
{ // just a commandline -> promotion
NewTerminalArgs args{};
args.Commandline(L"foo.exe");
const auto settingsStruct{ TerminalSettings::CreateWithNewTerminalArgs(settings, args, nullptr) };
VERIFY_ARE_EQUAL(L"foo.exe", settingsStruct.DefaultSettings().StartingTitle());
}
// various typesof commandline follow
{
NewTerminalArgs args{};
args.Commandline(L"foo.exe bar");
const auto settingsStruct{ TerminalSettings::CreateWithNewTerminalArgs(settings, args, nullptr) };
VERIFY_ARE_EQUAL(L"foo.exe", settingsStruct.DefaultSettings().StartingTitle());
}
{
NewTerminalArgs args{};
args.Commandline(L"\"foo exe.exe\" bar");
const auto settingsStruct{ TerminalSettings::CreateWithNewTerminalArgs(settings, args, nullptr) };
VERIFY_ARE_EQUAL(L"foo exe.exe", settingsStruct.DefaultSettings().StartingTitle());
}
{
NewTerminalArgs args{};
args.Commandline(L"\"\" grand designs");
const auto settingsStruct{ TerminalSettings::CreateWithNewTerminalArgs(settings, args, nullptr) };
VERIFY_ARE_EQUAL(L"", settingsStruct.DefaultSettings().StartingTitle());
}
{
NewTerminalArgs args{};
args.Commandline(L" imagine a man");
const auto settingsStruct{ TerminalSettings::CreateWithNewTerminalArgs(settings, args, nullptr) };
VERIFY_ARE_EQUAL(L"", settingsStruct.DefaultSettings().StartingTitle());
}
}
}
7 changes: 0 additions & 7 deletions src/cascadia/TerminalApp/AppCommandlineArgs.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -603,13 +603,6 @@ NewTerminalArgs AppCommandlineArgs::_getNewTerminalArgs(AppCommandlineArgs::NewT
args.Profile(winrt::to_hstring(_profileName));
}

if (!*subcommand.profileNameOption && !_commandline.empty())
{
// If there's no profile, but there IS a command line, set the tab title to the first part of the command
// This will ensure that the tab we spawn has a name (since it didn't get one from its profile!)
args.TabTitle(winrt::to_hstring(til::at(_commandline, 0)));
}

if (*subcommand.startingDirectoryOption)
{
args.StartingDirectory(winrt::to_hstring(_startingDirectory));
Expand Down
15 changes: 15 additions & 0 deletions src/cascadia/TerminalSettingsModel/TerminalSettings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,21 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
{
defaultSettings.StartingTitle(newTerminalArgs.TabTitle());
}
else
{
// There was no title, and no profile from which to infer the title.
// Per GH#6776, promote the first component of the command line to the title.
// This will ensure that the tab we spawn has a name (since it didn't get one from its profile!)
if (newTerminalArgs.Profile().empty() && !newTerminalArgs.Commandline().empty())
{
const std::wstring_view commandLine{ newTerminalArgs.Commandline() };
const auto start{ til::at(commandLine, 0) == L'"' ? 1 : 0 };
const auto terminator{ commandLine.find_first_of(start ? L'"' : L' ', start) }; // look past the first character if it starts with "
// We have to take a copy here; winrt::param::hstring requires a null-terminated string
Comment on lines +135 to +137
Copy link
Member Author

Choose a reason for hiding this comment

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

this is rough but it works. doing a substr with npos-x later is safe (confirmed by docs)

Copy link
Member Author

Choose a reason for hiding this comment

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

if it yields a title of nothing, we already have a weird commandline. If the title is too long, oh well

Copy link
Member

Choose a reason for hiding this comment

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

Are you sure about this? winrt::param::hstring has an explicit std::wstring_view constructor and appears to work just fine with it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Dead sure. This blog post just came out today to reinforce: https://devblogs.microsoft.com/oldnewthing/20210823-00/?p=105598

Copy link
Member

Choose a reason for hiding this comment

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

shocked

Copy link
Member

Choose a reason for hiding this comment

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

Uh oh.

const std::wstring firstComponent{ commandLine.substr(start, terminator - start) };
defaultSettings.StartingTitle(firstComponent);
}
}
if (newTerminalArgs.TabColor())
{
defaultSettings.StartingTabColor(winrt::Windows::Foundation::IReference<winrt::Microsoft::Terminal::Core::Color>{ til::color{ newTerminalArgs.TabColor().Value() } });
Expand Down