Avoid moving the selection while typing a search query #15998

lhecker · 2023-09-19T15:55:52Z

This commit fixes 2 issues:

ControlCore::ScrollMarks() would call ResetIfStale
again while the search prompt hasn't changed.
This has been fixed by using _cachedSearchResultRows as
the indicator for whether it needs to be recreated or not.
While typing a search query, the selection would move among the
results with each typed character, because MovePastCurrentSelection
would do what its name indicates. It has been renamed and rewritten
to be MoveToCurrentSelection. To avoid breaking UIA, the previous
MovePastPoint implementation was kept.

Since the new MoveToCurrentSelection function would not move past the
current selection anymore, changing the direction would not move past
the current result either. To fix this, we now don't invalidate the
search cache when changing the direction.

Closes #15954

Validation Steps Performed

Run "helloworld`n"*20 in pwsh
Search for "helloworld"
While typing the characters the selection doesn't move ✅
...nor when searching downwards ✅
...nor when erasing parts of it ✅
...and it behaves identical in conhost ✅

lhecker · 2023-09-19T15:57:47Z

src/buffer/out/search.cpp

        _caseInsensitive == caseInsensitive &&
        _lastMutationId == lastMutationId)
    {
+        _step = reverse ? -1 : 1;


Since the new MoveToCurrentSelection function would not move past the current selection anymore, changing the direction would not move past the current result either. To fix this, we now don't invalidate the search cache when changing the direction.

lhecker · 2023-09-19T15:58:41Z

src/buffer/out/search.cpp

+void Search::MoveToPoint(const til::point anchor) noexcept
+{
+    if (_results.empty())
+    {
+        return;
+    }
+
+    const auto count = gsl::narrow_cast<ptrdiff_t>(_results.size());
+    ptrdiff_t index = 0;
+
+    if (_step < 0)
+    {
+        for (index = count - 1; index >= 0 && til::at(_results, index).start > anchor; --index)
+        {
+        }
+    }
+    else
+    {
+        for (index = 0; index < count && til::at(_results, index).start < anchor; ++index)
+        {
+        }
    }
+
+    _index = (index + count) % count;
 }


This new function is a copy of the existing one below but with < instead of <=.

lhecker · 2023-09-19T15:59:59Z

src/buffer/out/search.cpp


-    if (_reverse)
+    if (_step < 0)


I figured that if we don't invalidate on _reverse anymore we can use _step as the indicator for whether it's in reverse mode. That's why I slightly moved these variables around because it's now a bit neater.

lhecker · 2023-09-19T16:00:12Z

src/buffer/out/search.cpp

@@ -119,7 +135,7 @@ const std::vector<til::point_span>& Search::Results() const noexcept
    return _results;
 }

-size_t Search::CurrentMatch() const noexcept
+ptrdiff_t Search::CurrentMatch() const noexcept


_index is a ptrdiff_t.

lhecker · 2023-09-19T16:00:42Z

src/buffer/out/search.h

    void MovePastPoint(til::point anchor) noexcept;
    void FindNext() noexcept;

    const til::point_span* GetCurrent() const noexcept;
    bool SelectCurrent() const;

    const std::vector<til::point_span>& Results() const noexcept;
-    size_t CurrentMatch() const noexcept;
-    bool CurrentDirection() const noexcept;


CurrentDirection() had no definition?

lhecker · 2023-09-19T16:03:21Z

src/cascadia/TerminalControl/ControlCore.cpp

+        const auto& internalMarks = _terminal->GetScrollMarks();
+        std::vector<Control::ScrollMark> v;

-            // sneaky: always evaluate the color of the mark to a real value
-            // before shoving it into the optional. If the mark doesn't have a
-            // specific color set, we'll use the value from the color table
-            // that's appropriate for this category of mark. If we do have a
-            // color set, then great we'll use that. The TermControl can then
-            // always use the value in the Mark regardless if it was actually
-            // set or not.
-            m.Color = OptionalFromColor(_terminal->GetColorForMark(mark));
-            m.Start = mark.start.to_core_point();
-            m.End = mark.end.to_core_point();
+        v.reserve(internalMarks.size());

-            v.Append(m);
+        for (const auto& mark : internalMarks)
+        {
+            v.emplace_back(
+                mark.start.to_core_point(),
+                mark.end.to_core_point(),
+                OptionalFromColor(_terminal->GetColorForMark(mark)));
        }

-        return v;
+        return winrt::single_threaded_vector(std::move(v));


Adding this to the PR was a bit of a mistake (I did git restore from another branch). But it might be something we'd want to keep anyways - up to you all! 😅
The benefits are:

Much much faster vector creation (without WinRT indirection)

Reserve the right amount of memory

emplace_back constructs the trivial struct in-place instead of constructing it on the stack first and then copying it over to the heap

~~BTW I think this method is missing lock guards. Scroll marks are being written by the VT thread after all. I'm sure my other PR adds them. ^^~~ Merged the fix from main.

absolutely fine with me

…rch-fixup

Avoid moving the selection while typing a search query

b32d5fc

lhecker commented Sep 19, 2023

View reviewed changes

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

2188fa8

…rch-fixup

carlos-zamora approved these changes Sep 20, 2023

View reviewed changes

zadjii-msft approved these changes Sep 21, 2023

View reviewed changes

lhecker added the AutoMerge Marked for automatic merge by the bot when requirements are met label Sep 21, 2023

microsoft-github-policy-service bot merged commit fc4a37e into main Sep 21, 2023
17 checks passed

microsoft-github-policy-service bot deleted the dev/lhecker/15954-search-fixup branch September 21, 2023 11:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid moving the selection while typing a search query #15998

Avoid moving the selection while typing a search query #15998

lhecker commented Sep 19, 2023 •

edited

lhecker Sep 19, 2023

lhecker Sep 19, 2023

lhecker Sep 19, 2023

lhecker Sep 19, 2023

lhecker Sep 19, 2023

lhecker Sep 19, 2023 •

edited

DHowett Sep 20, 2023

Avoid moving the selection while typing a search query #15998

Avoid moving the selection while typing a search query #15998

Conversation

lhecker commented Sep 19, 2023 • edited

Validation Steps Performed

lhecker Sep 19, 2023

Choose a reason for hiding this comment

lhecker Sep 19, 2023

Choose a reason for hiding this comment

lhecker Sep 19, 2023

Choose a reason for hiding this comment

lhecker Sep 19, 2023

Choose a reason for hiding this comment

lhecker Sep 19, 2023

Choose a reason for hiding this comment

lhecker Sep 19, 2023 • edited

Choose a reason for hiding this comment

DHowett Sep 20, 2023

Choose a reason for hiding this comment

lhecker commented Sep 19, 2023 •

edited

lhecker Sep 19, 2023 •

edited