cleanup and comments before the PR

src/cascadia/Remoting/Monarch.cpp

@@ -1098,7 +1098,11 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
                               TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE),
                               TraceLoggingKeyword(TIL_KEYWORD_TRACE));
 
-            // TODO! create a window for that name / ID?
+            // TODO GH#5000
+            //
+            // In the case where window couldn't be found, then create a window
+            // for that name / ID. Do this as a part of tear-out (different than
+            // drag/drop)
         }
     }
 }

src/cascadia/TerminalApp/Pane.cpp

@@ -175,23 +175,27 @@ NewTerminalArgs Pane::GetTerminalArgsForPane(const bool asContent) const
 // Arguments:
 // - currentId: the id to use for the current/first pane
 // - nextId: the id to use for a new pane if we split
+// - asContent: We're serializing this set of actions as content actions for
+//   moving to other windows, so we need to make sure to include ContentGuid's
+//   in the final actions.
+// - asMovePane: only used with asContent. When this is true, we're building
+//   these actions as a part of moving the pane to another window, but without
+//   the context of the hosting tab. In that case, we'll want to build a
+//   splitPane action even if we're just a single leaf, because there's no other
+//   parent to try and build an action for us.
 // Return Value:
 // - The state from building the startup actions, includes a vector of commands,
 //   the original root pane, the id of the focused pane, and the number of panes
 //   created.
-Pane::BuildStartupState Pane::BuildStartupActions(uint32_t currentId, uint32_t nextId, const bool asContent, const bool asMovePane)
-{
-    // TODO! I f'ed something up here recently. Just moving a single pane no longer seems to work.
-    //
-    // If we call this directly on a leaf, then CURRENTLY, we'll return {} for
-    // the actions. Moving that will just move nothing, cause obviously.
-    //
-    // But this all works fine for a _tab_, where there's a parent Tab to build
-    // a newTab action first.
-    //
-    // So if we're moving a pane, as content, then we need to special case here where even moving just us returns a splitPane
-
-    // if we are a leaf then all there is to do is defer to the parent.
+Pane::BuildStartupState Pane::BuildStartupActions(uint32_t currentId,
+                                                  uint32_t nextId,
+                                                  const bool asContent,
+                                                  const bool asMovePane)
+{
+    // Normally, if we're a leaf, return an empt set of actions, because the
+    // parent pane will build the SplitPane action for us. If we're building
+    // actions for a movePane action though, we'll still need to include
+    // ourselves.
     if (!asMovePane && _IsLeaf())
     {
         if (_lastActive)

src/cascadia/TerminalApp/TerminalPage.cpp

@@ -1889,6 +1889,8 @@ namespace winrt::TerminalApp::implementation
             return false;
         }
 
+        // If there was a windowId in the action, try to move it to the
+        // specified window instead of moving it in our tab row.
         if (!windowId.empty())
         {
             if (const auto terminalTab{ _GetFocusedTabImpl() })
@@ -1934,6 +1936,8 @@ namespace winrt::TerminalApp::implementation
         return true;
     }
 
+    // Detach a tree of panes from this terminal. Helper used for moving panes
+    // and tabs to other windows.
     void TerminalPage::_DetachPaneFromWindow(std::shared_ptr<Pane> pane)
     {
         pane->WalkTree([&](auto p) {
@@ -1946,13 +1950,18 @@ namespace winrt::TerminalApp::implementation
 
     void TerminalPage::_DetachTabFromWindow(const winrt::com_ptr<TerminalTab>& terminalTab)
     {
-        // Collect all the content we're about to detach.
+        // Detach the root pane, which will act like the whole tab got detached.
         if (const auto rootPane = terminalTab->GetRootPane())
         {
             _DetachPaneFromWindow(rootPane);
         }
     }
 
+    // Method Description:
+    // - Serialize these actions to json, and raise them as a RequestMoveContent
+    //   event. Our Window will raise that to the window manager / monarch, who
+    //   will dispatch this blob of json back to the window that should handle
+    //   this.
     void TerminalPage::_MoveContent(std::vector<Settings::Model::ActionAndArgs>& actions,
                                     const winrt::hstring& windowName,
                                     const uint32_t tabIndex)
@@ -1967,6 +1976,8 @@ namespace winrt::TerminalApp::implementation
 
     bool TerminalPage::_MoveTab(MoveTabArgs args)
     {
+        // If there was a windowId in the action, try to move it to the
+        // specified window instead of moving it in our tab row.
         const auto windowId{ args.Window() };
         if (!windowId.empty())
         {
@@ -1994,7 +2005,18 @@ namespace winrt::TerminalApp::implementation
         return true;
     }
 
-    winrt::fire_and_forget TerminalPage::AttachContent(winrt::hstring content, uint32_t tabIndex)
+    // Method Description:
+    // - Called when it is determined that an existing tab or pane should be
+    //   attached to our window. content represents a blob of JSON describing
+    //   some startup actions for resbuilding the specified panes. They will
+    //   include `__content` properties with the GUID of the existing
+    //   ControlInteractivities we should use, rather than starting new ones.
+    // - _MakePane is already enlightened to use the ContentGuid property to
+    //   reattach instead of create new content, so this method simply needs to
+    //   parse the JSON and pump it into our action handler. Almost the same as
+    //   doing something like `wt -w 0 nt`.
+    winrt::fire_and_forget TerminalPage::AttachContent(winrt::hstring content,
+                                                       uint32_t tabIndex)
     {
         auto args = ActionAndArgs::Deserialize(content);
 

src/cascadia/TerminalSettingsModel/ActionArgs.h

@@ -332,10 +332,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
             JsonUtils::GetValueForKey(json, SuppressApplicationTitleKey, args->_SuppressApplicationTitle);
             JsonUtils::GetValueForKey(json, ColorSchemeKey, args->_ColorScheme);
             JsonUtils::GetValueForKey(json, ElevateKey, args->_Elevate);
-
-            // TODO! We should probably have the ContentGuid be optional, or at
-            // least serialized smarter, so we definitely don't write it to the
-            // file, ever.
             JsonUtils::GetValueForKey(json, ContentKey, args->_ContentGuid);
             return *args;
         }