Fixes visibility update missing when doing setParentItem on graphicsitem

Calling setParentItem is causing the previous opacity/visible updates
to be discarded because the dirty flags were not propagated to the new parent.

Also removed some unnecessary 'markDirty' and 'update' calls.

Task-number: QTBUG-6738
Reviewed-by: bnilsen

4 files changed, 127 insertions, 36 deletions

diff --git a/src/gui/graphicsview/qgraphicsitem.cpp b/src/gui/graphicsview/qgraphicsitem.cpp
index 66493b7..dc20faf 100644
--- a/src/gui/graphicsview/qgraphicsitem.cpp
+++ b/src/gui/graphicsview/qgraphicsitem.cpp
@@ -1113,11 +1113,9 @@ void QGraphicsItemPrivate::setParentItemHelper(QGraphicsItem *newParent, const Q
     }
 
     if ((parent = newParent)) {
-        bool implicitUpdate = false;
         if (parent->d_func()->scene && parent->d_func()->scene != scene) {
             // Move this item to its new parent's scene
             parent->d_func()->scene->addItem(q);
-            implicitUpdate = true;
         } else if (!parent->d_func()->scene && scene) {
             // Remove this item from its former scene
             scene->removeItem(q);
@@ -1127,25 +1125,25 @@ void QGraphicsItemPrivate::setParentItemHelper(QGraphicsItem *newParent, const Q
         if (thisPointerVariant)
             parent->itemChange(QGraphicsItem::ItemChildAddedChange, *thisPointerVariant);
         if (scene) {
-            if (!implicitUpdate)
-                scene->d_func()->markDirty(q_ptr);
-
             // Re-enable scene pos notifications for new ancestors
             if (scenePosDescendants || (flags & QGraphicsItem::ItemSendsScenePositionChanges))
                 scene->d_func()->setScenePosItemEnabled(q, true);
         }
 
+        // Propagate dirty flags to the new parent
+        markParentDirty(/*updateBoundingRect=*/true);
+
         // Inherit ancestor flags from the new parent.
         updateAncestorFlags();
 
         // Update item visible / enabled.
         if (parent->d_ptr->visible != visible) {
             if (!parent->d_ptr->visible || !explicitlyHidden)
-                setVisibleHelper(parent->d_ptr->visible, /* explicit = */ false, /* update = */ !implicitUpdate);
+                setVisibleHelper(parent->d_ptr->visible, /* explicit = */ false, /* update = */ false);
         }
         if (parent->isEnabled() != enabled) {
             if (!parent->d_ptr->enabled || !explicitlyDisabled)
-                setEnabledHelper(parent->d_ptr->enabled, /* explicit = */ false, /* update = */ !implicitUpdate);
+                setEnabledHelper(parent->d_ptr->enabled, /* explicit = */ false, /* update = */ false);
         }
 
         // Auto-activate if visible and the parent is active.
@@ -1161,10 +1159,6 @@ void QGraphicsItemPrivate::setParentItemHelper(QGraphicsItem *newParent, const Q
                 setVisibleHelper(true, /* explicit = */ false);
             if (!enabled && !explicitlyDisabled)
                 setEnabledHelper(true, /* explicit = */ false);
-
-            // If the item is being deleted, the whole scene will be updated.
-            if (scene)
-                scene->d_func()->markDirty(q_ptr);
         }
     }
 
@@ -7263,19 +7257,7 @@ void QGraphicsItem::prepareGeometryChange()
         }
     }
 
