Skip to content

Commit

Permalink
Fix TabManagement to use tab object rather than index (#9924)
Browse files Browse the repository at this point in the history 
## PR Checklist
* [x] Closes #8374
* [x] CLA signed. 
* [x] Tests added/passed
* [ ] Documentation updated. 
* [ ] Schema updated.
* [ ] I've discussed this with core contributors already.

## Detailed Description of the Pull Request / Additional comments
The majority of the work was already done earlier.
The fix is only in _SetFocusedTab, that runs asynchronously
and thus might result in a race or even overflow.
All other changes are decorative.

## Validation Steps Performed
UT and manual tests
  • Loading branch information
@Don-Vito
Don-Vito authored Apr 23, 2021
1 parent aa54de1 commit 51920d9
Show file tree
Hide file tree
Showing 3 changed files with 58 additions and 61 deletions.
10 changes: 5 additions & 5 deletions src/cascadia/LocalTests_TerminalApp/TabTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,7 @@ namespace TerminalAppLocalTests
auto tab = page->_tabs.GetAt(0);
auto tabImpl = page->_GetTerminalTabImpl(tab);
page->_tabView.SelectedItem(tabImpl->TabViewItem());
page->_UpdatedSelectedTab(0);
page->_UpdatedSelectedTab(tab);
});
VERIFY_SUCCEEDED(result);
}
Expand Down Expand Up @@ -984,10 +984,10 @@ namespace TerminalAppLocalTests

Log::Comment(L"Select the tabs from 0 to 3");
RunOnUIThread([&page]() {
page->_UpdatedSelectedTab(0);
page->_UpdatedSelectedTab(1);
page->_UpdatedSelectedTab(2);
page->_UpdatedSelectedTab(3);
page->_UpdatedSelectedTab(page->_tabs.GetAt(0));
page->_UpdatedSelectedTab(page->_tabs.GetAt(1));
page->_UpdatedSelectedTab(page->_tabs.GetAt(2));
page->_UpdatedSelectedTab(page->_tabs.GetAt(3));
});

VERIFY_ARE_EQUAL(4u, page->_mruTabs.Size());
Expand Down
103 changes: 50 additions & 53 deletions src/cascadia/TerminalApp/TabManagement.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -406,13 +406,8 @@ namespace winrt::TerminalApp::implementation
if (tabSwitchMode == TabSwitcherMode::MostRecentlyUsed)
{
const auto newSelectedTab = _mruTabs.GetAt(0);

uint32_t newSelectedIndex;
if (_tabs.IndexOf(newSelectedTab, newSelectedIndex))
{
_UpdatedSelectedTab(newSelectedIndex);
_tabView.SelectedItem(newSelectedTab.TabViewItem());
}
_UpdatedSelectedTab(newSelectedTab);
_tabView.SelectedItem(newSelectedTab.TabViewItem());
}
else
{
Expand All @@ -431,13 +426,13 @@ namespace winrt::TerminalApp::implementation
const auto newSelectedIndex = std::clamp<int32_t>(tabIndex - 1, 0, _tabs.Size());
// _UpdatedSelectedTab will do the work of setting up the new tab as
// the focused one, and unfocusing all the others.
_UpdatedSelectedTab(newSelectedIndex);
auto newSelectedTab{ _tabs.GetAt(newSelectedIndex) };
_UpdatedSelectedTab(newSelectedTab);

// Also, we need to _manually_ set the SelectedItem of the tabView
// here. If we don't, then the TabView will technically not have a
// selected item at all, which can make things like ClosePane not
// work correctly.
auto newSelectedTab{ _tabs.GetAt(newSelectedIndex) };
_tabView.SelectedItem(newSelectedTab.TabViewItem());
}
}
Expand Down Expand Up @@ -494,15 +489,15 @@ namespace winrt::TerminalApp::implementation
{
if (tabIndex >= 0 && tabIndex < _tabs.Size())
{
auto tab{ _tabs.GetAt(tabIndex) };
if (_startupState == StartupState::InStartup)
{
auto tab{ _tabs.GetAt(tabIndex) };
_tabView.SelectedItem(tab.TabViewItem());
_UpdatedSelectedTab(tabIndex);
_UpdatedSelectedTab(tab);
}
else
{
_SetFocusedTabIndex(tabIndex);
_SetFocusedTab(tab);
}

return true;
Expand Down Expand Up @@ -588,10 +583,10 @@ namespace winrt::TerminalApp::implementation
// in TerminalPage::_OnTabSelectionChanged, where we'll mark the new tab
// as focused.
// Arguments:
// - tabIndex: the index in the list of tabs to focus.
// - tab: tab to focus.
// Return Value:
// - <none>
winrt::fire_and_forget TerminalPage::_SetFocusedTabIndex(const uint32_t tabIndex)
winrt::fire_and_forget TerminalPage::_SetFocusedTab(const winrt::TerminalApp::TabBase tab)
{
// GH#1117: This is a workaround because _tabView.SelectedIndex(tabIndex)
// sometimes set focus to an incorrect tab after removing some tabs
Expand All @@ -601,8 +596,12 @@ namespace winrt::TerminalApp::implementation

if (auto page{ weakThis.get() })
{
auto tabToFocus = page->_tabs.GetAt(tabIndex);
_tabView.SelectedItem(tabToFocus.TabViewItem());
// Make sure the tab was not removed
uint32_t tabIndex{};
if (_tabs.IndexOf(tab, tabIndex))
{
_tabView.SelectedItem(tab.TabViewItem());
}
}
}

Expand Down Expand Up @@ -726,49 +725,44 @@ namespace winrt::TerminalApp::implementation
}
}

