Skip to content

Commit

Permalink
Hook up the keybindings to the SUI, redux (#10121)
Browse files Browse the repository at this point in the history
## Summary of the Pull Request

This is a redux of #8882.

From the original:

>  This is really similar to what we're doing with the `CommandPalette`. We're adding a ~~Preview~~`KeyDown` handler to the SUI `MainPage`, that connects to `TerminalPage::_HandleKey`. That allows the SUI a chance to search the keymap to dispatch actions for keybindings, similar to how the command palette does it.
>
> This also means it's now possible for the SUI to invoke _all_ the actions available to the Terminal. This includes the ones like `IncreaseFontSize`, which require a _Terminal_ to actually do something. So we have to make sure all the calls to `_GetActiveControl` actually check that the result is non-null before using it.
>
> A bunch of the actions do nothing now from a SUI tab, others behave _weird_. Like "Rename tab" / "Open Tab Renamer" do nothing. "Duplicate Tab" again does nothing - we try making a new settings tab, which just focuses the settings tab again. "Copy text" definitely does nothing, same with paste.

I don't know why I thought this wouldn't work. I thought we'd have to do this in `PreviewKeyDown` or something, which led to [weirdness](#8882 (comment)). Turns out, we don't need it to be in `PreviewKeyDown`. It can just be in the SUI's `KeyDown`.

## References
* Original: #8882
* Workaround was in #8885

## PR Checklist
* [x] Closes #8767
* [x] I work here
* [x] Tests added/passed
* [n/a] Requires documentation to be updated

## Detailed Description of the Pull Request / Additional comments

The special case handler from #8885 is no longer needed

## Validation Steps Performed

* Switching tabs with Ctrl+Tab works
* Command palette works
* fullscreen, focus mode works
* close window works
* copy paste on Ctrl+C/V works, even when bound
* Select all text in textboxes works
* tab navigation through UI elements works

(cherry picked from commit 52560ff)

# Conflicts:
#	src/cascadia/TerminalApp/TerminalPage.cpp
#	src/cascadia/TerminalApp/TerminalPage.h
  • Loading branch information
zadjii-msft authored and DHowett committed May 24, 2021
1 parent a2ab200 commit a3de0d0
Show file tree
Hide file tree
Showing 3 changed files with 41 additions and 48 deletions.
32 changes: 20 additions & 12 deletions src/cascadia/TerminalApp/AppActionHandlers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -117,9 +117,11 @@ namespace winrt::TerminalApp::implementation
}
else if (const auto& realArgs = args.ActionArgs().try_as<SendInputArgs>())
{
const auto termControl = _GetActiveControl();
termControl.SendInput(realArgs.Input());
args.Handled(true);
if (const auto termControl{ _GetActiveControl() })
{
termControl.SendInput(realArgs.Input());
args.Handled(true);
}
}
}

Expand Down Expand Up @@ -308,9 +310,11 @@ namespace winrt::TerminalApp::implementation
{
if (const auto& realArgs = args.ActionArgs().try_as<AdjustFontSizeArgs>())
{
const auto termControl = _GetActiveControl();
termControl.AdjustFontSize(realArgs.Delta());
args.Handled(true);
if (const auto& termControl{ _GetActiveControl() })
{
termControl.AdjustFontSize(realArgs.Delta());
args.Handled(true);
}
}
}

Expand All @@ -324,17 +328,21 @@ namespace winrt::TerminalApp::implementation
void TerminalPage::_HandleResetFontSize(const IInspectable& /*sender*/,
const ActionEventArgs& args)
{
const auto termControl = _GetActiveControl();
termControl.ResetFontSize();
args.Handled(true);
if (const auto& termControl{ _GetActiveControl() })
{
termControl.ResetFontSize();
args.Handled(true);
}
}

void TerminalPage::_HandleToggleShaderEffects(const IInspectable& /*sender*/,
const ActionEventArgs& args)
{
const auto termControl = _GetActiveControl();
termControl.ToggleShaderEffects();
args.Handled(true);
if (const auto& termControl{ _GetActiveControl() })
{
termControl.ToggleShaderEffects();
args.Handled(true);
}
}

void TerminalPage::_HandleToggleFocusMode(const IInspectable& /*sender*/,
Expand Down
56 changes: 21 additions & 35 deletions src/cascadia/TerminalApp/TerminalPage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -902,30 +902,6 @@ namespace winrt::TerminalApp::implementation
}
}

