QGraphicsItem: cached embedded widget item is not repainted when widget is updated

When calling QGraphicsItem::update() on a cached item, the cache is
meant to be invalidated.

In the reported bug, the user had a fixed scene rect
set for his scene, and removing an item caused the entire scene to be
updated (marked as "all needs to be updated"). In this case, calling
update() on the cached item did not cause the item's cache to be
invalidated. The item's new appearance didn't show up until the next
invalidation, which was the same call to update(), but this time without
a preceeding full scene update.

The fix is to always invalidate the cache, regardless. But only
schedule a repaint of the item in some cases (e.g., in this case the
whole scene was marked for update, in which case it's unnessary for this
one item to schedule a repaint of itself).

It's worth noting that in 4.6, removing an item be delete does not cause
the whole scene to be updated, and because of that this error was not
exposed. It's there nevertheless.

Reviewed-by: bnilsen

2 files changed, 41 insertions, 16 deletions

diff --git a/src/gui/graphicsview/qgraphicsitem.cpp b/src/gui/graphicsview/qgraphicsitem.cpp
index 5799fe7..4f77aa8 100644
--- a/src/gui/graphicsview/qgraphicsitem.cpp
+++ b/src/gui/graphicsview/qgraphicsitem.cpp
@@ -3975,26 +3975,20 @@ void QGraphicsItem::update(const QRectF &rect)
         return;
 
     if (CacheMode(d_ptr->cacheMode) != NoCache) {
+        // Invalidate cache.
         QGraphicsItemCache *cache = d_ptr->extraItemCache();
-        if (d_ptr->discardUpdateRequest(/* ignoreVisibleBit = */ false,
-                                        /* ignoreClipping = */ false,
-                                        /* ignoreDirtyBit = */ true)) {
-            return;
+        if (!cache->allExposed) {
+            if (rect.isNull()) {
+                cache->allExposed = true;
+                cache->exposed.clear();
+            } else {
+                cache->exposed.append(rect);
+            }
         }
+    }
 
-        // Invalidate cache.
-        if (rect.isNull()) {
-            cache->allExposed = true;
-            cache->exposed.clear();
-        } else {
-            cache->exposed.append(rect);
-        }
-        // Only invalidate cache; item is already dirty.
-        if (d_ptr->dirty)
-            return;
-    } else if (d_ptr->discardUpdateRequest()) {
+    if (d_ptr->discardUpdateRequest())
         return;
-    }
 
     // Effectively the same as updateHelper(rect);
     if (rect.isNull())
diff --git a/tests/auto/qgraphicsitem/tst_qgraphicsitem.cpp b/tests/auto/qgraphicsitem/tst_qgraphicsitem.cpp
index 391ccf8..55e9b34 100644
--- a/tests/auto/qgraphicsitem/tst_qgraphicsitem.cpp
+++ b/tests/auto/qgraphicsitem/tst_qgraphicsitem.cpp
@@ -56,6 +56,7 @@
 #include <QPainter>
 #include <QScrollBar>
 #include <QVBoxLayout>
+#include "../../shared/util.h"
 
 //TESTED_CLASS=
 //TESTED_FILES=
@@ -230,6 +231,7 @@ private slots:
     void task240400_clickOnTextItem();
     void task243707_addChildBeforeParent();
     void task197802_childrenVisibility();
+    void QTBUG_4233_updateCachedWithSceneRect();
 };
 
 void tst_QGraphicsItem::init()
@@ -6454,5 +6456,34 @@ void tst_QGraphicsItem::deviceTransform()
     QCOMPARE(rect3->deviceTransform(deviceX).map(QPointF(50, 50)), mapResult3);
 }
 
+void tst_QGraphicsItem::QTBUG_4233_updateCachedWithSceneRect()
+{
+    EventTester *tester = new EventTester;
+    tester->setCacheMode(QGraphicsItem::ItemCoordinateCache);
+
+    QGraphicsScene scene;
+    scene.addItem(tester);
+    scene.setSceneRect(-100, -100, 200, 200); // contains the tester item
+
+    QGraphicsView view(&scene);
+    view.show();
+    QTRY_COMPARE(QApplication::activeWindow(), (QWidget *)&view);
+
+    QCOMPARE(tester->repaints, 1);
+
+    scene.update(); // triggers "updateAll" optimization
+    qApp->processEvents();
+    qApp->processEvents(); // in 4.6 only one processEvents is necessary
+
+    QCOMPARE(tester->repaints, 1);
+
+    scene.update(); // triggers "updateAll" optimization
+    tester->update();
+    qApp->processEvents();
+    qApp->processEvents(); // in 4.6 only one processEvents is necessary
+
+    QCOMPARE(tester->repaints, 2);
+}
+
 QTEST_MAIN(tst_QGraphicsItem)
 #include "tst_qgraphicsitem.moc"