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.

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

Show number of search results & positions of hits in scrollbar #14045

Merged
merged 80 commits into from
Sep 5, 2023

Conversation

zadjii-msft
Copy link
Member

@zadjii-msft zadjii-msft commented Sep 20, 2022

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 showMarksOnScrollbar is also turned on).

It's also been made SUBSTANTIALLY easier after #15858 was merged.

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.

Co-authored-by: Don-Vito khvitaly@gmail.com

@ghost ghost added Area-Accessibility Issues related to accessibility Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal. labels Sep 20, 2022
@zadjii-msft zadjii-msft mentioned this pull request Sep 20, 2022
20 tasks
@zadjii-msft
Copy link
Member Author

search-results-realtime

@zadjii-msft
Copy link
Member Author

1fb4331...574f30b has the diff of this, rebased onto #15858

@zadjii-msft
Copy link
Member Author

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

zadjii-msft added a commit that referenced this pull request Aug 29, 2023
src/cascadia/TerminalControl/ControlCore.cpp Outdated Show resolved Hide resolved
// - enable: if true, the buttons should be enabled
// Return Value:
// - <none>
void SearchBoxControl::SetNavigationEnabled(bool enabled)
Copy link
Member

Choose a reason for hiding this comment

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

this is still a "property setter"; should it be modelled as such in the IDL?

@@ -195,7 +203,8 @@
Height="32"
Margin="4,0"
Padding="0"
BackgroundSizing="OuterBorderEdge">
BackgroundSizing="OuterBorderEdge"
Click="CaseSensitivityButtonClicked">
Copy link
Member

Choose a reason for hiding this comment

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

technically, the way to do this kind of thing is to have a bunch of data bindings and property changed callbacks. It's "right," but it's not "fun" and it confers no benefit except that of clarity... so I am not going to block over it!

@@ -302,6 +302,21 @@ namespace winrt::Microsoft::Terminal::Control::implementation
const auto fullHeight{ ScrollBarCanvas().ActualHeight() };
const auto totalBufferRows{ update.newMaximum + update.newViewportSize };

auto drawPip = [&](const auto row, const auto rightAlign, const auto& brush) {
Copy link
Member

Choose a reason for hiding this comment

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

LOL, auto. you could drawPip("e", 1.3f, nullptr) and it would dutifully attempt to produce a lambda that supports that.

Copy link
Member

Choose a reason for hiding this comment

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

It's only weird for the boolean, since it's the thing that is the most convertible.

src/cascadia/TerminalControl/TermControl.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalCore/Terminal.hpp Show resolved Hide resolved
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

Thanks so much for dragging this across the finish line, and thanks to @Don-Vito for the original work. We wouldn't have done any of the ICU stuff if not for the both of you 😄

@zadjii-msft if you haven't already, make sure to put a Co-authored-by line at the bottom of the commit message (below the Closes and stuff)

@zadjii-msft zadjii-msft merged commit 0cbde94 into main Sep 5, 2023
17 checks passed
@zadjii-msft zadjii-msft deleted the dev/migrie/fhl/search-marks branch September 5, 2023 21:23
@carlos-zamora
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Accessibility Issues related to accessibility Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal. zBugBash-Consider
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Display "find" matches in the scrollbar Search should display the number of results it finds
6 participants