Toolchains: Make options page more stable.

Avoid double deletions by using shared pointers. Reset the model
after discard as the structure may change. Use qtc_assert
(to catch cases of add/delete again).

src/plugins/projectexplorer/toolchainoptionspage.cpp

@@ -44,6 +44,7 @@
 
 #include <QtCore/QSignalMapper>
 #include <QtCore/QTextStream>
+
 #include <QtGui/QAction>
 #include <QtGui/QItemSelectionModel>
 #include <QtGui/QLabel>
@@ -53,14 +54,14 @@
 namespace ProjectExplorer {
 namespace Internal {
 
+typedef QSharedPointer<ToolChainNode> ToolChainNodePtr;
+
 class ToolChainNode
 {
 public:
-    explicit ToolChainNode(ToolChainNode *p, ToolChain *tc = 0, bool c = false) :
+    explicit ToolChainNode(ToolChain *tc = 0, bool c = false) :
-        parent(p), toolChain(tc), changed(c)
+         parent(0), toolChain(tc), changed(c)
     {
-        if (p)
-            p->childNodes.append(this);
         widget = tc ? tc->configurationWidget() : 0;
         if (widget) {
             widget->setEnabled(tc ? !tc->isAutoDetected() : false);
@@ -70,39 +71,60 @@ public:
 
     ~ToolChainNode()
     {
-        qDeleteAll(childNodes);
         // Do not delete toolchain, we do not own it.
         delete widget;
     }
 
+    void addChild(const ToolChainNodePtr &c)
+    {
+        c->parent = this;
+        childNodes.push_back(c);
+    }
+
     ToolChainNode *parent;
     QString newName;
-    QList<ToolChainNode *> childNodes;
+    QList<ToolChainNodePtr> childNodes;
     ToolChain *toolChain;
     ToolChainConfigWidget *widget;
     bool changed;
 };
 
+static int indexOfToolChain(const QList<ToolChainNodePtr> &l, const ToolChain *tc)
+{
+    const int count = l.size();
+    for (int i = 0; i < count ; i++)
+    if (l.at(i)->toolChain == tc)
+        return i;
+    return -1;
+}
+
+static int indexOfToolChainNode(const QList<ToolChainNodePtr> &l, const ToolChainNode *node)
+{
+    const int count = l.size();
+    for (int i = 0; i < count ; i++)
+    if (l.at(i).data() == node)
+        return i;
+    return -1;
+}
+
 // --------------------------------------------------------------------------
 // ToolChainModel
 // --------------------------------------------------------------------------
 
 ToolChainModel::ToolChainModel(QObject *parent) :
-    QAbstractItemModel(parent)
+    QAbstractItemModel(parent), m_autoRoot(new ToolChainNode), m_manualRoot(new ToolChainNode)
 {
     connect(ToolChainManager::instance(), SIGNAL(toolChainAdded(ProjectExplorer::ToolChain*)),
             this, SLOT(addToolChain(ProjectExplorer::ToolChain*)));
     connect(ToolChainManager::instance(), SIGNAL(toolChainRemoved(ProjectExplorer::ToolChain*)),
             this, SLOT(removeToolChain(ProjectExplorer::ToolChain*)));
 
-    m_autoRoot = new ToolChainNode(0);
-    m_manualRoot = new ToolChainNode(0);
-
     foreach (ToolChain *tc, ToolChainManager::instance()->toolChains()) {
-        if (tc->isAutoDetected())
+        if (tc->isAutoDetected()) {
-            new ToolChainNode(m_autoRoot, tc);
+            m_autoRoot->addChild(ToolChainNodePtr(new ToolChainNode(tc)));
-        else {
+       } else {
-            ToolChainNode *node = new ToolChainNode(m_manualRoot, tc);
+            ToolChainNodePtr node(new ToolChainNode(tc));
+            m_manualRoot->addChild(node);
             if (node->widget)
                 connect(node->widget, SIGNAL(dirty(ProjectExplorer::ToolChain*)),
                         this, SLOT(setDirty(ProjectExplorer::ToolChain*)));
@@ -112,21 +134,19 @@ ToolChainModel::ToolChainModel(QObject *parent) :
 
 ToolChainModel::~ToolChainModel()
 {
-    delete m_autoRoot;
-    delete m_manualRoot;
 }
 
 QModelIndex ToolChainModel::index(int row, int column, const QModelIndex &parent) const
 {
     if (!parent.isValid()) {
         if (row == 0)
-            return createIndex(0, 0, static_cast<void *>(m_autoRoot));
+            return createIndex(0, 0, static_cast<void *>(m_autoRoot.data()));
         else
-            return createIndex(1, 0, static_cast<void *>(m_manualRoot));
+            return createIndex(1, 0, static_cast<void *>(m_manualRoot.data()));
     }
     ToolChainNode *node = static_cast<ToolChainNode *>(parent.internalPointer());
     if (row < node->childNodes.count() && column < 2)
-        return createIndex(row, column, static_cast<void *>(node->childNodes.at(row)));
+        return createIndex(row, column, static_cast<void *>(node->childNodes.at(row).data()));
     else
         return QModelIndex();
 }
@@ -252,7 +272,7 @@ ToolChainConfigWidget *ToolChainModel::widget(const QModelIndex &index)
 
 bool ToolChainModel::isDirty() const
 {
-    foreach (ToolChainNode *n, m_manualRoot->childNodes) {
+    foreach (const ToolChainNodePtr &n, m_manualRoot->childNodes) {
         if (n->changed)
             return true;
     }
@@ -261,7 +281,7 @@ bool ToolChainModel::isDirty() const
 
 bool ToolChainModel::isDirty(ToolChain *tc) const
 {
-    foreach (ToolChainNode *n, m_manualRoot->childNodes) {
+    foreach (const ToolChainNodePtr &n, m_manualRoot->childNodes) {
         if (n->toolChain == tc && n->changed)
             return true;
     }
@@ -270,10 +290,10 @@ bool ToolChainModel::isDirty(ToolChain *tc) const
 
 void ToolChainModel::setDirty(ToolChain *tc)
 {
-    foreach (ToolChainNode *n, m_manualRoot->childNodes) {
+    foreach (const ToolChainNodePtr &n, m_manualRoot->childNodes) {
         if (n->toolChain == tc) {
             n->changed = true;
-            emit dataChanged(index(n, 0), index(n, columnCount(QModelIndex())));
+            emit dataChanged(index(n.data(), 0), index(n.data(), columnCount(QModelIndex())));
         }
     }
 }
@@ -281,15 +301,15 @@ void ToolChainModel::setDirty(ToolChain *tc)
 void ToolChainModel::apply()
 {
     // Remove unused ToolChains:
-    QList<ToolChainNode *> nodes = m_toRemoveList;
+    QList<ToolChainNodePtr> nodes = m_toRemoveList;
-    foreach (ToolChainNode *n, nodes) {
+    foreach (const ToolChainNodePtr &n, nodes) {
         ToolChainManager::instance()->deregisterToolChain(n->toolChain);
     }
-    Q_ASSERT(m_toRemoveList.isEmpty());
+    QTC_ASSERT(m_toRemoveList.isEmpty(), /*  */ );
 
     // Update toolchains:
-    foreach (ToolChainNode *n, m_manualRoot->childNodes) {
+    foreach (const ToolChainNodePtr &n, m_manualRoot->childNodes) {
-        Q_ASSERT(n);
+        Q_ASSERT(n.data());
         if (n->changed) {
             Q_ASSERT(n->toolChain);
             if (!n->newName.isEmpty())
@@ -298,72 +318,66 @@ void ToolChainModel::apply()
                 n->widget->apply();
             n->changed = false;
 
-            emit dataChanged(index(n, 0), index(n, columnCount(QModelIndex())));
+            emit dataChanged(index(n.data(), 0), index(n.data(), columnCount(QModelIndex())));
         }
     }
 
     // Add new (and already updated) toolchains
     nodes = m_toAddList;
-    foreach (ToolChainNode *n, nodes) {
+    foreach (const ToolChainNodePtr &n, nodes) {
         ToolChainManager::instance()->registerToolChain(n->toolChain);
     }
-    Q_ASSERT(m_toAddList.isEmpty());
+    QTC_ASSERT(m_toAddList.isEmpty(), qDebug() <<  m_toAddList.front()->toolChain; );
 }
 
 void ToolChainModel::discard()
 {
     // Remove newly "added" toolchains:
-    foreach (ToolChainNode *n, m_toAddList) {
+    foreach (const ToolChainNodePtr &n, m_toAddList) {
-        int pos = m_manualRoot->childNodes.indexOf(n);
+        const int pos = indexOfToolChainNode(m_manualRoot->childNodes, n.data());
-        Q_ASSERT(pos >= 0);
+        QTC_ASSERT(pos >= 0, continue ; );
         m_manualRoot->childNodes.removeAt(pos);
-
         // Clean up Node: We still own the toolchain!
         delete n->toolChain;
         n->toolChain = 0;
     }
-    qDeleteAll(m_toAddList);
     m_toAddList.clear();
 
     // Add "removed" toolchains again:
-    foreach (ToolChainNode *n, m_toRemoveList) {
+    foreach (const ToolChainNodePtr &n, m_toRemoveList) {
-        m_manualRoot->childNodes.append(n);
+        m_manualRoot->addChild(n);
     }
     m_toRemoveList.clear();
 
     // Reset toolchains:
-    foreach (ToolChainNode *n, m_manualRoot->childNodes) {
+    foreach (const ToolChainNodePtr &n, m_manualRoot->childNodes) {
-        Q_ASSERT(n);
         n->newName.clear();
         if (n->widget)
             n->widget->discard();
         n->changed = false;
     }
+    reset();
 }
 
 void ToolChainModel::markForRemoval(ToolChain *tc)
 {
-    ToolChainNode *node = 0;
+    // Delete newly added item?
-    foreach (ToolChainNode *n, m_manualRoot->childNodes) {
+    const int tcIndex = indexOfToolChain(m_manualRoot->childNodes, tc);
-        if (n->toolChain == tc) {
+    if (tcIndex != -1) {
-            node = n;
+        m_toRemoveList.append(m_manualRoot->childNodes.at(tcIndex));
-            break;
+        emit beginRemoveRows(index(m_manualRoot.data()), tcIndex, tcIndex);
-        }
+        m_manualRoot->childNodes.removeAt(tcIndex);
-    }
-    if (node) {
-        m_toRemoveList.append(node);
-        emit beginRemoveRows(index(m_manualRoot), m_manualRoot->childNodes.indexOf(node), m_manualRoot->childNodes.indexOf(node));
-        m_manualRoot->childNodes.removeOne(node);
         emit endRemoveRows();
     }
 }
 
 void ToolChainModel::markForAddition(ToolChain *tc)
 {
-    int pos = m_manualRoot->childNodes.size();
+    const int pos = m_manualRoot->childNodes.size();
-    emit beginInsertRows(index(m_manualRoot), pos, pos);
+    emit beginInsertRows(index(m_manualRoot.data()), pos, pos);
 
-    ToolChainNode *node = new ToolChainNode(m_manualRoot, tc);
+    const ToolChainNodePtr node(new ToolChainNode(tc));
+    m_manualRoot->addChild(node);
     node->changed = true;
     m_toAddList.append(node);
 
@@ -373,29 +387,28 @@ void ToolChainModel::markForAddition(ToolChain *tc)
 QModelIndex ToolChainModel::index(ToolChainNode *node, int column) const
 {
     if (!node->parent)
-        return index(node == m_autoRoot ? 0 : 1, column, QModelIndex());
+        return index(node == m_autoRoot.data() ? 0 : 1, column, QModelIndex());
-    else
+    else {
-        return index(node->parent->childNodes.indexOf(node), column, index(node->parent));
+        const int tcIndex = indexOfToolChainNode(node->parent->childNodes, node);
+        QTC_ASSERT(tcIndex != -1, return QModelIndex());
+        return index(tcIndex, column, index(node->parent));
+    }
 }
 
 void ToolChainModel::addToolChain(ToolChain *tc)
 {
-    QList<ToolChainNode *> nodes = m_toAddList;
+    const int tcIndex = indexOfToolChain(m_toAddList, tc);
-    foreach (ToolChainNode *n, nodes) {
+    if (tcIndex != -1) {
-        if (n->toolChain == tc) {
+        m_toAddList.removeAt(tcIndex);
-            m_toAddList.removeOne(n);
-            // do not delete n: Still used elsewhere!
         return;
     }
-    }
+    const ToolChainNodePtr parent = tc->isAutoDetected() ? m_autoRoot : m_manualRoot;
+    QTC_ASSERT(indexOfToolChain(parent->childNodes, tc) == -1, return ; )
 
-    ToolChainNode *parent = m_manualRoot;
+    const int row = parent->childNodes.count();
-    if (tc->isAutoDetected())
+    beginInsertRows(index(parent.data()), row, row);
-        parent = m_autoRoot;
+    ToolChainNodePtr newNode(new ToolChainNode(tc, true));
-    int row = parent->childNodes.count();
+    parent->addChild(newNode);
-
-    beginInsertRows(index(parent), row, row);
-    new ToolChainNode(parent, tc, true);
     endInsertRows();
 
     emit toolChainStateChanged();
@@ -403,32 +416,17 @@ void ToolChainModel::addToolChain(ToolChain *tc)
 
 void ToolChainModel::removeToolChain(ToolChain *tc)
 {
-    QList<ToolChainNode *> nodes = m_toRemoveList;
+    const int tcIndex = indexOfToolChain(m_toRemoveList, tc);
-    foreach (ToolChainNode *n, nodes) {
+    if (tcIndex != -1)
-        if (n->toolChain == tc) {
+        m_toRemoveList.removeAt(tcIndex);
-            m_toRemoveList.removeOne(n);
-            delete n;
-            return;
-        }
-    }
 
-    ToolChainNode *parent = m_manualRoot;
+    const ToolChainNodePtr parent = tc->isAutoDetected() ? m_autoRoot : m_manualRoot;
-    if (tc->isAutoDetected())
+    const int row = indexOfToolChain(parent->childNodes, tc);
-        parent = m_autoRoot;
+    if (row != -1) {
-    int row = 0;
+        beginRemoveRows(index(parent.data()), row, row);
-    ToolChainNode *node = 0;
-    foreach (ToolChainNode *current, parent->childNodes) {
-        if (current->toolChain == tc) {
-            node = current;
-            break;
-        }
-        ++row;
-    }
-
-    beginRemoveRows(index(parent), row, row);
         parent->childNodes.removeAt(row);
-    delete node;
         endRemoveRows();
+    }
 
     emit toolChainStateChanged();
 }

src/plugins/projectexplorer/toolchainoptionspage.h

@@ -39,6 +39,7 @@
 #include <coreplugin/dialogs/ioptionspage.h>
 
 #include <QtCore/QAbstractItemModel>
+#include <QtCore/QSharedPointer>
 
 QT_BEGIN_NAMESPACE
 class QTreeWidgetItem;
@@ -101,12 +102,11 @@ private slots:
 private:
     QModelIndex index(ToolChainNode *, int column = 0) const;
 
-    ToolChainNode * m_root;
+    QSharedPointer<ToolChainNode> m_autoRoot;
-    ToolChainNode * m_autoRoot;
+    QSharedPointer<ToolChainNode> m_manualRoot;
-    ToolChainNode * m_manualRoot;
 
-    QList<ToolChainNode *> m_toAddList;
+    QList<QSharedPointer<ToolChainNode> > m_toAddList;
-    QList<ToolChainNode *> m_toRemoveList;
+    QList<QSharedPointer<ToolChainNode> > m_toRemoveList;
 };
 
 // --------------------------------------------------------------------------