AutoTest: Redo check state handling

Simplify and re-arrange to avoid wrong indirections and
unnecessary delegation.
Beside this ensure correct check states of group nodes
when adding them.

Change-Id: I24a32249d785e48c9d27111d062c2a06a17327ef
Reviewed-by: David Schulz <david.schulz@qt.io>

src/plugins/autotest/qtest/qttesttreeitem.cpp

@@ -38,7 +38,7 @@ QtTestTreeItem::QtTestTreeItem(const QString &name, const QString &filePath, Tes
     : TestTreeItem(name, filePath, type)
 {
     if (type == TestDataTag)
-        setChecked(Qt::Checked);
+        setData(0, Qt::Checked, Qt::CheckStateRole);
 }
 
 QVariant QtTestTreeItem::data(int column, int role) const

src/plugins/autotest/testtreeitem.cpp

@@ -106,9 +106,9 @@ QVariant TestTreeItem::data(int /*column*/, int role) const
 bool TestTreeItem::setData(int /*column*/, const QVariant &data, int role)
 {
     if (role == Qt::CheckStateRole) {
-        Qt::CheckState old = checked();
+        Qt::CheckState old = m_checked;
-        setChecked((Qt::CheckState)data.toInt());
+        m_checked = (Qt::CheckState)data.toInt();
-        return checked() != old;
+        return m_checked != old;
     }
     return false;
 }
@@ -168,34 +168,6 @@ bool TestTreeItem::modifyLineAndColumn(const TestParseResult *result)
     return hasBeenModified;
 }
 
