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

Add NextSearchMatch and PreviouSearchMatch actions #8522

Closed
wants to merge 2 commits into from
Closed

Add NextSearchMatch and PreviouSearchMatch actions #8522

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c083fb0
Hook up with TerminalPage
kaihugo Dec 21, 2020
694d07a
code format
kaihugo Dec 21, 2020
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
14 changes: 14 additions & 0 deletions src/cascadia/TerminalApp/AppActionHandlers.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,20 @@ namespace winrt::TerminalApp::implementation
args.Handled(true);
}

void TerminalPage::_HandleNextSearchMatch(const IInspectable& /*sender*/,
const ActionEventArgs& args)
{
_GoForward();
args.Handled(true);
}

void TerminalPage::_HandlePrevSearchMatch(const IInspectable& /*sender*/,
const ActionEventArgs& args)
{
_GoBackward();
args.Handled(true);
}

void TerminalPage::_HandleOpenSettings(const IInspectable& /*sender*/,
const ActionEventArgs& args)
{
Expand Down
10 changes: 10 additions & 0 deletions src/cascadia/TerminalApp/ShortcutActionDispatch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,16 @@ namespace winrt::TerminalApp::implementation
_BreakIntoDebuggerHandlers(*this, eventArgs);
break;
}
case ShortcutAction::NextSearchMatch:
{
_NextSearchMatchHandlers(*this, eventArgs);
break;
}
case ShortcutAction::PrevSearchMatch:
{
_PrevSearchMatchHandlers(*this, eventArgs);
break;
}
default:
return false;
}
Expand Down
2 changes: 2 additions & 0 deletions src/cascadia/TerminalApp/ShortcutActionDispatch.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,8 @@ namespace winrt::TerminalApp::implementation
TYPED_EVENT(TabSearch, TerminalApp::ShortcutActionDispatch, Microsoft::Terminal::Settings::Model::ActionEventArgs);
TYPED_EVENT(MoveTab, TerminalApp::ShortcutActionDispatch, Microsoft::Terminal::Settings::Model::ActionEventArgs);
TYPED_EVENT(BreakIntoDebugger, TerminalApp::ShortcutActionDispatch, Microsoft::Terminal::Settings::Model::ActionEventArgs);
TYPED_EVENT(NextSearchMatch, TerminalApp::ShortcutActionDispatch, Microsoft::Terminal::Settings::Model::ActionEventArgs);
TYPED_EVENT(PrevSearchMatch, TerminalApp::ShortcutActionDispatch, Microsoft::Terminal::Settings::Model::ActionEventArgs);
// clang-format on

private:
Expand Down
2 changes: 2 additions & 0 deletions src/cascadia/TerminalApp/ShortcutActionDispatch.idl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,5 +51,7 @@ namespace TerminalApp
event Windows.Foundation.TypedEventHandler<ShortcutActionDispatch, Microsoft.Terminal.Settings.Model.ActionEventArgs> TabSearch;
event Windows.Foundation.TypedEventHandler<ShortcutActionDispatch, Microsoft.Terminal.Settings.Model.ActionEventArgs> MoveTab;
event Windows.Foundation.TypedEventHandler<ShortcutActionDispatch, Microsoft.Terminal.Settings.Model.ActionEventArgs> BreakIntoDebugger;
event Windows.Foundation.TypedEventHandler<ShortcutActionDispatch, Microsoft.Terminal.Settings.Model.ActionEventArgs> NextSearchMatch;
event Windows.Foundation.TypedEventHandler<ShortcutActionDispatch, Microsoft.Terminal.Settings.Model.ActionEventArgs> PrevSearchMatch;
}
}
15 changes: 15 additions & 0 deletions src/cascadia/TerminalApp/TerminalPage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -946,6 +946,7 @@ namespace winrt::TerminalApp::implementation
auto setting = AppLogic::CurrentAppSettings();
auto keymap = setting.GlobalSettings().KeyMap();
const auto actionAndArgs = keymap.TryLookup(kc);

if (actionAndArgs)
{
if (CommandPalette().Visibility() == Visibility::Visible && actionAndArgs.Action() != ShortcutAction::ToggleCommandPalette)
Expand Down Expand Up @@ -1022,6 +1023,8 @@ namespace winrt::TerminalApp::implementation
_actionDispatch->TabSearch({ this, &TerminalPage::_HandleOpenTabSearch });
_actionDispatch->MoveTab({ this, &TerminalPage::_HandleMoveTab });
_actionDispatch->BreakIntoDebugger({ this, &TerminalPage::_HandleBreakIntoDebugger });
_actionDispatch->NextSearchMatch({ this, &TerminalPage::_HandleNextSearchMatch });
_actionDispatch->PrevSearchMatch({ this, &TerminalPage::_HandlePrevSearchMatch });
}

// Method Description:
Expand Down Expand Up @@ -2541,6 +2544,18 @@ namespace winrt::TerminalApp::implementation
termControl.CreateSearchBoxControl();
}