void TerminalPage::_UpdatedSelectedTab(const int32_t index)
void TerminalPage::_UpdatedSelectedTab(const winrt::TerminalApp::TabBase& tab)
{
// Unfocus all the tabs.
for (auto tab : _tabs)
{
tab.Focus(FocusState::Unfocused);
}

if (index >= 0)
try
{
try
{
auto tab{ _tabs.GetAt(index) };

_tabContent.Children().Clear();
_tabContent.Children().Append(tab.Content());
_tabContent.Children().Clear();
_tabContent.Children().Append(tab.Content());

// GH#7409: If the tab switcher is open, then we _don't_ want to
// automatically focus the new tab here. The tab switcher wants
// to be able to "preview" the selected tab as the user tabs
// through the menu, but if we toss the focus to the control
// here, then the user won't be able to navigate the ATS any
// longer.
//
// When the tab switcher is eventually dismissed, the focus will
// get tossed back to the focused terminal control, so we don't
// need to worry about focus getting lost.
if (CommandPalette().Visibility() != Visibility::Visible)
{
tab.Focus(FocusState::Programmatic);
_UpdateMRUTab(index);
}
// GH#7409: If the tab switcher is open, then we _don't_ want to
// automatically focus the new tab here. The tab switcher wants
// to be able to "preview" the selected tab as the user tabs
// through the menu, but if we toss the focus to the control
// here, then the user won't be able to navigate the ATS any
// longer.
//
// When the tab switcher is eventually dismissed, the focus will
// get tossed back to the focused terminal control, so we don't
// need to worry about focus getting lost.
if (CommandPalette().Visibility() != Visibility::Visible)
{
tab.Focus(FocusState::Programmatic);
_UpdateMRUTab(tab);
}

tab.TabViewItem().StartBringIntoView();
tab.TabViewItem().StartBringIntoView();

// Raise an event that our title changed
if (_settings.GlobalSettings().ShowTitleInTitlebar())
{
_TitleChangedHandlers(*this, tab.Title());
}
// Raise an event that our title changed
if (_settings.GlobalSettings().ShowTitleInTitlebar())
{
_TitleChangedHandlers(*this, tab.Title());
}
CATCH_LOG();
}
CATCH_LOG();
}

// Method Description:
Expand All @@ -783,7 +777,11 @@ namespace winrt::TerminalApp::implementation
{
auto tabView = sender.as<MUX::Controls::TabView>();
auto selectedIndex = tabView.SelectedIndex();
_UpdatedSelectedTab(selectedIndex);
if (selectedIndex >= 0 && selectedIndex < gsl::narrow_cast<int32_t>(_tabs.Size()))
{
const auto tab{ _tabs.GetAt(selectedIndex) };
_UpdatedSelectedTab(tab);
}
}
}

