Add NextSearchMatch and PreviouSearchMatch actions #8522

ghost · 2020-12-08T13:30:34Z

Summary of the Pull Request

This PR adds new action so that users can search next an previous serching results.

References

PR Checklist

Closes Please add "next search match" and "prev search match" bind-able key for find operations #7695
CLA signed. If not, go over here and sign the CLA
Tests added/passed
Documentation updated. If checked, please file a pull request on our docs repo and link it here: #xxx
Schema updated.
I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

ghost · 2020-12-08T17:20:34Z

_searchBox->TextBox().Text().empty() in TermControl.cpp seems doesn't work. As a result terminal will crashes if there isn't any text in the TextBox.

Lo0oG · 2020-12-08T21:09:57Z

src/cascadia/TerminalControl/TermControl.cpp

+        _Search(_searchBox->TextBox().Text(), true, false);
+    }
+
+    void TermControl::SearchPreviousMatch()


Would TermControl::SearchPrevMatch() be a better name as would match all the other occurences of "Prev"?

zadjii-msft

So this is kinda the same deal as the Command Palette keybindings PR, right? If the user has the Find dialog open, the user won't be able to use these keybindings successfully right? We'd need to do a keymap lookup to see if the keypress is mapped to one of these actions, right?

zadjii-msft · 2020-12-15T03:57:11Z

src/cascadia/TerminalControl/SearchBoxControl.cpp

+        else if (e.OriginalKey() == winrt::Windows::System::VirtualKey::N)
+        {
+            if (shiftDown && ctrlDown)
+            {
+                _SearchHandlers(TextBox().Text(), !_GoForward(), _CaseSensitive());
+            }
+            e.Handled(true);
+        }
+        else if (e.OriginalKey() == winrt::Windows::System::VirtualKey::K)
+        {
+            if (shiftDown && ctrlDown)
            {
                _SearchHandlers(TextBox().Text(), _GoForward(), _CaseSensitive());
            }


Why are N and K being special cased here? What if someone unbinds those keys?

ghost · 2020-12-21T13:08:22Z

@zadjii-msft I used TerminalPage::_KeydownHandler(). Please review!

zadjii-msft · 2021-01-19T21:04:07Z

Hey sorry that this one has been sitting for so long. It must have been lost in my mailbox right as we went on holiday. I'll take a spin through the code now.

/cc @Don-Vito since you've been working in this area recently too

Don-Vito

@Hegunumo, @zadjii-msft - I have few questions 😊

Don-Vito · 2021-01-19T21:22:38Z

src/cascadia/TerminalControl/TermControl.cpp

@@ -224,6 +224,26 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
        }
    }

+    void TermControl::SearchNextMatch()
+    {
+        if (!_searchBox)


@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)?

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)

Don-Vito · 2021-01-19T21:25:05Z

src/cascadia/TerminalControl/TermControl.cpp

@@ -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)


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 😊

Don-Vito · 2021-01-19T21:29:13Z

src/cascadia/TerminalControl/TermControl.cpp

+            return;
+        }
+
+        if (keyBindingSearch)


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

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.

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?

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

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

Don-Vito · 2021-01-19T21:47:13Z

src/cascadia/TerminalControl/TermControl.cpp

        // 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())


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.

zadjii-msft

Okay, I'm a little confused why some of these cases are needed. Comments might help explain. I'm worried that our keyboard handling for the TermControl/SearchBox is terribly convoluted, and you're running into that. So it is probably not your fault that this doesn't really make a lot of sense. At the same time, I'd rather not make it worse 😬

zadjii-msft · 2021-01-19T21:18:31Z

src/cascadia/TerminalApp/TerminalPage.cpp

@@ -2541,6 +2544,18 @@ namespace winrt::TerminalApp::implementation
        termControl.CreateSearchBoxControl();
    }