-    QGraphicsItem *parent = this;
-    while ((parent = parent->d_ptr->parent)) {
-        QGraphicsItemPrivate *parentp = parent->d_ptr.data();
-        parentp->dirtyChildrenBoundingRect = 1;
-        // ### Only do this if the parent's effect applies to the entire subtree.
-        parentp->notifyBoundingRectChanged = 1;
-#ifndef QT_NO_GRAPHICSEFFECT
-        if (parentp->scene && parentp->graphicsEffect) {
-            parentp->notifyInvalidated = 1;
-            static_cast<QGraphicsItemEffectSourcePrivate *>(parentp->graphicsEffect->d_func()->source->d_func())->invalidateCache();
-        }
-#endif
-    }
+    d_ptr->markParentDirty(/*updateBoundingRect=*/true);
 }
 
 /*!
diff --git a/src/gui/graphicsview/qgraphicsitem_p.h b/src/gui/graphicsview/qgraphicsitem_p.h
index 986a977..5ad6cd5 100644
--- a/src/gui/graphicsview/qgraphicsitem_p.h
+++ b/src/gui/graphicsview/qgraphicsitem_p.h
@@ -406,6 +406,8 @@ public:
         return !visible || (childrenCombineOpacity() && isFullyTransparent());
     }
 
+    inline void markParentDirty(bool updateBoundingRect = false);
+
     void setFocusHelper(Qt::FocusReason focusReason, bool climb);
     void setSubFocus(QGraphicsItem *rootItem = 0);
     void clearSubFocus(QGraphicsItem *rootItem = 0);
@@ -754,6 +756,37 @@ inline bool QGraphicsItemPrivate::insertionOrder(QGraphicsItem *a, QGraphicsItem
     return a->d_ptr->siblingIndex < b->d_ptr->siblingIndex;
 }
 
+/*!
+    \internal
+*/
+inline void QGraphicsItemPrivate::markParentDirty(bool updateBoundingRect)
+{
+    QGraphicsItemPrivate *parentp = this;
+    while (parentp->parent) {
+        parentp = parentp->parent->d_ptr.data();
+        parentp->dirtyChildren = 1;
+
+        if (updateBoundingRect) {
+            parentp->dirtyChildrenBoundingRect = 1;
+            // ### Only do this if the parent's effect applies to the entire subtree.
+            parentp->notifyBoundingRectChanged = 1;
+        }
+#ifndef QT_NO_GRAPHICSEFFECT
+        if (parentp->graphicsEffect) {
+            if (updateBoundingRect) {
+                parentp->notifyInvalidated = 1;
+                static_cast<QGraphicsItemEffectSourcePrivate *>(parentp->graphicsEffect->d_func()
+                                                                ->source->d_func())->invalidateCache();
+            }
+            if (parentp->graphicsEffect->isEnabled()) {
+                parentp->dirty = 1;
+                parentp->fullUpdatePending = 1;
+            }
+        }
+#endif
+    }
+}
+
 QT_END_NAMESPACE
 
 #endif // QT_NO_GRAPHICSVIEW
