replace search implementation with a more readable one

this replaces the current implementation that uses a lot of if cases
with a simpler one based on iterators and a wrapper for ease of usage

Author: lievenhey

src/models/disassemblymodel.cpp

@@ -224,14 +224,15 @@ QModelIndex DisassemblyModel::indexForFileLine(const Data::FileLine& fileLine) c
     return index(bestMatch, 0);
 }
 
-void DisassemblyModel::find(const QString& search, Direction direction, int offset)
+void DisassemblyModel::find(const QString& search, Direction direction, int current)
 {
     auto searchFunc = [&search](const DisassemblyOutput::DisassemblyLine& line) {
         return line.disassembly.indexOf(search, 0, Qt::CaseInsensitive) != -1;
     };
 
-    const auto resultIndex = ::search(
-        m_data.disassemblyLines, searchFunc, [this] { emit resultFound({}); }, direction, offset);
+    auto endReached = [this] { emit searchEndReached(); };
+
+    const int resultIndex = ::search(m_data.disassemblyLines, current, direction, searchFunc, endReached);
 
     if (resultIndex >= 0) {
         emit resultFound(createIndex(resultIndex, DisassemblyColumn));

src/models/search.h

@@ -7,6 +7,8 @@
 
 #pragma once
 
+#include <algorithm>
+#include <iterator>
 #include <QVector>
 
 enum class Direction
@@ -15,36 +17,51 @@ enum class Direction
     Backward
 };
 
-template<typename entry, typename SearchFunc, typename EndReached>
-int search(QVector<entry> source, SearchFunc&& searchFunc, EndReached&& endReached, Direction direction, int offset)
+/** a search function that wrap around at the end
+ * this function evaluates searchFunc starting from current and returns the offset from begin. In case end is reached,
+ * it calls endReached and starts again at begin
+ *
+ * return: offset from begin
+ * */
+template<typename iter, typename SearchFunc, typename EndReached>
+int search_impl(iter begin, iter end, iter current, SearchFunc searchFunc, EndReached endReached)
 {
-    if (offset > source.size() || offset < 0) {
-        offset = 0;
-    }
-
-    auto start = direction == Direction::Forward
-        ? (source.begin() + offset)
-        : (source.end() - (source.size() - offset - 1)); // 1 one due to offset of the reverse iterator
+    if (begin == end)
+        return -1;
 
-    auto it = direction == Direction::Forward
-        ? std::find_if(start, source.end(), searchFunc)
-        : (std::find_if(std::make_reverse_iterator(start), source.rend(), searchFunc) + 1).base();
+    auto start = current + 1;
+    auto found = std::find_if(start, end, searchFunc);
 
-    // it is less than source.begin() if the backward search ends at source.rend()
-    if (it >= source.begin() && it < source.end()) {
-        auto distance = std::distance(source.begin(), it);
-        return distance;
+    if (found != end) {
+        return std::distance(begin, found);
     }
 
-    it = direction == Direction::Forward
-        ? std::find_if(source.begin(), start, searchFunc)
-        : (std::find_if(source.rbegin(), std::make_reverse_iterator(start), searchFunc) + 1).base();
+    endReached();
 
-    if (it != source.end()) {
-        auto distance = std::distance(source.begin(), it);
-        endReached();
-        return distance;
+    found = std::find_if(begin, start, searchFunc);
+
+    if (found != end) {
+        return std::distance(begin, found);
     }
 
     return -1;
 }
+
+// a wrapper around search_impl that returns the result index from begin
+template<typename Array, typename SearchFunc, typename EndReached>
+int search(const Array& array, int current, Direction direction, SearchFunc searchFunc, EndReached endReached)
+{
+    current = std::clamp(0, static_cast<int>(array.size()), current);
+    int resultIndex = 0;
+
+    if (direction == Direction::Forward) {
+        resultIndex = ::search_impl(array.begin(), array.end(), array.begin() + current, searchFunc, endReached);
+    } else {
+        resultIndex = ::search_impl(array.rbegin(), array.rend(),
+                                    std::make_reverse_iterator(array.begin() + current) - 1, searchFunc, endReached);
+        if (resultIndex != -1) {
+            resultIndex = array.size() - resultIndex - 1;
+        }
+    }
+    return resultIndex;
+}

src/models/sourcecodemodel.cpp

@@ -15,6 +15,7 @@
 #include <QTextDocument>
 
 #include <algorithm>
+#include <iterator>
 #include <limits>
 
 #include "highlighter.hpp"
@@ -261,14 +262,15 @@ void SourceCodeModel::setSysroot(const QString& sysroot)
     m_sysroot = sysroot;
 }
 
-void SourceCodeModel::find(const QString& search, Direction direction, int offset)
+void SourceCodeModel::find(const QString& search, Direction direction, int current)
 {
     auto searchFunc = [&search](const SourceCodeLine& line) {
         return line.text.indexOf(search, 0, Qt::CaseInsensitive) != -1;
     };
 
-    const auto resultIndex = ::search(
-        m_sourceCodeLines, searchFunc, [this] { emit resultFound({}); }, direction, offset);
+    auto endReached = [this] { emit searchEndReached(); };
+
+    const int resultIndex = ::search(m_sourceCodeLines, current, direction, searchFunc, endReached);
 
     if (resultIndex >= 0) {
         emit resultFound(createIndex(resultIndex + 1, SourceCodeColumn));

src/models/sourcecodemodel.h

@@ -83,7 +83,7 @@ public slots:
     void updateHighlighting(int line);
     void setSysroot(const QString& sysroot);
 
-    void find(const QString& search, Direction direction, int offset);
+    void find(const QString& search, Direction direction, int current);
 
 private:
     QString m_sysroot;

src/resultsdisassemblypage.cpp

@@ -190,7 +190,8 @@ ResultsDisassemblyPage::ResultsDisassemblyPage(CostContextMenu* costContextMenu,
 
     auto setupSearchShortcuts = [this](QPushButton* search, QPushButton* next, QPushButton* prev, QPushButton* close,
                                        QWidget* searchWidget, QLineEdit* edit, QAbstractItemView* view,
-                                       KMessageWidget* endReached, auto* model, int additionalRows) {
+                                       KMessageWidget* endReached, auto* model, QModelIndex* searchResultIndex,
+                                       int additionalRows) {
         searchWidget->hide();
 
         auto actions = new QActionGroup(view);
@@ -205,13 +206,13 @@ ResultsDisassemblyPage::ResultsDisassemblyPage(CostContextMenu* costContextMenu,
         findAction->setShortcutContext(Qt::ShortcutContext::WidgetWithChildrenShortcut);
         view->addAction(findAction);
 
-        auto searchNext = [this, model, edit, additionalRows] {
-            const auto offset = m_currentSearchIndex.isValid() ? m_currentSearchIndex.row() - additionalRows + 1 : 0;
+        auto searchNext = [model, edit, additionalRows, searchResultIndex] {
+            const auto offset = searchResultIndex->isValid() ? searchResultIndex->row() - additionalRows : 0;
             model->find(edit->text(), Direction::Forward, offset);
         };
 
-        auto searchPrev = [this, model, edit, additionalRows] {
-            const auto offset = m_currentSearchIndex.isValid() ? m_currentSearchIndex.row() - additionalRows - 1 : 0;
+        auto searchPrev = [model, edit, additionalRows, searchResultIndex] {
+            const auto offset = searchResultIndex->isValid() ? searchResultIndex->row() - additionalRows : 0;
 
             model->find(edit->text(), Direction::Backward, offset);
         };
@@ -234,9 +235,9 @@ ResultsDisassemblyPage::ResultsDisassemblyPage(CostContextMenu* costContextMenu,
 
         connectModel(
             model,
-            [this, edit, view, colorScheme](const QModelIndex& index) {
+            [edit, view, colorScheme, searchResultIndex](const QModelIndex& index) {
                 auto palette = edit->palette();
-                m_currentSearchIndex = index;
+                *searchResultIndex = index;
                 palette.setBrush(QPalette::Text,
                                  index.isValid() ? colorScheme.foreground()
                                                  : colorScheme.foreground(KColorScheme::NegativeText));
@@ -250,10 +251,11 @@ ResultsDisassemblyPage::ResultsDisassemblyPage(CostContextMenu* costContextMenu,
     };
 
     setupSearchShortcuts(ui->searchButton, ui->nextResult, ui->prevResult, ui->closeButton, ui->searchWidget,
-                         ui->searchEdit, ui->sourceCodeView, ui->searchEndWidget, m_sourceCodeModel, 1);
+                         ui->searchEdit, ui->sourceCodeView, ui->searchEndWidget, m_sourceCodeModel,
+                         &m_currentSourceSearchIndex, 1);
     setupSearchShortcuts(ui->disasmSearchButton, ui->disasmNextButton, ui->disasmPrevButton, ui->disasmCloseButton,
                          ui->disasmSearchWidget, ui->disasmSearchEdit, ui->assemblyView, ui->disasmEndReachedWidget,
-                         m_disassemblyModel, 0);
+                         m_disassemblyModel, &m_currentDisasmSearchIndex, 0);
 
 #if KFSyntaxHighlighting_FOUND
     QStringList schemes;

src/resultsdisassemblypage.h

@@ -73,7 +73,8 @@ class ResultsDisassemblyPage : public QWidget
     // Model
     DisassemblyModel* m_disassemblyModel;
     SourceCodeModel* m_sourceCodeModel;
-    QModelIndex m_currentSearchIndex;
+    QModelIndex m_currentSourceSearchIndex;
+    QModelIndex m_currentDisasmSearchIndex;
     // Architecture
     QString m_arch;
     // Objdump binary name

tests/modeltests/CMakeLists.txt

@@ -71,3 +71,12 @@ endif()
 set_target_properties(
     tst_callgraphgenerator PROPERTIES RUNTIME_OUTPUT_DIRECTORY "${PROJECT_BINARY_DIR}/${KDE_INSTALL_BINDIR}"
 )
+
+ecm_add_test(
+    tst_search.cpp
+    LINK_LIBRARIES
+    Qt::Test
+    TEST_NAME
+    tst_search
+)
+set_target_properties(tst_search PROPERTIES RUNTIME_OUTPUT_DIRECTORY "${PROJECT_BINARY_DIR}/${KDE_INSTALL_BINDIR}")

tests/modeltests/tst_search.cpp

@@ -0,0 +1,89 @@
+/*
+    SPDX-FileCopyrightText: Lieven Hey <lieven.hey@kdab.com>
+    SPDX-FileCopyrightText: 2023 Klarälvdalens Datakonsult AB, a KDAB Group company, info@kdab.com
+
+    SPDX-License-Identifier: GPL-2.0-or-later
+*/
+
+#include <array>
+#include <QObject>
+#include <QTest>
+#include <qtest.h>
+#include <qtestcase.h>
+
+#include <iterator>
+#include <models/search.h>
+
+class TestSearch : public QObject
+{
+    Q_OBJECT
+private slots:
+    void testSearchEmpty()
+    {
+        const std::array<int, 0> testArray = {};
+
+        QCOMPARE(search_impl(
+                     testArray.begin(), testArray.end(), testArray.begin(), [](int) { return false; }, [] {}),
+                 -1);
+        QCOMPARE(search_impl(
+                     testArray.rbegin(), testArray.rend(), testArray.rbegin(), [](int) { return false; }, [] {}),
+                 -1);
+    }
+    void testSearch()
+    {
+        const std::array<int, 5> testArray = {1, 2, 3, 4, 5};
+
+        int maxOffset = testArray.size() - 1;
+        for (int offset = 0; offset < maxOffset; offset++) {
+            QCOMPARE(search_impl(
+                         testArray.begin(), testArray.end(), testArray.begin() + offset,
+                         [](int num) { return num == 2; }, [] {}),
+                     1);
+            QCOMPARE(search_impl(
+                         testArray.rbegin(), testArray.rend(), testArray.rbegin() + offset,
+                         [](int num) { return num == 2; }, [] {}),
+                     3);
+        }
+    }
+
+    void testEndReached()
+    {
+        const std::array<int, 5> testArray = {1, 2, 3, 4, 5};
+        {
+            bool endReached = false;
+            QCOMPARE(search_impl(
+                         testArray.begin(), testArray.end(), testArray.begin() + 1, [](int i) { return i == 1; },
+                         [&endReached] { endReached = true; }),
+                     0);
+            QCOMPARE(endReached, true);
+        }
+
+        {
+            bool endReached = false;
+            QCOMPARE(search_impl(
+                         testArray.rbegin(), testArray.rend(), std::make_reverse_iterator(testArray.begin() + 4 - 1),
+                         [](int i) { return i == 4; }, [&endReached] { endReached = true; }),
+                     1);
+            QCOMPARE(endReached, true);
+        }
+    }
+
+    void testWrapper()
+    {
+        const std::array<int, 5> testArray = {1, 2, 3, 4, 5};
+
+        int maxOffset = testArray.size() - 1;
+        for (int i = 0; i < maxOffset; i++) {
+            QCOMPARE(search(
+                         testArray, i, Direction::Forward, [](int i) { return i == 4; }, [] {}),
+                     3);
+            QCOMPARE(search(
+                         testArray, i, Direction::Backward, [](int i) { return i == 4; }, [] {}),
+                     3);
+        }
+    }
+};
+
+QTEST_GUILESS_MAIN(TestSearch)
+
+#include "tst_search.moc"