Locator: API thread-safety improvements in BaseFileFilter

Make it possible to set or clear the file iterator while a search is
currently running.

Change-Id: I5309a8920057112375ce22e5fd24806fb6f09857
Reviewed-by: Nikolai Kosjar <nikolai.kosjar@theqtcompany.com>

src/plugins/coreplugin/locator/basefilefilter.cpp

@@ -36,19 +36,63 @@
 
 #include <QDir>
 #include <QStringMatcher>
+#include <QTimer>
 
-using namespace Core;
 using namespace Core;
 using namespace Utils;
 
-BaseFileFilter::BaseFileFilter()
-  : m_forceNewSearchList(false)
+namespace Core {
+namespace Internal {
+
+class Data
 {
+public:
+    void clear()
+    {
+        iterator.clear();
+        previousResultPaths.clear();
+        previousResultNames.clear();
+        previousEntry.clear();
+    }
+
+    QSharedPointer<BaseFileFilter::Iterator> iterator;
+    QStringList previousResultPaths;
+    QStringList previousResultNames;
+    bool forceNewSearchList;
+    QString previousEntry;
+};
+
+class BaseFileFilterPrivate
+{
+public:
+    Data m_data;
+    Data m_current;
+};
+
+} // Internal
+} // Core
+
+BaseFileFilter::BaseFileFilter()
+  : d(new Internal::BaseFileFilterPrivate)
+{
+    d->m_data.forceNewSearchList = true;
     setFileIterator(new ListIterator(QStringList()));
 }
 
 BaseFileFilter::~BaseFileFilter()
 {
+    delete d;
+}
+
+void BaseFileFilter::prepareSearch(const QString &entry)
+{
+    Q_UNUSED(entry)
+    d->m_current.iterator = d->m_data.iterator;
+    d->m_current.previousResultPaths = d->m_data.previousResultPaths;
+    d->m_current.previousResultNames = d->m_data.previousResultNames;
+    d->m_current.forceNewSearchList = d->m_data.forceNewSearchList;
+    d->m_current.previousEntry = d->m_data.previousEntry;
+    d->m_data.forceNewSearchList = false;
 }
 
 QList<LocatorFilterEntry> BaseFileFilter::matchesFor(QFutureInterface<LocatorFilterEntry> &future, const QString &origEntry)
