From 01d8d26bb4b3c2f8da6717159963a05007149301 Mon Sep 17 00:00:00 2001 From: "Zheng, Lei" Date: Mon, 2 Aug 2021 11:34:42 +0800 Subject: [PATCH 1/5] Gui: fix document 'modified' status on view property change --- src/Gui/ViewProviderDocumentObject.cpp | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/src/Gui/ViewProviderDocumentObject.cpp b/src/Gui/ViewProviderDocumentObject.cpp index 0230117675..ec1e08c199 100644 --- a/src/Gui/ViewProviderDocumentObject.cpp +++ b/src/Gui/ViewProviderDocumentObject.cpp @@ -196,12 +196,19 @@ void ViewProviderDocumentObject::onChanged(const App::Property* prop) // this is undesired behaviour. So, if this change marks the document as // modified then it must be be reversed. if (!testStatus(Gui::ViewStatus::TouchDocument)) { - bool mod = false; - if (pcDocument) - mod = pcDocument->isModified(); + // Note: reverting document modified status like that is not + // appropreiate because we can't tell if there is any other + // property being changed due to the change of Visibility here. + // Temporary setting the Visibility property as 'NoModify' is + // the proper way. + Base::ObjectStatusLocker guard( + App::Property::NoModify, &Visibility); + // bool mod = false; + // if (pcDocument) + // mod = pcDocument->isModified(); getObject()->Visibility.setValue(Visibility.getValue()); - if (pcDocument) - pcDocument->setModified(mod); + // if (pcDocument) + // pcDocument->setModified(mod); } else { getObject()->Visibility.setValue(Visibility.getValue()); @@ -215,7 +222,10 @@ void ViewProviderDocumentObject::onChanged(const App::Property* prop) } } - if (pcDocument && !pcDocument->isModified() && testStatus(Gui::ViewStatus::TouchDocument)) { + if (prop && !prop->testStatus(App::Property::NoModify) + && pcDocument + && !pcDocument->isModified() + && testStatus(Gui::ViewStatus::TouchDocument)) { if (prop) FC_LOG(prop->getFullName() << " changed"); pcDocument->setModified(true); From 414ea71a28f793c4982912a364a3d0b71f8e66f4 Mon Sep 17 00:00:00 2001 From: "Zheng, Lei" Date: Mon, 2 Aug 2021 11:36:00 +0800 Subject: [PATCH 2/5] Part: fix unnecessary document 'modified' status --- src/Mod/Part/Gui/ViewProviderExt.cpp | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/Mod/Part/Gui/ViewProviderExt.cpp b/src/Mod/Part/Gui/ViewProviderExt.cpp index daf355e909..64f92b99e9 100644 --- a/src/Mod/Part/Gui/ViewProviderExt.cpp +++ b/src/Mod/Part/Gui/ViewProviderExt.cpp @@ -104,6 +104,7 @@ #include #include #include +#include #include #include @@ -396,6 +397,12 @@ void ViewProviderPartExt::onChanged(const App::Property* prop) // if the object was invisible and has been changed, recreate the visual if (prop == &Visibility && (isUpdateForced() || Visibility.getValue()) && VisualTouched) { updateVisual(); + // updateVisual() may not be triggered by any change (e.g. + // triggered by an external object through forceUpdate()). And + // since DiffuseColor is not changed here either, do not falsly set + // the document modified status + Base::ObjectStatusLocker guard( + App::Property::NoModify, &DiffuseColor); // The material has to be checked again (#0001736) onChanged(&DiffuseColor); } @@ -1292,8 +1299,8 @@ void ViewProviderPartExt::updateVisual() void ViewProviderPartExt::forceUpdate(bool enable) { if(enable) { if(++forceUpdateCount == 1) { - if(!isShow()) - Visibility.touch(); + if(!isShow() && VisualTouched) + updateVisual(); } }else if(forceUpdateCount) --forceUpdateCount; From fc9d3547ad3ae30236cb847acf4ef89aa1216683 Mon Sep 17 00:00:00 2001 From: "Zheng, Lei" Date: Mon, 2 Aug 2021 11:38:01 +0800 Subject: [PATCH 3/5] App: fix external document loading The problem happens when partial loading is enabled. If document A contains a link to some object in document B, it will load B as partial document with only that object and its necessary dependencies. But if document A contains another link to some object in document C which also has a link to some object in document B, the link in document C may not be restored, because document B is partially loaded without the linked object. This patch will check for this case and reload document B for more objects. See an example reported in https://forum.freecadweb.org/viewtopic.php?p=495078#p495078 --- src/App/Application.cpp | 345 +++++++++++++++++++++++-------------- src/App/Application.h | 26 ++- src/App/Document.cpp | 20 ++- src/App/Document.h | 7 +- src/App/DocumentObserver.h | 8 + src/App/PropertyLinks.cpp | 69 ++++++-- src/Gui/Document.cpp | 2 +- 7 files changed, 311 insertions(+), 166 deletions(-) diff --git a/src/App/Application.cpp b/src/App/Application.cpp index fb2755571f..97c3bce6b5 100644 --- a/src/App/Application.cpp +++ b/src/App/Application.cpp @@ -55,6 +55,8 @@ #include #endif +#include + #include "Application.h" #include "Document.h" @@ -99,6 +101,7 @@ #include "Document.h" #include "DocumentObjectGroup.h" #include "DocumentObjectFileIncluded.h" +#include "DocumentObserver.h" #include "InventorObject.h" #include "VRMLObject.h" #include "Annotation.h" @@ -487,6 +490,7 @@ bool Application::closeDocument(const char* name) setActiveDocument((Document*)0); std::unique_ptr delDoc (pos->second); DocMap.erase( pos ); + DocFileMap.erase(FileInfo(delDoc->FileName.getValue()).filePath()); _objCount = -1; @@ -566,8 +570,10 @@ int Application::addPendingDocument(const char *FileName, const char *objName, b return -1; assert(FileName && FileName[0]); assert(objName && objName[0]); - auto ret = _pendingDocMap.emplace(FileName,std::set()); - ret.first->second.emplace(objName); + if(!_docReloadAttempts[FileName].emplace(objName).second) + return -1; + auto ret = _pendingDocMap.emplace(FileName,std::vector()); + ret.first->second.push_back(objName); if(ret.second) { _pendingDocs.push_back(ret.first->first.c_str()); return 1; @@ -623,6 +629,41 @@ Document* Application::openDocument(const char * FileName, bool createView) { return 0; } +Document *Application::getDocumentByPath(const char *path, int checkCanonical) const { + if(!path || !path[0]) + return nullptr; + if(DocFileMap.empty()) { + for(auto &v : DocMap) { + const auto &file = v.second->FileName.getStrValue(); + if(file.size()) + DocFileMap[FileInfo(file.c_str()).filePath()] = v.second; + } + } + auto it = DocFileMap.find(FileInfo(path).filePath()); + if(it != DocFileMap.end()) + return it->second; + + if (!checkCanonical) + return nullptr; + + std::string filepath = FileInfo(path).filePath(); + QString canonicalPath = QFileInfo(QString::fromUtf8(path)).canonicalFilePath(); + for (auto &v : DocMap) { + QFileInfo fi(QString::fromUtf8(v.second->FileName.getValue())); + if (canonicalPath == fi.canonicalFilePath()) { + if (checkCanonical == 1) + return v.second; + bool samePath = (canonicalPath == QString::fromUtf8(filepath.c_str())); + FC_WARN("Identical physical path '" << canonicalPath.toUtf8().constData() << "'\n" + << (samePath?"":" for file '") << (samePath?"":filepath.c_str()) << (samePath?"":"'\n") + << " with existing document '" << v.second->Label.getValue() + << "' in path: '" << v.second->FileName.getValue() << "'"); + break; + } + } + return nullptr; +} + std::vector Application::openDocuments(const std::vector &filenames, const std::vector *paths, const std::vector *labels, @@ -640,6 +681,7 @@ std::vector Application::openDocuments(const std::vector _pendingDocs.clear(); _pendingDocsReopen.clear(); _pendingDocMap.clear(); + _docReloadAttempts.clear(); signalStartOpenDocument(); @@ -649,118 +691,162 @@ std::vector Application::openDocuments(const std::vector for (auto &name : filenames) _pendingDocs.push_back(name.c_str()); - std::map newDocs; + std::map timings; FC_TIME_INIT(t); - for (std::size_t count=0;; ++count) { - const char *name = _pendingDocs.front(); - _pendingDocs.pop_front(); - bool isMainDoc = count < filenames.size(); + std::vector openedDocs; - try { - _objCount = -1; - std::set objNames; - if (_allowPartial) { - auto it = _pendingDocMap.find(name); - if (it != _pendingDocMap.end()) - objNames.swap(it->second); - } + int pass = 0; + do { + std::set newDocs; + for (std::size_t count=0;; ++count) { + std::string name = std::move(_pendingDocs.front()); + _pendingDocs.pop_front(); + bool isMainDoc = (pass == 0 && count < filenames.size()); - FC_TIME_INIT(t1); - DocTiming timing; + try { + _objCount = -1; + std::vector objNames; + if (_allowPartial) { + auto it = _pendingDocMap.find(name); + if (it != _pendingDocMap.end()) { + if(isMainDoc) + it->second.clear(); + else + objNames.swap(it->second); + _pendingDocMap.erase(it); + } + } - const char *path = name; - const char *label = 0; - if (isMainDoc) { - if (paths && paths->size()>count) - path = (*paths)[count].c_str(); + FC_TIME_INIT(t1); + DocTiming timing; - if (labels && labels->size()>count) - label = (*labels)[count].c_str(); - } + const char *path = name.c_str(); + const char *label = 0; + if (isMainDoc) { + if (paths && paths->size()>count) + path = (*paths)[count].c_str(); - auto doc = openDocumentPrivate(path, name, label, isMainDoc, createView, objNames); - FC_DURATION_PLUS(timing.d1,t1); - if (doc) - newDocs.emplace(doc,timing); + if (labels && labels->size()>count) + label = (*labels)[count].c_str(); + } + + auto doc = openDocumentPrivate(path, name.c_str(), label, isMainDoc, createView, std::move(objNames)); + FC_DURATION_PLUS(timing.d1,t1); + if (doc) { + timings[doc].d1 += timing.d1; + newDocs.emplace(doc); + } - if (isMainDoc) - res[count] = doc; - _objCount = -1; - } - catch (const Base::Exception &e) { - if (!errs && isMainDoc) - throw; - if (errs && isMainDoc) - (*errs)[count] = e.what(); - else - Console().Error("Exception opening file: %s [%s]\n", name, e.what()); - } - catch (const std::exception &e) { - if (!errs && isMainDoc) - throw; - if (errs && isMainDoc) - (*errs)[count] = e.what(); - else - Console().Error("Exception opening file: %s [%s]\n", name, e.what()); - } - catch (...) { - if (errs) { if (isMainDoc) - (*errs)[count] = "unknown error"; + res[count] = doc; + _objCount = -1; } - else { - _pendingDocs.clear(); + catch (const Base::Exception &e) { + e.ReportException(); + if (!errs && isMainDoc) + throw; + if (errs && isMainDoc) + (*errs)[count] = e.what(); + else + Console().Error("Exception opening file: %s [%s]\n", name.c_str(), e.what()); + } + catch (const std::exception &e) { + if (!errs && isMainDoc) + throw; + if (errs && isMainDoc) + (*errs)[count] = e.what(); + else + Console().Error("Exception opening file: %s [%s]\n", name.c_str(), e.what()); + } + catch (...) { + if (errs) { + if (isMainDoc) + (*errs)[count] = "unknown error"; + } + else { + _pendingDocs.clear(); + _pendingDocsReopen.clear(); + _pendingDocMap.clear(); + throw; + } + } + + if (_pendingDocs.empty()) { + if(_pendingDocsReopen.empty()) + break; + _pendingDocs = std::move(_pendingDocsReopen); _pendingDocsReopen.clear(); - _pendingDocMap.clear(); - throw; + for(auto &file : _pendingDocs) { + auto doc = getDocumentByPath(file.c_str()); + if(doc) + closeDocument(doc->getName()); + } } } - if (_pendingDocs.empty()) { - if (_pendingDocsReopen.empty()) - break; - _allowPartial = false; - _pendingDocs.swap(_pendingDocsReopen); + ++pass; + _pendingDocMap.clear(); + + std::vector docs; + docs.reserve(newDocs.size()); + for(auto &d : newDocs) { + auto doc = d.getDocument(); + if(!doc) + continue; + // Notify PropertyXLink to attach newly opened documents and restore + // relevant external links + PropertyXLink::restoreDocument(*doc); + docs.push_back(doc); } - } - _pendingDocs.clear(); - _pendingDocsReopen.clear(); - _pendingDocMap.clear(); + Base::SequencerLauncher seq("Postprocessing...", docs.size()); - Base::SequencerLauncher seq("Postprocessing...", newDocs.size()); - - std::vector docs; - docs.reserve(newDocs.size()); - for (auto &v : newDocs) { - // Notify PropertyXLink to attach newly opened documents and restore - // relevant external links - PropertyXLink::restoreDocument(*v.first); - docs.push_back(v.first); - } - - // After external links has been restored, we can now sort the document - // according to their dependency order. - docs = Document::getDependentDocuments(docs, true); - for (auto it=docs.begin(); it!=docs.end();) { - Document *doc = *it; - // It is possible that the newly opened document depends on an existing - // document, which will be included with the above call to - // Document::getDependentDocuments(). Make sure to exclude that. - auto dit = newDocs.find(doc); - if (dit == newDocs.end()) { - it = docs.erase(it); - continue; + // After external links has been restored, we can now sort the document + // according to their dependency order. + try { + docs = Document::getDependentDocuments(docs, true); + } catch (Base::Exception &e) { + e.ReportException(); } - ++it; - FC_TIME_INIT(t1); - // Finalize document restoring with the correct order - doc->afterRestore(true); - FC_DURATION_PLUS(dit->second.d2,t1); - seq.next(); - } + for(auto it=docs.begin(); it!=docs.end();) { + auto doc = *it; + + // It is possible that the newly opened document depends on an existing + // document, which will be included with the above call to + // Document::getDependentDocuments(). Make sure to exclude that. + if(!newDocs.count(doc)) { + it = docs.erase(it); + continue; + } + + auto &timing = timings[doc]; + FC_TIME_INIT(t1); + // Finalize document restoring with the correct order + if(doc->afterRestore(true)) { + openedDocs.push_back(doc); + it = docs.erase(it); + } else { + ++it; + // Here means this is a partial loaded document, and we need to + // reload it fully because of touched objects. The reason of + // reloading a partial document with touched object is because + // partial document is supposed to be readonly, while a + // 'touched' object requires recomputation. And an object may + // become touched during restoring if externally linked + // document time stamp mismatches with the stamp saved. + _pendingDocs.push_back(doc->FileName.getValue()); + _pendingDocMap.erase(doc->FileName.getValue()); + } + FC_DURATION_PLUS(timing.d2,t1); + seq.next(); + } + // Close the document for reloading + for(auto doc : docs) + closeDocument(doc->getName()); + + }while(!_pendingDocs.empty()); // Set the active document using the first successfully restored main // document (i.e. documents explicitly asked for by caller). @@ -771,14 +857,14 @@ std::vector Application::openDocuments(const std::vector } } - for (auto doc : docs) { - auto &timing = newDocs[doc]; - FC_DURATION_LOG(timing.d1, doc->getName() << " restore"); - FC_DURATION_LOG(timing.d2, doc->getName() << " postprocess"); + for (auto &doc : openedDocs) { + auto &timing = timings[doc]; + FC_DURATION_LOG(timing.d1, doc.getDocumentName() << " restore"); + FC_DURATION_LOG(timing.d2, doc.getDocumentName() << " postprocess"); } FC_TIME_LOG(t,"total"); - _isRestoring = false; + signalFinishOpenDocument(); return res; } @@ -786,7 +872,7 @@ std::vector Application::openDocuments(const std::vector Document* Application::openDocumentPrivate(const char * FileName, const char *propFileName, const char *label, bool isMainDoc, bool createView, - const std::set &objNames) + std::vector &&objNames) { FileInfo File(FileName); @@ -797,55 +883,51 @@ Document* Application::openDocumentPrivate(const char * FileName, } // Before creating a new document we check whether the document is already open - std::string filepath = File.filePath(); - QString canonicalPath = QFileInfo(QString::fromUtf8(FileName)).canonicalFilePath(); - for (std::map::iterator it = DocMap.begin(); it != DocMap.end(); ++it) { - // get unique path separators - std::string fi = FileInfo(it->second->FileName.getValue()).filePath(); - if (filepath != fi) { - if (canonicalPath == QFileInfo(QString::fromUtf8(fi.c_str())).canonicalFilePath()) { - bool samePath = (canonicalPath == QString::fromUtf8(FileName)); - FC_WARN("Identical physical path '" << canonicalPath.toUtf8().constData() << "'\n" - << (samePath?"":" for file '") << (samePath?"":FileName) << (samePath?"":"'\n") - << " with existing document '" << it->second->Label.getValue() - << "' in path: '" << it->second->FileName.getValue() << "'"); - } - continue; - } - if(it->second->testStatus(App::Document::PartialDoc) - || it->second->testStatus(App::Document::PartialRestore)) { + auto doc = getDocumentByPath(File.filePath().c_str(), 2); + if(doc) { + if(doc->testStatus(App::Document::PartialDoc) + || doc->testStatus(App::Document::PartialRestore)) { // Here means a document is already partially loaded, but the document // is requested again, either partial or not. We must check if the // document contains the required object if(isMainDoc) { // Main document must be open fully, so close and reopen - closeDocument(it->first.c_str()); - break; - } - - if(_allowPartial) { + closeDocument(doc->getName()); + doc = nullptr; + } else if(_allowPartial) { bool reopen = false; for(auto &name : objNames) { - auto obj = it->second->getObject(name.c_str()); + auto obj = doc->getObject(name.c_str()); if(!obj || obj->testStatus(App::PartialObject)) { reopen = true; + // NOTE: We are about to reload this document with + // extra objects. However, it is possible to repeat + // this process several times, if it is linked by + // multiple documents and each with a different set of + // objects. To partially solve this problem, we do not + // close and reopen the document immediately here, but + // add it to _pendingDocsReopen to delay reloading. + for(auto obj : doc->getObjects()) + objNames.push_back(obj->getNameInDocument()); + _pendingDocMap[doc->FileName.getValue()] = std::move(objNames); break; } } if(!reopen) return 0; } - auto &names = _pendingDocMap[FileName]; - names.clear(); - _pendingDocsReopen.push_back(FileName); - return 0; + + if(doc) { + _pendingDocsReopen.emplace_back(FileName); + return 0; + } } if(!isMainDoc) return 0; - - return it->second; + else if(doc) + return doc; } std::string name; @@ -867,6 +949,8 @@ Document* Application::openDocumentPrivate(const char * FileName, try { // read the document newDoc->restore(File.filePath().c_str(),true,objNames); + if(DocFileMap.size()) + DocFileMap[FileInfo(newDoc->FileName.getValue()).filePath()] = newDoc; return newDoc; } // if the project file itself is corrupt then @@ -1463,6 +1547,7 @@ void Application::slotStartSaveDocument(const App::Document& doc, const std::str void Application::slotFinishSaveDocument(const App::Document& doc, const std::string& filename) { + DocFileMap.clear(); this->signalFinishSaveDocument(doc, filename); } diff --git a/src/App/Application.h b/src/App/Application.h index 506c3a364e..5042df74db 100644 --- a/src/App/Application.h +++ b/src/App/Application.h @@ -120,6 +120,16 @@ public: App::Document* getActiveDocument(void) const; /// Retrieve a named document App::Document* getDocument(const char *Name) const; + /** Retrieve a document based on file path + * + * @param path: file path + * @param checkCanonical: if zero, only match absolute file path. If 1, + * then match by canonical file path, where any intermediate '.' and '..' + * and symlinks are resolved. If 2, then only print warning message if + * there is identical canonical file path found, but will not return the + * matched document. + */ + App::Document* getDocumentByPath(const char *path, int checkCanonical=0) const; /// gets the (internal) name of the document const char * getDocumentName(const App::Document* ) const; /// get a list of all documents in the application @@ -190,6 +200,8 @@ public: boost::signals2::signal signalStartRestoreDocument; /// signal on restoring Document boost::signals2::signal signalFinishRestoreDocument; + /// signal on pending reloading of a partial Document + boost::signals2::signal signalPendingReloadDocument; /// signal on starting to save Document boost::signals2::signal signalStartSaveDocument; /// signal on saved Document @@ -441,7 +453,7 @@ protected: /// open single document only App::Document* openDocumentPrivate(const char * FileName, const char *propFileName, - const char *label, bool isMainDoc, bool createView, const std::set &objNames); + const char *label, bool isMainDoc, bool createView, std::vector &&objNames); /// Helper class for App::Document to signal on close/abort transaction class AppExport TransactionSignaller { @@ -559,13 +571,19 @@ private: std::vector _mImportTypes; std::vector _mExportTypes; std::map DocMap; + mutable std::map DocFileMap; std::map mpcPramManager; std::map &_mConfig; App::Document* _pActiveDoc; - std::deque _pendingDocs; - std::deque _pendingDocsReopen; - std::map > _pendingDocMap; + std::deque _pendingDocs; + std::deque _pendingDocsReopen; + std::map > _pendingDocMap; + + // To prevent infinite recursion of reloading a partial document due a truely + // missing object + std::map > _docReloadAttempts; + bool _isRestoring; bool _allowPartial; bool _isClosingAll; diff --git a/src/App/Document.cpp b/src/App/Document.cpp index 1f108b8df3..f6ec4f46ac 100644 --- a/src/App/Document.cpp +++ b/src/App/Document.cpp @@ -1866,7 +1866,6 @@ void Document::exportObjects(const std::vector& obj, std:: #define FC_ELEMENT_OBJECT_DEPS "ObjectDeps" #define FC_ATTR_DEP_COUNT "Count" #define FC_ATTR_DEP_OBJ_NAME "Name" -#define FC_ATTR_DEP_COUNT "Count" #define FC_ATTR_DEP_ALLOW_PARTIAL "AllowPartial" #define FC_ELEMENT_OBJECT_DEP "Dep" @@ -2693,7 +2692,7 @@ bool Document::isAnyRestoring() { // Open the document void Document::restore (const char *filename, - bool delaySignal, const std::set &objNames) + bool delaySignal, const std::vector &objNames) { clearUndos(); d->activeObject = 0; @@ -2752,8 +2751,7 @@ void Document::restore (const char *filename, d->partialLoadObjects.emplace(name,true); try { Document::Restore(reader); - } - catch (const Base::Exception& e) { + } catch (const Base::Exception& e) { Base::Console().Error("Invalid Document.xml: %s\n", e.what()); setStatus(Document::RestoreError, true); } @@ -2777,15 +2775,16 @@ void Document::restore (const char *filename, afterRestore(true); } -void Document::afterRestore(bool checkPartial) { +bool Document::afterRestore(bool checkPartial) { Base::FlagToggler<> flag(_IsRestoring,false); if(!afterRestore(d->objectArray,checkPartial)) { FC_WARN("Reload partial document " << getName()); - restore(); - return; + GetApplication().signalPendingReloadDocument(*this); + return false; } GetApplication().signalFinishRestoreDocument(*this); setStatus(Document::Restoring, false); + return true; } bool Document::afterRestore(const std::vector &objArray, bool checkPartial) @@ -2861,9 +2860,12 @@ bool Document::afterRestore(const std::vector &objArray, bool std::string errMsg; if(link && (res=link->checkRestore(&errMsg))) { d->touchedObjs.insert(obj); - if(res==1) + if(res==1 || checkPartial) { FC_WARN(obj->getFullName() << '.' << prop->getName() << ": " << errMsg); - else { + setStatus(Document::LinkStampChanged, true); + if(checkPartial) + return false; + } else { FC_ERR(obj->getFullName() << '.' << prop->getName() << ": " << errMsg); d->addRecomputeLog(errMsg,obj); setStatus(Document::PartialRestore, true); diff --git a/src/App/Document.h b/src/App/Document.h index a4b2285cdc..16e8053fb8 100644 --- a/src/App/Document.h +++ b/src/App/Document.h @@ -74,7 +74,8 @@ public: PartialDoc = 7, AllowPartialRecompute = 8, // allow recomputing editing object if SkipRecompute is set TempDoc = 9, // Mark as temporary document without prompt for save - RestoreError = 10 + RestoreError = 10, + LinkStampChanged = 11, // Indicates during restore time if any linked document's time stamp has changed }; /** @name Properties */ @@ -195,8 +196,8 @@ public: bool saveCopy(const char* file) const; /// Restore the document from the file in Property Path void restore (const char *filename=0, - bool delaySignal=false, const std::set &objNames={}); - void afterRestore(bool checkPartial=false); + bool delaySignal=false, const std::vector &objNames={}); + bool afterRestore(bool checkPartial=false); bool afterRestore(const std::vector &, bool checkPartial=false); enum ExportStatus { NotExporting, diff --git a/src/App/DocumentObserver.h b/src/App/DocumentObserver.h index c32c780a01..b93373ae2b 100644 --- a/src/App/DocumentObserver.h +++ b/src/App/DocumentObserver.h @@ -62,6 +62,14 @@ public: /*! Assignment operator */ void operator=(const std::string&); + bool operator==(const DocumentT &other) const { + return document == other.document; + } + + bool operator<(const DocumentT &other) const { + return document < other.document; + } + /*! Get a pointer to the document or 0 if it doesn't exist any more. */ Document* getDocument() const; /*! Get the name of the document. */ diff --git a/src/App/PropertyLinks.cpp b/src/App/PropertyLinks.cpp index e78abd3c22..f92879aa4e 100644 --- a/src/App/PropertyLinks.cpp +++ b/src/App/PropertyLinks.cpp @@ -2458,6 +2458,7 @@ class App::DocInfo : public: typedef boost::signals2::scoped_connection Connection; Connection connFinishRestoreDocument; + Connection connPendingReloadDocument; Connection connDeleteDocument; Connection connSaveDocument; Connection connDeletedObject; @@ -2589,6 +2590,7 @@ public: FC_LOG("deinit " << (pcDoc?pcDoc->getName():filePath())); assert(links.empty()); connFinishRestoreDocument.disconnect(); + connPendingReloadDocument.disconnect(); connDeleteDocument.disconnect(); connSaveDocument.disconnect(); connDeletedObject.disconnect(); @@ -2606,6 +2608,8 @@ public: App::Application &app = App::GetApplication(); connFinishRestoreDocument = app.signalFinishRestoreDocument.connect( boost::bind(&DocInfo::slotFinishRestoreDocument,this,bp::_1)); + connPendingReloadDocument = app.signalPendingReloadDocument.connect( + boost::bind(&DocInfo::slotFinishRestoreDocument,this,bp::_1)); connDeleteDocument = app.signalDeleteDocument.connect( boost::bind(&DocInfo::slotDeleteDocument,this,bp::_1)); connSaveDocument = app.signalSaveDocument.connect( @@ -2617,6 +2621,8 @@ public: else{ for(App::Document *doc : App::GetApplication().getDocuments()) { if(getFullPath(doc->getFileName()) == fullpath) { + if(doc->testStatus(App::Document::PartialDoc) && !doc->getObject(objName)) + break; attach(doc); return; } @@ -2642,22 +2648,36 @@ public: continue; } auto obj = doc->getObject(link->objectName.c_str()); - if(!obj) + if(obj) + link->restoreLink(obj); + else if (doc->testStatus(App::Document::PartialDoc)) { + App::GetApplication().addPendingDocument( + doc->FileName.getValue(), + link->objectName.c_str(), + false); + FC_WARN("reloading partial document '" << doc->FileName.getValue() + << "' due to object " << link->objectName); + } else FC_WARN("object '" << link->objectName << "' not found in document '" << doc->getName() << "'"); - else - link->restoreLink(obj); } for(auto &v : parentLinks) { v.first->setFlag(PropertyLinkBase::LinkRestoring); v.first->aboutToSetValue(); for(auto link : v.second) { auto obj = doc->getObject(link->objectName.c_str()); - if(!obj) + if(obj) + link->restoreLink(obj); + else if (doc->testStatus(App::Document::PartialDoc)) { + App::GetApplication().addPendingDocument( + doc->FileName.getValue(), + link->objectName.c_str(), + false); + FC_WARN("reloading partial document '" << doc->FileName.getValue() + << "' due to object " << link->objectName); + } else FC_WARN("object '" << link->objectName << "' not found in document '" << doc->getName() << "'"); - else - link->restoreLink(obj); } v.first->hasSetValue(); v.first->setFlag(PropertyLinkBase::LinkRestoring,false); @@ -2723,16 +2743,17 @@ public: } } - // time stamp changed, touch the linking document. Unfortunately, there - // is no way to setModfied() for an App::Document. We don't want to touch - // all PropertyXLink for a document, because the linked object is - // potentially unchanged. So we just touch at most one. + // time stamp changed, touch the linking document. std::set docs; for(auto link : links) { auto linkdoc = static_cast(link->getContainer())->getDocument(); auto ret = docs.insert(linkdoc); - if(ret.second && !linkdoc->isTouched()) - link->touch(); + if(ret.second) { + // This will signal the Gui::Document to call setModified(); + FC_LOG("touch document " << linkdoc->getName() + << " on time stamp change of " << link->getFullName()); + linkdoc->Comment.touch(); + } } } @@ -3473,7 +3494,12 @@ PropertyXLink::getDocumentOutList(App::Document *doc) { std::map > ret; for(auto &v : _DocInfoMap) { for(auto link : v.second->links) { - if(!v.second->pcDoc) continue; + if(!v.second->pcDoc + || link->getScope() == LinkScope::Hidden + || link->testStatus(Property::PropTransient) + || link->testStatus(Property::Transient) + || link->testStatus(Property::PropNoPersist)) + continue; auto obj = dynamic_cast(link->getContainer()); if(!obj || !obj->getNameInDocument() || !obj->getDocument()) continue; @@ -3493,6 +3519,11 @@ PropertyXLink::getDocumentInList(App::Document *doc) { continue; auto &docs = ret[v.second->pcDoc]; for(auto link : v.second->links) { + if(link->getScope() == LinkScope::Hidden + || link->testStatus(Property::PropTransient) + || link->testStatus(Property::Transient) + || link->testStatus(Property::PropNoPersist)) + continue; auto obj = dynamic_cast(link->getContainer()); if(obj && obj->getNameInDocument() && obj->getDocument()) docs.insert(obj->getDocument()); @@ -4460,12 +4491,12 @@ void PropertyXLinkContainer::breakLink(App::DocumentObject *obj, bool clear) { } int PropertyXLinkContainer::checkRestore(std::string *msg) const { - if(_LinkRestored) - return 1; - for(auto &v : _XLinks) { - int res = v.second->checkRestore(msg); - if(res) - return res; + if(_LinkRestored) { + for(auto &v : _XLinks) { + int res = v.second->checkRestore(msg); + if(res) + return res; + } } return 0; } diff --git a/src/Gui/Document.cpp b/src/Gui/Document.cpp index 589deecede..813e84a69f 100644 --- a/src/Gui/Document.cpp +++ b/src/Gui/Document.cpp @@ -1499,7 +1499,7 @@ void Document::slotFinishRestoreDocument(const App::Document& doc) } // reset modified flag - setModified(false); + setModified(doc.testStatus(App::Document::LinkStampChanged)); } void Document::slotShowHidden(const App::Document& doc) From 9885dadfff6bc8fe25ba7eb2cb45e4ddaeae1c33 Mon Sep 17 00:00:00 2001 From: "Zheng, Lei" Date: Fri, 6 Aug 2021 14:46:56 +0800 Subject: [PATCH 4/5] App: use enum in API Application::getDocumentByPath() --- src/App/Application.cpp | 8 ++++---- src/App/Application.h | 29 +++++++++++++++++++++++------ 2 files changed, 27 insertions(+), 10 deletions(-) diff --git a/src/App/Application.cpp b/src/App/Application.cpp index 97c3bce6b5..72db851cba 100644 --- a/src/App/Application.cpp +++ b/src/App/Application.cpp @@ -629,7 +629,7 @@ Document* Application::openDocument(const char * FileName, bool createView) { return 0; } -Document *Application::getDocumentByPath(const char *path, int checkCanonical) const { +Document *Application::getDocumentByPath(const char *path, PathMatchMode checkCanonical) const { if(!path || !path[0]) return nullptr; if(DocFileMap.empty()) { @@ -643,7 +643,7 @@ Document *Application::getDocumentByPath(const char *path, int checkCanonical) c if(it != DocFileMap.end()) return it->second; - if (!checkCanonical) + if (checkCanonical == MatchAbsolute) return nullptr; std::string filepath = FileInfo(path).filePath(); @@ -651,7 +651,7 @@ Document *Application::getDocumentByPath(const char *path, int checkCanonical) c for (auto &v : DocMap) { QFileInfo fi(QString::fromUtf8(v.second->FileName.getValue())); if (canonicalPath == fi.canonicalFilePath()) { - if (checkCanonical == 1) + if (checkCanonical == MatchCanonical) return v.second; bool samePath = (canonicalPath == QString::fromUtf8(filepath.c_str())); FC_WARN("Identical physical path '" << canonicalPath.toUtf8().constData() << "'\n" @@ -883,7 +883,7 @@ Document* Application::openDocumentPrivate(const char * FileName, } // Before creating a new document we check whether the document is already open - auto doc = getDocumentByPath(File.filePath().c_str(), 2); + auto doc = getDocumentByPath(File.filePath().c_str(), MatchCanonicalWarning); if(doc) { if(doc->testStatus(App::Document::PartialDoc) || doc->testStatus(App::Document::PartialRestore)) { diff --git a/src/App/Application.h b/src/App/Application.h index 5042df74db..738c0e19d7 100644 --- a/src/App/Application.h +++ b/src/App/Application.h @@ -120,16 +120,33 @@ public: App::Document* getActiveDocument(void) const; /// Retrieve a named document App::Document* getDocument(const char *Name) const; + + /// Path matching mode for getDocumentByPath() + enum PathMatchMode { + /// Match by resolving to absolute file path + MatchAbsolute = 0, + /** Match by absolute path first. If not found then match by resolving + * to canonical file path where any intermediate '.' '..' and symlinks + * are resolved. + */ + MatchCanonical = 1, + /** Same as MatchCanonical, but if a document is found by canonical + * path match, which means the document can be resolved using two + * different absolute path, a warning is printed and the found document + * is not returned. This is to allow the caller to intentionally load + * the same physical file as separate documents. + */ + MatchCanonicalWarning = 2, + }; /** Retrieve a document based on file path * * @param path: file path - * @param checkCanonical: if zero, only match absolute file path. If 1, - * then match by canonical file path, where any intermediate '.' and '..' - * and symlinks are resolved. If 2, then only print warning message if - * there is identical canonical file path found, but will not return the - * matched document. + * @param checkCanonical: file path matching mode, @sa PathMatchMode. + * @return Return the document found by matching with the given path */ - App::Document* getDocumentByPath(const char *path, int checkCanonical=0) const; + App::Document* getDocumentByPath(const char *path, + PathMatchMode checkCanonical = MatchAbsolute) const; + /// gets the (internal) name of the document const char * getDocumentName(const App::Document* ) const; /// get a list of all documents in the application From a2fb4a5d6d4a0f3365669b448617567b7e87223a Mon Sep 17 00:00:00 2001 From: "Zheng, Lei" Date: Sat, 30 Oct 2021 12:30:45 +0800 Subject: [PATCH 5/5] Minor code change according to suggestions --- src/App/Application.cpp | 18 +++++++++--------- src/App/Application.h | 4 ++-- src/App/Document.h | 2 +- src/Gui/ViewProviderDocumentObject.cpp | 2 +- src/Mod/Part/Gui/ViewProviderExt.cpp | 2 +- 5 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/App/Application.cpp b/src/App/Application.cpp index 72db851cba..d51a6740fc 100644 --- a/src/App/Application.cpp +++ b/src/App/Application.cpp @@ -633,7 +633,7 @@ Document *Application::getDocumentByPath(const char *path, PathMatchMode checkCa if(!path || !path[0]) return nullptr; if(DocFileMap.empty()) { - for(auto &v : DocMap) { + for(const auto &v : DocMap) { const auto &file = v.second->FileName.getStrValue(); if(file.size()) DocFileMap[FileInfo(file.c_str()).filePath()] = v.second; @@ -643,15 +643,15 @@ Document *Application::getDocumentByPath(const char *path, PathMatchMode checkCa if(it != DocFileMap.end()) return it->second; - if (checkCanonical == MatchAbsolute) + if (checkCanonical == PathMatchMode::MatchAbsolute) return nullptr; std::string filepath = FileInfo(path).filePath(); QString canonicalPath = QFileInfo(QString::fromUtf8(path)).canonicalFilePath(); - for (auto &v : DocMap) { + for (const auto &v : DocMap) { QFileInfo fi(QString::fromUtf8(v.second->FileName.getValue())); if (canonicalPath == fi.canonicalFilePath()) { - if (checkCanonical == MatchCanonical) + if (checkCanonical == PathMatchMode::MatchCanonical) return v.second; bool samePath = (canonicalPath == QString::fromUtf8(filepath.c_str())); FC_WARN("Identical physical path '" << canonicalPath.toUtf8().constData() << "'\n" @@ -778,7 +778,7 @@ std::vector Application::openDocuments(const std::vector break; _pendingDocs = std::move(_pendingDocsReopen); _pendingDocsReopen.clear(); - for(auto &file : _pendingDocs) { + for(const auto &file : _pendingDocs) { auto doc = getDocumentByPath(file.c_str()); if(doc) closeDocument(doc->getName()); @@ -791,7 +791,7 @@ std::vector Application::openDocuments(const std::vector std::vector docs; docs.reserve(newDocs.size()); - for(auto &d : newDocs) { + for(const auto &d : newDocs) { auto doc = d.getDocument(); if(!doc) continue; @@ -843,7 +843,7 @@ std::vector Application::openDocuments(const std::vector seq.next(); } // Close the document for reloading - for(auto doc : docs) + for(const auto doc : docs) closeDocument(doc->getName()); }while(!_pendingDocs.empty()); @@ -883,7 +883,7 @@ Document* Application::openDocumentPrivate(const char * FileName, } // Before creating a new document we check whether the document is already open - auto doc = getDocumentByPath(File.filePath().c_str(), MatchCanonicalWarning); + auto doc = getDocumentByPath(File.filePath().c_str(), PathMatchMode::MatchCanonicalWarning); if(doc) { if(doc->testStatus(App::Document::PartialDoc) || doc->testStatus(App::Document::PartialRestore)) { @@ -897,7 +897,7 @@ Document* Application::openDocumentPrivate(const char * FileName, doc = nullptr; } else if(_allowPartial) { bool reopen = false; - for(auto &name : objNames) { + for(const auto &name : objNames) { auto obj = doc->getObject(name.c_str()); if(!obj || obj->testStatus(App::PartialObject)) { reopen = true; diff --git a/src/App/Application.h b/src/App/Application.h index 738c0e19d7..0d38fcfdab 100644 --- a/src/App/Application.h +++ b/src/App/Application.h @@ -122,7 +122,7 @@ public: App::Document* getDocument(const char *Name) const; /// Path matching mode for getDocumentByPath() - enum PathMatchMode { + enum class PathMatchMode { /// Match by resolving to absolute file path MatchAbsolute = 0, /** Match by absolute path first. If not found then match by resolving @@ -145,7 +145,7 @@ public: * @return Return the document found by matching with the given path */ App::Document* getDocumentByPath(const char *path, - PathMatchMode checkCanonical = MatchAbsolute) const; + PathMatchMode checkCanonical = PathMatchMode::MatchAbsolute) const; /// gets the (internal) name of the document const char * getDocumentName(const App::Document* ) const; diff --git a/src/App/Document.h b/src/App/Document.h index 16e8053fb8..6d0e7c5ec2 100644 --- a/src/App/Document.h +++ b/src/App/Document.h @@ -75,7 +75,7 @@ public: AllowPartialRecompute = 8, // allow recomputing editing object if SkipRecompute is set TempDoc = 9, // Mark as temporary document without prompt for save RestoreError = 10, - LinkStampChanged = 11, // Indicates during restore time if any linked document's time stamp has changed + LinkStampChanged = 11, // Indicates during restore time if any linked document's time stamp has changed }; /** @name Properties */ diff --git a/src/Gui/ViewProviderDocumentObject.cpp b/src/Gui/ViewProviderDocumentObject.cpp index ec1e08c199..e4cc2bbb2b 100644 --- a/src/Gui/ViewProviderDocumentObject.cpp +++ b/src/Gui/ViewProviderDocumentObject.cpp @@ -197,7 +197,7 @@ void ViewProviderDocumentObject::onChanged(const App::Property* prop) // modified then it must be be reversed. if (!testStatus(Gui::ViewStatus::TouchDocument)) { // Note: reverting document modified status like that is not - // appropreiate because we can't tell if there is any other + // appropriate because we can't tell if there is any other // property being changed due to the change of Visibility here. // Temporary setting the Visibility property as 'NoModify' is // the proper way. diff --git a/src/Mod/Part/Gui/ViewProviderExt.cpp b/src/Mod/Part/Gui/ViewProviderExt.cpp index 64f92b99e9..9bd7037d1f 100644 --- a/src/Mod/Part/Gui/ViewProviderExt.cpp +++ b/src/Mod/Part/Gui/ViewProviderExt.cpp @@ -399,7 +399,7 @@ void ViewProviderPartExt::onChanged(const App::Property* prop) updateVisual(); // updateVisual() may not be triggered by any change (e.g. // triggered by an external object through forceUpdate()). And - // since DiffuseColor is not changed here either, do not falsly set + // since DiffuseColor is not changed here either, do not falsely set // the document modified status Base::ObjectStatusLocker guard( App::Property::NoModify, &DiffuseColor);