Skip to content

Commit

Permalink
Respect the startup info state initially passed to wt via ShellExec…
Browse files Browse the repository at this point in the history
…ute (#13838)

Original description, pre-process model v3:

> This is just the `SHOWDEFAULT` bit from #12979. This seems to also
work now, but I'm PR'ing it separately so it can be a separate revert
from #13811, if it is problematic.

More accurately: 
This PR enables terminal windows to use the `wShowCmd` from the
STARTUPINFO passed to `windowsterminal.exe` to set the initial
visibility of the window. We can't just use `SW_SHOWDEFAULT`, because
all the windows are running in the initial process! After the first
window, the subsequent ones would ignore any params passed to their
originating `windowsterminal.exe` processes. To mitigate, we pass that
`wShowCmd` info from the source process, to the actual running terminal
process. That accounts for most of the delta here.

Closes #9053


This doesn't do the same for defterm-initiated connections. This is
because we don't need to! Defterm very explicitly rejects handoff for
minimized console apps. This is probably for the best! I put an attempt
in 66f8b25 before I forgot that it was filtered long before the
Terminal. NOT doing this for /min saves us all sorts of "what happens if
`start /min cmd` tries to glom?" or "what if someone does `start /min
cmd && start /max cmd` and they glom together?".

<hr>

Also closes #15193, which was introduced as a part of this.

---------

Co-authored-by: Dustin L. Howett <duhowett@microsoft.com>
  • Loading branch information
zadjii-msft and DHowett authored Apr 18, 2023
1 parent 1825ca1 commit 2c16e7c
Show file tree
Hide file tree
Showing 11 changed files with 54 additions and 24 deletions.
1 change: 1 addition & 0 deletions .github/actions/spelling/expect/expect.txt
Original file line number Diff line number Diff line change
Expand Up @@ -804,6 +804,7 @@ HIBYTE
hicon
HIDEWINDOW
hinst
Hirots
HISTORYBUFS
HISTORYNODUP
HISTORYSIZE
Expand Down
8 changes: 6 additions & 2 deletions src/cascadia/Remoting/CommandlineArgs.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,11 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
}

CommandlineArgs(const winrt::array_view<const winrt::hstring>& args,
winrt::hstring currentDirectory) :
winrt::hstring currentDirectory,
const uint32_t showWindowCommand) :
_args{ args.begin(), args.end() },
_cwd{ currentDirectory }
_cwd{ currentDirectory },
_ShowWindowCommand{ showWindowCommand }
{
}

Expand All @@ -25,6 +27,8 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
void Commandline(const winrt::array_view<const winrt::hstring>& value);
winrt::com_array<winrt::hstring> Commandline();

WINRT_PROPERTY(uint32_t, ShowWindowCommand, SW_NORMAL); // SW_NORMAL is 1, 0 is SW_HIDE

private:
winrt::com_array<winrt::hstring> _args;
winrt::hstring _cwd;
Expand Down
4 changes: 3 additions & 1 deletion src/cascadia/Remoting/Monarch.h
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,8 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
_Id{ windowInfo.Id() ? windowInfo.Id().Value() : 0 }, // We'll use 0 as a sentinel, since no window will ever get to have that ID
_WindowName{ windowInfo.WindowName() },
_args{ command.Commandline() },
_CurrentDirectory{ command.CurrentDirectory() } {};
_CurrentDirectory{ command.CurrentDirectory() },
_ShowWindowCommand{ command.ShowWindowCommand() } {};

WindowRequestedArgs(const winrt::hstring& window, const winrt::hstring& content, const Windows::Foundation::IReference<Windows::Foundation::Rect>& bounds) :
_Id{ 0u },
Expand All @@ -63,6 +64,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
WINRT_PROPERTY(winrt::hstring, WindowName);
WINRT_PROPERTY(winrt::hstring, CurrentDirectory);
WINRT_PROPERTY(winrt::hstring, Content);
WINRT_PROPERTY(uint32_t, ShowWindowCommand, SW_NORMAL);
WINRT_PROPERTY(Windows::Foundation::IReference<Windows::Foundation::Rect>, InitialBounds);

private:
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/Remoting/Monarch.idl
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ namespace Microsoft.Terminal.Remoting

String[] Commandline { get; };
String CurrentDirectory { get; };
UInt32 ShowWindowCommand { get; };

