QmlDesigner: Remove extra isValid checks for anchors

QmlItemNode::isValid() is very expensive. NodeInstance like ModelNode
etc. should already handle if it is called in an invalid state and
return false. So there is no need for the extra check.

Change-Id: Ic60f50c0e6653189bb11904b735358d3b63d1fe2
Reviewed-by: Thomas Hartmann <thomas.hartmann@qt.io>

src/plugins/qmldesigner/components/formeditor/movemanipulator.cpp

@@ -104,7 +104,8 @@ void MoveManipulator::setDirectUpdateInNodeInstances(bool directUpdate)
     const auto allFormEditorItems = m_view->scene()->allFormEditorItems();
     for (FormEditorItem *item : std::as_const(m_itemList)) {
         if (item && allFormEditorItems.contains(item) && item->qmlItemNode().isValid())
-            item->qmlItemNode().nodeInstance().setDirectUpdate(directUpdate);
+            if (auto instance = item->qmlItemNode().nodeInstance())
+                instance.setDirectUpdate(directUpdate);
     }
 }
 

src/plugins/qmldesigner/components/propertyeditor/qmlanchorbindingproxy.cpp

@@ -274,7 +274,7 @@ bool QmlAnchorBindingProxy::executeInTransaction(const QByteArray &identifier, c
 
 bool QmlAnchorBindingProxy::hasParent() const
 {
-    return m_qmlItemNode.isValid() && m_qmlItemNode.hasNodeParent();
+    return m_qmlItemNode.hasNodeParent();
 }
 
 bool QmlAnchorBindingProxy::isInLayout() const
@@ -284,41 +284,36 @@ bool QmlAnchorBindingProxy::isInLayout() const
 
 bool QmlAnchorBindingProxy::isFilled() const
 {
-    return m_qmlItemNode.isValid()
-            && hasAnchors()
-            && topAnchored()
-            && bottomAnchored()
-            && leftAnchored()
-            && rightAnchored()
-            && (m_qmlItemNode.instanceValue("anchors.topMargin").toInt() == 0)
-            && (m_qmlItemNode.instanceValue("anchors.bottomMargin").toInt() == 0)
-            && (m_qmlItemNode.instanceValue("anchors.leftMargin").toInt() == 0)
-            && (m_qmlItemNode.instanceValue("anchors.rightMargin").toInt() == 0);
+    return hasAnchors() && topAnchored() && bottomAnchored() && leftAnchored() && rightAnchored()
+           && (m_qmlItemNode.instanceValue("anchors.topMargin").toInt() == 0)
+           && (m_qmlItemNode.instanceValue("anchors.bottomMargin").toInt() == 0)
+           && (m_qmlItemNode.instanceValue("anchors.leftMargin").toInt() == 0)
+           && (m_qmlItemNode.instanceValue("anchors.rightMargin").toInt() == 0);
 }
 
 bool QmlAnchorBindingProxy::topAnchored() const
 {
-    return m_qmlItemNode.isValid() && m_qmlItemNode.anchors().instanceHasAnchor(AnchorLineTop);
+    return m_qmlItemNode.anchors().instanceHasAnchor(AnchorLineTop);
 }
 
 bool QmlAnchorBindingProxy::bottomAnchored() const
 {
-    return m_qmlItemNode.isValid() && m_qmlItemNode.anchors().instanceHasAnchor(AnchorLineBottom);
+    return m_qmlItemNode.anchors().instanceHasAnchor(AnchorLineBottom);
 }
 
 bool QmlAnchorBindingProxy::leftAnchored() const
 {
-    return m_qmlItemNode.isValid() && m_qmlItemNode.anchors().instanceHasAnchor(AnchorLineLeft);
+    return m_qmlItemNode.anchors().instanceHasAnchor(AnchorLineLeft);
 }
 
 bool QmlAnchorBindingProxy::rightAnchored() const
 {
-    return m_qmlItemNode.isValid() && m_qmlItemNode.anchors().instanceHasAnchor(AnchorLineRight);
+    return m_qmlItemNode.anchors().instanceHasAnchor(AnchorLineRight);
 }
 
 bool QmlAnchorBindingProxy::hasAnchors() const
 {
-    return m_qmlItemNode.isValid() && m_qmlItemNode.anchors().instanceHasAnchors();
+    return m_qmlItemNode.anchors().instanceHasAnchors();
 }
 
 

src/plugins/qmldesigner/instances/nodeinstance.cpp

@@ -20,7 +20,10 @@ namespace QmlDesigner {
 class ProxyNodeInstanceData
 {
 public:
-    ProxyNodeInstanceData()
+    ProxyNodeInstanceData() = default;
+
+    ProxyNodeInstanceData(const ModelNode &node)
+        : modelNode{node}
     {}
 
     qint32 parentInstanceId{-1};
@@ -58,20 +61,14 @@ public:
     QStringList allStates;
 };
 
-NodeInstance::NodeInstance() = default;
-
-NodeInstance::NodeInstance(ProxyNodeInstanceData *dPointer)
-    : d(dPointer)
+NodeInstance::NodeInstance(const ModelNode &node)
+    : d(std::make_shared<ProxyNodeInstanceData>(node))
 {
 }
 
 NodeInstance NodeInstance::create(const ModelNode &node)
 {
-    auto d = new ProxyNodeInstanceData;
-
-    d->modelNode = node;
-
-    return NodeInstance(d);
+    return NodeInstance{node};
 }
 
 NodeInstance::~NodeInstance() = default;
@@ -141,17 +138,33 @@ bool NodeInstance::hasAnchors() const
 
 QString NodeInstance::error() const
 {
-    return d->errorMessage;
+    if (d)
+        return d->errorMessage;
+
+    return {};
 }
 
 bool NodeInstance::hasError() const
 {
-    return !d->errorMessage.isEmpty();
+    if (d)
+        return !d->errorMessage.isEmpty();
+
+    return false;
 }
 
 QStringList NodeInstance::allStateNames() const
 {
-    return d->allStates;
+    if (d)
+        return d->allStates;
+
+    return {};
+}
+
+static constinit NodeInstance nullInstance;
+
+NodeInstance &NodeInstance::null()
+{
+    return nullInstance;
 }
 
 bool NodeInstance::isValid() const
@@ -405,7 +418,7 @@ qint32 NodeInstance::parentId() const
     if (isValid())
         return d->parentInstanceId;
     else
-        return false;
+        return -1;
 }
 
 bool NodeInstance::hasAnchor(PropertyNameView name) const
@@ -489,11 +502,17 @@ void NodeInstance::setProperty(PropertyNameView name, const QVariant &value)
 
 QPixmap NodeInstance::renderPixmap() const
 {
-    return d->renderPixmap;
+    if (isValid())
+        return d->renderPixmap;
+
+    return {};
 }
 
 QPixmap NodeInstance::blurredRenderPixmap() const
 {
+    if (!isValid())
+        return {};
+
     if (d->blurredRenderPixmap.isNull()) {
         d->blurredRenderPixmap = QPixmap(d->renderPixmap.size());
         QPainter blurPainter(&d->blurredRenderPixmap);

src/plugins/qmldesigner/instances/nodeinstance.h

@@ -3,7 +3,6 @@
 
 #pragma once
 
-#include <QSharedPointer>
 #include <QTransform>
 #include <QPointF>
 #include <QSizeF>
@@ -12,6 +11,8 @@
 #include "commondefines.h"
 #include "nodeinstanceglobal.h"
 
+#include <memory>
+
 namespace QmlDesigner {
 
 class ModelNode;
@@ -24,13 +25,15 @@ class NodeInstance
 
 public:
     static NodeInstance create(const ModelNode &node);
-    NodeInstance();
+    constexpr NodeInstance() = default;
     ~NodeInstance();
     NodeInstance(const NodeInstance &other);
     NodeInstance& operator=(const NodeInstance &other);
 
     ModelNode modelNode() const;
     bool isValid() const;
+
+    explicit operator bool() const { return isValid(); }
     void makeInvalid();
     QRectF boundingRect() const;
     QRectF contentItemBoundingRect() const;
@@ -71,6 +74,8 @@ public:
     bool hasError() const;
     QStringList allStateNames() const;
 
+    static NodeInstance &null();
+
 protected:
     void setProperty(PropertyNameView name, const QVariant &value);
     InformationName setInformation(InformationName name,
@@ -107,10 +112,10 @@ protected:
     void setParentId(qint32 instanceId);
     void setRenderPixmap(const QImage &image);
     bool setError(const QString &errorMessage);
-    NodeInstance(ProxyNodeInstanceData *d);
+    NodeInstance(const ModelNode &node);
 
 private:
-    QSharedPointer<ProxyNodeInstanceData> d;
+    std::shared_ptr<ProxyNodeInstanceData> d;
 };
 
 bool operator ==(const NodeInstance &first, const NodeInstance &second);

src/plugins/qmldesigner/instances/nodeinstanceview.cpp

@@ -777,12 +777,21 @@ QList<NodeInstance> NodeInstanceView::instances() const
 
     \sa NodeInstance
 */
-NodeInstance NodeInstanceView::instanceForModelNode(const ModelNode &node) const
+const NodeInstance &NodeInstanceView::instanceForModelNode(const ModelNode &node) const
+{
+    return const_cast<NodeInstanceView &>(*this).instanceForModelNode(node);
+}
+
+NodeInstance &NodeInstanceView::instanceForModelNode(const ModelNode &node)
 {
     Q_ASSERT(node.isValid());
     Q_ASSERT(m_nodeInstanceHash.contains(node));
     Q_ASSERT(m_nodeInstanceHash.value(node).modelNode() == node);
-    return m_nodeInstanceHash.value(node);
+    auto found = m_nodeInstanceHash.find(node);
+    if (found != m_nodeInstanceHash.end())
+        return *found;
+
+    return NodeInstance::null();
 }
 
 bool NodeInstanceView::hasInstanceForModelNode(const ModelNode &node) const

src/plugins/qmldesigner/instances/nodeinstanceview.h

@@ -102,7 +102,8 @@ public:
     void sceneCreated(const SceneCreatedCommand &command) override;
 
     QList<NodeInstance> instances() const;
-    NodeInstance instanceForModelNode(const ModelNode &node) const ;
+    const NodeInstance &instanceForModelNode(const ModelNode &node) const;
+    NodeInstance &instanceForModelNode(const ModelNode &node);
     bool hasInstanceForModelNode(const ModelNode &node) const;
 
     NodeInstance instanceForId(qint32 id) const;

src/plugins/qmldesigner/qmltools/qmlanchors.cpp

@@ -458,9 +458,6 @@ bool QmlAnchors::instanceHasAnchor(AnchorLineType sourceAnchorLine, SL sl) const
                                keyValue("model node", *this),
                                keyValue("caller location", sl)};
 
-    if (!qmlItemNode().isValid())
-        return false;
-
     const PropertyNameView propertyName = anchorPropertyName(sourceAnchorLine);
 
     if (sourceAnchorLine & AnchorLineFill)

src/plugins/qmldesigner/qmltools/qmlobjectnode.cpp

@@ -822,9 +822,12 @@ bool QmlObjectNode::instanceHasBinding(PropertyNameView name, SL sl) const
     return nodeInstance().hasBindingForProperty(name);
 }
 
-NodeInstance QmlObjectNode::nodeInstance() const
+const NodeInstance &QmlObjectNode::nodeInstance() const
 {
-    return nodeInstanceView()->instanceForModelNode(modelNode());
+    if (auto view = nodeInstanceView())
+        return view->instanceForModelNode(modelNode());
+
+    return NodeInstance::null();
 }
 
 QmlObjectNode QmlObjectNode::nodeForInstance(const NodeInstance &instance) const

src/plugins/qmldesigner/qmltools/qmlobjectnode.h

@@ -127,7 +127,7 @@ public:
     QList<ModelNode> getAllConnections(SL sl = {}) const;
 
 protected:
-    NodeInstance nodeInstance() const;
+    const NodeInstance &nodeInstance() const;
     QmlObjectNode nodeForInstance(const NodeInstance &instance) const;
     QmlItemNode itemForInstance(const NodeInstance &instance) const;
 };