From c813c2120a865b5162652bc3b7990adffbdc9a50 Mon Sep 17 00:00:00 2001 From: codereader Date: Wed, 27 Oct 2021 20:20:22 +0200 Subject: [PATCH] #5408: Extend the IUndoSystem::Tracker interface to include the operation name --- include/iundo.h | 6 +- libs/UndoFileChangeTracker.h | 6 +- radiantcore/undo/UndoSystem.cpp | 16 ++-- test/UndoRedo.cpp | 128 ++++++++++++++++++++++++++++++++ 4 files changed, 143 insertions(+), 13 deletions(-) diff --git a/include/iundo.h b/include/iundo.h index 81ac01fb7a..b7e2f66d30 100644 --- 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 index 2986aa3c2e..2302bbbdef 100644 --- 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 index 0a51918d44..545d1c5e06 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(); } + 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 index 67944410b2..b56a46a2f7 100644 --- 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"; +} + }