Merge pull request #21975 from pieterhijma/transaction-rename-property

Core: Add redo/undo to property rename

src/App/Document.cpp

@@ -262,8 +262,9 @@ bool Document::redo(const int id)
     return false;
 }
 
-void Document::addOrRemovePropertyOfObject(TransactionalObject* obj,
-                                           const Property* prop, const bool add)
+void Document::changePropertyOfObject(TransactionalObject* obj,
+                                      const Property* prop,
+                                      const std::function<void()>& changeFunc)
 {
     if (!prop || !obj || !obj->isAttachedToDocument()) {
         return;
@@ -278,10 +279,26 @@ void Document::addOrRemovePropertyOfObject(TransactionalObject* obj,
         }
     }
     if (d->activeUndoTransaction && !d->rollback) {
-        d->activeUndoTransaction->addOrRemoveProperty(obj, prop, add);
+        changeFunc();
     }
 }
 
+void Document::renamePropertyOfObject(TransactionalObject* obj,
+                                      const Property* prop, const char* oldName)
+{
+    changePropertyOfObject(obj, prop, [this, obj, prop, oldName]() {
+        d->activeUndoTransaction->renameProperty(obj, prop, oldName);
+    });
+}
+
+void Document::addOrRemovePropertyOfObject(TransactionalObject* obj,
+                                           const Property* prop, const bool add)
+{
+    changePropertyOfObject(obj, prop, [this, obj, prop, add]() {
+        d->activeUndoTransaction->addOrRemoveProperty(obj, prop, add);
+    });
+}
+
 bool Document::isPerformingTransaction() const
 {
     return d->undoing || d->rollback;

src/App/Document.h

@@ -518,6 +518,7 @@ public:
     bool isPerformingTransaction() const;
     /// \internal add or remove property from a transactional object
     void addOrRemovePropertyOfObject(TransactionalObject*, const Property* prop, bool add);
+    void renamePropertyOfObject(TransactionalObject*, const Property* prop, const char* newName);
     //@}
 
     /** @name dependency stuff */
@@ -699,6 +700,10 @@ protected:
     /// Internally called by Application to abort the running transaction.
     void _abortTransaction();
 
+private:
+    void changePropertyOfObject(TransactionalObject* obj, const Property* prop,
+                                const std::function<void()>& changeFunc);
+
 private:
     // # Data Member of the document
     // +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

src/App/DocumentObject.cpp

@@ -712,6 +712,16 @@ bool DocumentObject::removeDynamicProperty(const char* name)
     return TransactionalObject::removeDynamicProperty(name);
 }
 
