QmlDesigner: Fix node order in QmlRewriter

This patch fixes the case for prepending an item to NodeListProperty

For the zero index corner case, If the order is available, we should
prepend the node. Otherwise, it should be appended.
This works for the case of NodeListProperty.

If the node order has not been changed, or is not important, we
should just append objects. This case works when we load and append
nodes consecutively, and node orders are not changed in the
transaction.

nodeLocation is optional. If it has a value, it means that we need
to consider the node location. Otherwise, we should only append.

When we search for the items to insert an object after them, if we
want to insert an object to the top (pos = -1), we should find the
current first object, and return the last non-object item found
before the first object.
If there's no object, it will be automatically return the last
non-object.

Fixes: QDS-15226
Change-Id: I5c5c23a27a956baa15a7a4eecae2fa5d4f9bb650
Reviewed-by: Knud Dollereder <knud.dollereder@qt.io>

src/plugins/qmldesigner/libs/designercore/filemanager/addobjectvisitor.cpp

@@ -10,7 +10,7 @@ using namespace QmlDesigner::Internal;
 
 AddObjectVisitor::AddObjectVisitor(QmlDesigner::TextModifier &modifier,
                                    quint32 parentLocation,
-                                   quint32 nodeLocation,
+                                   std::optional<int> nodeLocation,
                                    const QString &content,
                                    Utils::span<const PropertyNameView> propertyOrder)
     : QMLRewriter(modifier)
@@ -46,9 +46,11 @@ bool AddObjectVisitor::visit(QmlJS::AST::UiObjectDefinition *ast)
 // FIXME: duplicate code in the QmlJS::Rewriter class, remove this
 void AddObjectVisitor::insertInto(QmlJS::AST::UiObjectInitializer *ast)
 {
-    QmlJS::AST::UiObjectMemberList *insertAfter = searchChildrenToInsertAfter(ast->members,
+    QmlJS::AST::UiObjectMemberList *insertAfter;
-                                                                              m_propertyOrder,
+    if (m_nodeLocation.has_value())
-                                                                              m_nodeLocation - 1);
+        insertAfter = searchChildrenToInsertAfter(ast->members, m_propertyOrder, *m_nodeLocation - 1);
+    else
+        insertAfter = searchMemberToInsertAfter(ast->members, m_propertyOrder);
 
     int insertionPoint;
     int depth;

src/plugins/qmldesigner/libs/designercore/filemanager/addobjectvisitor.h

@@ -13,7 +13,7 @@ class AddObjectVisitor: public QMLRewriter
 public:
     AddObjectVisitor(QmlDesigner::TextModifier &modifier,
                      quint32 parentLocation,
-                     quint32 nodeLocation,
+                     std::optional<int> nodeLocation,
                      const QString &content,
                      Utils::span<const PropertyNameView> propertyOrder);
 
@@ -26,7 +26,7 @@ private:
 
 private:
     quint32 m_parentLocation;
-    quint32 m_nodeLocation;
+    std::optional<int> m_nodeLocation;
     QString m_content;
     Utils::span<const PropertyNameView> m_propertyOrder;
 };

src/plugins/qmldesigner/libs/designercore/filemanager/qmlrefactoring.cpp

@@ -77,7 +77,9 @@ bool QmlRefactoring::addToArrayMemberList(int parentLocation,
     return visit(qmlDocument->qmlProgram());
 }
 