void TerminalPage::_GoForward()
{
const auto termControl = _GetActiveControl();
Copy link
Member

Choose a reason for hiding this comment

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

Should these be

if (const auto termControl{ _GetActiveControl() })
{
    termControl.SearchNextMatch();
}

to make sure termControl isn't null?

termControl.SearchNextMatch();
}

void TerminalPage::_GoBackward()
{
const auto termControl = _GetActiveControl();
termControl.SearchPrevMatch();
}

// Method Description:
// - Toggles borderless mode. Hides the tab row, and raises our
// FocusModeChanged event.
Expand Down
4 changes: 4 additions & 0 deletions src/cascadia/TerminalApp/TerminalPage.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,8 @@ namespace winrt::TerminalApp::implementation
void _OnSwitchToTabRequested(const IInspectable& sender, const winrt::TerminalApp::TabBase& tab);

void _Find();
void _GoForward();
void _GoBackward();

winrt::fire_and_forget _RefreshUIForSettingsReload();

Expand Down Expand Up @@ -305,6 +307,8 @@ namespace winrt::TerminalApp::implementation
void _HandleOpenTabSearch(const IInspectable& sender, const Microsoft::Terminal::Settings::Model::ActionEventArgs& args);
void _HandleMoveTab(const IInspectable& sender, const Microsoft::Terminal::Settings::Model::ActionEventArgs& args);
void _HandleBreakIntoDebugger(const IInspectable& sender, const Microsoft::Terminal::Settings::Model::ActionEventArgs& args);
void _HandleNextSearchMatch(const IInspectable& sender, const Microsoft::Terminal::Settings::Model::ActionEventArgs& args);
void _HandlePrevSearchMatch(const IInspectable& sender, const Microsoft::Terminal::Settings::Model::ActionEventArgs& args);
// Make sure to hook new actions up in _RegisterActionCallbacks!
#pragma endregion

Expand Down
33 changes: 32 additions & 1 deletion src/cascadia/TerminalControl/TermControl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,26 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
}
}

void TermControl::SearchNextMatch()
{
if (!_searchBox)
Copy link
Contributor

Choose a reason for hiding this comment

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

@Hegunumo, @zadjii-msft - product question: if the search box is collapsed and we invoke "search next" binding, shouldn't we open a search box and try to search with the last needle (if exists)?

Copy link
Member

Choose a reason for hiding this comment

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

Okay, so I maybe mentioned this in #8588 - Maybe we should! Or we don't necessarily need to open the search dialog, but we could just move to the next needle.

It's a good idea, and something we should probably stick in #3920 as a follow up to (both these PRs)

{
return;
}
keyBindingSearch = true;
_Search(_searchBox->TextBox().Text(), true, false);
}

void TermControl::SearchPrevMatch()
{
if (!_searchBox)
{
return;
}
keyBindingSearch = true;
_Search(_searchBox->TextBox().Text(), false, false);
}

// Method Description:
// - Search text in text buffer. This is triggered if the user click
// search button or press enter.
Expand Down Expand Up @@ -938,10 +958,21 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation

