Locator filters: Fix various thread-safety issues

Introduces a "prepareSearch" method for locator filters that is called on
the UI thread prior to the threaded matching.
Fix various small thread-safety issues in the various filters.

Change-Id: If5ae7d205e126d367420936a93f8d9a84496edb8
Reviewed-by: Daniel Teske <daniel.teske@digia.com>

src/plugins/coreplugin/locator/basefilefilter.cpp

@@ -46,7 +46,6 @@ BaseFileFilter::BaseFileFilter()
 
 QList<LocatorFilterEntry> BaseFileFilter::matchesFor(QFutureInterface<Core::LocatorFilterEntry> &future, const QString &origEntry)
 {
-    updateFiles();
     QList<LocatorFilterEntry> betterEntries;
     QList<LocatorFilterEntry> goodEntries;
     QString needle = trimWildcards(QDir::fromNativeSeparators(origEntry));
@@ -120,7 +119,3 @@ void BaseFileFilter::generateFileNames()
     }
     m_forceNewSearchList = true;
 }
-
-void BaseFileFilter::updateFiles()
-{
-}

src/plugins/coreplugin/locator/basefilefilter.h

@@ -46,10 +46,10 @@ public:
     void accept(Core::LocatorFilterEntry selection) const;
 
 protected:
-    // runs in non-UI thread
-    virtual void updateFiles();
+    /* Generates the file names from the list of file paths in m_files. */
     void generateFileNames();
 
+    /* Subclasses should update the file list latest in their prepareSearch method. */
     inline QStringList &files() { return m_files; }
     inline const QStringList &files() const { return m_files; }
 

src/plugins/coreplugin/locator/filesystemfilter.cpp

@@ -63,6 +63,18 @@ FileSystemFilter::FileSystemFilter(LocatorWidget *locatorWidget)
     setIncludedByDefault(false);
 }
 
+void FileSystemFilter::prepareSearch(const QString &entry)
+{
+    Q_UNUSED(entry)
+    IDocument *document= EditorManager::currentDocument();
+    if (document && !document->filePath().isEmpty()) {
+        QFileInfo info(document->filePath());
+        m_currentDocumentDirectory = info.absolutePath() + QLatin1Char('/');
+    } else {
+        m_currentDocumentDirectory.clear();
+    }
+}
+
 QList<LocatorFilterEntry> FileSystemFilter::matchesFor(QFutureInterface<Core::LocatorFilterEntry> &future, const QString &entry)
 {
     QList<LocatorFilterEntry> goodEntries;
@@ -72,15 +84,10 @@ QList<LocatorFilterEntry> FileSystemFilter::matchesFor(QFutureInterface<Core::Lo
     QString directory = entryInfo.path();
     QString filePath = entryInfo.filePath();
     if (entryInfo.isRelative()) {
-        if (filePath.startsWith(QLatin1String("~/"))) {
+        if (filePath.startsWith(QLatin1String("~/")))
             directory.replace(0, 1, QDir::homePath());
-        } else {
-            IDocument *document= EditorManager::currentDocument();
-            if (document && !document->filePath().isEmpty()) {
-                QFileInfo info(document->filePath());
-                directory.prepend(info.absolutePath() + QLatin1Char('/'));
-            }
-        }
+        else if (!m_currentDocumentDirectory.isEmpty())
+            directory.prepend(m_currentDocumentDirectory);
     }
     QDir dirInfo(directory);
     QDir::Filters dirFilter = QDir::Dirs|QDir::Drives|QDir::NoDot;

src/plugins/coreplugin/locator/filesystemfilter.h

@@ -49,6 +49,7 @@ class FileSystemFilter : public Core::ILocatorFilter
 
 public:
     explicit FileSystemFilter(LocatorWidget *locatorWidget);
+    void prepareSearch(const QString &entry);
     QList<Core::LocatorFilterEntry> matchesFor(QFutureInterface<Core::LocatorFilterEntry> &future, const QString &entry);
     void accept(Core::LocatorFilterEntry selection) const;
     QByteArray saveState() const;
@@ -59,6 +60,7 @@ public:
 private:
     LocatorWidget *m_locatorWidget;
     bool m_includeHidden;
+    QString m_currentDocumentDirectory;
 };
 
 } // namespace Internal

