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

Jump to bottom

Fix TabManagement to use tab object rather than index #9924

Merged
1 commit merged into from
Apr 23, 2021
Merged

Fix TabManagement to use tab object rather than index #9924

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 5 additions & 5 deletions src/cascadia/LocalTests_TerminalApp/TabTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,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 @@ -910,10 +910,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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -397,13 +397,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 @@ -422,13 +417,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 @@ -485,15 +480,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 @@ -579,10 +574,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 @@ -592,8 +587,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 @@ -717,49 +716,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 @@ -774,7 +768,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 @@ -798,13 +796,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 @@ -886,10 +883,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
Open in desktop
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