Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Fix crash when closing last tab #892

Closed
wants to merge 8 commits into from
Closed

Fix crash when closing last tab #892

Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e0b3190
Fix crash when closing last tab
fghzxm May 18, 2019
a0b9e89
Convert tabs to spaces
fghzxm May 18, 2019
059c7c5
Assert function call that should not fail
fghzxm May 18, 2019
f665a9b
Restore variable initialization to avoid warnings
fghzxm May 18, 2019
302b37d
Restore removing last Tab when closing last tab
fghzxm May 20, 2019
953c737
Restore parentheses in ternary expression
fghzxm May 20, 2019
f8efdb9
Fix regression of issue #708
fghzxm May 21, 2019
0e9fddb
Revert "Fix regression of issue #708"
fghzxm May 21, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 13 additions & 11 deletions src/cascadia/TerminalApp/App.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = 0;
WINRT_ASSERT(_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())
else
{
_tabView.SelectedIndex((tabIndexFromControl > 0) ? tabIndexFromControl - 1 : 1);
// 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);
fghzxm marked this conversation as resolved.
Show resolved Hide resolved
}
_tabView.Items().RemoveAt(removingTabIndex);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be moved down with line 869 like it was before? Or does closing the window (line 855) basically handle the logic of removing the XAML TabView Control?

}

// 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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This got removed. But I'm noticing it's similar to line 863. Should we be removing this line and updating/replacing line 863 with this?

Copy link
Contributor Author

@fghzxm fghzxm May 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my experiments, line 867 is unnecessary. The SelectedIndex property is maintained correctly by the TabView component when a tab is closed. Fixed in f8efdb9. After further investigation I came to believe that issue #708 is not from a bug in Terminal, (which means PR #737 probably did not actually fix it,) but from one in MUX.Controls.TabView, namely it failing to maintain its SelectedIndex property in the case of one or more TabViewItems are being removed from it. I'm undoing f8efdb9 and instead opening an issue at MUX. Meanwhile unresolving.

_tabs.erase(_tabs.begin() + removingTabIndex);
}

// Method Description:
Expand Down