-bool QmlRefactoring::addToObjectMemberList(int parentLocation, int nodeLocation, const QString &content)
+bool QmlRefactoring::addToObjectMemberList(int parentLocation,
+                                           std::optional<int> nodeLocation,
+                                           const QString &content)
 {
     if (parentLocation < 0)
         return false;

src/plugins/qmldesigner/libs/designercore/filemanager/qmlrefactoring.h

@@ -35,7 +35,9 @@ public:
     bool removeImport(const Import &import);
 
     bool addToArrayMemberList(int parentLocation, PropertyNameView propertyName, const QString &content);
-    bool addToObjectMemberList(int parentLocation, int nodeLocation, const QString &content);
+    bool addToObjectMemberList(int parentLocation,
+                               std::optional<int> nodeLocation,
+                               const QString &content);
     bool addProperty(int parentLocation,
                      PropertyNameView name,
                      const QString &value,

src/plugins/qmldesigner/libs/designercore/filemanager/qmlrewriter.cpp

@@ -288,9 +288,6 @@ QmlJS::AST::UiObjectMemberList *QMLRewriter::searchMemberToInsertAfter(
 QmlJS::AST::UiObjectMemberList *QMLRewriter::searchChildrenToInsertAfter(
     QmlJS::AST::UiObjectMemberList *members, Utils::span<const PropertyNameView> propertyOrder, int pos)
 {
-    if (pos < 0)
-        return searchMemberToInsertAfter(members, propertyOrder);
-
     // An empty property name should be available in the propertyOrder List, which is the right place
     // to define the objects there.
     const int objectDefinitionInsertionPoint = indexOf(propertyOrder, "");
@@ -304,6 +301,8 @@ QmlJS::AST::UiObjectMemberList *QMLRewriter::searchChildrenToInsertAfter(
         int idx = -1;
 
         if (QmlJS::AST::cast<QmlJS::AST::UiObjectDefinition *>(member)) {
+            if (pos < 0)
+                break;
             lastObjectDef = iter;
             if (objectPos++ == pos)
                 break;

src/plugins/qmldesigner/libs/designercore/rewriter/rewriteaction.cpp

@@ -60,42 +60,47 @@ QStringView toString(QmlRefactoring::PropertyType type)
 
 } // namespace anonymous
 
+std::optional<int> PropertyRewriteAction::nodeLocation() const
+{
+    if (movedAfterCreation())
+        return std::make_optional(m_property.toNodeListProperty().indexOf(m_containedModelNode));
+
+    return std::nullopt;
+}
+
 bool AddPropertyRewriteAction::execute(QmlRefactoring &refactoring, ModelNodePositionStorage &positionStore)
 {
-    if (m_sheduledInHierarchy) {
+    if (scheduledInHierarchy()) {
-        const int parentLocation = positionStore.nodeOffset(m_property.parentModelNode());
+        const int parentLocation = positionStore.nodeOffset(property().parentModelNode());
         bool result = false;
 
-        if (m_propertyType != QmlRefactoring::ScriptBinding && m_property.isDefaultProperty()) {
+        if (propertyType() != QmlRefactoring::ScriptBinding && property().isDefaultProperty()) {
-            const int nodeLocation = movedAfterCreation()
+            result = refactoring.addToObjectMemberList(parentLocation, nodeLocation(), valueText());
-                                         ? m_property.toNodeListProperty().indexOf(m_containedModelNode)
-                                         : -1;
-            result = refactoring.addToObjectMemberList(parentLocation, nodeLocation, m_valueText);
 
             if (!result) {
                 qDebug() << "*** AddPropertyRewriteAction::execute failed in addToObjectMemberList("
-                         << parentLocation << ',' << nodeLocation << ',' << m_valueText << ") **"
+                         << parentLocation << ',' << nodeLocation() << ',' << valueText() << ") **"
                          << info();
             }
-        } else if (m_property.isNodeListProperty() && m_property.toNodeListProperty().count() > 1) {
+        } else if (property().isNodeListProperty() && property().toNodeListProperty().count() > 1) {
-            result = refactoring.addToArrayMemberList(parentLocation, m_property.name(), m_valueText);
+            result = refactoring.addToArrayMemberList(parentLocation, property().name(), valueText());
 
             if (!result) {
                 qDebug() << "*** AddPropertyRewriteAction::execute failed in addToArrayMemberList("
-                         << parentLocation << ',' << m_property.name() << ',' << m_valueText
+                         << parentLocation << ',' << property().name() << ',' << valueText()
                          << ") **" << info();
             }
         } else {
             result = refactoring.addProperty(parentLocation,
-                                             m_property.name(),
+                                             property().name(),
-                                             m_valueText,
+                                             valueText(),
-                                             m_propertyType,
+                                             propertyType(),
-                                             m_property.dynamicTypeName());
+                                             property().dynamicTypeName());
 
             if (!result) {
                 qDebug() << "*** AddPropertyRewriteAction::execute failed in addProperty("
-                         << parentLocation << ',' << m_property.name() << ',' << m_valueText << ","
+                         << parentLocation << ',' << property().name() << ',' << valueText() << ","
-                         << toString(m_propertyType) << ") **" << info();
+                         << toString(propertyType()) << ") **" << info();
             }
         }
 
@@ -109,12 +114,12 @@ QString AddPropertyRewriteAction::info() const
 {
     return QStringView(u"AddPropertyRewriteAction for property \"%1\" (type: %2) of node \"%3\" "
                        u"with value >>%4<< and contained object \"%5\"")
-        .arg(QString::fromUtf8(m_property.name()),
+        .arg(QString::fromUtf8(property().name()),
-             toString(m_propertyType),
+             toString(propertyType()),
-             (m_property.parentModelNode().isValid() ? m_property.parentModelNode().id()
+             (property().parentModelNode().isValid() ? property().parentModelNode().id()
                                                      : QLatin1String("(invalid)")),
-             QString(m_valueText).replace(QLatin1Char('\n'), QLatin1String("\\n")),
+             QString(valueText()).replace(QLatin1Char('\n'), QLatin1String("\\n")),
-             (m_containedModelNode.isValid() ? m_containedModelNode.id()
+             (containedModelNode().isValid() ? containedModelNode().id()
                                              : QString(QStringLiteral("(none)"))));
 }
 
@@ -166,46 +171,42 @@ QString ChangeIdRewriteAction::info() const
 
 bool ChangePropertyRewriteAction::execute(QmlRefactoring &refactoring, ModelNodePositionStorage &positionStore)
 {
-    if (m_sheduledInHierarchy) {
+    if (scheduledInHierarchy()) {
-        const int parentLocation = positionStore.nodeOffset(m_property.parentModelNode());
+        const int parentLocation = positionStore.nodeOffset(property().parentModelNode());
         if (parentLocation < 0) {
             qWarning() << "*** ChangePropertyRewriteAction::execute ignored. Invalid node location";
             return true;
         }
         bool result = false;
 
-        if (m_propertyType != QmlRefactoring::ScriptBinding && m_property.isDefaultProperty()) {
+        if (propertyType() != QmlRefactoring::ScriptBinding && property().isDefaultProperty()) {
-            const int nodeLocation = movedAfterCreation()
+            result = refactoring.addToObjectMemberList(parentLocation, nodeLocation(), valueText());
-                                         ? m_property.toNodeListProperty().indexOf(m_containedModelNode)
-                                         : -1;
-
-            result = refactoring.addToObjectMemberList(parentLocation, nodeLocation, m_valueText);
 
             if (!result) {
                 qDebug()
                     << "*** ChangePropertyRewriteAction::execute failed in addToObjectMemberList("
-                    << parentLocation << ',' << nodeLocation << ',' << m_valueText << ") **"
+                    << parentLocation << ',' << nodeLocation() << ',' << valueText() << ") **"
                     << info();
             }
-        } else if (m_propertyType == QmlRefactoring::ArrayBinding) {
+        } else if (propertyType() == QmlRefactoring::ArrayBinding) {
-            result = refactoring.addToArrayMemberList(parentLocation, m_property.name(), m_valueText);
+            result = refactoring.addToArrayMemberList(parentLocation, property().name(), valueText());
 
             if (!result) {
                 qDebug()
                     << "*** ChangePropertyRewriteAction::execute failed in addToArrayMemberList("
-                    << parentLocation << ',' << m_property.name() << ',' << m_valueText << ") **"
+                    << parentLocation << ',' << property().name() << ',' << valueText() << ") **"
                     << info();
             }
         } else {
             result = refactoring.changeProperty(parentLocation,
-                                                m_property.name(),
+                                                property().name(),
-                                                m_valueText,
+                                                valueText(),
-                                                m_propertyType);
+                                                propertyType());
 
             if (!result) {
                 qDebug() << "*** ChangePropertyRewriteAction::execute failed in changeProperty("
-                         << parentLocation << ',' << m_property.name() << ',' << m_valueText << ','
+                         << parentLocation << ',' << property().name() << ',' << valueText() << ','
-                         << toString(m_propertyType) << ") **" << info();
+                         << toString(propertyType()) << ") **" << info();
             }
         }
 
@@ -219,12 +220,12 @@ QString ChangePropertyRewriteAction::info() const
 {
     return QStringView(u"ChangePropertyRewriteAction for property \"%1\" (type: %2) of node \"%3\" "
                        u"with value >>%4<< and contained object \"%5\"")
-        .arg(QString::fromUtf8(m_property.name()),
+        .arg(QString::fromUtf8(property().name()),
-             toString(m_propertyType),
+             toString(propertyType()),
-             (m_property.parentModelNode().isValid() ? m_property.parentModelNode().id()
+             (property().parentModelNode().isValid() ? property().parentModelNode().id()
                                                      : QLatin1String("(invalid)")),
-             QString(m_valueText).replace(QLatin1Char('\n'), QLatin1String("\\n")),
+             QString(valueText()).replace(QLatin1Char('\n'), QLatin1String("\\n")),
-             (m_containedModelNode.isValid() ? m_containedModelNode.id()
+             (containedModelNode().isValid() ? containedModelNode().id()
                                              : QString(QStringLiteral("(none)"))));
 }
 

src/plugins/qmldesigner/libs/designercore/rewriter/rewriteaction.h

@@ -48,43 +48,53 @@ public:
     RewriteAction &operator=(const RewriteAction&) = delete;
 };
 
-class AddPropertyRewriteAction: public RewriteAction
+class PropertyRewriteAction : public RewriteAction
 {
 public:
-    AddPropertyRewriteAction(const AbstractProperty &property, const QString &valueText, QmlDesigner::QmlRefactoring::PropertyType propertyType, const ModelNode &containedModelNode/* = ModelNode()*/):
+    PropertyRewriteAction(const AbstractProperty &property,
-            m_property(property), m_valueText(valueText), m_propertyType(propertyType), m_containedModelNode(containedModelNode),
+                          const QString &valueText,
-            m_sheduledInHierarchy(property.isValid() && property.parentModelNode().isInHierarchy())
+                          QmlDesigner::QmlRefactoring::PropertyType propertyType,
+                          const ModelNode &containedModelNode /* = ModelNode()*/)
+        : m_property(property)
+        , m_valueText(valueText)
+        , m_propertyType(propertyType)
+        , m_containedModelNode(containedModelNode)
+        , m_scheduledInHierarchy(property.isValid() && property.parentModelNode().isInHierarchy())
     {}
 
-    bool execute(QmlDesigner::QmlRefactoring &refactoring, ModelNodePositionStorage &positionStore) override;
+    AbstractProperty property() const { return m_property; }
-    QString info() const override;
 
-    AddPropertyRewriteAction *asAddPropertyRewriteAction() override { return this; }
+    QString valueText() const { return m_valueText; }
 
-    AbstractProperty property() const
+    QmlDesigner::QmlRefactoring::PropertyType propertyType() const { return m_propertyType; }
-    { return m_property; }
 
-    QString valueText() const
+    ModelNode containedModelNode() const { return m_containedModelNode; }
-    { return m_valueText; }
 
-    QmlDesigner::QmlRefactoring::PropertyType propertyType() const
-    { return m_propertyType; }
-
-    ModelNode containedModelNode() const
-    { return m_containedModelNode; }
-
-    bool movedAfterCreation() const { return m_movedAfterCreation; }
     void setMovedAfterCreation(bool moved) { m_movedAfterCreation = moved; }
 
+protected:
+    bool scheduledInHierarchy() const { return m_scheduledInHierarchy; }
+    bool movedAfterCreation() const { return m_movedAfterCreation; }
+    std::optional<int> nodeLocation() const;
+
 private:
     AbstractProperty m_property;
     QString m_valueText;
     QmlDesigner::QmlRefactoring::PropertyType m_propertyType;
     ModelNode m_containedModelNode;
-    bool m_sheduledInHierarchy;
+    bool m_scheduledInHierarchy;
     bool m_movedAfterCreation = false;
 };
 
+class AddPropertyRewriteAction : public PropertyRewriteAction
+{
+public:
+    using PropertyRewriteAction::PropertyRewriteAction;
+    bool execute(QmlDesigner::QmlRefactoring &refactoring, ModelNodePositionStorage &positionStore) override;
+    QString info() const override;
+    AddPropertyRewriteAction *asAddPropertyRewriteAction() override { return this; }
+};
+
 class ChangeIdRewriteAction: public RewriteAction
 {
 public:
@@ -106,41 +116,13 @@ private:
     QString m_newId;
 };
 
-class ChangePropertyRewriteAction: public RewriteAction
+class ChangePropertyRewriteAction : public PropertyRewriteAction
 {
 public:
-    ChangePropertyRewriteAction(const AbstractProperty &property, const QString &valueText, QmlDesigner::QmlRefactoring::PropertyType propertyType, const ModelNode &containedModelNode/* = ModelNode()*/):
+    using PropertyRewriteAction::PropertyRewriteAction;
-            m_property(property), m_valueText(valueText), m_propertyType(propertyType), m_containedModelNode(containedModelNode),
-            m_sheduledInHierarchy(property.isValid() && property.parentModelNode().isInHierarchy())
-    {}
-
     bool execute(QmlDesigner::QmlRefactoring &refactoring, ModelNodePositionStorage &positionStore) override;
     QString info() const override;
-
     ChangePropertyRewriteAction *asChangePropertyRewriteAction() override { return this; }
-
-    AbstractProperty property() const
-    { return m_property; }
-
-    QString valueText() const
-    { return m_valueText; }
-
-    QmlDesigner::QmlRefactoring::PropertyType propertyType() const
-    { return m_propertyType; }
-
-    ModelNode containedModelNode() const
-    { return m_containedModelNode; }
-
-    bool movedAfterCreation() const { return m_movedAfterCreation; }
-    void setMovedAfterCreation(bool moved) { m_movedAfterCreation = moved; }
-
-private:
-    AbstractProperty m_property;
-    QString m_valueText;
-    QmlDesigner::QmlRefactoring::PropertyType m_propertyType;
-    ModelNode m_containedModelNode;
-    bool m_sheduledInHierarchy;
-    bool m_movedAfterCreation;
 };
 
 class ChangeTypeRewriteAction:public RewriteAction