Final cleanup for review

src/cascadia/Remoting/Monarch.cpp

@@ -16,6 +16,9 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
         _ourPID{ GetCurrentProcessId() }
     {
     }
+
+    // This is a private constructor to be used in unit tests, where we don't
+    // want each Monarch to necessarily use the current PID.
     Monarch::Monarch(const uint64_t testPID) :
         _ourPID{ testPID }
     {
@@ -30,6 +33,12 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
         return _ourPID;
     }
 
+    // Method Description:
+    // - Add the given peasant to the list of peasants we're tracking. This Peasant may have already been assigned an ID. If it hasn't, then give it an ID.
+    // Arguments:
+    // - peasant: the new Peasant to track.
+    // Return Value:
+    // - 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
@@ -55,11 +64,20 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
         peasant.WindowActivated({ this, &Monarch::_peasantWindowActivated });
 
         // TODO:projects/5 Wait on the peasant's PID, and remove them from the
-        // map if they die.
+        // map if they die. This won't work great in tests though, with fake
+        // PIDs.
 
         return newPeasantsId;
     }
 
+    // Method Description:
+    // - Event handler for the Peasant::WindowActivated event. Used as an
+    //   opportunity for us to update our internal stack of the "most recent
+    //   window".
+    // Arguments:
+    // - sender: the Peasant that raised this event. This might be out-of-proc!
+    // Return Value:
+    // - <none>
     void Monarch::_peasantWindowActivated(const winrt::Windows::Foundation::IInspectable& sender,
                                           const winrt::Windows::Foundation::IInspectable& /*args*/)
     {
@@ -94,8 +112,15 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
         _mostRecentPeasant = peasantID;
     }
 
-    bool Monarch::ProposeCommandline(array_view<const winrt::hstring> /*args*/,
-                                     winrt::hstring /*cwd*/)
+    // Method Description:
+    // - Try to handle a commandline from a new WT invocation. We might need to
+    //   hand the commandline to an existing window, or we might need to tell
+    //   the caller that they need to become a new window to handle it themself.
+    // Arguments:
+    // - <none>
+    // Return Value:
+    // - <none>
+    bool Monarch::ProposeCommandline(const Remoting::CommandlineArgs& /*args*/)
     {
         // TODO:projects/5
         // The branch dev/migrie/f/remote-commandlines has a more complete

src/cascadia/Remoting/Monarch.h

@@ -34,7 +34,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
 
         uint64_t AddPeasant(winrt::Microsoft::Terminal::Remoting::IPeasant peasant);
 
-        bool ProposeCommandline(array_view<const winrt::hstring> args, winrt::hstring cwd);
+        bool ProposeCommandline(const winrt::Microsoft::Terminal::Remoting::CommandlineArgs& args);
 
     private:
         Monarch(const uint64_t testPID);

src/cascadia/Remoting/Monarch.idl

@@ -7,6 +7,6 @@ namespace Microsoft.Terminal.Remoting
 
         UInt64 GetPID();
         UInt64 AddPeasant(IPeasant peasant);
-        Boolean ProposeCommandline(String[] args, String cwd);
+        Boolean ProposeCommandline(CommandlineArgs args);
     };
 }

src/cascadia/Remoting/Peasant.cpp

@@ -15,6 +15,9 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
         _ourPID{ GetCurrentProcessId() }
     {
     }