src/plugins/coreplugin/locator/ilocatorfilter.cpp

@@ -53,6 +53,11 @@ QString ILocatorFilter::shortcutString() const
     return m_shortcut;
 }
 
+void ILocatorFilter::prepareSearch(const QString &entry)
+{
+    Q_UNUSED(entry)
+}
+
 void ILocatorFilter::setShortcutString(const QString &shortcut)
 {
     m_shortcut = shortcut;

src/plugins/coreplugin/locator/ilocatorfilter.h

@@ -100,7 +100,13 @@ public:
     /* String to type to use this filter exclusively. */
     QString shortcutString() const;
 
-    /* List of matches for the given user entry. */
+    /* Called on the main thread before matchesFor is called in a separate thread.
+       Can be used to perform actions that need to be done in the main thread before actually
+       running the search. */
+    virtual void prepareSearch(const QString &entry);
+
+    /* List of matches for the given user entry. This runs in a separate thread, but only
+       a single thread at a time. */
     virtual QList<LocatorFilterEntry> matchesFor(QFutureInterface<Core::LocatorFilterEntry> &future, const QString &entry) = 0;
 
     /* User has selected the given entry that belongs to this filter. */

src/plugins/coreplugin/locator/locator_test.cpp

@@ -57,9 +57,6 @@ public:
     }
 
     void refresh(QFutureInterface<void> &) {}
-
-protected:
-    void updateFiles() {}
 };
 
 inline QString _(const QByteArray &ba) { return QString::fromLatin1(ba, ba.size()); }

src/plugins/coreplugin/locator/locatorfiltersfilter.cpp

@@ -52,11 +52,12 @@ LocatorFiltersFilter::LocatorFiltersFilter(Locator *plugin,
     setConfigurable(false);
 }
 
-QList<LocatorFilterEntry> LocatorFiltersFilter::matchesFor(QFutureInterface<Core::LocatorFilterEntry> &future, const QString &entry)
+void LocatorFiltersFilter::prepareSearch(const QString &entry)
 {
-    QList<LocatorFilterEntry> entries;
+    m_filterShortcutStrings.clear();
+    m_filterDisplayNames.clear();
     if (!entry.isEmpty())
-        return entries;
+        return;
 
     QMap<QString, ILocatorFilter *> uniqueFilters;
     foreach (ILocatorFilter *filter, m_plugin->filters()) {
@@ -65,27 +66,36 @@ QList<LocatorFilterEntry> LocatorFiltersFilter::matchesFor(QFutureInterface<Core
     }
 
     foreach (ILocatorFilter *filter, uniqueFilters) {
-        if (future.isCanceled())
-            break;
         if (!filter->shortcutString().isEmpty() && !filter->isHidden() && filter->isEnabled()) {
-            LocatorFilterEntry filterEntry(this,
-                                    filter->shortcutString(),
-                                    QVariant::fromValue(filter),
-                                    m_icon);
-            filterEntry.extraInfo = filter->displayName();
-            entries.append(filterEntry);
+            m_filterShortcutStrings.append(filter->shortcutString());
+            m_filterDisplayNames.append(filter->displayName());
         }
     }
+}
 
+QList<LocatorFilterEntry> LocatorFiltersFilter::matchesFor(QFutureInterface<Core::LocatorFilterEntry> &future, const QString &entry)
+{
+    Q_UNUSED(entry) // search is already done in the GUI thread in prepareSearch
+    QList<LocatorFilterEntry> entries;
+    for (int i = 0; i < m_filterShortcutStrings.size(); ++i) {
+        if (future.isCanceled())
+            break;
+        LocatorFilterEntry filterEntry(this,
+                                m_filterShortcutStrings.at(i),
+                                m_filterShortcutStrings.at(i),
+                                m_icon);
+        filterEntry.extraInfo = m_filterDisplayNames.at(i);
+        entries.append(filterEntry);
+    }
     return entries;
 }
 
 void LocatorFiltersFilter::accept(LocatorFilterEntry selection) const
 {
-    ILocatorFilter *filter = selection.internalData.value<ILocatorFilter *>();
-    if (filter)
-        m_locatorWidget->show(filter->shortcutString() + QLatin1Char(' '),
-                           filter->shortcutString().length() + 1);
+    const QString shortcutString = selection.internalData.toString();
+    if (!shortcutString.isEmpty())
+        m_locatorWidget->show(shortcutString + QLatin1Char(' '),
+                              shortcutString.length() + 1);
 }
 
 void LocatorFiltersFilter::refresh(QFutureInterface<void> &future)

src/plugins/coreplugin/locator/locatorfiltersfilter.h

@@ -53,6 +53,7 @@ public:
                          LocatorWidget *locatorWidget);
 
     // ILocatorFilter
