diff --git a/sources/palettegraphicsview.cpp b/sources/palettegraphicsview.cpp index bd5247e0d..b18bf725b 100644 --- a/sources/palettegraphicsview.cpp +++ b/sources/palettegraphicsview.cpp @@ -21,36 +21,40 @@ #include #include -#include #include #include #include #include +namespace { + /** + QGraphicsScene::drawItems() is protected, and QGraphicsView::drawItems() + hands the scene the viewport only when the painter is on it. The + view paints into an image, and still needs the scene to get the + viewport: that is what makes the scene record where it painted + each item, which is where the item is erased from when it moves. + Naming the member through a derived class is the standard way to + a pointer to a protected member; a call through it dispatches to + the scene's own override, if any. + */ + struct SceneAccess : QGraphicsScene + { + using DrawItems = void (QGraphicsScene::*)(QPainter *, int, QGraphicsItem *[], + const QStyleOptionGraphicsItem[], QWidget *); + static DrawItems drawItemsPointer() { return &SceneAccess::drawItems; } + }; +} + PaletteGraphicsView::PaletteGraphicsView(QWidget *parent) : QGraphicsView(parent) { qApp->installEventFilter(this); - setViewportUpdateMode(QGraphicsView::FullViewportUpdate); } PaletteGraphicsView::PaletteGraphicsView(QGraphicsScene *scene, QWidget *parent) : QGraphicsView(scene, parent) { qApp->installEventFilter(this); - setViewportUpdateMode(QGraphicsView::FullViewportUpdate); - listenToScene(scene); -} - -/** - @brief PaletteGraphicsView::setScene - Same as QGraphicsView::setScene, and keeps the scene's updates flowing - (see the class description). -*/ -void PaletteGraphicsView::setScene(QGraphicsScene *scene) -{ - QGraphicsView::setScene(scene); - listenToScene(scene); } /** @@ -72,21 +76,6 @@ void PaletteGraphicsView::paintingInverted(bool inverted) Q_UNUSED(inverted) } -/** - @brief PaletteGraphicsView::listenToScene - Connect a receiver to the scene's changed() signal, dropping the one - on the previous scene. Any receiver does; this one has nothing to do. - The connection dies with the view. -*/ -void PaletteGraphicsView::listenToScene(QGraphicsScene *scene) -{ - disconnect(m_scene_connection); - m_scene_connection = QMetaObject::Connection(); - if (scene) - m_scene_connection = connect(scene, &QGraphicsScene::changed, - this, [](const QList &) {}); -} - /** @brief PaletteGraphicsView::eventFilter Repaint the whole viewport when the application palette changes. Qt @@ -111,57 +100,121 @@ bool PaletteGraphicsView::eventFilter(QObject *watched, QEvent *event) void PaletteGraphicsView::paintEvent(QPaintEvent *event) { if (invertsLightness()) - paintInverted(event->rect()); - else - QGraphicsView::paintEvent(event); + { + paintInverted(event); + return; + } + m_buffer = QImage(); + QGraphicsView::paintEvent(event); } /** @brief PaletteGraphicsView::paintInverted - Render \a area of the viewport into an off-screen image, invert the - lightness of that image between the palette's Base and Text colors and - blit it to the viewport. Inverting the finished rendering turns the - white sheet dark and the black ink light in one pass, and keeps the - hue of colored strokes. - @param area the part of the viewport to repaint, in viewport coordinates + Run QGraphicsView::paintEvent() with the drawing hooks redirected to + an off-screen image of the viewport, then invert the lightness of the + exposed part of that image between the palette's Base and Text colors + and blit it to the viewport. Inverting the finished rendering turns + the white sheet dark and the black ink light in one pass, and keeps + the hue of colored strokes. The image is in viewport coordinates, so + the hooks paint with the view's own transform and the scene records + the items' places in the viewport, as it does on a light palette. + @param event the paint event, for the exposed area */ -void PaletteGraphicsView::paintInverted(const QRect &area) +void PaletteGraphicsView::paintInverted(QPaintEvent *event) { - const QRect rect = area.intersected(viewport()->rect()); - if (rect.isEmpty()) + const QRect exposed = event->rect().intersected(viewport()->rect()); + if (exposed.isEmpty()) return; const qreal ratio = viewport()->devicePixelRatioF(); - QImage buffer(qCeil(rect.width() * ratio), qCeil(rect.height() * ratio), - QImage::Format_RGB32); - buffer.setDevicePixelRatio(ratio); - // render() paints only what the scene draws; what it leaves blank is + const QSize size(qCeil(viewport()->width() * ratio), qCeil(viewport()->height() * ratio)); + if (m_buffer.size() != size || m_buffer.devicePixelRatio() != ratio) + { + m_buffer = QImage(size, QImage::Format_RGB32); + m_buffer.setDevicePixelRatio(ratio); + } + + m_buffer_painter.begin(&m_buffer); + // The hooks paint only what the scene draws; what they leave blank is // the white sheet, which the inversion turns into the Base color. - buffer.fill(Qt::white); + m_buffer_painter.fillRect(exposed, Qt::white); + m_buffer_painter.setClipRect(exposed); + m_buffer_painter.setRenderHints(renderHints()); + m_buffer_painter.setWorldTransform(viewportTransform()); - QPainter buffer_painter(&buffer); - buffer_painter.setRenderHints(renderHints()); + m_inverting = true; paintingInverted(true); - render(&buffer_painter, QRectF(QPointF(0, 0), QSizeF(rect.size())), - rect, Qt::IgnoreAspectRatio); + const OptimizationFlags flags = optimizationFlags(); + setOptimizationFlag(QGraphicsView::IndirectPainting, true); + QGraphicsView::paintEvent(event); + setOptimizationFlags(flags); paintingInverted(false); - buffer_painter.end(); + m_inverting = false; + m_buffer_painter.end(); + blitInverted(exposed); +} + +/** + @brief PaletteGraphicsView::drawBackground + Into the off-screen image while painting inverted, else as + QGraphicsView. +*/ +void PaletteGraphicsView::drawBackground(QPainter *painter, const QRectF &rect) +{ + QGraphicsView::drawBackground(m_inverting ? &m_buffer_painter : painter, rect); +} + +/** + @brief PaletteGraphicsView::drawItems + Into the off-screen image while painting inverted, with the viewport + as the scene's widget (see SceneAccess), else as QGraphicsView. +*/ +void PaletteGraphicsView::drawItems(QPainter *painter, int count, QGraphicsItem *items[], + const QStyleOptionGraphicsItem options[]) +{ + if (m_inverting && scene()) + (scene()->*SceneAccess::drawItemsPointer())(&m_buffer_painter, count, items, options, viewport()); + else + QGraphicsView::drawItems(painter, count, items, options); +} + +/** + @brief PaletteGraphicsView::drawForeground + Into the off-screen image while painting inverted, else as + QGraphicsView. +*/ +void PaletteGraphicsView::drawForeground(QPainter *painter, const QRectF &rect) +{ + QGraphicsView::drawForeground(m_inverting ? &m_buffer_painter : painter, rect); +} + +/** + @brief PaletteGraphicsView::blitInverted + Invert the lightness of \a area of the off-screen image and draw it on + the viewport, then the selection rubber band on top: the one + QGraphicsView::paintEvent() drew went under the blit. + @param area the part of the viewport to blit, in viewport coordinates +*/ +void PaletteGraphicsView::blitInverted(const QRect &area) +{ + const qreal ratio = m_buffer.devicePixelRatio(); + QImage part = m_buffer.copy(QRectF(area.topLeft() * ratio, area.size() * ratio).toAlignedRect()); + part.setDevicePixelRatio(ratio); // The application palette, for the reason given in invertsLightness(). const QPalette application_palette = QApplication::palette(); - QET::Palette::invertLightness(buffer, application_palette.color(QPalette::Base), + QET::Palette::invertLightness(part, application_palette.color(QPalette::Base), application_palette.color(QPalette::Text)); QPainter painter(viewport()); - painter.drawImage(rect.topLeft(), buffer); + painter.drawImage(area.topLeft(), part); drawRubberBand(painter); } /** @brief PaletteGraphicsView::drawRubberBand - Draw the selection rubber band the way QGraphicsView::paintEvent does. - Rendering the view into an off-screen image skips it, so it is drawn - here instead, after the inversion, in the palette colors. + Draw the selection rubber band the way QGraphicsView::paintEvent does, + after the inversion, in the palette colors. @param painter a painter on the viewport */ void PaletteGraphicsView::drawRubberBand(QPainter &painter) diff --git a/sources/palettegraphicsview.h b/sources/palettegraphicsview.h index 77924c356..512bba283 100644 --- a/sources/palettegraphicsview.h +++ b/sources/palettegraphicsview.h @@ -19,38 +19,39 @@ #define PALETTE_GRAPHICS_VIEW_H #include - -class QPainter; +#include +#include /** - A QGraphicsView that shows its scene with inverted lightness while its - palette is dark: white becomes the palette's Base color, black its Text - color, and colored strokes keep their hue. The scene itself is left as - drawn, so printing and exporting it still give black on white. On a - light palette the view paints exactly as QGraphicsView does. + A QGraphicsView that shows its scene with inverted lightness while the + application palette is dark: white becomes the palette's Base color, + black its Text color, and colored strokes keep their hue. The scene + itself is left as drawn, so printing and exporting it still give black + on white. On a light palette the view paints exactly as QGraphicsView + does. - The view paints through QGraphicsView::render() into an image and blits - the inverted image. QGraphicsView delivers scene updates straight to - its viewport when nobody listens to QGraphicsScene::changed(), and in - that mode the scene clears its "update everything" flag only when the - items are painted straight onto the viewport, which never happens - here: from the second QGraphicsScene::update() on, every scene update - and every item update would wait for an unrelated repaint. So the view - listens to changed() on every scene it is given, which makes the scene - clear the flag before it emits. Set the scene through this class, not - through a QGraphicsView pointer. + On a dark palette the view still runs QGraphicsView::paintEvent(), with + the IndirectPainting flag set for the duration of that call, so that + the background, the items and the foreground come through the + drawBackground(), drawItems() and drawForeground() hooks. (That flag + selects Qt's older item-painting algorithm, which first builds a list + of the exposed items and their style options; it is set only while + the view paints inverted.) The hooks + paint into an off-screen image the size of the viewport, in viewport + coordinates; paintEvent() then inverts the lightness of the exposed + part of that image and blits it. Going through the real paint event, + and handing the scene the viewport when the items are drawn, keeps the + view on QGraphicsView's default update path, which erases a moved item + where it was last painted, children included, even a child whose + geometry is set while its parent is painted (a terminal's help lines). + The alternative, rendering with QGraphicsView::render(), needed a + receiver on QGraphicsScene::changed() to keep the scene's updates + flowing, and that receiver puts the scene on its Qt 4.4 compatibility + path, which erases only the moved item's own old rectangle: a moved + element then left its terminals' help lines, which span the whole + sheet, behind at every step (#954). - The constructor also forces QGraphicsView::FullViewportUpdate in place - of the default MinimalViewportUpdate. #954 shipped with the default - kept, and moving an item then left conductor-shaped ghosts behind on - both a light and a dark palette, so the cause is shared code, not - paintInverted(): most likely listening to changed() at all, above, - changes which of QGraphicsScene's two update paths a view is on, and - MinimalViewportUpdate's job of turning the scene's reported dirty - rects into the smallest correct viewport region is where that would - show up first. FullViewportUpdate removes the need to get that region - right by repainting the whole viewport on every update; the class's - own benchmark already shows that cost is small next to a frame budget. + The CacheBackground cache mode is not supported on the inverted path. */ class PaletteGraphicsView : public QGraphicsView { @@ -60,14 +61,17 @@ class PaletteGraphicsView : public QGraphicsView explicit PaletteGraphicsView(QWidget *parent = nullptr); explicit PaletteGraphicsView(QGraphicsScene *scene, QWidget *parent = nullptr); - void setScene(QGraphicsScene *scene); bool invertsLightness() const; protected: bool eventFilter(QObject *watched, QEvent *event) override; void paintEvent(QPaintEvent *event) override; + void drawBackground(QPainter *painter, const QRectF &rect) override; + void drawItems(QPainter *painter, int count, QGraphicsItem *items[], + const QStyleOptionGraphicsItem options[]) override; + void drawForeground(QPainter *painter, const QRectF &rect) override; /** - Called with true right before the scene is rendered for an + Called with true right before the scene is painted for an inverted display and with false right after, so a scene can adapt what it draws (a softer grid, for instance). Does nothing by default. @@ -75,11 +79,17 @@ class PaletteGraphicsView : public QGraphicsView virtual void paintingInverted(bool inverted); private: - void listenToScene(QGraphicsScene *scene); - void paintInverted(const QRect &area); + void paintInverted(QPaintEvent *event); + void blitInverted(const QRect &area); void drawRubberBand(QPainter &painter); - QMetaObject::Connection m_scene_connection; + /// The off-screen image the hooks paint into while m_inverting: + /// the viewport's size, in its coordinates. Kept between paints, + /// dropped when the view paints on a light palette again. + QImage m_buffer; + QPainter m_buffer_painter; + /// True while paintEvent() paints for an inverted display. + bool m_inverting = false; }; #endif diff --git a/tests/qttest/tst_qetpalette.cpp b/tests/qttest/tst_qetpalette.cpp index 5c504e66a..c15ebdeb0 100644 --- a/tests/qttest/tst_qetpalette.cpp +++ b/tests/qttest/tst_qetpalette.cpp @@ -29,6 +29,7 @@ #include #include #include +#include #include #include #include @@ -85,6 +86,10 @@ class tst_qetpalette : public QObject void paletteViewDrawsTheRubberBand(); void paletteViewFollowsTheApplicationUnderAStyleSheet(); void paletteViewFillsWhatTheSceneLeavesBlank(); + void paletteViewErasesMovedChildren_data(); + void paletteViewErasesMovedChildren(); + void paletteViewErasesChildrenMovedWhilePainting_data(); + void paletteViewErasesChildrenMovedWhilePainting(); void styleSheetWidgetsFollowPaletteChange(); private: @@ -573,6 +578,37 @@ namespace { } }; + /** + A parent that, like a Terminal with its help lines, gives its child + line a new geometry from inside paint(): the line always runs + across the sheet at the parent's height, whatever the parent's + position. Where the child is painted is therefore only known once + the parent has been painted. + */ + class PaintTimeHelpLine : public QGraphicsRectItem + { + public: + explicit PaintTimeHelpLine(const QRectF &sheet) : + QGraphicsRectItem(0, 0, 20, 20), + m_sheet(sheet), + m_line(new QGraphicsLineItem(this)) + { + setPen(Qt::NoPen); + setBrush(Qt::black); + m_line->setPen(QPen(Qt::black, 2)); + } + void paint(QPainter *painter, const QStyleOptionGraphicsItem *option, QWidget *widget) override + { + QGraphicsRectItem::paint(painter, option, widget); + const qreal y = scenePos().y() + 10; + m_line->setLine(QLineF(m_line->mapFromScene(QPointF(m_sheet.left(), y)), + m_line->mapFromScene(QPointF(m_sheet.right(), y)))); + } + private: + QRectF m_sheet; + QGraphicsLineItem *m_line; + }; + /** A small folio: a white sheet with a black line and a red box. Returns the box, which is selectable. @@ -803,6 +839,112 @@ void tst_qetpalette::paletteViewFillsWhatTheSceneLeavesBlank() QCOMPARE(image.pixelColor(100, 100), base); } +/** + Moving an item must erase it where it was, children included, even a + child far bigger than its parent: a terminal's help lines span the + whole sheet. QGraphicsView does that on its default update path by + remembering where each item was last painted. A receiver on + QGraphicsScene::changed() switches the scene to its Qt 4.4 + compatibility path, which erases only the parent's own old rect and + leaves the children's trails behind (reported on #954). The check + reads the window's backing store, not a fresh rendering, and makes + sure the repaint after the move was a partial one: a full repaint of + the viewport would hide the bug, not fix it. +*/ +void tst_qetpalette::paletteViewErasesMovedChildren_data() +{ + addPaletteRows(); +} + +void tst_qetpalette::paletteViewErasesMovedChildren() +{ + QFETCH(QPalette, palette); + QApplication::setStyle(QStyleFactory::create("Fusion")); + QApplication::setPalette(palette); + + QGraphicsScene scene(0, 0, 200, 120); + scene.setBackgroundBrush(Qt::white); + QGraphicsRectItem *parent = scene.addRect(0, 0, 20, 20, QPen(Qt::NoPen), QBrush(Qt::black)); + parent->setPos(20, 20); + QGraphicsLineItem *line = new QGraphicsLineItem(-10, 10, 170, 10, parent); + line->setPen(QPen(Qt::black, 2)); + + ProbeView view(&scene); + showAsSheet(view); + QVERIFY(QTest::qWaitForWindowExposed(&view)); + QTRY_VERIFY(view.paints >= 1); + QScreen *screen = view.screen(); + const WId window = view.window()->winId(); + const QImage before = screen->grabWindow(window, 0, 0, 200, 120).toImage(); + const QColor sheet = sheetColor(before); + QVERIFY2(before.pixelColor(120, 30) != sheet, "the child line is not drawn"); + QVERIFY2(before.pixelColor(30, 30) != sheet, "the parent is not drawn"); + + const int from = view.paint_rects.size(); + parent->setPos(20, 70); + QTRY_VERIFY(view.paint_rects.size() > from); + QVERIFY2(!view.fullyRepaintedSince(from), "the move repainted the whole viewport"); + + const QImage after = screen->grabWindow(window, 0, 0, 200, 120).toImage(); + QVERIFY2(after.pixelColor(120, 30) == sheet, + qPrintable(QString("the child line left a trail: %1").arg(after.pixelColor(120, 30).name()))); + QVERIFY2(after.pixelColor(30, 30) == sheet, + qPrintable(QString("the parent left a trail: %1").arg(after.pixelColor(30, 30).name()))); + QVERIFY2(after.pixelColor(120, 80) != sheet, "the child line is missing at its new place"); + QVERIFY2(after.pixelColor(30, 80) != sheet, "the parent is missing at its new place"); +} + +/** + The same, for a child whose geometry is set while its parent is + painted, as a Terminal does with its help lines: the view must record + where the child was painted, not where it was before the paint, or + the next move erases the wrong place. Two moves, because the first + paint after a move is where the child gets its new geometry. +*/ +void tst_qetpalette::paletteViewErasesChildrenMovedWhilePainting_data() +{ + addPaletteRows(); +} + +void tst_qetpalette::paletteViewErasesChildrenMovedWhilePainting() +{ + QFETCH(QPalette, palette); + QApplication::setStyle(QStyleFactory::create("Fusion")); + QApplication::setPalette(palette); + + QGraphicsScene scene(0, 0, 200, 120); + scene.setBackgroundBrush(Qt::white); + PaintTimeHelpLine *parent = new PaintTimeHelpLine(scene.sceneRect()); + scene.addItem(parent); + parent->setPos(20, 20); + + ProbeView view(&scene); + showAsSheet(view); + QVERIFY(QTest::qWaitForWindowExposed(&view)); + QTRY_VERIFY(view.paints >= 1); + QScreen *screen = view.screen(); + const WId window = view.window()->winId(); + const QColor sheet = sheetColor(screen->grabWindow(window, 0, 0, 200, 120).toImage()); + + int from = view.paint_rects.size(); + parent->setPos(20, 50); + QTRY_VERIFY(view.paint_rects.size() > from); + QTRY_VERIFY2(screen->grabWindow(window, 0, 0, 200, 120).toImage().pixelColor(120, 60) != sheet, + "the line did not follow the parent"); + + from = view.paint_rects.size(); + parent->setPos(20, 80); + QTRY_VERIFY(view.paint_rects.size() > from); + QVERIFY2(!view.fullyRepaintedSince(from), "the move repainted the whole viewport"); + QImage after; + QTRY_VERIFY2((after = screen->grabWindow(window, 0, 0, 200, 120).toImage()).pixelColor(120, 90) != sheet, + "the line is missing at its new place"); + QVERIFY2(after.pixelColor(120, 60) == sheet, + qPrintable(QString("the line left a trail: %1").arg(after.pixelColor(120, 60).name()))); + QVERIFY2(after.pixelColor(120, 30) == sheet, + qPrintable(QString("the first line was never erased: %1").arg(after.pixelColor(120, 30).name()))); +} + /** The inversion runs on every repaint of the folio, so a 4K viewport has to cost a few milliseconds. Reported, not asserted: the bound