void TermControl::_KeyHandler(Input::KeyRoutedEventArgs const& e, const bool keyDown)
{
if (e.OriginalKey() == VirtualKey::Enter)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please document here why aren't we handling Enter in the terminal control? I guess it works, but I am not sure why 😊

{
return;
}
Comment on lines +961 to +964
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed?


// If the current focused element is a child element of searchbox,
// we do not send this event up to terminal
if (_searchBox && _searchBox->ContainsFocus())
if (_searchBox && _searchBox->ContainsFocus() && _searchBox->TextBox().Text().empty())
Copy link
Member

Choose a reason for hiding this comment

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

Why is this _searchBox->TextBox().Text().empty() clause needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Please update the documentation of the condition as you have restricted it.

Here as well I am not sure I understand why do we need the restriction. I mean now if the key event got here but the searchbox is focused are we sure it is OK to proceed? Cannot it result with sending a character to a terminal?

Optimally we don't need to check the ContainsFocus here at all. Instead we should make sure that SearchBoxControl handles the relevant key routed events.

{
return;
}

if (keyBindingSearch)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please document your logic here as well? If we searched with key binding then we should ignore the next key?

Copy link
Member

Choose a reason for hiding this comment

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

Curious. If we use the keybinding to invoke this action, then on the keyDown, we'll select the text. Then on the key_up_, we'll dismiss the selection.

Copy link
Member

Choose a reason for hiding this comment

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

Okay this is definitely tricky, and I don't know what a good solution is.

The action is either invoked:

  • with a keybinding, in the TermControl::_KeyHandler(true, ...) method
  • by some other magic way (command palette)?

So if it happened on a keydown, we do need to not dismiss the selection on the next keyup. Weird. I wonder how this worked in the keyboard-selection implementation. Wouldn't that have hit the same problem? Paging @carlos-zamora to see if he can here. Am I just being daft? Is there an obvious way to create a selection via action and have it not dismiss on the keyup?

Copy link
Member

Choose a reason for hiding this comment

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

Derp. I can't read.
#3758 (comment)

We need to do this for #3758 so I'm just gonna do it here anyways

{
keyBindingSearch = false;
return;
}
Comment on lines +973 to 977
Copy link
Member

Choose a reason for hiding this comment

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

This feels wrong to me. I know currently, actions are only triggerable with the keyboard, but theoretically, they might be invokable through other UI elements. Are we just trying to prevent the keys from being duplicated here?


Expand Down
4 changes: 4 additions & 0 deletions src/cascadia/TerminalControl/TermControl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,9 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation

void CreateSearchBoxControl();

void SearchNextMatch();
void SearchPrevMatch();

bool OnDirectKeyEvent(const uint32_t vkey, const uint8_t scanCode, const bool down);

bool OnMouseWheel(const Windows::Foundation::Point location, const int32_t delta, const bool leftButtonDown, const bool midButtonDown, const bool rightButtonDown);
Expand Down Expand Up @@ -187,6 +190,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
bool _initializedTerminal;

winrt::com_ptr<SearchBoxControl> _searchBox;
bool keyBindingSearch = true;

event_token _connectionOutputEventToken;
TerminalConnection::ITerminalConnection::StateChanged_revoker _connectionStateChangedRevoker;
Expand Down
3 changes: 3 additions & 0 deletions src/cascadia/TerminalControl/TermControl.idl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,9 @@ namespace Microsoft.Terminal.TerminalControl

void CreateSearchBoxControl();

void SearchNextMatch();
void SearchPrevMatch();

void AdjustFontSize(Int32 fontSizeDelta);
void ResetFontSize();

Expand Down
6 changes: 6 additions & 0 deletions src/cascadia/TerminalSettingsModel/ActionAndArgs.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ static constexpr std::string_view LegacyToggleRetroEffectKey{ "toggleRetroEffect
static constexpr std::string_view ToggleShaderEffectsKey{ "toggleShaderEffects" };
static constexpr std::string_view MoveTabKey{ "moveTab" };
static constexpr std::string_view BreakIntoDebuggerKey{ "breakIntoDebugger" };
static constexpr std::string_view NextSearchMatchKey{ "nextSearchMatchKey" };
static constexpr std::string_view PrevSearchMatchKey{ "prevSearchMatchKey" };

static constexpr std::string_view ActionKey{ "action" };

Expand Down Expand Up @@ -115,6 +117,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
{ MoveTabKey, ShortcutAction::MoveTab },
{ BreakIntoDebuggerKey, ShortcutAction::BreakIntoDebugger },
{ UnboundKey, ShortcutAction::Invalid },
{ NextSearchMatchKey, ShortcutAction::NextSearchMatch },
{ PrevSearchMatchKey, ShortcutAction::PrevSearchMatch },
};

using ParseResult = std::tuple<IActionArgs, std::vector<SettingsLoadWarnings>>;
Expand Down Expand Up @@ -314,6 +318,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
{ ShortcutAction::ToggleShaderEffects, RS_(L"ToggleShaderEffectsCommandKey") },
{ ShortcutAction::MoveTab, L"" }, // Intentionally omitted, must be generated by GenerateName
{ ShortcutAction::BreakIntoDebugger, RS_(L"BreakIntoDebuggerCommandKey") },
{ ShortcutAction::NextSearchMatch, RS_(L"NextSearchMatchCommandKey") },
{ ShortcutAction::PrevSearchMatch, RS_(L"PrevSearchMatchCommandKey") },
};
}();

Expand Down
4 changes: 3 additions & 1 deletion src/cascadia/TerminalSettingsModel/KeyMapping.idl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,9 @@ namespace Microsoft.Terminal.Settings.Model
CloseTabsAfter,
TabSearch,
MoveTab,
BreakIntoDebugger
BreakIntoDebugger,
NextSearchMatch,
PrevSearchMatch
};

[default_interface] runtimeclass ActionAndArgs {
Expand Down
6 changes: 6 additions & 0 deletions src/cascadia/TerminalSettingsModel/Resources/en-US/Resources.resw
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,12 @@
<data name="FindCommandKey" xml:space="preserve">
<value>Find</value>
</data>
<data name="NextSearchMatchCommandKey" xml:space="preserve">
<value>Next search match</value>
</data>
<data name="PrevSearchMatchCommandKey" xml:space="preserve">
<value>Previous search match</value>
</data>
<data name="IncreaseFontSizeCommandKey" xml:space="preserve">
<value>Increase font size</value>
</data>
Expand Down
2 changes: 2 additions & 0 deletions src/cascadia/TerminalSettingsModel/defaults.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,8 @@
{ "command": "openSettings", "keys": "ctrl+," },
{ "command": { "action": "openSettings", "target": "defaultsFile" }, "keys": "ctrl+alt+," },
{ "command": "find", "keys": "ctrl+shift+f" },
{ "command": "nextSearchMatchKey", "keys": "ctrl+shift+n" },
{ "command": "prevSearchMatchKey", "keys": "ctrl+shift+k" },
Comment on lines +288 to +289
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure how the team feels about adding these to the default keybindings - @DHowett thoughts?

{ "command": "toggleShaderEffects" },
{ "command": "openTabColorPicker" },
{ "command": "renameTab" },
Expand Down