Skip to content

Commit

Permalink
Fix exiting a zoomed pane (#7973)
Browse files Browse the repository at this point in the history
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_.

* Introduced in #6989

* [x] Closes #7252
* [x] I work here
* [x] Tests added/passed 🎉
* [n/a] Requires documentation to be updated

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.
```

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

(cherry picked from commit ccf9f03)
  • Loading branch information
zadjii-msft authored and DHowett committed Oct 27, 2020
1 parent a0a70cc commit 8a758f7
Show file tree
Hide file tree
Showing 9 changed files with 255 additions and 25 deletions.
193 changes: 193 additions & 0 deletions src/cascadia/LocalTests_TerminalApp/TabTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
InitializeJsonReader();
Expand All @@ -76,6 +80,7 @@ namespace TerminalAppLocalTests
private:
void _initializeTerminalPage(winrt::com_ptr<winrt::TerminalApp::implementation::TerminalPage>& page,
winrt::com_ptr<winrt::TerminalApp::implementation::CascadiaSettings>& initialSettings);
winrt::com_ptr<winrt::TerminalApp::implementation::TerminalPage> _commonSetup();
};

void TabTests::EnsureTestsActivate()
Expand Down Expand Up @@ -517,4 +522,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:
// - <none>
// Return Value:
// - The initialized TerminalPage, ready to use.
winrt::com_ptr<winrt::TerminalApp::implementation::TerminalPage> 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<impl::TerminalPage> 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<winrt::TerminalApp::implementation::TerminalPage> 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);
}
}
10 changes: 10 additions & 0 deletions src/cascadia/TerminalApp/ActionArgs.h
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,9 @@ namespace winrt::TerminalApp::implementation
struct MoveFocusArgs : public MoveFocusArgsT<MoveFocusArgs>
{
MoveFocusArgs() = default;
MoveFocusArgs(TerminalApp::Direction direction) :
_Direction{ direction } {};

GETSET_PROPERTY(TerminalApp::Direction, Direction, TerminalApp::Direction::None);

static constexpr std::string_view DirectionKey{ "direction" };
Expand Down Expand Up @@ -308,6 +311,11 @@ namespace winrt::TerminalApp::implementation
struct SplitPaneArgs : public SplitPaneArgsT<SplitPaneArgs>
{
SplitPaneArgs() = default;
SplitPaneArgs(winrt::TerminalApp::SplitState style, const winrt::TerminalApp::NewTerminalArgs& terminalArgs) :
_SplitStyle{ style },
_TerminalArgs{ terminalArgs } {};
SplitPaneArgs(SplitType splitMode) :
_SplitMode{ splitMode } {};
GETSET_PROPERTY(winrt::TerminalApp::SplitState, SplitStyle, winrt::TerminalApp::SplitState::Automatic);
GETSET_PROPERTY(winrt::TerminalApp::NewTerminalArgs, TerminalArgs, nullptr);
GETSET_PROPERTY(winrt::TerminalApp::SplitType, SplitMode, winrt::TerminalApp::SplitType::Manual);
Expand Down Expand Up @@ -553,4 +561,6 @@ namespace winrt::TerminalApp::factory_implementation
{
BASIC_FACTORY(ActionEventArgs);
BASIC_FACTORY(NewTerminalArgs);
BASIC_FACTORY(MoveFocusArgs);
BASIC_FACTORY(SplitPaneArgs);
}
4 changes: 4 additions & 0 deletions src/cascadia/TerminalApp/ActionArgs.idl
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ namespace TerminalApp

[default_interface] runtimeclass MoveFocusArgs : IActionArgs
{
MoveFocusArgs(Direction direction);
Direction Direction { get; };
};

Expand All @@ -102,6 +103,9 @@ namespace TerminalApp

[default_interface] runtimeclass SplitPaneArgs : IActionArgs
{
SplitPaneArgs(SplitState split, NewTerminalArgs terminalArgs);
SplitPaneArgs(SplitType splitMode);

SplitState SplitStyle { get; };
NewTerminalArgs TerminalArgs { get; };
SplitType SplitMode { get; };
Expand Down
5 changes: 2 additions & 3 deletions src/cascadia/TerminalApp/AppActionHandlers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -132,10 +132,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);
}
Expand Down
10 changes: 10 additions & 0 deletions src/cascadia/TerminalApp/Pane.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -657,6 +657,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();
Expand Down
37 changes: 19 additions & 18 deletions src/cascadia/TerminalApp/Tab.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ namespace winrt::TerminalApp::implementation
});

_activePane = _rootPane;
Content(_rootPane->GetRootElement());

_MakeTabViewItem();
}
Expand All @@ -58,24 +59,6 @@ namespace winrt::TerminalApp::implementation
_RecalculateAndApplyTabColor();
}

// Method Description:
// - Get the root UIElement of this Tab's root pane.
// Arguments:
// - <none>
// 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
Expand Down Expand Up @@ -502,6 +485,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:
Expand Down Expand Up @@ -1012,13 +1011,15 @@ namespace winrt::TerminalApp::implementation
_rootPane->Maximize(_zoomedPane);
// Update the tab header to show the magnifying glass
_UpdateTabHeader();
Content(_zoomedPane->GetRootElement());
}
void Tab::ExitZoom()
{
_rootPane->Restore(_zoomedPane);
_zoomedPane = nullptr;
// Update the tab header to hide the magnifying glass
_UpdateTabHeader();
Content(_rootPane->GetRootElement());
}

bool Tab::IsZoomed()
Expand Down
3 changes: 2 additions & 1 deletion src/cascadia/TerminalApp/Tab.h
Original file line number Diff line number Diff line change
Expand Up @@ -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<GUID> GetFocusedProfile() const noexcept;

Expand Down Expand Up @@ -77,6 +76,8 @@ namespace winrt::TerminalApp::implementation
OBSERVABLE_GETSET_PROPERTY(winrt::hstring, Title, _PropertyChangedHandlers);
OBSERVABLE_GETSET_PROPERTY(winrt::Windows::UI::Xaml::Controls::IconSource, IconSource, _PropertyChangedHandlers, nullptr);

OBSERVABLE_GETSET_PROPERTY(winrt::Windows::UI::Xaml::UIElement, Content, _PropertyChangedHandlers, nullptr);

private:
std::shared_ptr<Pane> _rootPane{ nullptr };
std::shared_ptr<Pane> _activePane{ nullptr };
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalApp/Tab.idl
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,6 @@ namespace TerminalApp
{
String Title { get; };
Windows.UI.Xaml.Controls.IconSource IconSource { get; };
Windows.UI.Xaml.UIElement Content { get; };
}
}
Loading

0 comments on commit 8a758f7

Please sign in to comment.