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

Jump to bottom

Add support for naming windows with the -w parameter #9300

Merged
11 commits merged into from Mar 17, 2021
Merged

Add support for naming windows with the -w parameter #9300

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
1c7da00
Rebase all the changes on main
zadjii-msft Feb 25, 2021
97818c6
fix a bug and fix the tests
zadjii-msft Feb 25, 2021
2ec9415
fix tests
zadjii-msft Feb 25, 2021
e13e1e7
Good ole Java
zadjii-msft Feb 25, 2021
136ce6d
finish that test
zadjii-msft Feb 25, 2021
27d12a2
THIS NEEDS TO GO TO THE PARENT
zadjii-msft Feb 26, 2021
c202257
feedback from review
zadjii-msft Mar 4, 2021
eaaa204
initial review feedback
zadjii-msft Mar 16, 2021
9f54229
Merge remote-tracking branch 'origin/main' into dev/migrie/f/name-win…
zadjii-msft Mar 16, 2021
bba09e3
Address PR concerns
zadjii-msft Mar 16, 2021
1105328
That's better
zadjii-msft Mar 17, 2021
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
187 changes: 145 additions & 42 deletions src/cascadia/LocalTests_TerminalApp/CommandlineTest.cpp
View file
Open in desktop
Expand Up @@ -1594,59 +1594,155 @@ namespace TerminalAppLocalTests
void CommandlineTest::TestFindTargetWindow()
{
{
Log::Comment(L"wt.exe with no args should always use the value from"
L" the settings (passed as the second argument).");

std::vector<winrt::hstring> args{ L"wt.exe" };
VERIFY_ARE_EQUAL(WindowingBehaviorUseNew,
appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseNew));
auto result = appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseNew);
Copy link
Member

Choose a reason for hiding this comment

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

It might be helpful to a future debugger to leave either a general comment or a Log comment per block here as to the general gist of each test section.

VERIFY_ARE_EQUAL(WindowingBehaviorUseNew, result.WindowId());
VERIFY_ARE_EQUAL(L"", result.WindowName());

VERIFY_ARE_EQUAL(WindowingBehaviorUseExisting,
appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseExisting));
result = appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseExisting);
VERIFY_ARE_EQUAL(WindowingBehaviorUseExisting, result.WindowId());
VERIFY_ARE_EQUAL(L"", result.WindowName());

VERIFY_ARE_EQUAL(WindowingBehaviorUseAnyExisting,
appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseAnyExisting));
result = appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseAnyExisting);
VERIFY_ARE_EQUAL(WindowingBehaviorUseAnyExisting, result.WindowId());
VERIFY_ARE_EQUAL(L"", result.WindowName());
}
{
Log::Comment(L"-w -1 should always result in a new window");

std::vector<winrt::hstring> args{ L"wt.exe", L"-w", L"-1" };
VERIFY_ARE_EQUAL(WindowingBehaviorUseNew,
appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseNew));
auto result = appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseNew);
VERIFY_ARE_EQUAL(WindowingBehaviorUseNew, result.WindowId());
VERIFY_ARE_EQUAL(L"", result.WindowName());

result = appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseExisting);
VERIFY_ARE_EQUAL(WindowingBehaviorUseNew, result.WindowId());
VERIFY_ARE_EQUAL(L"", result.WindowName());

result = appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseAnyExisting);
VERIFY_ARE_EQUAL(WindowingBehaviorUseNew, result.WindowId());
VERIFY_ARE_EQUAL(L"", result.WindowName());
}
{
Log::Comment(L"\"new\" should always result in a new window");

std::vector<winrt::hstring> args{ L"wt.exe", L"-w", L"new" };
auto result = appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseNew);
VERIFY_ARE_EQUAL(WindowingBehaviorUseNew, result.WindowId());
VERIFY_ARE_EQUAL(L"", result.WindowName());

VERIFY_ARE_EQUAL(WindowingBehaviorUseNew,
appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseExisting));
result = appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseExisting);
VERIFY_ARE_EQUAL(WindowingBehaviorUseNew, result.WindowId());
VERIFY_ARE_EQUAL(L"", result.WindowName());

