From 138fd3fe37c9355b26d35e8b756f06f5d179b02a Mon Sep 17 00:00:00 2001 From: Richard Szalay Date: Wed, 21 Aug 2019 09:19:26 +1000 Subject: [PATCH 1/4] Merge pane splitting methods Having separate Horizontal/Vertical versions made it hard to manage, and App.cpp already made use of Pane::SplitState so it made sense to have that be the descriminator --- src/cascadia/TerminalApp/App.cpp | 6 +-- src/cascadia/TerminalApp/Pane.cpp | 75 +++++-------------------------- src/cascadia/TerminalApp/Pane.h | 7 +-- src/cascadia/TerminalApp/Tab.cpp | 37 ++++----------- src/cascadia/TerminalApp/Tab.h | 6 +-- 5 files changed, 26 insertions(+), 105 deletions(-) diff --git a/src/cascadia/TerminalApp/App.cpp b/src/cascadia/TerminalApp/App.cpp index 6c88340b844..35e14d9c252 100644 --- a/src/cascadia/TerminalApp/App.cpp +++ b/src/cascadia/TerminalApp/App.cpp @@ -1493,8 +1493,7 @@ namespace winrt::TerminalApp::implementation const int focusedTabIndex = _GetFocusedTabIndex(); auto focusedTab = _tabs[focusedTabIndex]; - const auto canSplit = splitType == Pane::SplitState::Horizontal ? focusedTab->CanAddHorizontalSplit() : - focusedTab->CanAddVerticalSplit(); + const auto canSplit = focusedTab->CanAddSplit(splitType); if (!canSplit) { @@ -1506,8 +1505,7 @@ namespace winrt::TerminalApp::implementation // Hookup our event handlers to the new terminal _RegisterTerminalEvents(newControl, focusedTab); - return splitType == Pane::SplitState::Horizontal ? focusedTab->AddHorizontalSplit(realGuid, newControl) : - focusedTab->AddVerticalSplit(realGuid, newControl); + focusedTab->AddSplit(splitType, realGuid, newControl); } // Method Description: diff --git a/src/cascadia/TerminalApp/Pane.cpp b/src/cascadia/TerminalApp/Pane.cpp index 10e3bd09d6a..8ea82576e68 100644 --- a/src/cascadia/TerminalApp/Pane.cpp +++ b/src/cascadia/TerminalApp/Pane.cpp @@ -778,110 +778,57 @@ void Pane::_ApplySplitDefinitions() } // Method Description: -// - Determines whether the pane can be split vertically +// - Determines whether the pane can be split // Arguments: // - splitType: what type of split we want to create. // Return Value: -// - True if the pane can be split vertically. False otherwise. -bool Pane::CanSplitVertical() +// - True if the pane can be split. False otherwise. +bool Pane::CanSplit(SplitState splitType) { if (!_IsLeaf()) { if (_firstChild->_HasFocusedChild()) { - return _firstChild->CanSplitVertical(); + return _firstChild->CanSplit(splitType); } else if (_secondChild->_HasFocusedChild()) { - return _secondChild->CanSplitVertical(); + return _secondChild->CanSplit(splitType); } return false; } - return _CanSplit(SplitState::Vertical); + return _CanSplit(splitType); } // Method Description: -// - Vertically split the focused pane in our tree of panes, and place the given +// - Split the focused pane in our tree of panes, and place the given // TermControl into the newly created pane. If we're the focused pane, then // we'll create two new children, and place them side-by-side in our Grid. // Arguments: -// - profile: The profile GUID to associate with the newly created pane. -// - control: A TermControl to use in the new pane. -// Return Value: -// - -void Pane::SplitVertical(const GUID& profile, const TermControl& control) -{ - // If we're not the leaf, recurse into our children to split them. - if (!_IsLeaf()) - { - if (_firstChild->_HasFocusedChild()) - { - _firstChild->SplitVertical(profile, control); - } - else if (_secondChild->_HasFocusedChild()) - { - _secondChild->SplitVertical(profile, control); - } - - return; - } - - _Split(SplitState::Vertical, profile, control); -} - -// Method Description: -// - Determines whether the pane can be split horizontally -// Arguments: // - splitType: what type of split we want to create. -// Return Value: -// - True if the pane can be split horizontally. False otherwise. -bool Pane::CanSplitHorizontal() -{ - if (!_IsLeaf()) - { - if (_firstChild->_HasFocusedChild()) - { - return _firstChild->CanSplitHorizontal(); - } - else if (_secondChild->_HasFocusedChild()) - { - return _secondChild->CanSplitHorizontal(); - } - - return false; - } - - return _CanSplit(SplitState::Horizontal); -} - -// Method Description: -// - Horizontally split the focused pane in our tree of panes, and place the given -// TermControl into the newly created pane. If we're the focused pane, then -// we'll create two new children, and place them side-by-side in our Grid. -// Arguments: // - profile: The profile GUID to associate with the newly created pane. // - control: A TermControl to use in the new pane. // Return Value: // - -void Pane::SplitHorizontal(const GUID& profile, const TermControl& control) +void Pane::Split(SplitState splitType, const GUID& profile, const TermControl& control) { if (!_IsLeaf()) { if (_firstChild->_HasFocusedChild()) { - _firstChild->SplitHorizontal(profile, control); + _firstChild->Split(splitType, profile, control); } else if (_secondChild->_HasFocusedChild()) { - _secondChild->SplitHorizontal(profile, control); + _secondChild->Split(splitType, profile, control); } return; } - _Split(SplitState::Horizontal, profile, control); + _Split(splitType, profile, control); } // Method Description: diff --git a/src/cascadia/TerminalApp/Pane.h b/src/cascadia/TerminalApp/Pane.h index 31979b6d090..d526153720e 100644 --- a/src/cascadia/TerminalApp/Pane.h +++ b/src/cascadia/TerminalApp/Pane.h @@ -49,11 +49,8 @@ class Pane : public std::enable_shared_from_this bool ResizePane(const winrt::TerminalApp::Direction& direction); bool NavigateFocus(const winrt::TerminalApp::Direction& direction); - bool CanSplitHorizontal(); - void SplitHorizontal(const GUID& profile, const winrt::Microsoft::Terminal::TerminalControl::TermControl& control); - - bool CanSplitVertical(); - void SplitVertical(const GUID& profile, const winrt::Microsoft::Terminal::TerminalControl::TermControl& control); + bool CanSplit(SplitState splitType); + void Split(SplitState splitType, const GUID& profile, const winrt::Microsoft::Terminal::TerminalControl::TermControl& control); void Close(); diff --git a/src/cascadia/TerminalApp/Tab.cpp b/src/cascadia/TerminalApp/Tab.cpp index 4870d227f4b..603bd7a5fbe 100644 --- a/src/cascadia/TerminalApp/Tab.cpp +++ b/src/cascadia/TerminalApp/Tab.cpp @@ -204,47 +204,28 @@ void Tab::Scroll(const int delta) } // Method Description: -// - Determines whether the focused pane has sufficient space to be split vertically. -// Return Value: -// - True if the focused pane can be split horizontally. False otherwise. -bool Tab::CanAddVerticalSplit() -{ - return _rootPane->CanSplitVertical(); -} - -// Method Description: -// - Vertically split the focused pane in our tree of panes, and place the -// given TermControl into the newly created pane. +// - Determines whether the focused pane has sufficient space to be split. // Arguments: -// - profile: The profile GUID to associate with the newly created pane. -// - control: A TermControl to use in the new pane. -// Return Value: -// - -void Tab::AddVerticalSplit(const GUID& profile, TermControl& control) -{ - _rootPane->SplitVertical(profile, control); -} - -// Method Description: -// - Determines whether the focused pane has sufficient space to be split horizontally. +// - splitType: The type of split we want to create. // Return Value: -// - True if the focused pane can be split horizontally. False otherwise. -bool Tab::CanAddHorizontalSplit() +// - True if the focused pane can be split. False otherwise. +bool Tab::CanAddSplit(Pane::SplitState splitType) { - return _rootPane->CanSplitHorizontal(); + return _rootPane->CanSplit(splitType); } // Method Description: -// - Horizontally split the focused pane in our tree of panes, and place the +// - 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::AddHorizontalSplit(const GUID& profile, TermControl& control) +void Tab::AddSplit(Pane::SplitState splitType, const GUID& profile, TermControl& control) { - _rootPane->SplitHorizontal(profile, control); + _rootPane->Split(splitType, profile, control); } // Method Description: diff --git a/src/cascadia/TerminalApp/Tab.h b/src/cascadia/TerminalApp/Tab.h index 99c782201e3..5adac595fed 100644 --- a/src/cascadia/TerminalApp/Tab.h +++ b/src/cascadia/TerminalApp/Tab.h @@ -19,10 +19,8 @@ class Tab void SetFocused(const bool focused); void Scroll(const int delta); - bool CanAddVerticalSplit(); - void AddVerticalSplit(const GUID& profile, winrt::Microsoft::Terminal::TerminalControl::TermControl& control); - bool CanAddHorizontalSplit(); - void AddHorizontalSplit(const GUID& profile, winrt::Microsoft::Terminal::TerminalControl::TermControl& control); + bool CanAddSplit(Pane::SplitState splitType); + void AddSplit(Pane::SplitState splitType, const GUID& profile, winrt::Microsoft::Terminal::TerminalControl::TermControl& control); void UpdateFocus(); void UpdateIcon(const winrt::hstring iconPath); From 76ff6850051bdd2a14fe97b3230124832528af90 Mon Sep 17 00:00:00 2001 From: Richard Szalay Date: Wed, 21 Aug 2019 10:14:01 +1000 Subject: [PATCH 2/4] Rename Tab::(Can)AddSplit to (Can)SplitPane to align with Pane methods Split was used as a noun in Tab but a verb in Pane, which felt odd --- src/cascadia/TerminalApp/App.cpp | 4 ++-- src/cascadia/TerminalApp/Tab.cpp | 4 ++-- src/cascadia/TerminalApp/Tab.h | 5 +++-- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/cascadia/TerminalApp/App.cpp b/src/cascadia/TerminalApp/App.cpp index 35e14d9c252..439a6367907 100644 --- a/src/cascadia/TerminalApp/App.cpp +++ b/src/cascadia/TerminalApp/App.cpp @@ -1493,7 +1493,7 @@ namespace winrt::TerminalApp::implementation const int focusedTabIndex = _GetFocusedTabIndex(); auto focusedTab = _tabs[focusedTabIndex]; - const auto canSplit = focusedTab->CanAddSplit(splitType); + const auto canSplit = focusedTab->CanSplitPane(splitType); if (!canSplit) { @@ -1505,7 +1505,7 @@ namespace winrt::TerminalApp::implementation // Hookup our event handlers to the new terminal _RegisterTerminalEvents(newControl, focusedTab); - focusedTab->AddSplit(splitType, realGuid, newControl); + focusedTab->SplitPane(splitType, realGuid, newControl); } // Method Description: diff --git a/src/cascadia/TerminalApp/Tab.cpp b/src/cascadia/TerminalApp/Tab.cpp index 603bd7a5fbe..c4e40032695 100644 --- a/src/cascadia/TerminalApp/Tab.cpp +++ b/src/cascadia/TerminalApp/Tab.cpp @@ -209,7 +209,7 @@ void Tab::Scroll(const int delta) // - splitType: The type of split we want to create. // Return Value: // - True if the focused pane can be split. False otherwise. -bool Tab::CanAddSplit(Pane::SplitState splitType) +bool Tab::CanSplitPane(Pane::SplitState splitType) { return _rootPane->CanSplit(splitType); } @@ -223,7 +223,7 @@ bool Tab::CanAddSplit(Pane::SplitState splitType) // - control: A TermControl to use in the new pane. // Return Value: // - -void Tab::AddSplit(Pane::SplitState splitType, const GUID& profile, TermControl& control) +void Tab::SplitPane(Pane::SplitState splitType, const GUID& profile, TermControl& control) { _rootPane->Split(splitType, profile, control); } diff --git a/src/cascadia/TerminalApp/Tab.h b/src/cascadia/TerminalApp/Tab.h index 5adac595fed..8af4423e6b8 100644 --- a/src/cascadia/TerminalApp/Tab.h +++ b/src/cascadia/TerminalApp/Tab.h @@ -19,8 +19,9 @@ class Tab void SetFocused(const bool focused); void Scroll(const int delta); - bool CanAddSplit(Pane::SplitState splitType); - void AddSplit(Pane::SplitState splitType, const GUID& profile, winrt::Microsoft::Terminal::TerminalControl::TermControl& control); + + bool CanSplitPane(Pane::SplitState splitType); + void SplitPane(Pane::SplitState splitType, const GUID& profile, winrt::Microsoft::Terminal::TerminalControl::TermControl& control); void UpdateFocus(); void UpdateIcon(const winrt::hstring iconPath); From 50d7e097314e5ccf86f130b8dc4b7e8238406949 Mon Sep 17 00:00:00 2001 From: Richard Szalay Date: Wed, 21 Aug 2019 10:14:26 +1000 Subject: [PATCH 3/4] Remove unused local variable in Pane::_CanSplit --- src/cascadia/TerminalApp/Pane.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/cascadia/TerminalApp/Pane.cpp b/src/cascadia/TerminalApp/Pane.cpp index 8ea82576e68..5b12e7fa294 100644 --- a/src/cascadia/TerminalApp/Pane.cpp +++ b/src/cascadia/TerminalApp/Pane.cpp @@ -839,8 +839,6 @@ void Pane::Split(SplitState splitType, const GUID& profile, const TermControl& c // - True if the pane can be split. False otherwise. bool Pane::_CanSplit(SplitState splitType) { - const bool changeWidth = _splitState == SplitState::Vertical; - const Size actualSize{ gsl::narrow_cast(_root.ActualWidth()), gsl::narrow_cast(_root.ActualHeight()) }; From a09a5917783689b6e7c7895887cba92587390ade Mon Sep 17 00:00:00 2001 From: Richard Szalay Date: Wed, 21 Aug 2019 10:21:57 +1000 Subject: [PATCH 4/4] Remove redundant 'else' branches in Pane Improves readibility. All 'low hanging fruit' cases where the 'if' was returning. --- src/cascadia/TerminalApp/Pane.cpp | 121 +++++++++++++++--------------- 1 file changed, 59 insertions(+), 62 deletions(-) diff --git a/src/cascadia/TerminalApp/Pane.cpp b/src/cascadia/TerminalApp/Pane.cpp index 5b12e7fa294..3096dddb362 100644 --- a/src/cascadia/TerminalApp/Pane.cpp +++ b/src/cascadia/TerminalApp/Pane.cpp @@ -164,26 +164,26 @@ bool Pane::ResizePane(const Direction& direction) { return _Resize(direction); } - else + + // If neither of our children were the focused leaf, then recurse into + // our children and see if they can handle the resize. + // For each child, if it has a focused descendant, try having that child + // handle the resize. + // If the child wasn't able to handle the resize, it's possible that + // there were no descendants with a separator the correct direction. If + // our separator _is_ the correct direction, then we should be the pane + // to resize. Otherwise, just return false, as we couldn't handle it + // either. + if ((!_firstChild->_IsLeaf()) && _firstChild->_HasFocusedChild()) { - // If neither of our children were the focused leaf, then recurse into - // our children and see if they can handle the resize. - // For each child, if it has a focused descendant, try having that child - // handle the resize. - // If the child wasn't able to handle the resize, it's possible that - // there were no descendants with a separator the correct direction. If - // our separator _is_ the correct direction, then we should be the pane - // to resize. Otherwise, just return false, as we couldn't handle it - // either. - if ((!_firstChild->_IsLeaf()) && _firstChild->_HasFocusedChild()) - { - return _firstChild->ResizePane(direction) || _Resize(direction); - } - else if ((!_secondChild->_IsLeaf()) && _secondChild->_HasFocusedChild()) - { - return _secondChild->ResizePane(direction) || _Resize(direction); - } + return _firstChild->ResizePane(direction) || _Resize(direction); } + + if ((!_secondChild->_IsLeaf()) && _secondChild->_HasFocusedChild()) + { + return _secondChild->ResizePane(direction) || _Resize(direction); + } + return false; } @@ -253,26 +253,26 @@ bool Pane::NavigateFocus(const Direction& direction) { return _NavigateFocus(direction); } - else + + // If neither of our children were the focused leaf, then recurse into + // our children and see if they can handle the focus move. + // For each child, if it has a focused descendant, try having that child + // handle the focus move. + // If the child wasn't able to handle the focus move, it's possible that + // there were no descendants with a separator the correct direction. If + // our separator _is_ the correct direction, then we should be the pane + // to move focus into our other child. Otherwise, just return false, as + // we couldn't handle it either. + if ((!_firstChild->_IsLeaf()) && _firstChild->_HasFocusedChild()) { - // If neither of our children were the focused leaf, then recurse into - // our children and see if they can handle the focus move. - // For each child, if it has a focused descendant, try having that child - // handle the focus move. - // If the child wasn't able to handle the focus move, it's possible that - // there were no descendants with a separator the correct direction. If - // our separator _is_ the correct direction, then we should be the pane - // to move focus into our other child. Otherwise, just return false, as - // we couldn't handle it either. - if ((!_firstChild->_IsLeaf()) && _firstChild->_HasFocusedChild()) - { - return _firstChild->NavigateFocus(direction) || _NavigateFocus(direction); - } - else if ((!_secondChild->_IsLeaf()) && _secondChild->_HasFocusedChild()) - { - return _secondChild->NavigateFocus(direction) || _NavigateFocus(direction); - } + return _firstChild->NavigateFocus(direction) || _NavigateFocus(direction); } + + if ((!_secondChild->_IsLeaf()) && _secondChild->_HasFocusedChild()) + { + return _secondChild->NavigateFocus(direction) || _NavigateFocus(direction); + } + return false; } @@ -348,15 +348,13 @@ std::shared_ptr Pane::GetFocusedPane() { return _lastFocused ? shared_from_this() : nullptr; } - else + + auto firstFocused = _firstChild->GetFocusedPane(); + if (firstFocused != nullptr) { - auto firstFocused = _firstChild->GetFocusedPane(); - if (firstFocused != nullptr) - { - return firstFocused; - } - return _secondChild->GetFocusedPane(); + return firstFocused; } + return _secondChild->GetFocusedPane(); } // Method Description: @@ -785,21 +783,22 @@ void Pane::_ApplySplitDefinitions() // - True if the pane can be split. False otherwise. bool Pane::CanSplit(SplitState splitType) { - if (!_IsLeaf()) + if (_IsLeaf()) { - if (_firstChild->_HasFocusedChild()) - { - return _firstChild->CanSplit(splitType); - } - else if (_secondChild->_HasFocusedChild()) - { - return _secondChild->CanSplit(splitType); - } + return _CanSplit(splitType); + } - return false; + if (_firstChild->_HasFocusedChild()) + { + return _firstChild->CanSplit(splitType); + } + + if (_secondChild->_HasFocusedChild()) + { + return _secondChild->CanSplit(splitType); } - return _CanSplit(splitType); + return false; } // Method Description: @@ -951,14 +950,12 @@ Size Pane::_GetMinSize() const { return _control.MinimumSize(); } - else - { - const auto firstSize = _firstChild->_GetMinSize(); - const auto secondSize = _secondChild->_GetMinSize(); - const auto newWidth = firstSize.Width + secondSize.Width + (_splitState == SplitState::Vertical ? PaneSeparatorSize : 0); - const auto newHeight = firstSize.Height + secondSize.Height + (_splitState == SplitState::Horizontal ? PaneSeparatorSize : 0); - return { newWidth, newHeight }; - } + + const auto firstSize = _firstChild->_GetMinSize(); + const auto secondSize = _secondChild->_GetMinSize(); + const auto newWidth = firstSize.Width + secondSize.Width + (_splitState == SplitState::Vertical ? PaneSeparatorSize : 0); + const auto newHeight = firstSize.Height + secondSize.Height + (_splitState == SplitState::Horizontal ? PaneSeparatorSize : 0); + return { newWidth, newHeight }; } DEFINE_EVENT(Pane, Closed, _closedHandlers, ConnectionClosedEventArgs);