Make QPalette::setBrush() check before detaching

Setting the same brush on the same group and role should
not detach nor alter the result of QPalette::isCopyOf().

Task-number: QTBUG-56743
Change-Id: Ic2d0dd757d703b01e8c5d835a8c124b3317653f4
Reviewed-by: Friedemann Kleint <Friedemann.Kleint@qt.io>
Reviewed-by: Andy Shaw <andy.shaw@qt.io>
Reviewed-by: Richard Moe Gustavsen <richard.gustavsen@qt.io>

2 files changed, 48 insertions, 15 deletions

diff --git a/src/gui/kernel/qpalette.cpp b/src/gui/kernel/qpalette.cpp
index 665cc430cc..4905e51e01 100644
--- a/src/gui/kernel/qpalette.cpp
+++ b/src/gui/kernel/qpalette.cpp
@@ -751,21 +751,24 @@ const QBrush &QPalette::brush(ColorGroup gr, ColorRole cr) const
 void QPalette::setBrush(ColorGroup cg, ColorRole cr, const QBrush &b)
 {
     Q_ASSERT(cr < NColorRoles);
-    detach();
-    if(cg >= (int)NColorGroups) {
-        if(cg == All) {
-            for(int i = 0; i < (int)NColorGroups; i++)
-                d->br[i][cr] = b;
-            data.resolve_mask |= (1<<cr);
-            return;
-        } else if(cg == Current) {
-            cg = (ColorGroup)data.current_group;
-        } else {
-            qWarning("QPalette::setBrush: Unknown ColorGroup: %d", (int)cg);
-            cg = Active;
-        }
+
+    if (cg == All) {
+        for (uint i = 0; i < NColorGroups; i++)
+            setBrush(ColorGroup(i), cr, b);
+        return;
+    }
+
+    if (cg == Current) {
+        cg = ColorGroup(data.current_group);
+    } else if (cg >= NColorGroups) {
+        qWarning("QPalette::setBrush: Unknown ColorGroup: %d", cg);
+        cg = Active;
+    }
+
+    if (d->br[cg][cr] != b) {
+        detach();
+        d->br[cg][cr] = b;
     }
-    d->br[cg][cr] = b;
     data.resolve_mask |= (1<<cr);
 }
 
@@ -1091,7 +1094,6 @@ void QPalette::setColorGroup(ColorGroup cg, const QBrush &foreground, const QBru
                              const QBrush &link, const QBrush &link_visited,
                              const QBrush &toolTipBase, const QBrush &toolTipText)
 {
-    detach();
     setBrush(cg, WindowText, foreground);
     setBrush(cg, Button, button);
     setBrush(cg, Light, light);
diff --git a/tests/auto/gui/kernel/qpalette/tst_qpalette.cpp b/tests/auto/gui/kernel/qpalette/tst_qpalette.cpp
index f43e8d8ee6..ca6f677ba6 100644
--- a/tests/auto/gui/kernel/qpalette/tst_qpalette.cpp
+++ b/tests/auto/gui/kernel/qpalette/tst_qpalette.cpp
@@ -39,6 +39,7 @@ private Q_SLOTS:
     void roleValues();
     void copySemantics();
     void moveSemantics();
+    void setBrush();
 };
 
 void tst_QPalette::roleValues_data()
@@ -128,5 +129,35 @@ void tst_QPalette::moveSemantics()
 #endif
 }
 
+void tst_QPalette::setBrush()
+{
+    QPalette p(Qt::red);
+    const QPalette q = p;
+    QVERIFY(q.isCopyOf(p));
+
+    // Setting a different brush will detach
+    p.setBrush(QPalette::Disabled, QPalette::Button, Qt::green);
+    QVERIFY(!q.isCopyOf(p));
+    QVERIFY(q != p);
+
+    // Check we only changed what we said we would
+    for (int i = 0; i < QPalette::NColorGroups; i++)
+        for (int j = 0; j < QPalette::NColorRoles; j++) {
+            const auto g = QPalette::ColorGroup(i);
+            const auto r = QPalette::ColorRole(j);
+            const auto b = p.brush(g, r);
+            if (g == QPalette::Disabled && r == QPalette::Button)
+                QCOMPARE(b, QBrush(Qt::green));
+            else
+                QCOMPARE(b, q.brush(g, r));
+        }
+
+    const QPalette pp = p;
+    QVERIFY(pp.isCopyOf(p));
+    // Setting the same brush won't detach
+    p.setBrush(QPalette::Disabled, QPalette::Button, Qt::green);
+    QVERIFY(pp.isCopyOf(p));
+}
+
 QTEST_MAIN(tst_QPalette)
 #include "tst_qpalette.moc"