Fix search constantly triggering a scroll #17132
Conversation
lhecker
added
Issue-Bug
|
the build is highly imploded, so I may not review it yet ;P |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| { | ||
| _terminal->SetSearchHighlightFocused(gsl::narrow<size_t>(idx)); | ||
| } | ||
| _renderer->TriggerSearchHighlight(oldResults); |
There was a problem hiding this comment.
won't oldResults be empty if we aren't invalidating? i guess I don't understand
There was a problem hiding this comment.
You mean what happens if IsStale() returned false and we didn't call Reset()?
TriggerSearchHighlight is basically a "refresh all these areas" call. Normally we need to refresh all current areas, which is why oldResults is empty. It gets the current areas directly from IRenderData (previously set by SetSearchHighlights). During a reset oldResults contains the previous areas as well, since those aren't highlighted anymore = dirty.
Now that I think about it, I think we could also just remove the parameter to TriggerSearchHighlight and simply call it once before calling Reset() (= invalidate all the old highlights) and then once after (= all the new and current highlights).
There was a problem hiding this comment.
Now that I think about it, I think we could also just remove the parameter to TriggerSearchHighlight and simply call it once before calling Reset() (= invalidate all the old highlights) and then once after (= all the new and current highlights).
Nice 😃
TriggerSearchHighlight() also isn't completely correct in that the old highlights are supposed to be invalidated w.r.t old line renditions, but we currently don't store old line renditions so we're just using the current line renditions. I'm sure this can be fixed with buffer snapshot based rendering where we would pass in the line rendition from the current snapshot of the buffer that render engine has access to.
| @@ -1654,30 +1654,41 @@ namespace winrt::Microsoft::Terminal::Control::implementation | |||
| // - text: the text to search | |||
| // - goForward: boolean that represents if the current search direction is forward | |||
| // - caseSensitive: boolean that represents if the current search is case sensitive | |||
| // - resetOnly: If true, only Reset() will be called, if anything. FindNext() will never be called. | |||
There was a problem hiding this comment.
| // - resetOnly: If true, only Reset() will be called, if anything. FindNext() will never be called. | |
| // - resetOnly: If true, only Search::IsStale() will be called, if anything. FindNext() will never be called. |
right?
There was a problem hiding this comment.
No, actually the comment is correct, but the boolean logic inside the function is very difficult to read. Basically, if resetOnly is true then if (searchInvalidated || !resetOnly) is only entered if searchInvalidated is true. If searchInvalidated is true then only Reset() will be called.
There was a problem hiding this comment.
oh yea that's hard to parse
This addresses a review comment left by tusharsnx in #17092 which I
forgot to fix before merging the PR. The fix itself is somewhat simple:
Terminal::SetSearchHighlightFocusedtriggers a scroll if the targetis outside of the current (scrolled) viewport and avoiding the call
unless necessary fixes it. To do it properly though, I've split up
Search::ResetIfStaleintoIsStaleandReset. Now we can properlydetect staleness in advance and branch out the search reset cleanly.
Additionally, I've taken the liberty to replace the
IVectorinSearchResultRowswith a directconst std::vector&intoSearcher.This removes a bunch of code and makes it faster to boot.
Validation Steps Performed
showMarksOnScrollbarstill works ✅