String Content { get; };
Windows.Foundation.IReference<Windows.Foundation.Rect> InitialBounds { get; };
Expand Down
3 changes: 2 additions & 1 deletion src/cascadia/Remoting/Peasant.idl
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,11 @@ namespace Microsoft.Terminal.Remoting
runtimeclass CommandlineArgs
{
CommandlineArgs();
CommandlineArgs(String[] args, String cwd);
CommandlineArgs(String[] args, String cwd, UInt32 showWindowCommand);

String[] Commandline { get; set; };
String CurrentDirectory();
UInt32 ShowWindowCommand { get; };
};

runtimeclass RenameRequestArgs
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/Remoting/WindowManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -350,7 +350,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
// If the name wasn't specified, this will be an empty string.
p->WindowName(args.WindowName());

p->ExecuteCommandline(*winrt::make_self<CommandlineArgs>(args.Commandline(), args.CurrentDirectory()));
p->ExecuteCommandline(*winrt::make_self<CommandlineArgs>(args.Commandline(), args.CurrentDirectory(), args.ShowWindowCommand()));

_monarch.AddPeasant(*p);

Expand Down
4 changes: 4 additions & 0 deletions src/cascadia/ShellExtension/OpenTerminalHere.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,10 @@ try
STARTUPINFOEX siEx{ 0 };
siEx.StartupInfo.cb = sizeof(STARTUPINFOEX);

// Explicitly create the terminal window visible.
siEx.StartupInfo.dwFlags |= STARTF_USESHOWWINDOW;
siEx.StartupInfo.wShowWindow = SW_SHOWNORMAL;

std::filesystem::path modulePath{ wil::GetModuleFileNameW<std::wstring>(wil::GetModuleInstanceHandle()) };
std::wstring cmdline;
if (runElevated)
Expand Down
34 changes: 17 additions & 17 deletions src/cascadia/UnitTests_Remoting/RemotingTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -431,7 +431,7 @@ namespace RemotingUnitTests
m0->FindTargetWindowRequested(&RemotingTests::_findTargetWindowHelper);

std::vector<winrt::hstring> args{};
Remoting::CommandlineArgs eventArgs{ { args }, { L"" } };
Remoting::CommandlineArgs eventArgs{ { args }, { L"" }, SW_NORMAL };

auto result = m0->ProposeCommandline(eventArgs);
VERIFY_ARE_EQUAL(true, result.ShouldCreateWindow());
Expand Down Expand Up @@ -468,7 +468,7 @@ namespace RemotingUnitTests
});

std::vector<winrt::hstring> args{ L"1", L"arg[1]" };
Remoting::CommandlineArgs eventArgs{ { args }, { L"" } };
Remoting::CommandlineArgs eventArgs{ { args }, { L"" }, SW_NORMAL };

auto result = m0->ProposeCommandline(eventArgs);
VERIFY_ARE_EQUAL(false, result.ShouldCreateWindow());
Expand All @@ -489,15 +489,15 @@ namespace RemotingUnitTests

{
std::vector<winrt::hstring> args{ L"-1" };
Remoting::CommandlineArgs eventArgs{ { args }, { L"" } };
Remoting::CommandlineArgs eventArgs{ { args }, { L"" }, SW_NORMAL };

auto result = m0->ProposeCommandline(eventArgs);
VERIFY_ARE_EQUAL(true, result.ShouldCreateWindow());
VERIFY_ARE_EQUAL(false, (bool)result.Id());
}
{
std::vector<winrt::hstring> args{ L"-2" };
Remoting::CommandlineArgs eventArgs{ { args }, { L"" } };
Remoting::CommandlineArgs eventArgs{ { args }, { L"" }, SW_NORMAL };

auto result = m0->ProposeCommandline(eventArgs);
VERIFY_ARE_EQUAL(true, result.ShouldCreateWindow());
Expand Down Expand Up @@ -534,7 +534,7 @@ namespace RemotingUnitTests
winrt::clock().now() };
p1->ActivateWindow(activatedArgs);

Remoting::CommandlineArgs eventArgs{ { p1Args }, { L"" } };
Remoting::CommandlineArgs eventArgs{ { p1Args }, { L"" }, SW_NORMAL };

auto result = m0->ProposeCommandline(eventArgs);
VERIFY_ARE_EQUAL(false, result.ShouldCreateWindow());
Expand All @@ -559,7 +559,7 @@ namespace RemotingUnitTests
p2->ActivateWindow(activatedArgs);

