diff --git a/src/cascadia/Remoting/CommandlineArgs.cpp b/src/cascadia/Remoting/CommandlineArgs.cpp index db90f3381fd..4f200b7aa61 100644 --- a/src/cascadia/Remoting/CommandlineArgs.cpp +++ b/src/cascadia/Remoting/CommandlineArgs.cpp @@ -13,12 +13,12 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation // It must be defined after CommandlineArgs.g.cpp, otherwise the compiler // will give you just the most impossible template errors to try and // decipher. - void CommandlineArgs::Args(winrt::array_view const& value) + void CommandlineArgs::Commandline(winrt::array_view const& value) { _args = { value.begin(), value.end() }; } - winrt::com_array CommandlineArgs::Args() + winrt::com_array CommandlineArgs::Commandline() { return winrt::com_array{ _args.begin(), _args.end() }; } diff --git a/src/cascadia/Remoting/CommandlineArgs.h b/src/cascadia/Remoting/CommandlineArgs.h index 159071854f2..d76830f65d4 100644 --- a/src/cascadia/Remoting/CommandlineArgs.h +++ b/src/cascadia/Remoting/CommandlineArgs.h @@ -23,8 +23,8 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation winrt::hstring CurrentDirectory() { return _cwd; }; - void Args(winrt::array_view const& value); - winrt::com_array Args(); + void Commandline(winrt::array_view const& value); + winrt::com_array Commandline(); private: winrt::com_array _args; diff --git a/src/cascadia/Remoting/Monarch.cpp b/src/cascadia/Remoting/Monarch.cpp index 5d38fcfeb2e..08eb5913303 100644 --- a/src/cascadia/Remoting/Monarch.cpp +++ b/src/cascadia/Remoting/Monarch.cpp @@ -46,35 +46,48 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation // - the ID assigned to the peasant. uint64_t Monarch::AddPeasant(Remoting::IPeasant peasant) { - // TODO:projects/5 This is terrible. There's gotta be a better way - // of finding the first opening in a non-consecutive map of int->object - const auto providedID = peasant.GetID(); - - if (providedID == 0) - { - // Peasant doesn't currently have an ID. Assign it a new one. - peasant.AssignID(_nextPeasantID++); - } - else + try { - // Peasant already had an ID (from an older monarch). Leave that one - // be. Make sure that the next peasant's ID is higher than it. - _nextPeasantID = providedID >= _nextPeasantID ? providedID + 1 : _nextPeasantID; - } + // TODO:projects/5 This is terrible. There's gotta be a better way + // of finding the first opening in a non-consecutive map of int->object + const auto providedID = peasant.GetID(); - auto newPeasantsId = peasant.GetID(); - _peasants[newPeasantsId] = peasant; + if (providedID == 0) + { + // Peasant doesn't currently have an ID. Assign it a new one. + peasant.AssignID(_nextPeasantID++); + } + else + { + // Peasant already had an ID (from an older monarch). Leave that one + // be. Make sure that the next peasant's ID is higher than it. + _nextPeasantID = providedID >= _nextPeasantID ? providedID + 1 : _nextPeasantID; + } - // Add an event listener to the peasant's WindowActivated event. - peasant.WindowActivated({ this, &Monarch::_peasantWindowActivated }); + auto newPeasantsId = peasant.GetID(); + // Add an event listener to the peasant's WindowActivated event. + peasant.WindowActivated({ this, &Monarch::_peasantWindowActivated }); - TraceLoggingWrite(g_hRemotingProvider, - "Monarch_AddPeasant", - TraceLoggingUInt64(providedID, "providedID", "the provided ID for the peasant"), - TraceLoggingUInt64(newPeasantsId, "peasantID", "the ID of the new peasant"), - TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE)); + _peasants[newPeasantsId] = peasant; - return newPeasantsId; + TraceLoggingWrite(g_hRemotingProvider, + "Monarch_AddPeasant", + TraceLoggingUInt64(providedID, "providedID", "the provided ID for the peasant"), + TraceLoggingUInt64(newPeasantsId, "peasantID", "the ID of the new peasant"), + TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE)); + return newPeasantsId; + } + catch (...) + { + TraceLoggingWrite(g_hRemotingProvider, + "Monarch_AddPeasant_Failed", + TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE)); + + // We can only get into this try/catch if the peasant died on us. So + // the return value doesn't _really_ matter. They're not about to + // get it. + return -1; + } } // Method Description: @@ -104,6 +117,8 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation { auto peasantSearch = _peasants.find(peasantID); auto maybeThePeasant = peasantSearch == _peasants.end() ? nullptr : peasantSearch->second; + // Ask the peasant for their PID. This will validate that they're + // actually still alive. if (maybeThePeasant) { maybeThePeasant.GetPID(); @@ -148,23 +163,31 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation uint64_t Monarch::_getMostRecentPeasantID() { - if (_mostRecentPeasant == 0) + if (_mostRecentPeasant != 0) { - // We haven't yet been told the MRU peasant. Just use the first one. - // This is just gonna be a random one, but really shouldn't happen - // in practice. The WindowManager should set the MRU peasant - // immediately as soon as it creates the monarch/peasant for the - // first window. - if (_peasants.size() > 0) + return _mostRecentPeasant; + } + + // We haven't yet been told the MRU peasant. Just use the first one. + // This is just gonna be a random one, but really shouldn't happen + // in practice. The WindowManager should set the MRU peasant + // immediately as soon as it creates the monarch/peasant for the + // first window. + if (_peasants.size() > 0) + { + try { return _peasants.begin()->second.GetID(); } - return 0; - } - else - { - return _mostRecentPeasant; + catch (...) + { + // This shouldn't really happen. If we're the monarch, then the + // first peasant should also _be us_. So we should be able to + // get our own ID. + return 0; + } } + return 0; } // Method Description: @@ -187,6 +210,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation auto findWindowArgs = winrt::make_self(); findWindowArgs->Args(args); + // This is handled by some handler in-proc _FindTargetWindowRequestedHandlers(*this, *findWindowArgs); // After the event was handled, ResultTargetWindow() will be filled with @@ -205,20 +229,31 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation if (auto targetPeasant{ _getPeasant(windowID) }) { - // This will raise the peasant's ExecuteCommandlineRequested - // event, which will then ask the AppHost to handle the - // commandline, which will then pass it to AppLogic for - // handling. - targetPeasant.ExecuteCommandline(args); + auto result = winrt::make_self(); + + result->ShouldCreateWindow(false); + try + { + // This will raise the peasant's ExecuteCommandlineRequested + // event, which will then ask the AppHost to handle the + // commandline, which will then pass it to AppLogic for + // handling. + targetPeasant.ExecuteCommandline(args); + } + catch (...) + { + // If we fail to propose the commandline to the peasant (it + // died?) then just tell this process to become a new window + // instead. + result->ShouldCreateWindow(false); + } TraceLoggingWrite(g_hRemotingProvider, "Monarch_ProposeCommandline_Existing", - TraceLoggingUInt64(windowID, "peasantID", "the ID of the matching peasant"), + TraceLoggingUInt64(windowID, "peasantID", "the ID of the peasant the commandline waws intended for"), TraceLoggingBoolean(true, "foundMatch", "true if we found a peasant with that ID"), + TraceLoggingBoolean(!result->ShouldCreateWindow(), "succeeded", "true if we successfully dispatched the commandline to the peasant"), TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE)); - - auto result = winrt::make_self(); - result->ShouldCreateWindow(false); return *result; } else if (windowID > 0) @@ -229,7 +264,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation TraceLoggingWrite(g_hRemotingProvider, "Monarch_ProposeCommandline_Existing", - TraceLoggingUInt64(windowID, "peasantID", "the ID of the matching peasant"), + TraceLoggingUInt64(windowID, "peasantID", "the ID of the peasant the commandline waws intended for"), TraceLoggingBoolean(false, "foundMatch", "true if we found a peasant with that ID"), TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE)); diff --git a/src/cascadia/Remoting/Peasant.idl b/src/cascadia/Remoting/Peasant.idl index e3fb5af1b86..c77b5673e5c 100644 --- a/src/cascadia/Remoting/Peasant.idl +++ b/src/cascadia/Remoting/Peasant.idl @@ -9,7 +9,7 @@ namespace Microsoft.Terminal.Remoting CommandlineArgs(); CommandlineArgs(String[] args, String cwd); - String[] Args { get; set; }; + String[] Commandline { get; set; }; String CurrentDirectory(); }; diff --git a/src/cascadia/WindowsTerminal/AppHost.cpp b/src/cascadia/WindowsTerminal/AppHost.cpp index e2e7703de63..6b678cae94c 100644 --- a/src/cascadia/WindowsTerminal/AppHost.cpp +++ b/src/cascadia/WindowsTerminal/AppHost.cpp @@ -168,7 +168,7 @@ void AppHost::_HandleCommandlineArgs() { if (auto args{ peasant.InitialArgs() }) { - const auto result = _logic.SetStartupCommandline(args.Args()); + const auto result = _logic.SetStartupCommandline(args.Commandline()); const auto message = _logic.ParseCommandlineMessage(); if (!message.empty()) { @@ -530,7 +530,7 @@ bool AppHost::HasWindow() void AppHost::_DispatchCommandline(winrt::Windows::Foundation::IInspectable /*sender*/, Remoting::CommandlineArgs args) { - _logic.ExecuteCommandline(args.Args(), args.CurrentDirectory()); + _logic.ExecuteCommandline(args.Commandline(), args.CurrentDirectory()); } // Method Description: @@ -546,7 +546,7 @@ void AppHost::_DispatchCommandline(winrt::Windows::Foundation::IInspectable /*se void AppHost::_FindTargetWindow(const winrt::Windows::Foundation::IInspectable& /*sender*/, const Remoting::FindTargetWindowArgs& args) { - const auto targetWindow = _logic.FindTargetWindow(args.Args().Args()); + const auto targetWindow = _logic.FindTargetWindow(args.Args().Commandline()); args.ResultTargetWindow(targetWindow); }