App: add documentation to PropertyCleaner

move global variables static variables of PropertyCleaner
add unit test for removal of property in onBeforeChange

src/App/Property.cpp

@@ -138,9 +138,17 @@ ObjectIdentifier Property::canonicalPath(const ObjectIdentifier &p) const
     return p;
 }
 
-static std::vector<Property*> _RemovedProps;
-static int _PropCleanerCounter;
-
+namespace App {
+/*!
+ * \brief The PropertyCleaner struct
+ * Make deleting dynamic property safer by postponing its destruction.
+ *
+ * Dynamic property can be removed at any time, even during triggering of
+ * onChanged() signal of the removing property. This patch introduced
+ * static function Property::destroy() to make it safer by queueing any
+ * removed property, and only deleting them when no onChanged() call is
+ * active.
+ */
 struct PropertyCleaner {
     PropertyCleaner(Property *p)
         : prop(p)
@@ -151,7 +159,7 @@ struct PropertyCleaner {
         if(--_PropCleanerCounter)
             return;
         bool found = false;
-        while(_RemovedProps.size()) {
+        while (_RemovedProps.size()) {
             auto p = _RemovedProps.back();
             _RemovedProps.pop_back();
             if(p != prop)
@@ -159,21 +167,32 @@ struct PropertyCleaner {
             else
                 found = true;
         }
-        if(found)
+
+        if (found)
             _RemovedProps.push_back(prop);
     }
+    static void add(Property *prop) {
+        _RemovedProps.push_back(prop);
+    }
 
     Property *prop;
+
+    static std::vector<Property*> _RemovedProps;
+    static int _PropCleanerCounter;
 };
+}
+
+std::vector<Property*> PropertyCleaner::_RemovedProps;
+int PropertyCleaner::_PropCleanerCounter = 0;
 
 void Property::destroy(Property *p) {
-    if(p) {
+    if (p) {
         // Is it necessary to nullify the container? May cause crash if any
         // onChanged() caller assumes a non-null container.
         //
         // p->setContainer(0);
 
-        _RemovedProps.push_back(p);
+        PropertyCleaner::add(p);
     }
 }
 

src/Mod/Test/Document.py

@@ -1358,6 +1358,21 @@ class DocumentPropertyCases(unittest.TestCase):
     self.Doc.recompute()
     self.assertTrue(not p2 in p1.InList)
 
+  def testRemovePropertyOnChange(self):
+    class Feature:
+      def __init__(self, fp):
+        fp.Proxy = self
+        fp.addProperty("App::PropertyString","Test")
+      def onBeforeChange(self, fp, prop):
+        if prop == "Test":
+          fp.removeProperty("Test")
+      def onChanged(self, fp, prop):
+        getattr(fp, prop)
+
+    obj = self.Doc.addObject("App::FeaturePython")
+    fea = Feature(obj)
+    obj.Test = "test"
+
   def tearDown(self):
     #closing doc
     FreeCAD.closeDocument("PropertyTests")