Fix exiting a zoomed pane

[zadjii-msft]

Summary of the Pull Request

Fixes the bug where exiting inside a closed pane would leave the Terminal blank.

Additionally, removes Tab::GetRootElement and replaces it with the observable Tab::Content. This should be more resilient in the future.

Also adds some tests, though admittedly not for this exact scenario. This scenario requires a cooperating TerminalConnection that I can drive for the sake of testing, and ain't nobody got time for that.

References

• Introduced in Add a togglePaneZoom action for zooming a pane #6989

PR Checklist

• Closes Certain pane zoom operations make the terminal go blank #7252
• I work here
• Tests added/passed 🎉
• [n/a] Requires documentation to be updated

Detailed Description of the Pull Request / Additional comments

From notes I had left in Tab.cpp while I was working on this:

OKAY I see what's happening here the ActivePaneChanged Handler in TerminalPage
doesn't re-attach the tab content to the tree, it just updates the title of the
window.

So when the pane is `exit`ed, the pane's control is removed and re-attached to
the parent grid, which _isn't in the XAML tree_. And no one can go tell the
TerminalPage that it needs to re set up the tab content again.

The Page _manually_ does this in a few places, when various pane actions are
about to take place, it'll unzoom. It would be way easier if the Tab could just
manage the content of the page.

Or if the Tab just had a Content that was observable, that when that changed,
the page would auto readjust. That does sound like a LOT of work though.

Validation Steps Performed

Opened panes, closed panes, exited panes, zoomed panes, moved focus between panes, panes, panes, panes

[DHowett]

/cc @leonMSFT this will conflict with you i bet

[leonMSFT]

it probably will, but at least I named the variable Content as well 😄, i think the merge conflict won't be too bad for this property, it'll probably be bad in terms of me literally deleting Tab.cpp and replacing it with TerminalTab.cpp. though if you merge this PR first I'd be willing to resolve those conflicts 👍

[DHowett]

Love it!

[ghost]

Hello @zadjii-msft!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

[DHowett]

@leonMSFT I might have a conflict resolution prepared for this, as I wanted to produce a selfhost build.

If you look over this and deem it correct, I can push it into f/sui.

There's no way to get github to show the recorded resolution for merge conflicts, so I had to paste in a resolution patch. It shows two columns of + and -s instead of the normal one.

commit b1c44cc308c7773837185942c8ecf815aaf3cb41
Merge: aab6b0500 403b79317
Author: Dustin L. Howett <duhowett@microsoft.com>
Date:   Wed Oct 21 19:15:52 2020 -0700

    Merge remote-tracking branch 'origin/main' into dev/duhowett/selfhost-1.5

diff --cc src/cascadia/TerminalApp/AppActionHandlers.cpp
index 38b799aa4,fc5dd1582..66eef32f5
--- a/src/cascadia/TerminalApp/AppActionHandlers.cpp
+++ b/src/cascadia/TerminalApp/AppActionHandlers.cpp
@@@ -135,14 -133,10 +135,13 @@@ namespace winrt::TerminalApp::implement
                      // be removed before it's re-added in Pane::Restore
                      _tabContent.Children().Clear();
  
+                     // Togging the zoom on the tab will cause the tab to inform us of
+                     // the new root Content for this tab.
                      activeTab->ToggleZoom();
- 
-                     // Update the selected tab, to trigger us to re-add the tab's tab content to the UI tree
-                     _UpdatedSelectedTab(_tabView.SelectedIndex());
                  }
 +            }
 +        }
 +
          args.Handled(true);
      }
  
diff --cc src/cascadia/TerminalApp/Tab.idl
index db9746c99,7e88c5878..4e2bd8bdb
--- a/src/cascadia/TerminalApp/Tab.idl
+++ b/src/cascadia/TerminalApp/Tab.idl
@@@ -11,6 -11,8 +11,8 @@@ namespace TerminalAp
          Microsoft.Terminal.Settings.Model.Command SwitchToTabCommand { get; };
          UInt32 TabViewIndex { get; };
  
 -        Windows.UI.Xaml.UIElement Content { get; };
++        Windows.UI.Xaml.FrameworkElement Content { get; };
+ 
          void SetDispatch(ShortcutActionDispatch dispatch);
      }
  }