-void TestTreeItem::setChecked(const Qt::CheckState checkState)
-{
-    switch (m_type) {
-    case TestDataTag: {
-        m_checked = (checkState == Qt::Unchecked ? Qt::Unchecked : Qt::Checked);
-        if (auto parent = parentItem())
-            parent->revalidateCheckState();
-        break;
-    }
-    case Root:
-    case GroupNode:
-    case TestFunctionOrSet:
-    case TestCase: {
-        Qt::CheckState usedState = (checkState == Qt::Unchecked ? Qt::Unchecked : Qt::Checked);
-        for (int row = 0, count = childCount(); row < count; ++row)
-            childItem(row)->setChecked(usedState);
-        m_checked = usedState;
-        if (m_type != Root) {
-            if (auto parent = parentItem())
-                parent->revalidateCheckState();
-        }
-        break;
-    }
-    default:
-        return;
-    }
-}
-
 Qt::CheckState TestTreeItem::checked() const
 {
     switch (m_type) {
@@ -327,41 +299,6 @@ QSet<QString> TestTreeItem::internalTargets() const
     return targets;
 }
 
-void TestTreeItem::revalidateCheckState()
-{
-    const Type ttiType = type();
-    if (ttiType != TestCase && ttiType != TestFunctionOrSet && ttiType != Root && ttiType != GroupNode)
-        return;
-    if (childCount() == 0) // can this happen? (we're calling revalidateCS() on parentItem()
-        return;
-    bool foundChecked = false;
-    bool foundUnchecked = false;
-    bool foundPartiallyChecked = false;
-    for (int row = 0, count = childCount(); row < count; ++row) {
-        TestTreeItem *child = childItem(row);
-        switch (child->type()) {
-        case TestDataFunction:
-        case TestSpecialFunction:
-            continue;
-        default:
-            break;
-        }
-
-        foundChecked |= (child->checked() == Qt::Checked);
-        foundUnchecked |= (child->checked() == Qt::Unchecked);
-        foundPartiallyChecked |= (child->checked() == Qt::PartiallyChecked);
-        if (foundPartiallyChecked || (foundChecked && foundUnchecked)) {
-            m_checked = Qt::PartiallyChecked;
-            if (ttiType == TestFunctionOrSet || ttiType == TestCase || ttiType == GroupNode)
-                parentItem()->revalidateCheckState();
-            return;
-        }
-    }
-    m_checked = (foundUnchecked ? Qt::Unchecked : Qt::Checked);
-    if (ttiType == TestFunctionOrSet || ttiType == TestCase || ttiType == GroupNode)
-        parentItem()->revalidateCheckState();
-}
-
 inline bool TestTreeItem::modifyFilePath(const QString &filePath)
 {
     if (m_filePath != filePath) {

src/plugins/autotest/testtreeitem.h

@@ -89,7 +89,6 @@ public:
     unsigned column() const { return m_column; }
     QString proFile() const { return m_proFile; }
     void setProFile(const QString &proFile) { m_proFile = proFile; }
-    virtual void setChecked(const Qt::CheckState checked);
     virtual Qt::CheckState checked() const;
     Type type() const { return m_type; }
     void markForRemoval(bool mark);
@@ -123,7 +122,6 @@ protected:
                                                   const QString &file);
 
 private:
-    void revalidateCheckState();
     bool modifyFilePath(const QString &filePath);
     bool modifyName(const QString &name);
 
@@ -143,7 +141,7 @@ private:
     QString m_proFile;
     Status m_status = NewlyAdded;
 
-    friend class TestTreeModel; // grant access to (private) revalidateCheckState()
+    friend class TestTreeModel; // grant access to (protected) findChildBy()
 };
 
 class TestCodeLocationAndType

src/plugins/autotest/testtreemodel.cpp

@@ -108,21 +108,18 @@ bool TestTreeModel::setData(const QModelIndex &index, const QVariant &value, int
     if (item && item->setData(index.column(), value, role)) {
         emit dataChanged(index, index);
         if (role == Qt::CheckStateRole) {
-            switch (item->type()) {
+            Qt::CheckState checked = item->checked();
-            case TestTreeItem::Root:
+            if (item->hasChildren() && checked != Qt::PartiallyChecked) {
-            case TestTreeItem::GroupNode:
+                // handle the new checkstate for children as well...
-            case TestTreeItem::TestCase:
+                for (Utils::TreeItem *child : *item) {
-                if (item->childCount() > 0)
+                    const QModelIndex &idx = indexForItem(child);
-                    emit dataChanged(index.child(0, 0), index.child(item->childCount() - 1, 0));
+                    setData(idx, checked ? Qt::Checked : Qt::Unchecked, Qt::CheckStateRole);
-                break;
+                }
-            case TestTreeItem::TestFunctionOrSet:
-                emit dataChanged(index.parent(), index.parent());
-                break;
-            default: // avoid warning regarding unhandled enum member
-                break;
             }
+            if (item->parent() != rootItem() && item->parentItem()->checked() != checked)
+                revalidateCheckState(item->parentItem()); // handle parent too
+            return true;
         }
-        return true;
     }
     return false;
 }
@@ -254,7 +251,7 @@ bool TestTreeModel::sweepChildren(TestTreeItem *item)
 
         if (child->type() != TestTreeItem::Root && child->markedForRemoval()) {
             destroyItem(child);
-            item->revalidateCheckState();
+            revalidateCheckState(item);
             hasChanged = true;
         } else if (child->hasChildren()) {
             hasChanged |= sweepChildren(child);
@@ -281,6 +278,50 @@ void TestTreeModel::insertItemInParent(TestTreeItem *item, TestTreeItem *root, b
         }
     }
     parentNode->appendChild(item);
+    if (item->checked() != parentNode->checked())
+        revalidateCheckState(parentNode);
+}
+
+void TestTreeModel::revalidateCheckState(TestTreeItem *item)
+{
+    QTC_ASSERT(item, return);
+    QTC_ASSERT(item->hasChildren(), return);
+    const TestTreeItem::Type type = item->type();
+    if (type == TestTreeItem::TestSpecialFunction || type == TestTreeItem::TestDataFunction
+            || type == TestTreeItem::TestDataTag) {
+        return;
+    }
+    const Qt::CheckState oldState = (Qt::CheckState)item->data(0, Qt::CheckStateRole).toInt();
+    Qt::CheckState newState = Qt::Checked;
+    bool foundChecked = false;
+    bool foundUnchecked = false;
+    bool foundPartiallyChecked = false;
+    for (int row = 0, count = item->childCount(); row < count; ++row) {
+        TestTreeItem *child = item->childItem(row);
+        switch (child->type()) {
+        case TestTreeItem::TestDataFunction:
+        case TestTreeItem::TestSpecialFunction:
+            continue;
+        default:
+            break;
+        }
+
+        foundChecked |= (child->checked() == Qt::Checked);
+        foundUnchecked |= (child->checked() == Qt::Unchecked);
+        foundPartiallyChecked |= (child->checked() == Qt::PartiallyChecked);
+        if (foundPartiallyChecked || (foundChecked && foundUnchecked)) {
+            newState = Qt::PartiallyChecked;
+            break;
+        }
+    }
+    if (newState != Qt::PartiallyChecked)
+        newState = foundUnchecked ? Qt::Unchecked : Qt::Checked;
+    if (oldState != newState) {
+        item->setData(0, newState, Qt::CheckStateRole);
+        emit dataChanged(item->index(), item->index());
+        if (item->parent() != rootItem() && item->parentItem()->checked() != newState)
+            revalidateCheckState(item->parentItem());
+    }
 }
 
 void TestTreeModel::onParseResultReady(const TestParseResultPtr result)
@@ -321,12 +362,6 @@ void TestTreeModel::handleParseResult(const TestParseResult *result, TestTreeIte
     QTC_ASSERT(newItem, return);
 
     insertItemInParent(newItem, parentNode, groupingEnabled);
-    // new items are checked by default - revalidation of parents might be necessary
-    if (parentNode->checked() != Qt::Checked) {
-        parentNode->revalidateCheckState();
-        const QModelIndex &idx = indexForItem(parentNode);
-        emit dataChanged(idx, idx);
-    }
 }
 
 void TestTreeModel::removeAllTestItems()
@@ -335,7 +370,7 @@ void TestTreeModel::removeAllTestItems()
         item->removeChildren();
         TestTreeItem *testTreeItem = static_cast<TestTreeItem *>(item);
         if (testTreeItem->checked() == Qt::PartiallyChecked)
-            testTreeItem->setChecked(Qt::Checked);
+            testTreeItem->setData(0, Qt::Checked, Qt::CheckStateRole);
     }
     emit testTreeModelChanged();
 }

