From d44cbdd68996c5f5580dfaac9bb7ec2b3d314042 Mon Sep 17 00:00:00 2001 From: Ali Kianian Date: Wed, 9 Apr 2025 11:12:34 +0300 Subject: [PATCH] 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 --- .../filemanager/addobjectvisitor.cpp | 10 ++- .../filemanager/addobjectvisitor.h | 4 +- .../filemanager/qmlrefactoring.cpp | 4 +- .../designercore/filemanager/qmlrefactoring.h | 4 +- .../designercore/filemanager/qmlrewriter.cpp | 5 +- .../designercore/rewriter/rewriteaction.cpp | 89 ++++++++++--------- .../designercore/rewriter/rewriteaction.h | 80 +++++++---------- 7 files changed, 92 insertions(+), 104 deletions(-) diff --git a/src/plugins/qmldesigner/libs/designercore/filemanager/addobjectvisitor.cpp b/src/plugins/qmldesigner/libs/designercore/filemanager/addobjectvisitor.cpp index 728686ff098..873a77f8cff 100644 --- a/src/plugins/qmldesigner/libs/designercore/filemanager/addobjectvisitor.cpp +++ b/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 nodeLocation, const QString &content, Utils::span 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, - m_propertyOrder, - m_nodeLocation - 1); + QmlJS::AST::UiObjectMemberList *insertAfter; + if (m_nodeLocation.has_value()) + insertAfter = searchChildrenToInsertAfter(ast->members, m_propertyOrder, *m_nodeLocation - 1); + else + insertAfter = searchMemberToInsertAfter(ast->members, m_propertyOrder); int insertionPoint; int depth; diff --git a/src/plugins/qmldesigner/libs/designercore/filemanager/addobjectvisitor.h b/src/plugins/qmldesigner/libs/designercore/filemanager/addobjectvisitor.h index d63dcc35be7..02b3dffc870 100644 --- a/src/plugins/qmldesigner/libs/designercore/filemanager/addobjectvisitor.h +++ b/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 nodeLocation, const QString &content, Utils::span propertyOrder); @@ -26,7 +26,7 @@ private: private: quint32 m_parentLocation; - quint32 m_nodeLocation; + std::optional m_nodeLocation; QString m_content; Utils::span m_propertyOrder; }; diff --git a/src/plugins/qmldesigner/libs/designercore/filemanager/qmlrefactoring.cpp b/src/plugins/qmldesigner/libs/designercore/filemanager/qmlrefactoring.cpp index 34c0aa331a5..cf80b3d0e01 100644 --- a/src/plugins/qmldesigner/libs/designercore/filemanager/qmlrefactoring.cpp +++ b/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 nodeLocation, + const QString &content) { if (parentLocation < 0) return false; diff --git a/src/plugins/qmldesigner/libs/designercore/filemanager/qmlrefactoring.h b/src/plugins/qmldesigner/libs/designercore/filemanager/qmlrefactoring.h index 18460db592b..7d6bae12cc0 100644 --- a/src/plugins/qmldesigner/libs/designercore/filemanager/qmlrefactoring.h +++ b/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 nodeLocation, + const QString &content); bool addProperty(int parentLocation, PropertyNameView name, const QString &value, diff --git a/src/plugins/qmldesigner/libs/designercore/filemanager/qmlrewriter.cpp b/src/plugins/qmldesigner/libs/designercore/filemanager/qmlrewriter.cpp index ae0fc1c56c2..43345537685 100644 --- a/src/plugins/qmldesigner/libs/designercore/filemanager/qmlrewriter.cpp +++ b/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 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(member)) { + if (pos < 0) + break; lastObjectDef = iter; if (objectPos++ == pos) break; diff --git a/src/plugins/qmldesigner/libs/designercore/rewriter/rewriteaction.cpp b/src/plugins/qmldesigner/libs/designercore/rewriter/rewriteaction.cpp index 1035f8273a0..9b7f316d39e 100644 --- a/src/plugins/qmldesigner/libs/designercore/rewriter/rewriteaction.cpp +++ b/src/plugins/qmldesigner/libs/designercore/rewriter/rewriteaction.cpp @@ -60,42 +60,47 @@ QStringView toString(QmlRefactoring::PropertyType type) } // namespace anonymous +std::optional 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) { - const int parentLocation = positionStore.nodeOffset(m_property.parentModelNode()); + if (scheduledInHierarchy()) { + const int parentLocation = positionStore.nodeOffset(property().parentModelNode()); bool result = false; - if (m_propertyType != QmlRefactoring::ScriptBinding && m_property.isDefaultProperty()) { - const int nodeLocation = movedAfterCreation() - ? m_property.toNodeListProperty().indexOf(m_containedModelNode) - : -1; - result = refactoring.addToObjectMemberList(parentLocation, nodeLocation, m_valueText); + if (propertyType() != QmlRefactoring::ScriptBinding && property().isDefaultProperty()) { + result = refactoring.addToObjectMemberList(parentLocation, nodeLocation(), 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) { - result = refactoring.addToArrayMemberList(parentLocation, m_property.name(), m_valueText); + } else if (property().isNodeListProperty() && property().toNodeListProperty().count() > 1) { + 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(), - m_valueText, - m_propertyType, - m_property.dynamicTypeName()); + property().name(), + valueText(), + propertyType(), + property().dynamicTypeName()); if (!result) { qDebug() << "*** AddPropertyRewriteAction::execute failed in addProperty(" - << parentLocation << ',' << m_property.name() << ',' << m_valueText << "," - << toString(m_propertyType) << ") **" << info(); + << parentLocation << ',' << property().name() << ',' << valueText() << "," + << 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()), - toString(m_propertyType), - (m_property.parentModelNode().isValid() ? m_property.parentModelNode().id() + .arg(QString::fromUtf8(property().name()), + toString(propertyType()), + (property().parentModelNode().isValid() ? property().parentModelNode().id() : QLatin1String("(invalid)")), - QString(m_valueText).replace(QLatin1Char('\n'), QLatin1String("\\n")), - (m_containedModelNode.isValid() ? m_containedModelNode.id() + QString(valueText()).replace(QLatin1Char('\n'), QLatin1String("\\n")), + (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) { - const int parentLocation = positionStore.nodeOffset(m_property.parentModelNode()); + if (scheduledInHierarchy()) { + 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()) { - const int nodeLocation = movedAfterCreation() - ? m_property.toNodeListProperty().indexOf(m_containedModelNode) - : -1; - - result = refactoring.addToObjectMemberList(parentLocation, nodeLocation, m_valueText); + if (propertyType() != QmlRefactoring::ScriptBinding && property().isDefaultProperty()) { + result = refactoring.addToObjectMemberList(parentLocation, nodeLocation(), valueText()); if (!result) { qDebug() << "*** ChangePropertyRewriteAction::execute failed in addToObjectMemberList(" - << parentLocation << ',' << nodeLocation << ',' << m_valueText << ") **" + << parentLocation << ',' << nodeLocation() << ',' << valueText() << ") **" << info(); } - } else if (m_propertyType == QmlRefactoring::ArrayBinding) { - result = refactoring.addToArrayMemberList(parentLocation, m_property.name(), m_valueText); + } else if (propertyType() == QmlRefactoring::ArrayBinding) { + 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(), - m_valueText, - m_propertyType); + property().name(), + valueText(), + propertyType()); if (!result) { qDebug() << "*** ChangePropertyRewriteAction::execute failed in changeProperty(" - << parentLocation << ',' << m_property.name() << ',' << m_valueText << ',' - << toString(m_propertyType) << ") **" << info(); + << parentLocation << ',' << property().name() << ',' << valueText() << ',' + << 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()), - toString(m_propertyType), - (m_property.parentModelNode().isValid() ? m_property.parentModelNode().id() + .arg(QString::fromUtf8(property().name()), + toString(propertyType()), + (property().parentModelNode().isValid() ? property().parentModelNode().id() : QLatin1String("(invalid)")), - QString(m_valueText).replace(QLatin1Char('\n'), QLatin1String("\\n")), - (m_containedModelNode.isValid() ? m_containedModelNode.id() + QString(valueText()).replace(QLatin1Char('\n'), QLatin1String("\\n")), + (containedModelNode().isValid() ? containedModelNode().id() : QString(QStringLiteral("(none)")))); } diff --git a/src/plugins/qmldesigner/libs/designercore/rewriter/rewriteaction.h b/src/plugins/qmldesigner/libs/designercore/rewriter/rewriteaction.h index a1f77f70d64..00fa8bc04c6 100644 --- a/src/plugins/qmldesigner/libs/designercore/rewriter/rewriteaction.h +++ b/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()*/): - m_property(property), m_valueText(valueText), m_propertyType(propertyType), m_containedModelNode(containedModelNode), - m_sheduledInHierarchy(property.isValid() && property.parentModelNode().isInHierarchy()) + PropertyRewriteAction(const AbstractProperty &property, + const QString &valueText, + 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; - QString info() const override; + AbstractProperty property() const { return m_property; } - AddPropertyRewriteAction *asAddPropertyRewriteAction() override { return this; } + QString valueText() const { return m_valueText; } - AbstractProperty property() const - { return m_property; } + QmlDesigner::QmlRefactoring::PropertyType propertyType() const { return m_propertyType; } - QString valueText() const - { return m_valueText; } + ModelNode containedModelNode() const { return m_containedModelNode; } - 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 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()*/): - m_property(property), m_valueText(valueText), m_propertyType(propertyType), m_containedModelNode(containedModelNode), - m_sheduledInHierarchy(property.isValid() && property.parentModelNode().isInHierarchy()) - {} - + using PropertyRewriteAction::PropertyRewriteAction; 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