diff --git a/src/gui/graphicsview/qgraphicsscene.cpp b/src/gui/graphicsview/qgraphicsscene.cpp
index ae48bee..9219773 100644
--- a/src/gui/graphicsview/qgraphicsscene.cpp
+++ b/src/gui/graphicsview/qgraphicsscene.cpp
@@ -1955,7 +1955,7 @@ QList<QGraphicsItem *> QGraphicsScene::items(const QRectF &rectangle, Qt::ItemSe
     \since 4.3
 
     This convenience function is equivalent to calling items(QRectF(\a x, \a y, \a w, \a h), \a mode).
-    
+
     This function is deprecated and returns incorrect results if the scene
     contains items that ignore transformations. Use the overload that takes
     a QTransform instead.
@@ -4903,17 +4903,7 @@ void QGraphicsScenePrivate::markDirty(QGraphicsItem *item, const QRectF &rect, b
     if (ignoreOpacity)
         item->d_ptr->ignoreOpacity = 1;
 
-    QGraphicsItem *p = item->d_ptr->parent;
-    while (p) {
-        p->d_ptr->dirtyChildren = 1;
-#ifndef QT_NO_GRAPHICSEFFECT
-        if (p->d_ptr->graphicsEffect && p->d_ptr->graphicsEffect->isEnabled()) {
-            p->d_ptr->dirty = 1;
-            p->d_ptr->fullUpdatePending = 1;
-        }
-#endif //QT_NO_GRAPHICSEFFECT
-        p = p->d_ptr->parent;
-    }
+    item->d_ptr->markParentDirty();
 }
 
 static inline bool updateHelper(QGraphicsViewPrivate *view, QGraphicsItemPrivate *item,
diff --git a/tests/auto/qgraphicsitem/tst_qgraphicsitem.cpp b/tests/auto/qgraphicsitem/tst_qgraphicsitem.cpp
index 8e43bce..14b9ef0 100644
--- a/tests/auto/qgraphicsitem/tst_qgraphicsitem.cpp
+++ b/tests/auto/qgraphicsitem/tst_qgraphicsitem.cpp
@@ -316,6 +316,7 @@ private slots:
     void childrenBoundingRectTransformed();
     void childrenBoundingRect2();
     void childrenBoundingRect3();
+    void childrenBoundingRect4();
     void group();
     void setGroup();
     void setGroup2();
@@ -417,6 +418,7 @@ private slots:
     void task197802_childrenVisibility();
     void QTBUG_4233_updateCachedWithSceneRect();
     void QTBUG_5418_textItemSetDefaultColor();
+    void QTBUG_6738_missingUpdateWithSetParent();
 
 private:
     QList<QGraphicsItem *> paintedItems;
@@ -3257,6 +3259,32 @@ void tst_QGraphicsItem::childrenBoundingRect3()
     QCOMPARE(subTreeRect.height(), qreal(251.7766952966369));
 }
 
+void tst_QGraphicsItem::childrenBoundingRect4()
+{
+    QGraphicsScene scene;
+
+    QGraphicsRectItem *rect = scene.addRect(QRectF(0, 0, 10, 10));
+    QGraphicsRectItem *rect2 = scene.addRect(QRectF(0, 0, 20, 20));
+    QGraphicsRectItem *rect3 = scene.addRect(QRectF(0, 0, 30, 30));
+    rect2->setParentItem(rect);
+    rect3->setParentItem(rect);
+
+    QGraphicsView view(&scene);
+    view.show();
+
+    QTest::qWaitForWindowShown(&view);
+
+    // Try to mess up the cached bounding rect.
+    rect->childrenBoundingRect();
+    rect2->childrenBoundingRect();
+
+    rect3->setOpacity(0.0);
+    rect3->setParentItem(rect2);
+
+    QCOMPARE(rect->childrenBoundingRect(), rect3->boundingRect());
+    QCOMPARE(rect2->childrenBoundingRect(), rect3->boundingRect());
+}
+
 void tst_QGraphicsItem::group()
 {
     QGraphicsScene scene;
@@ -9869,5 +9897,63 @@ void  tst_QGraphicsItem::QTBUG_5418_textItemSetDefaultColor()
     QCOMPARE(i->painted, 0); //same color as before should not trigger an update (QTBUG-6242)
 }
 
+void tst_QGraphicsItem::QTBUG_6738_missingUpdateWithSetParent()
+{
+    // In all 3 test cases below the reparented item should disappear
+    EventTester *parent = new EventTester;
+    EventTester *child = new EventTester(parent);
+    EventTester *child2 = new EventTester(parent);
+    EventTester *child3 = new EventTester(parent);
+    EventTester *child4 = new EventTester(parent);
+
+    child->setPos(10, 10);
+    child2->setPos(20, 20);
+    child3->setPos(30, 30);
+    child4->setPos(40, 40);
+
+    QGraphicsScene scene;
+    scene.addItem(parent);
+
+    class MyGraphicsView : public QGraphicsView
+    { public:
+        int repaints;
+        QRegion paintedRegion;
+        MyGraphicsView(QGraphicsScene *scene) : QGraphicsView(scene), repaints(0) {}
+        void paintEvent(QPaintEvent *e)
+        {
+            ++repaints;
+            paintedRegion += e->region();
+            QGraphicsView::paintEvent(e);
+        }
+        void reset() { repaints = 0; paintedRegion = QRegion(); }
+    };
+
+    MyGraphicsView view(&scene);
+    view.show();
+    QTest::qWaitForWindowShown(&view);
+    QTRY_VERIFY(view.repaints > 0);
+
+    // test case #1
+    view.reset();
+    child2->setVisible(false);
+    child2->setParentItem(child);
+
+    QTRY_VERIFY(view.repaints == 1);
+
+    // test case #2
+    view.reset();
+    child3->setOpacity(0.0);
+    child3->setParentItem(child);
+
+    QTRY_VERIFY(view.repaints == 1);
+
+    // test case #3
+    view.reset();
+    child4->setParentItem(child);
+    child4->setVisible(false);
+
+    QTRY_VERIFY(view.repaints == 1);
+}
+
 QTEST_MAIN(tst_QGraphicsItem)
 #include "tst_qgraphicsitem.moc"