-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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
Conversation
Co-authored-by: Don-Vito <[email protected]>
1fb4331...574f30b has the diff of this, rebased onto #15858 |
Alrighty this is cleaned up post-#15858. No throttling or aggregating searches one at a time anymore. |
// - enable: if true, the buttons should be enabled | ||
// Return Value: | ||
// - <none> | ||
void SearchBoxControl::SetNavigationEnabled(bool enabled) |
There was a problem hiding this comment.
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"> |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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)
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. |
This is a resurrection of #8588. That PR became painfully stale after the
ControlCore
split. Original description: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 [email protected]