From e5f1e298a6d199ab9282dbc2a4e514085afd030f Mon Sep 17 00:00:00 2001 From: Eivind Kvedalen Date: Tue, 26 Jan 2016 22:22:45 +0100 Subject: [PATCH] ObjectIdentifier: Reworked resolve() function to solve issue #2389 and #2418. * Refactored code; moved mutable fields into a separate inner class. * Added resolvedProperty to ResolveResults class. * Set resolved document name, even if it does not resolve correctly, so we can give a better error message later. * If the document name is explicitly set, and it does not resolve, don't try any further. * If document name is set, use that instead of the ObjectIdentifier's owner when looking up the document object. --- src/App/ObjectIdentifier.cpp | 256 +++++++++++++++++++++-------------- src/App/ObjectIdentifier.h | 23 +++- 2 files changed, 173 insertions(+), 106 deletions(-) diff --git a/src/App/ObjectIdentifier.cpp b/src/App/ObjectIdentifier.cpp index 2c65d6791..1e4e59a32 100644 --- a/src/App/ObjectIdentifier.cpp +++ b/src/App/ObjectIdentifier.cpp @@ -115,18 +115,18 @@ ObjectIdentifier::ObjectIdentifier(const App::PropertyContainer * _owner, const : owner(_owner) , documentNameSet(false) , documentObjectNameSet(false) - , propertyIndex(-1) { if (owner) { const DocumentObject * docObj = freecad_dynamic_cast(owner); if (!docObj) throw Base::Exception("Property must be owned by a document object."); - const Document * doc = docObj->getDocument(); - - documentName = String(doc->getName(), false, true); - documentObjectName = String(docObj->getNameInDocument(), false, true); + if (property.size() > 0) { + const Document * doc = docObj->getDocument(); + documentName = String(doc->getName(), false, true); + documentObjectName = String(docObj->getNameInDocument(), false, true); + } } if (property.size() > 0) addComponent(Component::SimpleComponent(property)); @@ -141,7 +141,6 @@ ObjectIdentifier::ObjectIdentifier(const Property &prop) : owner(prop.getContainer()) , documentNameSet(false) , documentObjectNameSet(false) - , propertyIndex(-1) { DocumentObject * docObj = freecad_dynamic_cast(prop.getContainer()); @@ -163,11 +162,11 @@ ObjectIdentifier::ObjectIdentifier(const Property &prop) const std::string App::ObjectIdentifier::getPropertyName() const { - resolve(); + ResolveResults result(*this); - assert(propertyIndex >=0 && static_cast(propertyIndex) < components.size()); + assert(result.propertyIndex >=0 && static_cast(result.propertyIndex) < components.size()); - return components[propertyIndex].toString(); + return components[result.propertyIndex].toString(); } /** @@ -178,11 +177,11 @@ const std::string App::ObjectIdentifier::getPropertyName() const const App::ObjectIdentifier::Component &App::ObjectIdentifier::getPropertyComponent(int i) const { - resolve(); + ResolveResults result(*this); - assert(propertyIndex + i >=0 && static_cast(propertyIndex) + i < components.size()); + assert(result.propertyIndex + i >=0 && static_cast(result.propertyIndex) + i < components.size()); - return components[propertyIndex + i]; + return components[result.propertyIndex + i]; } /** @@ -193,14 +192,14 @@ const App::ObjectIdentifier::Component &App::ObjectIdentifier::getPropertyCompon bool ObjectIdentifier::operator ==(const ObjectIdentifier &other) const { - resolve(); - other.resolve(); + ResolveResults result1(*this); + ResolveResults result2(other); if (owner != other.owner) return false; - if (documentName != other.documentName) + if (result1.resolvedDocumentName != result2.resolvedDocumentName) return false; - if (documentObjectName != other.documentObjectName) + if (result1.resolvedDocumentObjectName != result2.resolvedDocumentObjectName) return false; if (components != other.components) return false; @@ -226,19 +225,19 @@ bool ObjectIdentifier::operator !=(const ObjectIdentifier &other) const bool ObjectIdentifier::operator <(const ObjectIdentifier &other) const { - resolve(); - other.resolve(); + ResolveResults result1(*this); + ResolveResults result2(other); - if (documentName < other.documentName) + if (result1.resolvedDocumentName < result2.resolvedDocumentName) return true; - if (documentName > other.documentName) + if (result1.resolvedDocumentName > result2.resolvedDocumentName) return false; - if (documentObjectName < other.documentObjectName) + if (result1.resolvedDocumentObjectName < result2.resolvedDocumentObjectName) return true; - if (documentObjectName > other.documentObjectName) + if (result1.resolvedDocumentObjectName > result2.resolvedDocumentObjectName) return false; if (components.size() < other.components.size()) @@ -289,7 +288,9 @@ int ObjectIdentifier::numComponents() const int ObjectIdentifier::numSubComponents() const { - return components.size() - propertyIndex; + ResolveResults result(*this); + + return components.size() - result.propertyIndex; } /** @@ -305,15 +306,14 @@ int ObjectIdentifier::numSubComponents() const std::string ObjectIdentifier::toString() const { std::stringstream s; - - resolve(); + ResolveResults result(*this); if (documentNameSet) - s << getDocumentName().toString() << "#"; + s << documentName.toString() << "#"; if (documentObjectNameSet) - s << getDocumentObjectName().toString() << "."; - else if (propertyIndex > 0) + s << documentObjectName.toString() << "."; + else if (result.propertyIndex > 0) s << components[0].toString() << "."; s << getPropertyName() << getSubPathStr(); @@ -348,17 +348,19 @@ bool ObjectIdentifier::renameDocumentObject(const std::string &oldName, const st else documentObjectName = ObjectIdentifier::String(newName, true); - resolve(); return true; } - else if (propertyIndex == 1 && documentObjectName == oldName) { - if (ExpressionParser::isTokenAnIndentifier(newName)) - components[0].name = newName; - else - components[0].name = ObjectIdentifier::String(newName, true); + else { + ResolveResults result(*this); - resolve(); - return true; + if (result.propertyIndex == 1 && result.resolvedDocumentObjectName == oldName) { + if (ExpressionParser::isTokenAnIndentifier(newName)) + components[0].name = newName; + else + components[0].name = ObjectIdentifier::String(newName, true); + + return true; + } } return false; } @@ -374,11 +376,18 @@ bool ObjectIdentifier::renameDocument(const std::string &oldName, const std::str if (oldName == newName) return false; - if (documentName == oldName) { + if (documentNameSet && documentName == oldName) { documentName = newName; - resolve(); return true; } + else { + ResolveResults result(*this); + + if (result.resolvedDocumentName == oldName) { + documentName = newName; + return true; + } + } return false; } @@ -390,10 +399,10 @@ bool ObjectIdentifier::renameDocument(const std::string &oldName, const std::str std::string ObjectIdentifier::getSubPathStr() const { - resolve(); + ResolveResults result(*this); std::stringstream s; - std::vector::const_iterator i = components.begin() + propertyIndex + 1; + std::vector::const_iterator i = components.begin() + result.propertyIndex + 1; while (i != components.end()) { s << "." << i->toString(); ++i; @@ -538,8 +547,10 @@ App::DocumentObject * ObjectIdentifier::getDocumentObject(const App::Document * // No object found with matching label, try using name directly objectById = doc->getObject(static_cast(name)); - if (name.isForceIdentifier()) + if (name.isForceIdentifier()) { + byIdentifier = true; return objectById; + } for (std::vector::iterator j = docObjects.begin(); j != docObjects.end(); ++j) { if (strcmp((*j)->Label.getValue(), static_cast(name)) == 0) { @@ -571,79 +582,109 @@ App::DocumentObject * ObjectIdentifier::getDocumentObject(const App::Document * /** * @brief Resolve the object identifier to a concrete document, documentobject, and property. * - * This method is a helper methos that updates mutable data in the object, to be used by - * other public methods of this class. + * This method is a helper method that fills out data in the given ResolveResults object. * */ -void ObjectIdentifier::resolve() const +void ObjectIdentifier::resolve(ResolveResults &results) const { - const App::Document * doc; - const App::DocumentObject * docObject; - if (freecad_dynamic_cast(owner) == 0) return; /* Document name specified? */ if (documentName.getString().size() > 0) { - doc = getDocument(documentName); + results.resolvedDocument = getDocument(documentName); + results.resolvedDocumentName = documentName; + } + else { + results.resolvedDocument = freecad_dynamic_cast(owner)->getDocument(); + results.resolvedDocumentName = String(results.resolvedDocument->getName(), false, true); } - else - doc = freecad_dynamic_cast(owner)->getDocument(); - propertyName = ""; - propertyIndex = 0; + results.propertyName = ""; + results.propertyIndex = 0; // Assume document name and object name from owner if not found - if (doc == 0) { - doc = freecad_dynamic_cast(owner)->getDocument(); - if (doc == 0) { - documentName = String(); - documentObjectName = String(); + if (results.resolvedDocument == 0) { + if (documentName.getString().size() > 0) + return; + + results.resolvedDocument = freecad_dynamic_cast(owner)->getDocument(); + if (results.resolvedDocument == 0) return; - } } - documentName = String(doc->getName(), false, documentName.isForceIdentifier()); + results.resolvedDocumentName = String(results.resolvedDocument->getName(), false, true); /* Document object name specified? */ - if (documentObjectNameSet) { + if (documentObjectName.getString().size() > 0) { bool dummy; - docObject = getDocumentObject(doc, documentObjectName, dummy); - if (!docObject) + results.resolvedDocumentObjectName = documentObjectName; + results.resolvedDocumentObject = getDocumentObject(results.resolvedDocument, documentObjectName, dummy); + if (!results.resolvedDocumentObject) return; if (components.size() > 0) { - propertyName = components[0].name.getString(); - propertyIndex = 0; + results.propertyName = components[0].name.getString(); + results.propertyIndex = 0; + results.resolvedProperty = results.resolvedDocumentObject->getPropertyByName(results.propertyName.c_str()); } else return; } else { /* Document object name not specified, resolve from path */ + + /* One component? */ if (components.size() == 1) { - documentObjectName = String(static_cast(owner)->getNameInDocument(), false, documentObjectName.isForceIdentifier()); - propertyName = components[0].name.getString(); - propertyIndex = 0; + /* Yes -- then this must be a property, so we get the document object's name from the owner */ + bool byIdentifier; + + results.resolvedDocumentObjectName = String(static_cast(owner)->getNameInDocument(), false, true); + results.resolvedDocumentObject = getDocumentObject(results.resolvedDocument, results.resolvedDocumentObjectName, byIdentifier); + results.propertyName = components[0].name.getString(); + if (results.resolvedDocumentObject) + results.resolvedProperty = results.resolvedDocumentObject->getPropertyByName(results.propertyName.c_str()); + results.propertyIndex = 0; } else if (components.size() >= 2) { + /* No -- */ bool byIdentifier; if (!components[0].isSimple()) return; - docObject = getDocumentObject(doc, components[0].name, byIdentifier); + results.resolvedDocumentObject = getDocumentObject(results.resolvedDocument, components[0].name, byIdentifier); - if (docObject) { - documentObjectName = String(components[0].name, false, byIdentifier); - propertyName = components[1].name.getString(); - propertyIndex = 1; + /* Possible to resolve component to a document object? */ + if (results.resolvedDocumentObject) { + /* Yes */ + results.resolvedDocumentObjectName = String(components[0].name, false, byIdentifier); + results.propertyName = components[1].name.getString(); + results.resolvedProperty = results.resolvedDocumentObject->getPropertyByName(results.propertyName.c_str()); + results.propertyIndex = 1; } else { - documentObjectName = String(static_cast(owner)->getNameInDocument(), false, true); - propertyName = components[0].name.getString(); - propertyIndex = 0; + + /* Document name set explicitely? */ + if (documentName.getString().size() > 0) { + /* Yes; then document object must follow */ + results.resolvedDocumentObjectName = String(components[0].name, false, false); + results.resolvedDocumentObject = results.resolvedDocument->getObject(static_cast(owner)->getNameInDocument()); + results.propertyIndex = 1; + } + else { + /* No, assume component is a property, and get document object's name from owner */ + const DocumentObject * docObj = static_cast(owner); + results.resolvedDocument = docObj->getDocument(); + results.resolvedDocumentName = String(results.resolvedDocument->getName(), false, true); + results.resolvedDocumentObjectName = String(docObj->getNameInDocument(), false, true); + results.resolvedDocumentObject = docObj->getDocument()->getObject(docObj->getNameInDocument()); + results.propertyIndex = 0; + } + results.propertyName = components[results.propertyIndex].name.getString(); + if (results.resolvedDocumentObject) + results.resolvedProperty = results.resolvedDocumentObject->getPropertyByName(results.propertyName.c_str()); } } else @@ -705,7 +746,9 @@ DocumentObject *ObjectIdentifier::getDocumentObject() const if (!doc) return 0; - return getDocumentObject(doc, documentObjectName, dummy); + ResolveResults result(*this); + + return getDocumentObject(doc, result.resolvedDocumentObjectName, dummy); } /** @@ -716,11 +759,12 @@ DocumentObject *ObjectIdentifier::getDocumentObject() const std::vector ObjectIdentifier::getStringList() const { std::vector l; + ResolveResults result(*this); if (documentNameSet) - l.push_back(documentName.toString()); + l.push_back(result.resolvedDocumentName.toString()); if (documentObjectNameSet) - l.push_back(documentObjectName.toString()); + l.push_back(result.resolvedDocumentObjectName.toString()); std::vector::const_iterator i = components.begin(); while (i != components.end()) { @@ -740,13 +784,15 @@ std::vector ObjectIdentifier::getStringList() const ObjectIdentifier ObjectIdentifier::relativeTo(const ObjectIdentifier &other) const { ObjectIdentifier result(owner); + ResolveResults thisresult(*this); + ResolveResults otherresult(other); - if (other.getDocument() != getDocument()) - result.setDocumentName(getDocumentName(), true); - if (other.getDocumentObject() != getDocumentObject()) - result.setDocumentObjectName(getDocumentObjectName(), true); + if (otherresult.resolvedDocument != thisresult.resolvedDocument) + result.setDocumentName(thisresult.resolvedDocumentName, true); + if (otherresult.resolvedDocumentObject != thisresult.resolvedDocumentObject) + result.setDocumentObjectName(thisresult.resolvedDocumentObjectName, true); - for (std::size_t i = propertyIndex; i < components.size(); ++i) + for (std::size_t i = thisresult.propertyIndex; i < components.size(); ++i) result << components[i]; return result; @@ -792,18 +838,9 @@ ObjectIdentifier &ObjectIdentifier::operator <<(const ObjectIdentifier::Componen Property *ObjectIdentifier::getProperty() const { - const App::Document * doc = getDocument(); - bool dummy; + ResolveResults result(*this); - if (!doc) - return 0; - - App::DocumentObject * docObj = getDocumentObject(doc, documentObjectName, dummy); - - if (!docObj) - return 0; - - return docObj->getPropertyByName(getPropertyComponent(0).getName().c_str()); + return result.resolvedProperty; } /** @@ -820,7 +857,9 @@ ObjectIdentifier ObjectIdentifier::canonicalPath() const // Simplify input path by ensuring that components array only has property + optional sub-properties first. ObjectIdentifier simplified(getDocumentObject()); - for (std::size_t i = propertyIndex; i < components.size(); ++i) + ResolveResults result(*this); + + for (std::size_t i = result.propertyIndex; i < components.size(); ++i) simplified << components[i]; Property * prop = getProperty(); @@ -853,8 +892,9 @@ void ObjectIdentifier::setDocumentName(const ObjectIdentifier::String &name, boo const ObjectIdentifier::String ObjectIdentifier::getDocumentName() const { - resolve(); - return documentName; + ResolveResults result(*this); + + return result.resolvedDocumentName; } /** @@ -880,8 +920,9 @@ void ObjectIdentifier::setDocumentObjectName(const ObjectIdentifier::String &nam const ObjectIdentifier::String ObjectIdentifier::getDocumentObjectName() const { - resolve(); - return documentObjectName; + ResolveResults result(*this); + + return result.resolvedDocumentObjectName; } /** @@ -1007,3 +1048,20 @@ void ObjectIdentifier::setValue(const boost::any &value) const Base::Interpreter().runString(ss.str().c_str()); } + +/** Construct and initialize a ResolveResults object, given an ObjectIdentifier instance. + * + * The constructor will invoke the ObjectIdentifier's resolve() method to initialize the object's data. + */ + +ObjectIdentifier::ResolveResults::ResolveResults(const ObjectIdentifier &oi) + : propertyIndex(-1) + , resolvedDocument(0) + , resolvedDocumentName() + , resolvedDocumentObject(0) + , resolvedDocumentObjectName() + , resolvedProperty(0) + , propertyName() +{ + oi.resolve(*this); +} diff --git a/src/App/ObjectIdentifier.h b/src/App/ObjectIdentifier.h index 9a27391ff..b77e5abad 100644 --- a/src/App/ObjectIdentifier.h +++ b/src/App/ObjectIdentifier.h @@ -227,23 +227,32 @@ public: protected: + struct ResolveResults { + + ResolveResults(const ObjectIdentifier & oi); + + int propertyIndex; + App::Document * resolvedDocument; + String resolvedDocumentName; + App::DocumentObject * resolvedDocumentObject; + String resolvedDocumentObjectName; + App::Property * resolvedProperty; + std::string propertyName; + }; + std::string getPythonAccessor() const; - void resolve() const; + void resolve(ResolveResults & results) const; App::DocumentObject *getDocumentObject(const App::Document *doc, const String &name, bool &byIdentifier) const; const App::PropertyContainer * owner; + String documentName; bool documentNameSet; + String documentObjectName; bool documentObjectNameSet; std::vector components; - /// Mutable elements, updated by resolve() - mutable int propertyIndex; - mutable String documentName; - mutable String documentObjectName; - mutable std::string propertyName; - }; std::size_t hash_value(const App::ObjectIdentifier & path);