src/plugins/autotest/testtreemodel.h

@@ -87,8 +87,8 @@ private:
     void removeTestRootNodes();
     void removeFiles(const QStringList &files);
     bool sweepChildren(TestTreeItem *item);
-    static void insertItemInParent(TestTreeItem *item, TestTreeItem *root, bool groupingEnabled);
+    void insertItemInParent(TestTreeItem *item, TestTreeItem *root, bool groupingEnabled);
-
+    void revalidateCheckState(TestTreeItem *item);
     explicit TestTreeModel(QObject *parent = 0);
     void setupParsingConnections();
 

src/plugins/autotest/testtreeview.cpp

@@ -71,7 +71,7 @@ void TestTreeView::changeCheckStateAll(const Qt::CheckState checkState)
             int funcCount = model->rowCount(classesIndex);
             TestTreeItem *item = static_cast<TestTreeItem *>(classesIndex.internalPointer());
             if (item) {
-                item->setChecked(checkState);
+                item->setData(0, checkState, Qt::CheckStateRole);
                 if (!item->childCount())
                     last = classesIndex;
             }
@@ -79,12 +79,12 @@ void TestTreeView::changeCheckStateAll(const Qt::CheckState checkState)
                 last = model->index(functionRow, 0, classesIndex);
                 TestTreeItem *item = static_cast<TestTreeItem *>(last.internalPointer());
                 if (item)
-                    item->setChecked(checkState);
+                    item->setData(0, checkState, Qt::CheckStateRole);
             }
         }
         if (count == 0) {
             if (auto item = static_cast<TestTreeItem *>(currentRootIndex.internalPointer()))
-                item->setChecked(checkState);
+                item->setData(0, checkState, Qt::CheckStateRole);
         }
         emit dataChanged(currentRootIndex, last);
     }