QmlDesigner: Fix issues with Components and Repeaters

- Fixed state preview rendering by deparenting nodes under
  component wraps.
- Ensure state preview image is always clipped to root item.
  The state preview bounding box broke when Repeater was direct
  child of root and had delegate with rotation.
- Allow dragging items under Repeater to create implicit component.
- Allow dragging Components under Repeater.
- Do not ask for target property when Component is dragged under
  any node.
- Update nodeSource properly when reparenting implicit component
  or removing last child of a Component item.
- Fixed scene update when last child of a repeater is deleted/moved.

Fixes: QDS-5197
Change-Id: Iaaf1745e25db3522ffc1dba7fd1b051da29f5aec
Reviewed-by: Samuel Ghinet <samuel.ghinet@qt.io>
Reviewed-by: Mahmoud Badri <mahmoud.badri@qt.io>
Reviewed-by: Thomas Hartmann <thomas.hartmann@qt.io>
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>

share/qtcreator/qml/qmlpuppet/qml2puppet/instances/nodeinstanceserver.cpp

@@ -397,8 +397,19 @@ void NodeInstanceServer::reparentInstances(const QVector<ReparentContainer> &con
         if (hasInstanceForId(container.instanceId())) {
             ServerNodeInstance instance = instanceForId(container.instanceId());
             if (instance.isValid()) {
-                instance.reparent(instanceForId(container.oldParentInstanceId()), container.oldParentProperty(),
-                                  instanceForId(container.newParentInstanceId()), container.newParentProperty());
+                ServerNodeInstance newParent = instanceForId(container.newParentInstanceId());
+                PropertyName newParentProperty = container.newParentProperty();
+                if (!isInformationServer()) {
+                    // Children of the component wraps are left out of the node tree to avoid
+                    // incorrectly rendering them
+                    if (newParent.isComponentWrap()) {
+                        newParent = {};
+                        newParentProperty.clear();
+                    }
+                }
+                instance.reparent(instanceForId(container.oldParentInstanceId()),
+                                  container.oldParentProperty(),
+                                  newParent, newParentProperty);
             }
         }
     }

share/qtcreator/qml/qmlpuppet/qml2puppet/instances/objectnodeinstance.cpp

@@ -415,6 +415,16 @@ bool ObjectNodeInstance::isLockedInEditor() const
     return m_isLockedInEditor;
 }
 
