Get rid of the Locker in TestResultModel...

...use signals and slots instead.

Additionally re-use the existing item that is displaying the
"Entering Test Function..." information.

Change-Id: Ibedac01ced9e987d542aa4dc878588fbec84d585
Reviewed-by: Tim Jenssen <tim.jenssen@theqtcompany.com>

plugins/autotest/autotestplugin.cpp

@@ -51,6 +51,8 @@ static AutotestPlugin *m_instance = 0;
 AutotestPlugin::AutotestPlugin()
     : m_settings(new TestSettings)
 {
+    // needed to be used in QueuedConnection connects
+    qRegisterMetaType<TestResult>();
     // Create your members
     m_instance = this;
 }

plugins/autotest/testresult.h

@@ -46,7 +46,8 @@ class TestResult
 {
 public:
 
-    TestResult(const QString &className, const QString &testCase, const QString &dataTag = QString(),
+    TestResult(const QString &className = QString(), const QString &testCase = QString(),
+               const QString &dataTag = QString(),
                ResultType result = UNKNOWN, const QString &description = QString());
 
     QString className() const { return m_class; }
@@ -57,6 +58,7 @@ public:
     QString fileName() const { return m_file; }
     int line() const { return m_line; }
 
+    void setDescription(const QString &description) { m_description = description; }
     void setFileName(const QString &fileName) { m_file = fileName; }
     void setLine(int line) { m_line = line; }
 
@@ -87,6 +89,7 @@ bool operator==(const TestResult &t1, const TestResult &t2);
 } // namespace Internal
 } // namespace Autotest
 
+Q_DECLARE_METATYPE(Autotest::Internal::TestResult)
 Q_DECLARE_METATYPE(Autotest::Internal::ResultType)
 
 #endif // TESTRESULT_H

plugins/autotest/testresultmodel.cpp

@@ -36,7 +36,6 @@ TestResultModel::TestResultModel(QObject *parent) :
 
 TestResultModel::~TestResultModel()
 {
-    QWriteLocker lock(&m_rwLock);
     m_testResults.clear();
 }
 
@@ -54,7 +53,6 @@ QModelIndex TestResultModel::parent(const QModelIndex &) const
 
 int TestResultModel::rowCount(const QModelIndex &parent) const
 {
-    QReadLocker lock(&m_rwLock);
     return parent.isValid() ? 0 : m_testResults.size();
 }
 
