App: fix undo/redo of dynamic property add/remove/change

src/App/Property.cpp

@@ -27,6 +27,8 @@
 #	include <cassert>
 #endif
 
+#include <atomic>
+
 /// Here the FreeCAD includes sorted by Base,App,Gui......
 #include "Property.h"
 #include "ObjectIdentifier.h"
@@ -50,11 +52,12 @@ TYPESYSTEM_SOURCE_ABSTRACT(App::Property , Base::Persistence)
 //**************************************************************************
 // Construction/Destruction
 
+static std::atomic<int64_t> _PropID;
+
 // Here is the implementation! Description should take place in the header file!
 Property::Property()
-  :father(0), myName(0)
+  :father(0), myName(0), _id(++_PropID)
 {
-
 }
 
 Property::~Property()

src/App/Property.h

@@ -252,6 +252,16 @@ class AppExport Property : public Base::Persistence
     /// Compare if this property has the same content as the given one
     virtual bool isSame(const Property &other) const;
 
+    /** Return a unique ID for the property
+     *
+     * The ID of a property is generated from a monotonically increasing
+     * internal counter. The intention of the ID is to be used as a key for
+     * mapping, instead of using the raw pointer. Because, it is possible for
+     * the runtime memory allocator to reuse just deleted memory, which will
+     * cause hard to debug problem if use pointer as key. 
+     */
+    int64_t getID() const {return _id;}
+
     friend class PropertyContainer;
     friend struct PropertyData;
     friend class DynamicProperty;
@@ -288,6 +298,7 @@ class AppExport Property : public Base::Persistence
 private:
     PropertyContainer *father;
     const char *myName;
+    int64_t _id;
 
 public:
     boost::signals2::signal<void (const App::Property&)> signalChanged;

src/App/Transactions.cpp

@@ -429,7 +429,7 @@ void TransactionObject::applyChn(Document & /*Doc*/, TransactionalObject *pcObj,
         // Property change order is not preserved, as it is recursive in nature
         for(auto &v : _PropChangeMap) {
             auto &data = v.second;
-            auto prop = const_cast<Property*>(v.first);
+            auto prop = const_cast<Property*>(data.propertyOrig);
 
             if(!data.property) {
                 // here means we are undoing/redoing and property add operation
@@ -441,9 +441,9 @@ void TransactionObject::applyChn(Document & /*Doc*/, TransactionalObject *pcObj,
             // been destroies. We must prepare for the case where user removed
             // a dynamic property but does not recordered as transaction.
             auto name = pcObj->getPropertyName(prop);
-            if(!name) {
+            if(!name || (data.name.size() && data.name != name) || data.propertyType != prop->getTypeId()) {
                 // Here means the original property is not found, probably removed
-                if(v.second.name.empty()) {
+                if(data.name.empty()) {
                     // not a dynamic property, nothing to do
                     continue;
                 }
@@ -452,12 +452,12 @@ void TransactionObject::applyChn(Document & /*Doc*/, TransactionalObject *pcObj,
                 // restored. But since restoring property is actually creating
                 // a new property, the property key inside redo stack will not
                 // match. So we search by name first.
-                prop = pcObj->getDynamicPropertyByName(v.second.name.c_str());
+                prop = pcObj->getDynamicPropertyByName(data.name.c_str());
                 if(!prop) {
                     // Still not found, re-create the property
                     prop = pcObj->addDynamicProperty(
-                            data.property->getTypeId().getName(),
-                            v.second.name.c_str(), data.group.c_str(), data.doc.c_str(),
+                            data.propertyType.getName(),
+                            data.name.c_str(), data.group.c_str(), data.doc.c_str(),
                             data.attr, data.readonly, data.hidden);
                     if(!prop)
                         continue;
@@ -493,10 +493,11 @@ void TransactionObject::applyChn(Document & /*Doc*/, TransactionalObject *pcObj,
 
 void TransactionObject::setProperty(const Property* pcProp)
 {
-    auto &data = _PropChangeMap[pcProp];
+    auto &data = _PropChangeMap[pcProp->getID()];
     if(!data.property && data.name.empty()) {
         static_cast<DynamicProperty::PropData&>(data) = 
             pcProp->getContainer()->getDynamicPropertyData(pcProp);
+        data.propertyOrig = pcProp;
         data.property = pcProp->Copy();
         data.propertyType = pcProp->getTypeId();
         data.property->setStatusValue(pcProp->getStatus());
@@ -509,20 +510,20 @@ void TransactionObject::addOrRemoveProperty(const Property* pcProp, bool add)
     if(!pcProp || !pcProp->getContainer())
         return;
 
-    auto &data = _PropChangeMap[pcProp];
+    auto &data = _PropChangeMap[pcProp->getID()];
     if(data.name.size()) {
         if(!add && !data.property) {
             // this means add and remove the same property inside a single
             // transaction, so they cancel each other out.
-            _PropChangeMap.erase(pcProp);
+            _PropChangeMap.erase(pcProp->getID());
         }
         return;
     }
     if(data.property) {
         delete data.property;
         data.property = 0;
     }
-
+    data.propertyOrig = pcProp;
     static_cast<DynamicProperty::PropData&>(data) = 
         pcProp->getContainer()->getDynamicPropertyData(pcProp);
     if(add) 

src/App/Transactions.h

@@ -137,8 +137,9 @@ class AppExport TransactionObject : public Base::Persistence
 
     struct PropData : DynamicProperty::PropData {
         Base::Type propertyType;
+        const Property *propertyOrig = nullptr;
     };
-    std::unordered_map<const Property*, PropData> _PropChangeMap;
+    std::unordered_map<int64_t, PropData> _PropChangeMap;
 
     std::string _NameInDocument;
 };