+bool ObjectNodeInstance::isComponentWrap() const
+{
+    return m_isComponentWrap;
+}
+
+void ObjectNodeInstance::setComponentWrap(bool wrap)
+{
+    m_isComponentWrap = wrap;
+}
+
 void ObjectNodeInstance::setModifiedFlag(bool b)
 {
     m_isModified = b;
@@ -732,6 +742,10 @@ QObject *ObjectNodeInstance::createComponentWrap(const QString &nodeSource, cons
     QQmlComponent *component = new QQmlComponent(context->engine());
 
     QByteArray data(nodeSource.toUtf8());
+    if (data.isEmpty()) {
+        // Add a fake root element as an empty component is not valid and crashes in some cases
+        data.append("QtObject{}");
+    }
     data.prepend(importCode);
     component->setData(data, context->baseUrl().resolved(QUrl("createComponent.qml")));
     QObject *object = component;

share/qtcreator/qml/qmlpuppet/qml2puppet/instances/objectnodeinstance.h

@@ -202,6 +202,9 @@ public:
     virtual void setLockedInEditor(bool b);
     bool isLockedInEditor() const;
 
+    bool isComponentWrap() const;
+    void setComponentWrap(bool wrap);
+
     void setModifiedFlag(bool b);
 
 protected:
@@ -234,6 +237,7 @@ private:
     bool m_isModified = false;
     bool m_isLockedInEditor = false;
     bool m_isHiddenInEditor = false;
+    bool m_isComponentWrap = false;
     static QHash<EnumerationName, QVariant> m_enumationValueHash;
 };
 

share/qtcreator/qml/qmlpuppet/qml2puppet/instances/qt5nodeinstanceserver.cpp

@@ -199,11 +199,24 @@ void Qt5NodeInstanceServer::markRepeaterParentDirty(qint32 id) const
     if (!hasInstanceForId(id))
         return;
 
-    // If a Repeater instance was moved/removed, the old parent must be marked dirty to rerender it
     ServerNodeInstance instance = instanceForId(id);
-    if (instance.isValid() && instance.isSubclassOf("QQuickRepeater") && instance.hasParent()) {
-        ServerNodeInstance parentInstance = instance.parent();
+    if (!instance.isValid())
+        return;
+
+    ServerNodeInstance parentInstance = instance.parent();
+    if (!parentInstance.isValid())
+        return;
+
+    // If a Repeater instance was moved/removed, the old parent must be marked dirty to rerender it
+    const QByteArray type("QQuickRepeater");
+    if (ServerNodeInstance::isSubclassOf(instance.internalObject(), type))
         DesignerSupport::addDirty(parentInstance.rootQuickItem(), QQuickDesignerSupport::Content);
+
+    // Repeater's parent must also be dirtied when a child of a repeater was moved/removed.
+    if (ServerNodeInstance::isSubclassOf(parentInstance.internalObject(), type)) {
+        ServerNodeInstance parentsParent = parentInstance.parent();
+        if (parentsParent.isValid())
+            DesignerSupport::addDirty(parentsParent.rootQuickItem(), QQuickDesignerSupport::Content);
     }
 }
 

share/qtcreator/qml/qmlpuppet/qml2puppet/instances/qt5previewnodeinstanceserver.cpp

@@ -101,6 +101,10 @@ void Qt5PreviewNodeInstanceServer::changeState(const ChangeStateCommand &/*comma
 
 QImage Qt5PreviewNodeInstanceServer::renderPreviewImage()
 {
+    // Ensure the state preview image is always clipped properly to root item dimensions
+    if (auto rootItem = qobject_cast<QQuickItem *>(rootNodeInstance().internalObject()))
+        rootItem->setClip(true);
+
     rootNodeInstance().updateDirtyNodeRecursive();
 
     QRectF boundingRect = rootNodeInstance().boundingRect();

share/qtcreator/qml/qmlpuppet/qml2puppet/instances/quickitemnodeinstance.cpp

@@ -769,6 +769,9 @@ void QuickItemNodeInstance::reparent(const ObjectNodeInstance::Pointer &oldParen
 
     ObjectNodeInstance::reparent(oldParentInstance, oldParentProperty, newParentInstance, newParentProperty);
 
+    if (!newParentInstance)
+        quickItem()->setParentItem(nullptr);
+
     if (instanceIsValidLayoutable(newParentInstance, newParentProperty)) {
         setInLayoutable(true);
         setMovable(false);

share/qtcreator/qml/qmlpuppet/qml2puppet/instances/servernodeinstance.cpp

@@ -150,6 +150,11 @@ bool ServerNodeInstance::holdsGraphical() const
     return m_nodeInstance->isQuickItem();
 }
 
+bool ServerNodeInstance::isComponentWrap() const
+{
+    return m_nodeInstance->isComponentWrap();
+}
+
 void ServerNodeInstance::updateDirtyNodeRecursive()
 {
     m_nodeInstance->updateAllDirtyNodesRecursive();
@@ -280,6 +285,8 @@ ServerNodeInstance ServerNodeInstance::create(NodeInstanceServer *nodeInstanceSe
 
     instance.internalInstance()->setInstanceId(instanceContainer.instanceId());
 
+    instance.internalInstance()->setComponentWrap(componentWrap == WrapAsComponent);
+
     instance.internalInstance()->initialize(instance.m_nodeInstance, instanceContainer.metaFlags());
 
     // Handle hidden state to initialize pickable state

share/qtcreator/qml/qmlpuppet/qml2puppet/instances/servernodeinstance.h

@@ -178,6 +178,8 @@ public:
     void updateDirtyNodeRecursive();
     bool holdsGraphical() const;
 
+    bool isComponentWrap() const;
+
 private: // functions
     ServerNodeInstance(const QSharedPointer<Internal::ObjectNodeInstance> &abstractInstance);
 

src/plugins/qmldesigner/components/navigator/navigatortreemodel.cpp

@@ -732,17 +732,22 @@ void NavigatorTreeModel::handleItemLibraryItemDrop(const QMimeData *mimeData, in
                     ModelNode targetNode = targetProperty.parentModelNode();
                     NodeMetaInfo metaInfo = targetNode.metaInfo();
                     TypeName typeName = newModelNode.type();
-                    const PropertyNameList nameList = targetNode.metaInfo().directPropertyNames();
-                    for (const auto &propertyName : nameList) {
-                        auto testType = metaInfo.propertyTypeName(propertyName);
-                        if (testType == typeName || newModelNode.isSubclassOf(testType)) {
-                            ChooseFromPropertyListDialog *dialog = nullptr;
-                            dialog = new ChooseFromPropertyListDialog(targetNode, testType, Core::ICore::dialogParent());
-                            dialog->exec();
-                            if (dialog->result() == QDialog::Accepted)
-                                targetNode.bindingProperty(dialog->selectedProperty()).setExpression(newModelNode.validId());
-                            delete dialog;
-                            break;
+
+                    // Empty components are not supported and having one as property value is generally
+                    // unstable, so let's not offer user to put a fresh Component into a property
+                    if (typeName != "QtQml.Component") {
+                        const PropertyNameList nameList = targetNode.metaInfo().directPropertyNames();
+                        for (const auto &propertyName : nameList) {
+                            auto testType = metaInfo.propertyTypeName(propertyName);
+                            if (testType == typeName || newModelNode.isSubclassOf(testType)) {
+                                ChooseFromPropertyListDialog *dialog = nullptr;
+                                dialog = new ChooseFromPropertyListDialog(targetNode, testType, Core::ICore::dialogParent());
+                                dialog->exec();
+                                if (dialog->result() == QDialog::Accepted)
+                                    targetNode.bindingProperty(dialog->selectedProperty()).setExpression(newModelNode.validId());
+                                delete dialog;
+                                break;
+                            }
                         }
                     }
                 }
@@ -1087,7 +1092,9 @@ void NavigatorTreeModel::moveNodesInteractive(NodeAbstractProperty &parentProper
                     && !modelNode.isAncestorOf(parentProperty.parentModelNode())
                     && (modelNode.metaInfo().isSubclassOf(propertyQmlType)
                         || propertyQmlType == "alias"
-                        || parentProperty.name() == "data")) {
+                        || parentProperty.name() == "data"
+                        || (parentProperty.parentModelNode().metaInfo().defaultPropertyName() == parentProperty.name()
+                            && propertyQmlType == "<cpp>.QQmlComponent"))) {
                 //### todo: allowing alias is just a heuristic
                 //once the MetaInfo is part of instances we can do this right
 
@@ -1097,6 +1104,10 @@ void NavigatorTreeModel::moveNodesInteractive(NodeAbstractProperty &parentProper
                 // When the actual reparenting happens, model will create the "data" property if
                 // it is missing.
 
+                // We allow move even if target property type doesn't match, if the target property
+                // is the default property of the parent and is of Component type.
+                // In that case an implicit component will be created.
+
                 bool nodeCanBeMovedToParentProperty = removeModelNodeFromNodeProperty(parentProperty, modelNode);
                 if (nodeCanBeMovedToParentProperty) {
                     reparentModelNodeToNodeProperty(parentProperty, modelNode);

src/plugins/qmldesigner/designercore/model/texttomodelmerger.cpp

@@ -1191,6 +1191,8 @@ void TextToModelMerger::syncNode(ModelNode &modelNode,
 
     if (isComponentType(typeName) || isImplicitComponent)
         setupComponentDelayed(modelNode, differenceHandler.isAmender());
+    else if (!modelNode.nodeSource().isEmpty() || modelNode.nodeSourceType() != ModelNode::NodeWithoutSource)
+        clearImplicitComponentDelayed(modelNode, differenceHandler.isAmender());
 
     if (isCustomParserType(typeName))
         setupCustomParserNodeDelayed(modelNode, differenceHandler.isAmender());
@@ -2079,18 +2081,23 @@ void TextToModelMerger::setupComponent(const ModelNode &node)
 
     QString componentText = m_rewriterView->extractText({node}).value(node);
 
-    if (componentText.isEmpty())
+    if (componentText.isEmpty() && node.nodeSource().isEmpty())
         return;
 
     QString result = extractComponentFromQml(componentText);
 
-    if (result.isEmpty())
+    if (result.isEmpty() && node.nodeSource().isEmpty())
         return; //No object definition found
 
     if (node.nodeSource() != result)
         ModelNode(node).setNodeSource(result, ModelNode::NodeWithComponentSource);
 }
 
+void TextToModelMerger::clearImplicitComponent(const ModelNode &node)
+{
+    ModelNode(node).setNodeSource({}, ModelNode::NodeWithoutSource);
+}
+
 void TextToModelMerger::collectLinkErrors(QList<DocumentMessage> *errors, const ReadingContext &ctxt)
 {
     foreach (const QmlJS::DiagnosticMessage &diagnosticMessage, ctxt.diagnosticLinkMessages()) {
@@ -2237,9 +2244,9 @@ void TextToModelMerger::addIsoIconQrcMapping(const QUrl &fileUrl)
     } while (dir.cdUp());
 }
 
-void TextToModelMerger::setupComponentDelayed(const ModelNode &node, bool synchron)
+void TextToModelMerger::setupComponentDelayed(const ModelNode &node, bool synchronous)
 {
-    if (synchron) {
+    if (synchronous) {
         setupComponent(node);
     } else {
         m_setupComponentList.insert(node);
@@ -2254,7 +2261,7 @@ void TextToModelMerger::setupCustomParserNode(const ModelNode &node)
 
     QString modelText = m_rewriterView->extractText({node}).value(node);
 
-    if (modelText.isEmpty())
+    if (modelText.isEmpty() && node.nodeSource().isEmpty())
         return;
 
     if (node.nodeSource() != modelText)
@@ -2262,11 +2269,11 @@ void TextToModelMerger::setupCustomParserNode(const ModelNode &node)
 
 }
 
-void TextToModelMerger::setupCustomParserNodeDelayed(const ModelNode &node, bool synchron)
+void TextToModelMerger::setupCustomParserNodeDelayed(const ModelNode &node, bool synchronous)
 {
     Q_ASSERT(isCustomParserType(node.type()));
 
-    if (synchron) {
+    if (synchronous) {
         setupCustomParserNode(node);
     } else {
         m_setupCustomParserList.insert(node);
@@ -2274,15 +2281,32 @@ void TextToModelMerger::setupCustomParserNodeDelayed(const ModelNode &node, bool
     }
 }
 
+void TextToModelMerger::clearImplicitComponentDelayed(const ModelNode &node, bool synchronous)
+{
+    Q_ASSERT(!isComponentType(node.type()));
+
+    if (synchronous) {
+        clearImplicitComponent(node);
+    } else {
+        m_clearImplicitComponentList.insert(node);
+        m_setupTimer.start();
+    }
+}
+
 void TextToModelMerger::delayedSetup()
 {
-    foreach (const ModelNode node, m_setupComponentList)
+    for (const ModelNode &node : std::as_const(m_setupComponentList))
         setupComponent(node);
 
-    foreach (const ModelNode node, m_setupCustomParserList)
+    for (const ModelNode &node : std::as_const(m_setupCustomParserList))
         setupCustomParserNode(node);
+
+    for (const ModelNode &node : std::as_const(m_clearImplicitComponentList))
+        clearImplicitComponent(node);
+
     m_setupCustomParserList.clear();
     m_setupComponentList.clear();
+    m_clearImplicitComponentList.clear();
 }
 
 QSet<QPair<QString, QString> > TextToModelMerger::qrcMapping() const

src/plugins/qmldesigner/designercore/model/texttomodelmerger.h

@@ -128,8 +128,9 @@ public:
                                       ReadingContext *context,
                                       DifferenceHandler &differenceHandler);
 
-    void setupComponentDelayed(const ModelNode &node, bool synchron);
-    void setupCustomParserNodeDelayed(const ModelNode &node, bool synchron);
+    void setupComponentDelayed(const ModelNode &node, bool synchronous);
+    void setupCustomParserNodeDelayed(const ModelNode &node, bool synchronous);
+    void clearImplicitComponentDelayed(const ModelNode &node, bool synchronous);
 
     void delayedSetup();
 
@@ -140,6 +141,7 @@ public:
 private:
     void setupCustomParserNode(const ModelNode &node);
     void setupComponent(const ModelNode &node);
+    void clearImplicitComponent(const ModelNode &node);
     void collectLinkErrors(QList<DocumentMessage> *errors, const ReadingContext &ctxt);
     void collectImportErrors(QList<DocumentMessage> *errors);
     void collectSemanticErrorsAndWarnings(QList<DocumentMessage> *errors,
@@ -163,6 +165,7 @@ private:
     QTimer m_setupTimer;
     QSet<ModelNode> m_setupComponentList;
     QSet<ModelNode> m_setupCustomParserList;
+    QSet<ModelNode> m_clearImplicitComponentList;
     QmlJS::ViewerContext m_vContext;
     QSet<QPair<QString, QString> > m_qrcMapping;
     QSet<QmlJS::ImportKey> m_possibleImportKeys;