Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed self reference capture in Tab and TerminalPage #3835

Merged
13 commits merged into from
Dec 5, 2019
Merged

Fixed self reference capture in Tab and TerminalPage #3835

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
d6c9d76
Added FindGuid helper function
mkitzan Nov 28, 2019
033861a
Style change
mkitzan Nov 28, 2019
b4c5e4e
Tests for FindGuid and FindProfile
mkitzan Nov 28, 2019
b4d57da
Fixed code format?
mkitzan Nov 28, 2019
10d9578
Code format guess
mkitzan Nov 28, 2019
08de164
optional<GUID> fix
mkitzan Nov 28, 2019
48ff6cd
Updated function desc
mkitzan Nov 28, 2019
d7cb7f4
Fixed throwing cast
mkitzan Nov 30, 2019
afc8ed3
Merge branch 'find-guid-helper'
mkitzan Dec 2, 2019
f7349d7
Merge branch 'floating-point-comp'
mkitzan Dec 2, 2019
27a4c4f
Merge remote-tracking branch 'upstream/master'
mkitzan Dec 3, 2019
c5542cc
Weak captures in Tab and TerminalPage lambdas
mkitzan Dec 4, 2019
2437a58
PR review fixes
mkitzan Dec 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 52 additions & 27 deletions src/cascadia/TerminalApp/Tab.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,6 @@ Tab::Tab(const GUID& profile, const TermControl& control)

_activePane = _rootPane;

_AttachEventHandlersToPane(_rootPane);

_AttachEventHandlersToControl(control);

_MakeTabViewItem();
}

Expand Down Expand Up @@ -108,6 +104,19 @@ std::optional<GUID> Tab::GetFocusedProfile() const noexcept
return _activePane->GetFocusedProfile();
}

// Method Description:
// - Called after construction of a Tab object to bind event handlers to its
// associated Pane and TermControl object
// Arguments:
// - control: reference to the TermControl object to bind event to
// Return Value:
// - <none>
void Tab::BindEventHandlers(const TermControl& control) noexcept
{
_AttachEventHandlersToPane(_rootPane);
_AttachEventHandlersToControl(control);
}

// Method Description:
// - Attempts to update the settings of this tab's tree of panes.
// Arguments:
Expand Down Expand Up @@ -147,8 +156,13 @@ void Tab::UpdateIcon(const winrt::hstring iconPath)

_lastIconPath = iconPath;

_tabViewItem.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [this]() {
_tabViewItem.IconSource(GetColoredIcon<winrt::MUX::Controls::IconSource>(_lastIconPath));
std::weak_ptr<Tab> weakThis{ shared_from_this() };

_tabViewItem.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [weakThis]() {
if (auto tab{ weakThis.lock() })
{
tab->_tabViewItem.IconSource(GetColoredIcon<winrt::MUX::Controls::IconSource>(tab->_lastIconPath));
}
});
}

Expand All @@ -175,8 +189,13 @@ void Tab::SetTabText(const winrt::hstring& text)
{
// Copy the hstring, so we don't capture a dead reference
winrt::hstring textCopy{ text };
_tabViewItem.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [text = std::move(textCopy), this]() {
_tabViewItem.Header(winrt::box_value(text));
std::weak_ptr<Tab> weakThis{ shared_from_this() };

_tabViewItem.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [text = std::move(textCopy), weakThis]() {
if (auto tab{ weakThis.lock() })
{
tab->_tabViewItem.Header(winrt::box_value(text));
}
});
}

