From 41f09db9e10eca1779b70bfed3a8f6fd40465a64 Mon Sep 17 00:00:00 2001 From: Kevin Martin Date: Mon, 24 Feb 2025 11:23:53 -0500 Subject: [PATCH] Address performance of existing unique-name generation (Part 2) (#18676) As described in Issue 16849, the existing Tools::getUniqueName method requires calling code to form a vector of existing names to be avoided. This leads to poor performance both in the O(n) cost of building such a vector and also getUniqueName's O(n) algorithm for actually generating the unique name (where 'n' is the number of pre-existing names). This has particularly noticeable cost in documents with large numbers of DocumentObjects because generating both Names and Labels for each new object incurs this cost. During an operation such as importing this results in an O(n^2) time spent generating names. The other major cost is in the saving of the temporary backup file, which uses name generation for the "files" embedded in the Zip file. Documents can easily need several such "files" for each object in the document. This update includes the following changes to use the newly-added UniqueNameManager as a replacement for the old Tools::getUniqueName method and deletes the latter to remove any temptation to use it as its usage model breeds inefficiency: Eliminate Tools::getUniqueName, its local functions, and its unit tests. Make DocumentObject naming use the new UniqueNameManager class. Make DocumentObject Label naming use the new UniqueNameManager class. This needs to monitor DocumentObject Labels for changes since this property is not read-only. The special handling for the Label property, which includes optionally forcing uniqueness and updating links in referencing objects, has been mostly moved from PropertyString to DocumentObject. Add Document::containsObject(DocumentObject*) for a definitive test of an object being in a Document. This is needed because DocumentObjects can be in a sort of limbo (e.g. when they are in the Undo/Redo lists) where they have a parent linkage to the Document but should not participate in Label collision checks. Rename Document.getStandardObjectName to getStandardObjectLabel to better represent what it does. Use new UniqueNameManager for Writer internal filenames within the zip file. Eliminate unneeded Reader::FileNames collection. The file names already exist in the FileList collection elements. The only existing use for the FileNames collection was to determine if there were any files at all, and with FileList and FileNames being parallel vectors, they both had the same length so FileList could be used for this test.. Use UniqueNameManager for document names and labels. This uses ad hoc UniqueNameManager objects created on the spot on the assumption that document creation is relatively rare and there are few documents, so although the cost is O(n), n itself is small. Use an ad hoc UniqueNameManager to name new DymanicProperty entries. This is only done if a property of the proposed name already exists, since such a check is more-or-less O(log(n)), almost never finds a collision, and avoids the O(n) building of the UniqueNameManager. If there is a collision an ad-hoc UniqueNameManager is built and discarded after use. The property management classes have a bit of a mess of methods including several to populate various collection types with all existing properties. Rather than introducing yet another such collection-specific method to fill a UniqueNameManager, a visitProperties method was added which calls a passed function for each property. The existing code (e.g. getPropertyMap) would be simpler if they all used this but the cost of calling a lambda for each property must be considered. It would clarify the semantics of these methods, which have a bit of variance in which properties populate the passed collection, e.g. when there are duplicate names.. Ideally the PropertyContainer class would keep a central directory of all properties ("static", Dynamic, and exposed by ExtensionContainer and other derivations) and a permanent UniqueNameManager. However the Property management is a bit of a mess making such a change a project unto itself. --- src/App/Application.cpp | 183 +++++++++++++++--------------- src/App/Application.h | 33 +++--- src/App/Document.cpp | 143 ++++++++++++++--------- src/App/Document.h | 30 +++-- src/App/DocumentObject.cpp | 73 ++++++++++++ src/App/DocumentObject.h | 22 +++- src/App/DynamicProperty.cpp | 48 ++++---- src/App/DynamicProperty.h | 13 ++- src/App/Extension.cpp | 7 ++ src/App/Extension.h | 17 ++- src/App/ExtensionContainer.cpp | 12 ++ src/App/ExtensionContainer.h | 12 +- src/App/PropertyContainer.cpp | 21 +++- src/App/PropertyContainer.h | 23 +++- src/App/PropertyStandard.cpp | 108 +++--------------- src/App/private/DocumentP.h | 16 ++- src/Base/Reader.cpp | 10 +- src/Base/Reader.h | 12 +- src/Base/Tools.cpp | 126 +------------------- src/Base/Tools.h | 9 +- src/Base/UniqueNameManager.cpp | 4 +- src/Base/UniqueNameManager.h | 2 +- src/Base/Writer.cpp | 53 ++------- src/Base/Writer.h | 31 +++-- src/Gui/Document.cpp | 11 +- src/Gui/ViewProviderLink.cpp | 26 ++++- src/Gui/ViewProviderLink.h | 15 ++- src/Mod/Spreadsheet/App/Sheet.cpp | 19 +++- src/Mod/Spreadsheet/App/Sheet.h | 13 ++- tests/src/Base/Tools.cpp | 40 +------ 30 files changed, 581 insertions(+), 551 deletions(-) diff --git a/src/App/Application.cpp b/src/App/Application.cpp index 573d0331ae..05402889d2 100644 --- a/src/App/Application.cpp +++ b/src/App/Application.cpp @@ -38,6 +38,15 @@ # include # include # include +# include +# include +# include +# include +# include +# include +# include +# include +# include # include #endif @@ -80,6 +89,7 @@ #include #include #include +#include #include #include #include @@ -454,101 +464,88 @@ void Application::renameDocument(const char *OldName, const char *NewName) throw Base::RuntimeError("Renaming document internal name is no longer allowed!"); } -Document* Application::newDocument(const char * Name, const char * UserName, DocumentCreateFlags CreateFlags) +Document* Application::newDocument(const char * proposedName, const char * proposedLabel, DocumentCreateFlags CreateFlags) { - auto getNameAndLabel = [this](const char * Name, const char * UserName) -> std::tuple { - bool isDefaultName = Tools::isNullOrEmpty(Name); - - // get a valid name anyway! - if (isDefaultName) { - Name = "Unnamed"; - } - - std::string userName; - if (!Base::Tools::isNullOrEmpty(UserName)) { - userName = UserName; - } - else { - userName = isDefaultName ? QObject::tr("Unnamed").toStdString() : Name; - - std::vector names; - names.reserve(DocMap.size()); - for (const auto& pos : DocMap) { - names.emplace_back(pos.second->Label.getValue()); - } - - if (!names.empty()) { - userName = Base::Tools::getUniqueName(userName, names); - } - } - - return std::make_tuple(std::string(Name), userName); - }; - - auto tuple = getNameAndLabel(Name, UserName); - std::string name = std::get<0>(tuple); - std::string userName = std::get<1>(tuple); - name = getUniqueDocumentName(name.c_str(), CreateFlags.temporary); + bool isUsingDefaultName = Tools::isNullOrEmpty(proposedName); + // get a valid name anyway! + if (isUsingDefaultName) { + proposedName = "Unnamed"; + } + std::string name(getUniqueDocumentName(proposedName, CreateFlags.temporary)); // return the temporary document if it exists if (CreateFlags.temporary) { auto it = DocMap.find(name); - if (it != DocMap.end() && it->second->testStatus(Document::TempDoc)) + if (it != DocMap.end() && it->second->testStatus(Document::TempDoc)) { return it->second; + } } - // create the FreeCAD document - std::unique_ptr newDoc(new Document(name.c_str())); - newDoc->setStatus(Document::TempDoc, CreateFlags.temporary); + // Determine the document's Label + std::string label; + if (!Tools::isNullOrEmpty(proposedLabel)) { + // If a label is supplied it is used even if not unique + label = proposedLabel; + } + else { + label = isUsingDefaultName ? QObject::tr("Unnamed").toStdString() : proposedName; - auto oldActiveDoc = _pActiveDoc; - auto doc = newDoc.release(); // now owned by the Application + if (!DocMap.empty()) { + // The assumption here is that there are not many documents and + // documents are rarely created so the cost + // of building this manager each time is inconsequential + Base::UniqueNameManager names; + for (const auto& pos : DocMap) { + names.addExactName(pos.second->Label.getValue()); + } + + label = names.makeUniqueName(label); + } + } + // create the FreeCAD document + auto doc = new Document(name.c_str()); + doc->setStatus(Document::TempDoc, CreateFlags.temporary); // add the document to the internal list DocMap[name] = doc; - _pActiveDoc = doc; //NOLINTBEGIN // clang-format off // connect the signals to the application for the new document - _pActiveDoc->signalBeforeChange.connect(std::bind(&App::Application::slotBeforeChangeDocument, this, sp::_1, sp::_2)); - _pActiveDoc->signalChanged.connect(std::bind(&App::Application::slotChangedDocument, this, sp::_1, sp::_2)); - _pActiveDoc->signalNewObject.connect(std::bind(&App::Application::slotNewObject, this, sp::_1)); - _pActiveDoc->signalDeletedObject.connect(std::bind(&App::Application::slotDeletedObject, this, sp::_1)); - _pActiveDoc->signalBeforeChangeObject.connect(std::bind(&App::Application::slotBeforeChangeObject, this, sp::_1, sp::_2)); - _pActiveDoc->signalChangedObject.connect(std::bind(&App::Application::slotChangedObject, this, sp::_1, sp::_2)); - _pActiveDoc->signalRelabelObject.connect(std::bind(&App::Application::slotRelabelObject, this, sp::_1)); - _pActiveDoc->signalActivatedObject.connect(std::bind(&App::Application::slotActivatedObject, this, sp::_1)); - _pActiveDoc->signalUndo.connect(std::bind(&App::Application::slotUndoDocument, this, sp::_1)); - _pActiveDoc->signalRedo.connect(std::bind(&App::Application::slotRedoDocument, this, sp::_1)); - _pActiveDoc->signalRecomputedObject.connect(std::bind(&App::Application::slotRecomputedObject, this, sp::_1)); - _pActiveDoc->signalRecomputed.connect(std::bind(&App::Application::slotRecomputed, this, sp::_1)); - _pActiveDoc->signalBeforeRecompute.connect(std::bind(&App::Application::slotBeforeRecompute, this, sp::_1)); - _pActiveDoc->signalOpenTransaction.connect(std::bind(&App::Application::slotOpenTransaction, this, sp::_1, sp::_2)); - _pActiveDoc->signalCommitTransaction.connect(std::bind(&App::Application::slotCommitTransaction, this, sp::_1)); - _pActiveDoc->signalAbortTransaction.connect(std::bind(&App::Application::slotAbortTransaction, this, sp::_1)); - _pActiveDoc->signalStartSave.connect(std::bind(&App::Application::slotStartSaveDocument, this, sp::_1, sp::_2)); - _pActiveDoc->signalFinishSave.connect(std::bind(&App::Application::slotFinishSaveDocument, this, sp::_1, sp::_2)); - _pActiveDoc->signalChangePropertyEditor.connect(std::bind(&App::Application::slotChangePropertyEditor, this, sp::_1, sp::_2)); + doc->signalBeforeChange.connect(std::bind(&App::Application::slotBeforeChangeDocument, this, sp::_1, sp::_2)); + doc->signalChanged.connect(std::bind(&App::Application::slotChangedDocument, this, sp::_1, sp::_2)); + doc->signalNewObject.connect(std::bind(&App::Application::slotNewObject, this, sp::_1)); + doc->signalDeletedObject.connect(std::bind(&App::Application::slotDeletedObject, this, sp::_1)); + doc->signalBeforeChangeObject.connect(std::bind(&App::Application::slotBeforeChangeObject, this, sp::_1, sp::_2)); + doc->signalChangedObject.connect(std::bind(&App::Application::slotChangedObject, this, sp::_1, sp::_2)); + doc->signalRelabelObject.connect(std::bind(&App::Application::slotRelabelObject, this, sp::_1)); + doc->signalActivatedObject.connect(std::bind(&App::Application::slotActivatedObject, this, sp::_1)); + doc->signalUndo.connect(std::bind(&App::Application::slotUndoDocument, this, sp::_1)); + doc->signalRedo.connect(std::bind(&App::Application::slotRedoDocument, this, sp::_1)); + doc->signalRecomputedObject.connect(std::bind(&App::Application::slotRecomputedObject, this, sp::_1)); + doc->signalRecomputed.connect(std::bind(&App::Application::slotRecomputed, this, sp::_1)); + doc->signalBeforeRecompute.connect(std::bind(&App::Application::slotBeforeRecompute, this, sp::_1)); + doc->signalOpenTransaction.connect(std::bind(&App::Application::slotOpenTransaction, this, sp::_1, sp::_2)); + doc->signalCommitTransaction.connect(std::bind(&App::Application::slotCommitTransaction, this, sp::_1)); + doc->signalAbortTransaction.connect(std::bind(&App::Application::slotAbortTransaction, this, sp::_1)); + doc->signalStartSave.connect(std::bind(&App::Application::slotStartSaveDocument, this, sp::_1, sp::_2)); + doc->signalFinishSave.connect(std::bind(&App::Application::slotFinishSaveDocument, this, sp::_1, sp::_2)); + doc->signalChangePropertyEditor.connect(std::bind(&App::Application::slotChangePropertyEditor, this, sp::_1, sp::_2)); // clang-format on //NOLINTEND - // make sure that the active document is set in case no GUI is up - { - Base::PyGILStateLocker lock; - Py::Object active(_pActiveDoc->getPyObject(), true); - Py::Module("FreeCAD").setAttr(std::string("ActiveDocument"), active); - } + // (temporarily) make this the active document for the upcoming notifications. + // Signal NewDocument rather than ActiveDocument (which is what setActiveDocument would do) + auto oldActiveDoc = _pActiveDoc; + setActiveDocumentNoSignal(doc); + signalNewDocument(*doc, CreateFlags.createView); - signalNewDocument(*_pActiveDoc, CreateFlags.createView); - - // set the UserName after notifying all observers - _pActiveDoc->Label.setValue(userName); + doc->Label.setValue(label); // set the old document active again if the new is temporary - if (CreateFlags.temporary && oldActiveDoc) + if (CreateFlags.temporary && oldActiveDoc) { setActiveDocument(oldActiveDoc); - + } return doc; } @@ -618,29 +615,31 @@ std::vector Application::getDocuments() const return docs; } -std::string Application::getUniqueDocumentName(const char *Name, bool tempDoc) const +std::string Application::getUniqueDocumentName(const char* Name, bool tempDoc) const { - if (!Name || *Name == '\0') + if (!Name || *Name == '\0') { return {}; + } std::string CleanName = Base::Tools::getIdentifier(Name); // name in use? - std::map::const_iterator pos; - pos = DocMap.find(CleanName); + auto pos = DocMap.find(CleanName); if (pos == DocMap.end() || (tempDoc && pos->second->testStatus(Document::TempDoc))) { // if not, name is OK return CleanName; } - else { - std::vector names; - names.reserve(DocMap.size()); - for (pos = DocMap.begin(); pos != DocMap.end(); ++pos) { - if (!tempDoc || !pos->second->testStatus(Document::TempDoc)) - names.push_back(pos->first); + // The assumption here is that there are not many documents and + // documents are rarely created so the cost + // of building this manager each time is inconsequential + Base::UniqueNameManager names; + for (const auto& pos : DocMap) { + if (!tempDoc || !pos.second->testStatus(Document::TempDoc)) { + names.addExactName(pos.first); } - return Base::Tools::getUniqueName(CleanName, names); } + + return names.makeUniqueName(CleanName); } int Application::addPendingDocument(const char *FileName, const char *objName, bool allowPartial) @@ -1057,6 +1056,15 @@ Document* Application::getActiveDocument() const } void Application::setActiveDocument(Document* pDoc) +{ + setActiveDocumentNoSignal(pDoc); + + if (pDoc) { + signalActiveDocument(*pDoc); + } +} + +void Application::setActiveDocumentNoSignal(Document* pDoc) { _pActiveDoc = pDoc; @@ -1064,18 +1072,15 @@ void Application::setActiveDocument(Document* pDoc) if (pDoc) { Base::PyGILStateLocker lock; Py::Object active(pDoc->getPyObject(), true); - Py::Module("FreeCAD").setAttr(std::string("ActiveDocument"),active); + Py::Module("FreeCAD").setAttr(std::string("ActiveDocument"), active); } else { Base::PyGILStateLocker lock; - Py::Module("FreeCAD").setAttr(std::string("ActiveDocument"),Py::None()); + Py::Module("FreeCAD").setAttr(std::string("ActiveDocument"), Py::None()); } - - if (pDoc) - signalActiveDocument(*pDoc); } -void Application::setActiveDocument(const char *Name) +void Application::setActiveDocument(const char* Name) { // If no active document is set, resort to a default. if (*Name == '\0') { diff --git a/src/App/Application.h b/src/App/Application.h index 00c9a0c735..a91f2e105a 100644 --- a/src/App/Application.h +++ b/src/App/Application.h @@ -21,13 +21,17 @@ * * ***************************************************************************/ -#ifndef APP_APPLICATION_H -#define APP_APPLICATION_H +#ifndef SRC_APP_APPLICATION_H_ +#define SRC_APP_APPLICATION_H_ #include #include #include +#include +#include +#include +#include #include #include @@ -89,15 +93,15 @@ public: /** @name methods for document handling */ //@{ /** Creates a new document - * The first name is a the identifier and some kind of an internal (english) - * name. It has to be like an identifier in a programming language, with no - * spaces and not starting with a number. This name gets also forced to be unique - * in this Application. You can avoid the renaming by using getUniqueDocumentName() - * to get a unique name before calling newDoucument(). - * The second name is a UTF8 name of any kind. It's that name normally shown to - * the user and stored in the App::Document::Name property. + * @param proposedName: a prototype name used to create the permanent Name for the document. + * It is converted to be like an identifier in a programming language, + * with no spaces and not starting with a number. This name gets also forced to be unique + * in this Application. You can obtain the unique name using doc.getDocumentName + * on the returned document. + * @param proposedLabel: a UTF8 name of any kind. It's that name normally shown to + * the user and stored in the App::Document::Label property. */ - App::Document* newDocument(const char * Name=nullptr, const char * UserName=nullptr, + App::Document* newDocument(const char * proposedName=nullptr, const char * proposedLabel=nullptr, DocumentCreateFlags CreateFlags=DocumentCreateFlags()); /// Closes the document \a name and removes it from the application. bool closeDocument(const char* name); @@ -164,7 +168,7 @@ public: std::vector getDocuments() const; /// Set the active document void setActiveDocument(App::Document* pDoc); - void setActiveDocument(const char *Name); + void setActiveDocument(const char* Name); /// close all documents (without saving) void closeAllDocuments(); /// Add pending document to open together with the current opening document @@ -503,13 +507,16 @@ protected: }; private: - /// Constructor + /// Constructor. The passed configuration must last for the lifetime of the constructed Application + // NOLINTNEXTLINE(runtime/references) explicit Application(std::map &mConfig); /// Destructor virtual ~Application(); static void cleanupUnits(); + void setActiveDocumentNoSignal(App::Document* pDoc); + /** @name member for parameter */ //@{ static Base::Reference _pcSysParamMngr; @@ -654,4 +661,4 @@ inline App::Application &GetApplication(){ } // namespace App -#endif // APP_APPLICATION_H +#endif // SRC_APP_APPLICATION_H_ diff --git a/src/App/Document.cpp b/src/App/Document.cpp index 384e1b899a..f0c5d0c0c6 100644 --- a/src/App/Document.cpp +++ b/src/App/Document.cpp @@ -60,6 +60,17 @@ recompute path. Also, it enables more complicated dependencies beyond trees. #ifndef _PreComp_ #include #include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include #include #endif @@ -645,8 +656,10 @@ void Document::clearDocument() setStatus(Document::PartialDoc, false); d->clearRecomputeLog(); + d->objectLabelManager.clear(); d->objectArray.clear(); d->objectMap.clear(); + d->objectNameManager.clear(); d->objectIdMap.clear(); d->lastObjectId = 0; } @@ -2250,6 +2263,34 @@ bool Document::saveToFile(const char* filename) const return true; } +void Document::registerLabel(const std::string& newLabel) +{ + if (!newLabel.empty()) { + d->objectLabelManager.addExactName(newLabel); + } +} + +void Document::unregisterLabel(const std::string& oldLabel) +{ + if (!oldLabel.empty()) { + d->objectLabelManager.removeExactName(oldLabel); + } +} + +bool Document::containsLabel(const std::string& label) +{ + return d->objectLabelManager.containsName(label); +} + +std::string Document::makeUniqueLabel(const std::string& modelLabel) +{ + if (modelLabel.empty()) { + return {}; + } + + return d->objectLabelManager.makeUniqueName(modelLabel, 3); +} + bool Document::isAnyRestoring() { return globalIsRestoring; @@ -2276,7 +2317,9 @@ void Document::restore(const char* filename, setStatus(Document::PartialDoc, false); d->clearRecomputeLog(); + d->objectLabelManager.clear(); d->objectArray.clear(); + d->objectNameManager.clear(); d->objectMap.clear(); d->objectIdMap.clear(); d->lastObjectId = 0; @@ -3577,6 +3620,7 @@ DocumentObject* Document::addObject(const char* sType, // insert in the name map d->objectMap[ObjectName] = pcObject; + d->objectNameManager.addExactName(ObjectName); // generate object id and add to id map; pcObject->_Id = ++d->lastObjectId; d->objectIdMap[pcObject->_Id] = pcObject; @@ -3585,6 +3629,8 @@ DocumentObject* Document::addObject(const char* sType, pcObject->pcNameInDocument = &(d->objectMap.find(ObjectName)->first); // insert in the vector d->objectArray.push_back(pcObject); + // Register the current Label even though it is (probably) about to change + registerLabel(pcObject->Label.getStrValue()); // If we are restoring, don't set the Label object now; it will be restored later. This is to // avoid potential duplicate label conflicts later. @@ -3645,13 +3691,6 @@ Document::addObjects(const char* sType, const std::vector& objectNa return objects; } - // get all existing object names - std::vector reservedNames; - reservedNames.reserve(d->objectMap.size()); - for (const auto& pos : d->objectMap) { - reservedNames.push_back(pos.first); - } - for (auto it = objects.begin(); it != objects.end(); ++it) { auto index = std::distance(objects.begin(), it); App::DocumentObject* pcObject = *it; @@ -3666,29 +3705,19 @@ Document::addObjects(const char* sType, const std::vector& objectNa } } - // get unique name + // get unique name. We don't use getUniqueObjectName because it takes a char* not a std::string std::string ObjectName = objectNames[index]; if (ObjectName.empty()) { ObjectName = sType; } ObjectName = Base::Tools::getIdentifier(ObjectName); - if (d->objectMap.find(ObjectName) != d->objectMap.end()) { - // remove also trailing digits from clean name which is to avoid to create lengthy names - // like 'Box001001' - if (!testStatus(KeepTrailingDigits)) { - std::string::size_type index = ObjectName.find_last_not_of("0123456789"); - if (index + 1 < ObjectName.size()) { - ObjectName = ObjectName.substr(0, index + 1); - } - } - - ObjectName = Base::Tools::getUniqueName(ObjectName, reservedNames, 3); + if (d->objectNameManager.containsName(ObjectName)) { + ObjectName = d->objectNameManager.makeUniqueName(ObjectName, 3); } - reservedNames.push_back(ObjectName); - // insert in the name map d->objectMap[ObjectName] = pcObject; + d->objectNameManager.addExactName(ObjectName); // generate object id and add to id map; pcObject->_Id = ++d->lastObjectId; d->objectIdMap[pcObject->_Id] = pcObject; @@ -3697,6 +3726,8 @@ Document::addObjects(const char* sType, const std::vector& objectNa pcObject->pcNameInDocument = &(d->objectMap.find(ObjectName)->first); // insert in the vector d->objectArray.push_back(pcObject); + // Register the current Label even though it is about to change + registerLabel(pcObject->Label.getStrValue()); pcObject->Label.setValue(ObjectName); @@ -3757,6 +3788,7 @@ void Document::addObject(DocumentObject* pcObject, const char* pObjectName) // insert in the name map d->objectMap[ObjectName] = pcObject; + d->objectNameManager.addExactName(ObjectName); // generate object id and add to id map; if (!pcObject->_Id) { pcObject->_Id = ++d->lastObjectId; @@ -3767,6 +3799,8 @@ void Document::addObject(DocumentObject* pcObject, const char* pObjectName) pcObject->pcNameInDocument = &(d->objectMap.find(ObjectName)->first); // insert in the vector d->objectArray.push_back(pcObject); + // Register the current Label even though it is about to change + registerLabel(pcObject->Label.getStrValue()); pcObject->Label.setValue(ObjectName); @@ -3790,12 +3824,14 @@ void Document::_addObject(DocumentObject* pcObject, const char* pObjectName) { std::string ObjectName = getUniqueObjectName(pObjectName); d->objectMap[ObjectName] = pcObject; + d->objectNameManager.addExactName(ObjectName); // generate object id and add to id map; if (!pcObject->_Id) { pcObject->_Id = ++d->lastObjectId; } d->objectIdMap[pcObject->_Id] = pcObject; d->objectArray.push_back(pcObject); + registerLabel(pcObject->Label.getStrValue()); // cache the pointer to the name string in the Object (for performance of // DocumentObject::getNameInDocument()) pcObject->pcNameInDocument = &(d->objectMap.find(ObjectName)->first); @@ -3824,6 +3860,15 @@ void Document::_addObject(DocumentObject* pcObject, const char* pObjectName) signalActivatedObject(*pcObject); } +bool Document::containsObject(const DocumentObject* pcObject) const +{ + // We could look for the object in objectMap (keyed by object name), + // or search in objectArray (a O(n) vector search) but looking by Id + // in objectIdMap would be fastest. + auto found = d->objectIdMap.find(pcObject->getID()); + return found != d->objectIdMap.end() && found->second == pcObject; +} + /// Remove an object out of the document void Document::removeObject(const char* sName) { @@ -3911,6 +3956,7 @@ void Document::removeObject(const char* sName) } } + unregisterLabel(pos->second->Label.getStrValue()); for (std::vector::iterator obj = d->objectArray.begin(); obj != d->objectArray.end(); ++obj) { @@ -3924,6 +3970,7 @@ void Document::removeObject(const char* sName) if (tobedestroyed) { tobedestroyed->pcNameInDocument = nullptr; } + d->objectNameManager.removeExactName(pos->first); d->objectMap.erase(pos); } @@ -3993,6 +4040,8 @@ void Document::_removeObject(DocumentObject* pcObject) // remove from map pcObject->setStatus(ObjectStatus::Remove, false); // Unset the bit to be on the safe side d->objectIdMap.erase(pcObject->_Id); + d->objectNameManager.removeExactName(pos->first); + unregisterLabel(pos->second->Label.getStrValue()); d->objectMap.erase(pos); for (std::vector::iterator it = d->objectArray.begin(); @@ -4269,50 +4318,32 @@ const char* Document::getObjectName(DocumentObject* pFeat) const return nullptr; } -std::string Document::getUniqueObjectName(const char* Name) const +std::string Document::getUniqueObjectName(const char* proposedName) const { - if (!Name || *Name == '\0') { + if (!proposedName || *proposedName == '\0') { return {}; } - std::string CleanName = Base::Tools::getIdentifier(Name); + std::string cleanName = Base::Tools::getIdentifier(proposedName); - // name in use? - auto pos = d->objectMap.find(CleanName); - - if (pos == d->objectMap.end()) { - // if not, name is OK - return CleanName; - } - else { - // remove also trailing digits from clean name which is to avoid to create lengthy names - // like 'Box001001' - if (!testStatus(KeepTrailingDigits)) { - std::string::size_type index = CleanName.find_last_not_of("0123456789"); - if (index + 1 < CleanName.size()) { - CleanName = CleanName.substr(0, index + 1); - } - } - - std::vector names; - names.reserve(d->objectMap.size()); - for (pos = d->objectMap.begin(); pos != d->objectMap.end(); ++pos) { - names.push_back(pos->first); - } - return Base::Tools::getUniqueName(CleanName, names, 3); + if (!d->objectNameManager.containsName(cleanName)) { + // Not in use yet, name is OK + return cleanName; } + return d->objectNameManager.makeUniqueName(cleanName, 3); } -std::string Document::getStandardObjectName(const char* Name, int d) const + bool +Document::haveSameBaseName(const std::string& name, const std::string& label) { - std::vector mm = getObjects(); - std::vector labels; - labels.reserve(mm.size()); + // Both Labels and Names use the same decomposition rules for names, + // i.e. the default one supplied by UniqueNameManager, so we can use either + // of the name managers to do this test. + return d->objectNameManager.haveSameBaseName(name, label); +} - for (auto it : mm) { - std::string label = it->Label.getValue(); - labels.push_back(label); - } - return Base::Tools::getUniqueName(Name, labels, d); +std::string Document::getStandardObjectLabel(const char* modelName, int digitCount) const +{ + return d->objectLabelManager.makeUniqueName(modelName, digitCount); } std::vector Document::getDependingObjects() const diff --git a/src/App/Document.h b/src/App/Document.h index cdabf8efce..369e29377c 100644 --- a/src/App/Document.h +++ b/src/App/Document.h @@ -20,8 +20,8 @@ * * ***************************************************************************/ -#ifndef APP_DOCUMENT_H -#define APP_DOCUMENT_H +#ifndef SRC_APP_DOCUMENT_H_ +#define SRC_APP_DOCUMENT_H_ #include #include @@ -35,6 +35,9 @@ #include #include +#include +#include +#include #include namespace Base @@ -303,6 +306,10 @@ public: */ void addObject(DocumentObject*, const char* pObjectName = nullptr); + /// returns whether this is actually contains the DocumentObject. + /// Testing the DocumentObject's pDoc pointer is not sufficient because the object + /// removeObject and _removeObject leave _pDoc unchanged + bool containsObject(const DocumentObject*) const; /** Copy objects from another document to this document * @@ -334,11 +341,13 @@ public: /// Returns true if the DocumentObject is contained in this document bool isIn(const DocumentObject* pFeat) const; /// Returns a Name of an Object or 0 - const char* getObjectName(DocumentObject* pFeat) const; - /// Returns a Name of an Object or 0 - std::string getUniqueObjectName(const char* Name) const; - /// Returns a name of the form prefix_number. d specifies the number of digits. - std::string getStandardObjectName(const char* Name, int d) const; + const char *getObjectName(DocumentObject* pFeat) const; + /// Returns a Name for a new Object or empty if proposedName is null or empty. + std::string getUniqueObjectName(const char* proposedName) const; + /// Returns a name different from any of the Labels of any objects in this document, based on the given modelName. + std::string getStandardObjectLabel(const char* modelName, int d) const; + /// Determine if a given DocumentObject Name and a proposed Label are based on the same base name + bool haveSameBaseName(const std::string& name, const std::string& label); /// Returns a list of document's objects including the dependencies std::vector getDependingObjects() const; /// Returns a list of all Objects @@ -585,6 +594,11 @@ public: /// Indicate if there is any document restoring/importing static bool isAnyRestoring(); + void registerLabel(const std ::string& newLabel); + void unregisterLabel(const std::string& oldLabel); + bool containsLabel(const std::string& label); + std::string makeUniqueLabel(const std::string& modelLabel); + friend class Application; /// because of transaction handling friend class TransactionalObject; @@ -686,4 +700,4 @@ T* Document::addObject(const char* pObjectName, bool isNew, const char* viewType } // namespace App -#endif // APP_DOCUMENT_H +#endif // SRC_APP_DOCUMENT_H_ diff --git a/src/App/DocumentObject.cpp b/src/App/DocumentObject.cpp index 17f2b83ce8..ef4fdde3a9 100644 --- a/src/App/DocumentObject.cpp +++ b/src/App/DocumentObject.cpp @@ -25,6 +25,11 @@ #include "PreCompiled.h" #ifndef _PreComp_ #include +#include +#include +#include +#include +#include #endif #include @@ -814,6 +819,69 @@ void DocumentObject::onBeforeChange(const Property* prop) signalBeforeChange(*this, *prop); } +std::vector>> +DocumentObject::onProposedLabelChange(std::string& newLabel) +{ + // Note that this work can't be done in onBeforeChangeLabel because FeaturePython overrides this + // method and does not initially base-call it. + + // We re only called if the new label differs from the old one, and our code to check duplicates + // may not work if this is not the case. + std::string oldLabel = Label.getStrValue(); + assert(newLabel != oldLabel); + if (!isAttachedToDocument()) { + return {}; + } + App::Document* doc = getDocument(); + if (doc->isPerformingTransaction() + || (doc->testStatus(App::Document::Restoring) + && !doc->testStatus(App::Document::Importing))) { + return {}; + } + static ParameterGrp::handle _hPGrp; + if (!_hPGrp) { + _hPGrp = App::GetApplication().GetParameterGroupByPath("User parameter:BaseApp/Preferences/Document"); + } + if (doc && !newLabel.empty() && !_hPGrp->GetBool("DuplicateLabels") && !allowDuplicateLabel() + && doc->containsLabel(newLabel)) { + // We must ensure the Label is unique in the document (well, sort of...). + std::string objName = getNameInDocument(); + if (doc->haveSameBaseName(objName, newLabel)) { + // The base name of the proposed label equals the base name of the object Name, so we + // use the object Name, which could actually be identical to another object's Label, but + // probably isn't. + newLabel = objName; + } + else { + // Otherwise we generate a unique Label using newLabel as a prototype name. In doing so, + // we must also act as if the current value of the property is not an existing Label + // entry. + // We deregister the old label so it does not interfere with making the new label, + // and re-register it after. This is probably a bit less efficient that having a special + // make-unique-label-as-if-this-one-did-not-exist method, but such a method would be a real + // ugly wart. + doc->unregisterLabel(oldLabel); + newLabel = doc->makeUniqueLabel(newLabel); + doc->registerLabel(oldLabel); + } + } + + // Despite our efforts to make a unique label, onBeforeLabelChange can change it. + onBeforeChangeLabel(newLabel); + + if (oldLabel == newLabel || getDocument()->testStatus(App::Document::Restoring)) { + // Don't update label reference if we are restoring or if the label is unchanged. + // When importing (which also counts as restoring), it is possible the + // new object changes its label. However, we cannot update label + // references here, because object being restored is not based on + // dependency order. It can only be done in afterRestore(). + // + // See PropertyLinkBase::restoreLabelReference() for more details. + return {}; + } + return PropertyLinkBase::updateLabelReferences(this, newLabel.c_str()); +} + void DocumentObject::onEarlyChange(const Property* prop) { if (GetApplication().isClosingAll()) { @@ -836,6 +904,11 @@ void DocumentObject::onEarlyChange(const Property* prop) /// get called by the container when a Property was changed void DocumentObject::onChanged(const Property* prop) { + if (prop == &Label && _pDoc && _pDoc->containsObject(this) && oldLabel != Label.getStrValue()) { + _pDoc->unregisterLabel(oldLabel); + _pDoc->registerLabel(Label.getStrValue()); + } + if (isFreezed() && prop != &Visibility) { return; } diff --git a/src/App/DocumentObject.h b/src/App/DocumentObject.h index 72f78fe13d..c26a085731 100644 --- a/src/App/DocumentObject.h +++ b/src/App/DocumentObject.h @@ -22,8 +22,8 @@ ***************************************************************************/ -#ifndef APP_DOCUMENTOBJECT_H -#define APP_DOCUMENTOBJECT_H +#ifndef SRC_APP_DOCUMENTOBJECT_H_ +#define SRC_APP_DOCUMENTOBJECT_H_ #include #include @@ -33,6 +33,11 @@ #include #include +#include +#include +#include +#include +#include namespace Base { @@ -513,7 +518,14 @@ public: { return false; } - + /// Handle Label changes, including forcing unique label values, + /// signalling OnBeforeLabelChange, and arranging to update linked references, + /// on the assumption that after returning the label will indeed be changed to + /// the (altered) value of newLabel. + /// Returns a vector of referenging (linking) properties as produced by + /// PropertyLinkBase::updateLabelReferences which is needed for undo/redo purposes. + std::vector>> + onProposedLabelChange(std::string& newLabel); /*** Called to let object itself control relabeling * * @param newLabel: input as the new label, which can be modified by object itself @@ -765,10 +777,10 @@ protected: // attributes /// Old label; used for renaming expressions std::string oldLabel; +private: // pointer to the document name string (for performance) const std::string* pcNameInDocument {nullptr}; -private: // accessed by App::Document to record and restore the correct view provider type std::string _pcViewProviderName; @@ -787,4 +799,4 @@ private: } // namespace App -#endif // APP_DOCUMENTOBJECT_H +#endif // SRC_APP_DOCUMENTOBJECT_H_ diff --git a/src/App/DynamicProperty.cpp b/src/App/DynamicProperty.cpp index f79ecd5650..07921e87f4 100644 --- a/src/App/DynamicProperty.cpp +++ b/src/App/DynamicProperty.cpp @@ -22,9 +22,15 @@ #include "PreCompiled.h" +#ifndef _PreComp_ +#include +#include +#include +#endif #include #include +#include #include #include "DynamicProperty.h" @@ -70,7 +76,13 @@ void DynamicProperty::getPropertyNamedList( } } -void DynamicProperty::getPropertyMap(std::map& Map) const +void DynamicProperty::visitProperties(const std::function& visitor) const { + for (auto& v : props.get<0>()) { + visitor(v.property); + } +} + +void DynamicProperty::getPropertyMap(std::map& Map) const { for (auto& v : props.get<0>()) { Map[v.name] = v.property; @@ -296,27 +308,23 @@ bool DynamicProperty::removeDynamicProperty(const char* name) return false; } -std::string DynamicProperty::getUniquePropertyName(PropertyContainer& pc, const char* Name) const +std::string DynamicProperty::getUniquePropertyName(const PropertyContainer& pc, const char* Name) const { - std::string CleanName = Base::Tools::getIdentifier(Name); + std::string cleanName = Base::Tools::getIdentifier(Name); - // name in use? - std::map objectProps; - pc.getPropertyMap(objectProps); - auto pos = objectProps.find(CleanName); - - if (pos == objectProps.end()) { - // if not, name is OK - return CleanName; - } - else { - std::vector names; - names.reserve(objectProps.size()); - for (pos = objectProps.begin(); pos != objectProps.end(); ++pos) { - names.push_back(pos->first); - } - return Base::Tools::getUniqueName(CleanName, names); + // We test if the property already exists by finding it, which is not much more expensive than + // having a separate propertyExists(name) method. This avoids building the UniqueNameManager + // (which could also tell if the name exists) except in the relatively rare condition of + // the name already existing. + if (pc.getPropertyByName(cleanName.c_str()) == nullptr) { + return cleanName; } + Base::UniqueNameManager names; + // Build the index of existing names. + pc.visitProperties([&](Property* prop) { + names.addExactName(prop->getName()); + }); + return names.makeUniqueName(cleanName); } void DynamicProperty::save(const Property* prop, Base::Writer& writer) const @@ -335,7 +343,7 @@ void DynamicProperty::save(const Property* prop, Base::Writer& writer) const Property* DynamicProperty::restore(PropertyContainer& pc, const char* PropName, const char* TypeName, - Base::XMLReader& reader) + const Base::XMLReader& reader) { if (!reader.hasAttribute("group")) { return nullptr; diff --git a/src/App/DynamicProperty.h b/src/App/DynamicProperty.h index 29936fa665..1a0c0a42f9 100644 --- a/src/App/DynamicProperty.h +++ b/src/App/DynamicProperty.h @@ -21,12 +21,13 @@ ***************************************************************************/ -#ifndef APP_DYNAMICPROPERTY_H -#define APP_DYNAMICPROPERTY_H +#ifndef SRC_APP_DYNAMICPROPERTY_H_ +#define SRC_APP_DYNAMICPROPERTY_H_ #include #include #include +#include #include #include @@ -87,6 +88,8 @@ public: void getPropertyList(std::vector& List) const; /// get all properties with their names void getPropertyNamedList(std::vector>& List) const; + /// See PropertyContainer::visitProperties for semantics + void visitProperties(const std::function& visitor) const; /// Get all properties of the class (including parent) void getPropertyMap(std::map& Map) const; /// Find a dynamic property by its name @@ -166,7 +169,7 @@ public: Property* restore(PropertyContainer& pc, const char* PropName, const char* TypeName, - Base::XMLReader& reader); + const Base::XMLReader& reader); struct PropData { @@ -208,7 +211,7 @@ public: bool changeDynamicProperty(const Property* prop, const char* group, const char* doc); private: - std::string getUniquePropertyName(PropertyContainer& pc, const char* Name) const; + std::string getUniquePropertyName(const PropertyContainer& pc, const char* Name) const; private: bmi::multi_index_container< @@ -223,4 +226,4 @@ private: } // namespace App -#endif // APP_DYNAMICPROPERTY_H +#endif // SRC_APP_DYNAMICPROPERTY_H_ diff --git a/src/App/Extension.cpp b/src/App/Extension.cpp index c84e613de4..5ec6242da7 100644 --- a/src/App/Extension.cpp +++ b/src/App/Extension.cpp @@ -25,6 +25,9 @@ #ifndef _PreComp_ #include +#include +#include +#include #endif #include @@ -181,6 +184,10 @@ void Extension::extensionGetPropertyMap(std::map& Map) c extensionGetPropertyData().getPropertyMap(this, Map); } +void Extension::extensionVisitProperties(const std::function& visitor) const +{ + extensionGetPropertyData().visitProperties(this, visitor); +} void Extension::initExtensionSubclass(Base::Type& toInit, const char* ClassName, const char* ParentName, diff --git a/src/App/Extension.h b/src/App/Extension.h index 7c1545435e..8ee6b31098 100644 --- a/src/App/Extension.h +++ b/src/App/Extension.h @@ -21,8 +21,11 @@ ***************************************************************************/ -#ifndef APP_EXTENSION_H -#define APP_EXTENSION_H +#ifndef SRC_APP_EXTENSION_H_ +#define SRC_APP_EXTENSION_H_ + +#include +#include #include "PropertyContainer.h" #include @@ -46,7 +49,7 @@ private: \ #define EXTENSION_TYPESYSTEM_HEADER_WITH_OVERRIDE() \ public: \ static Base::Type getExtensionClassTypeId(void); \ - virtual Base::Type getExtensionTypeId(void) const override; \ + Base::Type getExtensionTypeId(void) const override; \ static void init(void);\ static void *create(void);\ private: \ @@ -97,7 +100,7 @@ private: \ EXTENSION_TYPESYSTEM_HEADER_WITH_OVERRIDE(); \ protected: \ static const App::PropertyData * extensionGetPropertyDataPtr(void); \ - virtual const App::PropertyData &extensionGetPropertyData(void) const override; \ + const App::PropertyData &extensionGetPropertyData(void) const override; \ private: \ static App::PropertyData propertyData @@ -254,6 +257,8 @@ public: virtual const char* extensionGetPropertyName(const Property* prop) const; /// get all properties of the class (including properties of the parent) virtual void extensionGetPropertyMap(std::map &Map) const; + /// See PropertyContainer::visitProperties for semantics + virtual void extensionVisitProperties(const std::function& visitor) const; /// get all properties of the class (including properties of the parent) virtual void extensionGetPropertyList(std::vector &List) const; @@ -327,6 +332,6 @@ private: // clang-format on -} //App +} // namespace App -#endif // APP_EXTENSION_H +#endif // SRC_APP_EXTENSION_H_ diff --git a/src/App/ExtensionContainer.cpp b/src/App/ExtensionContainer.cpp index 2fd642990d..9a002e576b 100644 --- a/src/App/ExtensionContainer.cpp +++ b/src/App/ExtensionContainer.cpp @@ -22,6 +22,11 @@ #include "PreCompiled.h" +#ifndef _PreComp_ +#include +#include +#include +#endif #include #include @@ -176,6 +181,13 @@ void ExtensionContainer::getPropertyMap(std::map& Map) c entry.second->extensionGetPropertyMap(Map); } } +void ExtensionContainer::visitProperties(const std::function& visitor) const +{ + App::PropertyContainer::visitProperties(visitor); + for (const auto &entry : _extensions) { + entry.second->extensionVisitProperties(visitor); + }; +} Property* ExtensionContainer::getPropertyByName(const char* name) const { diff --git a/src/App/ExtensionContainer.h b/src/App/ExtensionContainer.h index 7972ba4975..11b97c9054 100644 --- a/src/App/ExtensionContainer.h +++ b/src/App/ExtensionContainer.h @@ -21,8 +21,12 @@ ***************************************************************************/ -#ifndef APP_EXTENSIONCONTAINER_H -#define APP_EXTENSIONCONTAINER_H +#ifndef SRC_APP_EXTENSIONCONTAINER_H_ +#define SRC_APP_EXTENSIONCONTAINER_H_ + +#include +#include +#include #include "PropertyContainer.h" @@ -182,6 +186,8 @@ public: const char* getPropertyName(const Property* prop) const override; /// get all properties of the class (including properties of the parent) void getPropertyMap(std::map& Map) const override; + /// See PropertyContainer::visitProperties for semantics + void visitProperties(const std::function& visitor) const override; /// get all properties of the class (including properties of the parent) void getPropertyList(std::vector& List) const override; @@ -250,4 +256,4 @@ private: } // namespace App -#endif // APP_EXTENSIONCONTAINER_H +#endif // SRC_APP_EXTENSIONCONTAINER_H_ diff --git a/src/App/PropertyContainer.cpp b/src/App/PropertyContainer.cpp index 10100e65a3..9efdd7a0fd 100644 --- a/src/App/PropertyContainer.cpp +++ b/src/App/PropertyContainer.cpp @@ -22,6 +22,11 @@ #include "PreCompiled.h" +#ifndef _PreComp_ +#include +#include +#include +#endif #include #include @@ -92,6 +97,12 @@ void PropertyContainer::getPropertyList(std::vector &List) const getPropertyData().getPropertyList(this,List); } +void PropertyContainer::visitProperties(const std::function& visitor) const +{ + dynamicProps.visitProperties(visitor); + getPropertyData().visitProperties(this, visitor); +} + void PropertyContainer::getPropertyNamedList(std::vector > &List) const { dynamicProps.getPropertyNamedList(List); @@ -619,7 +630,15 @@ void PropertyData::getPropertyNamedList(OffsetBase offsetBase, } } - +void PropertyData::visitProperties(OffsetBase offsetBase, + const std::function& visitor) const +{ + merge(); + char* offset = offsetBase.getOffset(); + for (const auto& spec : propertyData.get<0>()) { + visitor(reinterpret_cast(spec.Offset + offset)); + }; +} /** \defgroup PropFrame Property framework \ingroup APP diff --git a/src/App/PropertyContainer.h b/src/App/PropertyContainer.h index 5a6b7647ae..c20cc2a9cf 100644 --- a/src/App/PropertyContainer.h +++ b/src/App/PropertyContainer.h @@ -21,11 +21,13 @@ ***************************************************************************/ -#ifndef APP_PROPERTYCONTAINER_H -#define APP_PROPERTYCONTAINER_H +#ifndef SRC_APP_PROPERTYCONTAINER_H_ +#define SRC_APP_PROPERTYCONTAINER_H_ #include #include +#include +#include #include #include "DynamicProperty.h" @@ -75,8 +77,12 @@ struct AppExport PropertyData //accepting void* struct OffsetBase { - OffsetBase(const App::PropertyContainer* container) : m_container(container) {}//explicit bombs - OffsetBase(const App::Extension* container) : m_container(container) {}//explicit bombs + // Lint wants these marked explicit, but they are currently used implicitly in enough + // places that I don't wnt to fix it. Instead we disable the Lint message. + // NOLINTNEXTLINE(runtime/explicit) + OffsetBase(const App::PropertyContainer* container) : m_container(container) {} + // NOLINTNEXTLINE(runtime/explicit) + OffsetBase(const App::Extension* container) : m_container(container) {} short int getOffsetTo(const App::Property* prop) const { auto *pt = (const char*)prop; @@ -134,6 +140,8 @@ struct AppExport PropertyData void getPropertyMap(OffsetBase offsetBase,std::map &Map) const; void getPropertyList(OffsetBase offsetBase,std::vector &List) const; void getPropertyNamedList(OffsetBase offsetBase, std::vector > &List) const; + /// See PropertyContainer::visitProperties for semantics + void visitProperties(OffsetBase offsetBase, const std::function& visitor) const; void merge(PropertyData *other=nullptr) const; void split(PropertyData *other); @@ -172,6 +180,11 @@ public: virtual void getPropertyMap(std::map &Map) const; /// get all properties of the class (including properties of the parent) virtual void getPropertyList(std::vector &List) const; + /// Call the given visitor for each property. The visiting order is undefined. + /// This method is necessary because PropertyContainer has no begin and end methods + /// and it is not practical to implement these. + /// What gets visited is undefined if the collection of Properties is changed during this call. + virtual void visitProperties(const std::function& visitor) const; /// get all properties with their names, may contain duplicates and aliases virtual void getPropertyNamedList(std::vector > &List) const; /// set the Status bit of all properties at once @@ -395,4 +408,4 @@ template<> void _class_::init(void){\ } // namespace App -#endif // APP_PROPERTYCONTAINER_H +#endif // SRC_APP_PROPERTYCONTAINER_H_ diff --git a/src/App/PropertyStandard.cpp b/src/App/PropertyStandard.cpp index 238df5d8b5..c89e4d1b6f 100644 --- a/src/App/PropertyStandard.cpp +++ b/src/App/PropertyStandard.cpp @@ -22,6 +22,16 @@ #include "PreCompiled.h" +#ifndef _PreComp_ +#include +#include +#include +#include +#include +#include +#include +#include +#endif #include #include @@ -1425,101 +1435,13 @@ void PropertyString::setValue(const char* newLabel) return; } - std::string _newLabel; - std::vector>> propChanges; - std::string label; + std::string label = newLabel; auto obj = dynamic_cast(getContainer()); bool commit = false; - if (obj && obj->isAttachedToDocument() && this == &obj->Label - && (!obj->getDocument()->testStatus(App::Document::Restoring) - || obj->getDocument()->testStatus(App::Document::Importing)) - && !obj->getDocument()->isPerformingTransaction()) { - // allow object to control label change - - static ParameterGrp::handle _hPGrp; - if (!_hPGrp) { - _hPGrp = GetApplication().GetUserParameter().GetGroup("BaseApp"); - _hPGrp = _hPGrp->GetGroup("Preferences")->GetGroup("Document"); - } - App::Document* doc = obj->getDocument(); - if (doc && !_hPGrp->GetBool("DuplicateLabels") && !obj->allowDuplicateLabel()) { - std::vector objectLabels; - std::vector::const_iterator it; - std::vector objs = doc->getObjects(); - bool match = false; - for (it = objs.begin(); it != objs.end(); ++it) { - if (*it == obj) { - continue; // don't compare object with itself - } - std::string objLabel = (*it)->Label.getValue(); - if (!match && objLabel == newLabel) { - match = true; - } - objectLabels.push_back(objLabel); - } - - // make sure that there is a name conflict otherwise we don't have to do anything - if (match && *newLabel) { - label = newLabel; - // remove number from end to avoid lengthy names - size_t lastpos = label.length() - 1; - while (label[lastpos] >= 48 && label[lastpos] <= 57) { - // if 'lastpos' becomes 0 then all characters are digits. In this case we use - // the complete label again - if (lastpos == 0) { - lastpos = label.length() - 1; - break; - } - lastpos--; - } - - bool changed = false; - label = label.substr(0, lastpos + 1); - if (label != obj->getNameInDocument() - && boost::starts_with(obj->getNameInDocument(), label)) { - // In case the label has the same base name as object's - // internal name, use it as the label instead. - const char* objName = obj->getNameInDocument(); - const char* c = &objName[lastpos + 1]; - for (; *c; ++c) { - if (*c < 48 || *c > 57) { - break; - } - } - if (*c == 0 - && std::find(objectLabels.begin(), - objectLabels.end(), - obj->getNameInDocument()) - == objectLabels.end()) { - label = obj->getNameInDocument(); - changed = true; - } - } - if (!changed) { - label = Base::Tools::getUniqueName(label, objectLabels, 3); - } - } - } - - if (label.empty()) { - label = newLabel; - } - obj->onBeforeChangeLabel(label); - newLabel = label.c_str(); - - if (!obj->getDocument()->testStatus(App::Document::Restoring)) { - // Only update label reference if we are not restoring. When - // importing (which also counts as restoring), it is possible the - // new object changes its label. However, we cannot update label - // references here, because object restoring is not based on - // dependency order. It can only be done in afterRestore(). - // - // See PropertyLinkBase::restoreLabelReference() for more details. - propChanges = PropertyLinkBase::updateLabelReferences(obj, newLabel); - } - + if (obj && this == &obj->Label) { + propChanges = obj->onProposedLabelChange(label); if (!propChanges.empty() && !GetApplication().getActiveTransaction()) { commit = true; std::ostringstream str; @@ -1529,11 +1451,11 @@ void PropertyString::setValue(const char* newLabel) } aboutToSetValue(); - _cValue = newLabel; + _cValue = label; hasSetValue(); for (auto& change : propChanges) { - change.first->Paste(*change.second.get()); + change.first->Paste(*change.second); } if (commit) { diff --git a/src/App/private/DocumentP.h b/src/App/private/DocumentP.h index 64d3b6697d..c7e73b6098 100644 --- a/src/App/private/DocumentP.h +++ b/src/App/private/DocumentP.h @@ -20,16 +20,22 @@ * * ***************************************************************************/ -#ifndef APP_DOCUMENTP_H -#define APP_DOCUMENTP_H +#ifndef SRC_APP_PRIVATE_DOCUMENTP_H_ +#define SRC_APP_PRIVATE_DOCUMENTP_H_ #ifdef _MSC_VER #pragma warning(disable : 4834) #endif +#include +#include +#include +#include + #include #include #include +#include #include #include #include @@ -65,6 +71,8 @@ struct DocumentP std::vector objectArray; std::unordered_set touchedObjs; std::unordered_map objectMap; + Base::UniqueNameManager objectNameManager; + Base::UniqueNameManager objectLabelManager; std::unordered_map objectIdMap; std::unordered_map partialLoadObjects; std::vector pendingRemove; @@ -129,6 +137,7 @@ struct DocumentP void clearDocument() { + objectLabelManager.clear(); objectArray.clear(); for (auto& v : objectMap) { v.second->setStatus(ObjectStatus::Destroy, true); @@ -136,6 +145,7 @@ struct DocumentP v.second = nullptr; } objectMap.clear(); + objectNameManager.clear(); objectIdMap.clear(); } @@ -161,4 +171,4 @@ struct DocumentP } // namespace App -#endif // APP_DOCUMENTP_H +#endif // SRC_APP_PRIVATE_DOCUMENTP_H_ diff --git a/src/Base/Reader.cpp b/src/Base/Reader.cpp index 69a8ecc23c..b63a294d23 100644 --- a/src/Base/Reader.cpp +++ b/src/Base/Reader.cpp @@ -24,7 +24,10 @@ #include "PreCompiled.h" #ifndef _PreComp_ -#include +#include +#include +#include +#include #include #include #endif @@ -465,14 +468,13 @@ const char* Base::XMLReader::addFile(const char* Name, Base::Persistence* Object temp.Object = Object; FileList.push_back(temp); - FileNames.push_back(temp.FileName); return Name; } -const std::vector& Base::XMLReader::getFilenames() const +bool Base::XMLReader::hasFilenames() const { - return FileNames; + return !FileList.empty(); } bool Base::XMLReader::hasReadFailed(const std::string& filename) const diff --git a/src/Base/Reader.h b/src/Base/Reader.h index f26c0617c8..ad4d5173ee 100644 --- a/src/Base/Reader.h +++ b/src/Base/Reader.h @@ -20,13 +20,14 @@ * * ***************************************************************************/ -#ifndef BASE_READER_H -#define BASE_READER_H +#ifndef SRC_BASE_READER_H_ +#define SRC_BASE_READER_H_ #include #include #include #include +#include #include #include @@ -250,8 +251,8 @@ public: const char* addFile(const char* Name, Base::Persistence* Object); /// process the requested file writes void readFiles(zipios::ZipInputStream& zipstream) const; - /// get all registered file names - const std::vector& getFilenames() const; + /// Returns whether reader has any registered filenames + bool hasFilenames() const; /// returns true if reading the file \a filename has failed bool hasReadFailed(const std::string& filename) const; bool isRegistered(Base::Persistence* Object) const; @@ -363,7 +364,6 @@ public: std::vector FileList; private: - std::vector FileNames; mutable std::vector FailedFiles; std::bitset<32> StatusBits; @@ -391,4 +391,4 @@ private: } // namespace Base -#endif +#endif // SRC_BASE_READER_H_ diff --git a/src/Base/Tools.cpp b/src/Base/Tools.cpp index 1c17850611..26a0867c8b 100644 --- a/src/Base/Tools.cpp +++ b/src/Base/Tools.cpp @@ -23,6 +23,8 @@ #include "PreCompiled.h" #ifndef _PreComp_ +#include +#include #include #include #endif @@ -31,130 +33,6 @@ #include "Interpreter.h" #include "Tools.h" -namespace Base -{ -struct string_comp -{ - // s1 and s2 must be numbers represented as string - bool operator()(const std::string& s1, const std::string& s2) - { - if (s1.size() < s2.size()) { - return true; - } - if (s1.size() > s2.size()) { - return false; - } - - return s1 < s2; - } - static std::string increment(const std::string& s) - { - std::string n = s; - int addcarry = 1; - for (std::string::reverse_iterator it = n.rbegin(); it != n.rend(); ++it) { - if (addcarry == 0) { - break; - } - int d = *it - 48; - d = d + addcarry; - *it = ((d % 10) + 48); - addcarry = d / 10; - } - if (addcarry > 0) { - std::string b; - b.resize(1); - b[0] = addcarry + 48; - n = b + n; - } - - return n; - } -}; - -class unique_name -{ -public: - unique_name(std::string name, const std::vector& names, int padding) - : base_name {std::move(name)} - , padding {padding} - { - removeDigitsFromEnd(); - findHighestSuffix(names); - } - - std::string get() const - { - return appendSuffix(); - } - -private: - void removeDigitsFromEnd() - { - std::string::size_type pos = base_name.find_last_not_of("0123456789"); - if (pos != std::string::npos && (pos + 1) < base_name.size()) { - num_suffix = base_name.substr(pos + 1); - base_name.erase(pos + 1); - } - } - - void findHighestSuffix(const std::vector& names) - { - for (const auto& name : names) { - if (name.substr(0, base_name.length()) == base_name) { // same prefix - std::string suffix(name.substr(base_name.length())); - if (!suffix.empty()) { - std::string::size_type pos = suffix.find_first_not_of("0123456789"); - if (pos == std::string::npos) { - num_suffix = std::max(num_suffix, suffix, Base::string_comp()); - } - } - } - } - } - - std::string appendSuffix() const - { - std::stringstream str; - str << base_name; - if (padding > 0) { - str.fill('0'); - str.width(padding); - } - str << Base::string_comp::increment(num_suffix); - return str.str(); - } - -private: - std::string num_suffix; - std::string base_name; - int padding; -}; - -} // namespace Base - -std::string -Base::Tools::getUniqueName(const std::string& name, const std::vector& names, int pad) -{ - if (names.empty()) { - return name; - } - - Base::unique_name unique(name, names, pad); - return unique.get(); -} - -std::string Base::Tools::addNumber(const std::string& name, unsigned int num, int d) -{ - std::stringstream str; - str << name; - if (d > 0) { - str.fill('0'); - str.width(d); - } - str << num; - return str.str(); -} - std::string Base::Tools::getIdentifier(const std::string& name) { if (name.empty()) { diff --git a/src/Base/Tools.h b/src/Base/Tools.h index 107be7617d..22a705708f 100644 --- a/src/Base/Tools.h +++ b/src/Base/Tools.h @@ -21,8 +21,8 @@ ***************************************************************************/ -#ifndef BASE_TOOLS_H -#define BASE_TOOLS_H +#ifndef SRC_BASE_TOOLS_H_ +#define SRC_BASE_TOOLS_H_ #ifndef FC_GLOBAL_H #include @@ -283,9 +283,6 @@ public: struct BaseExport Tools { - static std::string - getUniqueName(const std::string&, const std::vector&, int d = 0); - static std::string addNumber(const std::string&, unsigned int, int d = 0); static std::string getIdentifier(const std::string&); static std::wstring widen(const std::string& str); static std::string narrow(const std::wstring& str); @@ -341,4 +338,4 @@ struct BaseExport ZipTools } // namespace Base -#endif // BASE_TOOLS_H +#endif // SRC_BASE_TOOLS_H_ diff --git a/src/Base/UniqueNameManager.cpp b/src/Base/UniqueNameManager.cpp index e2941ffb45..34a050606a 100644 --- a/src/Base/UniqueNameManager.cpp +++ b/src/Base/UniqueNameManager.cpp @@ -136,8 +136,8 @@ Base::UniqueNameManager::decomposeName(const std::string& name) const digitCount, digitCount == 0 ? 0U : std::stoul(name.substr(name.crend() - digitsStart, digitCount))}; } -bool Base::UniqueNameManager::sameBaseName(const std::string& first, - const std::string& second) const +bool Base::UniqueNameManager::haveSameBaseName(const std::string& first, + const std::string& second) const { auto firstSuffixStart = getNameSuffixStartPosition(first); auto secondSuffixStart = getNameSuffixStartPosition(second); diff --git a/src/Base/UniqueNameManager.h b/src/Base/UniqueNameManager.h index ee17a39be8..c91c9772f1 100644 --- a/src/Base/UniqueNameManager.h +++ b/src/Base/UniqueNameManager.h @@ -124,7 +124,7 @@ public: virtual ~UniqueNameManager() = default; /// Check if two names are unique forms of the same base name - bool sameBaseName(const std::string& first, const std::string& second) const; + bool haveSameBaseName(const std::string& first, const std::string& second) const; /// Register a name in the collection. /// This collection acts as a multiset, so multiple registrations of the same diff --git a/src/Base/Writer.cpp b/src/Base/Writer.cpp index 91e67bbdb9..a068884923 100644 --- a/src/Base/Writer.cpp +++ b/src/Base/Writer.cpp @@ -22,6 +22,12 @@ #include "PreCompiled.h" +#ifndef _PreComp_ +#include +#include +#include +#include +#endif #include #include @@ -246,56 +252,19 @@ std::string Writer::addFile(const char* Name, const Base::Persistence* Object) assert(!isForceXML()); FileEntry temp; - temp.FileName = getUniqueFileName(Name); + temp.FileName = Name ? Name : ""; + if (FileNameManager.containsName(temp.FileName)) { + temp.FileName = FileNameManager.makeUniqueName(temp.FileName); + } temp.Object = Object; FileList.push_back(temp); - - FileNames.push_back(temp.FileName); + FileNameManager.addExactName(temp.FileName); // return the unique file name return temp.FileName; } -std::string Writer::getUniqueFileName(const char* Name) -{ - // name in use? - std::string CleanName = (Name ? Name : ""); - std::vector::const_iterator pos; - pos = find(FileNames.begin(), FileNames.end(), CleanName); - - if (pos == FileNames.end()) { - // if not, name is OK - return CleanName; - } - - std::vector names; - names.reserve(FileNames.size()); - FileInfo fi(CleanName); - CleanName = fi.fileNamePure(); - std::string ext = fi.extension(); - for (pos = FileNames.begin(); pos != FileNames.end(); ++pos) { - fi.setFile(*pos); - std::string FileName = fi.fileNamePure(); - if (fi.extension() == ext) { - names.push_back(FileName); - } - } - - std::stringstream str; - str << Base::Tools::getUniqueName(CleanName, names); - if (!ext.empty()) { - str << "." << ext; - } - - return str.str(); -} - -const std::vector& Writer::getFilenames() const -{ - return FileNames; -} - void Writer::incInd() { if (indent < 1020) { diff --git a/src/Base/Writer.h b/src/Base/Writer.h index 15d8368768..5211a0874b 100644 --- a/src/Base/Writer.h +++ b/src/Base/Writer.h @@ -20,8 +20,8 @@ * * ***************************************************************************/ -#ifndef BASE_WRITER_H -#define BASE_WRITER_H +#ifndef SRC_BASE_WRITER_H_ +#define SRC_BASE_WRITER_H_ #include @@ -32,6 +32,8 @@ #include +#include + #include "FileInfo.h" @@ -49,6 +51,24 @@ class Persistence; */ class BaseExport Writer { +private: + // This overrides UniqueNameManager's suffix-locating function so that the last '.' and + // everything after it is considered suffix. + class UniqueFileNameManager: public UniqueNameManager + { + protected: + std::string::const_reverse_iterator + getNameSuffixStartPosition(const std::string& name) const override + { + // This is an awkward way to do this, because the FileInfo class only yields pieces of + // the path, not delimiter positions. We can't just use fi.extension().size() because + // both "xyz" and "xyz." would yield three; we need the length of the extension + // *including its delimiter* so we use the length difference between the fileName and + // fileNamePure. + FileInfo fi(name); + return name.rbegin() + (fi.fileName().size() - fi.fileNamePure().size()); + } + }; public: Writer(); @@ -77,8 +97,6 @@ public: std::string addFile(const char* Name, const Base::Persistence* Object); /// process the requested file storing virtual void writeFiles() = 0; - /// get all registered file names - const std::vector& getFilenames() const; /// Set mode void setMode(const std::string& mode); /// Set modes @@ -144,14 +162,13 @@ public: std::string ObjectName; protected: - std::string getUniqueFileName(const char* Name); struct FileEntry { std::string FileName; const Base::Persistence* Object; }; std::vector FileList; - std::vector FileNames; + UniqueFileNameManager FileNameManager; std::vector Errors; std::set Modes; @@ -283,4 +300,4 @@ protected: } // namespace Base -#endif // BASE_WRITER_H +#endif // SRC_BASE_WRITER_H_ diff --git a/src/Gui/Document.cpp b/src/Gui/Document.cpp index 4a16841c4c..2edff11f6b 100644 --- a/src/Gui/Document.cpp +++ b/src/Gui/Document.cpp @@ -23,6 +23,14 @@ #include "PreCompiled.h" #ifndef _PreComp_ +# include +# include +# include +# include +# include +# include +# include +# include # include # include # include @@ -1938,8 +1946,9 @@ void Document::importObjects(const std::vector& obj, Base: localreader->readEndElement("Document"); // In the file GuiDocument.xml new data files might be added - if (!localreader->getFilenames().empty()) + if (localreader->hasFilenames()) { reader.initLocalReader(localreader); + } } void Document::slotFinishImportObjects(const std::vector &objs) { diff --git a/src/Gui/ViewProviderLink.cpp b/src/Gui/ViewProviderLink.cpp index 14b15f44ac..b4531a9fe5 100644 --- a/src/Gui/ViewProviderLink.cpp +++ b/src/Gui/ViewProviderLink.cpp @@ -25,6 +25,16 @@ #ifndef _PreComp_ # include # include +# include +# include +# include +# include +# include +# include +# include +# include +# include +# include # include # include # include @@ -41,7 +51,6 @@ # include # include # include -# include # include # include # include @@ -191,7 +200,7 @@ public: } } - LinkInfo(ViewProviderDocumentObject *vp) + explicit LinkInfo(ViewProviderDocumentObject *vp) :ref(0),pcLinked(vp) { FC_LOG("new link to " << pcLinked->getObject()->getFullName()); @@ -697,7 +706,7 @@ namespace Gui { px->release(); } -} +} // namespace Gui #endif //////////////////////////////////////////////////////////////////////////////////// @@ -3286,7 +3295,16 @@ void ViewProviderLink::getPropertyMap(std::map &Map) } } -void ViewProviderLink::getPropertyList(std::vector &List) const { +void ViewProviderLink::visitProperties(const std::function& visitor) const +{ + inherited::visitProperties(visitor); + if (childVp != nullptr) { + childVp->visitProperties(visitor); + } +} + +void ViewProviderLink::getPropertyList(std::vector& List) const +{ std::map Map; getPropertyMap(Map); List.reserve(List.size()+Map.size()); diff --git a/src/Gui/ViewProviderLink.h b/src/Gui/ViewProviderLink.h index c61fb19420..c84d31c504 100644 --- a/src/Gui/ViewProviderLink.h +++ b/src/Gui/ViewProviderLink.h @@ -20,10 +20,15 @@ * * ****************************************************************************/ -#ifndef GUI_VIEWPROVIDER_LINK_H -#define GUI_VIEWPROVIDER_LINK_H +#ifndef SRC_GUI_VIEWPROVIDER_LINK_H_ +#define SRC_GUI_VIEWPROVIDER_LINK_H_ #include +#include +#include +#include +#include +#include #include "Selection/SoFCUnifiedSelection.h" #include "ViewProviderDocumentObject.h" @@ -260,7 +265,9 @@ public: App::Property *getPropertyByName(const char* name) const override; void getPropertyMap(std::map &Map) const override; - void getPropertyList(std::vector &List) const override; + /// See PropertyContainer::visitProperties for semantics + void visitProperties(const std::function& visitor) const override; + void getPropertyList(std::vector& List) const override; ViewProviderDocumentObject *getLinkedViewProvider( std::string *subname=nullptr, bool recursive=false) const override; @@ -337,4 +344,4 @@ using ViewProviderLinkPython = ViewProviderFeaturePythonT; } //namespace Gui -#endif // GUI_VIEWPROVIDER_LINK_H +#endif // SRC_GUI_VIEWPROVIDER_LINK_H_ diff --git a/src/Mod/Spreadsheet/App/Sheet.cpp b/src/Mod/Spreadsheet/App/Sheet.cpp index f94a1d9642..1e309c1bff 100644 --- a/src/Mod/Spreadsheet/App/Sheet.cpp +++ b/src/Mod/Spreadsheet/App/Sheet.cpp @@ -27,6 +27,12 @@ #include #include #include +#include +#include +#include +#include +#include +#include #endif #include @@ -874,6 +880,17 @@ void Sheet::getPropertyNamedList(std::vector>& } } +void Sheet::visitProperties(const std::function& visitor) const +{ + DocumentObject::visitProperties(visitor); + for (const auto& v : cells.aliasProp) { + auto prop = getProperty(v.first); + if (prop != nullptr) { + visitor(prop); + } + }; +} + void Sheet::touchCells(Range range) { do { @@ -1136,7 +1153,7 @@ DocumentObjectExecReturn* Sheet::execute() catch (std::exception&) { // TODO: evaluate using a more specific exception (not_a_dag) // Cycle detected; flag all with errors Base::Console().Error("Cyclic dependency detected in spreadsheet : %s\n", - *pcNameInDocument); + getNameInDocument()); std::ostringstream ss; ss << "Cyclic dependency"; int count = 0; diff --git a/src/Mod/Spreadsheet/App/Sheet.h b/src/Mod/Spreadsheet/App/Sheet.h index 9b6ae0f6dc..9c5a259c1f 100644 --- a/src/Mod/Spreadsheet/App/Sheet.h +++ b/src/Mod/Spreadsheet/App/Sheet.h @@ -20,8 +20,8 @@ * * ***************************************************************************/ -#ifndef Spreadsheet_Spreadsheet_H -#define Spreadsheet_Spreadsheet_H +#ifndef SRC_MOD_SPREADSHEET_APP_SHEET_H_ +#define SRC_MOD_SPREADSHEET_APP_SHEET_H_ #ifdef signals #undef signals @@ -29,6 +29,10 @@ #endif #include +#include +#include +#include +#include #include #include @@ -189,6 +193,9 @@ public: void getPropertyNamedList(std::vector>& List) const override; + /// See PropertyContainer::visitProperties for semantics + void visitProperties(const std::function& visitor) const override; + short mustExecute() const override; App::DocumentObjectExecReturn* execute() override; @@ -307,4 +314,4 @@ using SheetPython = App::FeaturePythonT; } // namespace Spreadsheet -#endif // Spreadsheet_Spreadsheet_H +#endif // SRC_MOD_SPREADSHEET_APP_SHEET_H_ diff --git a/tests/src/Base/Tools.cpp b/tests/src/Base/Tools.cpp index f2385433d3..6bcb0b4c0b 100644 --- a/tests/src/Base/Tools.cpp +++ b/tests/src/Base/Tools.cpp @@ -1,47 +1,9 @@ #include #include #include +#include // NOLINTBEGIN(cppcoreguidelines-*,readability-*) -TEST(BaseToolsSuite, TestUniqueName1) -{ - EXPECT_EQ(Base::Tools::getUniqueName("Body", {}), "Body"); -} - -TEST(BaseToolsSuite, TestUniqueName2) -{ - EXPECT_EQ(Base::Tools::getUniqueName("Body", {"Body"}, 1), "Body1"); -} - -TEST(BaseToolsSuite, TestUniqueName3) -{ - EXPECT_EQ(Base::Tools::getUniqueName("Body", {"Body"}, 3), "Body001"); -} - -TEST(BaseToolsSuite, TestUniqueName4) -{ - EXPECT_EQ(Base::Tools::getUniqueName("Body", {"Body001"}, 3), "Body002"); -} - -TEST(BaseToolsSuite, TestUniqueName5) -{ - EXPECT_EQ(Base::Tools::getUniqueName("Body", {"Body", "Body001"}, 3), "Body002"); -} - -TEST(BaseToolsSuite, TestUniqueName6) -{ - EXPECT_EQ(Base::Tools::getUniqueName("Body001", {"Body", "Body001"}, 3), "Body002"); -} - -TEST(BaseToolsSuite, TestUniqueName7) -{ - EXPECT_EQ(Base::Tools::getUniqueName("Body001", {"Body"}, 3), "Body002"); -} - -TEST(BaseToolsSuite, TestUniqueName8) -{ - EXPECT_EQ(Base::Tools::getUniqueName("Body12345", {"Body"}, 3), "Body12346"); -} TEST(Tools, TestIota) {