Show number of search results & positions of hits in scrollbar

[zadjii-msft]

🔬 this is FHL code 🔬

This is a resurrection of #8588. That PR became painfully stale after the ControlCore split. Original description:

Summary of the Pull Request

This is a PoC for:

• Search status in SearchBox (aka number of matches + index of the current match)
• Live search (aka search upon typing)

Detailed Description of the Pull Request / Additional comments

• Introduced this optionally (global setting to enable it)
• The approach is following:
  • Every time the filter changes, enumerate all matches
  • Upon navigation just take the relevant match and select it

I cleaned it up a bit, and added support for also displaying the positions of the matches in the scrollbar (if showMarksInScrollbar is also turned on). I moved a lot of work into just using a ThrottledFunc. Seems to work alright.

Similar to before, searching while there's piles of output running isn't perfect. But it's pretty awful currently, so that's not the end of the world.

Gifs below.

• closes #8631 (which is a bullet point in #3920)
• closes #6319

Co-authored-by: Don-Vito [email protected]

[zadjii-msft]

[github-actions[bot]]

@check-spelling-bot Report

:red_circle: Please review

See the :open_file_folder: files view or the :scroll:action log for details.

Unrecognized words (7)

caculate Copys finsh inbetween targetting teminal versionf

To accept :heavy_check_mark: these unrecognized words as correct, run the following commands

... in a clone of the [email protected]:microsoft/terminal.git repository on the dev/migrie/fhl/search-marks branch (:information_source: how do I use this?):

curl -s -S -L 'https://raw.githubusercontent.com/check-spelling/check-spelling/v0.0.21/apply.pl' |
perl - 'https://github.com/microsoft/terminal/actions/runs/3568352769/attempts/1'

:pencil2: Contributor please read this

By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.

:warning: The command is written for posix shells. If it doesn't work for you, you can manually add (one word per line) / remove items to expect.txt and the excludes.txt files.

If the listed items are:

• ... misspelled, then please correct them instead of using the command.
• ... names, please add them to .github/actions/spelling/allow/names.txt.
• ... APIs, you can add them to a file in .github/actions/spelling/allow/.
• ... just things you're using, please add them to an appropriate file in .github/actions/spelling/expect/.
• ... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in .github/actions/spelling/patterns/.

See the README.md in each directory for more information.

:microscope: You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. :wink:

If the flagged items are :exploding_head: false positives

If items relate to a ...

• binary file (or some other file you wouldn't want to check at all).

  Please add a file path to the excludes.txt file matching the containing file.

  File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

  ^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).

• well-formed pattern.

  If you can write a pattern that would match it, try adding it to the patterns.txt file.

  Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines.

  Note that patterns can't match multiline strings.

[zadjii-msft]

@lhecker I'm looping back on this one now that tearout is shipped.

Your earlier comments were mostly about the co_awaits back and forth, once per result basically. That was terrible, you're right. I think in its current form, this will:

• hop to a bg thread once, in _SearchAsync
• in the BG, collect all the matches.
  • while doing this it will lock/unlock once per result. Still not great, but not a whole thread hop.
• return all results at once.

Yea, Search should probably be better, but that seems like,,, a separate follow up. You okay with that?

[lhecker]

Yeah, it's fine. We can improve upon it later if we ever want to. 🙂 It'll then be feasible to accumulate all matches without throttling which should simplify the code in this PR a bit, which might also be helpful.

BTW reading through the PR I noticed a few nits - I'll comment them.

[zadjii-msft]

Test that's broken got broken in a merge with main.

• circa fa06c6f...b288f5dec97a9170d5e8db05efb97863b75c537a, I had added the rowsPushedOffTopOfBuffer != 0 check to

  if (updatedViewport || rowsPushedOffTopOfBuffer != 0)
  

  in Terminal::_AdjustCursorPosition. That was so that we'd get scroll notifications in the control even if all that happened was a buffer rotation.
• At some point in the past, we entirely nuked _AdjustCursorPosition.
• Some of that functionality moved to Terminal::NotifyBufferRotation.
• Notably there, we only send the scroll notification on buffer rotation if there's scroll marks.

I vaguely recall in the past that Terminal::_NotifyScrollEvent was a little bit of a hot path in raw throughput - it'd get called like, once per line. So, limiting to when there was a real delta was ideal.

But now, we've got these scroll marks on the scrollbar, and they really need all the notifications - otherwise rotating the buffer just leaves the result pips where they are.

A dumb option might be a bool Terminal::AlwaysNotifyOnBufferRotation that ControlCore could enable on it. That would limit throughpuit impact to when there's actually scroll marks... but that feels wrong too?

[zadjii-msft]

OR

we could add a circled to _pfnScrollPositionChanged and have ControlCore filter there. That's an idea....

err, but that still impacts raw throughput, cause each line still needs to calculate Terminal::_GetVisibleViewport just to have ControlCore later discard it. uhg.

[DHowett]

If this is ready to merge, can you remove the "FHL CODE" disclaimer? :D

[zadjii-msft]

https://github.com/microsoft/terminal/compare/1fb4331cb364a9b332fc767791df50d6e0aa55bc...574f30b4249a576413087a4bc11bb7250055d694 has the diff of this, rebased onto #15858

[zadjii-msft]

Alrighty this is cleaned up post-#15858. No throttling or aggregating searches one at a time anymore.

[carlos-zamora]

Tested this out with Narrator. It reads out "results found" or "no results found", which is great!

I think we should announce the index too (since that's information that visual users get but non-visual users can't access it). That's basically covered by #14153 though.