#5408: Remove IUndoSystem::Tracker interface in favour of a sigc++ si…

…gnal, which has safer ways of unsubscribing.
codereader · Oct 28, 2021 · 74e6d3e · 74e6d3e
1 parent c813c21
commit 74e6d3e
Show file tree

Hide file tree

Showing 9 changed files with 86 additions and 88 deletions.
diff --git a/include/iundo.h b/include/iundo.h
@@ -92,30 +92,19 @@ class IUndoSystem
 	// it immediately from the stack, therefore it never existed.
 	virtual void cancel() = 0;
 
-    /**
-    * Observer implementation which gets notified
-    * on undo/redo operations.
-    */
-    class Tracker
+    enum class EventType
     {
-    public:
-        virtual ~Tracker() {}
-
-        // Invoked when a non-empty operation has been recorded by the undo system
-        virtual void onOperationRecorded(const std::string& operationName) = 0;
-
-        // Called when a single operation has been undone
-        virtual void onOperationUndone(const std::string& operationName) = 0;
-
-        // Called when a single operation has been redone
-        virtual void onOperationRedone(const std::string& operationName) = 0;
-
-        // Invoked when the undo and redo stacks have been cleared
-        virtual void onAllOperationsCleared() = 0;
+        OperationRecorded,
+        OperationUndone,
+        OperationRedone,
+        AllOperationsCleared,
     };
 
-	virtual void attachTracker(Tracker& tracker) = 0;
-	virtual void detachTracker(Tracker& tracker) = 0;
+    /**
+     * Emitted on edit/undo/redo and clear events, passes the operation type and name
+     * as arguments. Except for AllOperationsCleared, which will have an empty name argument.
+     */
+    virtual sigc::signal<void(EventType, const std::string&)>& signal_undoEvent() = 0;
 };
 
 class IUndoSystemFactory :

diff --git a/libs/UndoFileChangeTracker.h b/libs/UndoFileChangeTracker.h
@@ -6,7 +6,6 @@
 #include <sigc++/signal.h>
 
 class UndoFileChangeTracker :
-    public IUndoSystem::Tracker,
     public IMapFileChangeTracker
 {
 private:
@@ -44,33 +43,32 @@ class UndoFileChangeTracker :
         return _currentChangeCount;
     }
 
-    void onOperationRecorded(const std::string& operationName) override
+    void onUndoEvent(IUndoSystem::EventType type, const std::string& operationName)
     {
-        if (_currentChangeCount < _savedChangeCount)
+        switch (type)
         {
-            // redo queue has been flushed.. it is now impossible to get back to the saved state via undo/redo
-            _savedChangeCount = MAPFILE_MAX_CHANGES;
-        }
-
-        ++_currentChangeCount;
-        _changed.emit();
-    }
+        case IUndoSystem::EventType::OperationRecorded:
+            if (_currentChangeCount < _savedChangeCount)
+            {
+                // redo queue has been flushed.. it is now impossible to get back to the saved state via undo/redo
+                _savedChangeCount = MAPFILE_MAX_CHANGES;
+            }
+            ++_currentChangeCount;
+            break;
 
-    void onOperationUndone(const std::string& operationName) override
-    {
-        --_currentChangeCount;
-        _changed.emit();
-    }
-
-    void onOperationRedone(const std::string& operationName) override
-    {
-        ++_currentChangeCount;
-        _changed.emit();
-    }
+        case IUndoSystem::EventType::OperationUndone:
+            --_currentChangeCount;
+            break;
+
+        case IUndoSystem::EventType::OperationRedone:
+            ++_currentChangeCount;
+            break;
+
+        case IUndoSystem::EventType::AllOperationsCleared:
+            _currentChangeCount = 0;
+            break;
+        }
 
-    void onAllOperationsCleared() override
-    {
-        _currentChangeCount = 0;
         _changed.emit();
     }
 };
diff --git a/radiantcore/map/Map.cpp b/radiantcore/map/Map.cpp
@@ -94,6 +94,7 @@ void Map::connectToUndoSystem()
     _postUndoListener.disconnect();
     _postRedoListener.disconnect();
     _modifiedStatusListener.disconnect();
+    _undoEventListener.disconnect();
 
     _modifiedStatusListener = _resource->signal_modifiedStatusChanged().connect(
         [this](bool newStatus) { setModified(newStatus); }
@@ -109,6 +110,27 @@ void Map::connectToUndoSystem()
     {
         _mapPostRedoSignal.emit();
     });