@@ -85,7 +83,6 @@ static QIcon testResultIcon(ResultType result) {
 
 QVariant TestResultModel::data(const QModelIndex &index, int role) const
 {
-    QReadLocker lock(&m_rwLock);
     if (!index.isValid() || index.row() >= m_testResults.count() || index.column() != 0)
         return QVariant();
     if (role == Qt::DisplayRole) {
@@ -118,23 +115,17 @@ void TestResultModel::addTestResult(const TestResult &testResult)
     const bool isCurrentTestMssg = testResult.result() == ResultType::MESSAGE_CURRENT_TEST;
     const bool hasCurrentTestMssg = m_availableResultTypes.contains(ResultType::MESSAGE_CURRENT_TEST);
 
-    QReadLocker rLock(&m_rwLock);
     int position = m_testResults.size();
-    rLock.unlock();
 
     if (hasCurrentTestMssg && isCurrentTestMssg) {
-        beginRemoveRows(QModelIndex(), position, position);
-        QWriteLocker wLock(&m_rwLock);
-        m_testResults.replace(position - 1, testResult);
-        wLock.unlock();
-        endRemoveRows();
+        m_testResults.last().setDescription(testResult.description());
+        const QModelIndex changed = index(m_testResults.size() - 1, 0, QModelIndex());
+        emit dataChanged(changed, changed);
     } else {
         if (!isCurrentTestMssg && position) // decrement only if at least one other item
             --position;
         beginInsertRows(QModelIndex(), position, position);
-        QWriteLocker wLock(&m_rwLock);
         m_testResults.insert(position, testResult);
-        wLock.unlock();
         endInsertRows();
     }
 
@@ -148,13 +139,9 @@ void TestResultModel::addTestResult(const TestResult &testResult)
 
 void TestResultModel::removeCurrentTestMessage()
 {
-    QReadLocker rLock(&m_rwLock);
     if (m_availableResultTypes.contains(ResultType::MESSAGE_CURRENT_TEST)) {
         beginRemoveRows(QModelIndex(), m_testResults.size() - 1, m_testResults.size() - 1);
-        rLock.unlock();
-        QWriteLocker wLock(&m_rwLock);
         m_testResults.removeLast();
-        wLock.unlock();
         endRemoveRows();
         m_availableResultTypes.remove(ResultType::MESSAGE_CURRENT_TEST);
     }
@@ -162,14 +149,10 @@ void TestResultModel::removeCurrentTestMessage()
 
 void TestResultModel::clearTestResults()
 {
-    QReadLocker rLock(&m_rwLock);
     if (m_testResults.size() == 0)
         return;
     beginRemoveRows(QModelIndex(), 0, m_testResults.size() - 1);
-    rLock.unlock();
-    QWriteLocker wLock(&m_rwLock);
     m_testResults.clear();
-    wLock.unlock();
     m_testResultCount.clear();
     m_lastMaxWidthIndex = 0;
     m_maxWidthOfFileName = 0;
@@ -182,15 +165,12 @@ TestResult TestResultModel::testResult(const QModelIndex &index) const
 {
     if (!index.isValid())
         return TestResult(QString(), QString());
-    QReadLocker lock(&m_rwLock);
     return m_testResults.at(index.row());
 }
 
 int TestResultModel::maxWidthOfFileName(const QFont &font)
 {
-    QReadLocker lock(&m_rwLock);
     int count = m_testResults.size();
-    lock.unlock();
     if (count == 0)
         return 0;
     if (m_maxWidthOfFileName > 0 && font == m_measurementFont && m_lastMaxWidthIndex == count - 1)
@@ -200,9 +180,7 @@ int TestResultModel::maxWidthOfFileName(const QFont &font)
     m_measurementFont = font;
 
     for (int i = m_lastMaxWidthIndex; i < count; ++i) {
-        lock.relock();
         QString filename = m_testResults.at(i).fileName();
-        lock.unlock();
         const int pos = filename.lastIndexOf(QLatin1Char('/'));
         if (pos != -1)
             filename = filename.mid(pos +1);

plugins/autotest/testresultmodel.h

@@ -24,7 +24,6 @@
 #include <QAbstractItemModel>
 #include <QSortFilterProxyModel>
 #include <QFont>
-#include <QReadWriteLock>
 #include <QSet>
 
 namespace Autotest {
@@ -67,7 +66,6 @@ private:
     int m_lastMaxWidthIndex;
     QFont m_measurementFont;
     QSet<ResultType> m_availableResultTypes;
-    mutable QReadWriteLock m_rwLock;
 };
 
 class TestResultFilterModel : public QSortFilterProxyModel

plugins/autotest/testresultspane.h

@@ -52,8 +52,6 @@ public:
     virtual ~TestResultsPane();
     static TestResultsPane *instance();
 
-    void addTestResult(const TestResult &result);
-
     // IOutputPane interface
     QWidget *outputWidget(QWidget *parent);
     QList<QWidget *> toolBarWidgets() const;
@@ -73,6 +71,7 @@ public:
 signals:
 
 public slots:
+    void addTestResult(const TestResult &result);
 
 private slots:
     void onItemActivated(const QModelIndex &index);

plugins/autotest/testrunner.cpp

@@ -192,6 +192,11 @@ static bool xmlExtractBenchmarkInformation(const QString &code, const QString &t
     return false;
 }
 
+void emitTestResultCreated(const TestResult &testResult)
+{
+    emit m_instance->testResultCreated(testResult);
+}
+
 /****************** XML line parser helper end ******************/
 
 void processOutput()
@@ -228,8 +233,8 @@ void processOutput()
             result = ResultType::UNKNOWN;
             lineNumber = 0;
             readingDescription = false;
-            TestResultsPane::instance()->addTestResult(
-                        TestResult(className, testCase, QString(), ResultType::MESSAGE_CURRENT_TEST,
+            emitTestResultCreated(
+                        TestResult(QString(), QString(), QString(), ResultType::MESSAGE_CURRENT_TEST,
                                    QObject::tr("Entering Test Function %1::%2")
                                    .arg(className).arg(testCase)));
             continue;
@@ -253,13 +258,13 @@ void processOutput()
                     file = QFileInfo(m_runner->workingDirectory(), file).canonicalFilePath();
                 testResult.setFileName(file);
                 testResult.setLine(lineNumber);
-                TestResultsPane::instance()->addTestResult(testResult);
+                emitTestResultCreated(testResult);
             }
             continue;
         }
         if (xmlExtractBenchmarkInformation(line, QLatin1String("<BenchmarkResult"), bmDescription)) {
             TestResult testResult(className, testCase, dataTag, ResultType::BENCHMARK, bmDescription);
-            TestResultsPane::instance()->addTestResult(testResult);
+            emitTestResultCreated(testResult);
             continue;
         }
         if (line == QLatin1String("</Message>") || line == QLatin1String("</Incident>")) {
@@ -268,17 +273,17 @@ void processOutput()
                 file = QFileInfo(m_runner->workingDirectory(), file).canonicalFilePath();
             testResult.setFileName(file);
             testResult.setLine(lineNumber);
-            TestResultsPane::instance()->addTestResult(testResult);
+            emitTestResultCreated(testResult);
             description = QString();
         } else if (line == QLatin1String("</TestFunction>") && !duration.isEmpty()) {
             TestResult testResult(className, testCase, QString(), ResultType::MESSAGE_INTERNAL,
                                   QObject::tr("execution took %1ms").arg(duration));
-            TestResultsPane::instance()->addTestResult(testResult);
+            emitTestResultCreated(testResult);
             m_currentFuture->setProgressValue(m_currentFuture->progressValue() + 1);
         } else if (line == QLatin1String("</TestCase>") && !duration.isEmpty()) {
             TestResult testResult(className, QString(), QString(), ResultType::MESSAGE_INTERNAL,
                                   QObject::tr("Test execution took %1ms").arg(duration));
-            TestResultsPane::instance()->addTestResult(testResult);
+            emitTestResultCreated(testResult);
         } else if (readingDescription) {
             if (line.endsWith(QLatin1String("]]></Description>"))) {
                 description.append(QLatin1Char('\n'));
@@ -289,10 +294,10 @@ void processOutput()
                 description.append(line);
             }
         } else if (xmlStartsWith(line, QLatin1String("<QtVersion>"), qtVersion)) {
-            TestResultsPane::instance()->addTestResult(FaultyTestResult(ResultType::MESSAGE_INTERNAL,
+            emitTestResultCreated(FaultyTestResult(ResultType::MESSAGE_INTERNAL,
                 QObject::tr("Qt Version: %1").arg(qtVersion)));
         } else if (xmlStartsWith(line, QLatin1String("<QTestVersion>"), qtestVersion)) {
-            TestResultsPane::instance()->addTestResult(FaultyTestResult(ResultType::MESSAGE_INTERNAL,
+            emitTestResultCreated(FaultyTestResult(ResultType::MESSAGE_INTERNAL,
                 QObject::tr("QTest Version: %1").arg(qtestVersion)));
         } else {
 //            qDebug() << "Unhandled line:" << line; // TODO remove
@@ -344,7 +349,7 @@ bool performExec(const QString &cmd, const QStringList &args, const QString &wor
     }
 
     if (runCmd.isEmpty()) {
-        TestResultsPane::instance()->addTestResult(FaultyTestResult(ResultType::MESSAGE_FATAL,
+        emitTestResultCreated(FaultyTestResult(ResultType::MESSAGE_FATAL,
             QObject::tr("*** Could not find command '%1' ***").arg(cmd)));
         return false;
     }
@@ -366,7 +371,7 @@ bool performExec(const QString &cmd, const QStringList &args, const QString &wor
             if (m_currentFuture->isCanceled()) {
                 m_runner->kill();
                 m_runner->waitForFinished();
-                TestResultsPane::instance()->addTestResult(FaultyTestResult(ResultType::MESSAGE_FATAL,
+                emitTestResultCreated(FaultyTestResult(ResultType::MESSAGE_FATAL,
                     QObject::tr("*** Test Run canceled by user ***")));
             }
             qApp->processEvents();
@@ -378,7 +383,7 @@ bool performExec(const QString &cmd, const QStringList &args, const QString &wor
         if (m_runner->state() != QProcess::NotRunning) {
             m_runner->kill();
             m_runner->waitForFinished();
-            TestResultsPane::instance()->addTestResult(FaultyTestResult(ResultType::MESSAGE_FATAL,
+            emitTestResultCreated(FaultyTestResult(ResultType::MESSAGE_FATAL,
                 QObject::tr("*** Test Case canceled due to timeout ***\nMaybe raise the timeout?")));
         }
         return false;
@@ -484,6 +489,10 @@ void TestRunner::runTests()
     }
 
     m_executingTests = true;
+    connect(this, &TestRunner::testResultCreated,
+            TestResultsPane::instance(), &TestResultsPane::addTestResult,
+            Qt::QueuedConnection);
+
     emit testRunStarted();
     QFuture<void> future = QtConcurrent::run(&performTestRun , m_selectedTests);
     Core::FutureProgress *progress = Core::ProgressManager::addTask(future, tr("Running Tests"),
@@ -516,6 +525,9 @@ void TestRunner::buildFinished(bool success)
 
 void TestRunner::onFinished()
 {
+    disconnect(this, &TestRunner::testResultCreated,
+               TestResultsPane::instance(), &TestResultsPane::addTestResult);
+
     m_executingTests = false;
     emit testRunFinished();
 }

plugins/autotest/testrunner.h

@@ -46,6 +46,7 @@ public:
 signals:
     void testRunStarted();
     void testRunFinished();
+    void testResultCreated(const TestResult &testResult);
 
 public slots:
     void runTests();