Log::Comment(L"Send a commandline to the current window, which should be p2");
Remoting::CommandlineArgs eventArgs{ { p2Args }, { L"" } };
Remoting::CommandlineArgs eventArgs{ { p2Args }, { L"" }, SW_NORMAL };
auto result = m0->ProposeCommandline(eventArgs);
VERIFY_ARE_EQUAL(false, result.ShouldCreateWindow());
VERIFY_ARE_EQUAL(false, (bool)result.Id());
Expand All @@ -572,7 +572,7 @@ namespace RemotingUnitTests
p1->ActivateWindow(activatedArgs);

Log::Comment(L"Send a commandline to the current window, which should be p1 again");
Remoting::CommandlineArgs eventArgs{ { p1Args }, { L"" } };
Remoting::CommandlineArgs eventArgs{ { p1Args }, { L"" }, SW_NORMAL };
auto result = m0->ProposeCommandline(eventArgs);
VERIFY_ARE_EQUAL(false, result.ShouldCreateWindow());
VERIFY_ARE_EQUAL(false, (bool)result.Id());
Expand All @@ -593,7 +593,7 @@ namespace RemotingUnitTests

{
std::vector<winrt::hstring> args{ L"2" };
Remoting::CommandlineArgs eventArgs{ { args }, { L"" } };
Remoting::CommandlineArgs eventArgs{ { args }, { L"" }, SW_NORMAL };

auto result = m0->ProposeCommandline(eventArgs);
VERIFY_ARE_EQUAL(true, result.ShouldCreateWindow());
Expand All @@ -602,7 +602,7 @@ namespace RemotingUnitTests
}
{
std::vector<winrt::hstring> args{ L"10" };
Remoting::CommandlineArgs eventArgs{ { args }, { L"" } };
Remoting::CommandlineArgs eventArgs{ { args }, { L"" }, SW_NORMAL };

auto result = m0->ProposeCommandline(eventArgs);
VERIFY_ARE_EQUAL(true, result.ShouldCreateWindow());
Expand Down Expand Up @@ -648,15 +648,15 @@ namespace RemotingUnitTests
{
Log::Comment(L"Send a commandline to p2, who is still alive. We won't create a new window.");

Remoting::CommandlineArgs eventArgs{ { p2Args }, { L"" } };
Remoting::CommandlineArgs eventArgs{ { p2Args }, { L"" }, SW_NORMAL };

auto result = m0->ProposeCommandline(eventArgs);
VERIFY_ARE_EQUAL(false, result.ShouldCreateWindow());
VERIFY_ARE_EQUAL(false, (bool)result.Id());
}
{
Log::Comment(L"Send a commandline to p1, who is dead. We will create a new window.");
Remoting::CommandlineArgs eventArgs{ { p1Args }, { L"" } };
Remoting::CommandlineArgs eventArgs{ { p1Args }, { L"" }, SW_NORMAL };

auto result = m0->ProposeCommandline(eventArgs);
VERIFY_ARE_EQUAL(true, result.ShouldCreateWindow());
Expand Down Expand Up @@ -1359,7 +1359,7 @@ namespace RemotingUnitTests
std::vector<winrt::hstring> p2Args{ L"two", L"this is for p2" };

{
Remoting::CommandlineArgs eventArgs{ { p1Args }, { L"" } };
Remoting::CommandlineArgs eventArgs{ { p1Args }, { L"" }, SW_NORMAL };
auto result = m0->ProposeCommandline(eventArgs);
VERIFY_ARE_EQUAL(false, result.ShouldCreateWindow());
VERIFY_ARE_EQUAL(false, (bool)result.Id()); // Casting to (bool) checks if the reference has a value
Expand All @@ -1368,7 +1368,7 @@ namespace RemotingUnitTests

{
Log::Comment(L"Send a commandline to \"two\", which should be p2");
Remoting::CommandlineArgs eventArgs{ { p2Args }, { L"" } };
Remoting::CommandlineArgs eventArgs{ { p2Args }, { L"" }, SW_NORMAL };
auto result = m0->ProposeCommandline(eventArgs);
VERIFY_ARE_EQUAL(false, result.ShouldCreateWindow());
VERIFY_ARE_EQUAL(false, (bool)result.Id()); // Casting to (bool) checks if the reference has a value
Expand All @@ -1380,7 +1380,7 @@ namespace RemotingUnitTests

{
Log::Comment(L"Send a commandline to \"two\", who is now dead.");
Remoting::CommandlineArgs eventArgs{ { p2Args }, { L"" } };
Remoting::CommandlineArgs eventArgs{ { p2Args }, { L"" }, SW_NORMAL };
auto result = m0->ProposeCommandline(eventArgs);
VERIFY_ARE_EQUAL(true, result.ShouldCreateWindow());
VERIFY_ARE_EQUAL(false, (bool)result.Id()); // Casting to (bool) checks if the reference has a value
Expand Down Expand Up @@ -2392,7 +2392,7 @@ namespace RemotingUnitTests
VERIFY_ARE_EQUAL(p1->GetID(), m0->_mruPeasants[1].PeasantID());

std::vector<winrt::hstring> commandlineArgs{ L"0", L"arg[1]" };
Remoting::CommandlineArgs eventArgs{ { commandlineArgs }, { L"" } };
Remoting::CommandlineArgs eventArgs{ { commandlineArgs }, { L"" }, SW_NORMAL };

Log::Comment(L"When we attempt to send a commandline to the MRU window,"
L" we should find peasant 1 (who's name is \"one\"), not 2"
Expand Down Expand Up @@ -2577,15 +2577,15 @@ namespace RemotingUnitTests
auto m0 = make_private<Remoting::implementation::Monarch>(monarch0PID);

{
Remoting::CommandlineArgs args{ { L"wt.exe" }, { L"-Embedding" } };
Remoting::CommandlineArgs args{ { L"wt.exe" }, { L"-Embedding" }, SW_NORMAL };
const auto result = m0->ProposeCommandline(args);
auto shouldCreateWindow = result.ShouldCreateWindow();
VERIFY_IS_TRUE(shouldCreateWindow);
}

auto m1 = make_self<DeadMonarch>();
{
Remoting::CommandlineArgs args{ { L"wt.exe" }, { L"-Embedding" } };
Remoting::CommandlineArgs args{ { L"wt.exe" }, { L"-Embedding" }, SW_NORMAL };

try
{
Expand Down
9 changes: 8 additions & 1 deletion src/cascadia/WindowsTerminal/AppHost.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,8 @@ void AppHost::_HandleCommandlineArgs(const Remoting::WindowRequestedArgs& window
}
}

_launchShowWindowCommand = windowArgs.ShowWindowCommand();

// This is a fix for GH#12190 and hopefully GH#12169.
//
// If the commandline we were provided is going to result in us only
Expand Down Expand Up @@ -1247,7 +1249,12 @@ winrt::fire_and_forget AppHost::_WindowInitializedHandler(const winrt::Windows::
// match the initial settings, and then call ShowWindow to finally make us
// visible.

auto nCmdShow = SW_SHOW;
// Use the visibility that we were originally requested with as a base. We
// can't just use SW_SHOWDEFAULT, because that is set on a per-process
// basis. That means that a second window needs to have its STARTUPINFO's
// wShowCmd passed into the original process.
auto nCmdShow = _launchShowWindowCommand;

if (WI_IsFlagSet(_launchMode, LaunchMode::MaximizedMode))
{
nCmdShow = SW_MAXIMIZE;
Expand Down
2 changes: 2 additions & 0 deletions src/cascadia/WindowsTerminal/AppHost.h
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ class AppHost

std::shared_ptr<ThrottledFuncTrailing<bool>> _showHideWindowThrottler;

uint32_t _launchShowWindowCommand{ SW_NORMAL };

void _preInit();

void _HandleCommandlineArgs(const winrt::Microsoft::Terminal::Remoting::WindowRequestedArgs& args);
Expand Down
10 changes: 9 additions & 1 deletion src/cascadia/WindowsTerminal/WindowEmperor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,15 @@ bool WindowEmperor::HandleCommandlineArgs()
_buildArgsFromCommandline(args);
auto cwd{ wil::GetCurrentDirectoryW<std::wstring>() };

Remoting::CommandlineArgs eventArgs{ { args }, { cwd } };
// Get the requested initial state of the window from our startup info. For
// something like `start /min`, this will set the wShowWindow member to
// SW_SHOWMINIMIZED. We'll need to make sure is bubbled all the way through,
// so we can open a new window with the same state.
STARTUPINFOW si;
GetStartupInfoW(&si);
const auto showWindow = si.wShowWindow;

This comment has been minimized.

Copy link
@zadjii-msft

zadjii-msft Apr 20, 2023

Author Member

psychic debugging, re: that one internal thread - we need to only use this when specified in the dwFlags. Otherwise default to SW_SHOW.


Remoting::CommandlineArgs eventArgs{ { args }, { cwd }, showWindow };

const auto isolatedMode{ _app.Logic().IsolatedMode() };

Expand Down

0 comments on commit 2c16e7c

Please sign in to comment.