From 74e6d3e47cd8fe535981b0cb5d582c3183d9f0ec Mon Sep 17 00:00:00 2001 From: codereader Date: Thu, 28 Oct 2021 21:22:01 +0200 Subject: [PATCH] #5408: Remove IUndoSystem::Tracker interface in favour of a sigc++ signal, which has safer ways of unsubscribing. --- include/iundo.h | 31 +++++++--------------- libs/UndoFileChangeTracker.h | 46 ++++++++++++++++----------------- radiantcore/map/Map.cpp | 23 +++++++++++++++++ radiantcore/map/Map.h | 4 +++ radiantcore/map/RootNode.cpp | 6 +++-- radiantcore/map/RootNode.h | 5 +++- radiantcore/undo/UndoSystem.cpp | 19 +++++--------- radiantcore/undo/UndoSystem.h | 7 +++-- test/UndoRedo.cpp | 33 +++++++---------------- 9 files changed, 86 insertions(+), 88 deletions(-) diff --git a/include/iundo.h b/include/iundo.h index b7e2f66d30..31956dc03c 100644 --- 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& signal_undoEvent() = 0; }; class IUndoSystemFactory : diff --git a/libs/UndoFileChangeTracker.h b/libs/UndoFileChangeTracker.h index 2302bbbdef..21452550b5 100644 --- a/libs/UndoFileChangeTracker.h +++ b/libs/UndoFileChangeTracker.h @@ -6,7 +6,6 @@ #include 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 index 444d7397a4..e109805d03 100644 --- 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 index dffb174182..21f96b6577 100644 --- 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 _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 index 77ed280993..4ce92ac3a2 100644 --- 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 index a0a3a6c297..79ab3ec4da 100644 --- 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 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 index 545d1c5e06..4aeefc7651 100644 --- 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& UndoSystem::signal_postRedo() return _signalPostRedo; } -void UndoSystem::attachTracker(Tracker& tracker) +sigc::signal& 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 index 5414d104d4..7d334ae892 100644 --- a/radiantcore/undo/UndoSystem.h +++ b/radiantcore/undo/UndoSystem.h @@ -48,11 +48,11 @@ class UndoSystem final : registry::CachedKey _undoLevels; - std::set _trackers; - sigc::signal _signalPostUndo; sigc::signal _signalPostRedo; + sigc::signal _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& signal_postRedo() override; - void attachTracker(Tracker& tracker) override; - void detachTracker(Tracker& tracker) override; + sigc::signal& signal_undoEvent() override; private: void startUndo(); diff --git a/test/UndoRedo.cpp b/test/UndoRedo.cpp index b56a46a2f7..f296550329 100644 --- a/test/UndoRedo.cpp +++ b/test/UndoRedo.cpp @@ -1,5 +1,6 @@ #include "RadiantTest.h" +#include #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