From c3178343dbb896994d7fdf6a13c4eaf40f8509ec Mon Sep 17 00:00:00 2001 From: Zheng Lei Date: Mon, 21 Feb 2022 19:29:01 +0800 Subject: [PATCH] App: fix property ordering problem when undo/redo (#3255) * Part: fix Placement/Shape onChanged() handling * App: fix property ordering problem when undo/redo See https://tracker.freecadweb.org/view.php?id=4265#c14271 * Gui: fix undo/redo signaling Make sure to signal after all properties has been restored --- src/App/Document.cpp | 69 +++++-------- src/App/Document.h | 2 + src/App/DocumentObject.cpp | 4 + src/App/DocumentObject.h | 1 + src/App/Property.cpp | 22 ++--- src/App/PropertyContainer.h | 10 ++ src/App/Transactions.cpp | 130 +++++++++++++++++++++++++ src/App/Transactions.h | 19 ++++ src/Gui/Document.cpp | 31 +++--- src/Gui/ViewProviderDocumentObject.cpp | 4 + src/Gui/ViewProviderDocumentObject.h | 2 + src/Mod/Part/App/PartFeature.cpp | 15 ++- src/Mod/Part/Gui/ViewProviderExt.cpp | 7 ++ src/Mod/Part/Gui/ViewProviderExt.h | 2 + 14 files changed, 250 insertions(+), 68 deletions(-) diff --git a/src/App/Document.cpp b/src/App/Document.cpp index 2440a03452..a3575cbef3 100644 --- a/src/App/Document.cpp +++ b/src/App/Document.cpp @@ -957,6 +957,7 @@ bool Document::undo(int id) if(it == mUndoMap.end()) return false; if(it->second != d->activeUndoTransaction) { + TransactionGuard guard(true); while(mUndoTransactions.size() && mUndoTransactions.back()!=it->second) undo(0); } @@ -966,11 +967,13 @@ bool Document::undo(int id) _commitTransaction(true); if (mUndoTransactions.empty()) return false; + + TransactionGuard guard(true); + // redo d->activeUndoTransaction = new Transaction(mUndoTransactions.back()->getID()); d->activeUndoTransaction->Name = mUndoTransactions.back()->Name; - { Base::FlagToggler flag(d->undoing); // applying the undo mUndoTransactions.back()->apply(*this,false); @@ -983,18 +986,6 @@ bool Document::undo(int id) mUndoMap.erase(mUndoTransactions.back()->getID()); delete mUndoTransactions.back(); mUndoTransactions.pop_back(); - - } - - for(auto & obj:d->objectArray) { - if(obj->testStatus(ObjectStatus::PendingTransactionUpdate)) { - obj->onUndoRedoFinished(); - obj->setStatus(ObjectStatus::PendingTransactionUpdate,false); - } - } - - signalUndo(*this); // now signal the undo - return true; } @@ -1008,8 +999,11 @@ bool Document::redo(int id) auto it = mRedoMap.find(id); if(it == mRedoMap.end()) return false; - while(mRedoTransactions.size() && mRedoTransactions.back()!=it->second) - redo(0); + { + TransactionGuard guard(false); + while(mRedoTransactions.size() && mRedoTransactions.back()!=it->second) + redo(0); + } } if (d->activeUndoTransaction) @@ -1017,12 +1011,13 @@ bool Document::redo(int id) assert(mRedoTransactions.size()!=0); + TransactionGuard guard(false); + // undo d->activeUndoTransaction = new Transaction(mRedoTransactions.back()->getID()); d->activeUndoTransaction->Name = mRedoTransactions.back()->Name; // do the redo - { Base::FlagToggler flag(d->undoing); mRedoTransactions.back()->apply(*this,true); @@ -1033,16 +1028,6 @@ bool Document::redo(int id) mRedoMap.erase(mRedoTransactions.back()->getID()); delete mRedoTransactions.back(); mRedoTransactions.pop_back(); - } - - for(auto & obj:d->objectArray) { - if(obj->testStatus(ObjectStatus::PendingTransactionUpdate)) { - obj->onUndoRedoFinished(); - obj->setStatus(ObjectStatus::PendingTransactionUpdate,false); - } - } - - signalRedo(*this); return true; } @@ -1067,7 +1052,7 @@ void Document::addOrRemovePropertyOfObject(TransactionalObject* obj, Property *p bool Document::isPerformingTransaction() const { - return d->undoing || d->rollback; + return d->undoing || d->rollback || Transaction::isApplying(); } std::vector Document::getAvailableUndoNames() const @@ -1222,17 +1207,12 @@ void Document::commitTransaction() { void Document::_commitTransaction(bool notify) { - if (isPerformingTransaction()) { - if (FC_LOG_INSTANCE.isEnabled(FC_LOGLEVEL_LOG)) - FC_WARN("Cannot commit transaction while transacting"); - return; - } - else if (d->committing) { - // for a recursive call return without printing a warning - return; - } - if (d->activeUndoTransaction) { + if(d->undoing || d->rollback || d->committing) { + if (FC_LOG_INSTANCE.isEnabled(FC_LOGLEVEL_LOG)) + FC_WARN("Cannot commit transaction while transacting"); + return; + } Base::FlagToggler<> flag(d->committing); Application::TransactionSignaller signaller(false,true); int id = d->activeUndoTransaction->getID(); @@ -1264,12 +1244,13 @@ void Document::abortTransaction() { void Document::_abortTransaction() { - if(isPerformingTransaction() || d->committing) { - if (FC_LOG_INSTANCE.isEnabled(FC_LOGLEVEL_LOG)) - FC_WARN("Cannot abort transaction while transacting"); - } - if (d->activeUndoTransaction) { + if(d->undoing || d->rollback || d->committing) { + if (FC_LOG_INSTANCE.isEnabled(FC_LOGLEVEL_LOG)) + FC_WARN("Cannot abort transaction while transacting"); + return; + } + Base::FlagToggler flag(d->rollback); Application::TransactionSignaller signaller(true,true); @@ -2937,6 +2918,10 @@ std::string Document::getFullName() const { return myName; } +App::Document *Document::getOwnerDocument() const { + return const_cast(this); +} + const char* Document::getProgramVersion() const { return d->programVersion.c_str(); diff --git a/src/App/Document.h b/src/App/Document.h index 9ca8bc97e8..701b5992fb 100644 --- a/src/App/Document.h +++ b/src/App/Document.h @@ -503,6 +503,8 @@ public: virtual std::string getFullName() const override; + virtual App::Document *getOwnerDocument() const override; + /// Indicate if there is any document restoring/importing static bool isAnyRestoring(); diff --git a/src/App/DocumentObject.cpp b/src/App/DocumentObject.cpp index f6c09bc764..a4ee1f5af6 100644 --- a/src/App/DocumentObject.cpp +++ b/src/App/DocumentObject.cpp @@ -267,6 +267,10 @@ std::string DocumentObject::getFullName() const { return name; } +App::Document *DocumentObject::getOwnerDocument() const { + return _pDoc; +} + const char *DocumentObject::getNameInDocument() const { // Note: It can happen that we query the internal name of an object even if it is not diff --git a/src/App/DocumentObject.h b/src/App/DocumentObject.h index b747d73eca..5a35a3263c 100644 --- a/src/App/DocumentObject.h +++ b/src/App/DocumentObject.h @@ -139,6 +139,7 @@ public: std::string getExportName(bool forced=false) const; /// Return the object full name of the form DocName#ObjName virtual std::string getFullName() const override; + virtual App::Document *getOwnerDocument() const override; virtual bool isAttachedToDocument() const override; virtual const char* detachFromDocument() override; /// gets the document in which this Object is handled diff --git a/src/App/Property.cpp b/src/App/Property.cpp index 2e7fafe282..7e05364964 100644 --- a/src/App/Property.cpp +++ b/src/App/Property.cpp @@ -31,6 +31,7 @@ #include "Property.h" #include "ObjectIdentifier.h" #include "PropertyContainer.h" +#include "Transactions.h" #include #include #include "Application.h" @@ -58,7 +59,7 @@ Property::Property() Property::~Property() { - + Transaction::removePendingProperty(this); } const char* Property::getName() const @@ -212,9 +213,14 @@ void Property::destroy(Property *p) { void Property::touch() { PropertyCleaner guard(this); - if (father) - father->onChanged(this); StatusBits.set(Touched); + if (getName() && father && !Transaction::isApplying(this)) { + father->onChanged(this); + if(!testStatus(Busy)) { + Base::BitsetLocker guard(StatusBits,Busy); + signalChanged(*this); + } + } } void Property::setReadOnly(bool readOnly) @@ -224,15 +230,7 @@ void Property::setReadOnly(bool readOnly) void Property::hasSetValue(void) { - PropertyCleaner guard(this); - if (father) { - father->onChanged(this); - if(!testStatus(Busy)) { - Base::BitsetLocker guard(StatusBits,Busy); - signalChanged(*this); - } - } - StatusBits.set(Touched); + touch(); } void Property::aboutToSetValue(void) diff --git a/src/App/PropertyContainer.h b/src/App/PropertyContainer.h index 92458b8d5a..991cc5bd41 100644 --- a/src/App/PropertyContainer.h +++ b/src/App/PropertyContainer.h @@ -42,6 +42,7 @@ class Property; class PropertyContainer; class DocumentObject; class Extension; +class Document; enum PropertyType { @@ -161,6 +162,10 @@ public: virtual std::string getFullName() const {return std::string();} + /// Return owner document of this container + virtual App::Document *getOwnerDocument() const + {return nullptr;} + /// find a property by its name virtual Property *getPropertyByName(const char* name) const; /// get the name of a property @@ -231,6 +236,7 @@ public: } friend class Property; + friend class TransactionObject; friend class DynamicProperty; @@ -247,6 +253,10 @@ protected: virtual void handleChangedPropertyName(Base::XMLReader &reader, const char * TypeName, const char *PropName); virtual void handleChangedPropertyType(Base::XMLReader &reader, const char * TypeName, Property * prop); + virtual bool isOnChangeBlocked() const { + return false; + } + private: // forbidden PropertyContainer(const PropertyContainer&); diff --git a/src/App/Transactions.cpp b/src/App/Transactions.cpp index 57359c5bee..c64a231fee 100644 --- a/src/App/Transactions.cpp +++ b/src/App/Transactions.cpp @@ -36,8 +36,10 @@ using Base::Writer; #include using Base::XMLReader; #include +#include #include "Transactions.h" #include "Property.h" +#include "Application.h" #include "Document.h" #include "DocumentObject.h" @@ -163,6 +165,133 @@ void Transaction::addOrRemoveProperty(TransactionalObject *Obj, //************************************************************************** // separator for other implementation aspects +static int _TransactionActive; +static bool _FlushingProps; + +// Hold all changed property when transactions are applied. The mapped value is +// index for remembering the order. Although the property change order within +// the same object is not kept, but there is some ordering of changes between +// different objects +static std::unordered_map _PendingProps; +static int _PendingPropIndex; + +TransactionGuard::TransactionGuard(bool undo) + :undo(undo) +{ + if(_FlushingProps) { + FC_ERR("Recursive transaction"); + return; + } + ++_TransactionActive; +} + +TransactionGuard::~TransactionGuard() +{ + if(_FlushingProps) + return; + + if(--_TransactionActive) + return; + + Base::StateLocker locker(_FlushingProps); + + std::vector > props; + props.reserve(_PendingProps.size()); + props.insert(props.end(),_PendingProps.begin(),_PendingProps.end()); + std::sort(props.begin(), props.end(), + [](const std::pair &a, const std::pair &b) { + return a.second < b.second; + }); + + std::vector docs; + std::set docSet; + for(auto &v : props) { + auto container = v.first->getContainer(); + if (!container) continue; + auto doc = container->getOwnerDocument(); + if (doc && docSet.insert(doc).second) + docs.push_back(doc); + } + + std::string errMsg; + for(auto &v : props) { + auto prop = v.first; + // double check if the property exists, because it may be removed + // while we are looping. + if(_PendingProps.count(prop)) { + try { + FC_LOG("transaction touch " << prop->getFullName()); + prop->touch(); + }catch(Base::Exception &e) { + e.ReportException(); + errMsg = e.what(); + }catch(std::exception &e) { + errMsg = e.what(); + }catch(...) { + errMsg = "Unknown exception"; + } + if(errMsg.size()) { + FC_ERR("Exception on finishing transaction " << errMsg); + errMsg.clear(); + } + } + } + _PendingProps.clear(); + _PendingPropIndex = 0; + + try { + if(undo) { + for (auto doc : docs) + doc->signalUndo(*doc); + GetApplication().signalUndo(); + } else { + for (auto doc : docs) + doc->signalRedo(*doc); + GetApplication().signalRedo(); + } + } catch(Base::Exception &e) { + e.ReportException(); + errMsg = e.what(); + } catch(...) { + errMsg = "Unknown exception"; + } + if (errMsg.size()) { + FC_ERR("Exception on " << (undo?"undo: ":"redo: ") << errMsg); + errMsg.clear(); + } +} + + +bool Transaction::isApplying(Property *prop) +{ + if (_TransactionActive) { + if(prop) + _PendingProps.insert(std::make_pair(prop, _PendingPropIndex++)); + return true; + } + + // If we are flushing the property changes, then + // Document::isPerformingTransaction() shall still report true. That's why + // we return true here if prop is not given. + if (!prop) + return _FlushingProps; + + // Property::hasSetValue/touch() also call us (with a given prop) to see if + // it shall notify its container about the change. Now, it is debatable if + // we shall allow further propagation of the change notification, because + // optimally speaking, no recomputation shall be performed while undo/redo. + // We can stop the chain of notification after informing change of the + // given property. This, however, may cause problem for those not + // 'optimally' coded objects. So we didn't do that, but only remove the + // soon to be notified property here. + _PendingProps.erase(prop); + return false; +} + +void Transaction::removePendingProperty(Property *prop) +{ + _PendingProps.erase(prop); +} void Transaction::apply(Document &Doc, bool forward) { @@ -175,6 +304,7 @@ void Transaction::apply(Document &Doc, bool forward) info.second->applyNew(Doc, const_cast(info.first)); for(auto &info : index) info.second->applyChn(Doc, const_cast(info.first), forward); + }catch(Base::Exception &e) { e.ReportException(); errMsg = e.what(); diff --git a/src/App/Transactions.h b/src/App/Transactions.h index 94eceb7a44..01bcd1d2c2 100644 --- a/src/App/Transactions.h +++ b/src/App/Transactions.h @@ -87,6 +87,11 @@ public: void addObjectDel(const TransactionalObject *Obj); void addObjectChange(const TransactionalObject *Obj, const Property *Prop); + /// Check if any transaction is being applied. + static bool isApplying(Property *prop = nullptr); + + static void removePendingProperty(Property *prop); + private: int transID; typedef std::pair Info; @@ -191,6 +196,20 @@ public: } }; +class AppExport TransactionGuard +{ +private: + /// Private new operator to prevent heap allocation + void* operator new(size_t size); + +public: + TransactionGuard(bool undo); + ~TransactionGuard(); + +private: + bool undo; +}; + } //namespace App #endif // APP_TRANSACTION_H diff --git a/src/Gui/Document.cpp b/src/Gui/Document.cpp index 8e1d4bba78..e2daebbd81 100644 --- a/src/Gui/Document.cpp +++ b/src/Gui/Document.cpp @@ -2359,13 +2359,18 @@ void Document::undo(int iSteps) { Base::FlagToggler<> flag(d->_isTransacting); - if(!checkTransactionID(true,iSteps)) - return; + Gui::Selection().clearCompleteSelection(); - for (int i=0;iundo(); + { + App::TransactionGuard guard(true); + + if(!checkTransactionID(true,iSteps)) + return; + + for (int i=0;iundo(); + } } - App::GetApplication().signalUndo(); } /// Will REDO one or more steps @@ -2373,14 +2378,18 @@ void Document::redo(int iSteps) { Base::FlagToggler<> flag(d->_isTransacting); - if(!checkTransactionID(false,iSteps)) - return; + Gui::Selection().clearCompleteSelection(); - for (int i=0;iredo(); + { + App::TransactionGuard guard(false); + + if(!checkTransactionID(false,iSteps)) + return; + + for (int i=0;iredo(); + } } - App::GetApplication().signalRedo(); - for (auto it : d->_redoViewProviders) handleChildren3D(it); d->_redoViewProviders.clear(); diff --git a/src/Gui/ViewProviderDocumentObject.cpp b/src/Gui/ViewProviderDocumentObject.cpp index 2c245d9bea..59b7b151b8 100644 --- a/src/Gui/ViewProviderDocumentObject.cpp +++ b/src/Gui/ViewProviderDocumentObject.cpp @@ -701,3 +701,7 @@ std::string ViewProviderDocumentObject::getFullName() const { return pcObject->getFullName() + ".ViewObject"; return std::string("?"); } + +App::Document *ViewProviderDocumentObject::getOwnerDocument() const { + return pcObject?pcObject->getDocument():0; +} diff --git a/src/Gui/ViewProviderDocumentObject.h b/src/Gui/ViewProviderDocumentObject.h index aa8dfbe7ce..da578f3b78 100644 --- a/src/Gui/ViewProviderDocumentObject.h +++ b/src/Gui/ViewProviderDocumentObject.h @@ -144,6 +144,8 @@ public: virtual std::string getFullName() const override; + virtual App::Document *getOwnerDocument() const override; + /** Allow this class to be used as an override for the original view provider of the given object * * @sa App::DocumentObject::getViewProviderNameOverride() diff --git a/src/Mod/Part/App/PartFeature.cpp b/src/Mod/Part/App/PartFeature.cpp index e132fe5543..89d9ec64c2 100644 --- a/src/Mod/Part/App/PartFeature.cpp +++ b/src/Mod/Part/App/PartFeature.cpp @@ -62,6 +62,7 @@ #include #include #include +#include #include #include #include @@ -540,8 +541,17 @@ void Feature::onChanged(const App::Property* prop) { // if the placement has changed apply the change to the point data as well if (prop == &this->Placement) { - TopoShape& shape = const_cast(this->Shape.getShape()); + // The following code bypasses transaction, which may cause problem to + // undo/redo + // + // TopoShape& shape = const_cast(this->Shape.getShape()); + // shape.setTransform(this->Placement.getValue().toMatrix()); + + TopoShape shape = this->Shape.getShape(); shape.setTransform(this->Placement.getValue().toMatrix()); + Base::ObjectStatusLocker guard( + App::Property::NoRecompute, &this->Shape); + this->Shape.setValue(shape); } // if the point data has changed check and adjust the transformation as well else if (prop == &this->Shape) { @@ -554,8 +564,7 @@ void Feature::onChanged(const App::Property* prop) // shape must not be null to override the placement if (!this->Shape.getValue().IsNull()) { p.fromMatrix(this->Shape.getShape().getTransform()); - if (p != this->Placement.getValue()) - this->Placement.setValue(p); + this->Placement.setValueIfChanged(p); } } } diff --git a/src/Mod/Part/Gui/ViewProviderExt.cpp b/src/Mod/Part/Gui/ViewProviderExt.cpp index 3cda4c59c5..72368e0b8d 100644 --- a/src/Mod/Part/Gui/ViewProviderExt.cpp +++ b/src/Mod/Part/Gui/ViewProviderExt.cpp @@ -840,6 +840,12 @@ void ViewProviderPartExt::updateData(const App::Property* prop) { const char *propName = prop->getName(); if (propName && (strcmp(propName, "Shape") == 0 || strstr(propName, "Touched") != nullptr)) { + TopoDS_Shape cShape = Part::Feature::getShape(getObject()); + if(cachedShape.IsPartner(cShape)) { + Gui::ViewProviderGeometryObject::updateData(prop); + return; + } + // calculate the visual only if visible if (isUpdateForced() || Visibility.getValue()) updateVisual(); @@ -918,6 +924,7 @@ void ViewProviderPartExt::updateVisual() haction.apply(this->nodeset); TopoDS_Shape cShape = Part::Feature::getShape(getObject()); + cachedShape = cShape; if (cShape.IsNull()) { coords ->point .setNum(0); norm ->vector .setNum(0); diff --git a/src/Mod/Part/Gui/ViewProviderExt.h b/src/Mod/Part/Gui/ViewProviderExt.h index 4dcbd6ac5c..49b8c8215f 100644 --- a/src/Mod/Part/Gui/ViewProviderExt.h +++ b/src/Mod/Part/Gui/ViewProviderExt.h @@ -188,6 +188,8 @@ private: static App::PropertyQuantityConstraint::Constraints angDeflectionRange; static const char* LightingEnums[]; static const char* DrawStyleEnums[]; + + TopoDS_Shape cachedShape; }; }