From cf6f27be06ce692bd9f147632a3aa68bdbf52818 Mon Sep 17 00:00:00 2001
From: Vlad Zahorodnii <vlad.zahorodnii@kde.org>
Date: Tue, 29 Oct 2024 17:34:38 +0200
Subject: [PATCH] scene: Fix item restacking repaints

Item::scheduleRepaint() invalidates part of an item. It doesn't
invalidate the corresponding part of the scene globally. There are a
couple of reasons why it works the way it is:

- the first, and the main one, is to keep effects like blur working
- the second is to cull out invisible repaints, i.e. repaints scheduled
  by obscured items

This kind of repaints is suitable when the size or the position of the
item changes. However, it's not good in case the stacking order changes.

For example, consider two sibling items: `[A, B]`, `B` is on top of
`A`. If the `z` of item `B` is set to `-1`, the effective stacking order
will be `[B, A]`, and only repaints for item `B` will be scheduled. When
`WorkspaceScene::preparePaintSimpleScreen()` processes the next frame, it
will see that item B is occluded by item A, so it will discard item B's
repaints and the corresponding graphics buffer will _not_ be repainted
even though it should be.

In order to prevent that, we need to schedule a global repaint.

Is there a less error prone design? Yes, if we could specify the contents
of every frame declaratively, then we could diff two consecutive frames
without manual meticulous repaint tracking. The magnitude of such changes
is beyond a bugfix though.
---
 src/scene/item.cpp | 25 ++++++++++++++++++++-----
 src/scene/item.h   |  2 ++
 2 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/src/scene/item.cpp b/src/scene/item.cpp
index 3ecaaf501a..a6586050a0 100644
--- a/src/scene/item.cpp
+++ b/src/scene/item.cpp
@@ -82,7 +82,7 @@ void Item::setZ(int z)
     if (m_parentItem) {
         m_parentItem->markSortedChildItemsDirty();
     }
-    scheduleRepaint(boundingRect());
+    scheduleSceneRepaint(boundingRect());
 }
 
 Item *Item::parentItem() const
@@ -316,8 +316,8 @@ void Item::stackBefore(Item *sibling)
     m_parentItem->m_childItems.move(selfIndex, selfIndex > siblingIndex ? siblingIndex : siblingIndex - 1);
     m_parentItem->markSortedChildItemsDirty();
 
-    scheduleRepaint(boundingRect());
-    sibling->scheduleRepaint(sibling->boundingRect());
+    scheduleSceneRepaint(boundingRect());
+    sibling->scheduleSceneRepaint(sibling->boundingRect());
 }
 
 void Item::stackAfter(Item *sibling)
@@ -344,8 +344,8 @@ void Item::stackAfter(Item *sibling)
     m_parentItem->m_childItems.move(selfIndex, selfIndex > siblingIndex ? siblingIndex + 1 : siblingIndex);
     m_parentItem->markSortedChildItemsDirty();
 
-    scheduleRepaint(boundingRect());
-    sibling->scheduleRepaint(sibling->boundingRect());
+    scheduleSceneRepaint(boundingRect());
+    sibling->scheduleSceneRepaint(sibling->boundingRect());
 }
 
 void Item::scheduleRepaint(const QRegion &region)
@@ -468,6 +468,21 @@ void Item::scheduleRepaint(const QRectF &region)
     scheduleRepaint(QRegion(region.toAlignedRect()));
 }
 
+void Item::scheduleSceneRepaint(const QRectF &region)
+{
+    scheduleSceneRepaint(QRegion(region.toAlignedRect()));
+}
+
+void Item::scheduleSceneRepaint(const QRegion &region)
+{
+    if (!m_scene) {
+        return;
+    }
+    if (isVisible()) {
+        m_scene->addRepaint(mapToScene(region));
+    }
+}
+
 bool Item::computeEffectiveVisibility() const
 {
     return m_explicitVisible && (!m_parentItem || m_parentItem->isVisible());
diff --git a/src/scene/item.h b/src/scene/item.h
index 39f2ecaaa7..8eb10b72fb 100644
--- a/src/scene/item.h
+++ b/src/scene/item.h
@@ -121,7 +121,9 @@ public:
     void setVisible(bool visible);
 
     void scheduleRepaint(const QRectF &region);
+    void scheduleSceneRepaint(const QRectF &region);
     void scheduleRepaint(const QRegion &region);
+    void scheduleSceneRepaint(const QRegion &region);
     void scheduleRepaint(SceneDelegate *delegate, const QRegion &region);
     void scheduleFrame();
     QRegion repaints(SceneDelegate *delegate) const;