VERIFY_ARE_EQUAL(WindowingBehaviorUseNew,
appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseAnyExisting));
result = appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseAnyExisting);
VERIFY_ARE_EQUAL(WindowingBehaviorUseNew, result.WindowId());
VERIFY_ARE_EQUAL(L"", result.WindowName());
}
{
Log::Comment(L"-w with a negative number should always result in a "
L"new window");

std::vector<winrt::hstring> args{ L"wt.exe", L"-w", L"-12345" };
VERIFY_ARE_EQUAL(WindowingBehaviorUseNew,
appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseNew));
auto result = appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseNew);
VERIFY_ARE_EQUAL(WindowingBehaviorUseNew, result.WindowId());
VERIFY_ARE_EQUAL(L"", result.WindowName());

result = appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseExisting);
VERIFY_ARE_EQUAL(WindowingBehaviorUseNew, result.WindowId());
VERIFY_ARE_EQUAL(L"", result.WindowName());

result = appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseAnyExisting);
VERIFY_ARE_EQUAL(WindowingBehaviorUseNew, result.WindowId());
VERIFY_ARE_EQUAL(L"", result.WindowName());
}
{
Log::Comment(L"-w with a positive number should result in us trying"
L" to either make a new one or find an existing one "
L"with that ID, depending on the provided argument");

std::vector<winrt::hstring> args{ L"wt.exe", L"-w", L"12345" };
auto result = appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseNew);
VERIFY_ARE_EQUAL(12345, result.WindowId());
VERIFY_ARE_EQUAL(L"", result.WindowName());

VERIFY_ARE_EQUAL(WindowingBehaviorUseNew,
appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseExisting));
result = appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseExisting);
VERIFY_ARE_EQUAL(12345, result.WindowId());
VERIFY_ARE_EQUAL(L"", result.WindowName());

VERIFY_ARE_EQUAL(WindowingBehaviorUseNew,
appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseAnyExisting));
result = appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseAnyExisting);
VERIFY_ARE_EQUAL(12345, result.WindowId());
VERIFY_ARE_EQUAL(L"", result.WindowName());
}
{
Log::Comment(L"-w 0 should always use the \"current\" window");

std::vector<winrt::hstring> args{ L"wt.exe", L"-w", L"0" };
VERIFY_ARE_EQUAL(WindowingBehaviorUseCurrent,
appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseNew));
auto result = appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseNew);
VERIFY_ARE_EQUAL(WindowingBehaviorUseCurrent, result.WindowId());
VERIFY_ARE_EQUAL(L"", result.WindowName());

result = appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseExisting);
VERIFY_ARE_EQUAL(WindowingBehaviorUseCurrent, result.WindowId());
VERIFY_ARE_EQUAL(L"", result.WindowName());

result = appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseAnyExisting);
VERIFY_ARE_EQUAL(WindowingBehaviorUseCurrent, result.WindowId());
VERIFY_ARE_EQUAL(L"", result.WindowName());
}
{
Log::Comment(L"-w last should always use the most recent window on "
L"this desktop");

std::vector<winrt::hstring> args{ L"wt.exe", L"-w", L"last" };
auto result = appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseNew);
VERIFY_ARE_EQUAL(WindowingBehaviorUseExisting, result.WindowId());
VERIFY_ARE_EQUAL(L"", result.WindowName());

VERIFY_ARE_EQUAL(WindowingBehaviorUseCurrent,
appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseExisting));
result = appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseExisting);
VERIFY_ARE_EQUAL(WindowingBehaviorUseExisting, result.WindowId());
VERIFY_ARE_EQUAL(L"", result.WindowName());

VERIFY_ARE_EQUAL(WindowingBehaviorUseCurrent,
appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseAnyExisting));
result = appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseAnyExisting);
VERIFY_ARE_EQUAL(WindowingBehaviorUseExisting, result.WindowId());
VERIFY_ARE_EQUAL(L"", result.WindowName());
}
{
Log::Comment(L"Make sure we follow the provided argument when a "
L"--window-id wasn't explicitly provided");

std::vector<winrt::hstring> args{ L"wt.exe", L"new-tab" };
VERIFY_ARE_EQUAL(WindowingBehaviorUseNew,
appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseNew));
auto result = appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseNew);
VERIFY_ARE_EQUAL(WindowingBehaviorUseNew, result.WindowId());
VERIFY_ARE_EQUAL(L"", result.WindowName());

result = appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseExisting);
VERIFY_ARE_EQUAL(WindowingBehaviorUseExisting, result.WindowId());
VERIFY_ARE_EQUAL(L"", result.WindowName());

result = appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseAnyExisting);
VERIFY_ARE_EQUAL(WindowingBehaviorUseAnyExisting, result.WindowId());
VERIFY_ARE_EQUAL(L"", result.WindowName());
}
{
Log::Comment(L"Even if someone uses a subcommand as a window name, "
L"that should work");

std::vector<winrt::hstring> args{ L"wt.exe", L"-w", L"new-tab" };
auto result = appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseNew);
VERIFY_ARE_EQUAL(WindowingBehaviorUseName, result.WindowId());
VERIFY_ARE_EQUAL(L"new-tab", result.WindowName());

VERIFY_ARE_EQUAL(WindowingBehaviorUseExisting,
appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseExisting));
result = appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseExisting);
VERIFY_ARE_EQUAL(WindowingBehaviorUseName, result.WindowId());
VERIFY_ARE_EQUAL(L"new-tab", result.WindowName());

VERIFY_ARE_EQUAL(WindowingBehaviorUseAnyExisting,
appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseAnyExisting));
result = appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseAnyExisting);
VERIFY_ARE_EQUAL(WindowingBehaviorUseName, result.WindowId());
VERIFY_ARE_EQUAL(L"new-tab", result.WindowName());
}
}

Expand All @@ -1657,19 +1753,23 @@ namespace TerminalAppLocalTests
// This is a little helper to make sure that these args _always_ return
// UseNew, regardless of the windowing behavior.
auto testHelper = [](auto&& args) {
VERIFY_ARE_EQUAL(WindowingBehaviorUseNew,
appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseNew));
auto result = appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseNew);
VERIFY_ARE_EQUAL(WindowingBehaviorUseNew, result.WindowId());
VERIFY_ARE_EQUAL(L"", result.WindowName());

VERIFY_ARE_EQUAL(WindowingBehaviorUseNew,
appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseExisting));
result = appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseExisting);
VERIFY_ARE_EQUAL(WindowingBehaviorUseNew, result.WindowId());
VERIFY_ARE_EQUAL(L"", result.WindowName());

VERIFY_ARE_EQUAL(WindowingBehaviorUseNew,
appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseAnyExisting));
result = appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseAnyExisting);
VERIFY_ARE_EQUAL(WindowingBehaviorUseNew, result.WindowId());
VERIFY_ARE_EQUAL(L"", result.WindowName());
};

testHelper(std::vector<winrt::hstring>{ L"wt.exe", L"--help" });
testHelper(std::vector<winrt::hstring>{ L"wt.exe", L"new-tab", L"--help" });
testHelper(std::vector<winrt::hstring>{ L"wt.exe", L"-w", L"0", L"new-tab", L"--help" });
testHelper(std::vector<winrt::hstring>{ L"wt.exe", L"-w", L"foo", L"new-tab", L"--help" });
testHelper(std::vector<winrt::hstring>{ L"wt.exe", L"new-tab", L";", L"--help" });
}

Expand All @@ -1680,14 +1780,17 @@ namespace TerminalAppLocalTests
// This is a little helper to make sure that these args _always_ return
// UseNew, regardless of the windowing behavior.
auto testHelper = [](auto&& args) {
VERIFY_ARE_EQUAL(WindowingBehaviorUseNew,
appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseNew));
auto result = appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseNew);
VERIFY_ARE_EQUAL(WindowingBehaviorUseNew, result.WindowId());
VERIFY_ARE_EQUAL(L"", result.WindowName());

VERIFY_ARE_EQUAL(WindowingBehaviorUseNew,
appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseExisting));
result = appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseExisting);
VERIFY_ARE_EQUAL(WindowingBehaviorUseNew, result.WindowId());
VERIFY_ARE_EQUAL(L"", result.WindowName());

VERIFY_ARE_EQUAL(WindowingBehaviorUseNew,
appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseAnyExisting));
result = appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseAnyExisting);
VERIFY_ARE_EQUAL(WindowingBehaviorUseNew, result.WindowId());
VERIFY_ARE_EQUAL(L"", result.WindowName());
};

