notes, comments, cleanup

src/cascadia/Remoting/Monarch.cpp

@@ -139,7 +139,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
         if (_mostRecentPeasant == 0)
         {
             // We haven't yet been told the MRU peasant. Just use the first one.
-            // TODO: GOD this is just gonna be a random one. Hacks on hacks on hacks
+            // TODO:MG GOD this is just gonna be a random one. Hacks on hacks on hacks
             if (_peasants.size() > 0)
             {
                 return _peasants.begin()->second.GetID();
@@ -164,18 +164,21 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
     //   to another window in this case.
     bool Monarch::ProposeCommandline(const Remoting::CommandlineArgs& args)
     {
-        // TODO:projects/5
-        // The branch dev/migrie/f/remote-commandlines has a more complete
-        // version of this function, with a naive implementation. For now, we
-        // always want to create a new window, so we'll just return true. This
-        // will tell the caller that we didn't handle the commandline, and they
-        // should open a new window to deal with it themselves.
+        // Raise an event, to ask how to handle this commandline. We can't ask
+        // the app ourselves - we exist isolated from that knowledge (and
+        // dependency hell). The WindowManager will raise this up to the app
+        // host, which will then ask the AppLogic, who will then parse the
+        // commandline and determine the provided ID of the window.
         auto findWindowArgs = winrt::make_self<Remoting::implementation::FindTargetWindowArgs>();
         findWindowArgs->Args(args);
+
         _FindTargetWindowRequestedHandlers(*this, *findWindowArgs);
+
+        // After the event was handled, ResultTargetWindow() will be filled with
+        // the parsed result.
         const auto targetWindow = findWindowArgs->ResultTargetWindow();
 
-        // TODO:projects/5 targetWindow==0 -> use the currently active window
+        // If there's a valid ID returned, then let's try and find the peasant that goes with it.
         if (targetWindow >= 0)
         {
             uint64_t windowID = ::base::saturated_cast<uint64_t>(targetWindow);
@@ -188,12 +191,24 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
             if (auto targetPeasant{ _getPeasant(windowID) })
             {
                 targetPeasant.ExecuteCommandline(args);
+                // TODO:MG if the targeted peasant fails to execute the
+                // commandline, we should create our own window to display the
+                // message box.
                 return false;
             }
+            else
+            {
+                // TODO:MG in this case, an ID was provided, but there's no
+                // peasant with that ID. Instead, we should tell the caller that
+                // they should make a new window, but _with that ID_.
+                //
+                // `Monarch::ProposeCommandline` needs to return a structure of
+                // `{ shouldCreateWindow: bool, givenID: optional<uint> }`
+                //
+            }
         }
-        // TEMPORARY: if the target window is -1, then we want a new window. All
-        // other cases, just do it in this window (for now).
-        // return targetWindow == -1;
+
+        // TODO:MG in this case, no usable ID was provided. Return { true, nullopt }
         return true;
     }
 

src/cascadia/Remoting/WindowManager.cpp

@@ -52,13 +52,22 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
         _shouldCreateWindow = isKing ||
                               _monarch.ProposeCommandline(args);
 
+        // TODO:projects/5 The monarch may respond back "you should be a new
+        // window, with ID,name of (id, name)". Really the responses are:
+        // * You should not create a new window
+        // * Create a new window (but without a given ID or name). The Monarch
+        //   will assign your ID/name later
+        // * Create a new window, and you'll have this ID or name
+        //   - This is the case where the user provides `wt -w 1`, and there's
+        //     no existing window 1
+
         if (_shouldCreateWindow)
         {
             // If we should create a new window, then instantiate our Peasant
             // instance, and tell that peasant to handle that commandline.
             _createOurPeasant();
 
-            // TODO:MG Spawn a thread to wait on the monarch, and handle the election
+            // Spawn a thread to wait on the monarch, and handle the election
             if (!isKing)
             {
                 _createPeasantThread();
@@ -150,9 +159,10 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
 
         if (_areWeTheKing())
         {
-            // This is only called when a _new_ monarch is elected. We need to
-            // do this _always_, even on the first instance, which won't have an
-            // election
+            // This is only called when a _new_ monarch is elected. So don't do
+            // anything here that needs to be done for all monarch windows. This
+            // should only be for work that's done when a window _becomes_ a
+            // monarch, after the death of the previous monarch.
             return true;
         }
         return false;

src/cascadia/TerminalApp/AppLogic.cpp

@@ -1125,25 +1125,45 @@ namespace winrt::TerminalApp::implementation
         return result; // TODO:MG does a return value make sense
     }
 
+    // Method Description:
+    // - Parse the given commandline args in an attempt to find the specified
+    //   window. The rest of the args are ignored for now (they'll be handled
+    //   whenever the commandline fets to the window it was intended for).
+    // - Note that this function will only ever be called by the monarch. A
+    //   return value of `0` in this case does not mean "run the commandline in
+    //   _this_ process", rather it means "run the commandline in the current
+    //   process", whoever that may be.
+    // Arguments:
+    // - args: an array of strings to process as a commandline. These args can contain spaces
+    // Return Value:
+    // - 0: We should handle the args "in the current window".
+    // - -1: We should handle the args in a new window
+    // - anything else: We should handle the commandline in the window with the given ID.
     int32_t AppLogic::FindTargetWindow(array_view<const winrt::hstring> args)
     {
         ::TerminalApp::AppCommandlineArgs appArgs;
         auto result = appArgs.ParseArgs(args);
         if (result == 0)
         {
-            // TODO:MG Right now, any successful parse will end up in the same window
-            // return 0;
             return appArgs.GetTargetWindow();
 
-            // TODO:projects/5 We'll want to use the windowingBehavior setting to determine
-            // well
-            // Maybe that'd be a special return value out of here, to tell the monarch to do something special
+            // TODO:projects/5
+            //
+            // In the future, we'll want to use the windowingBehavior setting to
+            // determine what happens when a window ID wasn't manually provided.
+            //
+            // Maybe that'd be a special return value out of here, to tell the
+            // monarch to do something special:
+            //
             // -1 -> create a new window
             // -2 -> find the mru, this desktop
-            // -3 -> MRU, any desktop
+            // -3 -> MRU, any desktop (is this not just 0?)
         }
 
-        // Any unsuccessful parse will be a new window.
+        // Any unsuccessful parse will be a new window. That new window will try
+        // to handle the commandline iteslf, and find that the commandline
+        // failed to parse. When that happens, the new window will display the
+        // message box.
         return -1;
     }