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>

12 files changed, 77 insertions, 37 deletions

diff --git a/src/plugins/coreplugin/locator/basefilefilter.cpp b/src/plugins/coreplugin/locator/basefilefilter.cpp
index ff6afc088d..f38b61b74e 100644
--- a/src/plugins/coreplugin/locator/basefilefilter.cpp
+++ b/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()
-{
-}
diff --git a/src/plugins/coreplugin/locator/basefilefilter.h b/src/plugins/coreplugin/locator/basefilefilter.h
index b8e95dc06f..9c8cfad82d 100644
--- a/src/plugins/coreplugin/locator/basefilefilter.h
+++ b/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; }
 
diff --git a/src/plugins/coreplugin/locator/filesystemfilter.cpp b/src/plugins/coreplugin/locator/filesystemfilter.cpp
index f3c6aa0c82..85ec1f3f84 100644
--- a/src/plugins/coreplugin/locator/filesystemfilter.cpp
+++ b/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;
diff --git a/src/plugins/coreplugin/locator/filesystemfilter.h b/src/plugins/coreplugin/locator/filesystemfilter.h
index a6f0283065..d478650332 100644
--- a/src/plugins/coreplugin/locator/filesystemfilter.h
+++ b/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
diff --git a/src/plugins/coreplugin/locator/ilocatorfilter.cpp b/src/plugins/coreplugin/locator/ilocatorfilter.cpp
index ea91197cc6..7a019ed61a 100644
--- a/src/plugins/coreplugin/locator/ilocatorfilter.cpp
+++ b/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;
diff --git a/src/plugins/coreplugin/locator/ilocatorfilter.h b/src/plugins/coreplugin/locator/ilocatorfilter.h
index 3c6e213fa0..8887973b1e 100644
--- a/src/plugins/coreplugin/locator/ilocatorfilter.h
+++ b/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. */
diff --git a/src/plugins/coreplugin/locator/locator_test.cpp b/src/plugins/coreplugin/locator/locator_test.cpp
index c9bcdb880b..cb88d0261b 100644
--- a/src/plugins/coreplugin/locator/locator_test.cpp
+++ b/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()); }
diff --git a/src/plugins/coreplugin/locator/locatorfiltersfilter.cpp b/src/plugins/coreplugin/locator/locatorfiltersfilter.cpp
index 7f99d475bc..dcb9172f83 100644
--- a/src/plugins/coreplugin/locator/locatorfiltersfilter.cpp
+++ b/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)
diff --git a/src/plugins/coreplugin/locator/locatorfiltersfilter.h b/src/plugins/coreplugin/locator/locatorfiltersfilter.h
index bfb7d2655b..5ea89f44f3 100644
--- a/src/plugins/coreplugin/locator/locatorfiltersfilter.h
+++ b/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;
 };
 
diff --git a/src/plugins/coreplugin/locator/locatorwidget.cpp b/src/plugins/coreplugin/locator/locatorwidget.cpp
index 348a91da71..91032d07a0 100644
--- a/src/plugins/coreplugin/locator/locatorwidget.cpp
+++ b/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);
 }
diff --git a/src/plugins/coreplugin/locator/opendocumentsfilter.cpp b/src/plugins/coreplugin/locator/opendocumentsfilter.cpp
index 422b9afa20..c7f8627629 100644
--- a/src/plugins/coreplugin/locator/opendocumentsfilter.cpp
+++ b/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)
diff --git a/src/plugins/coreplugin/locator/opendocumentsfilter.h b/src/plugins/coreplugin/locator/opendocumentsfilter.h
index b16ca7144b..63c4944a80 100644
--- a/src/plugins/coreplugin/locator/opendocumentsfilter.h
+++ b/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;
 };