ClearCase: Fix threading issues

* Menu parameters were set (and enabled/disabled) from non-GUI thread
* Access to statusMap was not thread-safe

Change-Id: I029cb7090af87903c599bb3fcc1cba93e196d45e
Reviewed-by: Tobias Hunger <tobias.hunger@nokia.com>

src/plugins/clearcase/clearcase.pro

@@ -17,6 +17,7 @@ HEADERS += activityselector.h \
     clearcasesettings.h \
     clearcasesubmiteditor.h \
     clearcasesubmiteditorwidget.h \
+    clearcasesync.h \
     settingspage.h \
     versionselector.h
 
@@ -29,6 +30,7 @@ SOURCES += activityselector.cpp \
     clearcasesettings.cpp \
     clearcasesubmiteditor.cpp \
     clearcasesubmiteditorwidget.cpp \
+    clearcasesync.cpp \
     settingspage.cpp \
     versionselector.cpp
 

src/plugins/clearcase/clearcase.qbs

@@ -47,6 +47,8 @@ QtcPlugin {
         "clearcasesubmiteditor.h",
         "clearcasesubmiteditorwidget.cpp",
         "clearcasesubmiteditorwidget.h",
+        "clearcasesync.cpp",
+        "clearcasesync.h",
         "settingspage.cpp",
         "settingspage.h",
         "settingspage.ui",

src/plugins/clearcase/clearcaseplugin.cpp

@@ -38,6 +38,7 @@
 #include "clearcaseplugin.h"
 #include "clearcasesubmiteditor.h"
 #include "clearcasesubmiteditorwidget.h"
+#include "clearcasesync.h"
 #include "settingspage.h"
 #include "versionselector.h"
 #include "ui_undocheckout.h"
@@ -115,8 +116,6 @@ static const char CMD_ID_UPDATE_VIEW[]        = "ClearCase.UpdateView";
 static const char CMD_ID_CHECKIN_ALL[]        = "ClearCase.CheckInAll";
 static const char CMD_ID_STATUS[]             = "ClearCase.Status";
 
-StatusMap ClearCasePlugin::s_statusMap;
-
 static const VcsBase::VcsBaseEditorParameters editorParameters[] = {
 {
     VcsBase::RegularCommandOutput,
@@ -182,8 +181,10 @@ ClearCasePlugin::ClearCasePlugin() :
     m_submitRedoAction(0),
     m_menuAction(0),
     m_submitActionTriggered(false),
-    m_activityMutex(new QMutex)
+    m_activityMutex(new QMutex),
+    m_statusMap(new StatusMap)
 {
+    qRegisterMetaType<ClearCase::Internal::FileStatus::Status>("ClearCase::Internal::FileStatus::Status");
 }
 
 ClearCasePlugin::~ClearCasePlugin()
@@ -538,7 +539,7 @@ QStringList ClearCasePlugin::ccGetActiveVobs() const
 // file must be relative to topLevel, and using '/' path separator
 FileStatus ClearCasePlugin::vcsStatus(const QString &file) const
 {
-    return s_statusMap.value(file, FileStatus(FileStatus::Unknown));
+    return m_statusMap->value(file, FileStatus(FileStatus::Unknown));
 }
 
 QString ClearCasePlugin::ccGetFileActivity(const QString &workingDir, const QString &file)
@@ -568,7 +569,7 @@ ClearCaseSubmitEditor *ClearCasePlugin::openClearCaseSubmitEditor(const QString
 void ClearCasePlugin::updateStatusActions()
 {
     bool hasFile = currentState().hasFile();
-    FileStatus fileStatus = s_statusMap.value(currentState().relativeCurrentFile(), FileStatus(FileStatus::Unknown));
+    FileStatus fileStatus = m_statusMap->value(currentState().relativeCurrentFile(), FileStatus(FileStatus::Unknown));
     m_checkOutAction->setEnabled(hasFile && (fileStatus.status & (FileStatus::CheckedIn | FileStatus::Hijacked)));
     m_undoCheckOutAction->setEnabled(hasFile && (fileStatus.status & FileStatus::CheckedOut));
     m_undoHijackAction->setEnabled(hasFile && (fileStatus.status & FileStatus::Hijacked));
@@ -617,9 +618,9 @@ void ClearCasePlugin::addCurrentFile()
     vcsAdd(state.currentFileTopLevel(), state.relativeCurrentFile());
 }
 
-void ClearCasePlugin::setStatus(const QString &file, FileStatus::Status status, bool update)
+void ClearCasePlugin::setStatus(const QString &file, ClearCase::Internal::FileStatus::Status status, bool update)
 {
-    s_statusMap[file] = FileStatus(status, QFileInfo(currentState().topLevel(), file).permissions());
+    m_statusMap->insert(file, FileStatus(status, QFileInfo(currentState().topLevel(), file).permissions()));
     if (update && (currentState().relativeCurrentFile() == file))
         updateStatusActions();
 }
@@ -742,7 +743,7 @@ void ClearCasePlugin::ccDiffWithPred(const QStringList &files)
 
     if ((m_settings.diffType == GraphicalDiff) && (files.count() == 1)) {
         QString file = files.first();
-        if (s_statusMap[file].status == FileStatus::Hijacked)
+        if (m_statusMap->value(file).status == FileStatus::Hijacked)
             diffGraphical(ccGetFileVersion(topLevel, file), file);
         else
             diffGraphical(file);
@@ -750,7 +751,7 @@ void ClearCasePlugin::ccDiffWithPred(const QStringList &files)
     }
     QString result;
     foreach (const QString &file, files) {
-        if (s_statusMap[QDir::fromNativeSeparators(file)].status == FileStatus::Hijacked)
+        if (m_statusMap->value(QDir::fromNativeSeparators(file)).status == FileStatus::Hijacked)
             result += diffExternal(ccGetFileVersion(topLevel, file), file);
         else
             result += diffExternal(file);
@@ -885,8 +886,8 @@ void ClearCasePlugin::startCheckInAll()
     QTC_ASSERT(state.hasTopLevel(), return);
     QString topLevel = state.topLevel();
     QStringList files;
-    for (StatusMap::ConstIterator iterator = s_statusMap.constBegin();
+    for (StatusMap::ConstIterator iterator = m_statusMap->constBegin();
-         iterator != s_statusMap.constEnd();
+         iterator != m_statusMap->constEnd();
          ++iterator)
     {
         if (iterator.value().status == FileStatus::CheckedOut)
@@ -1025,8 +1026,8 @@ void ClearCasePlugin::viewStatus()
     VcsBase::VcsBaseOutputWindow *outputwindow = VcsBase::VcsBaseOutputWindow::instance();
     outputwindow->appendCommand(QLatin1String("Indexed files status (C=Checked Out, H=Hijacked, ?=Missing)"));
     bool anymod = false;
-    for (StatusMap::ConstIterator it = s_statusMap.constBegin();
+    for (StatusMap::ConstIterator it = m_statusMap->constBegin();
-         it != s_statusMap.constEnd();
+         it != m_statusMap->constEnd();
          ++it)
     {
         char cstat = 0;
@@ -1266,13 +1267,13 @@ bool ClearCasePlugin::vcsOpen(const QString &workingDir, const QString &fileName
     const QString title = QString::fromLatin1("Checkout %1").arg(file);
     CheckOutDialog coDialog(title);
     if (!m_settings.disableIndexer &&
-            (fi.isWritable() || s_statusMap[relFile].status == FileStatus::Unknown))
+            (fi.isWritable() || m_statusMap->value(relFile).status == FileStatus::Unknown))
         QtConcurrent::run(&sync, topLevel, QStringList(relFile)).waitForFinished();
-    if (s_statusMap[relFile].status == FileStatus::CheckedOut) {
+    if (m_statusMap->value(relFile).status == FileStatus::CheckedOut) {
         QMessageBox::information(0, tr("ClearCase Checkout"), tr("File is already checked out."));
         return true;
     }
-    bool isHijacked = (s_statusMap[relFile].status & FileStatus::Hijacked);
+    bool isHijacked = (m_statusMap->value(relFile).status & FileStatus::Hijacked);
     if (!isHijacked)
         coDialog.hideHijack();
     if (coDialog.exec() == QDialog::Accepted) {
@@ -1693,7 +1694,7 @@ void ClearCasePlugin::updateIndex()
     if (!project)
         return;
     m_checkInAllAction->setEnabled(false);
-    s_statusMap.clear();
+    m_statusMap->clear();
     QFuture<void> result = QtConcurrent::run(&sync, currentState().topLevel(),
                project->files(ProjectExplorer::Project::ExcludeGeneratedFiles));
     if (!m_settings.disableIndexer)
@@ -1847,109 +1848,11 @@ void ClearCasePlugin::closing()
 void ClearCasePlugin::sync(QFutureInterface<void> &future, QString topLevel, QStringList files)
 {
     ClearCasePlugin *plugin = ClearCasePlugin::instance();
-
+    ClearCaseSync ccSync(plugin, plugin->m_statusMap);
-    ClearCaseSettings settings = plugin->settings();
+    connect(&ccSync, SIGNAL(updateStreamAndView()), plugin, SLOT(updateStreamAndView()));
-    QString program = settings.ccBinaryPath;
+    connect(&ccSync, SIGNAL(setStatus(QString, ClearCase::Internal::FileStatus::Status, bool)),
-    if (program.isEmpty())
+            plugin, SLOT(setStatus(QString, ClearCase::Internal::FileStatus::Status, bool)));
-        return;
+    ccSync.run(future, topLevel, files);
-    int total = files.size();
-    bool hot = (total < 10);
-    int processed = 0;
-    QString view = plugin->currentView();
-    if (view.isEmpty())
-        plugin->updateStreamAndView();
-    if (!hot)
-        total = settings.totalFiles.value(view, total);
-
-    // refresh activities list
-    plugin->refreshActivities();
-
-    if (settings.disableIndexer)
-        return;
-    QStringList vobs;
-    if (!settings.indexOnlyVOBs.isEmpty())
-        vobs = settings.indexOnlyVOBs.split(QLatin1Char(','));
-    else
-        vobs = plugin->ccGetActiveVobs();
-    QDir topLevelDir(topLevel);
-    QStringList args(QLatin1String("ls"));
-    if (hot) {
-        // find all files whose permissions changed OR hijacked files
-        // (might have become checked out)
-        foreach (const QString &file, s_statusMap.keys()) {
-            bool permChanged =
-                    s_statusMap[file].permissions != QFileInfo(topLevel, file).permissions();
-            if (permChanged || s_statusMap[file].status == FileStatus::Hijacked) {
-                files.append(file);
-                s_statusMap[file].status = FileStatus::Unknown;
-                ++total;
-            }
-        }
-        args << files;
-    } else {
-        foreach (const QString &file, files)
-            plugin->setStatus(topLevelDir.relativeFilePath(file), FileStatus::Unknown, false);
-        args << QLatin1String("-recurse");
-        args << vobs;
-    }
-
-    // adding 1 for initial sync in which total is not accurate, to prevent finishing
-    // (we don't want it to become green)
-    future.setProgressRange(0, total + 1);
-    QProcess process;
-    process.setWorkingDirectory(topLevel);
-    process.start(program, args);
-    if (!process.waitForStarted())
-        return;
-    QString buffer;
-    while (process.waitForReadyRead() && !future.isCanceled()) {
-        while (process.state() == QProcess::Running &&
-               process.bytesAvailable() && !future.isCanceled())
-        {
-            QString line = QString::fromLocal8Bit(process.readLine().constData());
-            buffer += line;
-            if (buffer.endsWith(QLatin1Char('\n')) || process.atEnd()) {
-                int atatpos = buffer.indexOf(QLatin1String("@@"));
-                if (atatpos != -1) { // probably managed file
-                    // find first whitespace. anything before that is not interesting
-                    int wspos = buffer.indexOf(QRegExp(QLatin1String("\\s")));
-                    const QString file = QDir::fromNativeSeparators(buffer.left(atatpos));
-                    QString ccState;
-                    QRegExp reState(QLatin1String("^\\s*\\[[^\\]]*\\]")); // [hijacked]; [loaded but missing]
-                    if (reState.indexIn(buffer, wspos + 1, QRegExp::CaretAtOffset) != -1) {
-                        ccState = reState.cap();
-                        if (ccState.indexOf(QLatin1String("hijacked")) != -1)
-                            plugin->setStatus(file, FileStatus::Hijacked, true);
-                        else if (ccState.indexOf(QLatin1String("loaded but missing")) != -1)
-                            plugin->setStatus(file, FileStatus::Missing, false);
-                    }
-                    else if (buffer.lastIndexOf(QLatin1String("CHECKEDOUT"), wspos) != -1)
-                        plugin->setStatus(file, FileStatus::CheckedOut, true);
-                    // don't care about checked-in files not listed in project
-                    else if (s_statusMap.contains(file))
-                        plugin->setStatus(file, FileStatus::CheckedIn, false);
-                }
-                buffer.clear();
-                future.setProgressValue(qMin(total, ++processed));
-            }
-        }
-    }
-
-    if (!future.isCanceled()) {
-        foreach (const QString &file, files) {
-            QString relFile = topLevelDir.relativeFilePath(file);
-            if (s_statusMap[relFile].status == FileStatus::Unknown)
-                plugin->setStatus(relFile, FileStatus::NotManaged, false);
-        }
-        future.setProgressValue(total + 1);
-        if (!hot) {
-            settings.totalFiles[view] = processed;
-            plugin->setSettings(settings);
-        }
-    }
-    if (process.state() == QProcess::Running)
-        process.kill();
-    process.waitForFinished();
 }
 
 } // namespace Internal

src/plugins/clearcase/clearcaseplugin.h

@@ -39,6 +39,8 @@
 #include <QFile>
 #include <QPair>
 #include <QStringList>
+#include <QMetaType>
+#include <QSharedPointer>
 
 QT_BEGIN_NAMESPACE
 class QAction;
@@ -145,11 +147,14 @@ public:
                   const QString &fileName, const QString &file2 = QString());
     FileStatus vcsStatus(const QString &file) const;
     QString currentView() const { return m_view; }
+    void refreshActivities();
 
 public slots:
     void vcsAnnotate(const QString &workingDir, const QString &file,
                      const QString &revision = QString(), int lineNumber = -1) const;
     bool newActivity();
+    void updateStreamAndView();
+    void setStatus(const QString &file, ClearCase::Internal::FileStatus::Status status, bool update = true);
 
 private slots:
     void checkOutCurrentFile();
@@ -179,7 +184,6 @@ protected:
     void updateActions(VcsBase::VcsBasePlugin::ActionState);
     bool submitEditorAboutToClose(VcsBase::VcsBaseSubmitEditor *submitEditor);
     QString ccGet(const QString &workingDir, const QString &file, const QString &prefix = QString());
-    void refreshActivities();
     QList<QStringPair> ccGetActivities() const;
 
 private:
@@ -204,7 +208,6 @@ private:
     void cleanCheckInMessageFile();
     inline ClearCaseControl *clearCaseControl() const;
     QString ccGetFileActivity(const QString &workingDir, const QString &file);
-    void setStatus(const QString &file, FileStatus::Status status, bool update = true);
     QStringList ccGetActivityVersions(const QString &workingDir, const QString &activity);
     void updateStatusActions();
     void diffGraphical(const QString &file1, const QString &file2 = QString());
@@ -214,11 +217,9 @@ private:
     QString runExtDiff(const QString &workingDir, const QStringList &arguments,
                        int timeOut, QTextCodec *outputCodec = 0);
     QString ccGetView(const QString &workingDir, bool *isDynamic = 0) const;
-    void updateStreamAndView();
 
     ClearCaseSettings m_settings;
 
-    static StatusMap s_statusMap;
     QString m_checkInMessageFileName;
     QString m_topLevel;
     QString m_stream;
@@ -251,6 +252,7 @@ private:
     bool m_submitActionTriggered;
     QMutex *m_activityMutex;
     QList<QStringPair> m_activities;
+    QSharedPointer<StatusMap> m_statusMap;
 
     static ClearCasePlugin *m_clearcasePluginInstance;
 };
@@ -258,4 +260,6 @@ private:
 } // namespace Internal
 } // namespace ClearCase
 
+Q_DECLARE_METATYPE(ClearCase::Internal::FileStatus::Status)
+
 #endif // CLEARCASEPLUGIN_H

src/plugins/clearcase/clearcasesync.cpp

@@ -0,0 +1,125 @@
+#include "clearcasesync.h"
+
+#include <QDir>
+#include <QFutureInterface>
+#include <QProcess>
+#include <QStringList>
+
+namespace ClearCase {
+namespace Internal {
+
+ClearCaseSync::ClearCaseSync(ClearCasePlugin *plugin, QSharedPointer<StatusMap> statusMap) :
+    m_plugin(plugin),
+    m_statusMap(statusMap)
+{
+}
+
+void ClearCaseSync::run(QFutureInterface<void> &future, const QString &topLevel, QStringList &files)
+{
+    ClearCaseSettings settings = m_plugin->settings();
+    QString program = settings.ccBinaryPath;
+    if (program.isEmpty())
+        return;
+    int total = files.size();
+    bool hot = (total < 10);
+    int processed = 0;
+    QString view = m_plugin->currentView();
+    if (view.isEmpty())
+        emit updateStreamAndView();
+    if (!hot)
+        total = settings.totalFiles.value(view, total);
+
+    // refresh activities list
+    m_plugin->refreshActivities();
+
+    if (settings.disableIndexer)
+        return;
+    QStringList vobs;
+    if (!settings.indexOnlyVOBs.isEmpty())
+        vobs = settings.indexOnlyVOBs.split(QLatin1Char(','));
+    else
+        vobs = m_plugin->ccGetActiveVobs();
+    QDir topLevelDir(topLevel);
+    QStringList args(QLatin1String("ls"));
+    if (hot) {
+        // find all files whose permissions changed OR hijacked files
+        // (might have become checked out)
+        foreach (const QString &file, m_statusMap->keys()) {
+            bool permChanged =
+                    m_statusMap->value(file).permissions != QFileInfo(topLevel, file).permissions();
+            if (permChanged || m_statusMap->value(file).status == FileStatus::Hijacked) {
+                files.append(file);
+                (*m_statusMap)[file].status = FileStatus::Unknown;
+                ++total;
+            }
+        }
+        args << files;
+    } else {
+        foreach (const QString &file, files)
+            emit setStatus(topLevelDir.relativeFilePath(file), FileStatus::Unknown, false);
+        args << QLatin1String("-recurse");
+        args << vobs;
+    }
+
+    // adding 1 for initial sync in which total is not accurate, to prevent finishing
+    // (we don't want it to become green)
+    future.setProgressRange(0, total + 1);
+    QProcess process;
+    process.setWorkingDirectory(topLevel);
+    process.start(program, args);
+    if (!process.waitForStarted())
+        return;
+    QString buffer;
+    while (process.waitForReadyRead() && !future.isCanceled()) {
+        while (process.state() == QProcess::Running &&
+               process.bytesAvailable() && !future.isCanceled())
+        {
+            QString line = QString::fromLocal8Bit(process.readLine().constData());
+            buffer += line;
+            if (buffer.endsWith(QLatin1Char('\n')) || process.atEnd()) {
+                int atatpos = buffer.indexOf(QLatin1String("@@"));
+                if (atatpos != -1) { // probably managed file
+                    // find first whitespace. anything before that is not interesting
+                    int wspos = buffer.indexOf(QRegExp(QLatin1String("\\s")));
+                    const QString file = QDir::fromNativeSeparators(buffer.left(atatpos));
+                    QString ccState;
+                    QRegExp reState(QLatin1String("^\\s*\\[[^\\]]*\\]")); // [hijacked]; [loaded but missing]
+                    if (reState.indexIn(buffer, wspos + 1, QRegExp::CaretAtOffset) != -1) {
+                        ccState = reState.cap();
+                        if (ccState.indexOf(QLatin1String("hijacked")) != -1)
+                            emit setStatus(file, FileStatus::Hijacked, true);
+                        else if (ccState.indexOf(QLatin1String("loaded but missing")) != -1)
+                            emit setStatus(file, FileStatus::Missing, false);
+                    }
+                    else if (buffer.lastIndexOf(QLatin1String("CHECKEDOUT"), wspos) != -1)
+                        emit setStatus(file, FileStatus::CheckedOut, true);
+                    // don't care about checked-in files not listed in project
+                    else if (m_statusMap->contains(file))
+                        emit setStatus(file, FileStatus::CheckedIn, true);
+                }
+                buffer.clear();
+                future.setProgressValue(qMin(total, ++processed));
+            }
+        }
+    }
+
+    if (!future.isCanceled()) {
+        foreach (const QString &file, files) {
+            QString relFile = topLevelDir.relativeFilePath(file);
+            if (m_statusMap->value(relFile).status == FileStatus::Unknown)
+                emit setStatus(relFile, FileStatus::NotManaged, false);
+        }
+        future.setProgressValue(total + 1);
+        if (!hot) {
+            settings = m_plugin->settings(); // Might have changed while task was running
+            settings.totalFiles[view] = processed;
+            m_plugin->setSettings(settings);
+        }
+    }
+    if (process.state() == QProcess::Running)
+        process.kill();
+    process.waitForFinished();
+}
+
+} // namespace Internal
+} // namespace ClearCase

src/plugins/clearcase/clearcasesync.h

@@ -0,0 +1,28 @@
+#ifndef CLEARCASESYNC_H
+#define CLEARCASESYNC_H
+
+#include "clearcaseplugin.h"
+
+namespace ClearCase {
+namespace Internal {
+
+class ClearCaseSync : public QObject
+{
+    Q_OBJECT
+public:
+    explicit ClearCaseSync(ClearCasePlugin *plugin, QSharedPointer<StatusMap> statusMap);
+    void run(QFutureInterface<void> &future, const QString &topLevel, QStringList &files);
+
+signals:
+    void updateStreamAndView();
+    void setStatus(const QString &file, ClearCase::Internal::FileStatus::Status status, bool update);
+
+private:
+    ClearCasePlugin *m_plugin;
+    QSharedPointer<StatusMap> m_statusMap;
+};
+
+} // namespace Internal
+} // namespace ClearCase
+
+#endif // CLEARCASESYNC_H