From ccf9f03ed3c495871015b99e19a6b54641e868c9 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 21 Oct 2020 16:33:56 -0500 Subject: [PATCH] Fix `exit`ing a zoomed pane (#7973) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary of the Pull Request Fixes the bug where `exit`ing 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 #6989 ## PR Checklist * [x] Closes #7252 * [x] I work here * [x] 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 --- .../LocalTests_TerminalApp/TabTests.cpp | 193 ++++++++++++++++++ .../TerminalApp/AppActionHandlers.cpp | 5 +- src/cascadia/TerminalApp/Pane.cpp | 14 +- src/cascadia/TerminalApp/Tab.cpp | 37 ++-- src/cascadia/TerminalApp/Tab.h | 3 +- src/cascadia/TerminalApp/Tab.idl | 2 + src/cascadia/TerminalApp/TerminalPage.cpp | 17 +- .../TerminalSettingsModel/ActionArgs.h | 6 + .../TerminalSettingsModel/ActionArgs.idl | 5 +- 9 files changed, 255 insertions(+), 27 deletions(-) diff --git a/src/cascadia/LocalTests_TerminalApp/TabTests.cpp b/src/cascadia/LocalTests_TerminalApp/TabTests.cpp index e623c9cf055..d32a41aee94 100644 --- a/src/cascadia/LocalTests_TerminalApp/TabTests.cpp +++ b/src/cascadia/LocalTests_TerminalApp/TabTests.cpp @@ -62,6 +62,10 @@ namespace TerminalAppLocalTests TEST_METHOD(TryDuplicateBadTab); TEST_METHOD(TryDuplicateBadPane); + TEST_METHOD(TryZoomPane); + TEST_METHOD(MoveFocusFromZoomedPane); + TEST_METHOD(CloseZoomedPane); + TEST_CLASS_SETUP(ClassSetup) { return true; @@ -75,6 +79,7 @@ namespace TerminalAppLocalTests private: void _initializeTerminalPage(winrt::com_ptr& page, CascadiaSettings initialSettings); + winrt::com_ptr _commonSetup(); }; void TabTests::EnsureTestsActivate() @@ -496,4 +501,192 @@ namespace TerminalAppLocalTests }); } + // Method Description: + // - This is a helper method for setting up a TerminalPage with some common + // settings, and creating the first tab. + // Arguments: + // - + // Return Value: + // - The initialized TerminalPage, ready to use. + winrt::com_ptr TabTests::_commonSetup() + { + const std::string settingsJson0{ R"( + { + "defaultProfile": "{6239a42c-1111-49a3-80bd-e8fdd045185c}", + "profiles": [ + { + "name" : "profile0", + "guid": "{6239a42c-1111-49a3-80bd-e8fdd045185c}", + "historySize": 1 + }, + { + "name" : "profile1", + "guid": "{6239a42c-2222-49a3-80bd-e8fdd045185c}", + "historySize": 2 + } + ] + })" }; + + CascadiaSettings settings0{ til::u8u16(settingsJson0) }; + VERIFY_IS_NOT_NULL(settings0); + + const auto guid1 = Microsoft::Console::Utils::GuidFromString(L"{6239a42c-1111-49a3-80bd-e8fdd045185c}"); + const auto guid2 = Microsoft::Console::Utils::GuidFromString(L"{6239a42c-2222-49a3-80bd-e8fdd045185c}"); + + // This is super wacky, but we can't just initialize the + // com_ptr in the lambda and assign it back out of + // the lambda. We'll crash trying to get a weak_ref to the TerminalPage + // during TerminalPage::Create() below. + // + // Instead, create the winrt object, then get a com_ptr to the + // implementation _from_ the winrt object. This seems to work, even if + // it's weird. + winrt::com_ptr page{ nullptr }; + _initializeTerminalPage(page, settings0); + + auto result = RunOnUIThread([&page]() { + VERIFY_ARE_EQUAL(1u, page->_tabs.Size()); + }); + VERIFY_SUCCEEDED(result); + + return page; + } + + void TabTests::TryZoomPane() + { + auto page = _commonSetup(); + + Log::Comment(L"Create a second pane"); + auto result = RunOnUIThread([&page]() { + SplitPaneArgs args{ SplitType::Duplicate }; + ActionEventArgs eventArgs{ args }; + // eventArgs.Args(args); + page->_HandleSplitPane(nullptr, eventArgs); + auto firstTab = page->_GetStrongTabImpl(0); + + VERIFY_ARE_EQUAL(2, firstTab->GetLeafPaneCount()); + VERIFY_IS_FALSE(firstTab->IsZoomed()); + }); + VERIFY_SUCCEEDED(result); + + Log::Comment(L"Zoom in on the pane"); + result = RunOnUIThread([&page]() { + ActionEventArgs eventArgs{}; + page->_HandleTogglePaneZoom(nullptr, eventArgs); + auto firstTab = page->_GetStrongTabImpl(0); + VERIFY_ARE_EQUAL(2, firstTab->GetLeafPaneCount()); + VERIFY_IS_TRUE(firstTab->IsZoomed()); + }); + VERIFY_SUCCEEDED(result); + + Log::Comment(L"Zoom out of the pane"); + result = RunOnUIThread([&page]() { + ActionEventArgs eventArgs{}; + page->_HandleTogglePaneZoom(nullptr, eventArgs); + auto firstTab = page->_GetStrongTabImpl(0); + VERIFY_ARE_EQUAL(2, firstTab->GetLeafPaneCount()); + VERIFY_IS_FALSE(firstTab->IsZoomed()); + }); + VERIFY_SUCCEEDED(result); + } + + void TabTests::MoveFocusFromZoomedPane() + { + auto page = _commonSetup(); + + Log::Comment(L"Create a second pane"); + auto result = RunOnUIThread([&page]() { + // Set up action + SplitPaneArgs args{ SplitType::Duplicate }; + ActionEventArgs eventArgs{ args }; + page->_HandleSplitPane(nullptr, eventArgs); + auto firstTab = page->_GetStrongTabImpl(0); + + VERIFY_ARE_EQUAL(2, firstTab->GetLeafPaneCount()); + VERIFY_IS_FALSE(firstTab->IsZoomed()); + }); + VERIFY_SUCCEEDED(result); + + Log::Comment(L"Zoom in on the pane"); + result = RunOnUIThread([&page]() { + // Set up action + ActionEventArgs eventArgs{}; + + page->_HandleTogglePaneZoom(nullptr, eventArgs); + + auto firstTab = page->_GetStrongTabImpl(0); + VERIFY_ARE_EQUAL(2, firstTab->GetLeafPaneCount()); + VERIFY_IS_TRUE(firstTab->IsZoomed()); + }); + VERIFY_SUCCEEDED(result); + + Log::Comment(L"Move focus. This will cause us to un-zoom."); + result = RunOnUIThread([&page]() { + // Set up action + MoveFocusArgs args{ Direction::Left }; + ActionEventArgs eventArgs{ args }; + + page->_HandleMoveFocus(nullptr, eventArgs); + + auto firstTab = page->_GetStrongTabImpl(0); + VERIFY_ARE_EQUAL(2, firstTab->GetLeafPaneCount()); + VERIFY_IS_FALSE(firstTab->IsZoomed()); + }); + VERIFY_SUCCEEDED(result); + } + + void TabTests::CloseZoomedPane() + { + auto page = _commonSetup(); + + Log::Comment(L"Create a second pane"); + auto result = RunOnUIThread([&page]() { + // Set up action + SplitPaneArgs args{ SplitType::Duplicate }; + ActionEventArgs eventArgs{ args }; + page->_HandleSplitPane(nullptr, eventArgs); + auto firstTab = page->_GetStrongTabImpl(0); + + VERIFY_ARE_EQUAL(2, firstTab->GetLeafPaneCount()); + VERIFY_IS_FALSE(firstTab->IsZoomed()); + }); + VERIFY_SUCCEEDED(result); + + Log::Comment(L"Zoom in on the pane"); + result = RunOnUIThread([&page]() { + // Set up action + ActionEventArgs eventArgs{}; + + page->_HandleTogglePaneZoom(nullptr, eventArgs); + + auto firstTab = page->_GetStrongTabImpl(0); + VERIFY_ARE_EQUAL(2, firstTab->GetLeafPaneCount()); + VERIFY_IS_TRUE(firstTab->IsZoomed()); + }); + VERIFY_SUCCEEDED(result); + + Log::Comment(L"Close Pane. This should cause us to un-zoom, and remove the second pane from the tree"); + result = RunOnUIThread([&page]() { + // Set up action + ActionEventArgs eventArgs{}; + + page->_HandleClosePane(nullptr, eventArgs); + + auto firstTab = page->_GetStrongTabImpl(0); + VERIFY_IS_FALSE(firstTab->IsZoomed()); + }); + VERIFY_SUCCEEDED(result); + + // Introduce a slight delay to let the events finish propagating + Sleep(250); + + Log::Comment(L"Check to ensure there's only one pane left."); + + result = RunOnUIThread([&page]() { + auto firstTab = page->_GetStrongTabImpl(0); + VERIFY_ARE_EQUAL(1, firstTab->GetLeafPaneCount()); + VERIFY_IS_FALSE(firstTab->IsZoomed()); + }); + VERIFY_SUCCEEDED(result); + } } diff --git a/src/cascadia/TerminalApp/AppActionHandlers.cpp b/src/cascadia/TerminalApp/AppActionHandlers.cpp index 2efcd1c735b..fc5dd1582c7 100644 --- a/src/cascadia/TerminalApp/AppActionHandlers.cpp +++ b/src/cascadia/TerminalApp/AppActionHandlers.cpp @@ -133,10 +133,9 @@ namespace winrt::TerminalApp::implementation // 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 GetRootElement to the UI tree - _UpdatedSelectedTab(_tabView.SelectedIndex()); } args.Handled(true); } diff --git a/src/cascadia/TerminalApp/Pane.cpp b/src/cascadia/TerminalApp/Pane.cpp index 96f99a6d644..8641793606d 100644 --- a/src/cascadia/TerminalApp/Pane.cpp +++ b/src/cascadia/TerminalApp/Pane.cpp @@ -702,6 +702,16 @@ void Pane::_CloseChild(const bool closeFirst) if (_lastActive) { _control.Focus(FocusState::Programmatic); + + // See GH#7252 + // Manually fire off the GotFocus event. Typically, this is done + // automatically when the control gets focused. However, if we're + // `exit`ing a zoomed pane, then the other sibling isn't in the UI + // tree currently. So the above call to Focus won't actually focus + // the control. Because Tab is relying on GotFocus to know who the + // active pane in the tree is, without this call, _no one_ will be + // the active pane any longer. + _GotFocusHandlers(shared_from_this()); } _UpdateBorders(); @@ -813,11 +823,13 @@ winrt::fire_and_forget Pane::_CloseChildRoutine(const bool closeFirst) const auto animationsEnabledInOS = uiSettings.AnimationsEnabled(); const auto animationsEnabledInApp = Media::Animation::Timeline::AllowDependentAnimations(); + // GH#7252: If either child is zoomed, just skip the animation. It won't work. + const bool eitherChildZoomed = pane->_firstChild->_zoomed || pane->_secondChild->_zoomed; // If animations are disabled, just skip this and go straight to // _CloseChild. Curiously, the pane opening animation doesn't need this, // and will skip straight to Completed when animations are disabled, but // this one doesn't seem to. - if (!animationsEnabledInOS || !animationsEnabledInApp) + if (!animationsEnabledInOS || !animationsEnabledInApp || eitherChildZoomed) { pane->_CloseChild(closeFirst); co_return; diff --git a/src/cascadia/TerminalApp/Tab.cpp b/src/cascadia/TerminalApp/Tab.cpp index 21c8b17d39f..2f9cc705838 100644 --- a/src/cascadia/TerminalApp/Tab.cpp +++ b/src/cascadia/TerminalApp/Tab.cpp @@ -33,6 +33,7 @@ namespace winrt::TerminalApp::implementation }); _activePane = _rootPane; + Content(_rootPane->GetRootElement()); _MakeTabViewItem(); _MakeSwitchToTabCommand(); @@ -61,24 +62,6 @@ namespace winrt::TerminalApp::implementation _RecalculateAndApplyTabColor(); } - // Method Description: - // - Get the root UIElement of this Tab's root pane. - // Arguments: - // - - // Return Value: - // - The UIElement acting as root of the Tab's root pane. - UIElement Tab::GetRootElement() - { - if (_zoomedPane) - { - return _zoomedPane->GetRootElement(); - } - else - { - return _rootPane->GetRootElement(); - } - } - // Method Description: // - Returns nullptr if no children of this tab were the last control to be // focused, or the TermControl that _was_ the last control to be focused (if @@ -509,6 +492,22 @@ namespace winrt::TerminalApp::implementation tab->_RecalculateAndApplyTabColor(); } }); + + // Add a Closed event handler to the Pane. If the pane closes out from + // underneath us, and it's zoomed, we want to be able to make sure to + // update our state accordingly to un-zoom that pane. See GH#7252. + pane->Closed([weakThis](auto&& /*s*/, auto && /*e*/) -> winrt::fire_and_forget { + if (auto tab{ weakThis.get() }) + { + if (tab->_zoomedPane) + { + co_await winrt::resume_foreground(tab->Content().Dispatcher()); + + tab->Content(tab->_rootPane->GetRootElement()); + tab->ExitZoom(); + } + } + }); } // Method Description: @@ -1070,6 +1069,7 @@ namespace winrt::TerminalApp::implementation _rootPane->Maximize(_zoomedPane); // Update the tab header to show the magnifying glass _UpdateTabHeader(); + Content(_zoomedPane->GetRootElement()); } void Tab::ExitZoom() { @@ -1077,6 +1077,7 @@ namespace winrt::TerminalApp::implementation _zoomedPane = nullptr; // Update the tab header to hide the magnifying glass _UpdateTabHeader(); + Content(_rootPane->GetRootElement()); } bool Tab::IsZoomed() diff --git a/src/cascadia/TerminalApp/Tab.h b/src/cascadia/TerminalApp/Tab.h index 4dae6bc8744..adfa4307b7b 100644 --- a/src/cascadia/TerminalApp/Tab.h +++ b/src/cascadia/TerminalApp/Tab.h @@ -24,7 +24,6 @@ namespace winrt::TerminalApp::implementation void Initialize(const winrt::Microsoft::Terminal::TerminalControl::TermControl& control); winrt::Microsoft::UI::Xaml::Controls::TabViewItem GetTabViewItem(); - winrt::Windows::UI::Xaml::UIElement GetRootElement(); winrt::Microsoft::Terminal::TerminalControl::TermControl GetActiveTerminalControl() const; std::optional GetFocusedProfile() const noexcept; @@ -88,6 +87,8 @@ namespace winrt::TerminalApp::implementation // 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); + private: std::shared_ptr _rootPane{ nullptr }; std::shared_ptr _activePane{ nullptr }; diff --git a/src/cascadia/TerminalApp/Tab.idl b/src/cascadia/TerminalApp/Tab.idl index db9746c99ee..7e88c587816 100644 --- a/src/cascadia/TerminalApp/Tab.idl +++ b/src/cascadia/TerminalApp/Tab.idl @@ -11,6 +11,8 @@ namespace TerminalApp Microsoft.Terminal.Settings.Model.Command SwitchToTabCommand { get; }; UInt32 TabViewIndex { get; }; + Windows.UI.Xaml.UIElement Content { get; }; + void SetDispatch(ShortcutActionDispatch dispatch); } } diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index 290b2c777e5..2b9be96ef29 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -1142,6 +1142,16 @@ namespace winrt::TerminalApp::implementation { page->_UpdateTitle(*tab); } + else if (args.PropertyName() == L"Content") + { + if (tab == page->_GetFocusedTab()) + { + page->_tabContent.Children().Clear(); + page->_tabContent.Children().Append(tab->Content()); + + tab->SetFocused(true); + } + } } }); @@ -1259,9 +1269,10 @@ namespace winrt::TerminalApp::implementation // 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->GetRootElement()); } } @@ -1948,7 +1959,7 @@ namespace winrt::TerminalApp::implementation auto tab{ _GetStrongTabImpl(index) }; _tabContent.Children().Clear(); - _tabContent.Children().Append(tab->GetRootElement()); + _tabContent.Children().Append(tab->Content()); // GH#7409: If the tab switcher is open, then we _don't_ want to // automatically focus the new tab here. The tab switcher wants diff --git a/src/cascadia/TerminalSettingsModel/ActionArgs.h b/src/cascadia/TerminalSettingsModel/ActionArgs.h index b53ea55b548..c70e438373e 100644 --- a/src/cascadia/TerminalSettingsModel/ActionArgs.h +++ b/src/cascadia/TerminalSettingsModel/ActionArgs.h @@ -255,6 +255,9 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation struct MoveFocusArgs : public MoveFocusArgsT { MoveFocusArgs() = default; + MoveFocusArgs(Model::Direction direction) : + _Direction{ direction } {}; + GETSET_PROPERTY(Model::Direction, Direction, Direction::None); static constexpr std::string_view DirectionKey{ "direction" }; @@ -370,6 +373,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation SplitPaneArgs(SplitState style, const Model::NewTerminalArgs& terminalArgs) : _SplitStyle{ style }, _TerminalArgs{ terminalArgs } {}; + SplitPaneArgs(SplitType splitMode) : + _SplitMode{ splitMode } {}; GETSET_PROPERTY(SplitState, SplitStyle, SplitState::Automatic); GETSET_PROPERTY(Model::NewTerminalArgs, TerminalArgs, nullptr); GETSET_PROPERTY(SplitType, SplitMode, SplitType::Manual); @@ -673,6 +678,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::factory_implementation BASIC_FACTORY(SwitchToTabArgs); BASIC_FACTORY(NewTerminalArgs); BASIC_FACTORY(NewTabArgs); + BASIC_FACTORY(MoveFocusArgs); BASIC_FACTORY(SplitPaneArgs); BASIC_FACTORY(ExecuteCommandlineArgs); BASIC_FACTORY(CloseOtherTabsArgs); diff --git a/src/cascadia/TerminalSettingsModel/ActionArgs.idl b/src/cascadia/TerminalSettingsModel/ActionArgs.idl index 6d460484dd6..7a4d7eb0bf6 100644 --- a/src/cascadia/TerminalSettingsModel/ActionArgs.idl +++ b/src/cascadia/TerminalSettingsModel/ActionArgs.idl @@ -94,6 +94,7 @@ namespace Microsoft.Terminal.Settings.Model [default_interface] runtimeclass MoveFocusArgs : IActionArgs { + MoveFocusArgs(Direction direction); Direction Direction { get; }; }; @@ -109,7 +110,9 @@ namespace Microsoft.Terminal.Settings.Model [default_interface] runtimeclass SplitPaneArgs : IActionArgs { - SplitPaneArgs(SplitState style, NewTerminalArgs terminalArgs); + SplitPaneArgs(SplitState split, NewTerminalArgs terminalArgs); + SplitPaneArgs(SplitType splitMode); + SplitState SplitStyle { get; }; NewTerminalArgs TerminalArgs { get; }; SplitType SplitMode { get; };