From 045250ed4258932d7fbc13cdad163db1cd64dca7 Mon Sep 17 00:00:00 2001 From: Vitaly Fanaskov Date: Fri, 22 Nov 2019 14:26:12 +0100 Subject: Fix QPalette::isBrushSet MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous implementation did not take into account different color groups in resolve mask. It led to some issues when resolving a palette or checking whether a brush is set or not. Task-number: QTBUG-78544 Change-Id: I9b67b2c444eb62c022643022a874dc400005e6ee Reviewed-by: Tor Arne Vestbø Reviewed-by: Shawn Rutledge --- src/gui/kernel/qpalette.cpp | 97 ++++++++++++++++++------- src/gui/kernel/qpalette.h | 24 +++--- src/widgets/kernel/qwidget.cpp | 4 +- src/widgets/kernel/qwidget_p.h | 4 +- src/widgets/styles/qstylesheetstyle_p.h | 2 +- tests/auto/gui/kernel/qpalette/tst_qpalette.cpp | 63 ++++++++++++++++ 6 files changed, 150 insertions(+), 44 deletions(-) diff --git a/src/gui/kernel/qpalette.cpp b/src/gui/kernel/qpalette.cpp index fdcdef4345..44efad5910 100644 --- a/src/gui/kernel/qpalette.cpp +++ b/src/gui/kernel/qpalette.cpp @@ -48,6 +48,22 @@ QT_BEGIN_NAMESPACE static int qt_palette_count = 1; +static constexpr QPalette::ResolveMask colorRoleOffset(QPalette::ColorGroup colorGroup) +{ + return QPalette::NColorRoles * colorGroup; +} + +static constexpr QPalette::ResolveMask bitPosition(QPalette::ColorGroup colorGroup, + QPalette::ColorRole colorRole) +{ + return colorRole + colorRoleOffset(colorGroup); +} + +Q_STATIC_ASSERT_X(bitPosition(QPalette::ColorGroup(QPalette::NColorGroups - 1), + QPalette::ColorRole(QPalette::NColorRoles - 1)) + < sizeof(QPalette::ResolveMask) * CHAR_BIT, + "The resolve mask type is not wide enough to fit the entire bit mask."); + class QPalettePrivate { public: QPalettePrivate() : ref(1), ser_no(qt_palette_count++), detach_no(0) { } @@ -535,8 +551,6 @@ static void qt_palette_from_color(QPalette &pal, const QColor &button) QPalette::QPalette() : d(nullptr) { - data.current_group = Active; - data.resolve_mask = 0; // Initialize to application palette if present, else default to black. // This makes it possible to instantiate QPalette outside QGuiApplication, // for example in the platform plugins. @@ -546,7 +560,7 @@ QPalette::QPalette() } else { init(); qt_palette_from_color(*this, Qt::black); - data.resolve_mask = 0; + data.resolveMask = 0; } } @@ -678,8 +692,6 @@ QPalette::~QPalette() /*!\internal*/ void QPalette::init() { d = new QPalettePrivate; - data.resolve_mask = 0; - data.current_group = Active; //as a default.. } /*! @@ -736,7 +748,7 @@ const QBrush &QPalette::brush(ColorGroup gr, ColorRole cr) const Q_ASSERT(cr < NColorRoles); if(gr >= (int)NColorGroups) { if(gr == Current) { - gr = (ColorGroup)data.current_group; + gr = data.currentGroup; } else { qWarning("QPalette::brush: Unknown ColorGroup: %d", (int)gr); gr = Active; @@ -774,7 +786,7 @@ void QPalette::setBrush(ColorGroup cg, ColorRole cr, const QBrush &b) } if (cg == Current) { - cg = ColorGroup(data.current_group); + cg = data.currentGroup; } else if (cg >= NColorGroups) { qWarning("QPalette::setBrush: Unknown ColorGroup: %d", cg); cg = Active; @@ -784,7 +796,8 @@ void QPalette::setBrush(ColorGroup cg, ColorRole cr, const QBrush &b) detach(); d->br[cg][cr] = b; } - data.resolve_mask |= (1<= NColorGroups) { + qWarning() << "Wrong color group:" << cg; + return false; + } + + if (cr >= NColorRoles) { + qWarning() << "Wrong color role:" << cr; + return false; + } + + return data.resolveMask & (ResolveMask(1) << bitPosition(cg, cr)); } /*! @@ -863,7 +894,7 @@ bool QPalette::isEqual(QPalette::ColorGroup group1, QPalette::ColorGroup group2) { if(group1 >= (int)NColorGroups) { if(group1 == Current) { - group1 = (ColorGroup)data.current_group; + group1 = data.currentGroup; } else { qWarning("QPalette::brush: Unknown ColorGroup(1): %d", (int)group1); group1 = Active; @@ -871,7 +902,7 @@ bool QPalette::isEqual(QPalette::ColorGroup group1, QPalette::ColorGroup group2) } if(group2 >= (int)NColorGroups) { if(group2 == Current) { - group2 = (ColorGroup)data.current_group; + group2 = data.currentGroup; } else { qWarning("QPalette::brush: Unknown ColorGroup(2): %d", (int)group2); group2 = Active; @@ -922,21 +953,25 @@ qint64 QPalette::cacheKey() const */ QPalette QPalette::resolve(const QPalette &other) const { - if ((*this == other && data.resolve_mask == other.data.resolve_mask) - || data.resolve_mask == 0) { + if ((*this == other && data.resolveMask == other.data.resolveMask) + || data.resolveMask == 0) { QPalette o = other; - o.data.resolve_mask = data.resolve_mask; + o.data.resolveMask = data.resolveMask; return o; } QPalette palette(*this); palette.detach(); - for(int role = 0; role < (int)NColorRoles; role++) - if (!(data.resolve_mask & (1<br[grp][role] = other.d->br[grp][role]; - palette.data.resolve_mask |= other.data.resolve_mask; + } + } + } + + palette.data.resolveMask |= other.data.resolveMask; return palette; } @@ -947,7 +982,12 @@ QPalette QPalette::resolve(const QPalette &other) const */ /*! - \fn void QPalette::resolve(uint mask) + \typedef ResolveMaskType + \internal + */ + +/*! + \fn void QPalette::resolve(ResolveMaskType mask) \internal */ @@ -1081,10 +1121,15 @@ void QPalette::setColorGroup(ColorGroup cg, const QBrush &windowText, const QBru QBrush(Qt::blue), QBrush(Qt::magenta), QBrush(toolTipBase), QBrush(toolTipText)); - data.resolve_mask &= ~(1 << Highlight); - data.resolve_mask &= ~(1 << HighlightedText); - data.resolve_mask &= ~(1 << LinkVisited); - data.resolve_mask &= ~(1 << Link); + for (int cr = Highlight; cr <= LinkVisited; ++cr) { + if (cg == All) { + for (int group = Active; group < NColorGroups; ++group) { + data.resolveMask &= ~(ResolveMask(1) << bitPosition(ColorGroup(group), ColorRole(cr))); + } + } else { + data.resolveMask &= ~(ResolveMask(1) << bitPosition(ColorGroup(cg), ColorRole(cr))); + } + } } @@ -1189,7 +1234,7 @@ QDebug operator<<(QDebug dbg, const QPalette &p) "ToolTipBase","ToolTipText", "PlaceholderText" }; QDebugStateSaver saver(dbg); QDebug nospace = dbg.nospace(); - const uint mask = p.resolve(); + auto mask = p.resolve(); nospace << "QPalette(resolve=" << Qt::hex << Qt::showbase << mask << ','; for (int role = 0; role < (int)QPalette::NColorRoles; ++role) { if (mask & (1<(data.current_group); } - inline void setCurrentColorGroup(ColorGroup cg) { data.current_group = cg; } + inline ColorGroup currentColorGroup() const { return data.currentGroup; } + inline void setCurrentColorGroup(ColorGroup cg) { data.currentGroup = cg; } inline const QColor &color(ColorGroup cg, ColorRole cr) const { return brush(cg, cr).color(); } @@ -158,9 +156,11 @@ public: #endif qint64 cacheKey() const; - QPalette resolve(const QPalette &) const; - inline uint resolve() const { return data.resolve_mask; } - inline void resolve(uint mask) { data.resolve_mask = mask; } + QPalette resolve(const QPalette &other) const; + + using ResolveMask = quint64; + inline ResolveMask resolve() const { return data.resolveMask; } + inline void resolve(ResolveMask mask) { data.resolveMask = mask; } private: void setColorGroup(ColorGroup cr, const QBrush &windowText, const QBrush &button, @@ -185,13 +185,11 @@ private: QPalettePrivate *d; struct Data { - uint current_group : 4; - uint resolve_mask : 28; - }; - union { - Data data; - quint32 for_faster_swapping_dont_use; + ResolveMask resolveMask{0}; + ColorGroup currentGroup{Active}; }; + Data data; + friend Q_GUI_EXPORT QDataStream &operator<<(QDataStream &s, const QPalette &p); }; diff --git a/src/widgets/kernel/qwidget.cpp b/src/widgets/kernel/qwidget.cpp index ecea94c66a..2160e54b8f 100644 --- a/src/widgets/kernel/qwidget.cpp +++ b/src/widgets/kernel/qwidget.cpp @@ -1850,7 +1850,7 @@ void QWidgetPrivate::propagatePaletteChange() if (q->isWindow() && !q->testAttribute(Qt::WA_WindowPropagation)) { inheritedPaletteResolveMask = 0; } - int mask = data.pal.resolve() | inheritedPaletteResolveMask; + QPalette::ResolveMask mask = data.pal.resolve() | inheritedPaletteResolveMask; const bool useStyleSheetPropagationInWidgetStyles = QCoreApplication::testAttribute(Qt::AA_UseStyleSheetPropagationInWidgetStyles); @@ -4374,7 +4374,7 @@ void QWidget::setPalette(const QPalette &palette) widget's palette are implicitly imposed on this widget by the user). Note that this font does not take into account the palette set on \a w itself. */ -QPalette QWidgetPrivate::naturalWidgetPalette(uint inheritedMask) const +QPalette QWidgetPrivate::naturalWidgetPalette(QPalette::ResolveMask inheritedMask) const { Q_Q(const QWidget); diff --git a/src/widgets/kernel/qwidget_p.h b/src/widgets/kernel/qwidget_p.h index 6915782cb3..0d0077548c 100644 --- a/src/widgets/kernel/qwidget_p.h +++ b/src/widgets/kernel/qwidget_p.h @@ -299,7 +299,7 @@ public: void setPalette_helper(const QPalette &); void resolvePalette(); - QPalette naturalWidgetPalette(uint inheritedMask) const; + QPalette naturalWidgetPalette(QPalette::ResolveMask inheritedMask) const; void setMask_sys(const QRegion &); @@ -672,7 +672,7 @@ public: // Other variables. uint directFontResolveMask; uint inheritedFontResolveMask; - uint inheritedPaletteResolveMask; + QPalette::ResolveMask inheritedPaletteResolveMask; short leftmargin; short topmargin; short rightmargin; diff --git a/src/widgets/styles/qstylesheetstyle_p.h b/src/widgets/styles/qstylesheetstyle_p.h index c5266558af..81c532bf6a 100644 --- a/src/widgets/styles/qstylesheetstyle_p.h +++ b/src/widgets/styles/qstylesheetstyle_p.h @@ -194,7 +194,7 @@ public: template struct Tampered { T oldWidgetValue; - uint resolveMask; + decltype(std::declval().resolve()) resolveMask; // only call this function on an rvalue *this (it mangles oldWidgetValue) T reverted(T current) diff --git a/tests/auto/gui/kernel/qpalette/tst_qpalette.cpp b/tests/auto/gui/kernel/qpalette/tst_qpalette.cpp index 6ce6422f48..a4764a47c0 100644 --- a/tests/auto/gui/kernel/qpalette/tst_qpalette.cpp +++ b/tests/auto/gui/kernel/qpalette/tst_qpalette.cpp @@ -41,6 +41,12 @@ private Q_SLOTS: void copySemantics(); void moveSemantics(); void setBrush(); + + void isBrushSet(); + void setAllPossibleBrushes(); + void noBrushesSetForDefaultPalette(); + void cannotCheckIfInvalidBrushSet(); + void checkIfBrushForCurrentGroupSet(); }; void tst_QPalette::roleValues_data() @@ -194,5 +200,62 @@ void tst_QPalette::setBrush() QVERIFY(pp.isCopyOf(p)); } +void tst_QPalette::isBrushSet() +{ + QPalette p; + + // Set only one color group + p.setBrush(QPalette::Active, QPalette::Mid, QBrush(Qt::red)); + QVERIFY(p.isBrushSet(QPalette::Active, QPalette::Mid)); + QVERIFY(!p.isBrushSet(QPalette::Inactive, QPalette::Mid)); + QVERIFY(!p.isBrushSet(QPalette::Disabled, QPalette::Mid)); + + // Set all color groups + p.setBrush(QPalette::LinkVisited, QBrush(Qt::green)); + QVERIFY(p.isBrushSet(QPalette::Active, QPalette::LinkVisited)); + QVERIFY(p.isBrushSet(QPalette::Inactive, QPalette::LinkVisited)); + QVERIFY(p.isBrushSet(QPalette::Disabled, QPalette::LinkVisited)); +} + +void tst_QPalette::setAllPossibleBrushes() +{ + QPalette p; + + QCOMPARE(p.resolve(), QPalette::ResolveMask(0)); + + for (int r = 0; r < QPalette::NColorRoles; ++r) { + p.setBrush(QPalette::All, QPalette::ColorRole(r), Qt::red); + } + + for (int r = 0; r < QPalette::NColorRoles; ++r) { + for (int g = 0; g < QPalette::NColorGroups; ++g) { + QVERIFY(p.isBrushSet(QPalette::ColorGroup(g), QPalette::ColorRole(r))); + } + } +} + +void tst_QPalette::noBrushesSetForDefaultPalette() +{ + QCOMPARE(QPalette().resolve(), QPalette::ResolveMask(0)); +} + +void tst_QPalette::cannotCheckIfInvalidBrushSet() +{ + QPalette p(Qt::red); + + QVERIFY(!p.isBrushSet(QPalette::All, QPalette::LinkVisited)); + QVERIFY(!p.isBrushSet(QPalette::Active, QPalette::NColorRoles)); +} + +void tst_QPalette::checkIfBrushForCurrentGroupSet() +{ + QPalette p; + + p.setCurrentColorGroup(QPalette::Disabled); + p.setBrush(QPalette::Current, QPalette::Link, QBrush(Qt::yellow)); + + QVERIFY(p.isBrushSet(QPalette::Current, QPalette::Link)); +} + QTEST_MAIN(tst_QPalette) #include "tst_qpalette.moc" -- cgit v1.2.3