Expand Down Expand Up @@ -296,12 +315,16 @@ void Tab::ClosePane()
// - <none>
void Tab::_AttachEventHandlersToControl(const TermControl& control)
{
control.TitleChanged([this](auto newTitle) {
// The title of the control changed, but not necessarily the title
// of the tab. Get the title of the active pane of the tab, and set
// the tab's text to the active panes' text.
auto newTabTitle = GetActiveTitle();
SetTabText(newTabTitle);
std::weak_ptr<Tab> weakThis{ shared_from_this() };

control.TitleChanged([weakThis](auto newTitle) {
// Check if Tab's lifetime has expired
if (auto tab{ weakThis.lock() })
{
// The title of the control changed, but not necessarily the title of the tab.
// Set the tab's text to the active panes' text.
tab->SetTabText(tab->GetActiveTitle());
}
});
}

Expand All @@ -316,22 +339,24 @@ void Tab::_AttachEventHandlersToControl(const TermControl& control)
// - <none>
void Tab::_AttachEventHandlersToPane(std::shared_ptr<Pane> pane)
{
pane->GotFocus([this](std::shared_ptr<Pane> sender) {
// Do nothing if it's the same pane as before.
if (sender == _activePane)
std::weak_ptr<Tab> weakThis{ shared_from_this() };

pane->GotFocus([weakThis](std::shared_ptr<Pane> sender) {
// Do nothing if the Tab's lifetime is expired or pane isn't new.
auto tab{ weakThis.lock() };
if (tab && sender != tab->_activePane)
{
return;
}
// Clear the active state of the entire tree, and mark only the sender as active.
_rootPane->ClearActive();
_activePane = sender;
_activePane->SetActive();
// Clear the active state of the entire tree, and mark only the sender as active.
tab->_rootPane->ClearActive();
tab->_activePane = sender;
tab->_activePane->SetActive();

// Update our own title text to match the newly-active pane.
SetTabText(GetActiveTitle());
// Update our own title text to match the newly-active pane.
tab->SetTabText(tab->GetActiveTitle());

// Raise our own ActivePaneChanged event.
_ActivePaneChangedHandlers();
// Raise our own ActivePaneChanged event.
tab->_ActivePaneChangedHandlers();
}
});
}

Expand Down
6 changes: 5 additions & 1 deletion src/cascadia/TerminalApp/Tab.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,15 @@
#include <winrt/Microsoft.UI.Xaml.Controls.h>
#include "Pane.h"

class Tab
// TODO: make Tab a WinRT type
zadjii-msft marked this conversation as resolved.
Show resolved Hide resolved
class Tab : public std::enable_shared_from_this<Tab>
{
public:
Tab(const GUID& profile, const winrt::Microsoft::Terminal::TerminalControl::TermControl& control);

// Called after construction to setup events with weak_ptr
void BindEventHandlers(const winrt::Microsoft::Terminal::TerminalControl::TermControl& control) noexcept;

winrt::Microsoft::UI::Xaml::Controls::TabViewItem GetTabViewItem();
winrt::Windows::UI::Xaml::UIElement GetRootElement();
winrt::Microsoft::Terminal::TerminalControl::TermControl GetActiveTerminalControl() const;
Expand Down
122 changes: 83 additions & 39 deletions src/cascadia/TerminalApp/TerminalPage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,23 +60,36 @@ namespace winrt::TerminalApp::implementation
_tabView = _tabRow.TabView();
_rearranging = false;

_tabView.TabDragStarting([this](auto&& /*o*/, auto&& /*a*/) {
_rearranging = true;
_rearrangeFrom = std::nullopt;
_rearrangeTo = std::nullopt;
});
// weak_ptr to this TerminalPage object lambda capturing
auto weakThis{ get_weak() };

_tabView.TabDragCompleted([this](auto&& /*o*/, auto&& /*a*/) {
if (_rearrangeFrom.has_value() && _rearrangeTo.has_value() && _rearrangeTo != _rearrangeFrom)
_tabView.TabDragStarting([weakThis](auto&& /*o*/, auto&& /*a*/) {
if (auto page{ weakThis.get() })
{
auto tab = _tabs.at(_rearrangeFrom.value());
_tabs.erase(_tabs.begin() + _rearrangeFrom.value());
_tabs.insert(_tabs.begin() + _rearrangeTo.value(), tab);
page->_rearranging = true;
page->_rearrangeFrom = std::nullopt;
page->_rearrangeTo = std::nullopt;
}
});

_tabView.TabDragCompleted([weakThis](auto&& /*o*/, auto&& /*a*/) {
if (auto page{ weakThis.get() })
{
auto& from{ page->_rearrangeFrom };
auto& to{ page->_rearrangeTo };

_rearranging = false;
_rearrangeFrom = std::nullopt;
_rearrangeTo = std::nullopt;
if (from.has_value() && to.has_value() && to != from)
{
auto& tabs{ page->_tabs };
auto tab = tabs.at(to.value());
zadjii-msft marked this conversation as resolved.
Show resolved Hide resolved
tabs.erase(tabs.begin() + from.value());
tabs.insert(tabs.begin() + to.value(), tab);
}

page->_rearranging = false;
from = std::nullopt;
to = std::nullopt;
}
});

auto tabRowImpl = winrt::get_self<implementation::TabRowControl>(_tabRow);
Expand All @@ -100,8 +113,11 @@ namespace winrt::TerminalApp::implementation
_RegisterActionCallbacks();

//Event Bindings (Early)
_newTabButton.Click([this](auto&&, auto&&) {
this->_OpenNewTab(std::nullopt);
_newTabButton.Click([weakThis](auto&&, auto&&) {
if (auto page{ weakThis.get() })
{
page->_OpenNewTab(std::nullopt);
}
});
_tabView.SelectionChanged({ this, &TerminalPage::_OnTabSelectionChanged });
_tabView.TabCloseRequested({ this, &TerminalPage::_OnTabCloseRequested });
Expand Down Expand Up @@ -301,8 +317,13 @@ namespace winrt::TerminalApp::implementation
profileMenuItem.FontWeight(FontWeights::Bold());
}

profileMenuItem.Click([this, profileIndex](auto&&, auto&&) {
this->_OpenNewTab({ profileIndex });
auto weakThis{ get_weak() };

profileMenuItem.Click([profileIndex, weakThis](auto&&, auto&&) {
if (auto page{ weakThis.get() })
{
page->_OpenNewTab({ profileIndex });
}
});
newTabFlyout.Items().Append(profileMenuItem);
}
Expand Down Expand Up @@ -440,17 +461,21 @@ namespace winrt::TerminalApp::implementation
// Don't capture a strong ref to the tab. If the tab is removed as this
// is called, we don't really care anymore about handling the event.
std::weak_ptr<Tab> weakTabPtr = newTab;
auto weakThis{ get_weak() };

// When the tab's active pane changes, we'll want to lookup a new icon
// for it, and possibly propogate the title up to the window.
newTab->ActivePaneChanged([this, weakTabPtr]() {
if (auto tab = weakTabPtr.lock())
newTab->ActivePaneChanged([weakTabPtr, weakThis]() {
auto page{ weakThis.get() };
auto tab{ weakTabPtr.lock() };

if (page && tab)
{
// Possibly update the icon of the tab.
_UpdateTabIcon(tab);

page->_UpdateTabIcon(tab);
// Possibly update the title of the tab, window to match the newly
// focused pane.
_UpdateTitle(tab);
page->_UpdateTitle(tab);
}
});

Expand All @@ -467,10 +492,16 @@ namespace winrt::TerminalApp::implementation
tabViewItem.PointerPressed({ this, &TerminalPage::_OnTabClick });

// When the tab is closed, remove it from our list of tabs.
newTab->Closed([tabViewItem, this](auto&& /*s*/, auto&& /*e*/) {
_tabView.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [tabViewItem, this]() {
_RemoveTabViewItem(tabViewItem);
});
newTab->Closed([tabViewItem, weakThis](auto&& /*s*/, auto&& /*e*/) {
if (auto page{ weakThis.get() })
{
page->_tabView.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [tabViewItem, weakThis]() {
if (auto page{ weakThis.get() })
{
page->_RemoveTabViewItem(tabViewItem);
}
});
}
});

// This is one way to set the tab's selected background color.
Expand Down Expand Up @@ -762,19 +793,25 @@ namespace winrt::TerminalApp::implementation
// Add an event handler when the terminal wants to paste data from the Clipboard.
term.PasteFromClipboard({ this, &TerminalPage::_PasteFromClipboardHandler });

// Bind Tab events to the TermControl and the Tab's Pane
hostingTab->BindEventHandlers(term);
zadjii-msft marked this conversation as resolved.
Show resolved Hide resolved

// Don't capture a strong ref to the tab. If the tab is removed as this
// is called, we don't really care anymore about handling the event.
std::weak_ptr<Tab> weakTabPtr = hostingTab;
term.TitleChanged([this, weakTabPtr](auto newTitle) {
auto tab = weakTabPtr.lock();
if (!tab)
auto weakThis{ get_weak() };

term.TitleChanged([weakTabPtr, weakThis](auto newTitle) {
auto page{ weakThis.get() };
auto tab{ weakTabPtr.lock() };

if (page && tab)
{
return;
// The title of the control changed, but not necessarily the title
// of the tab. Get the title of the focused pane of the tab, and set
// the tab's text to the focused panes' text.
page->_UpdateTitle(tab);
}
// The title of the control changed, but not necessarily the title
// of the tab. Get the title of the focused pane of the tab, and set
// the tab's text to the focused panes' text.
_UpdateTitle(tab);
});
}

Expand Down Expand Up @@ -849,9 +886,12 @@ namespace winrt::TerminalApp::implementation
// GH#1117: This is a workaround because _tabView.SelectedIndex(tabIndex)
// sometimes set focus to an incorrect tab after removing some tabs
auto tab = _tabs.at(tabIndex);
_tabView.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [tab, this]() {
auto tabViewItem = tab->GetTabViewItem();
_tabView.SelectedItem(tabViewItem);
auto weakThis{ get_weak() };
_tabView.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [tab, weakThis]() {
if (auto page{ weakThis.get() })
{
page->_tabView.SelectedItem(tab->GetTabViewItem());
}
});
}

Expand Down Expand Up @@ -1350,10 +1390,14 @@ namespace winrt::TerminalApp::implementation
_UpdateTitle(tab);
}

this->Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [this]() {
auto weakThis{ get_weak() };
this->Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [weakThis]() {
// repopulate the new tab button's flyout with entries for each
// profile, which might have changed
_CreateNewTabFlyout();
if (auto page{ weakThis.get() })
{
page->_CreateNewTabFlyout();
}
});
}

Expand Down