Bounds check some tab GetAt()s #16016
Conversation
| @@ -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), | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Hold up, is there a way to correct this then? Get the actual tab title of the target tab?
There was a problem hiding this comment.
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 🤷
There was a problem hiding this comment.
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.
| tabs.InsertAt(to.value(), tab); | ||
| _UpdateTabIndices(); | ||
| } | ||
| CATCH_LOG(); |
There was a problem hiding this comment.
Why not do a bounds check here?
| @@ -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), | |||
There was a problem hiding this comment.
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.
| <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"> |
There was a problem hiding this comment.
ugh this requires a localization cycle :(
There was a problem hiding this comment.
Yep, that's why I was trying to get this in before 1.19 released 😉
| } | ||
|
|
||
| _rearranging = false; | ||
|
|
||
| if (to.has_value()) | ||
| if (to.has_value() && | ||
| *to < gsl::narrow_cast<int32_t>(TabRow().TabView().TabItems().Size())) |
There was a problem hiding this comment.
Whoa, we're reaching really deep into somebody else to get this. Is that normal?
| @@ -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()) | |||
There was a problem hiding this comment.
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
GetAtcan throw if the index is out of range. We don't check that in some places. This fixes some of those.I don't think this will take care of #15689, but it might help?