Fix search highlights during reflow by lhecker · Pull Request #17092 · microsoft/terminal

lhecker · 2024-04-20T14:46:15Z

This PR extends til::throttled_func to also support debouncing:

throttling: "At most 1 call every N seconds"
debouncing: "Exactly 1 call after N seconds of inactivity"

Based on the latter the following series of changes were made:

An OutputIdle event was added to ControlCore which is
raised once there hasn't been any incoming data in 100ms.
This also triggers an update of our regex patterns (URL detection).
The event is then caught by TermControl which calls Search().
Search() in turn was modified to return its results by-value
as a struct, which avoids the need for a search-update event
and simplifies how we update the UI.

This architectural change, most importantly the removal of the
TextLayoutUpdated event, fixes a DoS bug in Windows Terminal:
As the event leads to UI thread activity, printing lots of text
continuously results in the UI thread becoming unresponsive.

On top of these, a number of improvements were made:

IRenderEngine::InvalidateHighlight was changed to take the
TextBuffer by-reference which avoids the need to accumulate the
line renditions in a std::vector first. This improves Debug build
performance during reflow by what I guess must be roughly
a magnitude faster. This difference is very noticeable.
When closing the search box, ClearSearch() is called to remove
the highlights. The search text is restored when it's reopened,
however the current search position isn't.

Closes #17073
Closes #17089

Validation Steps Performed

UIA announcements:
- Pressing Ctrl+Shift+F the first time does not lead to one ✅
- Typing the first letter does ✅
- Closing doesn't ✅
- Reopening does (as it restores the letter) ✅
Closing the search box dismisses the highlights ✅
Resizing the window recalculates the highlights ✅
Changing the terminal output while the box is open
recalculates the highlights ✅

lhecker · 2024-04-20T23:26:22Z

src/buffer/out/search.cpp


-    if (_needle == needle &&
+    if (_renderData == &renderData &&
+        _needle == needle &&


I made this change because I grew a little worried about the robustness of the _lastMutationId == lastMutationId check.

lhecker · 2024-04-20T23:27:08Z

src/cascadia/TerminalControl/ControlCore.cpp

+                    {
+                        self->OutputIdle.raise(*self, nullptr);
+                    }
+                });


I can imagine that this debounced event may be quite useful for other things in the future.

lhecker · 2024-04-20T23:29:10Z

src/cascadia/TerminalControl/ControlCore.cpp


            _terminal->SetSearchHighlights(_searcher.Results());
-            _terminal->SetSearchHighlightFocused(_searcher.CurrentMatch());
+            _renderer->TriggerSearchHighlight(oldResults);


From what I can tell, we only need to call this function if the search got reset so I moved it inside this branch.

This results in focused search highlight to be not invalidated even though it might change in the below else block.

I know it sounds inefficient to invalidate all highlights when just the focused one is changed, but to fix this inefficiency I guess you have to add TriggerSearchHighlightFocused() or something along those lines. And the Atlas engine also doesn't know which one got invalidated so it currently just re-draws both 🤕🥲

Oh, I see. I think it's fine to have a bit of inefficiency here. I bet that the invalidation finishes in less than 1ms even if you had a million search hits - and we only refresh it 10 times per second at most.

If we get to the point where we have buffer snapshotting in the renderer, we can simply scribble the highlights into the snapshot directly. Then we simply check if the text or attributes of each row have changed compared to the last snapshot. (= We have 2 snapshots which we swap back and forth every other frame.) I'm pretty excited for that, as all those subtle issues like this one will just instantly vanish.

lhecker · 2024-04-20T23:29:14Z

src/cascadia/TerminalControl/ControlCore.cpp

+            _renderer->TriggerSearchHighlight(oldResults);
        }
-        else
+        else if (!reset)


This allows us to ensure that we don't accidentally call FindNext() just because the OutputIdle event got triggered, which results in a Search() call.

lhecker · 2024-04-20T23:31:02Z

src/cascadia/TerminalControl/ControlCore.cpp

+        _terminal->SetSearchHighlights({});
+        _terminal->SetSearchHighlightFocused({});
+        _renderer->TriggerSearchHighlight(_searcher.Results());
+        _searcher = {};
+        _cachedSearchResultRows = {};


