From 6b372448661cade132e89eec094902dbbf15ba2a Mon Sep 17 00:00:00 2001 From: yeshanshan Date: Tue, 20 Oct 2026 21:40:52 +0800 Subject: [PATCH] fix: prevent palette feedback binding loops MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. Store palette changed connection and disconnect it when control changes to avoid stale connections 2. Add null check for palette pointer before connecting to changed signal 3. Defer updateControlTheme via scheduleControlThemeUpdate using queued connection to break binding feedback loops 4. Add m_controlThemeUpdatePending flag to coalesce multiple update requests 5. Add unit tests for deferred/coalesced palette updates, destruction with pending updates, and styled control palette feedback The palette is both an input and output of the color selector, causing binding loops when palette changes triggered immediate writes back to the same palette. Deferring the update to the next event loop iteration allows existing bindings to finish before notifying again. The connection cleanup prevents dangling connections when the control's palette changes. Influence: 1. Test controls with palette bindings to D.ColorSelector (Button, ToolButton, MenuItem) 2. Verify no "Binding loop" warnings appear in console 3. Test theme switching between light and dark modes 4. Test hover, press, checked, disabled states with palette feedback 5. Verify selector destruction while a palette update is pending does not crash 6. Confirm palette updates settle and do not trigger continuous event processing fix: 修复调色板反馈导致的绑定循环 1. 保存调色板 changed 信号连接，在控件切换时断开以避免遗留连接 2. 连接 changed 信号前增加调色板空指针检查 3. 通过 scheduleControlThemeUpdate 使用队列连接延迟 updateControlTheme 以打破绑定反馈循环 4. 添加 m_controlThemeUpdatePending 标志合并多次更新请求 5. 添加延迟/合并调色板更新、销毁时存在待处理更新、以及带样式控件调色板反 馈的单元测试 调色板既是颜色选择器的输入也是输出，当调色板变化立即回写同一调色板时会触 发绑定循环。将更新延迟到下一个事件循环迭代，使已有绑定先完成再通知。连接 清理可避免控件调色板变化时产生悬空连接。 Influence: 1. 测试使用 D.ColorSelector 调色板绑定的控件（Button、ToolButton、 MenuItem） 2. 验证控制台不出现"Binding loop"警告 3. 测试明暗主题切换 4. 测试 hover、按下、选中、禁用状态下的调色板反馈 5. 验证存在待处理调色板更新时销毁选择器不会崩溃 6. 确认调色板更新能稳定下来，不会触发持续的事件处理 --- src/private/dquickcontrolpalette.cpp | 22 ++++- src/private/dquickcontrolpalette_p.h | 3 + tests/ut_colorselector.cpp | 138 ++++++++++++++++++++++++++- 3 files changed, 158 insertions(+), 5 deletions(-) diff --git a/src/private/dquickcontrolpalette.cpp b/src/private/dquickcontrolpalette.cpp index a8ea3dcc0..50c42b5be 100644 --- a/src/private/dquickcontrolpalette.cpp +++ b/src/private/dquickcontrolpalette.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #if QT_VERSION >= QT_VERSION_CHECK(6, 0, 0) #include #endif @@ -458,6 +459,7 @@ void DQuickControlColorSelector::setControl(QQuickItem *newControl) return; // Clean for old + disconnect(m_controlPaletteConnection); if (m_control) { m_control->disconnect(this); if (auto w = m_control->window()) @@ -469,14 +471,16 @@ void DQuickControlColorSelector::setControl(QQuickItem *newControl) if (m_control) { #if QT_VERSION >= QT_VERSION_CHECK(6, 0, 0) auto palette = m_control->property("palette").value(); - connect(palette, &QQuickPalette::changed, this, &DQuickControlColorSelector::updateControlTheme); + if (palette) + m_controlPaletteConnection = connect(palette, &QQuickPalette::changed, + this, &DQuickControlColorSelector::scheduleControlThemeUpdate); #endif connect(m_control, &QObject::destroyed, this, [this] { m_controlWindow = nullptr; m_control = nullptr; }); if (m_control->metaObject()->indexOfSignal("paletteChanged()") != -1) { - connect(m_control, SIGNAL(paletteChanged()), this, SLOT(updateControlTheme())); + connect(m_control, SIGNAL(paletteChanged()), this, SLOT(scheduleControlThemeUpdate())); } if (m_control->metaObject()->indexOfSignal("hoveredChanged()") != -1) { connect(m_control, SIGNAL(hoveredChanged()), this, SLOT(updateControlState())); @@ -1010,6 +1014,20 @@ void DQuickControlColorSelector::updateFamilyForChildrenRecu(QQuickItem *parent) } } +void DQuickControlColorSelector::scheduleControlThemeUpdate() +{ + if (m_controlThemeUpdatePending) + return; + + m_controlThemeUpdatePending = true; + // Palette changes can originate from a binding to one of our colors. + // Wait until that binding has finished before notifying it again. + QMetaObject::invokeMethod(this, [this] { + m_controlThemeUpdatePending = false; + updateControlTheme(); + }, Qt::QueuedConnection); +} + void DQuickControlColorSelector::updateControlTheme() { if (!m_control) diff --git a/src/private/dquickcontrolpalette_p.h b/src/private/dquickcontrolpalette_p.h index fb1631989..49bf8d8af 100644 --- a/src/private/dquickcontrolpalette_p.h +++ b/src/private/dquickcontrolpalette_p.h @@ -352,6 +352,7 @@ class DQuickControlColorSelector : public QObject QByteArray findPalettePropertyName(const DQuickControlPalette *palette) const; void clearAndInheritParentProperties(); + Q_SLOT void scheduleControlThemeUpdate(); Q_SLOT void updateControlTheme(); Q_SLOT bool updateControlState(); Q_SLOT void updateAllColorProperties(); @@ -371,6 +372,8 @@ class DQuickControlColorSelector : public QObject void setFamilyPropertyParent(DQuickControlColorSelector *parent); QPointer m_control; + QMetaObject::Connection m_controlPaletteConnection; + bool m_controlThemeUpdatePending = false; QPointer m_controlWindow; QPointer m_superColorSelector; QPointer m_parentOfFamilyProperty; diff --git a/tests/ut_colorselector.cpp b/tests/ut_colorselector.cpp index 3a7547ebe..ce749ccd4 100644 --- a/tests/ut_colorselector.cpp +++ b/tests/ut_colorselector.cpp @@ -1,4 +1,4 @@ -// SPDX-FileCopyrightText: 2023 UnionTech Software Technology Co., Ltd. +// SPDX-FileCopyrightText: 2023 - 2026 UnionTech Software Technology Co., Ltd. // // SPDX-License-Identifier: LGPL-3.0-or-later @@ -8,6 +8,13 @@ #include #include #include +#include +#include +#include + +#include "dquickcontrolpalette_p.h" + +DQUICK_USE_NAMESPACE class ut_ColorSelector : public ::testing::Test { @@ -27,6 +34,10 @@ void ut_ColorSelector::TearDown() TEST_F(ut_ColorSelector, getColorFromProperty) { ControlHelper helper("qrc:/qml/ColorSelector_GetColorFromProperty.qml"); + const auto cleanup = qScopeGuard([&helper] { + delete helper.object; + helper.object = nullptr; + }); ASSERT_TRUE(helper.object); auto listView = helper.object->property("view").value(); @@ -41,7 +52,128 @@ TEST_F(ut_ColorSelector, getColorFromProperty) QQmlEngine &engine = helper.engine; engine.globalObject().setProperty("background", engine.newQObject(background)); - ThemeTypeGuard themeGurad(DGuiApplicationHelper::LightType); - Q_UNUSED(themeGurad); + ASSERT_TRUE(QQmlProperty::write(helper.object, "palette.window", QColor(Qt::white))); + QTest::qWaitFor([&] { + return background->property("color") + == engine.evaluate("background.color1.normal.common.color()").toVariant(); + }); ASSERT_EQ(background->property("color"), engine.evaluate("background.color1.normal.common.color()").toVariant()); } + +// Use a template control to exercise palette feedback without unrelated style +// assets. The palette is both an input and an output of the color selector. +static const char paletteFeedbackQml[] = +#if QT_VERSION >= QT_VERSION_CHECK(6, 0, 0) + "import QtQuick\nimport QtQuick.Templates as T\n" +#else + "import QtQuick 2.11\nimport QtQuick.Templates 2.4 as T\n" +#endif +R"( +import org.deepin.dtk 1.0 as D + +T.Button { + property D.Palette textColor: D.Palette { + normal: ("red") + normalDark: ("blue") + } + palette.window: "white" + palette.windowText: D.ColorSelector.textColor +} +)"; + +TEST_F(ut_ColorSelector, paletteUpdatesAreDeferredAndCoalesced) +{ + QList warnings; + ControlHelper helper; + QObject::connect(&helper.engine, &QQmlEngine::warnings, &helper.engine, + [&warnings](const QList &errors) { warnings.append(errors); }); + ASSERT_TRUE(helper.setData(paletteFeedbackQml)); + const auto cleanup = qScopeGuard([&helper] { + delete helper.object; + helper.object = nullptr; + }); + auto selector = qobject_cast( + qmlAttachedPropertiesObject(helper.object)); + ASSERT_NE(selector, nullptr); + QCoreApplication::processEvents(); + QSignalSpy themeChanges(selector, &DQuickControlColorSelector::controlThemeChanged); + QSignalSpy colorChanges(selector, &DQuickControlColorSelector::colorPropertyChanged); + + ASSERT_TRUE(QQmlProperty::write(helper.object, "palette.window", QColor(Qt::black))); + ASSERT_TRUE(QQmlProperty::write(helper.object, "palette.window", QColor(Qt::white))); + EXPECT_EQ(themeChanges.count(), 0); + QCoreApplication::processEvents(); + EXPECT_EQ(themeChanges.count(), 0); + EXPECT_EQ(colorChanges.count(), 0); + + ASSERT_TRUE(QQmlProperty::write(helper.object, "palette.window", QColor(Qt::black))); + EXPECT_EQ(themeChanges.count(), 0); + QCoreApplication::processEvents(); + EXPECT_EQ(themeChanges.count(), 1); + EXPECT_EQ(QQmlProperty::read(helper.object, "palette.windowText").value(), QColor(Qt::blue)); + + // The palette writeback must settle rather than continuously queue updates. + const int settledChanges = colorChanges.count(); + for (int i = 0; i < 5; ++i) + QCoreApplication::processEvents(); + EXPECT_EQ(colorChanges.count(), settledChanges); + EXPECT_FALSE(selector->m_controlThemeUpdatePending); + for (const auto &warning : warnings) + EXPECT_FALSE(warning.description().contains("Binding loop")) << warning.toString().toStdString(); +} + +TEST_F(ut_ColorSelector, destroyWithPendingPaletteUpdate) +{ + ControlHelper helper; + ASSERT_TRUE(helper.setData(paletteFeedbackQml)); + QPointer selector = qmlAttachedPropertiesObject(helper.object); + ASSERT_TRUE(QQmlProperty::write(helper.object, "palette.window", QColor(Qt::black))); + delete helper.object; + helper.object = nullptr; + EXPECT_TRUE(selector.isNull()); + QCoreApplication::processEvents(); +} + +TEST_F(ut_ColorSelector, styledControlsPaletteFeedback) +{ + for (const QByteArray &type : {QByteArray("Button"), QByteArray("ToolButton"), QByteArray("MenuItem")}) { + SCOPED_TRACE(type.constData()); + QList warnings; + ControlHelper helper; + QObject::connect(&helper.engine, &QQmlEngine::warnings, &helper.engine, + [&warnings](const QList &errors) { warnings.append(errors); }); + ASSERT_TRUE(helper.setData("import QtQuick 2.11\nimport org.deepin.dtk 1.0 as D\nD." + + type + " { text: \"Palette feedback\" }")); + const auto cleanup = qScopeGuard([&helper] { + delete helper.object; + helper.object = nullptr; + }); + auto selector = qobject_cast( + qmlAttachedPropertiesObject(helper.object)); + ASSERT_NE(selector, nullptr); + QSignalSpy changes(selector, &DQuickControlColorSelector::colorPropertyChanged); + for (bool dark : {false, true}) { + ASSERT_TRUE(QQmlProperty::write(helper.object, "palette.window", + QColor(dark ? Qt::black : Qt::white))); + for (bool inactive : {true, false}) { + selector->setInactived(inactive); + for (int state = 0; state < 5; ++state) { + selector->setHovered(state == 1); + selector->setPressed(state == 2); + helper.object->setEnabled(state != 3); + helper.object->setProperty("checked", state == 4); + for (int i = 0; i < 5; ++i) + QCoreApplication::processEvents(); + EXPECT_EQ(QQmlProperty::read(helper.object, "palette.windowText").value(), + selector->property("textColor").value()); + const int settledChanges = changes.count(); + QCoreApplication::processEvents(); + EXPECT_EQ(changes.count(), settledChanges); + EXPECT_FALSE(selector->m_controlThemeUpdatePending); + } + } + } + for (const auto &warning : warnings) + EXPECT_FALSE(warning.description().contains("Binding loop")) << warning.toString().toStdString(); + } +}