Changes after first round of code reviewing.

This change removes all code that handles ItemIgnoresTransformations
from QGraphicsView, and changes the APIs of the scene index intersectors.

Reviewed-by: Alexis

1 files changed, 39 insertions, 122 deletions

diff --git a/src/gui/graphicsview/qgraphicsview.cpp b/src/gui/graphicsview/qgraphicsview.cpp
index 8c40878..a1e6d9c 100644
--- a/src/gui/graphicsview/qgraphicsview.cpp
+++ b/src/gui/graphicsview/qgraphicsview.cpp
@@ -914,44 +914,32 @@ extern QPainterPath qt_regionToPath(const QRegion &region);
     is at risk of painting 1 pixel outside the bounding rect. Therefore we
     must search for items with an adjustment of (-1, -1, 1, 1).
 */
-QList<QGraphicsItem *> QGraphicsViewPrivate::findItems(const QRegion &exposedRegion, bool *allItems) const
+QList<QGraphicsItem *> QGraphicsViewPrivate::findItems(const QRegion &exposedRegion, bool *allItems,
+                                                       const QTransform &viewTransform) const
 {
     Q_Q(const QGraphicsView);
 
     // Step 1) If all items are contained within the expose region, then
-    // return a list of all visible items.
+    // return a list of all visible items. ### the scene's growing bounding
+    // rect does not take into account untransformable items.
     const QRectF exposedRegionSceneBounds = q->mapToScene(exposedRegion.boundingRect().adjusted(-1, -1, 1, 1))
                                             .boundingRect();
     if (exposedRegionSceneBounds.contains(scene->d_func()->growingItemsBoundingRect)) {
         Q_ASSERT(allItems);
         *allItems = true;
 
-        // All items are guaranteed within the exposed region, don't bother using the index.
-        QList<QGraphicsItem *> itemList(scene->d_func()->index->items(Qt::DescendingOrder));
-        int i = 0;
-        while (i < itemList.size()) {
-            const QGraphicsItem *item = itemList.at(i);
-            // But we only want to include items that are visible
-            // The following check is basically the same as item->d_ptr->isInvisible(), except
-            // that we don't check whether the item clips children to shape or propagates its
-            // opacity (we loop through all items, so those checks are wrong in this context).
-            if (!item->isVisible() || item->d_ptr->isClippedAway() || item->d_ptr->isFullyTransparent())
-                itemList.removeAt(i);
-            else
-                ++i;
-        }
-        return itemList;
+        // All items are guaranteed within the exposed region.
+        return scene->items(Qt::DescendingOrder);
     }
 
     // Step 2) If the expose region is a simple rect and the view is only
     // translated or scaled, search for items using
     // QGraphicsScene::items(QRectF).
-    bool simpleRectLookup =  (scene->d_func()->largestUntransformableItem.isNull()
-                              && exposedRegion.numRects() == 1 && matrix.type() <= QTransform::TxScale);
+    bool simpleRectLookup =  exposedRegion.numRects() == 1 && matrix.type() <= QTransform::TxScale;
     if (simpleRectLookup) {
-        return scene->d_func()->index->items(exposedRegionSceneBounds,
-                                             Qt::IntersectsItemBoundingRect,
-                                             Qt::DescendingOrder);
+        return scene->items(exposedRegionSceneBounds,
+                            Qt::IntersectsItemBoundingRect,
+                            Qt::DescendingOrder, viewTransform);
     }
 
     // If the region is complex or the view has a complex transform, adjust
@@ -961,16 +949,9 @@ QList<QGraphicsItem *> QGraphicsViewPrivate::findItems(const QRegion &exposedReg
     foreach (const QRect &r, exposedRegion.rects())
         adjustedRegion += r.adjusted(-1, -1, 1, 1);
 
-    const QPainterPath exposedPath(qt_regionToPath(adjustedRegion));
-    if (scene->d_func()->largestUntransformableItem.isNull()) {
-        const QPainterPath exposedScenePath(q->mapToScene(exposedPath));
-        return scene->d_func()->index->items(exposedScenePath,
-                                             Qt::IntersectsItemBoundingRect,
-                                             Qt::DescendingOrder);
-    }
-
-    // NB! Path must be in viewport coordinates.
-    return itemsInArea(exposedPath, Qt::IntersectsItemBoundingRect, Qt::DescendingOrder);
+    const QPainterPath exposedScenePath(q->mapToScene(qt_regionToPath(adjustedRegion)));
+    return scene->items(exposedScenePath, Qt::IntersectsItemBoundingRect,
+                        Qt::DescendingOrder, viewTransform);
 }
 
 /*!
@@ -1875,6 +1856,8 @@ void QGraphicsView::fitInView(const QGraphicsItem *item, Qt::AspectRatioMode asp
 void QGraphicsView::render(QPainter *painter, const QRectF &target, const QRect &source,
                            Qt::AspectRatioMode aspectRatioMode)
 {
+    // ### Switch to using the recursive rendering algorithm instead.
+
     Q_D(QGraphicsView);
     if (!d->scene || !(painter && painter->isActive()))
         return;
@@ -1972,67 +1955,6 @@ QList<QGraphicsItem *> QGraphicsView::items() const
 }
 
 /*!
-    Returns all items in the area \a path, which is in viewport coordinates,
-    also taking untransformable items into consideration. This function is
-    considerably slower than just checking the scene directly. There is
-    certainly room for improvement.
-*/
-QList<QGraphicsItem *> QGraphicsViewPrivate::itemsInArea(const QPainterPath &path,
-                                                         Qt::ItemSelectionMode mode,
-                                                         Qt::SortOrder order) const
-{
-    Q_Q(const QGraphicsView);
-
-    // Determine the size of the largest untransformable subtree of children
-    // mapped to scene coordinates.
-    QRectF untr = scene->d_func()->largestUntransformableItem;
-    QRectF ltri = matrix.inverted().mapRect(untr);
-    ltri.adjust(-untr.width(), -untr.height(), untr.width(), untr.height());
-
-    QRectF rect = path.controlPointRect();
-
-    // Find all possible items in the relevant area.
-    // ### Improve this algorithm; it might be searching a too large area.
-    QRectF adjustedRect = q->mapToScene(rect.adjusted(-1, -1, 1, 1).toRect()).boundingRect();
-    adjustedRect.adjust(-ltri.width(), -ltri.height(), ltri.width(), ltri.height());
-
-    // First build a (potentially large) list of all items in the vicinity
-    // that might be untransformable.
-    QList<QGraphicsItem *> allCandidates = scene->d_func()->index->estimateItems(adjustedRect, order, q->transform());
-
-    // Then find the minimal list of items that are inside \a path, and
-    // convert it to a set.
-    QList<QGraphicsItem *> regularCandidates = scene->items(q->mapToScene(path), mode, order, q->transform());
-    QSet<QGraphicsItem *> candSet = QSet<QGraphicsItem *>::fromList(regularCandidates);
-
-    QTransform viewMatrix = q->viewportTransform();
-
-    QList<QGraphicsItem *> result;
-
-    //### this will disapear
-
-    // Run through all candidates and keep all items that are in candSet, or
-    // are untransformable and collide with \a path. ### We can improve this
-    // algorithm.
-    QList<QGraphicsItem *>::Iterator it = allCandidates.begin();
-    while (it != allCandidates.end()) {
-        QGraphicsItem *item = *it;
-        if (item->d_ptr->itemIsUntransformable()) {
-            // Check if this untransformable item collides with the
-            // original selection rect.
-            QTransform itemTransform = item->deviceTransform(viewMatrix);
-            if (QGraphicsScenePrivate::itemCollidesWithPath(item, itemTransform.inverted().map(path), mode))
-                result << item;
-        } else {
-            if (candSet.contains(item))
-                result << item;
-        }
-        ++it;
-    }
-    return result;
-}
-
-/*!
     Returns a list of all the items at the position \a pos in the view. The
     items are listed in descending Z order (i.e., the first item in the list
     is the top-most item, and the last item is the bottom-most item). \a pos
@@ -2051,17 +1973,22 @@ QList<QGraphicsItem *> QGraphicsView::items(const QPoint &pos) const
     Q_D(const QGraphicsView);
     if (!d->scene)
         return QList<QGraphicsItem *>();
-    if (d->scene->d_func()->largestUntransformableItem.isNull()) {
-        if ((d->identityMatrix || d->matrix.type() <= QTransform::TxScale)) {
-            QTransform xinv = viewportTransform().inverted();
-            return d->scene->items(xinv.mapRect(QRectF(pos.x(), pos.y(), 1, 1)));
-        }
-        return d->scene->items(mapToScene(pos.x(), pos.y(), 1, 1));
+    // ### Unify these two, and use the items(QPointF) version in
+    // QGraphicsScene instead. The scene items function could use the viewport
+    // transform to map the point to a rect/polygon.
+    if ((d->identityMatrix || d->matrix.type() <= QTransform::TxScale)) {
+        // Use the rect version
+        QTransform xinv = viewportTransform().inverted();
+        return d->scene->items(xinv.mapRect(QRectF(pos.x(), pos.y(), 1, 1)),
+                               Qt::IntersectsItemShape,
+                               Qt::AscendingOrder,
+                               viewportTransform());
     }
-
-    QPainterPath path;
-    path.addRect(QRectF(pos.x(), pos.y(), 1, 1));
-    return d->itemsInArea(path);
+    // Use the polygon version
+    return d->scene->items(mapToScene(pos.x(), pos.y(), 1, 1),
+                           Qt::IntersectsItemShape,
+                           Qt::AscendingOrder,
+                           viewportTransform());
 }
 
 /*!
@@ -2088,12 +2015,7 @@ QList<QGraphicsItem *> QGraphicsView::items(const QRect &rect, Qt::ItemSelection
     Q_D(const QGraphicsView);
     if (!d->scene)
         return QList<QGraphicsItem *>();
-    if (d->scene->d_func()->largestUntransformableItem.isNull())
-        return d->scene->items(mapToScene(rect), mode);
-
-    QPainterPath path;
-    path.addRect(rect);
-    return d->itemsInArea(path);
+    return d->scene->items(mapToScene(rect), mode, Qt::AscendingOrder, viewportTransform());
 }
 
 /*!
@@ -2121,13 +2043,7 @@ QList<QGraphicsItem *> QGraphicsView::items(const QPolygon &polygon, Qt::ItemSel
     Q_D(const QGraphicsView);
     if (!d->scene)
         return QList<QGraphicsItem *>();
-    if (d->scene->d_func()->largestUntransformableItem.isNull())
-        return d->scene->items(mapToScene(polygon), mode);
-
-    QPainterPath path;
-    path.addPolygon(polygon);
-    path.closeSubpath();
-    return d->itemsInArea(path);
+    return d->scene->items(mapToScene(polygon), mode, Qt::AscendingOrder, viewportTransform());
 }
 
 /*!
@@ -2147,9 +2063,7 @@ QList<QGraphicsItem *> QGraphicsView::items(const QPainterPath &path, Qt::ItemSe
     Q_D(const QGraphicsView);
     if (!d->scene)
         return QList<QGraphicsItem *>();
-    if (d->scene->d_func()->largestUntransformableItem.isNull())
-        return d->scene->items(mapToScene(path), mode);
-    return d->itemsInArea(path);
+    return d->scene->items(mapToScene(path), mode, Qt::AscendingOrder, viewportTransform());
 }
 
 /*!
@@ -2168,7 +2082,9 @@ QGraphicsItem *QGraphicsView::itemAt(const QPoint &pos) const
     Q_D(const QGraphicsView);
     if (!d->scene)
         return 0;
-    QList<QGraphicsItem *> itemsAtPos = items(pos);
+    // ### Use QGraphicsScene::itemAt() instead.
+    QList<QGraphicsItem *> itemsAtPos = d->scene->items(pos, Qt::IntersectsItemShape, Qt::AscendingOrder,
+                                                        viewportTransform());
     return itemsAtPos.isEmpty() ? 0 : itemsAtPos.first();
 }
 
@@ -3073,7 +2989,8 @@ void QGraphicsView::mouseMoveEvent(QMouseEvent *event)
             selectionArea.addPolygon(mapToScene(d->rubberBandRect));
             selectionArea.closeSubpath();
             if (d->scene)
-                d->scene->setSelectionArea(selectionArea, d->rubberBandSelectionMode);
+                d->scene->setSelectionArea(selectionArea, d->rubberBandSelectionMode,
+                                           viewportTransform());
             return;
         }
     } else
@@ -3280,7 +3197,7 @@ void QGraphicsView::paintEvent(QPaintEvent *event)
     } else {
         // Find all exposed items
         bool allItems = false;
-        QList<QGraphicsItem *> itemList = d->findItems(d->exposedRegion, &allItems);
+        QList<QGraphicsItem *> itemList = d->findItems(d->exposedRegion, &allItems, viewTransform);
         
         if (!itemList.isEmpty()) {
             // Generate the style options.