From ff01481900f1d19d392c8ed8fe0f3b5c85751b8e Mon Sep 17 00:00:00 2001 From: Andreas Aardal Hanssen Date: Thu, 13 Aug 2009 12:20:52 +0200 Subject: Fix focus proxy deletion bugs/crashes in QGraphicsItem. This change would have been much simpler if either QGraphicsItem inherited QObject, or if we had some similar QPointer-like class that supported QGraphicsItem. The issue is this: Each item can delegate another item to be its focus proxy. That item can be a parent or child, or something completely unrelated. Either of the two items can be deleted independently. The former solution was to store backpointers in a map in the scene. Problem is, the items may not be in a scene when this happens, they may be removed from the scene, and the items may be moved between two scenes. The bad part about this fix is that it adds another pointer to QGraphicsItemPrivate. Reviewed-by: Shane Kearns --- src/gui/graphicsview/qgraphicsitem.cpp | 29 +++++++++++++----- src/gui/graphicsview/qgraphicsitem_p.h | 2 ++ src/gui/graphicsview/qgraphicsscene.cpp | 6 +--- src/gui/graphicsview/qgraphicsscene_p.h | 1 - tests/auto/qgraphicsitem/tst_qgraphicsitem.cpp | 42 ++++++++++++++++++++++++++ 5 files changed, 66 insertions(+), 14 deletions(-) diff --git a/src/gui/graphicsview/qgraphicsitem.cpp b/src/gui/graphicsview/qgraphicsitem.cpp index c3e5501..537dab7 100644 --- a/src/gui/graphicsview/qgraphicsitem.cpp +++ b/src/gui/graphicsview/qgraphicsitem.cpp @@ -1196,10 +1196,12 @@ QGraphicsItem::~QGraphicsItem() Q_ASSERT(d_ptr->children.isEmpty()); } - if (d_ptr->scene) + if (d_ptr->scene) { d_ptr->scene->d_func()->removeItemHelper(this); - else + } else { + d_ptr->resetFocusProxy(); d_ptr->setParentItemHelper(0); + } if (d_ptr->transformData) { for(int i = 0; i < d_ptr->transformData->graphicsTransforms.size(); ++i) { @@ -2613,13 +2615,11 @@ void QGraphicsItem::setFocusProxy(QGraphicsItem *item) } QGraphicsItem *lastFocusProxy = d_ptr->focusProxy; + if (lastFocusProxy) + lastFocusProxy->d_ptr->focusProxyRefs.removeOne(&d_ptr->focusProxy); d_ptr->focusProxy = item; - if (d_ptr->scene) { - if (lastFocusProxy) - d_ptr->scene->d_func()->focusProxyReverseMap.remove(lastFocusProxy, this); - if (item) - d_ptr->scene->d_func()->focusProxyReverseMap.insert(item, this); - } + if (item) + item->d_ptr->focusProxyRefs << &d_ptr->focusProxy; } /*! @@ -4626,6 +4626,19 @@ void QGraphicsItemPrivate::clearSubFocus() /*! \internal + Sets the focusProxy pointer to 0 for all items that have this item as their + focusProxy. ### Qt 5: Use QPointer instead. +*/ +void QGraphicsItemPrivate::resetFocusProxy() +{ + for (int i = 0; i < focusProxyRefs.size(); ++i) + *focusProxyRefs.at(i) = 0; + focusProxyRefs.clear(); +} + +/*! + \internal + Tells us if it is a proxy widget */ bool QGraphicsItemPrivate::isProxyWidget() const diff --git a/src/gui/graphicsview/qgraphicsitem_p.h b/src/gui/graphicsview/qgraphicsitem_p.h index 982cdfc..c654d4f 100644 --- a/src/gui/graphicsview/qgraphicsitem_p.h +++ b/src/gui/graphicsview/qgraphicsitem_p.h @@ -398,6 +398,7 @@ public: void setSubFocus(); void clearSubFocus(); + void resetFocusProxy(); inline QTransform transformToParent() const; inline void ensureSortedChildren(); @@ -419,6 +420,7 @@ public: int siblingIndex; int depth; QGraphicsItem *focusProxy; + QList focusProxyRefs; QGraphicsItem *subFocusItem; Qt::InputMethodHints imHints; diff --git a/src/gui/graphicsview/qgraphicsscene.cpp b/src/gui/graphicsview/qgraphicsscene.cpp index bdd1ac6..2178850 100644 --- a/src/gui/graphicsview/qgraphicsscene.cpp +++ b/src/gui/graphicsview/qgraphicsscene.cpp @@ -494,11 +494,7 @@ void QGraphicsScenePrivate::removeItemHelper(QGraphicsItem *item) item->d_func()->scene = 0; // Unregister focus proxy. - QMultiHash::iterator it = focusProxyReverseMap.find(item); - while (it != focusProxyReverseMap.end() && it.key() == item) { - it.value()->d_ptr->focusProxy = 0; - it = focusProxyReverseMap.erase(it); - } + item->d_ptr->resetFocusProxy(); // Remove from parent, or unregister from toplevels. if (QGraphicsItem *parentItem = item->parentItem()) { diff --git a/src/gui/graphicsview/qgraphicsscene_p.h b/src/gui/graphicsview/qgraphicsscene_p.h index 8b53306..836522d 100644 --- a/src/gui/graphicsview/qgraphicsscene_p.h +++ b/src/gui/graphicsview/qgraphicsscene_p.h @@ -131,7 +131,6 @@ public: QGraphicsWidget *activeWindow; int activationRefCount; void setFocusItemHelper(QGraphicsItem *item, Qt::FocusReason focusReason); - QMultiHash focusProxyReverseMap; QList popupWidgets; void addPopup(QGraphicsWidget *widget); diff --git a/tests/auto/qgraphicsitem/tst_qgraphicsitem.cpp b/tests/auto/qgraphicsitem/tst_qgraphicsitem.cpp index eadf8b7..a623b50 100644 --- a/tests/auto/qgraphicsitem/tst_qgraphicsitem.cpp +++ b/tests/auto/qgraphicsitem/tst_qgraphicsitem.cpp @@ -280,6 +280,7 @@ private slots: void autoDetectFocusProxy(); void subFocus(); void reverseCreateAutoFocusProxy(); + void focusProxyDeletion(); // task specific tests below me void task141694_textItemEnsureVisible(); @@ -7516,5 +7517,46 @@ void tst_QGraphicsItem::reverseCreateAutoFocusProxy() QVERIFY(text2->hasFocus()); } +void tst_QGraphicsItem::focusProxyDeletion() +{ + QGraphicsRectItem *rect = new QGraphicsRectItem; + QGraphicsRectItem *rect2 = new QGraphicsRectItem; + rect->setFocusProxy(rect2); + QCOMPARE(rect->focusProxy(), (QGraphicsItem *)rect2); + + delete rect2; + QCOMPARE(rect->focusProxy(), (QGraphicsItem *)0); + + rect2 = new QGraphicsRectItem; + rect->setFocusProxy(rect2); + delete rect; // don't crash + + rect = new QGraphicsRectItem; + rect->setFocusProxy(rect2); + QGraphicsScene *scene = new QGraphicsScene; + scene->addItem(rect); + scene->addItem(rect2); + delete rect2; + QCOMPARE(rect->focusProxy(), (QGraphicsItem *)0); + + rect2 = new QGraphicsRectItem; + QTest::ignoreMessage(QtWarningMsg, "QGraphicsItem::setFocusProxy: focus proxy must be in same scene"); + rect->setFocusProxy(rect2); + QCOMPARE(rect->focusProxy(), (QGraphicsItem *)0); + scene->addItem(rect2); + rect->setFocusProxy(rect2); + QCOMPARE(rect->focusProxy(), (QGraphicsItem *)rect2); + delete rect; // don't crash + + rect = new QGraphicsRectItem; + rect2 = new QGraphicsRectItem; + rect->setFocusProxy(rect2); + QCOMPARE(rect->focusProxy(), (QGraphicsItem *)rect2); + scene->addItem(rect); + scene->addItem(rect2); + rect->setFocusProxy(rect2); + delete scene; // don't crash +} + QTEST_MAIN(tst_QGraphicsItem) #include "tst_qgraphicsitem.moc" -- cgit v0.12