#5408: Extend the IUndoSystem::Tracker interface to include the opera…

…tion name
codereader · Oct 27, 2021 · c813c21 · c813c21
1 parent f18aa2d
commit c813c21
Show file tree

Hide file tree

Showing 4 changed files with 143 additions and 13 deletions.
diff --git a/include/iundo.h b/include/iundo.h
@@ -102,13 +102,13 @@ class IUndoSystem
         virtual ~Tracker() {}
 
         // Invoked when a non-empty operation has been recorded by the undo system
-        virtual void onOperationRecorded() = 0;
+        virtual void onOperationRecorded(const std::string& operationName) = 0;
 
         // Called when a single operation has been undone
-        virtual void onOperationUndone() = 0;
+        virtual void onOperationUndone(const std::string& operationName) = 0;
 
         // Called when a single operation has been redone
-        virtual void onOperationRedone() = 0;
+        virtual void onOperationRedone(const std::string& operationName) = 0;
 
         // Invoked when the undo and redo stacks have been cleared
         virtual void onAllOperationsCleared() = 0;

diff --git a/libs/UndoFileChangeTracker.h b/libs/UndoFileChangeTracker.h
@@ -44,7 +44,7 @@ class UndoFileChangeTracker :
         return _currentChangeCount;
     }
 
-    void onOperationRecorded() override
+    void onOperationRecorded(const std::string& operationName) override
     {
         if (_currentChangeCount < _savedChangeCount)
         {
@@ -56,13 +56,13 @@ class UndoFileChangeTracker :
         _changed.emit();
     }
 
-    void onOperationUndone() override
+    void onOperationUndone(const std::string& operationName) override
     {
         --_currentChangeCount;
         _changed.emit();
     }
 
-    void onOperationRedone() override
+    void onOperationRedone(const std::string& operationName) override
     {
         ++_currentChangeCount;
         _changed.emit();

diff --git a/radiantcore/undo/UndoSystem.cpp b/radiantcore/undo/UndoSystem.cpp
@@ -69,7 +69,7 @@ void UndoSystem::finish(const std::string& command)
 	if (finishUndo(command))
     {
 		rMessage() << command << std::endl;
-        for (auto tracker : _trackers) { tracker->onOperationRecorded(); }
+        for (auto tracker : _trackers) { tracker->onOperationRecorded(command); }
 	}
 }
 
@@ -88,13 +88,14 @@ void UndoSystem::undo()
     }
 
 	const auto& operation = _undoStack.back();
-	rMessage() << "Undo: " << operation->getName() << std::endl;
+    auto operationName = operation->getName(); // copy this name, we need it after op destruction
+	rMessage() << "Undo: " << operationName << std::endl;
 
 	startRedo();
 	operation->restoreSnapshot();
-	finishRedo(operation->getName());
+	finishRedo(operationName);
 	_undoStack.pop_back();
-    for (auto tracker : _trackers) { tracker->onOperationUndone(); }
+    for (auto tracker : _trackers) { tracker->onOperationUndone(operationName); }
 
 	_signalPostUndo.emit();
 
@@ -123,13 +124,14 @@ void UndoSystem::redo()
     }
 
 	const auto& operation = _redoStack.back();
-	rMessage() << "Redo: " << operation->getName() << std::endl;
+    auto operationName = operation->getName(); // copy this name, we need it after op destruction
+	rMessage() << "Redo: " << operationName << std::endl;
 
 	startUndo();
 	operation->restoreSnapshot();
-	finishUndo(operation->getName());
+	finishUndo(operationName);
 	_redoStack.pop_back();
-    for (auto tracker : _trackers) { tracker->onOperationRedone(); }
+    for (auto tracker : _trackers) { tracker->onOperationRedone(operationName); }
 
 	_signalPostRedo.emit();
 

diff --git a/test/UndoRedo.cpp b/test/UndoRedo.cpp
@@ -757,4 +757,132 @@ TEST_F(UndoTest, MapUndoRedoSignalsWhenChangingMaps)
     EXPECT_TRUE(postRedoFired) << "Map didn't fire the post-redo signal after changing maps";
 }
 
