kindred/create
1
1
Fork 0
Code Issues 30 Pull Requests Actions Packages Projects 9 Releases 2 Wiki Activity

App: fix property ordering problem when undo/redo (#3255)

Browse Source 
* 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
This commit is contained in:
Zheng Lei
2022-02-21 19:29:01 +08:00
committed by GitHub
parent b42462ba14
commit c3178343db
14 changed files with 250 additions and 68 deletions
Download Patch File Download Diff File Expand all files Collapse all files

69
src/App/Document.cpp
View File

@@ -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<bool> 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<bool> 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<std::string> 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<bool> 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<App::Document*>(this);
}
const char* Document::getProgramVersion() const
{
return d->programVersion.c_str();

2
src/App/Document.h
View File

@@ -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();

4
src/App/DocumentObject.cpp
View File

@@ -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

1
src/App/DocumentObject.h
View File

@@ -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

22
src/App/Property.cpp
View File

@@ -31,6 +31,7 @@
#include "Property.h"
#include "ObjectIdentifier.h"
#include "PropertyContainer.h"
#include "Transactions.h"
#include <Base/Exception.h>
#include <Base/Tools.h>
#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<decltype(StatusBits)> 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<decltype(StatusBits)> guard(StatusBits,Busy);
signalChanged(*this);
}
}
StatusBits.set(Touched);
touch();
}
void Property::aboutToSetValue(void)

10
src/App/PropertyContainer.h
View File

@@ -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&);

130
src/App/Transactions.cpp
View File

@@ -36,8 +36,10 @@ using Base::Writer;
#include <Base/Reader.h>
using Base::XMLReader;
#include <Base/Console.h>
#include <Base/Tools.h>
#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<Property*, int> _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<std::pair<Property*,int> > props;
props.reserve(_PendingProps.size());
props.insert(props.end(),_PendingProps.begin(),_PendingProps.end());
std::sort(props.begin(), props.end(),
[](const std::pair<Property*,int> &a, const std::pair<Property*,int> &b) {
return a.second < b.second;
});
std::vector<App::Document*> docs;
std::set<App::Document*> 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<TransactionalObject*>(info.first));
for(auto &info : index)
info.second->applyChn(Doc, const_cast<TransactionalObject*>(info.first), forward);
}catch(Base::Exception &e) {
e.ReportException();
errMsg = e.what();

19
src/App/Transactions.h
View File

@@ -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<const TransactionalObject*, TransactionObject*> 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

31
src/Gui/Document.cpp
View File

@@ -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;i<iSteps;i++) {
getDocument()->undo();
{
App::TransactionGuard guard(true);
if(!checkTransactionID(true,iSteps))
return;
for (int i=0;i<iSteps;i++) {
getDocument()->undo();
}
}
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;i<iSteps;i++) {
getDocument()->redo();
{
App::TransactionGuard guard(false);
if(!checkTransactionID(false,iSteps))
return;
for (int i=0;i<iSteps;i++) {
getDocument()->redo();
}
}
App::GetApplication().signalRedo();
for (auto it : d->_redoViewProviders)
handleChildren3D(it);
d->_redoViewProviders.clear();

4
src/Gui/ViewProviderDocumentObject.cpp
View File

@@ -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;
}

2
src/Gui/ViewProviderDocumentObject.h
View File

@@ -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()

15
src/Mod/Part/App/PartFeature.cpp
View File

@@ -62,6 +62,7 @@
#include <Base/Stream.h>
#include <Base/Placement.h>
#include <Base/Rotation.h>
#include <Base/Tools.h>
#include <App/Application.h>
#include <App/FeaturePythonPyImp.h>
#include <App/Document.h>
@@ -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<TopoShape&>(this->Shape.getShape());
// The following code bypasses transaction, which may cause problem to
// undo/redo
//
// TopoShape& shape = const_cast<TopoShape&>(this->Shape.getShape());
// shape.setTransform(this->Placement.getValue().toMatrix());
TopoShape shape = this->Shape.getShape();
shape.setTransform(this->Placement.getValue().toMatrix());
Base::ObjectStatusLocker<App::Property::Status, App::Property> 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);
}
}
}

7
src/Mod/Part/Gui/ViewProviderExt.cpp
View File

@@ -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);

2
src/Mod/Part/Gui/ViewProviderExt.h
View File

@@ -188,6 +188,8 @@ private:
static App::PropertyQuantityConstraint::Constraints angDeflectionRange;
static const char* LightingEnums[];
static const char* DrawStyleEnums[];
TopoDS_Shape cachedShape;
};
}