+    _undoEventListener = _resource->getRootNode()->getUndoSystem().signal_undoEvent().connect(
+        sigc::mem_fun(this, &Map::onUndoEvent)
+    );
+}
+
+void Map::onUndoEvent(IUndoSystem::EventType type, const std::string& operationName)
+{
+    switch (type)
+    {
+    case IUndoSystem::EventType::OperationRecorded:
+        OperationMessage::Send(operationName);
+        break;
+
+    case IUndoSystem::EventType::OperationUndone:
+        OperationMessage::Send(fmt::format(_("Undo: {0}"), operationName));
+        break;
+
+    case IUndoSystem::EventType::OperationRedone:
+        OperationMessage::Send(fmt::format(_("Redo: {0}"), operationName));
+        break;
+    }
 }
 
 void Map::loadMapResourceFromPath(const std::string& path)
@@ -1479,6 +1501,7 @@ void Map::shutdownModule()
 {
     _postUndoListener.disconnect();
     _postRedoListener.disconnect();
+    _undoEventListener.disconnect();
 
     abortMergeOperation();
 

diff --git a/radiantcore/map/Map.h b/radiantcore/map/Map.h
@@ -2,6 +2,7 @@
 
 #include "inode.h"
 #include "imap.h"
+#include "iundo.h"
 #include "imapformat.h"
 #include "inamespace.h"
 #include "imapresource.h"
@@ -77,6 +78,7 @@ class Map :
     sigc::connection _postUndoListener;
     sigc::connection _postRedoListener;
     sigc::connection _modifiedStatusListener;
+    sigc::connection _undoEventListener;
 
     // Point trace for leak detection
     std::unique_ptr<PointFile> _pointTrace;
@@ -287,6 +289,8 @@ class Map :
 	void clearMapResource();
 	void connectToUndoSystem();
 
+    void onUndoEvent(IUndoSystem::EventType type, const std::string& operationName);
+
     void cleanupMergeOperation();
 
     /** greebo: Focus the XYViews and the Camera to the given point/angle.

diff --git a/radiantcore/map/RootNode.cpp b/radiantcore/map/RootNode.cpp
@@ -30,12 +30,14 @@ RootNode::RootNode(const std::string& name) :
     _undoSystem = GlobalUndoSystemFactory().createUndoSystem();
     assert(_undoSystem);
 
-    _undoSystem->attachTracker(*this);
+    _undoEventHandler = _undoSystem->signal_undoEvent().connect(
+        sigc::mem_fun(this, &RootNode::onUndoEvent)
+    );
 }
 
 RootNode::~RootNode()
 {
-    _undoSystem->detachTracker(*this);
+    _undoEventHandler.disconnect();
 
 	// Remove all child nodes to trigger their destruction
 	removeAllChildNodes();

diff --git a/radiantcore/map/RootNode.h b/radiantcore/map/RootNode.h
@@ -11,6 +11,7 @@
 #include "transformlib.h"
 #include "KeyValueStore.h"
 #include "undo/UndoSystem.h"
+#include <sigc++/connection.h>
 
 namespace map 
 {
@@ -29,7 +30,7 @@ class RootNode :
 	public scene::Node,
     public scene::IMapRootNode,
 	public IdentityTransform,
-    protected UndoFileChangeTracker,
+    public UndoFileChangeTracker,
 	public KeyValueStore
 {
 private:
@@ -51,6 +52,8 @@ class RootNode :
 
 	AABB _emptyAABB;
 
+    sigc::connection _undoEventHandler;
+
 public:
 	// Constructor, pass the name of the map to it
 	RootNode(const std::string& name);

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(command); }
+        _eventSignal.emit(EventType::OperationRecorded, command);
 	}
 }
 
@@ -95,7 +95,7 @@ void UndoSystem::undo()
 	operation->restoreSnapshot();
 	finishRedo(operationName);
 	_undoStack.pop_back();
-    for (auto tracker : _trackers) { tracker->onOperationUndone(operationName); }
+    _eventSignal.emit(EventType::OperationUndone, operationName);
 
 	_signalPostUndo.emit();
 
@@ -131,7 +131,7 @@ void UndoSystem::redo()
 	operation->restoreSnapshot();
 	finishUndo(operationName);
 	_redoStack.pop_back();
-    for (auto tracker : _trackers) { tracker->onOperationRedone(operationName); }
+    _eventSignal.emit(EventType::OperationRedone, operationName);
 
 	_signalPostRedo.emit();
 
@@ -150,7 +150,7 @@ void UndoSystem::clear()
 	setActiveUndoStack(nullptr);
 	_undoStack.clear();
 	_redoStack.clear();
-    for (auto tracker : _trackers) { tracker->onAllOperationsCleared(); }
+    _eventSignal.emit(EventType::AllOperationsCleared, std::string());
 
 	// greebo: This is called on map shutdown, so don't clear the observers,
 	// there are some "persistent" observers like EntityInspector and ShaderClipboard
@@ -167,16 +167,9 @@ sigc::signal<void>& UndoSystem::signal_postRedo()
 	return _signalPostRedo;
 }
 
-void UndoSystem::attachTracker(Tracker& tracker)
+sigc::signal<void(IUndoSystem::EventType, const std::string&)>& UndoSystem::signal_undoEvent()
 {
-	ASSERT_MESSAGE(_trackers.count(&tracker) == 0, "undo tracker already attached");
-	_trackers.insert(&tracker);
-}
-
-void UndoSystem::detachTracker(Tracker& tracker)
-{
-	ASSERT_MESSAGE(_trackers.count(&tracker) > 0, "undo tracker cannot be detached");
-	_trackers.erase(&tracker);
+    return _eventSignal;
 }
 
 void UndoSystem::startUndo()

diff --git a/radiantcore/undo/UndoSystem.h b/radiantcore/undo/UndoSystem.h
@@ -48,11 +48,11 @@ class UndoSystem final :
 
     registry::CachedKey<std::size_t> _undoLevels;
 
-    std::set<Tracker*> _trackers;
-
 	sigc::signal<void> _signalPostUndo;
 	sigc::signal<void> _signalPostRedo;
 
+    sigc::signal<void(EventType, const std::string&)> _eventSignal;
+
 public:
 	UndoSystem();
 	~UndoSystem();
@@ -81,8 +81,7 @@ class UndoSystem final :
 	// Emitted after a redo operation is fully completed, allows objects to refresh their state
 	sigc::signal<void>& signal_postRedo() override;
 
-	void attachTracker(Tracker& tracker) override;
-	void detachTracker(Tracker& tracker) override;
+    sigc::signal<void(EventType, const std::string&)>& signal_undoEvent() override;
 
 private:
 	void startUndo();

diff --git a/test/UndoRedo.cpp b/test/UndoRedo.cpp
@@ -1,5 +1,6 @@
 #include "RadiantTest.h"
 
+#include <sigc++/connection.h>
 #include "iundo.h"
 #include "ibrush.h"
 #include "ieclass.h"
@@ -757,8 +758,7 @@ TEST_F(UndoTest, MapUndoRedoSignalsWhenChangingMaps)
     EXPECT_TRUE(postRedoFired) << "Map didn't fire the post-redo signal after changing maps";
 }
 
-class TestUndoTracker :
-    public IUndoSystem::Tracker
+class TestUndoTracker
 {
 public:
     TestUndoTracker()
@@ -781,36 +781,23 @@ class TestUndoTracker :
         receivedOperationName.clear();
     }
 
-    void onOperationRecorded(const std::string& operationName) override
+    void onUndoEvent(IUndoSystem::EventType type, const std::string& operationName)
     {
-        recordedFired = true;
-        receivedOperationName = operationName;
-    }
+        recordedFired |= type == IUndoSystem::EventType::OperationRecorded;
+        undoneFired |= type == IUndoSystem::EventType::OperationUndone;
+        redoneFired |= type == IUndoSystem::EventType::OperationRedone;
+        clearedFired |= type == IUndoSystem::EventType::AllOperationsCleared;
 
-    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);
+    sigc::connection handler = GlobalMapModule().getRoot()->getUndoSystem()
+        .signal_undoEvent().connect(sigc::mem_fun(tracker, &TestUndoTracker::onUndoEvent));
 
     // Record one undoable operation in the existing map
     {
@@ -869,7 +856,7 @@ TEST_F(UndoTest, OperationTracking)
     EXPECT_FALSE(tracker.redoneFired) << "Wrong signal fired";
     EXPECT_EQ(tracker.receivedOperationName, "") << "Message not correct";
 
-    GlobalMapModule().getRoot()->getUndoSystem().detachTracker(tracker);
+    handler.disconnect();
     tracker.reset();
 
     // One more change after detaching