Sometimes wrong clipping in QWidget::render() when passing a device or

an untransformed painter

When passing a painter to QWidget::render, we use the
painter->paintEngine()->systemClip() as the "system viewport",
i.e. all painting triggered by render() should be limited to
this area. The only way to achieve this is by always ensuring the
system clip is clipped to the same area (systemClip &= systemViewport).
The problem however, was that we only did this for transformed
painters. We must of course always do it when there's a systemViewport
set, regardless of whether the painter is transformed or not.

Auto test included.

Task-number: 248852
Reviewed-by: Trond

4 files changed, 114 insertions, 11 deletions

diff --git a/src/gui/kernel/qwidget.cpp b/src/gui/kernel/qwidget.cpp
index 071e1bd..e9fd28b 100644
--- a/src/gui/kernel/qwidget.cpp
+++ b/src/gui/kernel/qwidget.cpp
@@ -4709,10 +4709,13 @@ void QWidget::render(QPaintDevice *target, const QPoint &targetOffset,
     if (redirected) {
         target = redirected;
         offset -= redirectionOffset;
-        if (!inRenderWithPainter) { // Clip handled by shared painter (in qpainter.cpp).
-            const QRegion redirectedSystemClip = redirected->paintEngine()->systemClip();
-            if (!redirectedSystemClip.isEmpty())
-                paintRegion &= redirectedSystemClip.translated(-offset);
+    }
+
+    if (!inRenderWithPainter) { // Clip handled by shared painter (in qpainter.cpp).
+        if (QPaintEngine *targetEngine = target->paintEngine()) {
+            const QRegion targetSystemClip = targetEngine->systemClip();
+            if (!targetSystemClip.isEmpty())
+                paintRegion &= targetSystemClip.translated(-offset);
         }
     }
 
diff --git a/src/gui/painting/qpaintengine.cpp b/src/gui/painting/qpaintengine.cpp
index ad09060..7de1ec4 100644
--- a/src/gui/painting/qpaintengine.cpp
+++ b/src/gui/painting/qpaintengine.cpp
@@ -949,8 +949,8 @@ void QPaintEngine::setSystemClip(const QRegion &region)
     Q_D(QPaintEngine);
     d->systemClip = region;
     // Be backward compatible and only call d->systemStateChanged()
-    // if we currently have a system transform set.
-    if (d->hasSystemTransform) {
+    // if we currently have a system transform/viewport set.
+    if (d->hasSystemTransform || d->hasSystemViewport) {
         d->transformSystemClip();
         d->systemStateChanged();
     }
diff --git a/src/gui/painting/qpaintengine_p.h b/src/gui/painting/qpaintengine_p.h
index eeba7ec..0b5e175 100644
--- a/src/gui/painting/qpaintengine_p.h
+++ b/src/gui/painting/qpaintengine_p.h
@@ -83,10 +83,12 @@ public:
         if (systemClip.isEmpty())
             return;
 
-        if (systemTransform.type() <= QTransform::TxTranslate)
-            systemClip.translate(qRound(systemTransform.dx()), qRound(systemTransform.dy()));
-        else
-            systemClip = systemTransform.map(systemClip);
+        if (hasSystemTransform) {
+            if (systemTransform.type() <= QTransform::TxTranslate)
+                systemClip.translate(qRound(systemTransform.dx()), qRound(systemTransform.dy()));
+            else
+                systemClip = systemTransform.map(systemClip);
+        }
 
         // Make sure we're inside the viewport.
         if (hasSystemViewport) {
@@ -101,7 +103,7 @@ public:
     inline void setSystemTransform(const QTransform &xform)
     {
         systemTransform = xform;
-        if ((hasSystemTransform = !xform.isIdentity()))
+        if ((hasSystemTransform = !xform.isIdentity()) || hasSystemViewport)
             transformSystemClip();
         systemStateChanged();
     }
diff --git a/tests/auto/qwidget/tst_qwidget.cpp b/tests/auto/qwidget/tst_qwidget.cpp
index a052034..767553a 100644
--- a/tests/auto/qwidget/tst_qwidget.cpp
+++ b/tests/auto/qwidget/tst_qwidget.cpp
@@ -283,6 +283,8 @@ private slots:
     void render_task217815();
     void render_windowOpacity();
     void render_systemClip();
+    void render_systemClip2_data();
+    void render_systemClip2();
 
     void setContentsMargins();
 
@@ -6897,6 +6899,102 @@ void tst_QWidget::render_systemClip()
 #endif
 }
 
+void tst_QWidget::render_systemClip2_data()
+{
+    QTest::addColumn<bool>("autoFillBackground");
+    QTest::addColumn<bool>("usePaintEvent");
+    QTest::addColumn<QColor>("expectedColor");
+
+    QTest::newRow("Only auto-fill background") << true << false << QColor(Qt::blue);
+    QTest::newRow("Only draw in paintEvent") << false << true << QColor(Qt::green);
+    QTest::newRow("Auto-fill background and draw in paintEvent") << true << true << QColor(Qt::green);
+}
+
+void tst_QWidget::render_systemClip2()
+{
+    QFETCH(bool, autoFillBackground);
+    QFETCH(bool, usePaintEvent);
+    QFETCH(QColor, expectedColor);
+
+    Q_ASSERT_X(expectedColor != QColor(Qt::red), Q_FUNC_INFO,
+               "Qt::red is the reference color for the image, pick another color");
+
+    class MyWidget : public QWidget
+    {
+    public:
+        bool usePaintEvent;
+        void paintEvent(QPaintEvent *)
+        {
+            if (usePaintEvent)
+                QPainter(this).fillRect(rect(), Qt::green);
+        }
+    };
+
+    MyWidget widget;
+    widget.usePaintEvent = usePaintEvent;
+    widget.setPalette(Qt::blue);
+    // NB! widget.setAutoFillBackground(autoFillBackground) won't do the
+    // trick here since the widget is a top-level. The background is filled
+    // regardless, unless Qt::WA_OpaquePaintEvent or Qt::WA_NoSystemBackground
+    // is set. We therefore use the opaque attribute to turn off auto-fill.
+    if (!autoFillBackground)
+        widget.setAttribute(Qt::WA_OpaquePaintEvent);
+    widget.resize(100, 100);
+
+    QImage image(widget.size(), QImage::Format_RGB32);
+    image.fill(QColor(Qt::red).rgb());
+
+    QPaintEngine *paintEngine = image.paintEngine();
+    QVERIFY(paintEngine);
+
+    QRegion systemClip(QRegion(50, 0, 50, 10));
+    systemClip += QRegion(90, 10, 10, 40);
+    paintEngine->setSystemClip(systemClip);
+
+    // Render entire widget directly onto device.
+    widget.render(&image);
+
+#ifndef RENDER_DEBUG
+    image.save("systemclip_with_device.png");
+#endif
+    // All pixels within the system clip should now be
+    // the expectedColor, and the rest should be red.
+    for (int i = 0; i < image.height(); ++i) {
+        for (int j = 0; j < image.width(); ++j) {
+            if (systemClip.contains(QPoint(j, i)))
+                QCOMPARE(image.pixel(j, i), expectedColor.rgb());
+            else
+                QCOMPARE(image.pixel(j, i), QColor(Qt::red).rgb());
+        }
+    }
+
+    // Refill image with red.
+    image.fill(QColor(Qt::red).rgb());
+
+    // Do the same with an untransformed painter.
+    QPainter painter(&image);
+    //Make sure we're using the same paint engine and has the right clip set.
+    paintEngine->setSystemClip(systemClip);
+    QCOMPARE(painter.paintEngine(), paintEngine);
+    QCOMPARE(paintEngine->systemClip(), systemClip);
+
+    widget.render(&painter);
+
+#ifndef RENDER_DEBUG
+    image.save("systemclip_with_untransformed_painter.png");
+#endif
+    // All pixels within the system clip should now be
+    // the expectedColor, and the rest should be red.
+    for (int i = 0; i < image.height(); ++i) {
+        for (int j = 0; j < image.width(); ++j) {
+            if (systemClip.contains(QPoint(j, i)))
+                QCOMPARE(image.pixel(j, i), expectedColor.rgb());
+            else
+                QCOMPARE(image.pixel(j, i), QColor(Qt::red).rgb());
+        }
+    }
+}
+
 void tst_QWidget::setContentsMargins()
 {
     QLabel label("why does it always rain on me?");