Skip to content

Commit

Permalink
Raise ShortcutActions with the sender (tab, control) context (#15773)
Browse files Browse the repository at this point in the history 
This PR's goal is to allow something like a `Tab` to raise a
ShortcutAction, by saying "this action should be performed on ME". We've
had a whole category of these issues in the past:

* #15734
* #15760
* #13579
* #13942
* #13942
* Heck even dating back to #10832

So, this tries to remove a bit of that footgun. This probably isn't the
_final_ form of what this refactor might look like, but the code is
certainly better than before.

Basically, there's a few bits:

* `ShortcutActionDispatch.DoAction` now takes a `sender`, which can be
_anything_.
* Most actions that use a "Get the focused _thing_ then do something to
it" are changed to "If there was a sender, let's use that - otherwise,
we'll use the focused _thing_".
* TerminalTab was largely refactored to use this, instead of making
requests to the `TerminalPage` to just do a thing to it.

I've got a few TODO!s left, but wanted to get initial feedback.
* [x] `TerminalPage::_HandleTogglePaneZoom`
* [x] `TerminalPage::_HandleFocusPane`
* [x] `TerminalPage::_MoveTab`

Closes #15734

(cherry picked from commit 30eb9ee)
Service-Card-Id: 90438493
Service-Version: 1.18
  • Loading branch information
@zadjii-msft @DHowett
zadjii-msft authored and DHowett committed Sep 22, 2023
1 parent 2b5b574 commit 1eab243
Show file tree
Hide file tree
Showing 12 changed files with 236 additions and 249 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 @@ -517,7 +517,7 @@ namespace TerminalAppLocalTests

Log::Comment(NoThrowString().Format(L"Duplicate the first pane"));
result = RunOnUIThread([&page]() {
page->_SplitPane(SplitDirection::Automatic, 0.5f, page->_MakePane(nullptr, page->_GetFocusedTab(), nullptr));
page->_SplitPane(nullptr, SplitDirection::Automatic, 0.5f, page->_MakePane(nullptr, page->_GetFocusedTab(), nullptr));

VERIFY_ARE_EQUAL(1u, page->_tabs.Size());
auto tab = page->_GetTerminalTabImpl(page->_tabs.GetAt(0));
Expand All @@ -535,7 +535,7 @@ namespace TerminalAppLocalTests

Log::Comment(NoThrowString().Format(L"Duplicate the pane, and don't crash"));
result = RunOnUIThread([&page]() {
page->_SplitPane(SplitDirection::Automatic, 0.5f, page->_MakePane(nullptr, page->_GetFocusedTab(), nullptr));
page->_SplitPane(nullptr, SplitDirection::Automatic, 0.5f, page->_MakePane(nullptr, page->_GetFocusedTab(), nullptr));

VERIFY_ARE_EQUAL(1u, page->_tabs.Size());
auto tab = page->_GetTerminalTabImpl(page->_tabs.GetAt(0));
Expand Down Expand Up @@ -857,7 +857,7 @@ namespace TerminalAppLocalTests
// | 1 | 2 |
// | | |
// -------------------
page->_SplitPane(SplitDirection::Right, 0.5f, page->_MakePane(nullptr, page->_GetFocusedTab(), nullptr));
page->_SplitPane(nullptr, SplitDirection::Right, 0.5f, page->_MakePane(nullptr, page->_GetFocusedTab(), nullptr));
secondId = tab->_activePane->Id().value();
});
Sleep(250);
Expand All @@ -875,7 +875,7 @@ namespace TerminalAppLocalTests
// | 3 | |
// | | |
// -------------------
page->_SplitPane(SplitDirection::Down, 0.5f, page->_MakePane(nullptr, page->_GetFocusedTab(), nullptr));
page->_SplitPane(nullptr, SplitDirection::Down, 0.5f, page->_MakePane(nullptr, page->_GetFocusedTab(), nullptr));
auto tab = page->_GetTerminalTabImpl(page->_tabs.GetAt(0));
// Split again to make the 3rd tab
thirdId = tab->_activePane->Id().value();
Expand All @@ -895,7 +895,7 @@ namespace TerminalAppLocalTests
// | 3 | 4 |
// | | |
// -------------------
page->_SplitPane(SplitDirection::Down, 0.5f, page->_MakePane(nullptr, page->_GetFocusedTab(), nullptr));
page->_SplitPane(nullptr, SplitDirection::Down, 0.5f, page->_MakePane(nullptr, page->_GetFocusedTab(), nullptr));
auto tab = page->_GetTerminalTabImpl(page->_tabs.GetAt(0));
fourthId = tab->_activePane->Id().value();
});
Expand Down
131 changes: 84 additions & 47 deletions src/cascadia/TerminalApp/AppActionHandlers.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,29 @@ namespace winrt

namespace winrt::TerminalApp::implementation
{
TermControl TerminalPage::_senderOrActiveControl(const IInspectable& sender)
{
if (sender)
{
if (auto arg{ sender.try_as<TermControl>() })
{
return arg;
}
}
return _GetActiveControl();
}
winrt::com_ptr<TerminalTab> TerminalPage::_senderOrFocusedTab(const IInspectable& sender)
{
if (sender)
{
if (auto tab{ sender.try_as<TerminalApp::TerminalTab>() })
{
return _GetTerminalTabImpl(tab);
}
}
return _GetFocusedTabImpl();
}

void TerminalPage::_HandleOpenNewTabDropdown(const IInspectable& /*sender*/,
const ActionEventArgs& args)
{
Expand Down Expand Up @@ -149,7 +172,7 @@ namespace winrt::TerminalApp::implementation
}
}

void TerminalPage::_HandleSendInput(const IInspectable& /*sender*/,
void TerminalPage::_HandleSendInput(const IInspectable& sender,
const ActionEventArgs& args)
{
if (args == nullptr)
Expand All @@ -158,18 +181,18 @@ namespace winrt::TerminalApp::implementation
}
else if (const auto& realArgs = args.ActionArgs().try_as<SendInputArgs>())
{
if (const auto termControl{ _GetActiveControl() })
if (const auto termControl{ _senderOrActiveControl(sender) })
{
termControl.SendInput(realArgs.Input());
args.Handled(true);
}
}
}

void TerminalPage::_HandleCloseOtherPanes(const IInspectable& /*sender*/,
void TerminalPage::_HandleCloseOtherPanes(const IInspectable& sender,
const ActionEventArgs& args)
{
if (const auto terminalTab{ _GetFocusedTabImpl() })
if (const auto& terminalTab{ _senderOrFocusedTab(sender) })
{
const auto activePane = terminalTab->GetActivePane();
if (terminalTab->GetRootPane() != activePane)
Expand Down Expand Up @@ -214,7 +237,7 @@ namespace winrt::TerminalApp::implementation
}
}

void TerminalPage::_HandleSplitPane(const IInspectable& /*sender*/,
void TerminalPage::_HandleSplitPane(const IInspectable& sender,
const ActionEventArgs& args)
{
if (args == nullptr)
Expand All @@ -236,7 +259,11 @@ namespace winrt::TerminalApp::implementation
}

const auto& duplicateFromTab{ realArgs.SplitMode() == SplitType::Duplicate ? _GetFocusedTab() : nullptr };
_SplitPane(realArgs.SplitDirection(),

const auto& terminalTab{ _senderOrFocusedTab(sender) };

_SplitPane(terminalTab,
realArgs.SplitDirection(),
// This is safe, we're already filtering so the value is (0, 1)
::base::saturated_cast<float>(realArgs.SplitSize()),
_MakePane(realArgs.TerminalArgs(), duplicateFromTab));
Expand All @@ -251,55 +278,49 @@ namespace winrt::TerminalApp::implementation
args.Handled(true);
}

void TerminalPage::_HandleTogglePaneZoom(const IInspectable& /*sender*/,
void TerminalPage::_HandleTogglePaneZoom(const IInspectable& sender,
const ActionEventArgs& args)
{
if (const auto activeTab{ _GetFocusedTabImpl() })
if (const auto terminalTab{ _senderOrFocusedTab(sender) })
{
// Don't do anything if there's only one pane. It's already zoomed.
if (activeTab->GetLeafPaneCount() > 1)
if (terminalTab->GetLeafPaneCount() > 1)
{
// First thing's first, remove the current content from the UI
// tree. This is important, because we might be leaving zoom, and if
// a pane is zoomed, then it's currently in the UI tree, and should
// be removed before it's re-added in Pane::Restore
_tabContent.Children().Clear();

// Togging the zoom on the tab will cause the tab to inform us of
// the new root Content for this tab.
activeTab->ToggleZoom();
terminalTab->ToggleZoom();
}
}

args.Handled(true);
}

void TerminalPage::_HandleTogglePaneReadOnly(const IInspectable& /*sender*/,
void TerminalPage::_HandleTogglePaneReadOnly(const IInspectable& sender,
const ActionEventArgs& args)
{
if (const auto activeTab{ _GetFocusedTabImpl() })
if (const auto activeTab{ _senderOrFocusedTab(sender) })
{
activeTab->TogglePaneReadOnly();
}

args.Handled(true);
}

void TerminalPage::_HandleEnablePaneReadOnly(const IInspectable& /*sender*/,
void TerminalPage::_HandleEnablePaneReadOnly(const IInspectable& sender,
const ActionEventArgs& args)
{
if (const auto activeTab{ _GetFocusedTabImpl() })
if (const auto activeTab{ _senderOrFocusedTab(sender) })
{
activeTab->SetPaneReadOnly(true);
}

args.Handled(true);
}

void TerminalPage::_HandleDisablePaneReadOnly(const IInspectable& /*sender*/,
void TerminalPage::_HandleDisablePaneReadOnly(const IInspectable& sender,
const ActionEventArgs& args)
{
if (const auto activeTab{ _GetFocusedTabImpl() })
if (const auto activeTab{ _senderOrFocusedTab(sender) })
{
activeTab->SetPaneReadOnly(false);
}
Expand Down Expand Up @@ -529,11 +550,12 @@ namespace winrt::TerminalApp::implementation
}
}

void TerminalPage::_HandleFind(const IInspectable& /*sender*/,
void TerminalPage::_HandleFind(const IInspectable& sender,
const ActionEventArgs& args)
{
if (const auto activeTab{ _GetFocusedTabImpl() })
if (const auto activeTab{ _senderOrFocusedTab(sender) })
{
_SetFocusedTab(*activeTab);
_Find(*activeTab);
}
args.Handled(true);
Expand Down Expand Up @@ -637,7 +659,7 @@ namespace winrt::TerminalApp::implementation
}
}

void TerminalPage::_HandleSetTabColor(const IInspectable& /*sender*/,
void TerminalPage::_HandleSetTabColor(const IInspectable& sender,
const ActionEventArgs& args)
{
Windows::Foundation::IReference<Windows::UI::Color> tabColor;
Expand All @@ -647,7 +669,7 @@ namespace winrt::TerminalApp::implementation
tabColor = realArgs.TabColor();
}

if (const auto activeTab{ _GetFocusedTabImpl() })
if (const auto activeTab{ _senderOrFocusedTab(sender) })
{
if (tabColor)
{
Expand All @@ -661,17 +683,22 @@ namespace winrt::TerminalApp::implementation
args.Handled(true);
}

void TerminalPage::_HandleOpenTabColorPicker(const IInspectable& /*sender*/,
void TerminalPage::_HandleOpenTabColorPicker(const IInspectable& sender,
const ActionEventArgs& args)
{
if (const auto activeTab{ _GetFocusedTabImpl() })
if (const auto activeTab{ _senderOrFocusedTab(sender) })
{
activeTab->RequestColorPicker();
if (!_tabColorPicker)
{
_tabColorPicker = winrt::make<ColorPickupFlyout>();
}

activeTab->AttachColorPicker(_tabColorPicker);
}
args.Handled(true);
}

void TerminalPage::_HandleRenameTab(const IInspectable& /*sender*/,
void TerminalPage::_HandleRenameTab(const IInspectable& sender,
const ActionEventArgs& args)
{
std::optional<winrt::hstring> title;
Expand All @@ -681,7 +708,7 @@ namespace winrt::TerminalApp::implementation
title = realArgs.Title();
}

if (const auto activeTab{ _GetFocusedTabImpl() })
if (const auto activeTab{ _senderOrFocusedTab(sender) })
{
if (title.has_value())
{
Expand All @@ -695,10 +722,10 @@ namespace winrt::TerminalApp::implementation
args.Handled(true);
}

void TerminalPage::_HandleOpenTabRenamer(const IInspectable& /*sender*/,
void TerminalPage::_HandleOpenTabRenamer(const IInspectable& sender,
const ActionEventArgs& args)
{
if (const auto activeTab{ _GetFocusedTabImpl() })
if (const auto activeTab{ _senderOrFocusedTab(sender) })
{
activeTab->ActivateTabRenamer();
}
Expand Down Expand Up @@ -807,12 +834,12 @@ namespace winrt::TerminalApp::implementation
args.Handled(true);
}

void TerminalPage::_HandleMoveTab(const IInspectable& /*sender*/,
void TerminalPage::_HandleMoveTab(const IInspectable& sender,
const ActionEventArgs& actionArgs)
{
if (const auto& realArgs = actionArgs.ActionArgs().try_as<MoveTabArgs>())
{
auto moved = _MoveTab(realArgs);
auto moved = _MoveTab(_senderOrFocusedTab(sender), realArgs);
actionArgs.Handled(moved);
}
}
Expand Down Expand Up @@ -1048,6 +1075,16 @@ namespace winrt::TerminalApp::implementation
if (const auto& realArgs = args.ActionArgs().try_as<FocusPaneArgs>())
{
const auto paneId = realArgs.Id();

// This action handler is not enlightened for _senderOrFocusedTab.
// There's currently no way for an inactive tab to be the sender of a focusPane command.
// If that ever changes, then we'll need to consider how this handler should behave.
// Should it
// * focus the tab that sent the command AND activate the requested pane?
// * or should it just activate the pane in the sender, and leave the focused tab alone?
//
// For now, we'll just focus the pane in the focused tab.

if (const auto activeTab{ _GetFocusedTabImpl() })
{
_UnZoomIfNeeded();
Expand All @@ -1064,10 +1101,10 @@ namespace winrt::TerminalApp::implementation
args.Handled(true);
}

void TerminalPage::_HandleExportBuffer(const IInspectable& /*sender*/,
void TerminalPage::_HandleExportBuffer(const IInspectable& sender,
const ActionEventArgs& args)
{
if (const auto activeTab{ _GetFocusedTabImpl() })
if (const auto activeTab{ _senderOrFocusedTab(sender) })
{
if (args)
{
Expand Down Expand Up @@ -1135,10 +1172,10 @@ namespace winrt::TerminalApp::implementation
}
}

void TerminalPage::_HandleSelectAll(const IInspectable& /*sender*/,
void TerminalPage::_HandleSelectAll(const IInspectable& sender,
const ActionEventArgs& args)
{
if (const auto& control{ _GetActiveControl() })
if (const auto& control{ _senderOrActiveControl(sender) })
{
control.SelectAll();
args.Handled(true);
Expand Down Expand Up @@ -1174,30 +1211,30 @@ namespace winrt::TerminalApp::implementation
}
}

void TerminalPage::_HandleMarkMode(const IInspectable& /*sender*/,
void TerminalPage::_HandleMarkMode(const IInspectable& sender,
const ActionEventArgs& args)
{
if (const auto& control{ _GetActiveControl() })
if (const auto& control{ _senderOrActiveControl(sender) })
{
control.ToggleMarkMode();
args.Handled(true);
}
}

void TerminalPage::_HandleToggleBlockSelection(const IInspectable& /*sender*/,
void TerminalPage::_HandleToggleBlockSelection(const IInspectable& sender,
const ActionEventArgs& args)
{
if (const auto& control{ _GetActiveControl() })
if (const auto& control{ _senderOrActiveControl(sender) })
{
const auto handled = control.ToggleBlockSelection();
args.Handled(handled);
}
}

void TerminalPage::_HandleSwitchSelectionEndpoint(const IInspectable& /*sender*/,
void TerminalPage::_HandleSwitchSelectionEndpoint(const IInspectable& sender,
const ActionEventArgs& args)
{
if (const auto& control{ _GetActiveControl() })
if (const auto& control{ _senderOrActiveControl(sender) })
{
const auto handled = control.SwitchSelectionEndpoint();
args.Handled(handled);
Expand Down Expand Up @@ -1229,10 +1266,10 @@ namespace winrt::TerminalApp::implementation
}
}

void TerminalPage::_HandleRestartConnection(const IInspectable& /*sender*/,
void TerminalPage::_HandleRestartConnection(const IInspectable& sender,
const ActionEventArgs& args)
{
if (const auto activeTab{ _GetFocusedTabImpl() })
if (const auto activeTab{ _senderOrFocusedTab(sender) })
{
if (const auto activePane{ activeTab->GetActivePane() })
{
Expand Down
4 changes: 2 additions & 2 deletions src/cascadia/TerminalApp/AppLogic.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -597,11 +597,11 @@ namespace winrt::TerminalApp::implementation
// use as a title though!
//
// First, check the reserved keywords:
if (parsedTarget == "new")
if (parsedTarget == NewWindow)
{
return winrt::make<FindTargetWindowResult>(WindowingBehaviorUseNew);
}
else if (parsedTarget == "last")
else if (parsedTarget == MostRecentlyUsedWindow)
{
return winrt::make<FindTargetWindowResult>(WindowingBehaviorUseExisting);
}
Expand Down
Loading

0 comments on commit 1eab243

Please sign in to comment.