+    void prepareSearch(const QString &entry);
     QList<LocatorFilterEntry> matchesFor(QFutureInterface<Core::LocatorFilterEntry> &future, const QString &entry);
     void accept(LocatorFilterEntry selection) const;
     void refresh(QFutureInterface<void> &future);
@@ -60,6 +61,8 @@ public:
 private:
     Locator *m_plugin;
     LocatorWidget *m_locatorWidget;
+    QStringList m_filterShortcutStrings;
+    QStringList m_filterDisplayNames;
     QIcon m_icon;
 };
 

src/plugins/coreplugin/locator/locatorwidget.cpp

@@ -491,6 +491,9 @@ void LocatorWidget::updateCompletionList(const QString &text)
 
     QString searchText;
     const QList<ILocatorFilter *> filters = filtersFor(text, searchText);
+
+    foreach (ILocatorFilter *filter, filters)
+        filter->prepareSearch(searchText);
     QFuture<LocatorFilterEntry> future = QtConcurrent::run(runSearch, filters, searchText);
     m_entriesWatcher->setFuture(future);
 }

src/plugins/coreplugin/locator/opendocumentsfilter.cpp

@@ -34,6 +34,7 @@
 #include <utils/fileutils.h>
 
 #include <QFileInfo>
+#include <QMutexLocker>
 
 using namespace Core;
 using namespace Core;