+class TestUndoTracker :
+    public IUndoSystem::Tracker
+{
+public:
+    TestUndoTracker()
+    {
+        reset();
+    }
+
+    bool recordedFired;
+    bool undoneFired;
+    bool redoneFired;
+    bool clearedFired;
+    std::string receivedOperationName;
+
+    void reset()
+    {
+        recordedFired = false;
+        undoneFired = false;
+        redoneFired = false;
+        clearedFired = false;
+        receivedOperationName.clear();
+    }
+
+    void onOperationRecorded(const std::string& operationName) override
+    {
+        recordedFired = true;
+        receivedOperationName = operationName;
+    }
+
+    void onOperationUndone(const std::string& operationName) override
+    {
+        undoneFired = true;
+        receivedOperationName = operationName;
+    }
+
+    void onOperationRedone(const std::string& operationName) override
+    {
+        redoneFired = true;
+        receivedOperationName = operationName;
+    }
+
+    void onAllOperationsCleared() override
+    {
+        clearedFired = true;
+        receivedOperationName.clear();
+    }
+};
+
+TEST_F(UndoTest, OperationTracking)
+{
+    TestUndoTracker tracker;
+
+    GlobalMapModule().getRoot()->getUndoSystem().attachTracker(tracker);
+
+    // Record one undoable operation in the existing map
+    {
+        UndoableCommand cmd("testOperation");
+        setupTestEntity();
+    }
+
+    EXPECT_TRUE(tracker.recordedFired) << "Expected signal not fired after operation complete";
+    EXPECT_FALSE(tracker.undoneFired) << "Wrong signal fired";
+    EXPECT_FALSE(tracker.redoneFired) << "Wrong signal fired";
+    EXPECT_FALSE(tracker.clearedFired) << "Wrong signal fired";
+    EXPECT_EQ(tracker.receivedOperationName, "testOperation") << "Message not correct";
+
+    tracker.reset();
+    GlobalUndoSystem().undo();
+
+    EXPECT_TRUE(tracker.undoneFired) << "Expected signal not fired after operation undone";
+    EXPECT_FALSE(tracker.recordedFired) << "Wrong signal fired";
+    EXPECT_FALSE(tracker.redoneFired) << "Wrong signal fired";
+    EXPECT_FALSE(tracker.clearedFired) << "Wrong signal fired";
+    EXPECT_EQ(tracker.receivedOperationName, "testOperation") << "Message not correct";
+
+    // Undo again
+    tracker.reset();
+    GlobalUndoSystem().undo();
+
+    EXPECT_FALSE(tracker.undoneFired) << "Nothing should fire, nothing to undo";
+    EXPECT_FALSE(tracker.recordedFired) << "Nothing should fire, nothing to undo";
+    EXPECT_FALSE(tracker.redoneFired) << "Nothing should fire, nothing to undo";
+    EXPECT_FALSE(tracker.clearedFired) << "Nothing should fire, nothing to undo";
+    EXPECT_EQ(tracker.receivedOperationName, "") << "Nothing should fire, nothing to undo";
+
+    tracker.reset();
+    GlobalUndoSystem().redo();
+
+    EXPECT_TRUE(tracker.redoneFired) << "Expected signal not fired after operation redone";
+    EXPECT_FALSE(tracker.recordedFired) << "Wrong signal fired";
+    EXPECT_FALSE(tracker.undoneFired) << "Wrong signal fired";
+    EXPECT_FALSE(tracker.clearedFired) << "Wrong signal fired";
+    EXPECT_EQ(tracker.receivedOperationName, "testOperation") << "Message not correct";
+
+    tracker.reset();
+    GlobalUndoSystem().redo();
+
+    EXPECT_FALSE(tracker.undoneFired) << "Nothing should fire, nothing to redo";
+    EXPECT_FALSE(tracker.recordedFired) << "Nothing should fire, nothing to redo";
+    EXPECT_FALSE(tracker.redoneFired) << "Nothing should fire, nothing to redo";
+    EXPECT_FALSE(tracker.clearedFired) << "Nothing should fire, nothing to redo";
+    EXPECT_EQ(tracker.receivedOperationName, "") << "Nothing should fire, nothing to redo";
+
+    GlobalMapModule().getRoot()->getUndoSystem().clear();
+
+    EXPECT_TRUE(tracker.clearedFired) << "Expected signal not fired after operation redone";
+    EXPECT_FALSE(tracker.recordedFired) << "Wrong signal fired";
+    EXPECT_FALSE(tracker.undoneFired) << "Wrong signal fired";
+    EXPECT_FALSE(tracker.redoneFired) << "Wrong signal fired";
+    EXPECT_EQ(tracker.receivedOperationName, "") << "Message not correct";
+
+    GlobalMapModule().getRoot()->getUndoSystem().detachTracker(tracker);
+    tracker.reset();
+
+    // One more change after detaching
+    {
+        UndoableCommand cmd("testOperation");
+        setupTestEntity();
+    }
+
+    EXPECT_FALSE(tracker.undoneFired) << "Nothing should fire, already detached";
+    EXPECT_FALSE(tracker.recordedFired) << "Nothing should fire, already detached";
+    EXPECT_FALSE(tracker.redoneFired) << "Nothing should fire, already detached";
+    EXPECT_FALSE(tracker.clearedFired) << "Nothing should fire, already detached";
+    EXPECT_EQ(tracker.receivedOperationName, "") << "Nothing should fire, already detached";
+}
+
 }