Repository: microsoft/terminal (language: cpp)

Issue:
[1.19+] Search acts hilariously when you continue typing a needle that has a lot of matches
![WindowsTerminal_5Uk8wL23Ww](https://github.com/microsoft/terminal/assets/189190/6e9d7002-5405-4c2b-8dda-39929a9be618)

It jumps back for each live match!

Regressed in #14045 


Discussion / hints:

Fix patch:
diff --git a/src/buffer/out/search.cpp b/src/buffer/out/search.cpp
index eef5fac17cb..9707e8fdeaa 100644
--- a/src/buffer/out/search.cpp
+++ b/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;
 }
diff --git a/src/buffer/out/search.h b/src/buffer/out/search.h
index 2c6bc80a724..c2d035e3e9c 100644
--- a/src/buffer/out/search.h
+++ b/src/buffer/out/search.h
@@ -25,10 +25,10 @@ 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;
 
@@ -36,14 +36,12 @@ class Search final
     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;
 
diff --git a/src/cascadia/TerminalControl/ControlCore.cpp b/src/cascadia/TerminalControl/ControlCore.cpp
index 31462753ff0..06ef1ce7715 100644
--- a/src/cascadia/TerminalControl/ControlCore.cpp
+++ b/src/cascadia/TerminalControl/ControlCore.cpp
@@ -1583,7 +1583,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation
 
         if (_searcher.ResetIfStale(*GetRenderData(), text, !goForward, !caseSensitive))
         {
-            _searcher.MovePastCurrentSelection();
+            _searcher.MoveToCurrentSelection();
+            _cachedSearchResultRows = {};
         }
         else
         {
@@ -1614,12 +1615,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 };
@@ -1629,6 +1631,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
                     lastRow = row;
                 }
             }
+
             _cachedSearchResultRows = winrt::single_threaded_vector<int32_t>(std::move(results));
         }
 
@@ -2265,27 +2268,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)
diff --git a/src/interactivity/win32/find.cpp b/src/interactivity/win32/find.cpp
index 93c4d55a8b4..4e7cf36a589 100644
--- a/src/interactivity/win32/find.cpp
+++ b/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
             {


Test patch: