Fixup update rect regression by adjusting expose rectangles.

This change shows a limitation in Graphics View caused by QPen's
default width being 0 (cosmetic), while Graphics View actually
does not support cosmetic pens at all. Because items are at risk of
drawing lines that poke 1 pixel outside their bounding rect,
QGraphicsView must look for items that are up to one pixel
larger than their bounding rect mapped to viewport coordinates.

Furthermore, mapToScene(QRect) forces us to adjust the
input rectangle by (0, 0, 1, 1), because it uses QRect::bottomRight()
(etc) when mapping the rectangle to a polygon (which is _wrong_). Since
this behavior has been there since 4.2, we don't want to fix it in
a 4.5 patch release...

The only _proper_ fix to this problem is for the view to know the item's
"adjust" in device coordinates, allowing items to use cosmetic pens
freely. Fex, we could introduce QGraphicsItem::viewportMargins() or so.

Added an autotest to ensure this doesn't break again.

Reviewed-by: bnilsen

2 files changed, 95 insertions, 8 deletions

diff --git a/src/gui/graphicsview/qgraphicsview.cpp b/src/gui/graphicsview/qgraphicsview.cpp
index c4297df..418638f 100644
--- a/src/gui/graphicsview/qgraphicsview.cpp
+++ b/src/gui/graphicsview/qgraphicsview.cpp
@@ -1046,13 +1046,24 @@ void QGraphicsViewPrivate::freeStyleOptionsArray(QStyleOptionGraphicsItem *array
 
 extern QPainterPath qt_regionToPath(const QRegion &region);
 
+/*!
+    ### Adjustments in findItems: mapToScene(QRect) forces us to adjust the
+    input rectangle by (0, 0, 1, 1), because it uses QRect::bottomRight()
+    (etc) when mapping the rectangle to a polygon (which is _wrong_). In
+    addition, as QGraphicsItem::boundingRect() is defined in logical space,
+    but the default pen for QPainter is cosmetic with a width of 0, QPainter
+    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
 {
     Q_Q(const QGraphicsView);
-    const QPainterPath exposedPath(qt_regionToPath(exposedRegion));
-    const QPainterPath exposedScenePath(q->mapToScene(exposedPath));
 
-    if (exposedScenePath.contains(scene->d_func()->growingItemsBoundingRect)) {
+    // Step 1) If all items are contained within the expose region, then
+    // return a list of all visible items.
+    const QRectF exposedRegionSceneBounds = q->mapToScene(exposedRegion.boundingRect().adjusted(-1, -1, 2, 2))
+                                            .boundingRect();
+    if (exposedRegionSceneBounds.contains(scene->d_func()->growingItemsBoundingRect)) {
         Q_ASSERT(allItems);
         *allItems = true;
 
@@ -1072,12 +1083,27 @@ QList<QGraphicsItem *> QGraphicsViewPrivate::findItems(const QRegion &exposedReg
         return itemList;
     }
 
+    // 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);
+    if (simpleRectLookup) {
+        return scene->d_func()->items_helper(exposedRegionSceneBounds,
+                                             Qt::IntersectsItemBoundingRect,
+                                             Qt::DescendingOrder);
+    }
+
+    // If the region is complex or the view has a complex transform, adjust
+    // the expose region, convert it to a path, and then search for items
+    // using QGraphicsScene::items(QPainterPath);
+    QRegion adjustedRegion;
+    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()) {
-        if (exposedRegion.numRects() == 1 && matrix.type() <= QTransform::TxScale) {
-            return scene->d_func()->items_helper(exposedScenePath.controlPointRect(),
-                                                 Qt::IntersectsItemBoundingRect,
-                                                 Qt::DescendingOrder);
-        }
+        const QPainterPath exposedScenePath(q->mapToScene(exposedPath));
         return scene->d_func()->items_helper(exposedScenePath,
                                              Qt::IntersectsItemBoundingRect,
                                              Qt::DescendingOrder);
diff --git a/tests/auto/qgraphicsview/tst_qgraphicsview.cpp b/tests/auto/qgraphicsview/tst_qgraphicsview.cpp
index 4368e76..412c6c5 100644
--- a/tests/auto/qgraphicsview/tst_qgraphicsview.cpp
+++ b/tests/auto/qgraphicsview/tst_qgraphicsview.cpp
@@ -155,6 +155,8 @@ private slots:
     void fitInView();
     void itemsAtPoint();
     void itemsInRect();
+    void itemsInRect_cosmeticAdjust_data();
+    void itemsInRect_cosmeticAdjust();
     void itemsInPoly();
     void itemsInPath();
     void itemAt();
@@ -1310,6 +1312,65 @@ void tst_QGraphicsView::itemsInRect()
     QCOMPARE(items.takeFirst()->zValue(), qreal(3));
 }
 
+class CountPaintItem : public QGraphicsRectItem
+{
+public:
+    int numPaints;
+
+    CountPaintItem(const QRectF &rect)
+        : QGraphicsRectItem(rect), numPaints(0)
+    { }
+
+    void paint(QPainter *painter, const QStyleOptionGraphicsItem *option, QWidget *widget = 0)
+    {
+        ++numPaints;
+        QGraphicsRectItem::paint(painter, option, widget);
+    }
+};
+
+void tst_QGraphicsView::itemsInRect_cosmeticAdjust_data()
+{
+    QTest::addColumn<QRect>("updateRect");
+    QTest::addColumn<int>("numPaints");
+
+    QTest::newRow("nil") << QRect() << 1;
+    QTest::newRow("0, 0, 300, 100") << QRect(0, 0, 300, 100) << 1;
+    QTest::newRow("0, 0, 100, 300") << QRect(0, 0, 100, 300) << 1;
+    QTest::newRow("200, 0, 100, 300") << QRect(200, 0, 100, 300) << 1;
+    QTest::newRow("0, 200, 300, 100") << QRect(0, 200, 300, 100) << 1;
+    QTest::newRow("0, 0, 300, 99") << QRect(0, 0, 300, 99) << 0;
+    QTest::newRow("0, 0, 99, 300") << QRect(0, 0, 99, 300) << 0;
+    QTest::newRow("201, 0, 99, 300") << QRect(201, 0, 99, 300) << 0;
+    QTest::newRow("0, 201, 300, 99") << QRect(0, 201, 300, 99) << 0;
+}
+
+void tst_QGraphicsView::itemsInRect_cosmeticAdjust()
+{
+    QFETCH(QRect, updateRect);
+    QFETCH(int, numPaints);
+    
+    QGraphicsScene scene(-100, -100, 200, 200);
+    CountPaintItem *rect = new CountPaintItem(QRectF(-50, -50, 100, 100));
+    scene.addItem(rect);
+
+    QGraphicsView view(&scene);
+    view.setFrameStyle(0);
+    view.resize(300, 300);
+    view.show();
+#ifdef Q_WS_X11
+    qt_x11_wait_for_window_manager(&view);
+#endif
+    QTest::qWait(125);
+
+    rect->numPaints = 0;
+    if (updateRect.isNull())
+        view.viewport()->update();
+    else
+        view.viewport()->update(updateRect);
+    qApp->processEvents();
+    QCOMPARE(rect->numPaints, numPaints);
+}
+
 void tst_QGraphicsView::itemsInPoly()
 {
     QGraphicsScene scene;