From 62496d7277fa47c6a69900c1218dd769b12db9b4 Mon Sep 17 00:00:00 2001 From: Uwe Date: Tue, 22 Feb 2022 01:21:49 +0100 Subject: [PATCH] Revert "App: fix property ordering problem when undo/redo (#3255)" This reverts commit c3178343dbb896994d7fdf6a13c4eaf40f8509ec. --- 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, 68 insertions(+), 250 deletions(-) diff --git a/src/App/Document.cpp b/src/App/Document.cpp index a3575cbef3..2440a03452 100644 --- a/src/App/Document.cpp +++ b/src/App/Document.cpp @@ -957,7 +957,6 @@ 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); } @@ -967,13 +966,11 @@ 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); @@ -986,6 +983,18 @@ 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; } @@ -999,11 +1008,8 @@ bool Document::redo(int id) auto it = mRedoMap.find(id); if(it == mRedoMap.end()) return false; - { - TransactionGuard guard(false); - while(mRedoTransactions.size() && mRedoTransactions.back()!=it->second) - redo(0); - } + while(mRedoTransactions.size() && mRedoTransactions.back()!=it->second) + redo(0); } if (d->activeUndoTransaction) @@ -1011,13 +1017,12 @@ 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); @@ -1028,6 +1033,16 @@ 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; } @@ -1052,7 +1067,7 @@ void Document::addOrRemovePropertyOfObject(TransactionalObject* obj, Property *p bool Document::isPerformingTransaction() const { - return d->undoing || d->rollback || Transaction::isApplying(); + return d->undoing || d->rollback; } std::vector Document::getAvailableUndoNames() const @@ -1207,12 +1222,17 @@ 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(); @@ -1244,13 +1264,12 @@ void Document::abortTransaction() { void Document::_abortTransaction() { - 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; - } + if(isPerformingTransaction() || d->committing) { + if (FC_LOG_INSTANCE.isEnabled(FC_LOGLEVEL_LOG)) + FC_WARN("Cannot abort transaction while transacting"); + } + if (d->activeUndoTransaction) { Base::FlagToggler flag(d->rollback); Application::TransactionSignaller signaller(true,true); @@ -2918,10 +2937,6 @@ 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 701b5992fb..9ca8bc97e8 100644 --- a/src/App/Document.h +++ b/src/App/Document.h @@ -503,8 +503,6 @@ 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 a4ee1f5af6..f6c09bc764 100644 --- a/src/App/DocumentObject.cpp +++ b/src/App/DocumentObject.cpp @@ -267,10 +267,6 @@ 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 5a35a3263c..b747d73eca 100644 --- a/src/App/DocumentObject.h +++ b/src/App/DocumentObject.h @@ -139,7 +139,6 @@ 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 a9990267a5..52876db92c 100644 --- a/src/App/Property.cpp +++ b/src/App/Property.cpp @@ -33,7 +33,6 @@ #include "Property.h" #include "ObjectIdentifier.h" #include "PropertyContainer.h" -#include "Transactions.h" #include #include #include "Application.h" @@ -62,7 +61,7 @@ Property::Property() Property::~Property() { - Transaction::removePendingProperty(this); + } const char* Property::getName() const @@ -216,14 +215,9 @@ void Property::destroy(Property *p) { void Property::touch() { PropertyCleaner guard(this); - StatusBits.set(Touched); - if (getName() && father && !Transaction::isApplying(this)) { + if (father) father->onChanged(this); - if(!testStatus(Busy)) { - Base::BitsetLocker guard(StatusBits,Busy); - signalChanged(*this); - } - } + StatusBits.set(Touched); } void Property::setReadOnly(bool readOnly) @@ -233,7 +227,15 @@ void Property::setReadOnly(bool readOnly) void Property::hasSetValue(void) { - touch(); + PropertyCleaner guard(this); + if (father) { + father->onChanged(this); + if(!testStatus(Busy)) { + Base::BitsetLocker guard(StatusBits,Busy); + signalChanged(*this); + } + } + StatusBits.set(Touched); } void Property::aboutToSetValue(void) diff --git a/src/App/PropertyContainer.h b/src/App/PropertyContainer.h index 991cc5bd41..92458b8d5a 100644 --- a/src/App/PropertyContainer.h +++ b/src/App/PropertyContainer.h @@ -42,7 +42,6 @@ class Property; class PropertyContainer; class DocumentObject; class Extension; -class Document; enum PropertyType { @@ -162,10 +161,6 @@ 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 @@ -236,7 +231,6 @@ public: } friend class Property; - friend class TransactionObject; friend class DynamicProperty; @@ -253,10 +247,6 @@ 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 5b547dafc6..0e57ff0e81 100644 --- a/src/App/Transactions.cpp +++ b/src/App/Transactions.cpp @@ -36,10 +36,8 @@ using Base::Writer; #include using Base::XMLReader; #include -#include #include "Transactions.h" #include "Property.h" -#include "Application.h" #include "Document.h" #include "DocumentObject.h" @@ -165,133 +163,6 @@ 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) { @@ -304,7 +175,6 @@ 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 cd35022d57..2eea78ee0b 100644 --- a/src/App/Transactions.h +++ b/src/App/Transactions.h @@ -87,11 +87,6 @@ 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; @@ -197,20 +192,6 @@ 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 e2daebbd81..8e1d4bba78 100644 --- a/src/Gui/Document.cpp +++ b/src/Gui/Document.cpp @@ -2359,18 +2359,13 @@ void Document::undo(int iSteps) { Base::FlagToggler<> flag(d->_isTransacting); - Gui::Selection().clearCompleteSelection(); + if(!checkTransactionID(true,iSteps)) + return; - { - App::TransactionGuard guard(true); - - if(!checkTransactionID(true,iSteps)) - return; - - for (int i=0;iundo(); - } + for (int i=0;iundo(); } + App::GetApplication().signalUndo(); } /// Will REDO one or more steps @@ -2378,18 +2373,14 @@ void Document::redo(int iSteps) { Base::FlagToggler<> flag(d->_isTransacting); - Gui::Selection().clearCompleteSelection(); + if(!checkTransactionID(false,iSteps)) + return; - { - App::TransactionGuard guard(false); - - if(!checkTransactionID(false,iSteps)) - return; - - for (int i=0;iredo(); - } + 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 59b7b151b8..2c245d9bea 100644 --- a/src/Gui/ViewProviderDocumentObject.cpp +++ b/src/Gui/ViewProviderDocumentObject.cpp @@ -701,7 +701,3 @@ 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 da578f3b78..aa8dfbe7ce 100644 --- a/src/Gui/ViewProviderDocumentObject.h +++ b/src/Gui/ViewProviderDocumentObject.h @@ -144,8 +144,6 @@ 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 89d9ec64c2..e132fe5543 100644 --- a/src/Mod/Part/App/PartFeature.cpp +++ b/src/Mod/Part/App/PartFeature.cpp @@ -62,7 +62,6 @@ #include #include #include -#include #include #include #include @@ -541,17 +540,8 @@ 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) { - // 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(); + TopoShape& shape = const_cast(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) { @@ -564,7 +554,8 @@ 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()); - this->Placement.setValueIfChanged(p); + if (p != this->Placement.getValue()) + this->Placement.setValue(p); } } } diff --git a/src/Mod/Part/Gui/ViewProviderExt.cpp b/src/Mod/Part/Gui/ViewProviderExt.cpp index 72368e0b8d..3cda4c59c5 100644 --- a/src/Mod/Part/Gui/ViewProviderExt.cpp +++ b/src/Mod/Part/Gui/ViewProviderExt.cpp @@ -840,12 +840,6 @@ 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(); @@ -924,7 +918,6 @@ 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 49b8c8215f..4dcbd6ac5c 100644 --- a/src/Mod/Part/Gui/ViewProviderExt.h +++ b/src/Mod/Part/Gui/ViewProviderExt.h @@ -188,8 +188,6 @@ private: static App::PropertyQuantityConstraint::Constraints angDeflectionRange; static const char* LightingEnums[]; static const char* DrawStyleEnums[]; - - TopoDS_Shape cachedShape; }; }