From ad8c3ba52ab9cef0c86a5a0389e058073b384616 Mon Sep 17 00:00:00 2001 From: Miikka Heikkinen Date: Mon, 24 Jun 2024 17:50:24 +0300 Subject: [PATCH] QmlDesigner: Fix composed effect reparenting and removal issues Reparenting or removing composed effect nodes requires recursive update of the old parent to ensure all children of the old parent are rendered correctly without the effect. If the new parent of composed effect node has Image children, they can in some situations randomly lose their textures in 2D view. It seems that the only way to fix this issue is to toggle the visibility of the affected node or its ancestor off and on, so we do that to the new parent item when reparenting composed effects. Fixes: QDS-11688 Change-Id: I003d3976d619f24164938846d9b4a15201bf7b59 Reviewed-by: Mahmoud Badri --- .../effectcomposer/effectcomposermodel.cpp | 30 ++++----- .../instances/objectnodeinstance.cpp | 5 ++ .../qml2puppet/instances/objectnodeinstance.h | 1 + .../instances/qt5rendernodeinstanceserver.cpp | 67 +++++++++++++++++-- .../instances/qt5rendernodeinstanceserver.h | 2 + .../instances/quickitemnodeinstance.cpp | 9 +++ .../instances/quickitemnodeinstance.h | 2 + .../instances/servernodeinstance.cpp | 5 ++ .../qml2puppet/instances/servernodeinstance.h | 2 + 9 files changed, 100 insertions(+), 23 deletions(-) diff --git a/src/plugins/effectcomposer/effectcomposermodel.cpp b/src/plugins/effectcomposer/effectcomposermodel.cpp index 43efefe51d6..41ad13b2485 100644 --- a/src/plugins/effectcomposer/effectcomposermodel.cpp +++ b/src/plugins/effectcomposer/effectcomposermodel.cpp @@ -846,7 +846,8 @@ R"( QString parentChanged{ R"( - onParentChanged: { + function setupParentLayer() + { if (_oldParent && _oldParent !== parent) { _oldParent.layer.enabled = false _oldParent.layer.effect = null @@ -860,26 +861,21 @@ R"( if (visible) { parent.layer.enabled = true parent.layer.effect = effectComponent + %6 + %4%1%5%3 + } else { + parent.layer.enabled = false + parent.layer.effect = null + %8 + %4%2 } - %6 - %4%1%5%3 + parent.update() } } - onVisibleChanged: { - if (visible) { - parent.layer.enabled = true - parent.layer.effect = effectComponent - %6 - %4%1%5%3 - } else { - parent.layer.enabled = false - parent.layer.effect = null - %8 - %4%2 - } - parent.update() - } + onParentChanged: setupParentLayer() + + onVisibleChanged: setupParentLayer() )" }; diff --git a/src/tools/qml2puppet/qml2puppet/instances/objectnodeinstance.cpp b/src/tools/qml2puppet/qml2puppet/instances/objectnodeinstance.cpp index 2ad26c42866..460a6b6c954 100644 --- a/src/tools/qml2puppet/qml2puppet/instances/objectnodeinstance.cpp +++ b/src/tools/qml2puppet/qml2puppet/instances/objectnodeinstance.cpp @@ -173,6 +173,11 @@ bool ObjectNodeInstance::isPropertyChange() const return false; } +bool ObjectNodeInstance::isComposedEffect() const +{ + return false; +} + bool ObjectNodeInstance::equalGraphicsItem(QGraphicsItem * /*item*/) const { return false; diff --git a/src/tools/qml2puppet/qml2puppet/instances/objectnodeinstance.h b/src/tools/qml2puppet/qml2puppet/instances/objectnodeinstance.h index 3fad24e661f..7164aae8319 100644 --- a/src/tools/qml2puppet/qml2puppet/instances/objectnodeinstance.h +++ b/src/tools/qml2puppet/qml2puppet/instances/objectnodeinstance.h @@ -83,6 +83,7 @@ public: virtual bool isLayoutable() const; virtual bool isRenderable() const; virtual bool isPropertyChange() const; + virtual bool isComposedEffect() const; virtual bool equalGraphicsItem(QGraphicsItem *item) const; diff --git a/src/tools/qml2puppet/qml2puppet/instances/qt5rendernodeinstanceserver.cpp b/src/tools/qml2puppet/qml2puppet/instances/qt5rendernodeinstanceserver.cpp index 53af00ab094..c8f1d489e3e 100644 --- a/src/tools/qml2puppet/qml2puppet/instances/qt5rendernodeinstanceserver.cpp +++ b/src/tools/qml2puppet/qml2puppet/instances/qt5rendernodeinstanceserver.cpp @@ -239,7 +239,7 @@ void Qt5RenderNodeInstanceServer::changePropertyValues(const ChangeValuesCommand instance = instanceForObject(targetObject); } - if (instance.hasParent() && instance.propertyNames().contains("_isEffectItem")) + if (instance.hasParent() && instance.isComposedEffect()) makeDirtyRecursive(instance.parent()); } else if (container.isDynamic() && hasInstanceForId(container.instanceId())) { // Changes to dynamic properties are not always noticed by normal signal spy mechanism @@ -263,13 +263,68 @@ void Qt5RenderNodeInstanceServer::changePropertyBindings(const ChangeBindingsCom } } -void Qt5RenderNodeInstanceServer::makeDirtyRecursive(const ServerNodeInstance &instance) +void Qt5RenderNodeInstanceServer::reparentInstances(const ReparentInstancesCommand &command) { - const QList children = instance.childItems(); - for (const auto &child : children) { - m_dirtyInstanceSet.insert(child); - makeDirtyRecursive(child); + ServerNodeInstance effectNode; + ServerNodeInstance oldParent; + const QVector containers = command.reparentInstances(); + for (const ReparentContainer &container : containers) { + if (hasInstanceForId(container.instanceId())) { + ServerNodeInstance instance = instanceForId(container.instanceId()); + if (instance.isComposedEffect()) { + oldParent = instance.parent(); + effectNode = instance; + break; + } + } + } + + Qt5NodeInstanceServer::reparentInstances(command); + + if (oldParent.isValid()) + makeDirtyRecursive(oldParent); + if (effectNode.isValid()) { + ServerNodeInstance newParent = effectNode.parent(); + if (newParent.isValid()) { + // This is a hack to work around Image elements sometimes losing their textures when + // used as children of an effect. Toggling the visibility of the affected node seems + // to be the only way to fix this issue. + // Note that just marking the children's visibility dirty doesn't fix this issue. + QQuickItem *parentItem = newParent.rootQuickItem(); + if (parentItem && parentItem->isVisible()) { + parentItem->setVisible(false); + parentItem->setVisible(true); + } + } } } +void Qt5RenderNodeInstanceServer::removeInstances(const RemoveInstancesCommand &command) +{ + ServerNodeInstance oldParent; + const QVector ids = command.instanceIds(); + for (qint32 id : ids) { + if (hasInstanceForId(id)) { + ServerNodeInstance instance = instanceForId(id); + if (instance.isComposedEffect()) { + oldParent = instance.parent(); + break; + } + } + } + + Qt5NodeInstanceServer::removeInstances(command); + + if (oldParent.isValid()) + makeDirtyRecursive(oldParent); +} + +void Qt5RenderNodeInstanceServer::makeDirtyRecursive(const ServerNodeInstance &instance) +{ + m_dirtyInstanceSet.insert(instance); + const QList children = instance.childItems(); + for (const auto &child : children) + makeDirtyRecursive(child); +} + } // namespace QmlDesigner diff --git a/src/tools/qml2puppet/qml2puppet/instances/qt5rendernodeinstanceserver.h b/src/tools/qml2puppet/qml2puppet/instances/qt5rendernodeinstanceserver.h index 1e0f885da16..cefa1ff7453 100644 --- a/src/tools/qml2puppet/qml2puppet/instances/qt5rendernodeinstanceserver.h +++ b/src/tools/qml2puppet/qml2puppet/instances/qt5rendernodeinstanceserver.h @@ -19,6 +19,8 @@ public: void removeSharedMemory(const RemoveSharedMemoryCommand &command) override; void changePropertyValues(const ChangeValuesCommand &command) override; void changePropertyBindings(const ChangeBindingsCommand &command) override; + void reparentInstances(const ReparentInstancesCommand &command) override; + void removeInstances(const RemoveInstancesCommand &command) override; protected: void collectItemChangesAndSendChangeCommands() override; diff --git a/src/tools/qml2puppet/qml2puppet/instances/quickitemnodeinstance.cpp b/src/tools/qml2puppet/qml2puppet/instances/quickitemnodeinstance.cpp index 408864d4916..87ce62e61d4 100644 --- a/src/tools/qml2puppet/qml2puppet/instances/quickitemnodeinstance.cpp +++ b/src/tools/qml2puppet/qml2puppet/instances/quickitemnodeinstance.cpp @@ -179,6 +179,10 @@ void QuickItemNodeInstance::doComponentComplete() if (contentItemProperty.isValid()) m_contentItem = contentItemProperty.read().value(); + QQmlProperty composedEffectProperty(quickItem(), "_isEffectItem", engine()); + if (composedEffectProperty.isValid()) + m_isComposedEffect = true; + quickItem()->update(); } @@ -488,6 +492,11 @@ bool QuickItemNodeInstance::isRenderable() const return quickItem() && (!s_unifiedRenderPath || isRootNodeInstance()); } +bool QuickItemNodeInstance::isComposedEffect() const +{ + return m_isComposedEffect; +} + QList QuickItemNodeInstance::stateInstances() const { QList instanceList; diff --git a/src/tools/qml2puppet/qml2puppet/instances/quickitemnodeinstance.h b/src/tools/qml2puppet/qml2puppet/instances/quickitemnodeinstance.h index 69ee6c1671a..7291d1ba004 100644 --- a/src/tools/qml2puppet/qml2puppet/instances/quickitemnodeinstance.h +++ b/src/tools/qml2puppet/qml2puppet/instances/quickitemnodeinstance.h @@ -75,6 +75,7 @@ public: bool isMovable() const override; bool isQuickItem() const override; bool isRenderable() const override; + bool isComposedEffect() const override; QList stateInstances() const override; @@ -123,6 +124,7 @@ private: //variables double m_width; double m_height; bool m_hidden = false; + bool m_isComposedEffect = false; static bool s_createEffectItem; static bool s_unifiedRenderPath; }; diff --git a/src/tools/qml2puppet/qml2puppet/instances/servernodeinstance.cpp b/src/tools/qml2puppet/qml2puppet/instances/servernodeinstance.cpp index aa91b8ffaa7..c238fcd4588 100644 --- a/src/tools/qml2puppet/qml2puppet/instances/servernodeinstance.cpp +++ b/src/tools/qml2puppet/qml2puppet/instances/servernodeinstance.cpp @@ -158,6 +158,11 @@ bool ServerNodeInstance::isComponentWrap() const return m_nodeInstance->isComponentWrap(); } +bool ServerNodeInstance::isComposedEffect() const +{ + return m_nodeInstance->isComposedEffect(); +} + QQuickItem *ServerNodeInstance::contentItem() const { return m_nodeInstance->contentItem(); diff --git a/src/tools/qml2puppet/qml2puppet/instances/servernodeinstance.h b/src/tools/qml2puppet/qml2puppet/instances/servernodeinstance.h index 079c2fad2ba..491daf92c2c 100644 --- a/src/tools/qml2puppet/qml2puppet/instances/servernodeinstance.h +++ b/src/tools/qml2puppet/qml2puppet/instances/servernodeinstance.h @@ -54,6 +54,7 @@ public: friend class Qt5BakeLightsNodeInstanceServer; friend class Qt5PreviewNodeInstanceServer; friend class Qt5CapturePreviewNodeInstanceServer; + friend class Qt5RenderNodeInstanceServer; friend class Qt5TestNodeInstanceServer; friend class QHash; friend QHashValueType qHash(const ServerNodeInstance &instance); @@ -160,6 +161,7 @@ public: bool holdsGraphical() const; bool isComponentWrap() const; + bool isComposedEffect() const; QQuickItem *contentItem() const;