From e0b3190ac73256342e2f10190d385457106f616e Mon Sep 17 00:00:00 2001 From: Fred Miller Date: Sat, 18 May 2019 14:36:44 +0800 Subject: [PATCH 1/8] Fix crash when closing last tab This commit fixes crashes that happen when the last tab is being closed. Commit bc69d1a99a8921b5088c035096e659b9e0a1d50e adds code that closes the window when the last tab is being closed, but code that should only be executed when closing one of multiple open tabs continues to be executed. This leads to App::_RemoveTabViewItem trying to call TabView::SelectedIndex(1) when there is only 1 tab remaining in the TabView, resulting in an argument exception. Signed-off-by: Fred Miller --- src/cascadia/TerminalApp/App.cpp | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/src/cascadia/TerminalApp/App.cpp b/src/cascadia/TerminalApp/App.cpp index 6e139d8d05b..b7e3b35ac01 100644 --- a/src/cascadia/TerminalApp/App.cpp +++ b/src/cascadia/TerminalApp/App.cpp @@ -846,25 +846,27 @@ namespace winrt::TerminalApp::implementation // - tabViewItem: the TabViewItem in the TabView that is being removed. void App::_RemoveTabViewItem(const IInspectable& tabViewItem) { - // To close the window here, we need to close the hosting window. + uint32_t removingTabIndex; + _tabView.Items().IndexOf(tabViewItem, removingTabIndex); + if (_tabs.size() == 1) { + // To close the window here, we need to close the hosting window. _lastTabClosedHandlers(); } - uint32_t tabIndexFromControl = 0; - _tabView.Items().IndexOf(tabViewItem, tabIndexFromControl); - - if (tabIndexFromControl == _GetFocusedTabIndex()) - { - _tabView.SelectedIndex((tabIndexFromControl > 0) ? tabIndexFromControl - 1 : 1); - } - - // Removing the tab from the collection will destroy its control and disconnect its connection. - _tabs.erase(_tabs.begin() + tabIndexFromControl); - _tabView.Items().RemoveAt(tabIndexFromControl); - - // ensure tabs and focus is sync - _tabView.SelectedIndex(tabIndexFromControl > 0 ? tabIndexFromControl - 1 : 0); + else + { + // If we are closing the tab we are on, flee to an adjacent tab first! + // We go to the tab on the right if we are on the 0th tab, or the one on the left otherwise. + if (removingTabIndex == _GetFocusedTabIndex()) + { + _tabView.SelectedIndex(removingTabIndex > 0 ? removingTabIndex - 1 : 1); + } + _tabView.Items().RemoveAt(removingTabIndex); + } + + // Removing the tab from the collection will destroy its control and disconnect its connection. + _tabs.erase(_tabs.begin() + removingTabIndex); } // Method Description: From a0b9e894d822f2c44ce8a5d583d57ea4aaa8b813 Mon Sep 17 00:00:00 2001 From: Fred Miller Date: Sat, 18 May 2019 14:52:19 +0800 Subject: [PATCH 2/8] Convert tabs to spaces Signed-off-by: Fred Miller --- src/cascadia/TerminalApp/App.cpp | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/src/cascadia/TerminalApp/App.cpp b/src/cascadia/TerminalApp/App.cpp index b7e3b35ac01..511bb37fe5a 100644 --- a/src/cascadia/TerminalApp/App.cpp +++ b/src/cascadia/TerminalApp/App.cpp @@ -846,27 +846,27 @@ namespace winrt::TerminalApp::implementation // - tabViewItem: the TabViewItem in the TabView that is being removed. void App::_RemoveTabViewItem(const IInspectable& tabViewItem) { - uint32_t removingTabIndex; - _tabView.Items().IndexOf(tabViewItem, removingTabIndex); + uint32_t removingTabIndex; + _tabView.Items().IndexOf(tabViewItem, removingTabIndex); if (_tabs.size() == 1) { - // To close the window here, we need to close the hosting window. + // To close the window here, we need to close the hosting window. _lastTabClosedHandlers(); } - else - { - // If we are closing the tab we are on, flee to an adjacent tab first! - // We go to the tab on the right if we are on the 0th tab, or the one on the left otherwise. - if (removingTabIndex == _GetFocusedTabIndex()) - { - _tabView.SelectedIndex(removingTabIndex > 0 ? removingTabIndex - 1 : 1); - } - _tabView.Items().RemoveAt(removingTabIndex); - } - - // Removing the tab from the collection will destroy its control and disconnect its connection. - _tabs.erase(_tabs.begin() + removingTabIndex); + else + { + // If we are closing the tab we are on, flee to an adjacent tab first! + // We go to the tab on the right if we are on the 0th tab, or the one on the left otherwise. + if (removingTabIndex == _GetFocusedTabIndex()) + { + _tabView.SelectedIndex(removingTabIndex > 0 ? removingTabIndex - 1 : 1); + } + _tabView.Items().RemoveAt(removingTabIndex); + } + + // Removing the tab from the collection will destroy its control and disconnect its connection. + _tabs.erase(_tabs.begin() + removingTabIndex); } // Method Description: From 059c7c56812a9a412c340ccc93bae512bba1c780 Mon Sep 17 00:00:00 2001 From: Fred Miller Date: Sat, 18 May 2019 16:35:09 +0800 Subject: [PATCH 3/8] Assert function call that should not fail Signed-off-by: Fred Miller --- src/cascadia/TerminalApp/App.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cascadia/TerminalApp/App.cpp b/src/cascadia/TerminalApp/App.cpp index 511bb37fe5a..f63fcd0bc54 100644 --- a/src/cascadia/TerminalApp/App.cpp +++ b/src/cascadia/TerminalApp/App.cpp @@ -847,7 +847,7 @@ namespace winrt::TerminalApp::implementation void App::_RemoveTabViewItem(const IInspectable& tabViewItem) { uint32_t removingTabIndex; - _tabView.Items().IndexOf(tabViewItem, removingTabIndex); + WINRT_ASSERT(_tabView.Items().IndexOf(tabViewItem, removingTabIndex)); if (_tabs.size() == 1) { From f665a9b1dcd585441dd7c3d9e970bd2493aa2216 Mon Sep 17 00:00:00 2001 From: Fred Miller Date: Sat, 18 May 2019 16:50:56 +0800 Subject: [PATCH 4/8] Restore variable initialization to avoid warnings Signed-off-by: Fred Miller --- src/cascadia/TerminalApp/App.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cascadia/TerminalApp/App.cpp b/src/cascadia/TerminalApp/App.cpp index f63fcd0bc54..e5fd0fca8f8 100644 --- a/src/cascadia/TerminalApp/App.cpp +++ b/src/cascadia/TerminalApp/App.cpp @@ -846,7 +846,7 @@ namespace winrt::TerminalApp::implementation // - tabViewItem: the TabViewItem in the TabView that is being removed. void App::_RemoveTabViewItem(const IInspectable& tabViewItem) { - uint32_t removingTabIndex; + uint32_t removingTabIndex = 0; WINRT_ASSERT(_tabView.Items().IndexOf(tabViewItem, removingTabIndex)); if (_tabs.size() == 1) From 302b37d615a3297ed2bf16a1e61c934f95cb2439 Mon Sep 17 00:00:00 2001 From: Fred Miller Date: Mon, 20 May 2019 14:36:30 +0800 Subject: [PATCH 5/8] Restore removing last Tab when closing last tab This commit restores `App::_RemoveTabViewItem`'s behavior of removing the last Tab control from the TabView when the tab being closed is the last remaining, and additionally destructs the TabView control as suggested (https://github.com/microsoft/terminal/pull/892#discussion_r285345801). Also restructures the function to handle 2 circumstances (whether the tab being closed is the last one or not) explicitly. Signed-off-by: Fred Miller --- src/cascadia/TerminalApp/App.cpp | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/src/cascadia/TerminalApp/App.cpp b/src/cascadia/TerminalApp/App.cpp index e5fd0fca8f8..bcf85fdcb9c 100644 --- a/src/cascadia/TerminalApp/App.cpp +++ b/src/cascadia/TerminalApp/App.cpp @@ -846,16 +846,22 @@ namespace winrt::TerminalApp::implementation // - tabViewItem: the TabViewItem in the TabView that is being removed. void App::_RemoveTabViewItem(const IInspectable& tabViewItem) { - uint32_t removingTabIndex = 0; - WINRT_ASSERT(_tabView.Items().IndexOf(tabViewItem, removingTabIndex)); - if (_tabs.size() == 1) { + _tabView.Items().RemoveAt(0); + _tabView = nullptr; + + // Removing the tab from the collection will destroy its control and disconnect its connection. + _tabs.clear(); + // To close the window here, we need to close the hosting window. _lastTabClosedHandlers(); } else { + uint32_t removingTabIndex = 0; + WINRT_ASSERT(_tabView.Items().IndexOf(tabViewItem, removingTabIndex)); + // If we are closing the tab we are on, flee to an adjacent tab first! // We go to the tab on the right if we are on the 0th tab, or the one on the left otherwise. if (removingTabIndex == _GetFocusedTabIndex()) @@ -863,10 +869,10 @@ namespace winrt::TerminalApp::implementation _tabView.SelectedIndex(removingTabIndex > 0 ? removingTabIndex - 1 : 1); } _tabView.Items().RemoveAt(removingTabIndex); - } - // Removing the tab from the collection will destroy its control and disconnect its connection. - _tabs.erase(_tabs.begin() + removingTabIndex); + // Removing the tab from the collection will destroy its control and disconnect its connection. + _tabs.erase(_tabs.begin() + removingTabIndex); + } } // Method Description: From 953c7371fff99e3da317b04f3a91d2a88f94e2fa Mon Sep 17 00:00:00 2001 From: Fred Miller Date: Mon, 20 May 2019 14:42:35 +0800 Subject: [PATCH 6/8] Restore parentheses in ternary expression Suggested at https://github.com/microsoft/terminal/pull/892#discussion_r285345070. Signed-off-by: Fred Miller --- src/cascadia/TerminalApp/App.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cascadia/TerminalApp/App.cpp b/src/cascadia/TerminalApp/App.cpp index bcf85fdcb9c..940741a2d7a 100644 --- a/src/cascadia/TerminalApp/App.cpp +++ b/src/cascadia/TerminalApp/App.cpp @@ -866,7 +866,7 @@ namespace winrt::TerminalApp::implementation // We go to the tab on the right if we are on the 0th tab, or the one on the left otherwise. if (removingTabIndex == _GetFocusedTabIndex()) { - _tabView.SelectedIndex(removingTabIndex > 0 ? removingTabIndex - 1 : 1); + _tabView.SelectedIndex((removingTabIndex > 0) ? removingTabIndex - 1 : 1); } _tabView.Items().RemoveAt(removingTabIndex); From f8efdb96f82570b947aad176c690b58ed919840d Mon Sep 17 00:00:00 2001 From: Fred Miller Date: Tue, 21 May 2019 16:43:31 +0800 Subject: [PATCH 7/8] Fix regression of issue #708 https://github.com/microsoft/terminal/pull/892#pullrequestreview-239653710 Signed-off-by: Fred Miller --- src/cascadia/TerminalApp/App.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/cascadia/TerminalApp/App.cpp b/src/cascadia/TerminalApp/App.cpp index 940741a2d7a..a2885afa276 100644 --- a/src/cascadia/TerminalApp/App.cpp +++ b/src/cascadia/TerminalApp/App.cpp @@ -869,6 +869,8 @@ namespace winrt::TerminalApp::implementation _tabView.SelectedIndex((removingTabIndex > 0) ? removingTabIndex - 1 : 1); } _tabView.Items().RemoveAt(removingTabIndex); + // Ensure tabs and focus are in sync (see PR 737) + _tabView.SelectedIndex((removingTabIndex > 0) ? removingTabIndex - 1 : 0); // Removing the tab from the collection will destroy its control and disconnect its connection. _tabs.erase(_tabs.begin() + removingTabIndex); From 0e9fddbf7f10c36c0027483605ea82e721c51d91 Mon Sep 17 00:00:00 2001 From: Fred Miller Date: Tue, 21 May 2019 18:00:37 +0800 Subject: [PATCH 8/8] Revert "Fix regression of issue #708" https://github.com/microsoft/terminal/pull/892#discussion_r285424937 Signed-off-by: Fred Miller --- src/cascadia/TerminalApp/App.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/cascadia/TerminalApp/App.cpp b/src/cascadia/TerminalApp/App.cpp index a2885afa276..940741a2d7a 100644 --- a/src/cascadia/TerminalApp/App.cpp +++ b/src/cascadia/TerminalApp/App.cpp @@ -869,8 +869,6 @@ namespace winrt::TerminalApp::implementation _tabView.SelectedIndex((removingTabIndex > 0) ? removingTabIndex - 1 : 1); } _tabView.Items().RemoveAt(removingTabIndex); - // Ensure tabs and focus are in sync (see PR 737) - _tabView.SelectedIndex((removingTabIndex > 0) ? removingTabIndex - 1 : 0); // Removing the tab from the collection will destroy its control and disconnect its connection. _tabs.erase(_tabs.begin() + removingTabIndex);