BT: Fix a regression to 4.4 in Graphics View's handling of child clippers

Regression caused by optimizations in QGraphicsItem and QGraphicsScene.
The changes in QGraphicsItem fix bugs in QGraphicsItem::mapToParent
functions, which did the translation before applying the transformation,
instead of the other way (transform, then translate). This bug caused
almost all mapToParent and mapRectToParent functions to behave wrongly.
Unfortunately the new helper functions in QGraphicsScene for discovering
items made use of these functions, which introduced a regression. Fixing
these functions also fixes item discovery.

The other part of this change fixes a regression caused by c1909321,
which luckily happened after 4.5.0 and never saw the light of day. The
fix is to also invalidate the cached clip path even if there is no scene,
which is necessary if you build your scene graph outside the scene, and
finish off by adding the root item to the scene.

Task-number: 250680
Reviewed-by: Alexis

3 files changed, 125 insertions, 14 deletions

diff --git a/src/gui/graphicsview/qgraphicsitem.cpp b/src/gui/graphicsview/qgraphicsitem.cpp
index b520a3f1dc..e6bcaa6204 100644
--- a/src/gui/graphicsview/qgraphicsitem.cpp
+++ b/src/gui/graphicsview/qgraphicsitem.cpp
@@ -4223,9 +4223,9 @@ QPolygonF QGraphicsItem::mapToItem(const QGraphicsItem *item, const QRectF &rect
 */
 QPolygonF QGraphicsItem::mapToParent(const QRectF &rect) const
 {
-    if (!d_ptr->hasTransform)
-        return QPolygonF(rect.translated(d_ptr->pos));
-    return transform().map(rect.translated(d_ptr->pos));
+    QPolygonF p = !d_ptr->hasTransform ? rect : transform().map(rect);
+    p.translate(d_ptr->pos);
+    return p;
 }
 
 /*!
@@ -4292,8 +4292,8 @@ QRectF QGraphicsItem::mapRectToItem(const QGraphicsItem *item, const QRectF &rec
 */
 QRectF QGraphicsItem::mapRectToParent(const QRectF &rect) const
 {
-    QRectF r = rect.translated(d_ptr->pos.x(), d_ptr->pos.y());
-    return !d_ptr->hasTransform ? r : transform().mapRect(r);
+    QRectF r = !d_ptr->hasTransform ? rect : transform().mapRect(rect);
+    return r.translated(d_ptr->pos);
 }
 
 /*!
@@ -4424,9 +4424,9 @@ QPolygonF QGraphicsItem::mapToItem(const QGraphicsItem *item, const QPolygonF &p
 */
 QPolygonF QGraphicsItem::mapToParent(const QPolygonF &polygon) const
 {
-    QPolygonF p = polygon;
+    QPolygonF p = !d_ptr->hasTransform ? polygon : transform().map(polygon);
     p.translate(d_ptr->pos);
-    return d_ptr->hasTransform ? transform().map(p) : p;
+    return p;
 }
 
 /*!
@@ -4468,7 +4468,10 @@ QPainterPath QGraphicsItem::mapToItem(const QGraphicsItem *item, const QPainterP
 */
 QPainterPath QGraphicsItem::mapToParent(const QPainterPath &path) const
 {
-    return d_ptr->parent ? itemTransform(d_ptr->parent).map(path) : mapToScene(path);
+    QTransform x = QTransform::fromTranslate(d_ptr->pos.x(), d_ptr->pos.y());
+    if (d_ptr->hasTransform)
+        x = transform() * x;
+    return x.map(path);
 }
 
 /*!
@@ -5697,12 +5700,11 @@ void QGraphicsItem::removeFromIndex()
 */
 void QGraphicsItem::prepareGeometryChange()
 {
-    if (!d_ptr->scene)
-        return;
-
-    d_ptr->updateHelper(QRectF(), false, /*maybeDirtyClipPath=*/!d_ptr->inSetPosHelper);
-    QGraphicsScenePrivate *scenePrivate = d_ptr->scene->d_func();
-    scenePrivate->removeFromIndex(this);
+    if (d_ptr->scene) {
+        d_ptr->updateHelper(QRectF(), false, /*maybeDirtyClipPath=*/!d_ptr->inSetPosHelper);
+        QGraphicsScenePrivate *scenePrivate = d_ptr->scene->d_func();
+        scenePrivate->removeFromIndex(this);
+    }
 
     if (d_ptr->inSetPosHelper)
         return;
diff --git a/tests/auto/qgraphicsitem/tst_qgraphicsitem.cpp b/tests/auto/qgraphicsitem/tst_qgraphicsitem.cpp
index 5dd7e1e050..2d1be37378 100644
--- a/tests/auto/qgraphicsitem/tst_qgraphicsitem.cpp
+++ b/tests/auto/qgraphicsitem/tst_qgraphicsitem.cpp
@@ -162,6 +162,8 @@ private slots:
     void mapFromToParent();
     void mapFromToScene();
     void mapFromToItem();
+    void mapRectFromToParent_data();
+    void mapRectFromToParent();
     void isAncestorOf();
     void commonAncestorItem();
     void data();
@@ -2515,6 +2517,87 @@ void tst_QGraphicsItem::mapFromToItem()
     delete item4;
 }
 
+void tst_QGraphicsItem::mapRectFromToParent_data()
+{
+    QTest::addColumn<bool>("parent");
+    QTest::addColumn<QPointF>("parentPos");
+    QTest::addColumn<QTransform>("parentTransform");
+    QTest::addColumn<QPointF>("pos");
+    QTest::addColumn<QTransform>("transform");
+    QTest::addColumn<QRectF>("inputRect");
+    QTest::addColumn<QRectF>("outputRect");
+
+    QTest::newRow("nil") << false << QPointF() << QTransform() << QPointF() << QTransform() << QRectF() << QRectF();
+    QTest::newRow("simple") << false << QPointF() << QTransform() << QPointF() << QTransform()
+                            << QRectF(0, 0, 10, 10) << QRectF(0, 0, 10, 10);
+    QTest::newRow("simple w/parent") << true
+                                     << QPointF() << QTransform()
+                                     << QPointF() << QTransform()
+                                     << QRectF(0, 0, 10, 10) << QRectF(0, 0, 10, 10);
+    QTest::newRow("simple w/parent parentPos") << true
+                                               << QPointF(50, 50) << QTransform()
+                                               << QPointF() << QTransform()
+                                               << QRectF(0, 0, 10, 10) << QRectF(0, 0, 10, 10);
+    QTest::newRow("simple w/parent parentPos parentRotation") << true
+                                                              << QPointF(50, 50) << QTransform().rotate(45)
+                                                              << QPointF() << QTransform()
+                                                              << QRectF(0, 0, 10, 10) << QRectF(0, 0, 10, 10);
+    QTest::newRow("pos w/parent") << true
+                                  << QPointF() << QTransform()
+                                  << QPointF(50, 50) << QTransform()
+                                  << QRectF(0, 0, 10, 10) << QRectF(50, 50, 10, 10);
+    QTest::newRow("rotation w/parent") << true
+                                       << QPointF() << QTransform()
+                                       << QPointF() << QTransform().rotate(90)
+                                       << QRectF(0, 0, 10, 10) << QRectF(-10, 0, 10, 10);
+    QTest::newRow("pos rotation w/parent") << true
+                                           << QPointF() << QTransform()
+                                           << QPointF(50, 50) << QTransform().rotate(90)
+                                           << QRectF(0, 0, 10, 10) << QRectF(40, 50, 10, 10);
+    QTest::newRow("pos rotation w/parent parentPos parentRotation") << true
+                                                                    << QPointF(-170, -190) << QTransform().rotate(90)
+                                                                    << QPointF(50, 50) << QTransform().rotate(90)
+                                                                    << QRectF(0, 0, 10, 10) << QRectF(40, 50, 10, 10);
+}
+
+void tst_QGraphicsItem::mapRectFromToParent()
+{
+    QFETCH(bool, parent);
+    QFETCH(QPointF, parentPos);
+    QFETCH(QTransform, parentTransform);
+    QFETCH(QPointF, pos);
+    QFETCH(QTransform, transform);
+    QFETCH(QRectF, inputRect);
+    QFETCH(QRectF, outputRect);
+
+    QGraphicsRectItem *rect = new QGraphicsRectItem;
+    rect->setPos(pos);
+    rect->setTransform(transform);
+
+    if (parent) {
+        QGraphicsRectItem *rectParent = new QGraphicsRectItem;
+        rect->setParentItem(rectParent);
+        rectParent->setPos(parentPos);
+        rectParent->setTransform(parentTransform);
+    }
+
+    // Make sure we use non-destructive transform operations (e.g., 90 degree
+    // rotations).
+    QCOMPARE(rect->mapRectToParent(inputRect), outputRect);
+    QCOMPARE(rect->mapRectFromParent(outputRect), inputRect);
+    QCOMPARE(rect->itemTransform(rect->parentItem()).mapRect(inputRect), outputRect);
+    QCOMPARE(rect->mapToParent(inputRect).boundingRect(), outputRect);
+    QCOMPARE(rect->mapToParent(QPolygonF(inputRect)).boundingRect(), outputRect);
+    QCOMPARE(rect->mapFromParent(outputRect).boundingRect(), inputRect);
+    QCOMPARE(rect->mapFromParent(QPolygonF(outputRect)).boundingRect(), inputRect);
+    QPainterPath inputPath;
+    inputPath.addRect(inputRect);
+    QPainterPath outputPath;
+    outputPath.addRect(outputRect);
+    QCOMPARE(rect->mapToParent(inputPath).boundingRect(), outputPath.boundingRect());
+    QCOMPARE(rect->mapFromParent(outputPath).boundingRect(), inputPath.boundingRect());
+}
+
 void tst_QGraphicsItem::isAncestorOf()
 {
     QGraphicsItem *grandPa = new QGraphicsRectItem;
diff --git a/tests/auto/qgraphicsscene/tst_qgraphicsscene.cpp b/tests/auto/qgraphicsscene/tst_qgraphicsscene.cpp
index 91ed851005..354d81ae4d 100644
--- a/tests/auto/qgraphicsscene/tst_qgraphicsscene.cpp
+++ b/tests/auto/qgraphicsscene/tst_qgraphicsscene.cpp
@@ -245,6 +245,7 @@ private slots:
     void task139782_containsItemBoundingRect();
     void task176178_itemIndexMethodBreaksSceneRect();
     void task160653_selectionChanged();
+    void task250680_childClip();
 };
 
 void tst_QGraphicsScene::initTestCase()
@@ -3384,6 +3385,31 @@ void tst_QGraphicsScene::task160653_selectionChanged()
     QCOMPARE(spy.count(), 1);
 }
 
+void tst_QGraphicsScene::task250680_childClip()
+{
+    QGraphicsRectItem *clipper = new QGraphicsRectItem;
+    clipper->setFlag(QGraphicsItem::ItemClipsChildrenToShape);
+    clipper->setPen(QPen(Qt::green));
+    clipper->setRect(200, 200, 640, 480);
+
+    QGraphicsRectItem *rect = new QGraphicsRectItem(clipper);
+    rect->setPen(QPen(Qt::red));
+    rect->setBrush(QBrush(QColor(255, 0, 0, 75)));
+    rect->setPos(320, 240);
+    rect->setRect(-25, -25, 50, 50);
+
+    QGraphicsScene scene;
+    scene.addItem(clipper);
+
+    QPainterPath path;
+    path.addRect(-25, -25, 50, 50);
+    QCOMPARE(rect->clipPath(), path);
+
+    QCOMPARE(scene.items(QRectF(320, 240, 5, 5)).size(), 2);
+    rect->rotate(45);
+    QCOMPARE(scene.items(QRectF(320, 240, 5, 5)).size(), 2);
+}
+
 void tst_QGraphicsScene::sorting_data()
 {
     QTest::addColumn<bool>("cache");