+bool DocumentObject::renameDynamicProperty(Property* prop, const char* name)
+{
+    std::string oldName = prop->getName();
+    bool renamed = TransactionalObject::renameDynamicProperty(prop, name);
+    if (renamed && _pDoc) {
+        _pDoc->renamePropertyOfObject(this, prop, oldName.c_str());
+    }
+    return renamed;
+}
+
 App::Property* DocumentObject::addDynamicProperty(const char* type,
                                                   const char* name,
                                                   const char* group,

src/App/DocumentObject.h

@@ -568,6 +568,8 @@ public:
 
     bool removeDynamicProperty(const char* prop) override;
 
+    bool renameDynamicProperty(Property *prop, const char *name) override;
+
     App::Property* addDynamicProperty(const char* type,
                                       const char* name = nullptr,
                                       const char* group = nullptr,

src/App/PropertyContainer.h

@@ -548,7 +548,7 @@ public:
    * @return `true` if the property was renamed; `false` otherwise.
    * @throw Base::NameError If the new name is invalid or already exists.
    */
-  bool renameDynamicProperty(Property *prop, const char *name) {
+  virtual bool renameDynamicProperty(Property *prop, const char *name) {
       return dynamicProps.renameDynamicProperty(prop,name);
   }
 

src/App/Transactions.cpp

@@ -148,7 +148,8 @@ bool Transaction::hasObject(const TransactionalObject* Obj) const
 #endif
 }
 
-void Transaction::addOrRemoveProperty(TransactionalObject* Obj, const Property* pcProp, bool add)
+void Transaction::changeProperty(TransactionalObject* Obj,
+                                 std::function<void(TransactionObject* to)> changeFunc)
 {
     auto& index = _Objects.get<1>();
     auto pos = index.find(Obj);
@@ -164,7 +165,21 @@ void Transaction::addOrRemoveProperty(TransactionalObject* Obj, const Property*
         index.emplace(Obj, To);
     }
 
-    To->addOrRemoveProperty(pcProp, add);
+    changeFunc(To);
+}
+
+void Transaction::renameProperty(TransactionalObject* Obj, const Property* pcProp, const char* oldName)
+{
+    changeProperty(Obj, [pcProp, oldName](TransactionObject* to) {
+        to->renameProperty(pcProp, oldName);
+    });
+}
+
+void Transaction::addOrRemoveProperty(TransactionalObject* Obj, const Property* pcProp, bool add)
+{
+    changeProperty(Obj, [pcProp, add](TransactionObject* to) {
+        to->addOrRemoveProperty(pcProp, add);
+    });
 }
 
 //**************************************************************************
@@ -294,7 +309,13 @@ TransactionObject::TransactionObject() = default;
 TransactionObject::~TransactionObject()
 {
     for (auto& v : _PropChangeMap) {
-        delete v.second.property;
+        auto& data = v.second;
+        // If nameOrig is used, it means it is a transaction of a rename
+        // operation.  This operation does not interact with v.second.property,
+        // so it should not be deleted in that case.
+	if (data.nameOrig.empty()) {
+	    delete v.second.property;
+	}
     }
 }
 
@@ -312,6 +333,15 @@ void TransactionObject::applyChn(Document& /*Doc*/, TransactionalObject* pcObj,
             auto& data = v.second;
             auto prop = const_cast<Property*>(data.propertyOrig);
 
+            if (!data.nameOrig.empty()) {
+                // This means we are undoing/redoing a rename operation
+                Property* currentProp = pcObj->getDynamicPropertyByName(data.name.c_str());
+                if (currentProp) {
+                    pcObj->renameDynamicProperty(currentProp, data.nameOrig.c_str());
+                }
+                continue;
+            }
+
             if (!data.property) {
                 // here means we are undoing/redoing and property add operation
                 pcObj->removeDynamicProperty(v.second.name.c_str());
@@ -393,6 +423,21 @@ void TransactionObject::setProperty(const Property* pcProp)
     }
 }
 
+void TransactionObject::renameProperty(const Property* pcProp, const char* oldName)
+{
+    if (!pcProp || !pcProp->getContainer()) {
+        return;
+    }
+
+    auto& data = _PropChangeMap[pcProp->getID()];
+
+    if (data.name.empty()) {
+        static_cast<DynamicProperty::PropData&>(data) =
+            pcProp->getContainer()->getDynamicPropertyData(pcProp);
+    }
+    data.nameOrig = oldName;
+}
+
 void TransactionObject::addOrRemoveProperty(const Property* pcProp, bool add)
 {
     (void)add;

src/App/Transactions.h

@@ -81,12 +81,17 @@ public:
     bool isEmpty() const;
     /// check if this object is used in a transaction
     bool hasObject(const TransactionalObject* Obj) const;
+    void renameProperty(TransactionalObject* Obj, const Property* pcProp, const char* oldName);
     void addOrRemoveProperty(TransactionalObject* Obj, const Property* pcProp, bool add);
 
     void addObjectNew(TransactionalObject* Obj);
     void addObjectDel(const TransactionalObject* Obj);
     void addObjectChange(const TransactionalObject* Obj, const Property* Prop);
 
+private:
+    void changeProperty(TransactionalObject* Obj,
+                        std::function<void(TransactionObject* to)> changeFunc);
+
 private:
     int transID;
     using Info = std::pair<const TransactionalObject*, TransactionObject*>;
@@ -115,6 +120,7 @@ public:
     virtual void applyChn(Document& Doc, TransactionalObject* pcObj, bool Forward);
 
     void setProperty(const Property* pcProp);
+    void renameProperty(const Property* pcProp, const char* newName);
     void addOrRemoveProperty(const Property* pcProp, bool add);
 
     unsigned int getMemSize() const override;
@@ -136,6 +142,8 @@ protected:
     {
         Base::Type propertyType;
         const Property* propertyOrig = nullptr;
+        // for property renaming
+        std::string nameOrig;
     };
     std::unordered_map<int64_t, PropData> _PropChangeMap;
 

tests/src/App/Property.cpp

@@ -28,6 +28,7 @@
 #include <Base/Interpreter.h>
 
 #include <App/Application.h>
+#include <App/AutoTransaction.h>
 #include <App/Document.h>
 #include <App/Expression.h>
 #include <App/ObjectIdentifier.h>
@@ -313,3 +314,74 @@ TEST_F(RenameProperty, updateExpressionDifferentDocument)
     EXPECT_EQ(varSet->getDynamicPropertyByName("NewName"), prop);
     EXPECT_EQ(prop2->getValue(), Value);
 }
+
+// Tests whether we can rename a property and undo it
+TEST_F(RenameProperty, undoRenameProperty)
+{
+    // Arrange
+    _doc->setUndoMode(1);
+
+    // Act
+    bool isRenamed = false;
+    {
+        App::AutoTransaction transaction("Rename Property");
+        isRenamed = varSet->renameDynamicProperty(prop, "NewName");
+    }
+
+    // Assert
+    EXPECT_TRUE(isRenamed);
+    EXPECT_STREQ(varSet->getPropertyName(prop), "NewName");
+    EXPECT_EQ(prop->getValue(), Value);
+    EXPECT_EQ(varSet->getDynamicPropertyByName("Variable"), nullptr);
+    EXPECT_EQ(varSet->getDynamicPropertyByName("NewName"), prop);
+
+    // Act: Undo the rename
+    bool undone = _doc->undo();
+
+    // Assert: The property should be back to its original name and value
+    EXPECT_TRUE(undone);
+    EXPECT_STREQ(varSet->getPropertyName(prop), "Variable");
+    EXPECT_EQ(prop->getValue(), Value);
+    EXPECT_EQ(varSet->getDynamicPropertyByName("Variable"), prop);
+    EXPECT_EQ(varSet->getDynamicPropertyByName("NewName"), nullptr);
+}
+
+
+// Tests whether we can rename a property, undo, and redo it
+TEST_F(RenameProperty, redoRenameProperty)
+{
+    // Arrange
+    _doc->setUndoMode(1);
+
+    // Act
+    bool isRenamed = false;
+    {
+        App::AutoTransaction transaction("Rename Property");
+        isRenamed = varSet->renameDynamicProperty(prop, "NewName");
+    }
+
+    // Assert
+    EXPECT_TRUE(isRenamed);
+    EXPECT_STREQ(varSet->getPropertyName(prop), "NewName");
+    EXPECT_EQ(prop->getValue(), Value);
+    EXPECT_EQ(varSet->getDynamicPropertyByName("Variable"), nullptr);
+    EXPECT_EQ(varSet->getDynamicPropertyByName("NewName"), prop);
+
+    // Act: Undo the rename
+    bool undone = _doc->undo();
+
+    // Assert: The property should be back to its original name and value
+    EXPECT_TRUE(undone);
+    EXPECT_STREQ(varSet->getPropertyName(prop), "Variable");
+    EXPECT_EQ(prop->getValue(), Value);
+    EXPECT_EQ(varSet->getDynamicPropertyByName("Variable"), prop);
+    EXPECT_EQ(varSet->getDynamicPropertyByName("NewName"), nullptr);
+
+    // Act: Redo the rename
+    bool redone = _doc->redo();
+    EXPECT_TRUE(redone);
+    EXPECT_STREQ(varSet->getPropertyName(prop), "NewName");
+    EXPECT_EQ(prop->getValue(), Value);
+    EXPECT_EQ(varSet->getDynamicPropertyByName("Variable"), nullptr);
+    EXPECT_EQ(varSet->getDynamicPropertyByName("NewName"), prop);
+}