Code cleanup

microsoft · Jan 7, 2021 · bc492f1 · bc492f1
1 parent 52b2cb6
commit bc492f1
Show file tree

Hide file tree

Showing 2 changed files with 23 additions and 19 deletions.
diff --git a/src/cascadia/Remoting/Monarch.cpp b/src/cascadia/Remoting/Monarch.cpp
@@ -205,10 +205,11 @@ 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);
-                // TODO:MG if the targeted peasant fails to execute the
-                // commandline, we should create our own window to display the
-                // message box.
 
                 TraceLoggingWrite(g_hRemotingProvider,
                                   "Monarch_ProposeCommandline_Existing",
@@ -222,18 +223,16 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
             }
             else if (windowID > 0)
             {
-                // TODO:MG in this case, an ID was provided, but there's no
+                // 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> }`
-                //
+
                 TraceLoggingWrite(g_hRemotingProvider,
                                   "Monarch_ProposeCommandline_Existing",
                                   TraceLoggingUInt64(windowID, "peasantID", "the ID of the matching peasant"),
                                   TraceLoggingBoolean(false, "foundMatch", "true if we found a peasant with that ID"),
                                   TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE));
+
                 auto result = winrt::make_self<Remoting::implementation::ProposeCommandlineResult>();
                 result->ShouldCreateWindow(true);
                 result->Id(windowID);
@@ -245,8 +244,8 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
                           "Monarch_ProposeCommandline_NewWindow",
                           TraceLoggingInt64(targetWindow, "targetWindow", "The provided ID"),
                           TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE));
-        // TODO:MG in this case, no usable ID was provided. Return { true, nullopt }
-        // return true;
+
+        // In this case, no usable ID was provided. Return { true, nullopt }
         auto result = winrt::make_self<Remoting::implementation::ProposeCommandlineResult>();
         result->ShouldCreateWindow(true);
         return *result;

diff --git a/src/cascadia/Remoting/WindowManager.cpp b/src/cascadia/Remoting/WindowManager.cpp
@@ -54,12 +54,24 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
 
         if (!isKing)
         {
+            // 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
+
             auto result = _monarch.ProposeCommandline(args);
             _shouldCreateWindow = result.ShouldCreateWindow();
             if (result.Id())
             {
                 givenID = result.Id().Value();
             }
+
+            // TraceLogging doesn't have a good solution for logging an
+            // optional. So we have to repeat the calls here:
             if (givenID)
             {
                 TraceLoggingWrite(g_hRemotingProvider,
@@ -79,21 +91,14 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
         }
         else
         {
+            // We're the monarch, we don't need to propose anything. We're just
+            // going to do it.
             TraceLoggingWrite(g_hRemotingProvider,
                               "WindowManager_ProposeCommandline_AsMonarch",
                               TraceLoggingBoolean(_shouldCreateWindow, "CreateWindow", "true iff we should create a new window"),
                               TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE));
         }
 
-        // 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