Expand All @@ -807,13 +805,12 @@ namespace winrt::TerminalApp::implementation
// Method Description:
// - Bumps the tab in its in-order index up to the top of the mru list.
// Arguments:
// - index: the in-order index of the tab to bump.
// - tab: tab to bump.
// Return Value:
// - <none>
void TerminalPage::_UpdateMRUTab(const uint32_t index)
void TerminalPage::_UpdateMRUTab(const winrt::TerminalApp::TabBase& tab)
{
uint32_t mruIndex;
const auto tab = _tabs.GetAt(index);
if (_mruTabs.IndexOf(tab, mruIndex))
{
if (mruIndex > 0)
Expand Down Expand Up @@ -895,10 +892,10 @@ namespace winrt::TerminalApp::implementation
if (focusAlways || !_newTabButton.Flyout().IsOpen())
{
// Return focus to the active control
if (auto index{ _GetFocusedTabIndex() })
if (auto tab{ _GetFocusedTab() })
{
_tabs.GetAt(*index).Focus(FocusState::Programmatic);
_UpdateMRUTab(index.value());
tab.Focus(FocusState::Programmatic);
_UpdateMRUTab(tab);
}
}
}
Expand Down
6 changes: 3 additions & 3 deletions src/cascadia/TerminalApp/TerminalPage.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,7 @@ namespace winrt::TerminalApp::implementation
winrt::com_ptr<TerminalTab> _GetFocusedTabImpl() const noexcept;
TerminalApp::TabBase _GetTabByTabViewItem(const Microsoft::UI::Xaml::Controls::TabViewItem& tabViewItem) const noexcept;

winrt::fire_and_forget _SetFocusedTabIndex(const uint32_t tabIndex);
winrt::fire_and_forget _SetFocusedTab(const winrt::TerminalApp::TabBase tab);
void _CloseFocusedTab();
winrt::fire_and_forget _CloseFocusedPane();

Expand Down Expand Up @@ -282,7 +282,7 @@ namespace winrt::TerminalApp::implementation
void _OnContentSizeChanged(const IInspectable& /*sender*/, Windows::UI::Xaml::SizeChangedEventArgs const& e);
void _OnTabCloseRequested(const IInspectable& sender, const Microsoft::UI::Xaml::Controls::TabViewTabCloseRequestedEventArgs& eventArgs);
void _OnFirstLayout(const IInspectable& sender, const IInspectable& eventArgs);
void _UpdatedSelectedTab(const int32_t index);
void _UpdatedSelectedTab(const winrt::TerminalApp::TabBase& tab);

void _OnDispatchCommandRequested(const IInspectable& sender, const Microsoft::Terminal::Settings::Model::Command& command);
void _OnCommandLineExecutionRequested(const IInspectable& sender, const winrt::hstring& commandLine);
Expand Down Expand Up @@ -311,7 +311,7 @@ namespace winrt::TerminalApp::implementation
static int _ComputeScrollDelta(ScrollDirection scrollDirection, const uint32_t rowsToScroll);
static uint32_t _ReadSystemRowsToScroll();

void _UpdateMRUTab(const uint32_t index);
void _UpdateMRUTab(const winrt::TerminalApp::TabBase& tab);

void _TryMoveTab(const uint32_t currentTabIndex, const int32_t suggestedNewTabIndex);

Expand Down

0 comments on commit 51920d9

Please sign in to comment.