@@ -67,7 +68,7 @@ QList<LocatorFilterEntry> OpenDocumentsFilter::matchesFor(QFutureInterface<Core:
     if (!regexp.isValid())
         return goodEntries;
     const Qt::CaseSensitivity caseSensitivityForPrefix = caseSensitivity(entry);
-    foreach (const DocumentModel::Entry &editorEntry, m_editors) {
+    foreach (const DocumentModel::Entry &editorEntry, editors()) {
         if (future.isCanceled())
             break;
         QString fileName = editorEntry.fileName();
@@ -90,6 +91,7 @@ QList<LocatorFilterEntry> OpenDocumentsFilter::matchesFor(QFutureInterface<Core:
 
 void OpenDocumentsFilter::refreshInternally()
 {
+    QMutexLocker lock(&m_mutex); Q_UNUSED(lock)
     m_editors.clear();
     foreach (DocumentModel::Entry *e, DocumentModel::entries()) {
         DocumentModel::Entry entry;
@@ -101,6 +103,12 @@ void OpenDocumentsFilter::refreshInternally()
     }
 }
 
+QList<DocumentModel::Entry> OpenDocumentsFilter::editors() const
+{
+    QMutexLocker lock(&m_mutex); Q_UNUSED(lock)
+    return m_editors;
+}
+
 void OpenDocumentsFilter::refresh(QFutureInterface<void> &future)
 {
     Q_UNUSED(future)

src/plugins/coreplugin/locator/opendocumentsfilter.h

@@ -34,9 +34,10 @@
 
 #include <coreplugin/editormanager/documentmodel.h>
 
-#include <QString>
-#include <QList>
 #include <QFutureInterface>
+#include <QList>
+#include <QMutex>
+#include <QString>
 
 namespace Core {
 namespace Internal {
@@ -55,6 +56,9 @@ public slots:
     void refreshInternally();
 
 private:
+    QList<Core::DocumentModel::Entry> editors() const;
+
+    mutable QMutex m_mutex;
     QList<Core::DocumentModel::Entry> m_editors;
 };
 

src/plugins/help/helpindexfilter.cpp

@@ -59,16 +59,19 @@ HelpIndexFilter::~HelpIndexFilter()
 {
 }
 
+void HelpIndexFilter::prepareSearch(const QString &entry)
+{
+    if (entry.length() < 2)
+        m_keywords = Core::HelpManager::findKeywords(entry, caseSensitivity(entry), 200);
+    else
+        m_keywords = Core::HelpManager::findKeywords(entry, caseSensitivity(entry));
+}
+
 QList<LocatorFilterEntry> HelpIndexFilter::matchesFor(QFutureInterface<LocatorFilterEntry> &future, const QString &entry)
 {
-    QStringList keywords;
-    if (entry.length() < 2)
-        keywords = Core::HelpManager::findKeywords(entry, caseSensitivity(entry), 200);
-    else
-        keywords = Core::HelpManager::findKeywords(entry, caseSensitivity(entry));
-
+    Q_UNUSED(entry) // search is already done in the GUI thread in prepareSearch
     QList<LocatorFilterEntry> entries;
-    foreach (const QString &keyword, keywords) {
+    foreach (const QString &keyword, m_keywords) {
         if (future.isCanceled())
             break;
         entries.append(LocatorFilterEntry(this, keyword, QVariant(), m_icon));

src/plugins/help/helpindexfilter.h

@@ -46,6 +46,7 @@ public:
     ~HelpIndexFilter();
 
     // ILocatorFilter
+    void prepareSearch(const QString &entry);
     QList<Core::LocatorFilterEntry> matchesFor(QFutureInterface<Core::LocatorFilterEntry> &future, const QString &entry);
     void accept(Core::LocatorFilterEntry selection) const;
     void refresh(QFutureInterface<void> &future);
@@ -55,6 +56,7 @@ signals:
 
 private:
     QIcon m_icon;
+    QStringList m_keywords;
 };
 
 } // namespace Internal

src/plugins/help/remotehelpfilter.cpp

@@ -29,6 +29,7 @@
 
 #include "remotehelpfilter.h"
 
+#include <QMutexLocker>
 #include <QUrl>
 
 namespace Help {
@@ -99,7 +100,7 @@ RemoteHelpFilter::~RemoteHelpFilter()
 QList<Core::LocatorFilterEntry> RemoteHelpFilter::matchesFor(QFutureInterface<Core::LocatorFilterEntry> &future, const QString &pattern)
 {
     QList<Core::LocatorFilterEntry> entries;
-    foreach (const QString &url, m_remoteUrls) {
+    foreach (const QString &url, remoteUrls()) {
         if (future.isCanceled())
             break;
 
@@ -156,6 +157,7 @@ bool RemoteHelpFilter::openConfigDialog(QWidget *parent, bool &needsRefresh)
     Q_UNUSED(needsRefresh)
     RemoteFilterOptions optionsDialog(this, parent);
     if (optionsDialog.exec() == QDialog::Accepted) {
+        QMutexLocker lock(&m_mutex); Q_UNUSED(lock)
         m_remoteUrls.clear();
         setIncludedByDefault(!optionsDialog.m_ui.limitCheck->isChecked());
         setShortcutString(optionsDialog.m_ui.shortcutEdit->text().trimmed());
@@ -166,5 +168,11 @@ bool RemoteHelpFilter::openConfigDialog(QWidget *parent, bool &needsRefresh)
     return true;
 }
 
+QStringList RemoteHelpFilter::remoteUrls() const
+{
+    QMutexLocker lock(&m_mutex); Q_UNUSED(lock)
+    return m_remoteUrls;
+}
+
     } // namespace Internal
 } // namespace Help

src/plugins/help/remotehelpfilter.h

@@ -35,6 +35,7 @@
 #include <coreplugin/locator/ilocatorfilter.h>
 
 #include <QIcon>
+#include <QMutex>
 
 namespace Help {
     namespace Internal {
@@ -54,7 +55,7 @@ public:
     bool restoreState(const QByteArray &state);
     bool openConfigDialog(QWidget *parent, bool &needsRefresh);
 
-    QStringList remoteUrls() const { return m_remoteUrls; }
+    QStringList remoteUrls() const;
 
 signals:
     void linkActivated(const QUrl &url) const;
@@ -62,6 +63,7 @@ signals:
 private:
     QIcon m_icon;
     QStringList m_remoteUrls;
+    mutable QMutex m_mutex;
 };
 
 class RemoteFilterOptions : public QDialog

src/plugins/projectexplorer/allprojectsfilter.cpp

@@ -34,6 +34,8 @@
 
 #include <utils/algorithm.h>
 
+#include <QMutexLocker>
+
 using namespace Core;
 using namespace ProjectExplorer;
 using namespace ProjectExplorer::Internal;
@@ -52,11 +54,14 @@ AllProjectsFilter::AllProjectsFilter() : m_filesUpToDate(false)
 
 void AllProjectsFilter::markFilesAsOutOfDate()
 {
+    QMutexLocker lock(&m_mutex); Q_UNUSED(lock)
     m_filesUpToDate = false;
 }
 
-void AllProjectsFilter::updateFilesImpl()
+void AllProjectsFilter::prepareSearch(const QString &entry)
 {
+    Q_UNUSED(entry)
+    QMutexLocker lock(&m_mutex); Q_UNUSED(lock)
     if (m_filesUpToDate)
         return;
     files().clear();
@@ -67,13 +72,8 @@ void AllProjectsFilter::updateFilesImpl()
     m_filesUpToDate = true;
 }
 
-void AllProjectsFilter::updateFiles()
-{
-    QMetaObject::invokeMethod(this, "updateFilesImpl", Qt::BlockingQueuedConnection);
-}
-
 void AllProjectsFilter::refresh(QFutureInterface<void> &future)
 {
     Q_UNUSED(future)
-    QMetaObject::invokeMethod(this, "markFilesAsOutOfDate", Qt::BlockingQueuedConnection);
+    markFilesAsOutOfDate();
 }

src/plugins/projectexplorer/allprojectsfilter.h

@@ -32,6 +32,7 @@
 
 #include <coreplugin/locator/basefilefilter.h>
 
+#include <QMutex>
 #include <QFutureInterface>
 
 namespace ProjectExplorer {
@@ -44,16 +45,14 @@ class AllProjectsFilter : public Core::BaseFileFilter
 public:
     AllProjectsFilter();
     void refresh(QFutureInterface<void> &future);
-
-protected:
-    void updateFiles();
+    void prepareSearch(const QString &entry);
 
 private slots:
     void markFilesAsOutOfDate();
-    void updateFilesImpl();
 
 private:
     bool m_filesUpToDate;
+    QMutex m_mutex;
 };
 
 } // namespace Internal

src/plugins/projectexplorer/currentprojectfilter.cpp

@@ -33,7 +33,7 @@
 
 #include <utils/algorithm.h>
 
-#include <QDebug>
+#include <QMutexLocker>
 
 using namespace Core;
 using namespace ProjectExplorer;
@@ -54,25 +54,23 @@ CurrentProjectFilter::CurrentProjectFilter()
 
 void CurrentProjectFilter::markFilesAsOutOfDate()
 {
+    QMutexLocker lock(&m_filesUpToDateMutex); Q_UNUSED(lock)
     m_filesUpToDate = false;
 }
 
-void CurrentProjectFilter::updateFilesImpl()
+void CurrentProjectFilter::prepareSearch(const QString &entry)
 {
+    Q_UNUSED(entry)
+    QMutexLocker lock(&m_filesUpToDateMutex); Q_UNUSED(lock)
     if (m_filesUpToDate)
         return;
     files().clear();
+    m_filesUpToDate = true;
     if (!m_project)
         return;
     files() = m_project->files(Project::AllFiles);
     Utils::sort(files());
     generateFileNames();
-    m_filesUpToDate = true;
-}
-
-void CurrentProjectFilter::updateFiles()
-{
-    QMetaObject::invokeMethod(this, "updateFilesImpl", Qt::BlockingQueuedConnection);
 }
 
 void CurrentProjectFilter::currentProjectChanged(ProjectExplorer::Project *project)
@@ -92,5 +90,5 @@ void CurrentProjectFilter::currentProjectChanged(ProjectExplorer::Project *proje
 void CurrentProjectFilter::refresh(QFutureInterface<void> &future)
 {
     Q_UNUSED(future)
-    QMetaObject::invokeMethod(this, "markFilesAsOutOfDate", Qt::BlockingQueuedConnection);
+    markFilesAsOutOfDate();
 }

src/plugins/projectexplorer/currentprojectfilter.h

@@ -32,6 +32,7 @@
 
 #include <coreplugin/locator/basefilefilter.h>
 
+#include <QMutex>
 #include <QFutureInterface>
 
 namespace ProjectExplorer {
@@ -47,18 +48,16 @@ class CurrentProjectFilter : public Core::BaseFileFilter
 public:
     CurrentProjectFilter();
     void refresh(QFutureInterface<void> &future);
-
-protected:
-    void updateFiles();
+    void prepareSearch(const QString &entry);
 
 private slots:
     void currentProjectChanged(ProjectExplorer::Project *project);
     void markFilesAsOutOfDate();
-    void updateFilesImpl();
 
 private:
     Project *m_project;
     bool m_filesUpToDate;
+    QMutex m_filesUpToDateMutex;
 };
 
 } // namespace Internal

src/plugins/texteditor/linenumberfilter.cpp

@@ -56,6 +56,12 @@ LineNumberFilter::LineNumberFilter(QObject *parent)
     setIncludedByDefault(true);
 }
 
+void LineNumberFilter::prepareSearch(const QString &entry)
+{
+    Q_UNUSED(entry)
+    m_hasCurrentEditor = EditorManager::currentEditor() != 0;
+}
+
 QList<LocatorFilterEntry> LineNumberFilter::matchesFor(QFutureInterface<Core::LocatorFilterEntry> &, const QString &entry)
 {
     QList<LocatorFilterEntry> value;
@@ -70,7 +76,7 @@ QList<LocatorFilterEntry> LineNumberFilter::matchesFor(QFutureInterface<Core::Lo
         column = lineAndColumn.at(1).toInt(&ok);
     if (!ok)
         return value;
-    if (EditorManager::currentEditor() && (line > 0 || column > 0)) {
+    if (m_hasCurrentEditor && (line > 0 || column > 0)) {
         LineColumn data;
         data.first = line;
         data.second = column - 1;  // column API is 0-based

src/plugins/texteditor/linenumberfilter.h

@@ -48,9 +48,13 @@ class LineNumberFilter : public Core::ILocatorFilter
 public:
     explicit LineNumberFilter(QObject *parent = 0);
 
+    void prepareSearch(const QString &entry);
     QList<Core::LocatorFilterEntry> matchesFor(QFutureInterface<Core::LocatorFilterEntry> &future, const QString &entry);
     void accept(Core::LocatorFilterEntry selection) const;
     void refresh(QFutureInterface<void> &) {}
+
+private:
+    bool m_hasCurrentEditor;
 };
 
 } // namespace Internal