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

Bounds check some tab GetAt()s #16016

Merged
merged 1 commit into from
Oct 5, 2023
Merged

Bounds check some tab GetAt()s #16016

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
6 changes: 5 additions & 1 deletion src/cascadia/TerminalApp/Resources/en-US/Resources.resw
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -882,7 +882,11 @@
</data>
<data name="TerminalPage_PaneMovedAnnouncement_ExistingWindow" xml:space="preserve">
<value>Active pane moved to "{0}" tab in "{1}" window</value>
<comment>{Locked="{0}"}{Locked="{1}"}This text is read out by screen readers upon a successful pane movement. {0} is the name of the tab the pane was moved to. {1} is the name of the window the pane was moved to.</comment>
<comment>{Locked="{0}"}{Locked="{1}"}This text is read out by screen readers upon a successful pane movement. {0} is the name of the tab the pane was moved to. {1} is the name of the window the pane was moved to. Replaced in 1.19 by TerminalPage_PaneMovedAnnouncement_ExistingWindow2</comment>
</data>
<data name="TerminalPage_PaneMovedAnnouncement_ExistingWindow2" xml:space="preserve">
Copy link
Member

Choose a reason for hiding this comment

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

ugh this requires a localization cycle :(

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, that's why I was trying to get this in before 1.19 released 😉

<value>Active pane moved to "{0}" window</value>
<comment>{Locked="{0}"}This text is read out by screen readers upon a successful pane movement. {0} is the name of the window the pane was moved to.</comment>
</data>
<data name="TerminalPage_PaneMovedAnnouncement_NewWindow" xml:space="preserve">
<value>Active pane moved to new window</value>
Expand Down
24 changes: 15 additions & 9 deletions src/cascadia/TerminalApp/TabManagement.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -511,7 +511,7 @@ namespace winrt::TerminalApp::implementation
// * B (tabIndex=1): We'll want to focus tab A (now in index 0)
// * C (tabIndex=2): We'll want to focus tab B (now in index 1)
// * D (tabIndex=3): We'll want to focus tab C (now in index 2)
const auto newSelectedIndex = std::clamp<int32_t>(tabIndex - 1, 0, _tabs.Size());
const auto newSelectedIndex = std::clamp<int32_t>(tabIndex - 1, 0, _tabs.Size() - 1);
// _UpdatedSelectedTab will do the work of setting up the new tab as
// the focused one, and unfocusing all the others.
auto newSelectedTab{ _tabs.GetAt(newSelectedIndex) };
Expand Down Expand Up @@ -678,7 +678,7 @@ namespace winrt::TerminalApp::implementation
{
uint32_t tabIndexFromControl{};
const auto items{ _tabView.TabItems() };
if (items.IndexOf(tabViewItem, tabIndexFromControl))
if (items.IndexOf(tabViewItem, tabIndexFromControl) && tabIndexFromControl < _tabs.Size())
{
// If IndexOf returns true, we've actually got an index
return _tabs.GetAt(tabIndexFromControl);
Expand Down Expand Up @@ -1035,7 +1035,8 @@ namespace winrt::TerminalApp::implementation
// - suggestedNewTabIndex: the new index of the tab, might get clamped to fit int the tabs row boundaries
// Return Value:
// - <none>
void TerminalPage::_TryMoveTab(const uint32_t currentTabIndex, const int32_t suggestedNewTabIndex)
void TerminalPage::_TryMoveTab(const uint32_t currentTabIndex,
const int32_t suggestedNewTabIndex)
{
auto newTabIndex = gsl::narrow_cast<uint32_t>(std::clamp<int32_t>(suggestedNewTabIndex, 0, _tabs.Size() - 1));
if (currentTabIndex != newTabIndex)
Expand Down Expand Up @@ -1077,16 +1078,21 @@ namespace winrt::TerminalApp::implementation

if (from.has_value() && to.has_value() && to != from)
{
auto& tabs{ _tabs };
auto tab = tabs.GetAt(from.value());
tabs.RemoveAt(from.value());
tabs.InsertAt(to.value(), tab);
_UpdateTabIndices();
try
{
auto& tabs{ _tabs };
auto tab = tabs.GetAt(from.value());
tabs.RemoveAt(from.value());
tabs.InsertAt(to.value(), tab);
_UpdateTabIndices();
}
CATCH_LOG();
Copy link
Member

Choose a reason for hiding this comment

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

Why not do a bounds check here?

}

_rearranging = false;

if (to.has_value())
if (to.has_value() &&
*to < gsl::narrow_cast<int32_t>(TabRow().TabView().TabItems().Size()))
Copy link
Member

Choose a reason for hiding this comment

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

Whoa, we're reaching really deep into somebody else to get this. Is that normal?

{
// Selecting the dropped tab
TabRow().TabView().SelectedIndex(to.value());
Expand Down
7 changes: 2 additions & 5 deletions src/cascadia/TerminalApp/TerminalPage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2063,9 +2063,6 @@ namespace winrt::TerminalApp::implementation
{
if (const auto pane{ terminalTab->GetActivePane() })
{
// Get the tab title _before_ moving things around in case the tabIdx doesn't point to the right one after the move
const auto tabTitle = _tabs.GetAt(tabIdx).Title();

auto startupActions = pane->BuildStartupActions(0, 1, true, true);
_DetachPaneFromWindow(pane);
_MoveContent(std::move(startupActions.args), windowId, tabIdx);
Expand All @@ -2084,7 +2081,7 @@ namespace winrt::TerminalApp::implementation
{
autoPeer.RaiseNotificationEvent(Automation::Peers::AutomationNotificationKind::ActionCompleted,
Automation::Peers::AutomationNotificationProcessing::ImportantMostRecent,
fmt::format(std::wstring_view{ RS_(L"TerminalPage_PaneMovedAnnouncement_ExistingWindow") }, tabTitle, windowId),
Copy link
Member Author

Choose a reason for hiding this comment

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

This was straight up not right before. It was getting the title of the tab in the current window, at the index of the target tab.

Copy link
Member

Choose a reason for hiding this comment

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

Hold up, is there a way to correct this then? Get the actual tab title of the target tab?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probs not trivially. Each TerminalWindow & TerminalPage is pretty agnostic to the existence of other windows at all. maybe if the window that "caught" the moved pane raised the message, it could be done, but 🤷

Copy link
Member

Choose a reason for hiding this comment

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

Ok, since we're still keeping the functionality, I think that won't be a problem for #15159. I'm just trying to avoid a Sev2 affecting our score again.

fmt::format(std::wstring_view{ RS_(L"TerminalPage_PaneMovedAnnouncement_ExistingWindow2") }, windowId),
L"TerminalPageMovePaneToExistingWindow" /* unique name for this notification category */);
}
}
Expand All @@ -2101,7 +2098,7 @@ namespace winrt::TerminalApp::implementation

// Moving the pane from the current tab might close it, so get the next
// tab before its index changes.
if (_tabs.Size() > tabIdx)
if (tabIdx < _tabs.Size())
Copy link
Member

Choose a reason for hiding this comment

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

change in behavior?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think so

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know why I felt compelled to make it small thing < big thing in the true case. Just felt like a more sane bounds check here

{
auto targetTab = _GetTerminalTabImpl(_tabs.GetAt(tabIdx));
// if the selected tab is not a host of terminals (e.g. settings)
Expand Down
Loading