@@ -60,37 +104,39 @@ QList<LocatorFilterEntry> BaseFileFilter::matchesFor(QFutureInterface<LocatorFil
     QStringMatcher matcher(needle, Qt::CaseInsensitive);
     const QChar asterisk = QLatin1Char('*');
     QRegExp regexp(asterisk + needle+ asterisk, Qt::CaseInsensitive, QRegExp::Wildcard);
-    if (!regexp.isValid())
+    if (!regexp.isValid()) {
+        d->m_current.clear(); // free memory
         return betterEntries;
+    }
     const QChar pathSeparator(QLatin1Char('/'));
     const bool hasPathSeparator = needle.contains(pathSeparator);
     const bool hasWildcard = needle.contains(asterisk) || needle.contains(QLatin1Char('?'));
-    const bool containsPreviousEntry = !m_previousEntry.isEmpty()
-            && needle.contains(m_previousEntry);
-    const bool pathSeparatorAdded = !m_previousEntry.contains(pathSeparator)
+    const bool containsPreviousEntry = !d->m_current.previousEntry.isEmpty()
+            && needle.contains(d->m_current.previousEntry);
+    const bool pathSeparatorAdded = !d->m_current.previousEntry.contains(pathSeparator)
             && needle.contains(pathSeparator);
-    const bool searchInPreviousResults = !m_forceNewSearchList && containsPreviousEntry
+    const bool searchInPreviousResults = !d->m_current.forceNewSearchList && containsPreviousEntry
             && !pathSeparatorAdded;
-    QSharedPointer<Iterator> iterator;
     if (searchInPreviousResults)
-        iterator.reset(new ListIterator(m_previousResultPaths, m_previousResultNames));
-    else
-        iterator = fileIterator();
+        d->m_current.iterator.reset(new ListIterator(d->m_current.previousResultPaths,
+                                                     d->m_current.previousResultNames));
 
-    QTC_ASSERT(iterator.data(), return QList<LocatorFilterEntry>());
-    m_previousResultPaths.clear();
-    m_previousResultNames.clear();
-    m_forceNewSearchList = false;
-    m_previousEntry = needle;
+    QTC_ASSERT(d->m_current.iterator.data(), return QList<LocatorFilterEntry>());
+    d->m_current.previousResultPaths.clear();
+    d->m_current.previousResultNames.clear();
+    d->m_current.previousEntry = needle;
     const Qt::CaseSensitivity caseSensitivityForPrefix = caseSensitivity(needle);
-    iterator->toFront();
-    while (iterator->hasNext()) {
-        if (future.isCanceled())
+    d->m_current.iterator->toFront();
+    bool canceled = false;
+    while (d->m_current.iterator->hasNext()) {
+        if (future.isCanceled()) {
+            canceled = true;
             break;
+        }
 
-        iterator->next();
-        QString path = iterator->filePath();
-        QString name = iterator->fileName();
+        d->m_current.iterator->next();
+        QString path = d->m_current.iterator->filePath();
+        QString name = d->m_current.iterator->fileName();
         QString matchText = hasPathSeparator ? path : name;
         if ((hasWildcard && regexp.exactMatch(matchText))
                 || (!hasWildcard && matcher.indexIn(matchText) != -1)) {
@@ -102,12 +148,21 @@ QList<LocatorFilterEntry> BaseFileFilter::matchesFor(QFutureInterface<LocatorFil
                 betterEntries.append(entry);
             else
                 goodEntries.append(entry);
-            m_previousResultPaths.append(path);
-            m_previousResultNames.append(name);
+            d->m_current.previousResultPaths.append(path);
+            d->m_current.previousResultNames.append(name);
         }
     }
 
     betterEntries.append(goodEntries);
+    if (canceled) {
+        // we keep the old list of previous search results if this search was canceled
+        // so a later search without foreNewSearchList will use that previous list instead of an
+        // incomplete list of a canceled search
+        d->m_current.clear(); // free memory
+    } else {
+        d->m_current.iterator.clear();
+        QTimer::singleShot(0, this, SLOT(updatePreviousResultData()));
+    }
     return betterEntries;
 }
 
@@ -117,14 +172,6 @@ void BaseFileFilter::accept(LocatorFilterEntry selection) const
                               EditorManager::CanContainLineNumber);
 }
 
