From b98b683bdc350490551119a656159a8dd21f6e65 Mon Sep 17 00:00:00 2001 From: JiDe Zhang Date: Sun, 3 Jul 2022 11:38:34 +0800 Subject: [PATCH] Always update QPalette resolve mask, regardless of QBrush change 56bd1b76d2e81c58a80bf6b5d74219c6b1ab8499 changed the update resolve mask behavior in QPalette to avoid detaching brush data when modifying the resolve mask if the brush value is not changed. But this behavior broke compatibility, it introduced unknown risks, and we cannot ensure that other code in Qt does not depend on the old behavior. We both need to ensure that we don't detach when the value is not changed, and ensure that the resolveMask is always updated regardless of whether the value changes, so we need to split them up and independently share the brush data. QFont will update its corresponding resolveMask even if the value has not changed, so it is better to correct this behavior so that QPalette and QFont are consistent. [ChangeLog][QtGui][QPalette] Always update resolve mask in QPalette::setBrush, even if the value of brush has not changed. Fixes: QTBUG-98762 Change-Id: Ib845361b30f21c3d78c16ced923c1678b12e05ac Reviewed-by: JiDe Zhang Reviewed-by: Shawn Rutledge Reviewed-by: Volker Hilsheimer (cherry picked from commit 1d961491d817490da156769ddce6fce48a0bce4a) Reviewed-by: Qt Cherry-pick Bot --- src/gui/kernel/qpalette.cpp | 55 ++++++++++++------- .../auto/gui/kernel/qpalette/tst_qpalette.cpp | 5 +- 2 files changed, 37 insertions(+), 23 deletions(-) diff --git a/src/gui/kernel/qpalette.cpp b/src/gui/kernel/qpalette.cpp index 311658d26ce..ea876d935ee 100644 --- a/src/gui/kernel/qpalette.cpp +++ b/src/gui/kernel/qpalette.cpp @@ -33,12 +33,25 @@ static_assert(bitPosition(QPalette::ColorGroup(QPalette::NColorGroups - 1), class QPalettePrivate { public: - QPalettePrivate() : ref(1), ser_no(qt_palette_count++), detach_no(0) { } + class Data : public QSharedData { + public: + Data() : ser_no(qt_palette_count++) { } + + QBrush br[QPalette::NColorGroups][QPalette::NColorRoles]; + const int ser_no; + }; + + QPalettePrivate(const QExplicitlySharedDataPointer &data) + : ref(1), detach_no(0), data(data) + { } + QPalettePrivate() + : QPalettePrivate(QExplicitlySharedDataPointer(new Data)) + { } + QAtomicInt ref; - QBrush br[QPalette::NColorGroups][QPalette::NColorRoles]; QPalette::ResolveMask resolveMask = {0}; - int ser_no; int detach_no; + QExplicitlySharedDataPointer data; }; static QColor qt_mix_colors(QColor a, QColor b) @@ -710,7 +723,7 @@ const QBrush &QPalette::brush(ColorGroup gr, ColorRole cr) const gr = Active; } } - return d->br[gr][cr]; + return d->data->br[gr][cr]; } /*! @@ -748,11 +761,18 @@ void QPalette::setBrush(ColorGroup cg, ColorRole cr, const QBrush &b) cg = Active; } - if (d->br[cg][cr] != b) { + const auto newResolveMask = d->resolveMask | ResolveMask(1) << bitPosition(cg, cr); + const auto valueChanged = d->data->br[cg][cr] != b; + + if (valueChanged) { + detach(); + d->data.detach(); + d->data->br[cg][cr] = b; + } else if (d->resolveMask != newResolveMask) { detach(); - d->br[cg][cr] = b; - d->resolveMask |= ResolveMask(1) << bitPosition(cg, cr); } + + d->resolveMask = newResolveMask; } /*! @@ -793,11 +813,7 @@ bool QPalette::isBrushSet(ColorGroup cg, ColorRole cr) const void QPalette::detach() { if (d->ref.loadRelaxed() != 1) { - QPalettePrivate *x = new QPalettePrivate; - for(int grp = 0; grp < (int)NColorGroups; grp++) { - for(int role = 0; role < (int)NColorRoles; role++) - x->br[grp][role] = d->br[grp][role]; - } + QPalettePrivate *x = new QPalettePrivate(d->data); x->resolveMask = d->resolveMask; if (!d->ref.deref()) delete d; @@ -829,11 +845,11 @@ void QPalette::detach() */ bool QPalette::operator==(const QPalette &p) const { - if (isCopyOf(p)) + if (isCopyOf(p) || d->data == p.d->data) return true; for(int grp = 0; grp < (int)NColorGroups; grp++) { for(int role = 0; role < (int)NColorRoles; role++) { - if (d->br[grp][role] != p.d->br[grp][role]) + if (d->data->br[grp][role] != p.d->data->br[grp][role]) return false; } } @@ -867,7 +883,7 @@ bool QPalette::isEqual(QPalette::ColorGroup group1, QPalette::ColorGroup group2) if (group1 == group2) return true; for(int role = 0; role < (int)NColorRoles; role++) { - if (d->br[group1][role] != d->br[group2][role]) + if (d->data->br[group1][role] != d->data->br[group2][role]) return false; } return true; @@ -882,7 +898,7 @@ bool QPalette::isEqual(QPalette::ColorGroup group1, QPalette::ColorGroup group2) */ qint64 QPalette::cacheKey() const { - return (((qint64) d->ser_no) << 32) | ((qint64) (d->detach_no)); + return (((qint64) d->data->ser_no) << 32) | ((qint64) (d->detach_no)); } static constexpr QPalette::ResolveMask allResolveMask() @@ -918,7 +934,8 @@ QPalette QPalette::resolve(const QPalette &other) const for (int role = 0; role < int(NColorRoles); ++role) { for (int grp = 0; grp < int(NColorGroups); ++grp) { if (!(d->resolveMask & (ResolveMask(1) << bitPosition(ColorGroup(grp), ColorRole(role))))) { - palette.d->br[grp][role] = other.d->br[grp][role]; + palette.d->data.detach(); + palette.d->data->br[grp][role] = other.d->data->br[grp][role]; } } } @@ -981,7 +998,7 @@ QDataStream &operator<<(QDataStream &s, const QPalette &p) if (s.version() == 1) { // Qt 1.x for (int i = 0; i < NumOldRoles; ++i) - s << p.d->br[grp][oldRoles[i]].color(); + s << p.d->data->br[grp][oldRoles[i]].color(); } else { int max = (int)QPalette::NColorRoles; if (s.version() <= QDataStream::Qt_2_1) @@ -991,7 +1008,7 @@ QDataStream &operator<<(QDataStream &s, const QPalette &p) else if (s.version() <= QDataStream::Qt_5_11) max = QPalette::ToolTipText + 1; for (int r = 0; r < max; r++) - s << p.d->br[grp][r]; + s << p.d->data->br[grp][r]; } } return s; diff --git a/tests/auto/gui/kernel/qpalette/tst_qpalette.cpp b/tests/auto/gui/kernel/qpalette/tst_qpalette.cpp index 22aaaecf19a..cdb68175981 100644 --- a/tests/auto/gui/kernel/qpalette/tst_qpalette.cpp +++ b/tests/auto/gui/kernel/qpalette/tst_qpalette.cpp @@ -190,9 +190,6 @@ void tst_QPalette::setBrush() 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)); } void tst_QPalette::isBrushSet() @@ -217,7 +214,7 @@ void tst_QPalette::isBrushSet() QVERIFY(!p2.isBrushSet(QPalette::Active, QPalette::Dark)); p2.setBrush(QPalette::Active, QPalette::Dark, p2.brush(QPalette::Active, QPalette::Dark)); QVERIFY(!p3.isBrushSet(QPalette::Active, QPalette::Dark)); - QVERIFY(!p2.isBrushSet(QPalette::Active, QPalette::Dark)); + QVERIFY(p2.isBrushSet(QPalette::Active, QPalette::Dark)); } void tst_QPalette::setAllPossibleBrushes()