+
+    // This is a private constructor to be used in unit tests, where we don't
+    // want each Peasant to necessarily use the current PID.
     Peasant::Peasant(const uint64_t testPID) :
         _ourPID{ testPID }
     {
@@ -36,11 +39,17 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
 
     bool Peasant::ExecuteCommandline(const Remoting::CommandlineArgs& args)
     {
+        // If this is the first set of args we were ever told about, stash them
+        // away. We'll need to get at them later, when we setup the startup
+        // actions for the window.
         if (_initialArgs == nullptr)
         {
             _initialArgs = args;
         }
 
+        // Raise an event with these args. The AppHost will listen for this
+        // event to know when to take these args and dispatch them to a
+        // currently-running window.
         _ExecuteCommandlineRequestedHandlers(*this, args);
 
         return true;

src/cascadia/Remoting/WindowManager.cpp

@@ -31,25 +31,23 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
         _registrationHostClass = 0;
     }
 
-    void WindowManager::ProposeCommandline(array_view<const winrt::hstring> args,
-                                           const winrt::hstring cwd)
+    void WindowManager::ProposeCommandline(const Remoting::CommandlineArgs& args)
     {
         const bool isKing = _areWeTheKing();
         // If we're the king, we _definitely_ want to process the arguments, we were
         // launched with them!
         //
         // Otherwise, the King will tell us if we should make a new window
         _shouldCreateWindow = isKing ||
-                              _monarch.ProposeCommandline(args, cwd);
+                              _monarch.ProposeCommandline(args);
 
         if (_shouldCreateWindow)
         {
             // If we should create a new window, then instantiate our Peasant
             // instance, and tell that peasant to handle that commandline.
             _createOurPeasant();
 
-            auto eventArgs = winrt::make_self<implementation::CommandlineArgs>(args, cwd);
-            _peasant.ExecuteCommandline(*eventArgs);
+            _peasant.ExecuteCommandline(args);
         }
         // Othersize, we'll do _nothing_.
     }

src/cascadia/Remoting/WindowManager.h

@@ -12,7 +12,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
         WindowManager();
         ~WindowManager();
 
-        void ProposeCommandline(array_view<const winrt::hstring> args, const winrt::hstring cwd);
+        void ProposeCommandline(const winrt::Microsoft::Terminal::Remoting::CommandlineArgs& args);
         bool ShouldCreateWindow();
 
         winrt::Microsoft::Terminal::Remoting::Peasant CurrentWindow();

src/cascadia/Remoting/WindowManager.idl

@@ -3,12 +3,11 @@ import "Peasant.idl";
 
 namespace Microsoft.Terminal.Remoting
 {
-
     [default_interface] runtimeclass WindowManager
     {
         WindowManager();
-        void ProposeCommandline(String[] commands, String cwd);
+        void ProposeCommandline(CommandlineArgs args);
         Boolean ShouldCreateWindow { get; };
-        Peasant CurrentWindow();
+        IPeasant CurrentWindow();
     };
 }

src/cascadia/UnitTests_Remoting/RemotingTests.cpp

@@ -12,17 +12,23 @@ using namespace WEX::Common;
 using namespace winrt;
 using namespace winrt::Microsoft::Terminal;
 
+// These are some gross macros that let us call a private ctor for
+// Monarch/Peasant. We can't just use make_self, because that doesn't let us
+// call a private ctor. We can use com_ptr::attach, but since we're allocating
+// the thing on the stack, we need to make sure to call detach before the object
+// is destructed.
+
 #define MAKE_MONARCH(name, pid)                               \
     Remoting::implementation::Monarch _local_##name##{ pid }; \
     com_ptr<Remoting::implementation::Monarch> name;          \
     name.attach(&_local_##name##);                            \
-    auto cleanup = wil::scope_exit([&]() { name.detach(); });
+    auto cleanup_##name## = wil::scope_exit([&]() { name.detach(); });
 
 #define MAKE_PEASANT(name, pid)                               \
     Remoting::implementation::Peasant _local_##name##{ pid }; \
     com_ptr<Remoting::implementation::Peasant> name;          \
     name.attach(&_local_##name##);                            \
-    auto cleanup = wil::scope_exit([&]() { name.detach(); });
+    auto cleanup_##name## = wil::scope_exit([&]() { name.detach(); });
 
 namespace RemotingUnitTests
 {

src/cascadia/WindowsTerminal/AppHost.cpp

@@ -14,6 +14,7 @@ using namespace winrt::Windows::UI::Composition;
 using namespace winrt::Windows::UI::Xaml;
 using namespace winrt::Windows::UI::Xaml::Hosting;
 using namespace winrt::Windows::Foundation::Numerics;
+using namespace winrt::Microsoft::Terminal;
 using namespace winrt::Microsoft::Terminal::Settings::Model;
 using namespace ::Microsoft::Console;
 using namespace ::Microsoft::Console::Types;
@@ -147,7 +148,9 @@ void AppHost::_HandleCommandlineArgs()
     std::vector<winrt::hstring> args;
     _buildArgsFromCommandline(args);
     std::wstring cwd{ wil::GetCurrentDirectoryW<std::wstring>() };
-    _windowManager.ProposeCommandline({ args }, { cwd });
+
+    Remoting::CommandlineArgs eventArgs{ { args }, { cwd } };
+    _windowManager.ProposeCommandline(eventArgs);
 
     _shouldCreateWindow = _windowManager.ShouldCreateWindow();
     if (!_shouldCreateWindow)