Avoid moving the selection while typing a search query (#15998)

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 ✅

src/buffer/out/search.cpp

@@ -8,30 +8,21 @@
 
 using namespace Microsoft::Console::Types;
 
-bool Search::ResetIfStale(Microsoft::Console::Render::IRenderData& renderData)
-{
-    return ResetIfStale(renderData,
-                        _needle,
-                        _step == -1, // this is the opposite of the initializer below
-                        _caseInsensitive);
-}
-
 bool Search::ResetIfStale(Microsoft::Console::Render::IRenderData& renderData, const std::wstring_view& needle, bool reverse, bool caseInsensitive)
 {
     const auto& textBuffer = renderData.GetTextBuffer();
     const auto lastMutationId = textBuffer.GetLastMutationId();
 
     if (_needle == needle &&
-        _reverse == reverse &&
         _caseInsensitive == caseInsensitive &&
         _lastMutationId == lastMutationId)
     {
+        _step = reverse ? -1 : 1;
         return false;
     }
 
     _renderData = &renderData;
     _needle = needle;
-    _reverse = reverse;
     _caseInsensitive = caseInsensitive;
     _lastMutationId = lastMutationId;
 
@@ -42,12 +33,38 @@ bool Search::ResetIfStale(Microsoft::Console::Render::IRenderData& renderData, c
     return true;
 }
 
-void Search::MovePastCurrentSelection()
+void Search::MoveToCurrentSelection()
 {
     if (_renderData->IsSelectionActive())
     {
-        MovePastPoint(_renderData->GetTextBuffer().ScreenToBufferPosition(_renderData->GetSelectionAnchor()));
+        MoveToPoint(_renderData->GetTextBuffer().ScreenToBufferPosition(_renderData->GetSelectionAnchor()));
+    }
+}
+
+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;
 }
 
 void Search::MovePastPoint(const til::point anchor) noexcept
@@ -58,18 +75,17 @@ void Search::MovePastPoint(const til::point anchor) noexcept
     }
 
     const auto count = gsl::narrow_cast<ptrdiff_t>(_results.size());
-    const auto highestIndex = count - 1;
-    auto index = _reverse ? highestIndex : 0;
+    ptrdiff_t index = 0;
 
-    if (_reverse)
+    if (_step < 0)
     {
-        for (; index >= 0 && til::at(_results, index).start >= anchor; --index)
+        for (index = count - 1; index >= 0 && til::at(_results, index).start >= anchor; --index)
         {
         }
     }
     else
     {
-        for (; index <= highestIndex && til::at(_results, index).start <= anchor; ++index)
+        for (index = 0; index < count && til::at(_results, index).start <= anchor; ++index)
         {
         }
     }
@@ -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
 {
     return _index;
 }

src/buffer/out/search.h

@@ -25,25 +25,23 @@ class Search final
 public:
     Search() = default;
 
-    bool ResetIfStale(Microsoft::Console::Render::IRenderData& renderData);
     bool ResetIfStale(Microsoft::Console::Render::IRenderData& renderData, const std::wstring_view& needle, bool reverse, bool caseInsensitive);
 
-    void MovePastCurrentSelection();
+    void MoveToCurrentSelection();
+    void MoveToPoint(til::point anchor) noexcept;
     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;
+    ptrdiff_t CurrentMatch() const noexcept;
 
 private:
     // _renderData is a pointer so that Search() is constexpr default constructable.
     Microsoft::Console::Render::IRenderData* _renderData = nullptr;
     std::wstring _needle;
-    bool _reverse = false;
     bool _caseInsensitive = false;
     uint64_t _lastMutationId = 0;
 

src/cascadia/TerminalControl/ControlCore.cpp

@@ -1584,7 +1584,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation
 
         if (_searcher.ResetIfStale(*GetRenderData(), text, !goForward, !caseSensitive))
         {
-            _searcher.MovePastCurrentSelection();
+            _searcher.MoveToCurrentSelection();
+            _cachedSearchResultRows = {};
         }
         else
         {
@@ -1615,12 +1616,13 @@ namespace winrt::Microsoft::Terminal::Control::implementation
 
     Windows::Foundation::Collections::IVector<int32_t> ControlCore::SearchResultRows()
     {
-        auto lock = _terminal->LockForWriting();
-        if (_searcher.ResetIfStale(*GetRenderData()))
+        const auto lock = _terminal->LockForReading();
+
+        if (!_cachedSearchResultRows)
         {
             auto results = std::vector<int32_t>();
-
             auto lastRow = til::CoordTypeMin;
+
             for (const auto& match : _searcher.Results())
             {
                 const auto row{ match.start.y };
@@ -1630,6 +1632,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
                     lastRow = row;
                 }
             }
+
             _cachedSearchResultRows = winrt::single_threaded_vector<int32_t>(std::move(results));
         }
 
@@ -2266,27 +2269,20 @@ namespace winrt::Microsoft::Terminal::Control::implementation
     Windows::Foundation::Collections::IVector<Control::ScrollMark> ControlCore::ScrollMarks() const
     {
         const auto lock = _terminal->LockForReading();
-        const auto& internalMarks{ _terminal->GetScrollMarks() };
-        auto v = winrt::single_threaded_observable_vector<Control::ScrollMark>();
-        for (const auto& mark : internalMarks)
-        {
-            Control::ScrollMark m{};
+        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));
     }
 
     void ControlCore::AddMark(const Control::ScrollMark& mark)

src/interactivity/win32/find.cpp

@@ -54,7 +54,7 @@ INT_PTR CALLBACK FindDialogProc(HWND hWnd, UINT Message, WPARAM wParam, LPARAM l
 
             if (searcher.ResetIfStale(gci.renderData, lastFindString, reverse, caseInsensitive))
             {
-                searcher.MovePastCurrentSelection();
+                searcher.MoveToCurrentSelection();
             }
             else
             {