testHelper(std::vector<winrt::hstring>{ L"wt.exe", L"--version" });
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/Remoting/FindTargetWindowArgs.h
View file
Open in desktop
Expand Up @@ -27,6 +27,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
{
WINRT_PROPERTY(winrt::Microsoft::Terminal::Remoting::CommandlineArgs, Args, nullptr);
WINRT_PROPERTY(int, ResultTargetWindow, -1);
WINRT_PROPERTY(winrt::hstring, ResultTargetWindowName);

public:
FindTargetWindowArgs(winrt::Microsoft::Terminal::Remoting::CommandlineArgs args) :
Expand Down
65 changes: 63 additions & 2 deletions src/cascadia/Remoting/Monarch.cpp
View file
Open in desktop
Expand Up @@ -147,6 +147,59 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
}
}

// Method Description:
// - Find the ID of the peasant with the given name. If no such peasant
// exists, then we'll return 0. If we encounter any peasants who have died
// during this process, then we'll remove them from the set of _peasants
// Arguments:
// - name: The window name to look for
// Return Value:
// - 0 if we didn't find the given peasant, otherwise a positive number for
// the window's ID.
uint64_t Monarch::_lookupPeasantIdForName(std::wstring_view name)
{
if (name.empty())
{
return 0;
}

std::vector<uint64_t> peasantsToErase{};
uint64_t result = 0;
for (const auto& [id, p] : _peasants)
{
try
{
auto otherName = p.WindowName();
if (otherName == name)
Copy link
Member

Choose a reason for hiding this comment

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

Case sensitive compare? Are you sure? Lots of people love to be lazy on the capitalization when doing a command line thing and I suspect that might apply to -w.

{
result = id;
break;
}
}
catch (...)
{
LOG_CAUGHT_EXCEPTION();
// Normally, we'd just erase the peasant here. However, we can't
// erase from the map while we're iterating over it like this.
// Instead, pull a good ole Java and collect this id for removal
// later.
peasantsToErase.push_back(id);
}
}

// Remove the dead peasants we came across while iterating.
for (const auto& id : peasantsToErase)
{
// Remove the peasant from the list of peasants
_peasants.erase(id);
// Remove the peasant from the list of MRU windows. They're dead.
// They can't be the MRU anymore.
_clearOldMruEntries(id);
}

return result;
}

// Method Description:
// - Handler for the `Peasant::WindowActivated` event. We'll make a in-proc
// copy of the WindowActivatedArgs from the peasant. That way, we won't
Expand Down Expand Up @@ -400,6 +453,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
// After the event was handled, ResultTargetWindow() will be filled with
// the parsed result.
const auto targetWindow = findWindowArgs->ResultTargetWindow();
const auto targetWindowName = findWindowArgs->ResultTargetWindowName();

TraceLoggingWrite(g_hRemotingProvider,
"Monarch_ProposeCommandline",
Expand All @@ -410,6 +464,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
// that goes with it. Alternatively, if we were given a magic windowing
// constant, we can use that to look up an appropriate peasant.
if (targetWindow >= 0 ||
targetWindow == WindowingBehaviorUseName ||
targetWindow == WindowingBehaviorUseExisting ||
targetWindow == WindowingBehaviorUseAnyExisting)
{
Expand All @@ -429,6 +484,9 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
case WindowingBehaviorUseAnyExisting:
windowID = _getMostRecentPeasantID(false);
break;
case WindowingBehaviorUseName:
windowID = _lookupPeasantIdForName(targetWindowName);
break;
default:
windowID = ::base::saturated_cast<uint64_t>(targetWindow);
break;
Expand All @@ -449,7 +507,6 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
if (auto targetPeasant{ _getPeasant(windowID) })
{
auto result{ winrt::make_self<Remoting::implementation::ProposeCommandlineResult>(false) };

try
{
// This will raise the peasant's ExecuteCommandlineRequested
Expand All @@ -463,6 +520,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
// If we fail to propose the commandline to the peasant (it
// died?) then just tell this process to become a new window
// instead.
result->WindowName(targetWindowName);
result->ShouldCreateWindow(true);

// If this fails, it'll be logged in the following
Expand Down Expand Up @@ -497,6 +555,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation

auto result{ winrt::make_self<Remoting::implementation::ProposeCommandlineResult>(true) };
result->Id(windowID);
result->WindowName(targetWindowName);
return *result;
}
}
Expand All @@ -508,6 +567,8 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE));

// In this case, no usable ID was provided. Return { true, nullopt }
return winrt::make<Remoting::implementation::ProposeCommandlineResult>(true);
auto result = winrt::make_self<Remoting::implementation::ProposeCommandlineResult>(true);
result->WindowName(targetWindowName);
return *result;
}
}