From 1373081ce19698e50fdde95ab18012950f729d62 Mon Sep 17 00:00:00 2001 From: "Eduardo M. Fleury" Date: Thu, 28 May 2009 17:19:17 -0300 Subject: QGraphicsAnchorLayout: Update m_vertexList reference count system The reference count system used to delete AnchorVertex objects stored in m_vertexList had an issue that prevented the count values from being stored properly. This is a tentative fix for the memory leaks in the layout, changes were made to the m_vertexList access methods as well as those that use them. 1) addInternalVertex() After incrementing the ref count, store it in m_vertexList. 2) removeInternalVertex() To make it symmetrical to addInternalVertex() we delete the anchor vertex if its reach count reaches zero. From now on, the only methods that create or delete vertices are these two. All other methods should rely on add/removeInternalVertex. Also made the method 'void'. This method is called to release the associated object and may trigger a deletion of it, thus any use of the object reference after that is unsafe. Other methods can get a reference with internalVertex(). 3) removeAnchor() Follow above instructions. No longer relies on the reference returned by removeInternalVertex(), instead, use the reference first then release. Note: In this particular case, we know that "removeEdge(v1, v2)" will only access the contents of the pointers, not the referenced object. Yet, the change probably pays itself for the sake of clarity and safety. 4) removeAnchors() Each tiime an anchor is created, the reference count of its both vertices is incremented, thus we should decrement that count every time we remove an anchor. (Instead of removing v1 only once). Also, leave the task of deleting v1 to removeInternalVertex() as explained above. Removing the constraints handling code. I understand that we cannot leave constraints with dangling pointers inside of them, however that raises an even worse problem. Every time an item is added, we create an item center constraint, thus we must be sure to delete those when the item is removed. Once we do that, we won't have bad constraints laying around. This issue remains open in this commit but will be solved soon. Signed-off-by: Eduardo M. Fleury --- src/gui/graphicsview/qgraphicsanchorlayout_p.cpp | 54 +++++++++++++----------- src/gui/graphicsview/qgraphicsanchorlayout_p.h | 2 +- 2 files changed, 30 insertions(+), 26 deletions(-) diff --git a/src/gui/graphicsview/qgraphicsanchorlayout_p.cpp b/src/gui/graphicsview/qgraphicsanchorlayout_p.cpp index 1f6813f..b0ffea4 100644 --- a/src/gui/graphicsview/qgraphicsanchorlayout_p.cpp +++ b/src/gui/graphicsview/qgraphicsanchorlayout_p.cpp @@ -250,12 +250,13 @@ AnchorVertex *QGraphicsAnchorLayoutPrivate::addInternalVertex(QGraphicsLayoutIte { QPair pair(item, edge); QPair v = m_vertexList.value(pair); + if (!v.first) { Q_ASSERT(v.second == 0); v.first = new AnchorVertex(item, edge); - m_vertexList.insert(pair, v); } v.second++; + m_vertexList.insert(pair, v); return v.first; } @@ -265,20 +266,26 @@ AnchorVertex *QGraphicsAnchorLayoutPrivate::addInternalVertex(QGraphicsLayoutIte * returns the AnchorVertex that was dereferenced, also when it was removed. * returns 0 if it did not exist. */ -AnchorVertex *QGraphicsAnchorLayoutPrivate::removeInternalVertex(QGraphicsLayoutItem *item, - QGraphicsAnchorLayout::Edge edge) +void QGraphicsAnchorLayoutPrivate::removeInternalVertex(QGraphicsLayoutItem *item, + QGraphicsAnchorLayout::Edge edge) { QPair pair(item, edge); QPair v = m_vertexList.value(pair); - if (v.first) { - v.second--; - if (v.second == 0) { - m_vertexList.remove(pair); - } - } else { + + if (!v.first) { qWarning("This item with this edge is not in the graph"); + return; + } + + v.second--; + if (v.second == 0) { + // Remove reference and delete vertex + m_vertexList.remove(pair); + delete v.first; + } else { + // Update reference count + m_vertexList.insert(pair, v); } - return v.first; } void QGraphicsAnchorLayoutPrivate::removeAnchor(QGraphicsLayoutItem *firstItem, @@ -286,14 +293,18 @@ void QGraphicsAnchorLayoutPrivate::removeAnchor(QGraphicsLayoutItem *firstItem, QGraphicsLayoutItem *secondItem, QGraphicsAnchorLayout::Edge secondEdge) { - // Is there a representation for the Vertex (firstItem, firstEdge) - // in our internal structure? - AnchorVertex *v1 = removeInternalVertex(firstItem, firstEdge); - AnchorVertex *v2 = removeInternalVertex(secondItem, secondEdge); + // Look for both vertices + AnchorVertex *v1 = internalVertex(firstItem, firstEdge); + AnchorVertex *v2 = internalVertex(secondItem, secondEdge); + // Remove edge from graph if (v1 && v2) { graph[edgeOrientation(firstEdge)].removeEdge(v1, v2); } + + // Decrease vertices reference count (may trigger a deletion) + removeInternalVertex(firstItem, firstEdge); + removeInternalVertex(secondItem, secondEdge); } void QGraphicsAnchorLayoutPrivate::removeAnchors(QGraphicsLayoutItem *item) @@ -307,22 +318,15 @@ void QGraphicsAnchorLayoutPrivate::removeAnchors(QGraphicsLayoutItem *item) // Remove all vertex for all edges QGraphicsAnchorLayout::Edge e = static_cast(edge); - if ((v1 = removeInternalVertex(item, e))) { + if ((v1 = internalVertex(item, e))) { // Remove all edges allVertex = graph[edgeOrientation(e)].adjacentVertices(v1); - QList constraints = itemCenterConstraints[edgeOrientation(e)]; foreach (v2, allVertex) { - AnchorData *data = graph[edgeOrientation(e)].takeEdge(v1, v2); - Q_ASSERT(data); - for (int i = 0; i < constraints.count(); ++i) { - QSimplexConstraint *c = constraints.at(i); - c->variables.remove(data); - } - delete data; + graph[edgeOrientation(e)].removeEdge(v1, v2); + removeInternalVertex(item, e); + removeInternalVertex(v2->m_item, v2->m_edge); } - qDebug("removing anchor: %s", qPrintable(v1->toString())); - delete v1; } } } diff --git a/src/gui/graphicsview/qgraphicsanchorlayout_p.h b/src/gui/graphicsview/qgraphicsanchorlayout_p.h index ee1a181..27855e6 100644 --- a/src/gui/graphicsview/qgraphicsanchorlayout_p.h +++ b/src/gui/graphicsview/qgraphicsanchorlayout_p.h @@ -276,7 +276,7 @@ public: } AnchorVertex *addInternalVertex(QGraphicsLayoutItem *item, QGraphicsAnchorLayout::Edge edge); - AnchorVertex *removeInternalVertex(QGraphicsLayoutItem *item, QGraphicsAnchorLayout::Edge edge); + void removeInternalVertex(QGraphicsLayoutItem *item, QGraphicsAnchorLayout::Edge edge); // Geometry interpolation methods void setItemsGeometries(); -- cgit v0.12