We can't check _searcher.GetCurrent() here because otherwise we don't reset the _lastMutationId of a Searcher that found nothing. This is relevant in case you open the search box (the Ctrl+Shift+F one), enter something that doesn't exist, close it, and reopen it. If it gets closed and doesn't get reset here, then reopening it won't trigger a search otherwise.

lhecker · 2024-04-20T23:32:18Z

src/cascadia/TerminalControl/TermControl.cpp

+                    .newMinimum = scrollBar.Minimum(),
+                    .newViewportSize = scrollBar.ViewportSize(),
+                };
+                _updateScrollBar->Run(update);


Using the throttled func here is IMO just fine, and should ensure that we don't unnecessarily overwrite a pending scrollbar update.

lhecker · 2024-04-20T23:32:43Z

src/cascadia/TerminalControl/TermControl.cpp

+        _searchBox->SetStatus(results.TotalMatches, results.CurrentMatch);

-        if (_searchBox)
+        if (results.SearchInvalidated)


This avoids doing a RaiseNotificationEvent even though the search mask hasn't changed.

lhecker · 2024-04-20T23:41:00Z

src/renderer/atlas/AtlasEngine.api.cpp

 }

-[[nodiscard]] HRESULT AtlasEngine::InvalidateHighlight(std::span<const til::point_span> highlights, const std::vector<LineRendition>& renditions) noexcept
+[[nodiscard]] HRESULT AtlasEngine::InvalidateHighlight(std::span<const til::point_span> highlights, const TextBuffer& buffer) noexcept


Passing the TextBuffer directly is IMO fine, because long-term we'll remove Renderer and give each render engine direct access to the underlying TextBuffer anyway. (In all my tests so far I found that we can remove at least half of all rendering code by doing that. I'm excited to think about how much easier it'll make introducing new features!)

BTW, as mentioned in the PR comment, you may have noticed that a debug build felt kind of laggy while resizing the window - This change fixes it.

lhecker · 2024-04-20T23:45:45Z

@tusharsnx I apologize for making this many changes so soon after we merged your PR. I hope these are fine. 😣 They do fix a couple more edge cases around highlights, however. If you have any concerns with my changes, or dislike anything, let me know and I'll try to fix it ASAP.

BTW ResetIfStale really isn't a good API, isn't it? I refrained from changing any of that in this PR, however.

tusharsnx · 2024-04-21T09:36:39Z

I hope these are fine.

I'm ABSOLUTELY ~~fine~~ happy that this PR fixed the remaining issues. I can see how it simplifies the whole adapt-dispatcher ➡️ Terminal ➡️ TermControl interop thing. This is how my PR should've done it already. Thanks for doing this 😄

BTW ResetIfStale really isn't a good API, isn't it?

Its idempotency is questionable 😅

OK, now on to this PR:

The Focused highlight seems to force a scroll towards it. If I scroll toward the top, it scrolls the buffer back to where the focused highlight is.
The renderer doesn't seem to re-render the focused highlight region correctly. (I also added a review comment about it). For testing, you can try searching for a word/letter that appears more than once within the viewport.

tusharsnx · 2024-04-22T13:30:29Z

src/cascadia/TerminalControl/TermControl.cpp

-            .newViewportSize = scrollBar.ViewportSize(),
-        };
-        _throttledUpdateScrollbar(update);
+        if (!_searchBox || _searchBox->Visibility() != Visibility::Visible)


Could this cause a problem when the search box is closed and the user uses Find previous/next match actions to navigate through the results?

Opened the search box, and typed "tusha" into it, and then closed it. Search highlights were removed as expected. Used Find next search match, and the highlights are active again. Entered some text containing "tusha". Highlight didn't refresh. After running clear command:

Oh, I wasn't aware we had an action for that. If you use such an action in VS Code while its search box is closed, it opens it. I think we should do the same thing. Thanks for finding this!

(Did we fix this?)

Both underlying issues should be fixed now:

When using the action, we now open the search box. This triggers a search once it finished opening

While searching we'll now always call TriggerSearchHighlight

tusharsnx · 2024-04-22T13:30:51Z

