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)