From 83202d8ad67a719cb62e29d9dc3496a076999f1f Mon Sep 17 00:00:00 2001 From: Kevin Martin Date: Fri, 13 Dec 2024 11:54:46 -0500 Subject: [PATCH] Address the poor performance of the existing unique-name generation (#17944) * Address the poor performance of the existing unique-name generation 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: Create UniqueNameManager to keep a list of existing names organized in a manner that eases unique-name generation. This class essentially acts as a set of names, with the ability to add and remove names and check if a name is already there, with the added ability to take a prototype name and generate a unique form for it which is not already in the set. Eliminate Tools::getUniqueName Make DocumentObject naming use the new UniqueNameManager class Make DocumentObject Label naming use the new UniqueNameManager class. Labels are not always unique; unique labels are generated if the settings at the time request it (and other conditions). Because of this the Label management requires additionally keeping a map of counts for labels which already exist more than once. These collections are maintained via notifications of value changes on the Label properties of the objects in the document. 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 would be simpler if existing fill-container methods all used this. 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. The unit tests for Tools:getUniqueName have been changed to test UniqueNameManager.makeUniqueName instead. This revealed a small regression insofar as passing a prototype name like "xyz1234" to the old code would yield "xyz1235" whether or not "xyz1234" already existed, while the new code will return the next name above the currently-highest name on the "xyz" model, which could be "xyz" or "xyz1". * Correct wrong case on include path * Implement suggested code changes Also change the semantics of visitProperties to not have any short-circuit return * Remove reference through undefined iterator * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Fix up some comments for DOxygen --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> --- src/App/Application.cpp | 156 ++++++++------- src/App/Application.h | 4 +- src/App/Document.cpp | 156 +++++++++------ src/App/Document.h | 22 ++- src/App/DocumentObject.cpp | 73 +++++++ src/App/DocumentObject.h | 11 +- src/App/DynamicProperty.cpp | 38 ++-- src/App/DynamicProperty.h | 2 + src/App/Extension.cpp | 4 + src/App/Extension.h | 2 + src/App/ExtensionContainer.cpp | 7 + src/App/ExtensionContainer.h | 2 + src/App/PropertyContainer.cpp | 16 +- src/App/PropertyContainer.h | 7 + src/App/PropertyStandard.cpp | 96 +-------- src/App/private/DocumentP.h | 7 + src/Base/Reader.cpp | 5 +- src/Base/Reader.h | 5 +- src/Base/Tools.cpp | 310 +++++++++++++++++++----------- src/Base/Tools.h | 96 ++++++++- src/Base/Writer.cpp | 47 +---- src/Base/Writer.h | 25 ++- src/Gui/Document.cpp | 2 +- src/Gui/ViewProviderLink.cpp | 11 +- src/Gui/ViewProviderLink.h | 4 +- src/Mod/Spreadsheet/App/Sheet.cpp | 13 +- src/Mod/Spreadsheet/App/Sheet.h | 3 + tests/src/Base/Tools.cpp | 32 ++- 28 files changed, 721 insertions(+), 435 deletions(-) diff --git a/src/App/Application.cpp b/src/App/Application.cpp index 56ef0a38b2..9b9a0b7f56 100644 --- a/src/App/Application.cpp +++ b/src/App/Application.cpp @@ -450,41 +450,16 @@ 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, bool createView, bool tempDoc) +Document* Application::newDocument(const char * proposedName, const char * proposedLabel, bool createView, bool tempDoc) { - auto getNameAndLabel = [this](const char * Name, const char * UserName) -> std::tuple { - bool defaultName = (!Name || Name[0] == '\0'); + std::string name; + bool useDefaultName = (!proposedName || proposedName[0] == '\0'); + // get a valid name anyway! + if (useDefaultName) { + proposedName = "Unnamed"; + } - // get a valid name anyway! - if (defaultName) { - Name = "Unnamed"; - } - - std::string userName; - if (UserName && UserName[0] != '\0') { - userName = UserName; - } - else { - userName = defaultName ? 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(), tempDoc); + name = getUniqueDocumentName(proposedName, tempDoc); // return the temporary document if it exists if (tempDoc) { @@ -493,53 +468,65 @@ Document* Application::newDocument(const char * Name, const char * UserName, boo return it->second; } - // create the FreeCAD document - std::unique_ptr newDoc(new Document(name.c_str())); - newDoc->setStatus(Document::TempDoc, tempDoc); + // Determine the document's Label + std::string label; + if (proposedLabel && proposedLabel[0] != '\0') { + label = proposedLabel; + } + else { + label = useDefaultName ? 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 + Document* doc = new Document(name.c_str()); + doc->setStatus(Document::TempDoc, tempDoc); // 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 + auto oldActiveDoc = _pActiveDoc; + setActiveDocumentNoSignal(doc); + signalNewDocument(*doc, createView); - signalNewDocument(*_pActiveDoc, 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 (tempDoc && oldActiveDoc) @@ -629,13 +616,17 @@ std::string Application::getUniqueDocumentName(const char *Name, bool tempDoc) c 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); } } @@ -1053,6 +1044,14 @@ Document* Application::getActiveDocument() const } void Application::setActiveDocument(Document* pDoc) +{ + setActiveDocumentNoSignal(pDoc); + + if (pDoc) + signalActiveDocument(*pDoc); +} + +void Application::setActiveDocumentNoSignal(Document* pDoc) { _pActiveDoc = pDoc; @@ -1060,18 +1059,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 e2a32adefe..2a19bd80fd 100644 --- a/src/App/Application.h +++ b/src/App/Application.h @@ -160,7 +160,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 @@ -505,6 +505,8 @@ private: static void cleanupUnits(); + void setActiveDocumentNoSignal(App::Document* pDoc); + /** @name member for parameter */ //@{ static Base::Reference _pcSysParamMngr; diff --git a/src/App/Document.cpp b/src/App/Document.cpp index 131a18ba85..b834a4777b 100644 --- a/src/App/Document.cpp +++ b/src/App/Document.cpp @@ -645,8 +645,11 @@ void Document::clearDocument() setStatus(Document::PartialDoc, false); d->clearRecomputeLog(); + d->objectLabelCounts.clear(); + d->objectLabelManager.clear(); d->objectArray.clear(); d->objectMap.clear(); + d->objectNameManager.clear(); d->objectIdMap.clear(); d->lastObjectId = 0; } @@ -2250,6 +2253,59 @@ bool Document::saveToFile(const char* filename) const return true; } +void Document::registerLabel(const std::string& newLabel) +{ + if (newLabel.empty()) { + return; + } + if (!d->objectLabelManager.containsName(newLabel)) { + // First occurrence of label. We make no entry in objectLabelCounts when the count is one. + d->objectLabelManager.addExactName(newLabel); + } + else { + auto it = d->objectLabelCounts.find(newLabel); + if (it != d->objectLabelCounts.end()) { + // There is a count already greater then one, so increment it + it->second++; + } + else { + // There is no count entry, which implies one, so register a count of two + d->objectLabelCounts.insert(std::pair(newLabel, 2)); + } + } +} + +void Document::unregisterLabel(const std::string& oldLabel) +{ + if (oldLabel.empty()) { + return; + } + auto it = d->objectLabelCounts.find(oldLabel); + if (it == d->objectLabelCounts.end()) { + // Missing count implies a count of one, or an unregistered name + d->objectLabelManager.removeExactName(oldLabel); + return; + } + if (--it->second == 1) { + // Decremented to one, remove the count entry + d->objectLabelCounts.erase(it); + } +} + +bool Document::containsLabel(const std::string& label) +{ + return d->objectLabelManager.containsName(label); +} + +std::string Document::makeUniqueLabel(const std::string& modelLabel) +{ + if (modelLabel.empty()) { + return std::string(); + } + + return d->objectLabelManager.makeUniqueName(modelLabel, 3); +} + bool Document::isAnyRestoring() { return globalIsRestoring; @@ -2276,7 +2332,10 @@ void Document::restore(const char* filename, setStatus(Document::PartialDoc, false); d->clearRecomputeLog(); + d->objectLabelCounts.clear(); + d->objectLabelManager.clear(); d->objectArray.clear(); + d->objectNameManager.clear(); d->objectMap.clear(); d->objectIdMap.clear(); d->lastObjectId = 0; @@ -3585,6 +3644,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; @@ -3593,6 +3653,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. @@ -3653,13 +3715,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; @@ -3674,29 +3729,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; @@ -3705,6 +3750,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); @@ -3765,6 +3812,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; @@ -3775,6 +3823,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); @@ -3798,12 +3848,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); @@ -3832,6 +3884,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) { @@ -3919,6 +3980,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) { @@ -3932,6 +3994,7 @@ void Document::removeObject(const char* sName) if (tobedestroyed) { tobedestroyed->pcNameInDocument = nullptr; } + d->objectNameManager.removeExactName(pos->first); d->objectMap.erase(pos); } @@ -4001,6 +4064,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(); @@ -4277,50 +4342,29 @@ 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 + std::tuple +Document::decomposeName(const std::string& name, std::string& baseName, std::string& nameExtension) { - std::vector mm = getObjects(); - std::vector labels; - labels.reserve(mm.size()); + return d->objectNameManager.decomposeName(name, baseName, nameExtension); +} - 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 36a878ee10..a740c2e530 100644 --- a/src/App/Document.h +++ b/src/App/Document.h @@ -287,6 +287,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 * @@ -318,11 +322,14 @@ 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; + /// Break an object Name or Label into its base parts, returning tuple(digitCount, digitsValue) + std::tuple + decomposeName(const std::string& name, std::string& baseName, std::string& nameExtension); /// Returns a list of document's objects including the dependencies std::vector getDependingObjects() const; /// Returns a list of all Objects @@ -567,6 +574,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; diff --git a/src/App/DocumentObject.cpp b/src/App/DocumentObject.cpp index bd2b3b395e..8c2ad17b92 100644 --- a/src/App/DocumentObject.cpp +++ b/src/App/DocumentObject.cpp @@ -814,6 +814,74 @@ 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); + std::string label; + if (!isAttachedToDocument() + || (getDocument()->testStatus(App::Document::Restoring) + && !getDocument()->testStatus(App::Document::Importing)) + || getDocument()->isPerformingTransaction()) { + return {}; + } + static ParameterGrp::handle _hPGrp; + if (!_hPGrp) { + _hPGrp = GetApplication().GetUserParameter().GetGroup("BaseApp"); + _hPGrp = _hPGrp->GetGroup("Preferences")->GetGroup("Document"); + } + App::Document* doc = getDocument(); + if (doc && newLabel.size() > 0 && !_hPGrp->GetBool("DuplicateLabels") && !allowDuplicateLabel() + && doc->containsLabel(newLabel)) { + // We must ensure the Label is unique in the document (well, sort of...). + // If the base name of the proposed label equals the base name of the object Name, we use + // the (uniquefied) object Name, which could actually be identical to another object's Label, + // but probably isn't. + // 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. + std::string objName = getNameInDocument(); + std::string objBaseName; + std::string objSuffix; + doc->decomposeName(objName, objBaseName, objSuffix); + std::string newBaseName; + std::string newSuffix; + doc->decomposeName(newLabel, newBaseName, newSuffix); + if (newBaseName == objBaseName && newSuffix == objSuffix) { + newLabel = objName; + } + else { + // 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..666f066dfb 100644 --- a/src/App/DocumentObject.h +++ b/src/App/DocumentObject.h @@ -513,7 +513,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 +772,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; diff --git a/src/App/DynamicProperty.cpp b/src/App/DynamicProperty.cpp index f79ecd5650..c9bdb89981 100644 --- a/src/App/DynamicProperty.cpp +++ b/src/App/DynamicProperty.cpp @@ -70,7 +70,13 @@ void DynamicProperty::getPropertyNamedList( } } -void DynamicProperty::getPropertyMap(std::map& Map) const +void DynamicProperty::visitProperties(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; @@ -298,25 +304,21 @@ bool DynamicProperty::removeDynamicProperty(const char* name) std::string DynamicProperty::getUniquePropertyName(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 diff --git a/src/App/DynamicProperty.h b/src/App/DynamicProperty.h index 29936fa665..0e24c63ca5 100644 --- a/src/App/DynamicProperty.h +++ b/src/App/DynamicProperty.h @@ -87,6 +87,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(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 diff --git a/src/App/Extension.cpp b/src/App/Extension.cpp index c84e613de4..c942a0e5cb 100644 --- a/src/App/Extension.cpp +++ b/src/App/Extension.cpp @@ -181,6 +181,10 @@ void Extension::extensionGetPropertyMap(std::map& Map) c extensionGetPropertyData().getPropertyMap(this, Map); } +void Extension::extensionVisitProperties(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..b4395d8704 100644 --- a/src/App/Extension.h +++ b/src/App/Extension.h @@ -254,6 +254,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(std::function visitor) const; /// get all properties of the class (including properties of the parent) virtual void extensionGetPropertyList(std::vector &List) const; diff --git a/src/App/ExtensionContainer.cpp b/src/App/ExtensionContainer.cpp index 2fd642990d..e16f7bcac9 100644 --- a/src/App/ExtensionContainer.cpp +++ b/src/App/ExtensionContainer.cpp @@ -176,6 +176,13 @@ void ExtensionContainer::getPropertyMap(std::map& Map) c entry.second->extensionGetPropertyMap(Map); } } +void ExtensionContainer::visitProperties(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 9fb5d6284f..957c8a34d7 100644 --- a/src/App/ExtensionContainer.h +++ b/src/App/ExtensionContainer.h @@ -182,6 +182,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(std::function visitor) const override; /// get all properties of the class (including properties of the parent) void getPropertyList(std::vector& List) const override; diff --git a/src/App/PropertyContainer.cpp b/src/App/PropertyContainer.cpp index 10100e65a3..3b33044d76 100644 --- a/src/App/PropertyContainer.cpp +++ b/src/App/PropertyContainer.cpp @@ -92,6 +92,12 @@ void PropertyContainer::getPropertyList(std::vector &List) const getPropertyData().getPropertyList(this,List); } +void PropertyContainer::visitProperties(std::function visitor) const +{ + dynamicProps.visitProperties(visitor); + getPropertyData().visitProperties(this, visitor); +} + void PropertyContainer::getPropertyNamedList(std::vector > &List) const { dynamicProps.getPropertyNamedList(List); @@ -619,7 +625,15 @@ void PropertyData::getPropertyNamedList(OffsetBase offsetBase, } } - +void PropertyData::visitProperties(OffsetBase offsetBase, + 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 b8b5ac91ae..bbec1ab618 100644 --- a/src/App/PropertyContainer.h +++ b/src/App/PropertyContainer.h @@ -134,6 +134,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, std::function visitor) const; void merge(PropertyData *other=nullptr) const; void split(PropertyData *other); @@ -172,6 +174,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(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 diff --git a/src/App/PropertyStandard.cpp b/src/App/PropertyStandard.cpp index 696afa0ec4..1d91a97eda 100644 --- a/src/App/PropertyStandard.cpp +++ b/src/App/PropertyStandard.cpp @@ -1385,101 +1385,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; @@ -1489,7 +1401,7 @@ void PropertyString::setValue(const char* newLabel) } aboutToSetValue(); - _cValue = newLabel; + _cValue = label; hasSetValue(); for (auto& change : propChanges) { diff --git a/src/App/private/DocumentP.h b/src/App/private/DocumentP.h index 64d3b6697d..a6006be3b8 100644 --- a/src/App/private/DocumentP.h +++ b/src/App/private/DocumentP.h @@ -30,6 +30,7 @@ #include #include #include +#include #include #include #include @@ -65,6 +66,9 @@ struct DocumentP std::vector objectArray; std::unordered_set touchedObjs; std::unordered_map objectMap; + Base::UniqueNameManager objectNameManager; + Base::UniqueNameManager objectLabelManager; + std::unordered_map objectLabelCounts; std::unordered_map objectIdMap; std::unordered_map partialLoadObjects; std::vector pendingRemove; @@ -129,6 +133,8 @@ struct DocumentP void clearDocument() { + objectLabelCounts.clear(); + objectLabelManager.clear(); objectArray.clear(); for (auto& v : objectMap) { v.second->setStatus(ObjectStatus::Destroy, true); @@ -136,6 +142,7 @@ struct DocumentP v.second = nullptr; } objectMap.clear(); + objectNameManager.clear(); objectIdMap.clear(); } diff --git a/src/Base/Reader.cpp b/src/Base/Reader.cpp index 63b85bfa81..2cbbf032d7 100644 --- a/src/Base/Reader.cpp +++ b/src/Base/Reader.cpp @@ -463,14 +463,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.size() > 0; } bool Base::XMLReader::hasReadFailed(const std::string& filename) const diff --git a/src/Base/Reader.h b/src/Base/Reader.h index a11b1966a2..005114027d 100644 --- a/src/Base/Reader.h +++ b/src/Base/Reader.h @@ -251,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; @@ -364,7 +364,6 @@ public: std::vector FileList; private: - std::vector FileNames; mutable std::vector FailedFiles; std::bitset<32> StatusBits; diff --git a/src/Base/Tools.cpp b/src/Base/Tools.cpp index 71f6021bee..374a15b237 100644 --- a/src/Base/Tools.cpp +++ b/src/Base/Tools.cpp @@ -33,130 +33,214 @@ #include "Interpreter.h" #include "Tools.h" -namespace Base +void Base::UniqueNameManager::PiecewiseSparseIntegerSet::Add(uint value) { -struct string_comp + etype newSpan(value, 1); + iterator above = Spans.lower_bound(newSpan); + if (above != Spans.end() && above->first <= value) { + // The found span includes value so there is nothing to do as it is already in the set. + return; + } + + // Set below to the next span down, if any + iterator below; + if (above == Spans.begin()) { + below = Spans.end(); + } + else { + below = above; + --below; + } + + if (above != Spans.end() && below != Spans.end() + && above->first - below->first + 1 == below->second) { + // below and above have a gap of exactly one between them, and this must be value + // so we coalesce the two spans (and the gap) into one. + newSpan = etype(below->first, below->second + above->second + 1); + Spans.erase(above); + above = Spans.erase(below); + } + if (below != Spans.end() && value - below->first == below->second) { + // value is adjacent to the end of below, so just expand below by one + newSpan = etype(below->first, below->second + 1); + above = Spans.erase(below); + } + else if (above != Spans.end() && above->first - value == 1) { + // value is adjacent to the start of above, so juse expand above down by one + newSpan = etype(above->first - 1, above->second + 1); + above = Spans.erase(above); + } + // else value is not adjacent to any existing span, so just make anew span for it + Spans.insert(above, newSpan); +} +void Base::UniqueNameManager::PiecewiseSparseIntegerSet::Remove(uint value) { - // 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; + etype newSpan(value, 1); + iterator at = Spans.lower_bound(newSpan); + if (at == Spans.end() || at->first > value) { + // The found span does not include value so there is nothing to do, as it is already not in + // the set. + return; } - 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; + if (at->second == 1) { + // value is the only in this span, just remove the span + Spans.erase(at); } -}; - -class unique_name + else if (at->first == value) { + // value is the first in this span, trim the lower end + etype replacement(at->first + 1, at->second - 1); + Spans.insert(Spans.erase(at), replacement); + } + else if (value - at->first == at->second - 1) { + // value is the last in this span, trim the upper end + etype replacement(at->first, at->second - 1); + Spans.insert(Spans.erase(at), replacement); + } + else { + // value is in the moddle of the span, so we must split it. + etype firstReplacement(at->first, value - at->first); + etype secondReplacement(value + 1, at->second - ((value + 1) - at->first)); + // Because erase returns the iterator after the erased element, and insert returns the + // iterator for the inserted item, we want to insert secondReplacement first. + Spans.insert(Spans.insert(Spans.erase(at), secondReplacement), firstReplacement); + } +} +bool Base::UniqueNameManager::PiecewiseSparseIntegerSet::Contains(uint value) const { -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(); + iterator at = Spans.lower_bound(etype(value, 1)); + return at != Spans.end() && at->first <= value; } -std::string Base::Tools::addNumber(const std::string& name, unsigned int num, int d) +std::tuple Base::UniqueNameManager::decomposeName(const std::string& name, + std::string& baseNameOut, + std::string& nameSuffixOut) const { - std::stringstream str; - str << name; - if (d > 0) { - str.fill('0'); - str.width(d); + auto suffixStart = std::make_reverse_iterator(GetNameSuffixStartPosition(name)); + nameSuffixOut = name.substr(name.crend() - suffixStart); + auto digitsStart = std::find_if_not(suffixStart, name.crend(), [](char c) { + return std::isdigit(c); + }); + baseNameOut = name.substr(0, name.crend() - digitsStart); + uint digitCount = digitsStart - suffixStart; + if (digitCount == 0) { + // No digits in name + return std::tuple {0, 0}; } - str << num; - return str.str(); + else { + return std::tuple { + digitCount, + std::stoul(name.substr(name.crend() - digitsStart, digitCount))}; + } +} +void Base::UniqueNameManager::addExactName(const std::string& name) +{ + std::string baseName; + std::string nameSuffix; + uint digitCount; + uint digitsValue; + std::tie(digitCount, digitsValue) = decomposeName(name, baseName, nameSuffix); + baseName += nameSuffix; + auto baseNameEntry = UniqueSeeds.find(baseName); + if (baseNameEntry == UniqueSeeds.end()) { + // First use of baseName + baseNameEntry = + UniqueSeeds.emplace(baseName, std::vector()).first; + } + if (digitCount >= baseNameEntry->second.size()) { + // First use of this digitCount + baseNameEntry->second.resize(digitCount + 1); + } + PiecewiseSparseIntegerSet& baseNameAndDigitCountEntry = baseNameEntry->second[digitCount]; + // Name should not already be there + assert(!baseNameAndDigitCountEntry.Contains(digitsValue)); + baseNameAndDigitCountEntry.Add(digitsValue); +} +std::string Base::UniqueNameManager::makeUniqueName(const std::string& modelName, + int minDigits) const +{ + std::string namePrefix; + std::string nameSuffix; + decomposeName(modelName, namePrefix, nameSuffix); + std::string baseName = namePrefix + nameSuffix; + auto baseNameEntry = UniqueSeeds.find(baseName); + if (baseNameEntry == UniqueSeeds.end()) { + // First use of baseName, just return it with no unique digits + return baseName; + } + // We don't care about the digit count of the suggested name, we always use at least the most + // digits ever used before. + int digitCount = baseNameEntry->second.size() - 1; + uint digitsValue; + if (digitCount < minDigits) { + // Caller is asking for more digits than we have in any registered name. + // We start the longer digit string at 000...0001 even though we might have shorter strings + // with larger numeric values. + digitCount = minDigits; + digitsValue = 1; + } + else { + digitsValue = baseNameEntry->second[digitCount].Next(); + } + std::string digits = std::to_string(digitsValue); + if (digitCount > digits.size()) { + namePrefix += std::string(digitCount - digits.size(), '0'); + } + return namePrefix + digits + nameSuffix; } +void Base::UniqueNameManager::removeExactName(const std::string& name) +{ + std::string baseName; + std::string nameSuffix; + uint digitCount; + uint digitsValue; + std::tie(digitCount, digitsValue) = decomposeName(name, baseName, nameSuffix); + baseName += nameSuffix; + auto baseNameEntry = UniqueSeeds.find(baseName); + if (baseNameEntry == UniqueSeeds.end()) { + // name must not be registered, so nothing to do. + return; + } + auto& digitValueSets = baseNameEntry->second; + if (digitCount >= digitValueSets.size()) { + // First use of this digitCount, name must not be registered, so nothing to do. + return; + } + digitValueSets[digitCount].Remove(digitsValue); + // an element of digitValueSets may now be newly empty and so may other elements below it + // Prune off all such trailing empty entries. + auto lastNonemptyEntry = + std::find_if(digitValueSets.crbegin(), digitValueSets.crend(), [](auto& it) { + return it.Any(); + }); + if (lastNonemptyEntry == digitValueSets.crend()) { + // All entries are empty, so the entire baseName can be forgotten. + UniqueSeeds.erase(baseName); + } + else { + digitValueSets.resize(digitValueSets.crend() - lastNonemptyEntry); + } +} + +bool Base::UniqueNameManager::containsName(const std::string& name) const +{ + std::string baseName; + std::string nameSuffix; + uint digitCount; + uint digitsValue; + std::tie(digitCount, digitsValue) = decomposeName(name, baseName, nameSuffix); + baseName += nameSuffix; + auto baseNameEntry = UniqueSeeds.find(baseName); + if (baseNameEntry == UniqueSeeds.end()) { + // base name is not registered + return false; + } + if (digitCount >= baseNameEntry->second.size()) { + // First use of this digitCount, name must not be registered, so not in collection + return false; + } + return baseNameEntry->second[digitCount].Contains(digitsValue); +} 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 baf61e872e..ca76945e8a 100644 --- a/src/Base/Tools.h +++ b/src/Base/Tools.h @@ -33,6 +33,7 @@ #include #include #include +#include #include #include @@ -264,11 +265,100 @@ public: // ---------------------------------------------------------------------------- + +class BaseExport UniqueNameManager +{ +protected: + // This method returns the position of the start of the suffix (or name.cend() if no + // suffix). It must return the same suffix lentgh (name.size() - returnValue) for both + // unique names (one containing digits) and the corresponding base name (with no digits). + virtual std::string::const_iterator GetNameSuffixStartPosition(const std::string& name) const + { + return name.cend(); + } + +private: + class PiecewiseSparseIntegerSet + { + public: + PiecewiseSparseIntegerSet() + {} + + private: + // Each pair being represents the span of integers from lowest to + // (lowest+count-1) inclusive + using etype = std::pair; + // This span comparer class is analogous to std::less and treats overlapping spans as being + // neither greater nor less than each other + class comparer + { + public: + bool operator()(const etype& lhs, const etype& rhs) const + { + // The equality case here is when lhs is below and directly adjacent to rhs. + return rhs.first - lhs.first >= lhs.second; + } + }; + // Spans is the set of spans. Adjacent spans are coalesced so there are always gaps between + // the entries. + std::set Spans; + using iterator = typename std::set::iterator; + using const_iterator = typename std::set::const_iterator; + + public: + void Add(uint value); + void Remove(uint value); + bool Contains(uint value) const; + bool Any() const + { + return Spans.size() != 0; + } + void Clear() + { + Spans.clear(); + } + uint Next() const + { + if (Spans.size() == 0) { + return 0; + } + iterator last = Spans.end(); + --last; + return last->first + last->second; + } + }; + // Keyed as UniqueSeeds[baseName][digitCount][digitValue] iff that seed is taken. + // We need the double-indexing so that Name01 and Name001 can both be indexed, although we only + // ever allocate off the longest for each name i.e. UniqueSeeds[baseName].size()-1 digits. + std::map> UniqueSeeds; + +public: + std::tuple decomposeName(const std::string& name, + std::string& baseNameOut, + std::string& nameSuffixOut) const; + + UniqueNameManager() + {} + + // Register a name in the collection. It is an error (detected only by assertions) to register a + // name more than once. The effect if undetected is that the second registration will have no + // effect + void addExactName(const std::string& name); + std::string makeUniqueName(const std::string& modelName, int minDigits = 0) const; + + // Remove a registered name so it can be generated again. + // Nothing happens if you try to remove a non-registered name. + void removeExactName(const std::string& name); + + bool containsName(const std::string& name) const; + + void clear() + { + UniqueSeeds.clear(); + } +}; 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); diff --git a/src/Base/Writer.cpp b/src/Base/Writer.cpp index d80af5a743..17aa16d507 100644 --- a/src/Base/Writer.cpp +++ b/src/Base/Writer.cpp @@ -247,56 +247,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 5ca61be9df..0b59815814 100644 --- a/src/Base/Writer.h +++ b/src/Base/Writer.h @@ -39,6 +39,8 @@ #include #include +#include + #include "FileInfo.h" @@ -56,6 +58,24 @@ class Persistence; */ class BaseExport Writer { +private: + // This overrides UniqueNameManager's suffix-locating function so thet the last '.' and + // everything after it is considered suffix. + class UniqueFileNameManager: public UniqueNameManager + { + protected: + virtual std::string::const_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.end() - (fi.fileName().size() - fi.fileNamePure().size()); + } + }; public: Writer(); @@ -84,8 +104,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 @@ -151,14 +169,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; diff --git a/src/Gui/Document.cpp b/src/Gui/Document.cpp index d75604d5b0..e7df35552d 100644 --- a/src/Gui/Document.cpp +++ b/src/Gui/Document.cpp @@ -1938,7 +1938,7 @@ 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); } diff --git a/src/Gui/ViewProviderLink.cpp b/src/Gui/ViewProviderLink.cpp index a2cad7a67f..39447e29be 100644 --- a/src/Gui/ViewProviderLink.cpp +++ b/src/Gui/ViewProviderLink.cpp @@ -3408,7 +3408,16 @@ void ViewProviderLink::getPropertyMap(std::map &Map) } } -void ViewProviderLink::getPropertyList(std::vector &List) const { +void ViewProviderLink::visitProperties(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 fc296f2561..67c355991c 100644 --- a/src/Gui/ViewProviderLink.h +++ b/src/Gui/ViewProviderLink.h @@ -265,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(std::function visitor) const override; + void getPropertyList(std::vector& List) const override; ViewProviderDocumentObject *getLinkedViewProvider( std::string *subname=nullptr, bool recursive=false) const override; diff --git a/src/Mod/Spreadsheet/App/Sheet.cpp b/src/Mod/Spreadsheet/App/Sheet.cpp index 0171fd1eee..71d32b42e5 100644 --- a/src/Mod/Spreadsheet/App/Sheet.cpp +++ b/src/Mod/Spreadsheet/App/Sheet.cpp @@ -872,6 +872,17 @@ void Sheet::getPropertyNamedList(std::vector>& } } +void Sheet::visitProperties(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 { @@ -1134,7 +1145,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..217f4f7efe 100644 --- a/src/Mod/Spreadsheet/App/Sheet.h +++ b/src/Mod/Spreadsheet/App/Sheet.h @@ -189,6 +189,9 @@ public: void getPropertyNamedList(std::vector>& List) const override; + /// See PropertyContainer::visitProperties for semantics + void visitProperties(std::function visitor) const override; + short mustExecute() const override; App::DocumentObjectExecReturn* execute() override; diff --git a/tests/src/Base/Tools.cpp b/tests/src/Base/Tools.cpp index f2385433d3..fa219b31ee 100644 --- a/tests/src/Base/Tools.cpp +++ b/tests/src/Base/Tools.cpp @@ -5,42 +5,58 @@ // NOLINTBEGIN(cppcoreguidelines-*,readability-*) TEST(BaseToolsSuite, TestUniqueName1) { - EXPECT_EQ(Base::Tools::getUniqueName("Body", {}), "Body"); + EXPECT_EQ(Base::UniqueNameManager().makeUniqueName("Body"), "Body"); } TEST(BaseToolsSuite, TestUniqueName2) { - EXPECT_EQ(Base::Tools::getUniqueName("Body", {"Body"}, 1), "Body1"); + Base::UniqueNameManager manager; + manager.addExactName("Body"); + EXPECT_EQ(manager.makeUniqueName("Body", 1), "Body1"); } TEST(BaseToolsSuite, TestUniqueName3) { - EXPECT_EQ(Base::Tools::getUniqueName("Body", {"Body"}, 3), "Body001"); + Base::UniqueNameManager manager; + manager.addExactName("Body"); + EXPECT_EQ(manager.makeUniqueName("Body", 3), "Body001"); } TEST(BaseToolsSuite, TestUniqueName4) { - EXPECT_EQ(Base::Tools::getUniqueName("Body", {"Body001"}, 3), "Body002"); + Base::UniqueNameManager manager; + manager.addExactName("Body001"); + EXPECT_EQ(manager.makeUniqueName("Body", 3), "Body002"); } TEST(BaseToolsSuite, TestUniqueName5) { - EXPECT_EQ(Base::Tools::getUniqueName("Body", {"Body", "Body001"}, 3), "Body002"); + Base::UniqueNameManager manager; + manager.addExactName("Body"); + manager.addExactName("Body001"); + EXPECT_EQ(manager.makeUniqueName("Body", 3), "Body002"); } TEST(BaseToolsSuite, TestUniqueName6) { - EXPECT_EQ(Base::Tools::getUniqueName("Body001", {"Body", "Body001"}, 3), "Body002"); + Base::UniqueNameManager manager; + manager.addExactName("Body"); + manager.addExactName("Body001"); + EXPECT_EQ(manager.makeUniqueName("Body001", 3), "Body002"); } TEST(BaseToolsSuite, TestUniqueName7) { - EXPECT_EQ(Base::Tools::getUniqueName("Body001", {"Body"}, 3), "Body002"); + Base::UniqueNameManager manager; + manager.addExactName("Body"); + EXPECT_EQ(manager.makeUniqueName("Body001", 3), "Body001"); } TEST(BaseToolsSuite, TestUniqueName8) { - EXPECT_EQ(Base::Tools::getUniqueName("Body12345", {"Body"}, 3), "Body12346"); + Base::UniqueNameManager manager; + manager.addExactName("Body"); + EXPECT_EQ(manager.makeUniqueName("Body12345", 3), "Body001"); } TEST(Tools, TestIota)