src/cascadia/TerminalControl/TermControl.cpp

    {
-        co_await wil::resume_foreground(Dispatcher());
-        if (auto automationPeer{ Automation::Peers::FrameworkElementAutomationPeer::FromElement(*this) })
+        if (!_searchBox)


Same query as above.

I guess this one isn't a problem because we're only checking for the existence of the search box and not its visibility.

…rch-highlights

zadjii-msft · 2024-04-23T15:11:06Z

FWIW this does seem to fix #17089

zadjii-msft

i'm 23/27 but i'll review those in post tomorrow, I just want my sweet sweet canary builds back

zadjii-msft

confirming: yea I'm cool with this

zadjii-msft · 2024-04-24T11:30:53Z

src/inc/til/throttled_func.h

-            if (!_storage.emplace(std::forward<MakeArgs>(args)...))
+            const auto hadValue = _storage.emplace(std::forward<MakeArgs>(args)...);
+
+            if constexpr (Debounce)


oh oh okay I get it now. For a while I was thinking "a debounced trailing func is just a trailing func", but the difference is subtle:

A trailing func will call the callback {one timeout} after the first call to the throttled_func. If there are more calls to the throttled_func in that timeout, they'll just update the value which will be used at the end of the first timeout.

A debounced trailing func will call the callback {one timeout} after the last call to the throttled_func. If there are more calls to the throttled_func in that timeout, they'll further delay the callback.

tusharsnx · 2024-04-24T12:41:00Z

Sorry for the late report, but it seems like this is not yet fixed:

The Focused highlight seems to force a scroll towards it. If I scroll toward the top, it scrolls the buffer back to where the focused highlight is.

lhecker · 2024-04-24T13:55:59Z

Ah fuck I forgot about that comment. Sorry about that! 😣
I'll try to investigate soon.

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::SetSearchHighlightFocused` triggers a scroll if the target is outside of the current (scrolled) viewport and avoiding the call unless necessary fixes it. To do it properly though, I've split up `Search::ResetIfStale` into `IsStale` and `Reset`. Now we can properly detect staleness in advance and branch out the search reset cleanly. Additionally, I've taken the liberty to replace the `IVector` in `SearchResultRows` with a direct `const std::vector&` into `Searcher`. This removes a bunch of code and makes it faster to boot. ## Validation Steps Performed * Print lots of text * Search a common letter * Scroll up * Doesn't scroll back down ✅ * Hold enter to search more occurrences scrolls up as needed ✅ * `showMarksOnScrollbar` still works ✅

lhecker added 3 commits April 20, 2024 16:32

Fix search highlights during reflow

ea40774

Fix build

1df0efa

Remove unused declaration

c7f2ae3

lhecker commented Apr 20, 2024

View reviewed changes

Revert accidental change

435f66b

tusharsnx reviewed Apr 22, 2024

View reviewed changes

Merge remote-tracking branch 'origin/main' into dev/lhecker/17073-sea…

070edeb

…rch-highlights

Address feedback

00073e4

DHowett approved these changes Apr 23, 2024

View reviewed changes

DHowett enabled auto-merge April 23, 2024 20:23

zadjii-msft approved these changes Apr 23, 2024

View reviewed changes

DHowett added this pull request to the merge queue Apr 23, 2024

Merged via the queue into main with commit 360e86b Apr 23, 2024

DHowett deleted the dev/lhecker/17073-search-highlights branch April 23, 2024 22:59

zadjii-msft reviewed Apr 24, 2024

View reviewed changes

lhecker mentioned this pull request Apr 25, 2024

Fix search constantly triggering a scroll #17132

Merged

Conversation

lhecker commented Apr 20, 2024

Validation Steps Performed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lhecker commented Apr 20, 2024

Uh oh!

tusharsnx commented Apr 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lhecker Apr 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tusharsnx Apr 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zadjii-msft commented Apr 23, 2024

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tusharsnx commented Apr 24, 2024

Uh oh!

lhecker commented Apr 24, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tusharsnx commented Apr 21, 2024 •

edited

Loading

lhecker Apr 23, 2024 •

edited

Loading

tusharsnx Apr 22, 2024 •

edited

Loading