From b9fa082138148585c206f37bac30a2c2d2768c94 Mon Sep 17 00:00:00 2001 From: Leon Liang Date: Wed, 22 Jan 2020 15:39:59 -0800 Subject: [PATCH 01/17] Tab is now a WinRT type, changed its shared_ptr self calls to get_weaks --- .../LocalTests_TerminalApp/TabTests.cpp | 6 +- src/cascadia/TerminalApp/Tab.cpp | 646 +++++++++--------- src/cascadia/TerminalApp/Tab.h | 81 ++- src/cascadia/TerminalApp/Tab.idl | 11 + src/cascadia/TerminalApp/TerminalApp.vcxproj | 1 + .../TerminalApp/lib/TerminalAppLib.vcxproj | 11 +- .../lib/TerminalAppLib.vcxproj.filters | 3 + src/cascadia/inc/cppwinrt_utils.h | 12 + 8 files changed, 409 insertions(+), 362 deletions(-) create mode 100644 src/cascadia/TerminalApp/Tab.idl diff --git a/src/cascadia/LocalTests_TerminalApp/TabTests.cpp b/src/cascadia/LocalTests_TerminalApp/TabTests.cpp index f900c4a4047..42acb7946b5 100644 --- a/src/cascadia/LocalTests_TerminalApp/TabTests.cpp +++ b/src/cascadia/LocalTests_TerminalApp/TabTests.cpp @@ -118,9 +118,9 @@ namespace TerminalAppLocalTests // If you leave the Tab shared_ptr owned by the RunOnUIThread lambda, it // will crash when the test tears down. Not totally clear why, but make // sure it's owned outside the lambda - std::shared_ptr newTab{ nullptr }; + /*winrt::TerminalApp::Tab*/ - auto result = RunOnUIThread([&newTab]() { + auto result = RunOnUIThread([]() { // Try creating all of: // 1. one of our pure c++ types (Profile) // 2. one of our c++winrt types (TerminalSettings, EchoConnection) @@ -135,7 +135,7 @@ namespace TerminalAppLocalTests winrt::Microsoft::Terminal::TerminalControl::TermControl term{ settings, conn }; VERIFY_IS_NOT_NULL(term); - newTab = std::make_shared(profileGuid, term); + auto newTab = winrt::make_self(profileGuid, term); VERIFY_IS_NOT_NULL(newTab); }); diff --git a/src/cascadia/TerminalApp/Tab.cpp b/src/cascadia/TerminalApp/Tab.cpp index cea9db3470c..52e36a706c6 100644 --- a/src/cascadia/TerminalApp/Tab.cpp +++ b/src/cascadia/TerminalApp/Tab.cpp @@ -3,8 +3,10 @@ #include "pch.h" #include "Tab.h" +#include "Tab.g.cpp" #include "Utils.h" +using namespace winrt; using namespace winrt::Windows::UI::Xaml; using namespace winrt::Windows::UI::Core; using namespace winrt::Microsoft::Terminal::Settings; @@ -15,372 +17,376 @@ namespace winrt namespace MUX = Microsoft::UI::Xaml; } -Tab::Tab(const GUID& profile, const TermControl& control) +namespace winrt::TerminalApp::implementation { - _rootPane = std::make_shared(profile, control, true); - - _rootPane->Closed([=](auto&& /*s*/, auto&& /*e*/) { - _ClosedHandlers(nullptr, nullptr); - }); - - _activePane = _rootPane; - - _MakeTabViewItem(); -} - -void Tab::_MakeTabViewItem() -{ - _tabViewItem = ::winrt::MUX::Controls::TabViewItem{}; -} + Tab::Tab(const GUID& profile, const TermControl& control) + { + _rootPane = std::make_shared(profile, control, true); -UIElement Tab::GetRootElement() -{ - return _rootPane->GetRootElement(); -} + _rootPane->Closed([=](auto&& /*s*/, auto&& /*e*/) { + _ClosedHandlers(nullptr, nullptr); + }); -// 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 -// there was one). -// - This control might not currently be focused, if the tab itself is not -// currently focused. -// Arguments: -// - -// Return Value: -// - nullptr if no children were marked `_lastFocused`, else the TermControl -// that was last focused. -TermControl Tab::GetActiveTerminalControl() const -{ - return _activePane->GetTerminalControl(); -} + _activePane = _rootPane; -winrt::MUX::Controls::TabViewItem Tab::GetTabViewItem() -{ - return _tabViewItem; -} + _MakeTabViewItem(); + } -// Method Description: -// - Returns true if this is the currently focused tab. For any set of tabs, -// there should only be one tab that is marked as focused, though each tab has -// no control over the other tabs in the set. -// Arguments: -// - -// Return Value: -// - true iff this tab is focused. -bool Tab::IsFocused() const noexcept -{ - return _focused; -} + void Tab::_MakeTabViewItem() + { + _tabViewItem = ::winrt::MUX::Controls::TabViewItem{}; + } -// Method Description: -// - Updates our focus state. If we're gaining focus, make sure to transfer -// focus to the last focused terminal control in our tree of controls. -// Arguments: -// - focused: our new focus state. If true, we should be focused. If false, we -// should be unfocused. -// Return Value: -// - -void Tab::SetFocused(const bool focused) -{ - _focused = focused; + UIElement Tab::GetRootElement() + { + return _rootPane->GetRootElement(); + } - if (_focused) + // 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 + // there was one). + // - This control might not currently be focused, if the tab itself is not + // currently focused. + // Arguments: + // - + // Return Value: + // - nullptr if no children were marked `_lastFocused`, else the TermControl + // that was last focused. + TermControl Tab::GetActiveTerminalControl() const { - _Focus(); + return _activePane->GetTerminalControl(); } -} -// Method Description: -// - Returns nullopt if no children of this tab were the last control to be -// focused, or the GUID of the profile of the last control to be focused (if -// there was one). -// Arguments: -// - -// Return Value: -// - nullopt if no children of this tab were the last control to be -// focused, else the GUID of the profile of the last control to be focused -std::optional Tab::GetFocusedProfile() const noexcept -{ - return _activePane->GetFocusedProfile(); -} + winrt::MUX::Controls::TabViewItem Tab::GetTabViewItem() + { + return _tabViewItem; + } -// 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: -// - -void Tab::BindEventHandlers(const TermControl& control) noexcept -{ - _AttachEventHandlersToPane(_rootPane); - _AttachEventHandlersToControl(control); -} + // Method Description: + // - Returns true if this is the currently focused tab. For any set of tabs, + // there should only be one tab that is marked as focused, though each tab has + // no control over the other tabs in the set. + // Arguments: + // - + // Return Value: + // - true iff this tab is focused. + bool Tab::IsFocused() const noexcept + { + return _focused; + } -// Method Description: -// - Attempts to update the settings of this tab's tree of panes. -// Arguments: -// - settings: The new TerminalSettings to apply to any matching controls -// - profile: The GUID of the profile these settings should apply to. -// Return Value: -// - -void Tab::UpdateSettings(const TerminalSettings& settings, const GUID& profile) -{ - _rootPane->UpdateSettings(settings, profile); -} + // Method Description: + // - Updates our focus state. If we're gaining focus, make sure to transfer + // focus to the last focused terminal control in our tree of controls. + // Arguments: + // - focused: our new focus state. If true, we should be focused. If false, we + // should be unfocused. + // Return Value: + // - + void Tab::SetFocused(const bool focused) + { + _focused = focused; -// Method Description: -// - Focus the last focused control in our tree of panes. -// Arguments: -// - -// Return Value: -// - -void Tab::_Focus() -{ - _focused = true; + if (_focused) + { + _Focus(); + } + } - auto lastFocusedControl = GetActiveTerminalControl(); - if (lastFocusedControl) + // Method Description: + // - Returns nullopt if no children of this tab were the last control to be + // focused, or the GUID of the profile of the last control to be focused (if + // there was one). + // Arguments: + // - + // Return Value: + // - nullopt if no children of this tab were the last control to be + // focused, else the GUID of the profile of the last control to be focused + std::optional Tab::GetFocusedProfile() const noexcept { - lastFocusedControl.Focus(FocusState::Programmatic); + return _activePane->GetFocusedProfile(); } -} -winrt::fire_and_forget Tab::UpdateIcon(const winrt::hstring iconPath) -{ - // Don't reload our icon if it hasn't changed. - if (iconPath == _lastIconPath) + // 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: + // - + void Tab::BindEventHandlers(const TermControl& control) noexcept { - return; + _AttachEventHandlersToPane(_rootPane); + _AttachEventHandlersToControl(control); } - _lastIconPath = iconPath; + // Method Description: + // - Attempts to update the settings of this tab's tree of panes. + // Arguments: + // - settings: The new TerminalSettings to apply to any matching controls + // - profile: The GUID of the profile these settings should apply to. + // Return Value: + // - + void Tab::UpdateSettings(const TerminalSettings& settings, const GUID& profile) + { + _rootPane->UpdateSettings(settings, profile); + } - std::weak_ptr weakThis{ shared_from_this() }; + // Method Description: + // - Focus the last focused control in our tree of panes. + // Arguments: + // - + // Return Value: + // - + void Tab::_Focus() + { + _focused = true; - co_await winrt::resume_foreground(_tabViewItem.Dispatcher()); + auto lastFocusedControl = GetActiveTerminalControl(); + if (lastFocusedControl) + { + lastFocusedControl.Focus(FocusState::Programmatic); + } + } - if (auto tab{ weakThis.lock() }) + winrt::fire_and_forget Tab::UpdateIcon(const winrt::hstring iconPath) { - _tabViewItem.IconSource(GetColoredIcon(_lastIconPath)); - } -} + // Don't reload our icon if it hasn't changed. + if (iconPath == _lastIconPath) + { + return; + } -// Method Description: -// - Gets the title string of the last focused terminal control in our tree. -// Returns the empty string if there is no such control. -// Arguments: -// - -// Return Value: -// - the title string of the last focused terminal control in our tree. -winrt::hstring Tab::GetActiveTitle() const -{ - const auto lastFocusedControl = GetActiveTerminalControl(); - return lastFocusedControl ? lastFocusedControl.Title() : L""; -} + _lastIconPath = iconPath; -// Method Description: -// - Set the text on the TabViewItem for this tab. -// Arguments: -// - text: The new text string to use as the Header for our TabViewItem -// Return Value: -// - -winrt::fire_and_forget Tab::SetTabText(const winrt::hstring text) -{ - // Copy the hstring, so we don't capture a dead reference - winrt::hstring textCopy{ text }; - std::weak_ptr weakThis{ shared_from_this() }; + auto weakThis{ get_weak() }; - co_await winrt::resume_foreground(_tabViewItem.Dispatcher()); + co_await winrt::resume_foreground(_tabViewItem.Dispatcher()); - if (auto tab{ weakThis.lock() }) - { - _tabViewItem.Header(winrt::box_value(text)); + if (auto tab{ weakThis.get() }) + { + _tabViewItem.IconSource(GetColoredIcon(_lastIconPath)); + } } -} -// Method Description: -// - Move the viewport of the terminal up or down a number of lines. Negative -// values of `delta` will move the view up, and positive values will move -// the viewport down. -// Arguments: -// - delta: a number of lines to move the viewport relative to the current viewport. -// Return Value: -// - -winrt::fire_and_forget Tab::Scroll(const int delta) -{ - auto control = GetActiveTerminalControl(); + // Method Description: + // - Gets the title string of the last focused terminal control in our tree. + // Returns the empty string if there is no such control. + // Arguments: + // - + // Return Value: + // - the title string of the last focused terminal control in our tree. + winrt::hstring Tab::GetActiveTitle() const + { + const auto lastFocusedControl = GetActiveTerminalControl(); + return lastFocusedControl ? lastFocusedControl.Title() : L""; + } - co_await winrt::resume_foreground(control.Dispatcher()); + // Method Description: + // - Set the text on the TabViewItem for this tab. + // Arguments: + // - text: The new text string to use as the Header for our TabViewItem + // Return Value: + // - + winrt::fire_and_forget Tab::SetTabText(const winrt::hstring text) + { + // Copy the hstring, so we don't capture a dead reference + winrt::hstring textCopy{ text }; + auto weakThis{ get_weak() }; - const auto currentOffset = control.GetScrollOffset(); - control.KeyboardScrollViewport(currentOffset + delta); -} + co_await winrt::resume_foreground(_tabViewItem.Dispatcher()); -// Method Description: -// - Determines whether the focused pane has sufficient space to be split. -// Arguments: -// - splitType: The type of split we want to create. -// Return Value: -// - True if the focused pane can be split. False otherwise. -bool Tab::CanSplitPane(winrt::TerminalApp::SplitState splitType) -{ - return _activePane->CanSplit(splitType); -} + if (auto tab{ weakThis.get() }) + { + _tabViewItem.Header(winrt::box_value(text)); + } + } -// Method Description: -// - Split the focused pane in our tree of panes, and place the -// given TermControl into the newly created pane. -// Arguments: -// - splitType: The type of split we want to create. -// - profile: The profile GUID to associate with the newly created pane. -// - control: A TermControl to use in the new pane. -// Return Value: -// - -void Tab::SplitPane(winrt::TerminalApp::SplitState splitType, const GUID& profile, TermControl& control) -{ - auto [first, second] = _activePane->Split(splitType, profile, control); + // Method Description: + // - Move the viewport of the terminal up or down a number of lines. Negative + // values of `delta` will move the view up, and positive values will move + // the viewport down. + // Arguments: + // - delta: a number of lines to move the viewport relative to the current viewport. + // Return Value: + // - + winrt::fire_and_forget Tab::Scroll(const int delta) + { + auto control = GetActiveTerminalControl(); - _AttachEventHandlersToControl(control); + co_await winrt::resume_foreground(control.Dispatcher()); - // Add a event handlers to the new panes' GotFocus event. When the pane - // gains focus, we'll mark it as the new active pane. - _AttachEventHandlersToPane(first); - _AttachEventHandlersToPane(second); -} + const auto currentOffset = control.GetScrollOffset(); + control.KeyboardScrollViewport(currentOffset + delta); + } -// Method Description: -// - See Pane::CalcSnappedDimension -float Tab::CalcSnappedDimension(const bool widthOrHeight, const float dimension) const -{ - return _rootPane->CalcSnappedDimension(widthOrHeight, dimension); -} + // Method Description: + // - Determines whether the focused pane has sufficient space to be split. + // Arguments: + // - splitType: The type of split we want to create. + // Return Value: + // - True if the focused pane can be split. False otherwise. + bool Tab::CanSplitPane(winrt::TerminalApp::SplitState splitType) + { + return _activePane->CanSplit(splitType); + } -// Method Description: -// - Update the size of our panes to fill the new given size. This happens when -// the window is resized. -// Arguments: -// - newSize: the amount of space that the panes have to fill now. -// Return Value: -// - -void Tab::ResizeContent(const winrt::Windows::Foundation::Size& newSize) -{ - // NOTE: This _must_ be called on the root pane, so that it can propogate - // throughout the entire tree. - _rootPane->ResizeContent(newSize); -} + // Method Description: + // - Split the focused pane in our tree of panes, and place the + // given TermControl into the newly created pane. + // Arguments: + // - splitType: The type of split we want to create. + // - profile: The profile GUID to associate with the newly created pane. + // - control: A TermControl to use in the new pane. + // Return Value: + // - + void Tab::SplitPane(winrt::TerminalApp::SplitState splitType, const GUID& profile, TermControl& control) + { + auto [first, second] = _activePane->Split(splitType, profile, control); -// Method Description: -// - Attempt to move a separator between panes, as to resize each child on -// either size of the separator. See Pane::ResizePane for details. -// Arguments: -// - direction: The direction to move the separator in. -// Return Value: -// - -void Tab::ResizePane(const winrt::TerminalApp::Direction& direction) -{ - // NOTE: This _must_ be called on the root pane, so that it can propogate - // throughout the entire tree. - _rootPane->ResizePane(direction); -} + _AttachEventHandlersToControl(control); -// Method Description: -// - Attempt to move focus between panes, as to focus the child on -// the other side of the separator. See Pane::NavigateFocus for details. -// Arguments: -// - direction: The direction to move the focus in. -// Return Value: -// - -void Tab::NavigateFocus(const winrt::TerminalApp::Direction& direction) -{ - // NOTE: This _must_ be called on the root pane, so that it can propogate - // throughout the entire tree. - _rootPane->NavigateFocus(direction); -} + // Add a event handlers to the new panes' GotFocus event. When the pane + // gains focus, we'll mark it as the new active pane. + _AttachEventHandlersToPane(first); + _AttachEventHandlersToPane(second); + } -// Method Description: -// - Closes the currently focused pane in this tab. If it's the last pane in -// this tab, our Closed event will be fired (at a later time) for anyone -// registered as a handler of our close event. -// Arguments: -// - -// Return Value: -// - -void Tab::ClosePane() -{ - _activePane->Close(); -} + // Method Description: + // - See Pane::CalcSnappedDimension + float Tab::CalcSnappedDimension(const bool widthOrHeight, const float dimension) const + { + return _rootPane->CalcSnappedDimension(widthOrHeight, dimension); + } -// Method Description: -// - Register any event handlers that we may need with the given TermControl. -// This should be called on each and every TermControl that we add to the tree -// of Panes in this tab. We'll add events too: -// * notify us when the control's title changed, so we can update our own -// title (if necessary) -// Arguments: -// - control: the TermControl to add events to. -// Return Value: -// - -void Tab::_AttachEventHandlersToControl(const TermControl& control) -{ - std::weak_ptr weakThis{ shared_from_this() }; + // Method Description: + // - Update the size of our panes to fill the new given size. This happens when + // the window is resized. + // Arguments: + // - newSize: the amount of space that the panes have to fill now. + // Return Value: + // - + void Tab::ResizeContent(const winrt::Windows::Foundation::Size& newSize) + { + // NOTE: This _must_ be called on the root pane, so that it can propogate + // throughout the entire tree. + _rootPane->ResizeContent(newSize); + } - 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()); - } - }); - - // This is called when the terminal changes its font size or sets it for the first - // time (because when we just create terminal via its ctor it has invalid font size). - // On the latter event, we tell the root pane to resize itself so that its descendants - // (including ourself) can properly snap to character grids. In future, we may also - // want to do that on regular font changes. - control.FontSizeChanged([this](const int /* fontWidth */, - const int /* fontHeight */, - const bool isInitialChange) { - if (isInitialChange) - { - _rootPane->Relayout(); - } - }); -} + // Method Description: + // - Attempt to move a separator between panes, as to resize each child on + // either size of the separator. See Pane::ResizePane for details. + // Arguments: + // - direction: The direction to move the separator in. + // Return Value: + // - + void Tab::ResizePane(const winrt::TerminalApp::Direction& direction) + { + // NOTE: This _must_ be called on the root pane, so that it can propogate + // throughout the entire tree. + _rootPane->ResizePane(direction); + } -// Method Description: -// - Add an event handler to this pane's GotFocus event. When that pane gains -// focus, we'll mark it as the new active pane. We'll also query the title of -// that pane when it's focused to set our own text, and finally, we'll trigger -// our own ActivePaneChanged event. -// Arguments: -// - -// Return Value: -// - -void Tab::_AttachEventHandlersToPane(std::shared_ptr pane) -{ - std::weak_ptr weakThis{ shared_from_this() }; + // Method Description: + // - Attempt to move focus between panes, as to focus the child on + // the other side of the separator. See Pane::NavigateFocus for details. + // Arguments: + // - direction: The direction to move the focus in. + // Return Value: + // - + void Tab::NavigateFocus(const winrt::TerminalApp::Direction& direction) + { + // NOTE: This _must_ be called on the root pane, so that it can propogate + // throughout the entire tree. + _rootPane->NavigateFocus(direction); + } - pane->GotFocus([weakThis](std::shared_ptr sender) { - // Do nothing if the Tab's lifetime is expired or pane isn't new. - auto tab{ weakThis.lock() }; + // Method Description: + // - Closes the currently focused pane in this tab. If it's the last pane in + // this tab, our Closed event will be fired (at a later time) for anyone + // registered as a handler of our close event. + // Arguments: + // - + // Return Value: + // - + void Tab::ClosePane() + { + _activePane->Close(); + } - if (tab && sender != tab->_activePane) - { - // Clear the active state of the entire tree, and mark only the sender as active. - tab->_rootPane->ClearActive(); - tab->_activePane = sender; - tab->_activePane->SetActive(); + // Method Description: + // - Register any event handlers that we may need with the given TermControl. + // This should be called on each and every TermControl that we add to the tree + // of Panes in this tab. We'll add events too: + // * notify us when the control's title changed, so we can update our own + // title (if necessary) + // Arguments: + // - control: the TermControl to add events to. + // Return Value: + // - + void Tab::_AttachEventHandlersToControl(const TermControl& control) + { + auto weakThis{ get_weak() }; + + control.TitleChanged([weakThis](auto newTitle) { + // Check if Tab's lifetime has expired + if (auto tab{ weakThis.get() }) + { + // 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()); + } + }); + + // This is called when the terminal changes its font size or sets it for the first + // time (because when we just create terminal via its ctor it has invalid font size). + // On the latter event, we tell the root pane to resize itself so that its descendants + // (including ourself) can properly snap to character grids. In future, we may also + // want to do that on regular font changes. + control.FontSizeChanged([this](const int /* fontWidth */, + const int /* fontHeight */, + const bool isInitialChange) { + if (isInitialChange) + { + _rootPane->Relayout(); + } + }); + } - // Update our own title text to match the newly-active pane. - tab->SetTabText(tab->GetActiveTitle()); + // Method Description: + // - Add an event handler to this pane's GotFocus event. When that pane gains + // focus, we'll mark it as the new active pane. We'll also query the title of + // that pane when it's focused to set our own text, and finally, we'll trigger + // our own ActivePaneChanged event. + // Arguments: + // - + // Return Value: + // - + void Tab::_AttachEventHandlersToPane(std::shared_ptr pane) + { + auto weakThis{ get_weak() }; + + pane->GotFocus([weakThis](std::shared_ptr sender) { + // Do nothing if the Tab's lifetime is expired or pane isn't new. + auto tab{ weakThis.get() }; + + if (tab && sender != tab->_activePane) + { + // 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. + tab->SetTabText(tab->GetActiveTitle()); + + // Raise our own ActivePaneChanged event. + tab->_ActivePaneChangedHandlers(); + } + }); + } - // Raise our own ActivePaneChanged event. - tab->_ActivePaneChangedHandlers(); - } - }); + DEFINE_EVENT(Tab, ActivePaneChanged, _ActivePaneChangedHandlers, winrt::delegate<>); + DEFINE_EVENT(Tab, PropertyChanged, _PropertyChanged, Windows::UI::Xaml::Data::PropertyChangedEventHandler); } - -DEFINE_EVENT(Tab, ActivePaneChanged, _ActivePaneChangedHandlers, winrt::delegate<>); diff --git a/src/cascadia/TerminalApp/Tab.h b/src/cascadia/TerminalApp/Tab.h index 56a97f75653..ec1ee4915b4 100644 --- a/src/cascadia/TerminalApp/Tab.h +++ b/src/cascadia/TerminalApp/Tab.h @@ -3,56 +3,65 @@ #pragma once #include "Pane.h" +#include "Tab.g.h" -class Tab : public std::enable_shared_from_this +namespace winrt::TerminalApp::implementation { -public: - Tab(const GUID& profile, const winrt::Microsoft::Terminal::TerminalControl::TermControl& control); + struct Tab : public TabT + { + public: + Tab() = delete; + 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; + // 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; - std::optional GetFocusedProfile() const noexcept; + 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; - bool IsFocused() const noexcept; - void SetFocused(const bool focused); + bool IsFocused() const noexcept; + void SetFocused(const bool focused); - winrt::fire_and_forget Scroll(const int delta); + winrt::fire_and_forget Scroll(const int delta); - bool CanSplitPane(winrt::TerminalApp::SplitState splitType); - void SplitPane(winrt::TerminalApp::SplitState splitType, const GUID& profile, winrt::Microsoft::Terminal::TerminalControl::TermControl& control); + bool CanSplitPane(winrt::TerminalApp::SplitState splitType); + void SplitPane(winrt::TerminalApp::SplitState splitType, const GUID& profile, winrt::Microsoft::Terminal::TerminalControl::TermControl& control); - winrt::fire_and_forget UpdateIcon(const winrt::hstring iconPath); + winrt::fire_and_forget UpdateIcon(const winrt::hstring iconPath); - float CalcSnappedDimension(const bool widthOrHeight, const float dimension) const; + float CalcSnappedDimension(const bool widthOrHeight, const float dimension) const; - void ResizeContent(const winrt::Windows::Foundation::Size& newSize); - void ResizePane(const winrt::TerminalApp::Direction& direction); - void NavigateFocus(const winrt::TerminalApp::Direction& direction); + void ResizeContent(const winrt::Windows::Foundation::Size& newSize); + void ResizePane(const winrt::TerminalApp::Direction& direction); + void NavigateFocus(const winrt::TerminalApp::Direction& direction); - void UpdateSettings(const winrt::Microsoft::Terminal::Settings::TerminalSettings& settings, const GUID& profile); - winrt::hstring GetActiveTitle() const; - winrt::fire_and_forget SetTabText(const winrt::hstring text); + void UpdateSettings(const winrt::Microsoft::Terminal::Settings::TerminalSettings& settings, const GUID& profile); + winrt::hstring GetActiveTitle() const; + winrt::fire_and_forget SetTabText(const winrt::hstring text); - void ClosePane(); + void ClosePane(); - WINRT_CALLBACK(Closed, winrt::Windows::Foundation::EventHandler); - DECLARE_EVENT(ActivePaneChanged, _ActivePaneChangedHandlers, winrt::delegate<>); + WINRT_CALLBACK(Closed, winrt::Windows::Foundation::EventHandler); + DECLARE_EVENT(ActivePaneChanged, _ActivePaneChangedHandlers, winrt::delegate<>); -private: - std::shared_ptr _rootPane{ nullptr }; - std::shared_ptr _activePane{ nullptr }; - winrt::hstring _lastIconPath{}; + DECLARE_EVENT(PropertyChanged, _PropertyChanged, Windows::UI::Xaml::Data::PropertyChangedEventHandler); + DEFINE_OBSERVABLE_GETSET_PROPERTY(hstring, Title, _PropertyChanged); + DEFINE_OBSERVABLE_GETSET_PROPERTY(hstring, IconPath, _PropertyChanged); - bool _focused{ false }; - winrt::Microsoft::UI::Xaml::Controls::TabViewItem _tabViewItem{ nullptr }; + private: + std::shared_ptr _rootPane{ nullptr }; + std::shared_ptr _activePane{ nullptr }; + winrt::hstring _lastIconPath{}; - void _MakeTabViewItem(); - void _Focus(); + bool _focused{ false }; + winrt::Microsoft::UI::Xaml::Controls::TabViewItem _tabViewItem{ nullptr }; - void _AttachEventHandlersToControl(const winrt::Microsoft::Terminal::TerminalControl::TermControl& control); - void _AttachEventHandlersToPane(std::shared_ptr pane); -}; + void _MakeTabViewItem(); + void _Focus(); + + void _AttachEventHandlersToControl(const winrt::Microsoft::Terminal::TerminalControl::TermControl& control); + void _AttachEventHandlersToPane(std::shared_ptr pane); + }; +} diff --git a/src/cascadia/TerminalApp/Tab.idl b/src/cascadia/TerminalApp/Tab.idl new file mode 100644 index 00000000000..303e95fce4f --- /dev/null +++ b/src/cascadia/TerminalApp/Tab.idl @@ -0,0 +1,11 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +namespace TerminalApp +{ + [default_interface] runtimeclass Tab : Windows.UI.Xaml.Data.INotifyPropertyChanged + { + String Title; + String IconPath; + } +} diff --git a/src/cascadia/TerminalApp/TerminalApp.vcxproj b/src/cascadia/TerminalApp/TerminalApp.vcxproj index 8c9a2861c2b..de2e646443f 100644 --- a/src/cascadia/TerminalApp/TerminalApp.vcxproj +++ b/src/cascadia/TerminalApp/TerminalApp.vcxproj @@ -35,6 +35,7 @@ + diff --git a/src/cascadia/TerminalApp/lib/TerminalAppLib.vcxproj b/src/cascadia/TerminalApp/lib/TerminalAppLib.vcxproj index 358feab1d28..0977978abe5 100644 --- a/src/cascadia/TerminalApp/lib/TerminalAppLib.vcxproj +++ b/src/cascadia/TerminalApp/lib/TerminalAppLib.vcxproj @@ -66,7 +66,9 @@ ../TabRowControl.xaml - + + ../Tab.idl + @@ -118,7 +120,9 @@ ../TabRowControl.xaml - + + ../Tab.idl + @@ -192,6 +196,7 @@ ../TabRowControl.xaml Code + @@ -336,4 +341,4 @@ - + \ No newline at end of file diff --git a/src/cascadia/TerminalApp/lib/TerminalAppLib.vcxproj.filters b/src/cascadia/TerminalApp/lib/TerminalAppLib.vcxproj.filters index 03e86156bdb..1a395f3ef59 100644 --- a/src/cascadia/TerminalApp/lib/TerminalAppLib.vcxproj.filters +++ b/src/cascadia/TerminalApp/lib/TerminalAppLib.vcxproj.filters @@ -121,6 +121,9 @@ settings + + tab + diff --git a/src/cascadia/inc/cppwinrt_utils.h b/src/cascadia/inc/cppwinrt_utils.h index a9a5cada77c..ff2dfa89435 100644 --- a/src/cascadia/inc/cppwinrt_utils.h +++ b/src/cascadia/inc/cppwinrt_utils.h @@ -107,6 +107,18 @@ public: \ private: \ type _##name{ __VA_ARGS__ }; +#define DEFINE_OBSERVABLE_GETSET_PROPERTY(type, name, event) \ +public: \ + type name() { return _##name; }; \ + void name(const type& value) \ + { \ + _##name = value; \ + event(*this, Windows::UI::Xaml::Data::PropertyChangedEventArgs{ L#name }); \ + }; \ + \ +private: \ + type _##name; + // Use this macro for quickly defining the factory_implementation part of a // class. CppWinrt requires these for the compiler, but more often than not, // they require no customization. See From ae2b0d5baacf3d8ded2be5a8988259108d8b6cc6 Mon Sep 17 00:00:00 2001 From: Leon Liang Date: Wed, 22 Jan 2020 16:31:21 -0800 Subject: [PATCH 02/17] changed test case to use com_ptr outside the lambda to heed the warning --- src/cascadia/LocalTests_TerminalApp/TabTests.cpp | 8 ++++---- src/cascadia/TerminalApp/Tab.cpp | 2 ++ 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/cascadia/LocalTests_TerminalApp/TabTests.cpp b/src/cascadia/LocalTests_TerminalApp/TabTests.cpp index 42acb7946b5..0a18ba3a833 100644 --- a/src/cascadia/LocalTests_TerminalApp/TabTests.cpp +++ b/src/cascadia/LocalTests_TerminalApp/TabTests.cpp @@ -115,12 +115,12 @@ namespace TerminalAppLocalTests void TabTests::TryCreateTab() { - // If you leave the Tab shared_ptr owned by the RunOnUIThread lambda, it + // If you leave the Tab ptr owned by the RunOnUIThread lambda, it // will crash when the test tears down. Not totally clear why, but make // sure it's owned outside the lambda - /*winrt::TerminalApp::Tab*/ + winrt::com_ptr newTab{ nullptr }; - auto result = RunOnUIThread([]() { + auto result = RunOnUIThread([&newTab]() { // Try creating all of: // 1. one of our pure c++ types (Profile) // 2. one of our c++winrt types (TerminalSettings, EchoConnection) @@ -135,7 +135,7 @@ namespace TerminalAppLocalTests winrt::Microsoft::Terminal::TerminalControl::TermControl term{ settings, conn }; VERIFY_IS_NOT_NULL(term); - auto newTab = winrt::make_self(profileGuid, term); + newTab = winrt::make_self(profileGuid, term); VERIFY_IS_NOT_NULL(newTab); }); diff --git a/src/cascadia/TerminalApp/Tab.cpp b/src/cascadia/TerminalApp/Tab.cpp index 52e36a706c6..0318e02aa90 100644 --- a/src/cascadia/TerminalApp/Tab.cpp +++ b/src/cascadia/TerminalApp/Tab.cpp @@ -166,6 +166,7 @@ namespace winrt::TerminalApp::implementation if (auto tab{ weakThis.get() }) { + IconPath(_lastIconPath); _tabViewItem.IconSource(GetColoredIcon(_lastIconPath)); } } @@ -199,6 +200,7 @@ namespace winrt::TerminalApp::implementation if (auto tab{ weakThis.get() }) { + Title(text); _tabViewItem.Header(winrt::box_value(text)); } } From b1324751d255258c43b6563ed058176733fe7ef2 Mon Sep 17 00:00:00 2001 From: Leon Liang Date: Wed, 22 Jan 2020 16:46:59 -0800 Subject: [PATCH 03/17] added a comment to the observable getset property macro --- src/cascadia/inc/cppwinrt_utils.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/cascadia/inc/cppwinrt_utils.h b/src/cascadia/inc/cppwinrt_utils.h index ff2dfa89435..195c3b0129f 100644 --- a/src/cascadia/inc/cppwinrt_utils.h +++ b/src/cascadia/inc/cppwinrt_utils.h @@ -107,6 +107,9 @@ public: \ private: \ type _##name{ __VA_ARGS__ }; +// Use this macro to quickly implement both the getter and setter for an observable property. +// This is similar to the GETSET_PROPERTY macro above, except this will also raise a +// PropertyChanged event with the name of the property that has changed inside of the settter. #define DEFINE_OBSERVABLE_GETSET_PROPERTY(type, name, event) \ public: \ type name() { return _##name; }; \ From 1afe573f0dda6a364b753b997d8099bc447a795c Mon Sep 17 00:00:00 2001 From: Leon Liang Date: Thu, 23 Jan 2020 15:54:06 -0800 Subject: [PATCH 04/17] modified TerminalPage to use an ObservableVector of WinRT tabs --- src/cascadia/TerminalApp/TerminalPage.cpp | 125 ++++++++++++---------- src/cascadia/TerminalApp/TerminalPage.h | 8 +- 2 files changed, 75 insertions(+), 58 deletions(-) diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index 70e98b95676..580e30b4ba3 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -39,7 +39,7 @@ namespace winrt namespace winrt::TerminalApp::implementation { TerminalPage::TerminalPage() : - _tabs{} + _tabs{ winrt::single_threaded_observable_vector() } { InitializeComponent(); } @@ -81,9 +81,9 @@ namespace winrt::TerminalApp::implementation if (from.has_value() && to.has_value() && to != from) { auto& tabs{ page->_tabs }; - auto tab = tabs.at(from.value()); - tabs.erase(tabs.begin() + from.value()); - tabs.insert(tabs.begin() + to.value(), tab); + auto tab = tabs.GetAt(from.value()); + tabs.RemoveAt(from.value()); + tabs.InsertAt(to.value(), tab); } page->_rearranging = false; @@ -411,8 +411,8 @@ namespace winrt::TerminalApp::implementation const auto [profileGuid, settings] = _settings->BuildSettings(newTerminalArgs); _CreateNewTabFromSettings(profileGuid, settings); - - const int tabCount = static_cast(_tabs.size()); + + const int tabCount = static_cast(_tabs.Size()); const bool usedManualProfile = (newTerminalArgs != nullptr) && (newTerminalArgs.ProfileIndex() != nullptr || newTerminalArgs.Profile().empty()); @@ -449,20 +449,22 @@ namespace winrt::TerminalApp::implementation TermControl term{ settings, connection }; // Add the new tab to the list of our tabs. - auto newTab = _tabs.emplace_back(std::make_shared(profileGuid, term)); + auto newTabProjection = winrt::make(profileGuid, term); + _tabs.Append(newTabProjection); // Hookup our event handlers to the new terminal - _RegisterTerminalEvents(term, newTab); + _RegisterTerminalEvents(term, newTabProjection); // 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 weakTabPtr = newTab; + auto weakTab = make_weak(newTabProjection); // 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([weakTabPtr, weakThis{ get_weak() }]() { + auto newTabImpl = winrt::get_self(newTabProjection); + newTabImpl->ActivePaneChanged([weakTab, weakThis{ get_weak() }]() { auto page{ weakThis.get() }; - auto tab{ weakTabPtr.lock() }; + auto tab{ weakTab.get() }; if (page && tab) { @@ -474,20 +476,20 @@ namespace winrt::TerminalApp::implementation } }); - auto tabViewItem = newTab->GetTabViewItem(); + auto tabViewItem = newTabImpl->GetTabViewItem(); _tabView.TabItems().Append(tabViewItem); // Set this tab's icon to the icon from the user's profile const auto* const profile = _settings->FindProfile(profileGuid); if (profile != nullptr && profile->HasIcon()) { - newTab->UpdateIcon(profile->GetExpandedIconPath()); + newTabImpl->UpdateIcon(profile->GetExpandedIconPath()); } tabViewItem.PointerPressed({ this, &TerminalPage::_OnTabClick }); // When the tab is closed, remove it from our list of tabs. - newTab->Closed([tabViewItem, weakThis{ get_weak() }](auto&& /*s*/, auto&& /*e*/) { + newTabImpl->Closed([tabViewItem, weakThis{ get_weak() }](auto&& /*s*/, auto&& /*e*/) { if (auto page{ weakThis.get() }) { page->_RemoveOnCloseRoutine(tabViewItem, page); @@ -658,12 +660,14 @@ namespace winrt::TerminalApp::implementation // TitleChanged event. // Arguments: // - tab: the Tab to update the title for. - void TerminalPage::_UpdateTitle(std::shared_ptr tab) + void TerminalPage::_UpdateTitle(winrt::TerminalApp::Tab tab) { - auto newTabTitle = tab->GetActiveTitle(); + auto tabImpl = winrt::get_self(tab); + + auto newTabTitle = tabImpl->GetActiveTitle(); if (_settings->GlobalSettings().GetShowTitleInTitlebar() && - tab->IsFocused()) + tabImpl->IsFocused()) { _titleChangeHandlers(*this, newTabTitle); } @@ -674,20 +678,21 @@ namespace winrt::TerminalApp::implementation // tab's icon to that icon. // Arguments: // - tab: the Tab to update the title for. - void TerminalPage::_UpdateTabIcon(std::shared_ptr tab) + void TerminalPage::_UpdateTabIcon(winrt::TerminalApp::Tab tab) { - const auto lastFocusedProfileOpt = tab->GetFocusedProfile(); + auto tabImpl = winrt::get_self(tab); + const auto lastFocusedProfileOpt = tabImpl->GetFocusedProfile(); if (lastFocusedProfileOpt.has_value()) { const auto lastFocusedProfile = lastFocusedProfileOpt.value(); const auto* const matchingProfile = _settings->FindProfile(lastFocusedProfile); if (matchingProfile) { - tab->UpdateIcon(matchingProfile->GetExpandedIconPath()); + tabImpl->UpdateIcon(matchingProfile->GetExpandedIconPath()); } else { - tab->UpdateIcon({}); + tabImpl->UpdateIcon({}); } } } @@ -708,7 +713,7 @@ namespace winrt::TerminalApp::implementation // show the tab bar. const bool isVisible = (!_isFullscreen) && (_settings->GlobalSettings().GetShowTabsInTitlebar() || - (_tabs.size() > 1) || + (_tabs.Size() > 1) || _settings->GlobalSettings().GetAlwaysShowTabs()); // collapse/show the tabs themselves @@ -724,7 +729,7 @@ namespace winrt::TerminalApp::implementation void TerminalPage::_DuplicateTabViewItem() { const int& focusedTabIndex = _GetFocusedTabIndex(); - const auto& _tab = _tabs.at(focusedTabIndex); + const auto& _tab = winrt::get_self(_tabs.GetAt(focusedTabIndex)); const auto& profileGuid = _tab->GetFocusedProfile(); if (profileGuid.has_value()) @@ -754,19 +759,19 @@ namespace winrt::TerminalApp::implementation void TerminalPage::_RemoveTabViewItemByIndex(uint32_t tabIndex) { // To close the window here, we need to close the hosting window. - if (_tabs.size() == 1) + if (_tabs.Size() == 1) { _lastTabClosedHandlers(*this, nullptr); } // Removing the tab from the collection will destroy its control and disconnect its connection. - _tabs.erase(_tabs.begin() + tabIndex); + _tabs.RemoveAt(tabIndex); _tabView.TabItems().RemoveAt(tabIndex); auto focusedTabIndex = _GetFocusedTabIndex(); if (gsl::narrow_cast(tabIndex) == focusedTabIndex) { - auto const tabCount = gsl::narrow_cast(_tabs.size()); + auto const tabCount = gsl::narrow_cast(_tabs.Size()); if (focusedTabIndex >= tabCount) { focusedTabIndex = tabCount - 1; @@ -789,8 +794,10 @@ namespace winrt::TerminalApp::implementation // Arguments: // - term: The newly created TermControl to connect the events for // - hostingTab: The Tab that's hosting this TermControl instance - void TerminalPage::_RegisterTerminalEvents(TermControl term, std::shared_ptr hostingTab) + void TerminalPage::_RegisterTerminalEvents(TermControl term, winrt::TerminalApp::Tab hostingTab) { + auto tabImpl = winrt::get_self(hostingTab); + // Add an event handler when the terminal's selection wants to be copied. // When the text buffer data is retrieved, we'll copy the data into the Clipboard term.CopyToClipboard({ this, &TerminalPage::_CopyToClipboardHandler }); @@ -799,15 +806,15 @@ namespace winrt::TerminalApp::implementation term.PasteFromClipboard({ this, &TerminalPage::_PasteFromClipboardHandler }); // Bind Tab events to the TermControl and the Tab's Pane - hostingTab->BindEventHandlers(term); + tabImpl->BindEventHandlers(term); // 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 weakTabPtr = hostingTab; + auto weakTab = make_weak(hostingTab); - term.TitleChanged([weakTabPtr, weakThis{ get_weak() }](auto newTitle) { + term.TitleChanged([weakTab, weakThis{ get_weak() }](auto newTitle) { auto page{ weakThis.get() }; - auto tab{ weakTabPtr.lock() }; + auto tab{ weakTab.get() }; if (page && tab) { @@ -824,7 +831,7 @@ namespace winrt::TerminalApp::implementation void TerminalPage::_SelectNextTab(const bool bMoveRight) { int focusedTabIndex = _GetFocusedTabIndex(); - auto tabCount = _tabs.size(); + auto tabCount = _tabs.Size(); // Wraparound math. By adding tabCount and then calculating modulo tabCount, // we clamp the values to the range [0, tabCount) while still supporting moving // leftward from 0 to tabCount - 1. @@ -839,7 +846,7 @@ namespace winrt::TerminalApp::implementation // true iff we were able to select that tab index, false otherwise bool TerminalPage::_SelectTab(const int tabIndex) { - if (tabIndex >= 0 && tabIndex < gsl::narrow_cast(_tabs.size())) + if (tabIndex >= 0 && tabIndex < gsl::narrow_cast(_tabs.Size())) { _SetFocusedTabIndex(tabIndex); return true; @@ -858,13 +865,14 @@ namespace winrt::TerminalApp::implementation void TerminalPage::_MoveFocus(const Direction& direction) { const auto focusedTabIndex = _GetFocusedTabIndex(); - _tabs[focusedTabIndex]->NavigateFocus(direction); + auto focusedTab = winrt::get_self(_tabs.GetAt(focusedTabIndex)); + focusedTab->NavigateFocus(direction); } winrt::Microsoft::Terminal::TerminalControl::TermControl TerminalPage::_GetActiveControl() { int focusedTabIndex = _GetFocusedTabIndex(); - auto focusedTab = _tabs[focusedTabIndex]; + auto focusedTab = winrt::get_self(_tabs.GetAt(focusedTabIndex)); return focusedTab->GetActiveTerminalControl(); } @@ -895,7 +903,7 @@ namespace winrt::TerminalApp::implementation if (auto page{ weakThis.get() }) { - auto tab = _tabs.at(tabIndex); + auto tab = winrt::get_self(_tabs.GetAt(tabIndex)); _tabView.SelectedItem(tab->GetTabViewItem()); } } @@ -915,7 +923,7 @@ namespace winrt::TerminalApp::implementation void TerminalPage::_CloseFocusedPane() { int focusedTabIndex = _GetFocusedTabIndex(); - std::shared_ptr focusedTab{ _tabs[focusedTabIndex] }; + auto focusedTab{ winrt::get_self(_tabs.GetAt(focusedTabIndex)) }; focusedTab->ClosePane(); } @@ -924,7 +932,7 @@ namespace winrt::TerminalApp::implementation // than one tab opened, show a warning dialog. void TerminalPage::CloseWindow() { - if (_tabs.size() > 1) + if (_tabs.Size() > 1) { _ShowCloseWarningDialog(); } @@ -939,7 +947,7 @@ namespace winrt::TerminalApp::implementation // on its own when the last tab is closed. void TerminalPage::_CloseAllTabs() { - while (!_tabs.empty()) + while (_tabs.Size() != 0) { _RemoveTabViewItemByIndex(0); } @@ -954,7 +962,8 @@ namespace winrt::TerminalApp::implementation void TerminalPage::_Scroll(int delta) { int focusedTabIndex = _GetFocusedTabIndex(); - _tabs[focusedTabIndex]->Scroll(delta); + auto tab = winrt::get_self(_tabs.GetAt(focusedTabIndex)); + tab->Scroll(delta); } // Method Description: @@ -981,9 +990,10 @@ namespace winrt::TerminalApp::implementation const auto controlConnection = _CreateConnectionFromSettings(realGuid, controlSettings); const int focusedTabIndex = _GetFocusedTabIndex(); - auto focusedTab = _tabs[focusedTabIndex]; + auto tabProj = _tabs.GetAt(focusedTabIndex); + auto tabImpl = winrt::get_self(tabProj); - const auto canSplit = focusedTab->CanSplitPane(splitType); + const auto canSplit = tabImpl->CanSplitPane(splitType); if (!canSplit) { @@ -993,9 +1003,9 @@ namespace winrt::TerminalApp::implementation TermControl newControl{ controlSettings, controlConnection }; // Hookup our event handlers to the new terminal - _RegisterTerminalEvents(newControl, focusedTab); + _RegisterTerminalEvents(newControl, tabProj); - focusedTab->SplitPane(splitType, realGuid, newControl); + tabImpl->SplitPane(splitType, realGuid, newControl); } // Method Description: @@ -1009,7 +1019,8 @@ namespace winrt::TerminalApp::implementation void TerminalPage::_ResizePane(const Direction& direction) { const auto focusedTabIndex = _GetFocusedTabIndex(); - _tabs[focusedTabIndex]->ResizePane(direction); + auto tab = winrt::get_self(_tabs.GetAt(focusedTabIndex)); + tab->ResizePane(direction); } // Method Description: @@ -1026,7 +1037,9 @@ namespace winrt::TerminalApp::implementation const auto focusedTabIndex = _GetFocusedTabIndex(); const auto control = _GetActiveControl(); const auto termHeight = control.GetViewHeight(); - _tabs[focusedTabIndex]->Scroll(termHeight * delta); + + auto tab = winrt::get_self(_tabs.GetAt(focusedTabIndex)); + tab->Scroll(termHeight * delta); } // Method Description: @@ -1136,7 +1149,8 @@ namespace winrt::TerminalApp::implementation if (_settings->GlobalSettings().SnapToGridOnResize()) { const auto focusedTabIndex = _GetFocusedTabIndex(); - return _tabs[focusedTabIndex]->CalcSnappedDimension(widthOrHeight, dimension); + auto tab = winrt::get_self(_tabs.GetAt(focusedTabIndex)); + return tab->CalcSnappedDimension(widthOrHeight, dimension); } else { @@ -1317,14 +1331,15 @@ namespace winrt::TerminalApp::implementation // Unfocus all the tabs. for (auto tab : _tabs) { - tab->SetFocused(false); + auto tabImpl = winrt::get_self(tab); + tabImpl->SetFocused(false); } if (selectedIndex >= 0) { try { - auto tab = _tabs.at(selectedIndex); + auto tab = winrt::get_self(_tabs.GetAt(selectedIndex)); _tabContent.Children().Clear(); _tabContent.Children().Append(tab->GetRootElement()); @@ -1348,9 +1363,10 @@ namespace winrt::TerminalApp::implementation void TerminalPage::_OnContentSizeChanged(const IInspectable& /*sender*/, Windows::UI::Xaml::SizeChangedEventArgs const& e) { const auto newSize = e.NewSize(); - for (auto& tab : _tabs) + for (auto tab : _tabs) { - tab->ResizeContent(newSize); + auto tabImpl = winrt::get_self(tab); + tabImpl->ResizeContent(newSize); } } @@ -1400,15 +1416,16 @@ namespace winrt::TerminalApp::implementation const GUID profileGuid = profile.GetGuid(); const auto settings = _settings->BuildSettings(profileGuid); - for (auto& tab : _tabs) + for (auto tab : _tabs) { // Attempt to reload the settings of any panes with this profile - tab->UpdateSettings(settings, profileGuid); + auto tabImpl = winrt::get_self(tab); + tabImpl->UpdateSettings(settings, profileGuid); } } // Update the icon of the tab for the currently focused profile in that tab. - for (auto& tab : _tabs) + for (auto tab : _tabs) { _UpdateTabIcon(tab); _UpdateTitle(tab); diff --git a/src/cascadia/TerminalApp/TerminalPage.h b/src/cascadia/TerminalApp/TerminalPage.h index 10278ac7269..8c19a91b5bc 100644 --- a/src/cascadia/TerminalApp/TerminalPage.h +++ b/src/cascadia/TerminalApp/TerminalPage.h @@ -60,7 +60,7 @@ namespace winrt::TerminalApp::implementation std::shared_ptr<::TerminalApp::CascadiaSettings> _settings{ nullptr }; - std::vector> _tabs; + Windows::Foundation::Collections::IObservableVector _tabs; bool _isFullscreen{ false }; @@ -87,15 +87,15 @@ namespace winrt::TerminalApp::implementation void _HookupKeyBindings(TerminalApp::AppKeyBindings bindings) noexcept; void _RegisterActionCallbacks(); - void _UpdateTitle(std::shared_ptr tab); - void _UpdateTabIcon(std::shared_ptr tab); + void _UpdateTitle(winrt::TerminalApp::Tab tab); + void _UpdateTabIcon(winrt::TerminalApp::Tab tab); void _UpdateTabView(); void _UpdateTabWidthMode(); void _DuplicateTabViewItem(); void _RemoveTabViewItem(const Microsoft::UI::Xaml::Controls::TabViewItem& tabViewItem); void _RemoveTabViewItemByIndex(uint32_t tabIndex); - void _RegisterTerminalEvents(Microsoft::Terminal::TerminalControl::TermControl term, std::shared_ptr hostingTab); + void _RegisterTerminalEvents(Microsoft::Terminal::TerminalControl::TermControl term, winrt::TerminalApp::Tab hostingTab); void _SelectNextTab(const bool bMoveRight); bool _SelectTab(const int tabIndex); From 571812798e47b71e9a4e1f6b8126dc868fd78418 Mon Sep 17 00:00:00 2001 From: Leon Liang Date: Mon, 27 Jan 2020 14:18:46 -0800 Subject: [PATCH 05/17] changing macro name, using winrt_callback macro, and changing RegisterTerminalEvents signature --- src/cascadia/TerminalApp/Tab.cpp | 1 - src/cascadia/TerminalApp/Tab.h | 6 ++--- src/cascadia/TerminalApp/TerminalPage.cpp | 31 +++++++++++++---------- src/cascadia/TerminalApp/TerminalPage.h | 2 +- src/cascadia/inc/cppwinrt_utils.h | 2 +- 5 files changed, 23 insertions(+), 19 deletions(-) diff --git a/src/cascadia/TerminalApp/Tab.cpp b/src/cascadia/TerminalApp/Tab.cpp index 0318e02aa90..be195cfe97c 100644 --- a/src/cascadia/TerminalApp/Tab.cpp +++ b/src/cascadia/TerminalApp/Tab.cpp @@ -390,5 +390,4 @@ namespace winrt::TerminalApp::implementation } DEFINE_EVENT(Tab, ActivePaneChanged, _ActivePaneChangedHandlers, winrt::delegate<>); - DEFINE_EVENT(Tab, PropertyChanged, _PropertyChanged, Windows::UI::Xaml::Data::PropertyChangedEventHandler); } diff --git a/src/cascadia/TerminalApp/Tab.h b/src/cascadia/TerminalApp/Tab.h index ec1ee4915b4..4cd89871d2f 100644 --- a/src/cascadia/TerminalApp/Tab.h +++ b/src/cascadia/TerminalApp/Tab.h @@ -46,9 +46,9 @@ namespace winrt::TerminalApp::implementation WINRT_CALLBACK(Closed, winrt::Windows::Foundation::EventHandler); DECLARE_EVENT(ActivePaneChanged, _ActivePaneChangedHandlers, winrt::delegate<>); - DECLARE_EVENT(PropertyChanged, _PropertyChanged, Windows::UI::Xaml::Data::PropertyChangedEventHandler); - DEFINE_OBSERVABLE_GETSET_PROPERTY(hstring, Title, _PropertyChanged); - DEFINE_OBSERVABLE_GETSET_PROPERTY(hstring, IconPath, _PropertyChanged); + WINRT_CALLBACK(PropertyChanged, Windows::UI::Xaml::Data::PropertyChangedEventHandler); + OBSERVABLE_GETSET_PROPERTY(hstring, Title, _PropertyChangedHandlers); + OBSERVABLE_GETSET_PROPERTY(hstring, IconPath, _PropertyChangedHandlers); private: std::shared_ptr _rootPane{ nullptr }; diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index 580e30b4ba3..62e2b43cf18 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -452,8 +452,11 @@ namespace winrt::TerminalApp::implementation auto newTabProjection = winrt::make(profileGuid, term); _tabs.Append(newTabProjection); + winrt::com_ptr newTabImpl; + newTabImpl.copy_from(winrt::get_self(newTabProjection)); + // Hookup our event handlers to the new terminal - _RegisterTerminalEvents(term, newTabProjection); + _RegisterTerminalEvents(term, newTabImpl); // 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. @@ -461,7 +464,6 @@ namespace winrt::TerminalApp::implementation // 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. - auto newTabImpl = winrt::get_self(newTabProjection); newTabImpl->ActivePaneChanged([weakTab, weakThis{ get_weak() }]() { auto page{ weakThis.get() }; auto tab{ weakTab.get() }; @@ -662,12 +664,13 @@ namespace winrt::TerminalApp::implementation // - tab: the Tab to update the title for. void TerminalPage::_UpdateTitle(winrt::TerminalApp::Tab tab) { - auto tabImpl = winrt::get_self(tab); + winrt::com_ptr newTabImpl; + newTabImpl.copy_from(winrt::get_self(tab)); - auto newTabTitle = tabImpl->GetActiveTitle(); + auto newTabTitle = newTabImpl->GetActiveTitle(); if (_settings->GlobalSettings().GetShowTitleInTitlebar() && - tabImpl->IsFocused()) + newTabImpl->IsFocused()) { _titleChangeHandlers(*this, newTabTitle); } @@ -680,7 +683,9 @@ namespace winrt::TerminalApp::implementation // - tab: the Tab to update the title for. void TerminalPage::_UpdateTabIcon(winrt::TerminalApp::Tab tab) { - auto tabImpl = winrt::get_self(tab); + winrt::com_ptr tabImpl; + tabImpl.copy_from(winrt::get_self(tab)); + const auto lastFocusedProfileOpt = tabImpl->GetFocusedProfile(); if (lastFocusedProfileOpt.has_value()) { @@ -794,10 +799,8 @@ namespace winrt::TerminalApp::implementation // Arguments: // - term: The newly created TermControl to connect the events for // - hostingTab: The Tab that's hosting this TermControl instance - void TerminalPage::_RegisterTerminalEvents(TermControl term, winrt::TerminalApp::Tab hostingTab) + void TerminalPage::_RegisterTerminalEvents(TermControl term, winrt::com_ptr hostingTab) { - auto tabImpl = winrt::get_self(hostingTab); - // Add an event handler when the terminal's selection wants to be copied. // When the text buffer data is retrieved, we'll copy the data into the Clipboard term.CopyToClipboard({ this, &TerminalPage::_CopyToClipboardHandler }); @@ -806,11 +809,11 @@ namespace winrt::TerminalApp::implementation term.PasteFromClipboard({ this, &TerminalPage::_PasteFromClipboardHandler }); // Bind Tab events to the TermControl and the Tab's Pane - tabImpl->BindEventHandlers(term); + hostingTab->BindEventHandlers(term); // 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. - auto weakTab = make_weak(hostingTab); + auto weakTab = make_weak(hostingTab.as()); term.TitleChanged([weakTab, weakThis{ get_weak() }](auto newTitle) { auto page{ weakThis.get() }; @@ -991,7 +994,9 @@ namespace winrt::TerminalApp::implementation const int focusedTabIndex = _GetFocusedTabIndex(); auto tabProj = _tabs.GetAt(focusedTabIndex); - auto tabImpl = winrt::get_self(tabProj); + + winrt::com_ptr tabImpl; + tabImpl.copy_from(winrt::get_self(tabProj)); const auto canSplit = tabImpl->CanSplitPane(splitType); @@ -1003,7 +1008,7 @@ namespace winrt::TerminalApp::implementation TermControl newControl{ controlSettings, controlConnection }; // Hookup our event handlers to the new terminal - _RegisterTerminalEvents(newControl, tabProj); + _RegisterTerminalEvents(newControl, tabImpl); tabImpl->SplitPane(splitType, realGuid, newControl); } diff --git a/src/cascadia/TerminalApp/TerminalPage.h b/src/cascadia/TerminalApp/TerminalPage.h index 8c19a91b5bc..43e85601124 100644 --- a/src/cascadia/TerminalApp/TerminalPage.h +++ b/src/cascadia/TerminalApp/TerminalPage.h @@ -95,7 +95,7 @@ namespace winrt::TerminalApp::implementation void _RemoveTabViewItem(const Microsoft::UI::Xaml::Controls::TabViewItem& tabViewItem); void _RemoveTabViewItemByIndex(uint32_t tabIndex); - void _RegisterTerminalEvents(Microsoft::Terminal::TerminalControl::TermControl term, winrt::TerminalApp::Tab hostingTab); + void _RegisterTerminalEvents(Microsoft::Terminal::TerminalControl::TermControl term, winrt::com_ptr hostingTab); void _SelectNextTab(const bool bMoveRight); bool _SelectTab(const int tabIndex); diff --git a/src/cascadia/inc/cppwinrt_utils.h b/src/cascadia/inc/cppwinrt_utils.h index 195c3b0129f..b4a46e3bb58 100644 --- a/src/cascadia/inc/cppwinrt_utils.h +++ b/src/cascadia/inc/cppwinrt_utils.h @@ -110,7 +110,7 @@ private: \ // Use this macro to quickly implement both the getter and setter for an observable property. // This is similar to the GETSET_PROPERTY macro above, except this will also raise a // PropertyChanged event with the name of the property that has changed inside of the settter. -#define DEFINE_OBSERVABLE_GETSET_PROPERTY(type, name, event) \ +#define OBSERVABLE_GETSET_PROPERTY(type, name, event) \ public: \ type name() { return _##name; }; \ void name(const type& value) \ From f608b3cf2c5f6049d311e5f10455a5cd9ee1df7a Mon Sep 17 00:00:00 2001 From: Leon Liang Date: Mon, 27 Jan 2020 14:44:49 -0800 Subject: [PATCH 06/17] formatting --- src/cascadia/TerminalApp/Tab.h | 78 +++++++++++------------ src/cascadia/TerminalApp/TerminalPage.cpp | 2 +- src/cascadia/inc/cppwinrt_utils.h | 2 +- 3 files changed, 41 insertions(+), 41 deletions(-) diff --git a/src/cascadia/TerminalApp/Tab.h b/src/cascadia/TerminalApp/Tab.h index 57532fb9d73..45d09b8b304 100644 --- a/src/cascadia/TerminalApp/Tab.h +++ b/src/cascadia/TerminalApp/Tab.h @@ -9,60 +9,60 @@ namespace winrt::TerminalApp::implementation { struct Tab : public TabT { - public: - Tab() = delete; - Tab(const GUID& profile, const winrt::Microsoft::Terminal::TerminalControl::TermControl& control); + public: + Tab() = delete; + 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; + // 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; - std::optional GetFocusedProfile() const noexcept; + 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; - bool IsFocused() const noexcept; - void SetFocused(const bool focused); + bool IsFocused() const noexcept; + void SetFocused(const bool focused); - winrt::fire_and_forget Scroll(const int delta); + winrt::fire_and_forget Scroll(const int delta); - bool CanSplitPane(winrt::TerminalApp::SplitState splitType); - void SplitPane(winrt::TerminalApp::SplitState splitType, const GUID& profile, winrt::Microsoft::Terminal::TerminalControl::TermControl& control); + bool CanSplitPane(winrt::TerminalApp::SplitState splitType); + void SplitPane(winrt::TerminalApp::SplitState splitType, const GUID& profile, winrt::Microsoft::Terminal::TerminalControl::TermControl& control); - winrt::fire_and_forget UpdateIcon(const winrt::hstring iconPath); + winrt::fire_and_forget UpdateIcon(const winrt::hstring iconPath); - float CalcSnappedDimension(const bool widthOrHeight, const float dimension) const; + float CalcSnappedDimension(const bool widthOrHeight, const float dimension) const; - void ResizeContent(const winrt::Windows::Foundation::Size& newSize); - void ResizePane(const winrt::TerminalApp::Direction& direction); - void NavigateFocus(const winrt::TerminalApp::Direction& direction); + void ResizeContent(const winrt::Windows::Foundation::Size& newSize); + void ResizePane(const winrt::TerminalApp::Direction& direction); + void NavigateFocus(const winrt::TerminalApp::Direction& direction); - void UpdateSettings(const winrt::Microsoft::Terminal::Settings::TerminalSettings& settings, const GUID& profile); - winrt::hstring GetActiveTitle() const; - winrt::fire_and_forget SetTabText(const winrt::hstring text); + void UpdateSettings(const winrt::Microsoft::Terminal::Settings::TerminalSettings& settings, const GUID& profile); + winrt::hstring GetActiveTitle() const; + winrt::fire_and_forget SetTabText(const winrt::hstring text); - void Shutdown(); - void ClosePane(); + void Shutdown(); + void ClosePane(); - WINRT_CALLBACK(Closed, winrt::Windows::Foundation::EventHandler); - DECLARE_EVENT(ActivePaneChanged, _ActivePaneChangedHandlers, winrt::delegate<>); + WINRT_CALLBACK(Closed, winrt::Windows::Foundation::EventHandler); + DECLARE_EVENT(ActivePaneChanged, _ActivePaneChangedHandlers, winrt::delegate<>); - WINRT_CALLBACK(PropertyChanged, Windows::UI::Xaml::Data::PropertyChangedEventHandler); - OBSERVABLE_GETSET_PROPERTY(hstring, Title, _PropertyChangedHandlers); - OBSERVABLE_GETSET_PROPERTY(hstring, IconPath, _PropertyChangedHandlers); + WINRT_CALLBACK(PropertyChanged, Windows::UI::Xaml::Data::PropertyChangedEventHandler); + OBSERVABLE_GETSET_PROPERTY(hstring, Title, _PropertyChangedHandlers); + OBSERVABLE_GETSET_PROPERTY(hstring, IconPath, _PropertyChangedHandlers); - private: - std::shared_ptr _rootPane{ nullptr }; - std::shared_ptr _activePane{ nullptr }; - winrt::hstring _lastIconPath{}; + private: + std::shared_ptr _rootPane{ nullptr }; + std::shared_ptr _activePane{ nullptr }; + winrt::hstring _lastIconPath{}; - bool _focused{ false }; - winrt::Microsoft::UI::Xaml::Controls::TabViewItem _tabViewItem{ nullptr }; + bool _focused{ false }; + winrt::Microsoft::UI::Xaml::Controls::TabViewItem _tabViewItem{ nullptr }; - void _MakeTabViewItem(); - void _Focus(); + void _MakeTabViewItem(); + void _Focus(); - void _AttachEventHandlersToControl(const winrt::Microsoft::Terminal::TerminalControl::TermControl& control); - void _AttachEventHandlersToPane(std::shared_ptr pane); + void _AttachEventHandlersToControl(const winrt::Microsoft::Terminal::TerminalControl::TermControl& control); + void _AttachEventHandlersToPane(std::shared_ptr pane); }; } diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index 3ae1fd89a9e..2415b4c5917 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -458,7 +458,7 @@ namespace winrt::TerminalApp::implementation const auto [profileGuid, settings] = _settings->BuildSettings(newTerminalArgs); _CreateNewTabFromSettings(profileGuid, settings); - + const int tabCount = static_cast(_tabs.Size()); const bool usedManualProfile = (newTerminalArgs != nullptr) && (newTerminalArgs.ProfileIndex() != nullptr || diff --git a/src/cascadia/inc/cppwinrt_utils.h b/src/cascadia/inc/cppwinrt_utils.h index b4a46e3bb58..92ee1a26b06 100644 --- a/src/cascadia/inc/cppwinrt_utils.h +++ b/src/cascadia/inc/cppwinrt_utils.h @@ -110,7 +110,7 @@ private: \ // Use this macro to quickly implement both the getter and setter for an observable property. // This is similar to the GETSET_PROPERTY macro above, except this will also raise a // PropertyChanged event with the name of the property that has changed inside of the settter. -#define OBSERVABLE_GETSET_PROPERTY(type, name, event) \ +#define OBSERVABLE_GETSET_PROPERTY(type, name, event) \ public: \ type name() { return _##name; }; \ void name(const type& value) \ From 516c0deab719dbc049d9a43aff3114eaa0f5967e Mon Sep 17 00:00:00 2001 From: Leon Liang Date: Mon, 27 Jan 2020 15:29:52 -0800 Subject: [PATCH 07/17] fixing build errors --- src/cascadia/TerminalApp/Tab.cpp | 13 ---------- src/cascadia/TerminalApp/TerminalPage.cpp | 29 ++++++++++++++--------- 2 files changed, 18 insertions(+), 24 deletions(-) diff --git a/src/cascadia/TerminalApp/Tab.cpp b/src/cascadia/TerminalApp/Tab.cpp index c885b33532e..a692dbf0ac5 100644 --- a/src/cascadia/TerminalApp/Tab.cpp +++ b/src/cascadia/TerminalApp/Tab.cpp @@ -324,19 +324,6 @@ namespace winrt::TerminalApp::implementation _activePane->Close(); } - // Method Description: - // - Closes the currently focused pane in this tab. If it's the last pane in - // this tab, our Closed event will be fired (at a later time) for anyone - // registered as a handler of our close event. - // Arguments: - // - - // Return Value: - // - - void Tab::ClosePane() - { - _activePane->Close(); - } - // Method Description: // - Register any event handlers that we may need with the given TermControl. // This should be called on each and every TermControl that we add to the tree diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index 2415b4c5917..bf433db74a5 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -488,7 +488,7 @@ namespace winrt::TerminalApp::implementation // - settings: the TerminalSettings object to use to create the TerminalControl with. void TerminalPage::_CreateNewTabFromSettings(GUID profileGuid, TerminalSettings settings) { - const bool isFirstTab = _tabs.empty(); + const bool isFirstTab = _tabs.Size() == 0; // Initialize the new tab // Create a connection based on the values in our settings object. @@ -552,7 +552,7 @@ namespace winrt::TerminalApp::implementation if (isFirstTab) { _tabContent.Children().Clear(); - _tabContent.Children().Append(newTab->GetRootElement()); + _tabContent.Children().Append(newTabImpl->GetRootElement()); } else { @@ -821,14 +821,15 @@ namespace winrt::TerminalApp::implementation { // Removing the tab from the collection should destroy its control and disconnect its connection, // but it doesn't always do so. The UI tree may still be holding the control and preventing its destruction. - auto iterator = _tabs.begin() + tabIndex; - (*iterator)->Shutdown(); + winrt::com_ptr shutdownTab; + shutdownTab.copy_from(winrt::get_self(_tabs.GetAt(tabIndex))); + shutdownTab->Shutdown(); _tabs.RemoveAt(tabIndex); _tabView.TabItems().RemoveAt(tabIndex); // To close the window here, we need to close the hosting window. - if (_tabs.size() == 0) + if (_tabs.Size() == 0) { _lastTabClosedHandlers(*this, nullptr); } @@ -1397,7 +1398,8 @@ namespace winrt::TerminalApp::implementation // Unfocus all the tabs. for (auto tab : _tabs) { - auto tabImpl = winrt::get_self(tab); + winrt::com_ptr tabImpl; + tabImpl.copy_from(winrt::get_self(tab)); tabImpl->SetFocused(false); } @@ -1405,12 +1407,13 @@ namespace winrt::TerminalApp::implementation { try { - auto tab = winrt::get_self(_tabs.GetAt(selectedIndex)); + winrt::com_ptr tabImpl; + tabImpl.copy_from(winrt::get_self(_tabs.GetAt(selectedIndex))); _tabContent.Children().Clear(); - _tabContent.Children().Append(tab->GetRootElement()); + _tabContent.Children().Append(tabImpl->GetRootElement()); - tab->SetFocused(true); + tabImpl->SetFocused(true); _titleChangeHandlers(*this, Title()); } CATCH_LOG(); @@ -1431,7 +1434,8 @@ namespace winrt::TerminalApp::implementation const auto newSize = e.NewSize(); for (auto tab : _tabs) { - auto tabImpl = winrt::get_self(tab); + winrt::com_ptr tabImpl; + tabImpl.copy_from(winrt::get_self(tab)); tabImpl->ResizeContent(newSize); } } @@ -1485,7 +1489,8 @@ namespace winrt::TerminalApp::implementation for (auto tab : _tabs) { // Attempt to reload the settings of any panes with this profile - auto tabImpl = winrt::get_self(tab); + winrt::com_ptr tabImpl; + tabImpl.copy_from(winrt::get_self(tab)); tabImpl->UpdateSettings(settings, profileGuid); } } @@ -1493,6 +1498,8 @@ namespace winrt::TerminalApp::implementation // Update the icon of the tab for the currently focused profile in that tab. for (auto tab : _tabs) { + winrt::com_ptr tabImpl; + tabImpl.copy_from(winrt::get_self(tab)); _UpdateTabIcon(tab); _UpdateTitle(tab); } From 091d95308baa32c89cd572051b13ccf0b541e0d2 Mon Sep 17 00:00:00 2001 From: Leon Liang Date: Tue, 28 Jan 2020 16:44:39 -0800 Subject: [PATCH 08/17] incorporating PR comments --- src/cascadia/TerminalApp/Tab.cpp | 28 ++++++++ src/cascadia/TerminalApp/Tab.h | 14 ++-- src/cascadia/TerminalApp/Tab.idl | 4 +- src/cascadia/TerminalApp/TerminalPage.cpp | 86 +++++++++++------------ src/cascadia/TerminalApp/TerminalPage.h | 8 +-- src/cascadia/inc/cppwinrt_utils.h | 15 ---- 6 files changed, 86 insertions(+), 69 deletions(-) diff --git a/src/cascadia/TerminalApp/Tab.cpp b/src/cascadia/TerminalApp/Tab.cpp index a692dbf0ac5..8d15dbe43a4 100644 --- a/src/cascadia/TerminalApp/Tab.cpp +++ b/src/cascadia/TerminalApp/Tab.cpp @@ -32,6 +32,34 @@ namespace winrt::TerminalApp::implementation _MakeTabViewItem(); } + hstring Tab::Title() + { + return _Title; + } + + void Tab::Title(const hstring& value) + { + if (_Title != value) + { + const_cast(_Title) = value; + _PropertyChangedHandlers(*this, Windows::UI::Xaml::Data::PropertyChangedEventArgs{ L"Title" }); + } + } + + hstring Tab::IconPath() + { + return _IconPath; + } + + void Tab::IconPath(const hstring& value) + { + if (_IconPath != value) + { + const_cast(_IconPath) = value; + _PropertyChangedHandlers(*this, Windows::UI::Xaml::Data::PropertyChangedEventArgs{ L"IconPath" }); + } + } + void Tab::_MakeTabViewItem() { _tabViewItem = ::winrt::MUX::Controls::TabViewItem{}; diff --git a/src/cascadia/TerminalApp/Tab.h b/src/cascadia/TerminalApp/Tab.h index 45d09b8b304..019a3fb32a2 100644 --- a/src/cascadia/TerminalApp/Tab.h +++ b/src/cascadia/TerminalApp/Tab.h @@ -44,12 +44,12 @@ namespace winrt::TerminalApp::implementation void Shutdown(); void ClosePane(); - WINRT_CALLBACK(Closed, winrt::Windows::Foundation::EventHandler); - DECLARE_EVENT(ActivePaneChanged, _ActivePaneChangedHandlers, winrt::delegate<>); + hstring Title(); + hstring IconPath(); + WINRT_CALLBACK(Closed, winrt::Windows::Foundation::EventHandler); WINRT_CALLBACK(PropertyChanged, Windows::UI::Xaml::Data::PropertyChangedEventHandler); - OBSERVABLE_GETSET_PROPERTY(hstring, Title, _PropertyChangedHandlers); - OBSERVABLE_GETSET_PROPERTY(hstring, IconPath, _PropertyChangedHandlers); + DECLARE_EVENT(ActivePaneChanged, _ActivePaneChangedHandlers, winrt::delegate<>); private: std::shared_ptr _rootPane{ nullptr }; @@ -59,6 +59,12 @@ namespace winrt::TerminalApp::implementation bool _focused{ false }; winrt::Microsoft::UI::Xaml::Controls::TabViewItem _tabViewItem{ nullptr }; + const winrt::hstring _Title{}; + void Title(const hstring& value); + + const winrt::hstring _IconPath{}; + void IconPath(const hstring& value); + void _MakeTabViewItem(); void _Focus(); diff --git a/src/cascadia/TerminalApp/Tab.idl b/src/cascadia/TerminalApp/Tab.idl index 303e95fce4f..955c243845a 100644 --- a/src/cascadia/TerminalApp/Tab.idl +++ b/src/cascadia/TerminalApp/Tab.idl @@ -5,7 +5,7 @@ namespace TerminalApp { [default_interface] runtimeclass Tab : Windows.UI.Xaml.Data.INotifyPropertyChanged { - String Title; - String IconPath; + String Title { get; }; + String IconPath { get; }; } } diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index bf433db74a5..d2dc42ebd2f 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -39,7 +39,7 @@ namespace winrt namespace winrt::TerminalApp::implementation { TerminalPage::TerminalPage() : - _tabs{ winrt::single_threaded_observable_vector() } + _tabs{ winrt::single_threaded_observable_vector() } { InitializeComponent(); } @@ -497,18 +497,15 @@ namespace winrt::TerminalApp::implementation TermControl term{ settings, connection }; // Add the new tab to the list of our tabs. - auto newTabProjection = winrt::make(profileGuid, term); - _tabs.Append(newTabProjection); - - winrt::com_ptr newTabImpl; - newTabImpl.copy_from(winrt::get_self(newTabProjection)); + auto newTabImpl = winrt::make_self(profileGuid, term); + _tabs.Append(*newTabImpl); // Hookup our event handlers to the new terminal _RegisterTerminalEvents(term, newTabImpl); // 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. - auto weakTab = make_weak(newTabProjection); + auto weakTab = make_weak(newTabImpl); // 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. @@ -718,15 +715,12 @@ namespace winrt::TerminalApp::implementation // TitleChanged event. // Arguments: // - tab: the Tab to update the title for. - void TerminalPage::_UpdateTitle(winrt::TerminalApp::Tab tab) + void TerminalPage::_UpdateTitle(const winrt::com_ptr& tab) { - winrt::com_ptr newTabImpl; - newTabImpl.copy_from(winrt::get_self(tab)); - - auto newTabTitle = newTabImpl->GetActiveTitle(); + auto newTabTitle = tab->GetActiveTitle(); if (_settings->GlobalSettings().GetShowTitleInTitlebar() && - newTabImpl->IsFocused()) + tab->IsFocused()) { _titleChangeHandlers(*this, newTabTitle); } @@ -737,23 +731,20 @@ namespace winrt::TerminalApp::implementation // tab's icon to that icon. // Arguments: // - tab: the Tab to update the title for. - void TerminalPage::_UpdateTabIcon(winrt::TerminalApp::Tab tab) + void TerminalPage::_UpdateTabIcon(const winrt::com_ptr& tab) { - winrt::com_ptr tabImpl; - tabImpl.copy_from(winrt::get_self(tab)); - - const auto lastFocusedProfileOpt = tabImpl->GetFocusedProfile(); + const auto lastFocusedProfileOpt = tab->GetFocusedProfile(); if (lastFocusedProfileOpt.has_value()) { const auto lastFocusedProfile = lastFocusedProfileOpt.value(); const auto* const matchingProfile = _settings->FindProfile(lastFocusedProfile); if (matchingProfile) { - tabImpl->UpdateIcon(matchingProfile->GetExpandedIconPath()); + tab->UpdateIcon(matchingProfile->GetExpandedIconPath()); } else { - tabImpl->UpdateIcon({}); + tab->UpdateIcon({}); } } } @@ -790,7 +781,7 @@ namespace winrt::TerminalApp::implementation void TerminalPage::_DuplicateTabViewItem() { const int& focusedTabIndex = _GetFocusedTabIndex(); - const auto& _tab = winrt::get_self(_tabs.GetAt(focusedTabIndex)); + const auto& _tab = winrt::get_self(_tabs.GetAt(focusedTabIndex)); const auto& profileGuid = _tab->GetFocusedProfile(); if (profileGuid.has_value()) @@ -822,7 +813,7 @@ namespace winrt::TerminalApp::implementation // Removing the tab from the collection should destroy its control and disconnect its connection, // but it doesn't always do so. The UI tree may still be holding the control and preventing its destruction. winrt::com_ptr shutdownTab; - shutdownTab.copy_from(winrt::get_self(_tabs.GetAt(tabIndex))); + shutdownTab.copy_from(winrt::get_self(_tabs.GetAt(tabIndex))); shutdownTab->Shutdown(); _tabs.RemoveAt(tabIndex); @@ -860,7 +851,7 @@ namespace winrt::TerminalApp::implementation // Arguments: // - term: The newly created TermControl to connect the events for // - hostingTab: The Tab that's hosting this TermControl instance - void TerminalPage::_RegisterTerminalEvents(TermControl term, winrt::com_ptr hostingTab) + void TerminalPage::_RegisterTerminalEvents(TermControl term, const winrt::com_ptr& hostingTab) { // Add an event handler when the terminal's selection wants to be copied. // When the text buffer data is retrieved, we'll copy the data into the Clipboard @@ -874,7 +865,7 @@ 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. - auto weakTab = make_weak(hostingTab.as()); + auto weakTab = make_weak(hostingTab); term.TitleChanged([weakTab, weakThis{ get_weak() }](auto newTitle) { auto page{ weakThis.get() }; @@ -929,7 +920,8 @@ namespace winrt::TerminalApp::implementation void TerminalPage::_MoveFocus(const Direction& direction) { const auto focusedTabIndex = _GetFocusedTabIndex(); - auto focusedTab = winrt::get_self(_tabs.GetAt(focusedTabIndex)); + winrt::com_ptr focusedTab; + focusedTab.copy_from(winrt::get_self(_tabs.GetAt(focusedTabIndex))); focusedTab->NavigateFocus(direction); } @@ -937,7 +929,7 @@ namespace winrt::TerminalApp::implementation { int focusedTabIndex = _GetFocusedTabIndex(); winrt::com_ptr focusedTab; - focusedTab.copy_from(winrt::get_self(_tabs.GetAt(focusedTabIndex))); + focusedTab.copy_from(winrt::get_self(_tabs.GetAt(focusedTabIndex))); return focusedTab->GetActiveTerminalControl(); } @@ -968,7 +960,8 @@ namespace winrt::TerminalApp::implementation if (auto page{ weakThis.get() }) { - auto tab = winrt::get_self(_tabs.GetAt(tabIndex)); + winrt::com_ptr tab; + tab.copy_from(winrt::get_self(_tabs.GetAt(tabIndex))); _tabView.SelectedItem(tab->GetTabViewItem()); } } @@ -988,8 +981,9 @@ namespace winrt::TerminalApp::implementation void TerminalPage::_CloseFocusedPane() { int focusedTabIndex = _GetFocusedTabIndex(); - auto focusedTab{ winrt::get_self(_tabs.GetAt(focusedTabIndex)) }; - focusedTab->ClosePane(); + winrt::com_ptr tab; + tab.copy_from(winrt::get_self(_tabs.GetAt(focusedTabIndex))); + tab->ClosePane(); } // Method Description: @@ -1027,7 +1021,8 @@ namespace winrt::TerminalApp::implementation void TerminalPage::_Scroll(int delta) { int focusedTabIndex = _GetFocusedTabIndex(); - auto tab = winrt::get_self(_tabs.GetAt(focusedTabIndex)); + winrt::com_ptr tab; + tab.copy_from(winrt::get_self(_tabs.GetAt(focusedTabIndex))); tab->Scroll(delta); } @@ -1058,7 +1053,7 @@ namespace winrt::TerminalApp::implementation auto tabProj = _tabs.GetAt(focusedTabIndex); winrt::com_ptr tabImpl; - tabImpl.copy_from(winrt::get_self(tabProj)); + tabImpl.copy_from(winrt::get_self(tabProj)); const auto canSplit = tabImpl->CanSplitPane(splitType); @@ -1086,7 +1081,8 @@ namespace winrt::TerminalApp::implementation void TerminalPage::_ResizePane(const Direction& direction) { const auto focusedTabIndex = _GetFocusedTabIndex(); - auto tab = winrt::get_self(_tabs.GetAt(focusedTabIndex)); + winrt::com_ptr tab; + tab.copy_from(winrt::get_self(_tabs.GetAt(focusedTabIndex))); tab->ResizePane(direction); } @@ -1105,7 +1101,8 @@ namespace winrt::TerminalApp::implementation const auto control = _GetActiveControl(); const auto termHeight = control.GetViewHeight(); - auto tab = winrt::get_self(_tabs.GetAt(focusedTabIndex)); + winrt::com_ptr tab; + tab.copy_from(winrt::get_self(_tabs.GetAt(focusedTabIndex))); tab->Scroll(termHeight * delta); } @@ -1216,7 +1213,8 @@ namespace winrt::TerminalApp::implementation if (_settings->GlobalSettings().SnapToGridOnResize()) { const auto focusedTabIndex = _GetFocusedTabIndex(); - auto tab = winrt::get_self(_tabs.GetAt(focusedTabIndex)); + winrt::com_ptr tab; + tab.copy_from(winrt::get_self(_tabs.GetAt(focusedTabIndex))); return tab->CalcSnappedDimension(widthOrHeight, dimension); } else @@ -1399,7 +1397,7 @@ namespace winrt::TerminalApp::implementation for (auto tab : _tabs) { winrt::com_ptr tabImpl; - tabImpl.copy_from(winrt::get_self(tab)); + tabImpl.copy_from(winrt::get_self(tab)); tabImpl->SetFocused(false); } @@ -1407,13 +1405,13 @@ namespace winrt::TerminalApp::implementation { try { - winrt::com_ptr tabImpl; - tabImpl.copy_from(winrt::get_self(_tabs.GetAt(selectedIndex))); + winrt::com_ptr tab; + tab.copy_from(winrt::get_self(_tabs.GetAt(selectedIndex))); _tabContent.Children().Clear(); - _tabContent.Children().Append(tabImpl->GetRootElement()); + _tabContent.Children().Append(tab->GetRootElement()); - tabImpl->SetFocused(true); + tab->SetFocused(true); _titleChangeHandlers(*this, Title()); } CATCH_LOG(); @@ -1435,7 +1433,7 @@ namespace winrt::TerminalApp::implementation for (auto tab : _tabs) { winrt::com_ptr tabImpl; - tabImpl.copy_from(winrt::get_self(tab)); + tabImpl.copy_from(winrt::get_self(tab)); tabImpl->ResizeContent(newSize); } } @@ -1490,7 +1488,7 @@ namespace winrt::TerminalApp::implementation { // Attempt to reload the settings of any panes with this profile winrt::com_ptr tabImpl; - tabImpl.copy_from(winrt::get_self(tab)); + tabImpl.copy_from(winrt::get_self(tab)); tabImpl->UpdateSettings(settings, profileGuid); } } @@ -1499,9 +1497,9 @@ namespace winrt::TerminalApp::implementation for (auto tab : _tabs) { winrt::com_ptr tabImpl; - tabImpl.copy_from(winrt::get_self(tab)); - _UpdateTabIcon(tab); - _UpdateTitle(tab); + tabImpl.copy_from(winrt::get_self(tab)); + _UpdateTabIcon(tabImpl); + _UpdateTitle(tabImpl); } auto weakThis{ get_weak() }; diff --git a/src/cascadia/TerminalApp/TerminalPage.h b/src/cascadia/TerminalApp/TerminalPage.h index fa15413923c..1b909aab192 100644 --- a/src/cascadia/TerminalApp/TerminalPage.h +++ b/src/cascadia/TerminalApp/TerminalPage.h @@ -63,7 +63,7 @@ namespace winrt::TerminalApp::implementation std::shared_ptr<::TerminalApp::CascadiaSettings> _settings{ nullptr }; - Windows::Foundation::Collections::IObservableVector _tabs; + Windows::Foundation::Collections::IObservableVector _tabs; bool _isFullscreen{ false }; @@ -94,15 +94,15 @@ namespace winrt::TerminalApp::implementation void _HookupKeyBindings(TerminalApp::AppKeyBindings bindings) noexcept; void _RegisterActionCallbacks(); - void _UpdateTitle(winrt::TerminalApp::Tab tab); - void _UpdateTabIcon(winrt::TerminalApp::Tab tab); + void _UpdateTitle(const winrt::com_ptr& tab); + void _UpdateTabIcon(const winrt::com_ptr& tab); void _UpdateTabView(); void _UpdateTabWidthMode(); void _DuplicateTabViewItem(); void _RemoveTabViewItem(const Microsoft::UI::Xaml::Controls::TabViewItem& tabViewItem); void _RemoveTabViewItemByIndex(uint32_t tabIndex); - void _RegisterTerminalEvents(Microsoft::Terminal::TerminalControl::TermControl term, winrt::com_ptr hostingTab); + void _RegisterTerminalEvents(Microsoft::Terminal::TerminalControl::TermControl term, const winrt::com_ptr& hostingTab); void _SelectNextTab(const bool bMoveRight); bool _SelectTab(const int tabIndex); diff --git a/src/cascadia/inc/cppwinrt_utils.h b/src/cascadia/inc/cppwinrt_utils.h index 92ee1a26b06..a9a5cada77c 100644 --- a/src/cascadia/inc/cppwinrt_utils.h +++ b/src/cascadia/inc/cppwinrt_utils.h @@ -107,21 +107,6 @@ public: \ private: \ type _##name{ __VA_ARGS__ }; -// Use this macro to quickly implement both the getter and setter for an observable property. -// This is similar to the GETSET_PROPERTY macro above, except this will also raise a -// PropertyChanged event with the name of the property that has changed inside of the settter. -#define OBSERVABLE_GETSET_PROPERTY(type, name, event) \ -public: \ - type name() { return _##name; }; \ - void name(const type& value) \ - { \ - _##name = value; \ - event(*this, Windows::UI::Xaml::Data::PropertyChangedEventArgs{ L#name }); \ - }; \ - \ -private: \ - type _##name; - // Use this macro for quickly defining the factory_implementation part of a // class. CppWinrt requires these for the compiler, but more often than not, // they require no customization. See From ff8ed261b014d67120ef79fca6bfd8e382a83d32 Mon Sep 17 00:00:00 2001 From: Leon Liang Date: Tue, 28 Jan 2020 17:15:21 -0800 Subject: [PATCH 09/17] moving this const cast magic into a macro magic --- src/cascadia/TerminalApp/Tab.cpp | 28 ---------------------------- src/cascadia/TerminalApp/Tab.h | 12 +++--------- src/cascadia/inc/cppwinrt_utils.h | 18 ++++++++++++++++++ 3 files changed, 21 insertions(+), 37 deletions(-) diff --git a/src/cascadia/TerminalApp/Tab.cpp b/src/cascadia/TerminalApp/Tab.cpp index 8d15dbe43a4..a692dbf0ac5 100644 --- a/src/cascadia/TerminalApp/Tab.cpp +++ b/src/cascadia/TerminalApp/Tab.cpp @@ -32,34 +32,6 @@ namespace winrt::TerminalApp::implementation _MakeTabViewItem(); } - hstring Tab::Title() - { - return _Title; - } - - void Tab::Title(const hstring& value) - { - if (_Title != value) - { - const_cast(_Title) = value; - _PropertyChangedHandlers(*this, Windows::UI::Xaml::Data::PropertyChangedEventArgs{ L"Title" }); - } - } - - hstring Tab::IconPath() - { - return _IconPath; - } - - void Tab::IconPath(const hstring& value) - { - if (_IconPath != value) - { - const_cast(_IconPath) = value; - _PropertyChangedHandlers(*this, Windows::UI::Xaml::Data::PropertyChangedEventArgs{ L"IconPath" }); - } - } - void Tab::_MakeTabViewItem() { _tabViewItem = ::winrt::MUX::Controls::TabViewItem{}; diff --git a/src/cascadia/TerminalApp/Tab.h b/src/cascadia/TerminalApp/Tab.h index 019a3fb32a2..906aab7de7f 100644 --- a/src/cascadia/TerminalApp/Tab.h +++ b/src/cascadia/TerminalApp/Tab.h @@ -44,13 +44,13 @@ namespace winrt::TerminalApp::implementation void Shutdown(); void ClosePane(); - hstring Title(); - hstring IconPath(); - WINRT_CALLBACK(Closed, winrt::Windows::Foundation::EventHandler); WINRT_CALLBACK(PropertyChanged, Windows::UI::Xaml::Data::PropertyChangedEventHandler); DECLARE_EVENT(ActivePaneChanged, _ActivePaneChangedHandlers, winrt::delegate<>); + OBSERVABLE_GETSET_PROPERTY(hstring, Title, _PropertyChangedHandlers); + OBSERVABLE_GETSET_PROPERTY(hstring, IconPath, _PropertyChangedHandlers); + private: std::shared_ptr _rootPane{ nullptr }; std::shared_ptr _activePane{ nullptr }; @@ -59,12 +59,6 @@ namespace winrt::TerminalApp::implementation bool _focused{ false }; winrt::Microsoft::UI::Xaml::Controls::TabViewItem _tabViewItem{ nullptr }; - const winrt::hstring _Title{}; - void Title(const hstring& value); - - const winrt::hstring _IconPath{}; - void IconPath(const hstring& value); - void _MakeTabViewItem(); void _Focus(); diff --git a/src/cascadia/inc/cppwinrt_utils.h b/src/cascadia/inc/cppwinrt_utils.h index a9a5cada77c..436df0fc54e 100644 --- a/src/cascadia/inc/cppwinrt_utils.h +++ b/src/cascadia/inc/cppwinrt_utils.h @@ -107,6 +107,24 @@ public: \ private: \ type _##name{ __VA_ARGS__ }; +// Use this macro to quickly implement both the getter and setter for an observable property. +// This is similar to the GETSET_PROPERTY macro above, except this will also raise a +// PropertyChanged event with the name of the property that has changed inside of the settter. +#define OBSERVABLE_GETSET_PROPERTY(type, name, event) \ +public: \ + type name() { return _##name; }; \ + \ +private: \ + const type _##name; \ + void name(const type& value) \ + { \ + if (_##name != value) \ + { \ + const_cast(_##name) = value; \ + event(*this, Windows::UI::Xaml::Data::PropertyChangedEventArgs{ L#name }); \ + } \ + }; + // Use this macro for quickly defining the factory_implementation part of a // class. CppWinrt requires these for the compiler, but more often than not, // they require no customization. See From 30ea840f3c2aaa30a359ce8a8f14532c1d962d24 Mon Sep 17 00:00:00 2001 From: Leon Liang Date: Tue, 28 Jan 2020 17:25:44 -0800 Subject: [PATCH 10/17] FORMATTING WHY --- src/cascadia/inc/cppwinrt_utils.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cascadia/inc/cppwinrt_utils.h b/src/cascadia/inc/cppwinrt_utils.h index 436df0fc54e..e2be08b4a3b 100644 --- a/src/cascadia/inc/cppwinrt_utils.h +++ b/src/cascadia/inc/cppwinrt_utils.h @@ -123,7 +123,7 @@ private: const_cast(_##name) = value; \ event(*this, Windows::UI::Xaml::Data::PropertyChangedEventArgs{ L#name }); \ } \ - }; + }; // Use this macro for quickly defining the factory_implementation part of a // class. CppWinrt requires these for the compiler, but more often than not, From f66525e5cd63b1ae5c7cd1d55732d37341e67ea3 Mon Sep 17 00:00:00 2001 From: Leon Liang Date: Fri, 31 Jan 2020 13:42:00 -0800 Subject: [PATCH 11/17] some more PR comments inc --- src/cascadia/TerminalApp/Tab.cpp | 24 ++++ src/cascadia/TerminalApp/Tab.h | 4 +- src/cascadia/TerminalApp/TerminalPage.cpp | 140 ++++++++++------------ src/cascadia/TerminalApp/TerminalPage.h | 7 +- 4 files changed, 92 insertions(+), 83 deletions(-) diff --git a/src/cascadia/TerminalApp/Tab.cpp b/src/cascadia/TerminalApp/Tab.cpp index a692dbf0ac5..061ee5f592c 100644 --- a/src/cascadia/TerminalApp/Tab.cpp +++ b/src/cascadia/TerminalApp/Tab.cpp @@ -32,11 +32,23 @@ namespace winrt::TerminalApp::implementation _MakeTabViewItem(); } + // Method Description: + // - Initializes a TabViewItem for this Tab instance. + // Arguments: + // - + // Return Value: + // - void Tab::_MakeTabViewItem() { _tabViewItem = ::winrt::MUX::Controls::TabViewItem{}; } + // 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() { return _rootPane->GetRootElement(); @@ -58,6 +70,12 @@ namespace winrt::TerminalApp::implementation return _activePane->GetTerminalControl(); } + // Method Description: + // - Gets the TabViewItem that represents this Tab + // Arguments: + // - + // Return Value: + // - The TabViewItem that represents this Tab winrt::MUX::Controls::TabViewItem Tab::GetTabViewItem() { return _tabViewItem; @@ -150,6 +168,12 @@ namespace winrt::TerminalApp::implementation } } + // Method Description: + // - Set the icon on the TabViewItem for this tab. + // Arguments: + // - iconPath: The new path string to use as the IconPath for our TabViewItem + // Return Value: + // - winrt::fire_and_forget Tab::UpdateIcon(const winrt::hstring iconPath) { // Don't reload our icon if it hasn't changed. diff --git a/src/cascadia/TerminalApp/Tab.h b/src/cascadia/TerminalApp/Tab.h index 906aab7de7f..af72398ffa0 100644 --- a/src/cascadia/TerminalApp/Tab.h +++ b/src/cascadia/TerminalApp/Tab.h @@ -48,8 +48,8 @@ namespace winrt::TerminalApp::implementation WINRT_CALLBACK(PropertyChanged, Windows::UI::Xaml::Data::PropertyChangedEventHandler); DECLARE_EVENT(ActivePaneChanged, _ActivePaneChangedHandlers, winrt::delegate<>); - OBSERVABLE_GETSET_PROPERTY(hstring, Title, _PropertyChangedHandlers); - OBSERVABLE_GETSET_PROPERTY(hstring, IconPath, _PropertyChangedHandlers); + OBSERVABLE_GETSET_PROPERTY(winrt::hstring, Title, _PropertyChangedHandlers); + OBSERVABLE_GETSET_PROPERTY(winrt::hstring, IconPath, _PropertyChangedHandlers); private: std::shared_ptr _rootPane{ nullptr }; diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index d2dc42ebd2f..210e497ddaf 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -459,7 +459,7 @@ namespace winrt::TerminalApp::implementation _CreateNewTabFromSettings(profileGuid, settings); - const int tabCount = static_cast(_tabs.Size()); + const uint32_t tabCount = _tabs.Size(); const bool usedManualProfile = (newTerminalArgs != nullptr) && (newTerminalArgs.ProfileIndex() != nullptr || newTerminalArgs.Profile().empty()); @@ -467,7 +467,7 @@ namespace winrt::TerminalApp::implementation g_hTerminalAppProvider, // handle to TerminalApp tracelogging provider "TabInformation", TraceLoggingDescription("Event emitted upon new tab creation in TerminalApp"), - TraceLoggingInt32(tabCount, "TabCount", "Count of tabs curently opened in TerminalApp"), + TraceLoggingUInt32(tabCount, "TabCount", "Count of tabs curently opened in TerminalApp"), TraceLoggingBool(usedManualProfile, "ProfileSpecified", "Whether the new tab specified a profile explicitly"), TraceLoggingGuid(profileGuid, "ProfileGuid", "The GUID of the profile spawned in the new tab"), TraceLoggingKeyword(MICROSOFT_KEYWORD_MEASURES), @@ -501,7 +501,7 @@ namespace winrt::TerminalApp::implementation _tabs.Append(*newTabImpl); // Hookup our event handlers to the new terminal - _RegisterTerminalEvents(term, newTabImpl); + _RegisterTerminalEvents(term, *newTabImpl); // 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. @@ -516,10 +516,10 @@ namespace winrt::TerminalApp::implementation if (page && tab) { // Possibly update the icon of the tab. - page->_UpdateTabIcon(tab); + page->_UpdateTabIcon(*tab); // Possibly update the title of the tab, window to match the newly // focused pane. - page->_UpdateTitle(tab); + page->_UpdateTitle(*tab); } }); @@ -715,12 +715,12 @@ namespace winrt::TerminalApp::implementation // TitleChanged event. // Arguments: // - tab: the Tab to update the title for. - void TerminalPage::_UpdateTitle(const winrt::com_ptr& tab) + void TerminalPage::_UpdateTitle(const Tab& tab) { - auto newTabTitle = tab->GetActiveTitle(); + auto newTabTitle = tab.GetActiveTitle(); if (_settings->GlobalSettings().GetShowTitleInTitlebar() && - tab->IsFocused()) + tab.IsFocused()) { _titleChangeHandlers(*this, newTabTitle); } @@ -731,20 +731,20 @@ namespace winrt::TerminalApp::implementation // tab's icon to that icon. // Arguments: // - tab: the Tab to update the title for. - void TerminalPage::_UpdateTabIcon(const winrt::com_ptr& tab) + void TerminalPage::_UpdateTabIcon(Tab& tab) { - const auto lastFocusedProfileOpt = tab->GetFocusedProfile(); + const auto lastFocusedProfileOpt = tab.GetFocusedProfile(); if (lastFocusedProfileOpt.has_value()) { const auto lastFocusedProfile = lastFocusedProfileOpt.value(); const auto* const matchingProfile = _settings->FindProfile(lastFocusedProfile); if (matchingProfile) { - tab->UpdateIcon(matchingProfile->GetExpandedIconPath()); + tab.UpdateIcon(matchingProfile->GetExpandedIconPath()); } else { - tab->UpdateIcon({}); + tab.UpdateIcon({}); } } } @@ -780,10 +780,8 @@ namespace winrt::TerminalApp::implementation // - Duplicates the current focused tab void TerminalPage::_DuplicateTabViewItem() { - const int& focusedTabIndex = _GetFocusedTabIndex(); - const auto& _tab = winrt::get_self(_tabs.GetAt(focusedTabIndex)); - - const auto& profileGuid = _tab->GetFocusedProfile(); + auto focusedTab{ _GetStrongTabImpl(_GetFocusedTabIndex()) }; + const auto& profileGuid = focusedTab->GetFocusedProfile(); if (profileGuid.has_value()) { const auto settings = _settings->BuildSettings(profileGuid.value()); @@ -812,9 +810,8 @@ namespace winrt::TerminalApp::implementation { // Removing the tab from the collection should destroy its control and disconnect its connection, // but it doesn't always do so. The UI tree may still be holding the control and preventing its destruction. - winrt::com_ptr shutdownTab; - shutdownTab.copy_from(winrt::get_self(_tabs.GetAt(tabIndex))); - shutdownTab->Shutdown(); + auto tab{ _GetStrongTabImpl(tabIndex) }; + tab->Shutdown(); _tabs.RemoveAt(tabIndex); _tabView.TabItems().RemoveAt(tabIndex); @@ -851,7 +848,7 @@ namespace winrt::TerminalApp::implementation // Arguments: // - term: The newly created TermControl to connect the events for // - hostingTab: The Tab that's hosting this TermControl instance - void TerminalPage::_RegisterTerminalEvents(TermControl term, const winrt::com_ptr& hostingTab) + void TerminalPage::_RegisterTerminalEvents(TermControl term, Tab& hostingTab) { // Add an event handler when the terminal's selection wants to be copied. // When the text buffer data is retrieved, we'll copy the data into the Clipboard @@ -861,13 +858,11 @@ namespace winrt::TerminalApp::implementation term.PasteFromClipboard({ this, &TerminalPage::_PasteFromClipboardHandler }); // Bind Tab events to the TermControl and the Tab's Pane - hostingTab->BindEventHandlers(term); + hostingTab.BindEventHandlers(term); // 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. - auto weakTab = make_weak(hostingTab); - - term.TitleChanged([weakTab, weakThis{ get_weak() }](auto newTitle) { + term.TitleChanged([weakTab{ hostingTab.get_weak() }, weakThis{ get_weak() }](auto newTitle) { auto page{ weakThis.get() }; auto tab{ weakTab.get() }; @@ -876,7 +871,7 @@ namespace winrt::TerminalApp::implementation // 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); + page->_UpdateTitle(*tab); } }); } @@ -919,17 +914,13 @@ namespace winrt::TerminalApp::implementation // - void TerminalPage::_MoveFocus(const Direction& direction) { - const auto focusedTabIndex = _GetFocusedTabIndex(); - winrt::com_ptr focusedTab; - focusedTab.copy_from(winrt::get_self(_tabs.GetAt(focusedTabIndex))); + auto focusedTab{ _GetStrongTabImpl(_GetFocusedTabIndex()) }; focusedTab->NavigateFocus(direction); } winrt::Microsoft::Terminal::TerminalControl::TermControl TerminalPage::_GetActiveControl() { - int focusedTabIndex = _GetFocusedTabIndex(); - winrt::com_ptr focusedTab; - focusedTab.copy_from(winrt::get_self(_tabs.GetAt(focusedTabIndex))); + auto focusedTab{ _GetStrongTabImpl(_GetFocusedTabIndex()) }; return focusedTab->GetActiveTerminalControl(); } @@ -960,8 +951,7 @@ namespace winrt::TerminalApp::implementation if (auto page{ weakThis.get() }) { - winrt::com_ptr tab; - tab.copy_from(winrt::get_self(_tabs.GetAt(tabIndex))); + auto tab{ _GetStrongTabImpl(tabIndex) }; _tabView.SelectedItem(tab->GetTabViewItem()); } } @@ -980,10 +970,8 @@ namespace winrt::TerminalApp::implementation // tab's Closed event. void TerminalPage::_CloseFocusedPane() { - int focusedTabIndex = _GetFocusedTabIndex(); - winrt::com_ptr tab; - tab.copy_from(winrt::get_self(_tabs.GetAt(focusedTabIndex))); - tab->ClosePane(); + auto focusedTab{ _GetStrongTabImpl(_GetFocusedTabIndex()) }; + focusedTab->ClosePane(); } // Method Description: @@ -1020,10 +1008,8 @@ namespace winrt::TerminalApp::implementation // - delta: a number of lines to move the viewport relative to the current viewport. void TerminalPage::_Scroll(int delta) { - int focusedTabIndex = _GetFocusedTabIndex(); - winrt::com_ptr tab; - tab.copy_from(winrt::get_self(_tabs.GetAt(focusedTabIndex))); - tab->Scroll(delta); + auto focusedTab{ _GetStrongTabImpl(_GetFocusedTabIndex()) }; + focusedTab->Scroll(delta); } // Method Description: @@ -1049,13 +1035,9 @@ namespace winrt::TerminalApp::implementation const auto controlConnection = _CreateConnectionFromSettings(realGuid, controlSettings); - const int focusedTabIndex = _GetFocusedTabIndex(); - auto tabProj = _tabs.GetAt(focusedTabIndex); - - winrt::com_ptr tabImpl; - tabImpl.copy_from(winrt::get_self(tabProj)); + auto focusedTab { _GetStrongTabImpl(_GetFocusedTabIndex()) }; - const auto canSplit = tabImpl->CanSplitPane(splitType); + const auto canSplit = focusedTab->CanSplitPane(splitType); if (!canSplit) { @@ -1065,9 +1047,9 @@ namespace winrt::TerminalApp::implementation TermControl newControl{ controlSettings, controlConnection }; // Hookup our event handlers to the new terminal - _RegisterTerminalEvents(newControl, tabImpl); + _RegisterTerminalEvents(newControl, *focusedTab); - tabImpl->SplitPane(splitType, realGuid, newControl); + focusedTab->SplitPane(splitType, realGuid, newControl); } // Method Description: @@ -1080,10 +1062,8 @@ namespace winrt::TerminalApp::implementation // - void TerminalPage::_ResizePane(const Direction& direction) { - const auto focusedTabIndex = _GetFocusedTabIndex(); - winrt::com_ptr tab; - tab.copy_from(winrt::get_self(_tabs.GetAt(focusedTabIndex))); - tab->ResizePane(direction); + auto focusedTab{ _GetStrongTabImpl(_GetFocusedTabIndex()) }; + focusedTab->ResizePane(direction); } // Method Description: @@ -1097,13 +1077,11 @@ namespace winrt::TerminalApp::implementation void TerminalPage::_ScrollPage(int delta) { delta = std::clamp(delta, -1, 1); - const auto focusedTabIndex = _GetFocusedTabIndex(); const auto control = _GetActiveControl(); const auto termHeight = control.GetViewHeight(); - winrt::com_ptr tab; - tab.copy_from(winrt::get_self(_tabs.GetAt(focusedTabIndex))); - tab->Scroll(termHeight * delta); + auto focusedTab{ _GetStrongTabImpl(_GetFocusedTabIndex()) }; + focusedTab->Scroll(termHeight * delta); } // Method Description: @@ -1212,10 +1190,8 @@ namespace winrt::TerminalApp::implementation { if (_settings->GlobalSettings().SnapToGridOnResize()) { - const auto focusedTabIndex = _GetFocusedTabIndex(); - winrt::com_ptr tab; - tab.copy_from(winrt::get_self(_tabs.GetAt(focusedTabIndex))); - return tab->CalcSnappedDimension(widthOrHeight, dimension); + auto focusedTab{ _GetStrongTabImpl(_GetFocusedTabIndex()) }; + return focusedTab->CalcSnappedDimension(widthOrHeight, dimension); } else { @@ -1394,19 +1370,17 @@ namespace winrt::TerminalApp::implementation auto selectedIndex = tabView.SelectedIndex(); // Unfocus all the tabs. - for (auto tab : _tabs) + for (uint32_t idx = 0; idx < _tabs.Size(); ++idx) { - winrt::com_ptr tabImpl; - tabImpl.copy_from(winrt::get_self(tab)); - tabImpl->SetFocused(false); + auto tab{ _GetStrongTabImpl(idx) }; + tab->SetFocused(false); } if (selectedIndex >= 0) { try { - winrt::com_ptr tab; - tab.copy_from(winrt::get_self(_tabs.GetAt(selectedIndex))); + auto tab{ _GetStrongTabImpl(selectedIndex) }; _tabContent.Children().Clear(); _tabContent.Children().Append(tab->GetRootElement()); @@ -1430,10 +1404,9 @@ namespace winrt::TerminalApp::implementation void TerminalPage::_OnContentSizeChanged(const IInspectable& /*sender*/, Windows::UI::Xaml::SizeChangedEventArgs const& e) { const auto newSize = e.NewSize(); - for (auto tab : _tabs) + for (uint32_t idx = 0; idx < _tabs.Size(); ++idx) { - winrt::com_ptr tabImpl; - tabImpl.copy_from(winrt::get_self(tab)); + auto tabImpl{ _GetStrongTabImpl(idx) }; tabImpl->ResizeContent(newSize); } } @@ -1484,22 +1457,20 @@ namespace winrt::TerminalApp::implementation const GUID profileGuid = profile.GetGuid(); const auto settings = _settings->BuildSettings(profileGuid); - for (auto tab : _tabs) + for (uint32_t idx = 0; idx < _tabs.Size(); ++idx) { // Attempt to reload the settings of any panes with this profile - winrt::com_ptr tabImpl; - tabImpl.copy_from(winrt::get_self(tab)); + auto tabImpl{ _GetStrongTabImpl(idx) }; tabImpl->UpdateSettings(settings, profileGuid); } } // Update the icon of the tab for the currently focused profile in that tab. - for (auto tab : _tabs) + for (uint32_t idx = 0; idx < _tabs.Size(); ++idx) { - winrt::com_ptr tabImpl; - tabImpl.copy_from(winrt::get_self(tab)); - _UpdateTabIcon(tabImpl); - _UpdateTitle(tabImpl); + auto tabImpl{ _GetStrongTabImpl(idx) }; + _UpdateTabIcon(*tabImpl); + _UpdateTitle(*tabImpl); } auto weakThis{ get_weak() }; @@ -1642,6 +1613,19 @@ namespace winrt::TerminalApp::implementation return winrt::to_hstring(_appArgs.GetExitMessage()); } + // Method Description: + // - Returns a com_ptr to the implementation type of the tab at the given index + // Arguments: + // - index: an unsigned integer index to a tab in _tabs + // Return Value: + // - a com_ptr to the implementation type of the Tab + winrt::com_ptr TerminalPage::_GetStrongTabImpl(const uint32_t& index) const + { + winrt::com_ptr tabImpl; + tabImpl.copy_from(winrt::get_self(_tabs.GetAt(index))); + return tabImpl; + } + // -------------------------------- WinRT Events --------------------------------- // Winrt events need a method for adding a callback to the event and removing the callback. // These macros will define them both for you. diff --git a/src/cascadia/TerminalApp/TerminalPage.h b/src/cascadia/TerminalApp/TerminalPage.h index 1b909aab192..b8eac4995de 100644 --- a/src/cascadia/TerminalApp/TerminalPage.h +++ b/src/cascadia/TerminalApp/TerminalPage.h @@ -64,6 +64,7 @@ namespace winrt::TerminalApp::implementation std::shared_ptr<::TerminalApp::CascadiaSettings> _settings{ nullptr }; Windows::Foundation::Collections::IObservableVector _tabs; + winrt::com_ptr _GetStrongTabImpl(const uint32_t& index) const; bool _isFullscreen{ false }; @@ -94,15 +95,15 @@ namespace winrt::TerminalApp::implementation void _HookupKeyBindings(TerminalApp::AppKeyBindings bindings) noexcept; void _RegisterActionCallbacks(); - void _UpdateTitle(const winrt::com_ptr& tab); - void _UpdateTabIcon(const winrt::com_ptr& tab); + void _UpdateTitle(const Tab& tab); + void _UpdateTabIcon(Tab& tab); void _UpdateTabView(); void _UpdateTabWidthMode(); void _DuplicateTabViewItem(); void _RemoveTabViewItem(const Microsoft::UI::Xaml::Controls::TabViewItem& tabViewItem); void _RemoveTabViewItemByIndex(uint32_t tabIndex); - void _RegisterTerminalEvents(Microsoft::Terminal::TerminalControl::TermControl term, const winrt::com_ptr& hostingTab); + void _RegisterTerminalEvents(Microsoft::Terminal::TerminalControl::TermControl term, Tab& hostingTab); void _SelectNextTab(const bool bMoveRight); bool _SelectTab(const int tabIndex); From 392ffc1cf70b90abad04dc4174fde7ced567e304 Mon Sep 17 00:00:00 2001 From: Leon Liang Date: Fri, 31 Jan 2020 13:52:06 -0800 Subject: [PATCH 12/17] i should staple ctrl-k ctrl-d to my forehead --- src/cascadia/TerminalApp/TerminalPage.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index 9bed76549b4..ec4aa739d67 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -1048,7 +1048,7 @@ namespace winrt::TerminalApp::implementation const auto controlConnection = _CreateConnectionFromSettings(realGuid, controlSettings); - auto focusedTab { _GetStrongTabImpl(_GetFocusedTabIndex()) }; + auto focusedTab{ _GetStrongTabImpl(_GetFocusedTabIndex()) }; const auto canSplit = focusedTab->CanSplitPane(splitType); From b3964518a87f244421c41abb166641d88e268633 Mon Sep 17 00:00:00 2001 From: Leon Liang Date: Fri, 31 Jan 2020 17:12:22 -0800 Subject: [PATCH 13/17] in the middle of some changes, it's failing though --- .../CommandlineTest.cpp | 2 +- src/cascadia/TerminalApp/ActionArgs.h | 4 +- src/cascadia/TerminalApp/ActionArgs.idl | 2 +- src/cascadia/TerminalApp/TerminalPage.cpp | 142 ++++++++++++------ src/cascadia/TerminalApp/TerminalPage.h | 6 +- 5 files changed, 99 insertions(+), 57 deletions(-) diff --git a/src/cascadia/LocalTests_TerminalApp/CommandlineTest.cpp b/src/cascadia/LocalTests_TerminalApp/CommandlineTest.cpp index 21690e66d5c..7cdda534908 100644 --- a/src/cascadia/LocalTests_TerminalApp/CommandlineTest.cpp +++ b/src/cascadia/LocalTests_TerminalApp/CommandlineTest.cpp @@ -887,7 +887,7 @@ namespace TerminalAppLocalTests VERIFY_IS_NOT_NULL(actionAndArgs.Args()); auto myArgs = actionAndArgs.Args().try_as(); VERIFY_IS_NOT_NULL(myArgs); - VERIFY_ARE_EQUAL(2, myArgs.TabIndex()); + VERIFY_ARE_EQUAL((uint32_t)2, myArgs.TabIndex()); } { diff --git a/src/cascadia/TerminalApp/ActionArgs.h b/src/cascadia/TerminalApp/ActionArgs.h index d3ef59058b8..dab05e1f9ef 100644 --- a/src/cascadia/TerminalApp/ActionArgs.h +++ b/src/cascadia/TerminalApp/ActionArgs.h @@ -143,7 +143,7 @@ namespace winrt::TerminalApp::implementation struct SwitchToTabArgs : public SwitchToTabArgsT { SwitchToTabArgs() = default; - GETSET_PROPERTY(int32_t, TabIndex, 0); + GETSET_PROPERTY(uint32_t, TabIndex, 0); static constexpr std::string_view TabIndexKey{ "index" }; @@ -163,7 +163,7 @@ namespace winrt::TerminalApp::implementation auto args = winrt::make_self(); if (auto tabIndex{ json[JsonKey(TabIndexKey)] }) { - args->_TabIndex = tabIndex.asInt(); + args->_TabIndex = tabIndex.asUInt(); } return *args; } diff --git a/src/cascadia/TerminalApp/ActionArgs.idl b/src/cascadia/TerminalApp/ActionArgs.idl index 92b21f6f7b5..88679099948 100644 --- a/src/cascadia/TerminalApp/ActionArgs.idl +++ b/src/cascadia/TerminalApp/ActionArgs.idl @@ -60,7 +60,7 @@ namespace TerminalApp [default_interface] runtimeclass SwitchToTabArgs : IActionArgs { - Int32 TabIndex { get; }; + UInt32 TabIndex { get; }; }; [default_interface] runtimeclass ResizePaneArgs : IActionArgs diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index ec4aa739d67..0bb300e5cf3 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -793,12 +793,15 @@ namespace winrt::TerminalApp::implementation // - Duplicates the current focused tab void TerminalPage::_DuplicateTabViewItem() { - auto focusedTab{ _GetStrongTabImpl(_GetFocusedTabIndex()) }; - const auto& profileGuid = focusedTab->GetFocusedProfile(); - if (profileGuid.has_value()) + if (auto index{ _GetFocusedTabIndex() }) { - const auto settings = _settings->BuildSettings(profileGuid.value()); - _CreateNewTabFromSettings(profileGuid.value(), settings); + auto focusedTab = _GetStrongTabImpl(*index); + const auto& profileGuid = focusedTab->GetFocusedProfile(); + if (profileGuid.has_value()) + { + const auto settings = _settings->BuildSettings(profileGuid.value()); + _CreateNewTabFromSettings(profileGuid.value(), settings); + } } } @@ -835,20 +838,23 @@ namespace winrt::TerminalApp::implementation _lastTabClosedHandlers(*this, nullptr); } - auto focusedTabIndex = _GetFocusedTabIndex(); - if (gsl::narrow_cast(tabIndex) == focusedTabIndex) + if (auto indexOpt{ _GetFocusedTabIndex() }) { - auto const tabCount = gsl::narrow_cast(_tabs.Size()); - if (focusedTabIndex >= tabCount) - { - focusedTabIndex = tabCount - 1; - } - else if (focusedTabIndex < 0) + auto focusedTabIndex = *indexOpt; + if (tabIndex == focusedTabIndex) { - focusedTabIndex = 0; - } + uint32_t tabCount = _tabs.Size(); + if (focusedTabIndex >= tabCount) + { + focusedTabIndex = tabCount - 1; + } + else if (focusedTabIndex < 0) + { + focusedTabIndex = 0; + } - _SelectTab(focusedTabIndex); + _SelectTab(focusedTabIndex); + } } } @@ -893,13 +899,14 @@ namespace winrt::TerminalApp::implementation // - Sets focus to the tab to the right or left the currently selected tab. void TerminalPage::_SelectNextTab(const bool bMoveRight) { - int focusedTabIndex = _GetFocusedTabIndex(); - auto tabCount = _tabs.Size(); - // Wraparound math. By adding tabCount and then calculating modulo tabCount, - // we clamp the values to the range [0, tabCount) while still supporting moving - // leftward from 0 to tabCount - 1. - _SetFocusedTabIndex( - static_cast((tabCount + focusedTabIndex + (bMoveRight ? 1 : -1)) % tabCount)); + if (auto index{ _GetFocusedTabIndex() }) + { + uint32_t tabCount = _tabs.Size(); + // Wraparound math. By adding tabCount and then calculating modulo tabCount, + // we clamp the values to the range [0, tabCount) while still supporting moving + // leftward from 0 to tabCount - 1. + _SetFocusedTabIndex(((tabCount + *index + (bMoveRight ? 1 : -1)) % tabCount)); + } } // Method Description: @@ -907,9 +914,9 @@ namespace winrt::TerminalApp::implementation // is greater than the number of tabs we have. // Return Value: // true iff we were able to select that tab index, false otherwise - bool TerminalPage::_SelectTab(const int tabIndex) + bool TerminalPage::_SelectTab(const uint32_t& tabIndex) { - if (tabIndex >= 0 && tabIndex < gsl::narrow_cast(_tabs.Size())) + if (tabIndex >= 0 && tabIndex < _tabs.Size()) { _SetFocusedTabIndex(tabIndex); return true; @@ -927,14 +934,24 @@ namespace winrt::TerminalApp::implementation // - void TerminalPage::_MoveFocus(const Direction& direction) { - auto focusedTab{ _GetStrongTabImpl(_GetFocusedTabIndex()) }; - focusedTab->NavigateFocus(direction); + if (auto index{ _GetFocusedTabIndex() }) + { + auto focusedTab{ _GetStrongTabImpl(*index) }; + focusedTab->NavigateFocus(direction); + } } winrt::Microsoft::Terminal::TerminalControl::TermControl TerminalPage::_GetActiveControl() { - auto focusedTab{ _GetStrongTabImpl(_GetFocusedTabIndex()) }; - return focusedTab->GetActiveTerminalControl(); + if (auto index{ _GetFocusedTabIndex() }) + { + auto focusedTab{ _GetStrongTabImpl(*index) }; + return focusedTab->GetActiveTerminalControl(); + } + else + { + return nullptr; + } } // Method Description: @@ -942,7 +959,7 @@ namespace winrt::TerminalApp::implementation // no tab is currently selected, returns -1. // Return Value: // - the index of the currently focused tab if there is one, else -1 - int TerminalPage::_GetFocusedTabIndex() const + std::optional TerminalPage::_GetFocusedTabIndex() const noexcept { // GH#1117: This is a workaround because _tabView.SelectedIndex() // sometimes return incorrect result after removing some tabs @@ -951,10 +968,10 @@ namespace winrt::TerminalApp::implementation { return focusedIndex; } - return -1; + return std::nullopt; } - winrt::fire_and_forget TerminalPage::_SetFocusedTabIndex(int tabIndex) + winrt::fire_and_forget TerminalPage::_SetFocusedTabIndex(const uint32_t& tabIndex) { // GH#1117: This is a workaround because _tabView.SelectedIndex(tabIndex) // sometimes set focus to an incorrect tab after removing some tabs @@ -973,8 +990,10 @@ namespace winrt::TerminalApp::implementation // - Close the currently focused tab. Focus will move to the left, if possible. void TerminalPage::_CloseFocusedTab() { - uint32_t focusedTabIndex = _GetFocusedTabIndex(); - _RemoveTabViewItemByIndex(focusedTabIndex); + if (auto index{ _GetFocusedTabIndex() }) + { + _RemoveTabViewItemByIndex(*index); + } } // Method Description: @@ -983,8 +1002,11 @@ namespace winrt::TerminalApp::implementation // tab's Closed event. void TerminalPage::_CloseFocusedPane() { - auto focusedTab{ _GetStrongTabImpl(_GetFocusedTabIndex()) }; - focusedTab->ClosePane(); + if (auto index{ _GetFocusedTabIndex() }) + { + auto focusedTab{ _GetStrongTabImpl(*index) }; + focusedTab->ClosePane(); + } } // Method Description: @@ -1021,8 +1043,11 @@ namespace winrt::TerminalApp::implementation // - delta: a number of lines to move the viewport relative to the current viewport. void TerminalPage::_Scroll(int delta) { - auto focusedTab{ _GetStrongTabImpl(_GetFocusedTabIndex()) }; - focusedTab->Scroll(delta); + if (auto index{ _GetFocusedTabIndex() }) + { + auto focusedTab{ _GetStrongTabImpl(*index) }; + focusedTab->Scroll(delta); + } } // Method Description: @@ -1044,12 +1069,20 @@ namespace winrt::TerminalApp::implementation return; } + auto indexOpt = _GetFocusedTabIndex(); + + // Do nothing if for some reason, there's no tab in focus. We don't want to crash. + if (!indexOpt) + { + return; + } + + auto focusedTab = _GetStrongTabImpl(*indexOpt); + const auto [realGuid, controlSettings] = _settings->BuildSettings(newTerminalArgs); const auto controlConnection = _CreateConnectionFromSettings(realGuid, controlSettings); - auto focusedTab{ _GetStrongTabImpl(_GetFocusedTabIndex()) }; - const auto canSplit = focusedTab->CanSplitPane(splitType); if (!canSplit) @@ -1075,8 +1108,11 @@ namespace winrt::TerminalApp::implementation // - void TerminalPage::_ResizePane(const Direction& direction) { - auto focusedTab{ _GetStrongTabImpl(_GetFocusedTabIndex()) }; - focusedTab->ResizePane(direction); + if (auto index{ _GetFocusedTabIndex() }) + { + auto focusedTab{ _GetStrongTabImpl(*index) }; + focusedTab->ResizePane(direction); + } } // Method Description: @@ -1089,11 +1125,17 @@ namespace winrt::TerminalApp::implementation // is clamped between -1 and 1) void TerminalPage::_ScrollPage(int delta) { + auto indexOpt = _GetFocusedTabIndex(); + // Do nothing if for some reason, there's no tab in focus. We don't want to crash. + if (!indexOpt) + { + return; + } + delta = std::clamp(delta, -1, 1); const auto control = _GetActiveControl(); const auto termHeight = control.GetViewHeight(); - - auto focusedTab{ _GetStrongTabImpl(_GetFocusedTabIndex()) }; + auto focusedTab{ _GetStrongTabImpl(*indexOpt) }; focusedTab->Scroll(termHeight * delta); } @@ -1203,13 +1245,13 @@ namespace winrt::TerminalApp::implementation { if (_settings->GlobalSettings().SnapToGridOnResize()) { - auto focusedTab{ _GetStrongTabImpl(_GetFocusedTabIndex()) }; - return focusedTab->CalcSnappedDimension(widthOrHeight, dimension); - } - else - { - return dimension; + if (auto index{ _GetFocusedTabIndex() }) + { + auto focusedTab{ _GetStrongTabImpl(*index) }; + return focusedTab->CalcSnappedDimension(widthOrHeight, dimension); + } } + return dimension; } // Method Description: diff --git a/src/cascadia/TerminalApp/TerminalPage.h b/src/cascadia/TerminalApp/TerminalPage.h index b8eac4995de..2e9f3884301 100644 --- a/src/cascadia/TerminalApp/TerminalPage.h +++ b/src/cascadia/TerminalApp/TerminalPage.h @@ -106,12 +106,12 @@ namespace winrt::TerminalApp::implementation void _RegisterTerminalEvents(Microsoft::Terminal::TerminalControl::TermControl term, Tab& hostingTab); void _SelectNextTab(const bool bMoveRight); - bool _SelectTab(const int tabIndex); + bool _SelectTab(const uint32_t& tabIndex); void _MoveFocus(const Direction& direction); winrt::Microsoft::Terminal::TerminalControl::TermControl _GetActiveControl(); - int _GetFocusedTabIndex() const; - winrt::fire_and_forget _SetFocusedTabIndex(int tabIndex); + std::optional _GetFocusedTabIndex() const noexcept; + winrt::fire_and_forget _SetFocusedTabIndex(const uint32_t& tabIndex); void _CloseFocusedTab(); void _CloseFocusedPane(); void _CloseAllTabs(); From 0b447ba531f2e5001e0a628f5e3326acc87d0cfe Mon Sep 17 00:00:00 2001 From: Leon Liang Date: Sun, 2 Feb 2020 18:24:30 -0800 Subject: [PATCH 14/17] changing function signatures to NOT take reference --- src/cascadia/TerminalApp/TerminalPage.cpp | 6 +++--- src/cascadia/TerminalApp/TerminalPage.h | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index 0bb300e5cf3..47afb85d028 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -914,7 +914,7 @@ namespace winrt::TerminalApp::implementation // is greater than the number of tabs we have. // Return Value: // true iff we were able to select that tab index, false otherwise - bool TerminalPage::_SelectTab(const uint32_t& tabIndex) + bool TerminalPage::_SelectTab(const uint32_t tabIndex) { if (tabIndex >= 0 && tabIndex < _tabs.Size()) { @@ -971,7 +971,7 @@ namespace winrt::TerminalApp::implementation return std::nullopt; } - winrt::fire_and_forget TerminalPage::_SetFocusedTabIndex(const uint32_t& tabIndex) + winrt::fire_and_forget TerminalPage::_SetFocusedTabIndex(const uint32_t tabIndex) { // GH#1117: This is a workaround because _tabView.SelectedIndex(tabIndex) // sometimes set focus to an incorrect tab after removing some tabs @@ -1674,7 +1674,7 @@ namespace winrt::TerminalApp::implementation // - index: an unsigned integer index to a tab in _tabs // Return Value: // - a com_ptr to the implementation type of the Tab - winrt::com_ptr TerminalPage::_GetStrongTabImpl(const uint32_t& index) const + winrt::com_ptr TerminalPage::_GetStrongTabImpl(const uint32_t index) const { winrt::com_ptr tabImpl; tabImpl.copy_from(winrt::get_self(_tabs.GetAt(index))); diff --git a/src/cascadia/TerminalApp/TerminalPage.h b/src/cascadia/TerminalApp/TerminalPage.h index 2e9f3884301..a75fdf997ed 100644 --- a/src/cascadia/TerminalApp/TerminalPage.h +++ b/src/cascadia/TerminalApp/TerminalPage.h @@ -64,7 +64,7 @@ namespace winrt::TerminalApp::implementation std::shared_ptr<::TerminalApp::CascadiaSettings> _settings{ nullptr }; Windows::Foundation::Collections::IObservableVector _tabs; - winrt::com_ptr _GetStrongTabImpl(const uint32_t& index) const; + winrt::com_ptr _GetStrongTabImpl(const uint32_t index) const; bool _isFullscreen{ false }; @@ -106,12 +106,12 @@ namespace winrt::TerminalApp::implementation void _RegisterTerminalEvents(Microsoft::Terminal::TerminalControl::TermControl term, Tab& hostingTab); void _SelectNextTab(const bool bMoveRight); - bool _SelectTab(const uint32_t& tabIndex); + bool _SelectTab(const uint32_t tabIndex); void _MoveFocus(const Direction& direction); winrt::Microsoft::Terminal::TerminalControl::TermControl _GetActiveControl(); std::optional _GetFocusedTabIndex() const noexcept; - winrt::fire_and_forget _SetFocusedTabIndex(const uint32_t& tabIndex); + winrt::fire_and_forget _SetFocusedTabIndex(const uint32_t tabIndex); void _CloseFocusedTab(); void _CloseFocusedPane(); void _CloseAllTabs(); From ce4c2ac3304e75216c901f797450f40f9d72954b Mon Sep 17 00:00:00 2001 From: Leon Liang <57155886+leonMSFT@users.noreply.github.com> Date: Tue, 4 Feb 2020 11:26:03 -0800 Subject: [PATCH 15/17] Update src/cascadia/LocalTests_TerminalApp/CommandlineTest.cpp Co-Authored-By: Carlos Zamora --- src/cascadia/LocalTests_TerminalApp/CommandlineTest.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/cascadia/LocalTests_TerminalApp/CommandlineTest.cpp b/src/cascadia/LocalTests_TerminalApp/CommandlineTest.cpp index 7cdda534908..033c45a8326 100644 --- a/src/cascadia/LocalTests_TerminalApp/CommandlineTest.cpp +++ b/src/cascadia/LocalTests_TerminalApp/CommandlineTest.cpp @@ -887,7 +887,8 @@ namespace TerminalAppLocalTests VERIFY_IS_NOT_NULL(actionAndArgs.Args()); auto myArgs = actionAndArgs.Args().try_as(); VERIFY_IS_NOT_NULL(myArgs); - VERIFY_ARE_EQUAL((uint32_t)2, myArgs.TabIndex()); + VERIFY_ARE_EQUAL(static_cast(2), myArgs.TabIndex()); + } { From 21b82c056986031049b7aaacd685fba16775e1b2 Mon Sep 17 00:00:00 2001 From: Leon Liang Date: Tue, 4 Feb 2020 12:20:30 -0800 Subject: [PATCH 16/17] f o r m a t t i n g --- src/cascadia/LocalTests_TerminalApp/CommandlineTest.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/cascadia/LocalTests_TerminalApp/CommandlineTest.cpp b/src/cascadia/LocalTests_TerminalApp/CommandlineTest.cpp index 033c45a8326..721ca500c04 100644 --- a/src/cascadia/LocalTests_TerminalApp/CommandlineTest.cpp +++ b/src/cascadia/LocalTests_TerminalApp/CommandlineTest.cpp @@ -888,7 +888,6 @@ namespace TerminalAppLocalTests auto myArgs = actionAndArgs.Args().try_as(); VERIFY_IS_NOT_NULL(myArgs); VERIFY_ARE_EQUAL(static_cast(2), myArgs.TabIndex()); - } { From b9203564cf2882282001fb4714b6c4cd1dfe4045 Mon Sep 17 00:00:00 2001 From: Leon Liang Date: Tue, 4 Feb 2020 13:25:19 -0800 Subject: [PATCH 17/17] overloading the function --- src/cascadia/TerminalApp/TerminalPage.cpp | 31 ++++++++++++++++------- src/cascadia/TerminalApp/TerminalPage.h | 1 + 2 files changed, 23 insertions(+), 9 deletions(-) diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index 47afb85d028..e1f4d1f3b58 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -1425,10 +1425,10 @@ namespace winrt::TerminalApp::implementation auto selectedIndex = tabView.SelectedIndex(); // Unfocus all the tabs. - for (uint32_t idx = 0; idx < _tabs.Size(); ++idx) + for (auto tab : _tabs) { - auto tab{ _GetStrongTabImpl(idx) }; - tab->SetFocused(false); + auto tabImpl{ _GetStrongTabImpl(tab) }; + tabImpl->SetFocused(false); } if (selectedIndex >= 0) @@ -1459,9 +1459,9 @@ namespace winrt::TerminalApp::implementation void TerminalPage::_OnContentSizeChanged(const IInspectable& /*sender*/, Windows::UI::Xaml::SizeChangedEventArgs const& e) { const auto newSize = e.NewSize(); - for (uint32_t idx = 0; idx < _tabs.Size(); ++idx) + for (auto tab : _tabs) { - auto tabImpl{ _GetStrongTabImpl(idx) }; + auto tabImpl{ _GetStrongTabImpl(tab) }; tabImpl->ResizeContent(newSize); } } @@ -1512,18 +1512,18 @@ namespace winrt::TerminalApp::implementation const GUID profileGuid = profile.GetGuid(); const auto settings = _settings->BuildSettings(profileGuid); - for (uint32_t idx = 0; idx < _tabs.Size(); ++idx) + for (auto tab : _tabs) { // Attempt to reload the settings of any panes with this profile - auto tabImpl{ _GetStrongTabImpl(idx) }; + auto tabImpl{ _GetStrongTabImpl(tab) }; tabImpl->UpdateSettings(settings, profileGuid); } } // Update the icon of the tab for the currently focused profile in that tab. - for (uint32_t idx = 0; idx < _tabs.Size(); ++idx) + for (auto tab : _tabs) { - auto tabImpl{ _GetStrongTabImpl(idx) }; + auto tabImpl{ _GetStrongTabImpl(tab) }; _UpdateTabIcon(*tabImpl); _UpdateTitle(*tabImpl); } @@ -1681,6 +1681,19 @@ namespace winrt::TerminalApp::implementation return tabImpl; } + // Method Description: + // - Returns a com_ptr to the implementation type of the given projected Tab + // Arguments: + // - tab: the projected type of a Tab + // Return Value: + // - a com_ptr to the implementation type of the Tab + winrt::com_ptr TerminalPage::_GetStrongTabImpl(const ::winrt::TerminalApp::Tab& tab) const + { + winrt::com_ptr tabImpl; + tabImpl.copy_from(winrt::get_self(tab)); + return tabImpl; + } + // -------------------------------- WinRT Events --------------------------------- // Winrt events need a method for adding a callback to the event and removing the callback. // These macros will define them both for you. diff --git a/src/cascadia/TerminalApp/TerminalPage.h b/src/cascadia/TerminalApp/TerminalPage.h index a75fdf997ed..3da62c5b03d 100644 --- a/src/cascadia/TerminalApp/TerminalPage.h +++ b/src/cascadia/TerminalApp/TerminalPage.h @@ -65,6 +65,7 @@ namespace winrt::TerminalApp::implementation Windows::Foundation::Collections::IObservableVector _tabs; winrt::com_ptr _GetStrongTabImpl(const uint32_t index) const; + winrt::com_ptr _GetStrongTabImpl(const ::winrt::TerminalApp::Tab& tab) const; bool _isFullscreen{ false };