// Method Description:
// Handles preview key on the SUI tab, by handling close tab / next tab / previous tab
// This is a temporary solution - we need to fix all key-bindings work from SUI as long as they don't harm
// the SUI behavior
// Arguments:
// - e: the KeyRoutedEventArgs containing info about the keystroke.
// Return Value:
// - <none>
void TerminalPage::_SUIPreviewKeyDownHandler(Windows::Foundation::IInspectable const& /*sender*/, Windows::UI::Xaml::Input::KeyRoutedEventArgs const& e)
{
auto key = e.OriginalKey();
auto const ctrlDown = WI_IsFlagSet(CoreWindow::GetForCurrentThread().GetKeyState(winrt::Windows::System::VirtualKey::Control), CoreVirtualKeyStates::Down);
auto const altDown = WI_IsFlagSet(CoreWindow::GetForCurrentThread().GetKeyState(winrt::Windows::System::VirtualKey::Menu), CoreVirtualKeyStates::Down);
auto const shiftDown = WI_IsFlagSet(CoreWindow::GetForCurrentThread().GetKeyState(winrt::Windows::System::VirtualKey::Shift), CoreVirtualKeyStates::Down);

winrt::Microsoft::Terminal::Control::KeyChord kc{ ctrlDown, altDown, shiftDown, static_cast<int32_t>(key) };
const auto actionAndArgs = _settings.KeyMap().TryLookup(kc);
if (actionAndArgs && (actionAndArgs.Action() == ShortcutAction::CloseTab || actionAndArgs.Action() == ShortcutAction::NextTab || actionAndArgs.Action() == ShortcutAction::PrevTab || actionAndArgs.Action() == ShortcutAction::ClosePane))
{
_actionDispatch->DoAction(actionAndArgs);
e.Handled(true);
}
}

// Method Description:
// - Configure the AppKeyBindings to use our ShortcutActionDispatch and the updated KeyMapping
// as the object to handle dispatching ShortcutAction events.
Expand Down Expand Up @@ -1290,10 +1266,12 @@ namespace winrt::TerminalApp::implementation
// Do nothing if for some reason, there's no terminal tab in focus. We don't want to crash.
if (const auto terminalTab{ _GetFocusedTabImpl() })
{
const auto control = _GetActiveControl();
const auto termHeight = control.GetViewHeight();
auto scrollDelta = _ComputeScrollDelta(scrollDirection, termHeight);
terminalTab->Scroll(scrollDelta);
if (const auto& control{ _GetActiveControl() })
{
const auto termHeight = control.ViewHeight();
auto scrollDelta = _ComputeScrollDelta(scrollDirection, termHeight);
terminalTab->Scroll(scrollDelta);
}
}
}

Expand Down Expand Up @@ -1689,8 +1667,11 @@ namespace winrt::TerminalApp::implementation
// - true iff we we able to copy text (if a selection was active)
bool TerminalPage::_CopyText(const bool singleLine, const Windows::Foundation::IReference<CopyFormat>& formats)
{
const auto control = _GetActiveControl();
return control.CopySelectionToClipboard(singleLine, formats);
if (const auto& control{ _GetActiveControl() })
{
return control.CopySelectionToClipboard(singleLine, formats);
}
return false;
}

// Method Description:
Expand All @@ -1707,8 +1688,10 @@ namespace winrt::TerminalApp::implementation
// - Paste text from the Windows Clipboard to the focused terminal
void TerminalPage::_PasteText()
{
const auto control = _GetActiveControl();
control.PasteTextFromClipboard();
if (const auto& control{ _GetActiveControl() })
{
control.PasteTextFromClipboard();
}
}

// Function Description:
Expand Down Expand Up @@ -2042,8 +2025,10 @@ namespace winrt::TerminalApp::implementation
// - <none>
void TerminalPage::_Find()
{
const auto termControl = _GetActiveControl();
termControl.CreateSearchBoxControl();
if (const auto& control{ _GetActiveControl() })
{
control.CreateSearchBoxControl();
}
}

// Method Description:
Expand Down Expand Up @@ -2312,7 +2297,8 @@ namespace winrt::TerminalApp::implementation
sui.SetHostingWindow(reinterpret_cast<uint64_t>(*_hostingHwnd));
}

sui.PreviewKeyDown({ this, &TerminalPage::_SUIPreviewKeyDownHandler });
// GH#8767 - let unhandled keys in the SUI try to run commands too.
sui.KeyDown({ this, &TerminalPage::_KeyDownHandler });

sui.OpenJson([weakThis{ get_weak() }](auto&& /*s*/, winrt::Microsoft::Terminal::Settings::Model::SettingsTarget e) {
if (auto page{ weakThis.get() })
Expand Down
1 change: 0 additions & 1 deletion src/cascadia/TerminalApp/TerminalPage.h
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,6 @@ namespace winrt::TerminalApp::implementation
void _ThirdPartyNoticesOnClick(const IInspectable& sender, const Windows::UI::Xaml::RoutedEventArgs& eventArgs);

void _KeyDownHandler(Windows::Foundation::IInspectable const& sender, Windows::UI::Xaml::Input::KeyRoutedEventArgs const& e);
void _SUIPreviewKeyDownHandler(Windows::Foundation::IInspectable const& sender, Windows::UI::Xaml::Input::KeyRoutedEventArgs const& e);
void _HookupKeyBindings(const Microsoft::Terminal::Settings::Model::KeyMapping& keymap) noexcept;
void _RegisterActionCallbacks();

Expand Down

0 comments on commit a3de0d0

Please sign in to comment.