+    void TerminalPage::_GoForward()
+    {
+        const auto termControl = _GetActiveControl();


Should these be

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

to make sure termControl isn't null?

zadjii-msft · 2021-01-19T21:44:06Z

src/cascadia/TerminalControl/TermControl.cpp

+        if (e.OriginalKey() == VirtualKey::Enter)
+        {
+            return;
+        }


Why is this needed?

zadjii-msft · 2021-01-19T21:46:07Z

src/cascadia/TerminalControl/TermControl.cpp

+        if (keyBindingSearch)
        {
+            keyBindingSearch = false;
            return;
        }


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?

zadjii-msft · 2021-01-19T21:46:28Z

src/cascadia/TerminalControl/TermControl.cpp

        // 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())


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

zadjii-msft · 2021-01-19T21:47:11Z

src/cascadia/TerminalSettingsModel/defaults.json

+        { "command": "nextSearchMatchKey", "keys": "ctrl+shift+n" },
+        { "command": "prevSearchMatchKey", "keys": "ctrl+shift+k" },


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

Don-Vito · 2021-01-19T21:53:07Z

@Hegunumo - it looks that @zadjii-msft and I wrote exactly the same comments 🤦 I am not removing mine, as they probably contain some more info, put please ignore the ones that seem double.

Don-Vito · 2021-01-25T23:57:54Z

@zadjii-msft - is it only me or Hegunumo user is shown as a deleted user? 😢

DHowett · 2021-01-26T00:00:15Z

Unfortunately, yes :( i was just putting together the selfhost announcement and the thanks section and found that I couldn’t correlate his username any longer.

zadjii-msft · 2021-01-27T13:10:44Z

Oh RIP. They had some great contributions. I'll inherit these abandoned PRs and help drive them across the finish line, since they're fundamentally good.

zadjii-msft · 2021-01-27T22:47:52Z

I'm moving this to #8917. Hopefully we can get this across the finish line ☺️

@carlos-zamora

This PR is a resurrection of #8522. @Hegunumo has apparently deleted their account, but the contribution was still valuable. I'm just here to get it across the finish line. This PR adds new action for navigating to the next & previous search results. These actions are unbound by default. These actions can be used from directly within the search dialog also, to immediately navigate the results. Furthermore, if you have a search started, and close the search box, then press this keybinding, _it will still perform the search_. So you can just hit <kbd>F3</kbd> repeatedly with the dialog closed to keep searching new results. Neat! If you dispatch the action on the key down, then dismiss a selection on a key up, we'll end up immediately destroying the selection when you release the bound key. That's annoying. It also bothers @carlos-zamora in #3758. However, I _think_ we can just only dismiss the selection on a key up. I _think_ that's fine. It _seems_ fine so far. We've got an entire release cycle to futz with it. ## Validation Steps Performed I've played with it all day and it seems _crisp_. Closes #7695 Co-authored-by: Kiminori Kaburagi <yukawa_hidenori@icloud.com>

Lo0oG reviewed Dec 8, 2020

View reviewed changes

Don-Vito mentioned this pull request Dec 10, 2020

Support key-bindings for "Find up" and "Find down" #4221

Closed

zadjii-msft self-assigned this Dec 10, 2020

zadjii-msft requested changes Dec 15, 2020

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Dec 15, 2020

zadjii-msft assigned ghost and unassigned zadjii-msft Dec 15, 2020

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Dec 15, 2020

kaihugo added 2 commits December 21, 2020 22:01

Hook up with TerminalPage

c083fb0

code format

694d07a

zadjii-msft assigned zadjii-msft and unassigned ghost Jan 19, 2021

Don-Vito reviewed Jan 19, 2021

View reviewed changes

zadjii-msft requested changes Jan 19, 2021

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jan 19, 2021

This was referenced Jan 19, 2021

Epic: Search v2 #3920

Open

Feature request: export console history as txt #642

Closed

zadjii-msft mentioned this pull request Jan 27, 2021

Add findNext, findPrev actions #8917

Merged

zadjii-msft closed this Jan 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add NextSearchMatch and PreviouSearchMatch actions #8522

Add NextSearchMatch and PreviouSearchMatch actions #8522

ghost commented Dec 8, 2020

ghost commented Dec 8, 2020 •

edited by ghost

Loading

Lo0oG Dec 8, 2020

zadjii-msft left a comment

zadjii-msft Dec 15, 2020

ghost commented Dec 21, 2020

zadjii-msft commented Jan 19, 2021

Don-Vito left a comment

Don-Vito Jan 19, 2021

zadjii-msft Jan 19, 2021

Don-Vito Jan 19, 2021

Don-Vito Jan 19, 2021

zadjii-msft Jan 27, 2021

zadjii-msft Jan 27, 2021

zadjii-msft Jan 27, 2021

Don-Vito Jan 19, 2021

zadjii-msft left a comment

zadjii-msft Jan 19, 2021

zadjii-msft Jan 19, 2021

zadjii-msft Jan 19, 2021

zadjii-msft Jan 19, 2021

zadjii-msft Jan 19, 2021

Don-Vito commented Jan 19, 2021

Don-Vito commented Jan 25, 2021 •

edited

Loading

DHowett commented Jan 26, 2021

zadjii-msft commented Jan 27, 2021

zadjii-msft commented Jan 27, 2021

		{ "command": "nextSearchMatchKey", "keys": "ctrl+shift+n" },
		{ "command": "prevSearchMatchKey", "keys": "ctrl+shift+k" },

Add NextSearchMatch and PreviouSearchMatch actions #8522

Add NextSearchMatch and PreviouSearchMatch actions #8522

Conversation

ghost commented Dec 8, 2020

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

ghost commented Dec 8, 2020 • edited by ghost Loading

Choose a reason for hiding this comment

zadjii-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Dec 21, 2020

zadjii-msft commented Jan 19, 2021

Don-Vito left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zadjii-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Don-Vito commented Jan 19, 2021

Don-Vito commented Jan 25, 2021 • edited Loading

DHowett commented Jan 26, 2021

zadjii-msft commented Jan 27, 2021

zadjii-msft commented Jan 27, 2021

ghost commented Dec 8, 2020 •

edited by ghost

Loading

Don-Vito commented Jan 25, 2021 •

edited

Loading