diff --cc src/cascadia/TerminalApp/TerminalPage.cpp
index c331bc083,2b9be96ef..ea25b0106
--- a/src/cascadia/TerminalApp/TerminalPage.cpp
+++ b/src/cascadia/TerminalApp/TerminalPage.cpp
@@@ -1136,6 -1142,16 +1136,16 @@@ namespace winrt::TerminalApp::implement
                  {
                      page->_UpdateTitle(*tab);
                  }
+                 else if (args.PropertyName() == L"Content")
+                 {
 -                    if (tab == page->_GetFocusedTab())
++                    if (*tab == page->_GetFocusedTab())
+                     {
+                         page->_tabContent.Children().Clear();
+                         page->_tabContent.Children().Append(tab->Content());
+ 
 -                        tab->SetFocused(true);
++                        tab->Focus(FocusState::Programmatic);
+                     }
+                 }
              }
          });
  
@@@ -1256,13 -1269,12 +1266,14 @@@
                      // Remove the content from the tab first, so Pane::UnZoom can
                      // re-attach the content to the tree w/in the pane
                      _tabContent.Children().Clear();
+                     // In ExitZoom, we'll change the Tab's Content(), triggering the
+                     // content changed event, which will re-attach the tab's new content
+                     // root to the tree.
                      activeTab->ExitZoom();
-                     // Re-attach the tab's content to the UI tree.
-                     _tabContent.Children().Append(activeTab->Content());
                  }
              }
 +        }
 +    }
  
      // Method Description:
      // - Attempt to move focus between panes, as to focus the child on
diff --cc src/cascadia/TerminalApp/TerminalTab.cpp
index 968865976,2f9cc7058..c43a19827
--- a/src/cascadia/TerminalApp/TerminalTab.cpp
+++ b/src/cascadia/TerminalApp/TerminalTab.cpp
@@@ -33,8 -33,10 +33,9 @@@ namespace winrt::TerminalApp::implement
          });
  
          _activePane = _rootPane;
+         Content(_rootPane->GetRootElement());
  
          _MakeTabViewItem();
 -        _MakeSwitchToTabCommand();
          _CreateContextMenu();
      }
  
@@@ -1045,16 -1069,18 +1044,18 @@@
          _rootPane->Maximize(_zoomedPane);
          // Update the tab header to show the magnifying glass
          _UpdateTabHeader();
+         Content(_zoomedPane->GetRootElement());
      }
 -    void Tab::ExitZoom()
 +    void TerminalTab::ExitZoom()
      {
          _rootPane->Restore(_zoomedPane);
          _zoomedPane = nullptr;
          // Update the tab header to hide the magnifying glass
          _UpdateTabHeader();
+         Content(_rootPane->GetRootElement());
      }
  
 -    bool Tab::IsZoomed()
 +    bool TerminalTab::IsZoomed()
      {
          return _zoomedPane != nullptr;
      }
diff --cc src/cascadia/TerminalApp/TerminalTab.h
index f29e80a9d,adfa4307b..05b584dd1
--- a/src/cascadia/TerminalApp/TerminalTab.h
+++ b/src/cascadia/TerminalApp/TerminalTab.h
@@@ -85,6 -87,8 +85,8 @@@ namespace winrt::TerminalApp::implement
          // The TabViewNumTabs is the number of Tab objects in TerminalPage's _tabs vector.
          OBSERVABLE_GETSET_PROPERTY(uint32_t, TabViewNumTabs, _PropertyChangedHandlers, 0);
  
 -        OBSERVABLE_GETSET_PROPERTY(winrt::Windows::UI::Xaml::UIElement, Content, _PropertyChangedHandlers, nullptr);
++        OBSERVABLE_GETSET_PROPERTY(winrt::Windows::UI::Xaml::FrameworkElement, Content, _PropertyChangedHandlers, nullptr);
+ 
      private:
          std::shared_ptr<Pane> _rootPane{ nullptr };
          std::shared_ptr<Pane> _activePane{ nullptr };

[leonMSFT]

@DHowett looks good to me! 👍

[DHowett]

Done!

[ghost]

🎉Windows Terminal v1.4.3141.0 has been released which incorporates this pull request.:tada:

Handy links:

• Release Notes
• Store Download

[ghost]

🎉Windows Terminal Preview v1.5.3142.0 has been released which incorporates this pull request.:tada:

Handy links:

• Release Notes
• Store Download