-void BaseFileFilter::invalidateCachedResults()
-{
-    m_forceNewSearchList = true;
-    m_previousEntry.clear();
-    m_previousResultPaths.clear();
-    m_previousResultNames.clear();
-}
-
 /*!
    Takes ownership of the \a iterator. The previously set iterator might not be deleted until
    a currently running search is finished.
@@ -132,13 +179,24 @@ void BaseFileFilter::invalidateCachedResults()
 
 void BaseFileFilter::setFileIterator(BaseFileFilter::Iterator *iterator)
 {
-    invalidateCachedResults();
-    m_iterator.reset(iterator);
+    d->m_data.clear();
+    d->m_data.forceNewSearchList = true;
+    d->m_data.iterator.reset(iterator);
 }
 
 QSharedPointer<BaseFileFilter::Iterator> BaseFileFilter::fileIterator()
 {
-    return m_iterator;
+    return d->m_data.iterator;
+}
+
+void BaseFileFilter::updatePreviousResultData()
+{
+    if (d->m_data.forceNewSearchList) // in the meantime the iterator was reset / cache invalidated
+        return; // do not update with the new result list etc
+    d->m_data.previousEntry = d->m_current.previousEntry;
+    d->m_data.previousResultPaths = d->m_current.previousResultPaths;
+    d->m_data.previousResultNames = d->m_current.previousResultNames;
+    // forceNewSearchList was already reset in prepareSearch
 }
 
 BaseFileFilter::ListIterator::ListIterator(const QStringList &filePaths)

src/plugins/coreplugin/locator/basefilefilter.h

@@ -38,6 +38,8 @@
 
 namespace Core {
 
+namespace Internal { class BaseFileFilterPrivate; }
+
 class CORE_EXPORT BaseFileFilter : public ILocatorFilter
 {
     Q_OBJECT
@@ -73,21 +75,19 @@ public:
 
     BaseFileFilter();
     ~BaseFileFilter();
+    void prepareSearch(const QString &entry);
     QList<LocatorFilterEntry> matchesFor(QFutureInterface<LocatorFilterEntry> &future, const QString &entry);
     void accept(LocatorFilterEntry selection) const;
 
 protected:
-    void invalidateCachedResults();
-
     void setFileIterator(Iterator *iterator);
     QSharedPointer<Iterator> fileIterator();
 
+private slots:
+    void updatePreviousResultData();
+
 private:
-    QSharedPointer<Iterator> m_iterator;
-    QStringList m_previousResultPaths;
-    QStringList m_previousResultNames;
-    bool m_forceNewSearchList;
-    QString m_previousEntry;
+    Internal::BaseFileFilterPrivate *d;
 };
 
 } // namespace Core

src/plugins/coreplugin/locator/directoryfilter.cpp

@@ -30,9 +30,11 @@
 
 #include "directoryfilter.h"
 
-#include <QFileDialog>
 #include <utils/filesearch.h>
 
+#include <QFileDialog>
+#include <QTimer>
+
 using namespace Core;
 using namespace Core::Internal;
 
@@ -83,7 +85,7 @@ bool DirectoryFilter::restoreState(const QByteArray &state)
     setShortcutString(shortcut);
     setIncludedByDefault(defaultFilter);
 
-    setFileIterator(new BaseFileFilter::ListIterator(m_files));
+    updateFileIterator();
     return true;
 }
 
@@ -169,6 +171,11 @@ void DirectoryFilter::updateOptionButtons()
     m_ui.removeButton->setEnabled(haveSelectedItem);
 }
 
+void DirectoryFilter::updateFileIterator()
+{
+    setFileIterator(new BaseFileFilter::ListIterator(m_files));
+}
+
 void DirectoryFilter::refresh(QFutureInterface<void> &future)
 {
     QStringList directories;
@@ -176,7 +183,7 @@ void DirectoryFilter::refresh(QFutureInterface<void> &future)
         QMutexLocker locker(&m_lock);
         if (m_directories.count() < 1) {
             m_files.clear();
-            setFileIterator(new BaseFileFilter::ListIterator(m_files));
+            QTimer::singleShot(0, this, SLOT(updateFileIterator()));
             future.setProgressRange(0, 1);
             future.setProgressValueAndText(1, tr("%1 filter update: 0 files").arg(displayName()));
             return;
@@ -198,7 +205,7 @@ void DirectoryFilter::refresh(QFutureInterface<void> &future)
     if (!future.isCanceled()) {
         QMutexLocker locker(&m_lock);
         m_files = filesFound;
-        setFileIterator(new BaseFileFilter::ListIterator(m_files));
+        QTimer::singleShot(0, this, SLOT(updateFileIterator()));
         future.setProgressValue(it.maxProgress());
     } else {
         future.setProgressValueAndText(it.currentProgress(), tr("%1 filter update: canceled").arg(displayName()));

src/plugins/coreplugin/locator/directoryfilter.h

@@ -58,6 +58,7 @@ private slots:
     void editDirectory();
     void removeDirectory();
     void updateOptionButtons();
+    void updateFileIterator();
 
 private:
     QStringList m_directories;

src/plugins/coreplugin/locator/locatorfiltertest.cpp

@@ -50,6 +50,7 @@ QList<LocatorFilterEntry> BasicLocatorFilterTest::matchesFor(const QString &sear
 {
     doBeforeLocatorRun();
     const QList<ILocatorFilter *> filters = QList<ILocatorFilter *>() << m_filter;
+    m_filter->prepareSearch(searchText);
     QFuture<LocatorFilterEntry> locatorSearch = QtConcurrent::run(Core::Internal::runSearch,
                                                            filters, searchText);
     locatorSearch.waitForFinished();

src/plugins/projectexplorer/allprojectsfilter.cpp

@@ -36,12 +36,13 @@
 #include <utils/algorithm.h>
 
 #include <QMutexLocker>
+#include <QTimer>
 
 using namespace Core;
 using namespace ProjectExplorer;
 using namespace ProjectExplorer::Internal;
 
-AllProjectsFilter::AllProjectsFilter() : m_filesUpToDate(false)
+AllProjectsFilter::AllProjectsFilter()
 {
     setId("Files in any project");
     setDisplayName(tr("Files in Any Project"));
@@ -54,27 +55,24 @@ AllProjectsFilter::AllProjectsFilter() : m_filesUpToDate(false)
 
 void AllProjectsFilter::markFilesAsOutOfDate()
 {
-    QMutexLocker lock(&m_mutex); Q_UNUSED(lock)
-    m_filesUpToDate = false;
-    invalidateCachedResults();
+    setFileIterator(0);
 }
 
 void AllProjectsFilter::prepareSearch(const QString &entry)
 {
     Q_UNUSED(entry)
-    QMutexLocker lock(&m_mutex); Q_UNUSED(lock)
-    if (m_filesUpToDate)
-        return;
+    if (!fileIterator()) {
         QStringList paths;
         foreach (Project *project, SessionManager::projects())
             paths.append(project->files(Project::AllFiles));
         Utils::sort(paths);
         setFileIterator(new BaseFileFilter::ListIterator(paths));
-    m_filesUpToDate = true;
+    }
+    BaseFileFilter::prepareSearch(entry);
 }
 
 void AllProjectsFilter::refresh(QFutureInterface<void> &future)
 {
     Q_UNUSED(future)
-    markFilesAsOutOfDate();
+    QTimer::singleShot(0, this, SLOT(markFilesAsOutOfDate()));
 }

src/plugins/projectexplorer/allprojectsfilter.h

@@ -33,7 +33,6 @@
 
 #include <coreplugin/locator/basefilefilter.h>
 
-#include <QMutex>
 #include <QFutureInterface>
 
 namespace ProjectExplorer {
@@ -50,10 +49,6 @@ public:
 
 private slots:
     void markFilesAsOutOfDate();
-
-private:
-    bool m_filesUpToDate;
-    QMutex m_mutex;
 };
 
 } // namespace Internal

src/plugins/projectexplorer/currentprojectfilter.cpp

@@ -35,13 +35,14 @@
 #include <utils/algorithm.h>
 
 #include <QMutexLocker>
+#include <QTimer>
 
 using namespace Core;
 using namespace ProjectExplorer;
 using namespace ProjectExplorer::Internal;
 
 CurrentProjectFilter::CurrentProjectFilter()
-  : BaseFileFilter(), m_project(0), m_filesUpToDate(false)
+  : BaseFileFilter(), m_project(0)
 {
     setId("Files in current project");
     setDisplayName(tr("Files in Current Project"));
@@ -54,18 +55,13 @@ CurrentProjectFilter::CurrentProjectFilter()
 
 void CurrentProjectFilter::markFilesAsOutOfDate()
 {
-    QMutexLocker lock(&m_filesUpToDateMutex); Q_UNUSED(lock)
-    m_filesUpToDate = false;
-    invalidateCachedResults();
+    setFileIterator(0);
 }
 
 void CurrentProjectFilter::prepareSearch(const QString &entry)
 {
     Q_UNUSED(entry)
-    QMutexLocker lock(&m_filesUpToDateMutex); Q_UNUSED(lock)
-    if (m_filesUpToDate)
-        return;
-    m_filesUpToDate = true;
+    if (!fileIterator()) {
         QStringList paths;
         if (m_project) {
             paths = m_project->files(Project::AllFiles);
@@ -73,6 +69,8 @@ void CurrentProjectFilter::prepareSearch(const QString &entry)
         }
         setFileIterator(new BaseFileFilter::ListIterator(paths));
     }
+    BaseFileFilter::prepareSearch(entry);
+}
 
 void CurrentProjectFilter::currentProjectChanged(ProjectExplorer::Project *project)
 {
@@ -91,5 +89,5 @@ void CurrentProjectFilter::currentProjectChanged(ProjectExplorer::Project *proje
 void CurrentProjectFilter::refresh(QFutureInterface<void> &future)
 {
     Q_UNUSED(future)
-    markFilesAsOutOfDate();
+    QTimer::singleShot(0, this, SLOT(markFilesAsOutOfDate()));
 }

src/plugins/projectexplorer/currentprojectfilter.h

@@ -58,8 +58,6 @@ private slots:
 
 private:
     QPointer<Project> m_project;
-    bool m_filesUpToDate;
-    QMutex m_filesUpToDateMutex;
 };
 
 } // namespace Internal