#5408: Some refactoring

radiantcore/undo/Operation.h

@@ -26,13 +26,16 @@ class Operation
 		IUndoMementoPtr _data;
 
 	public:
-		// Constructor
-		UndoableState(IUndoable& undoable) :
-			_undoable(undoable),
-			_data(_undoable.exportState())
-		{}
+        UndoableState(IUndoable& undoable) :
+            _undoable(undoable),
+            _data(_undoable.exportState())
+        {}
 
-		void restoreState()
+        // Noncopyable
+        UndoableState(const UndoableState& other) = delete;
+        UndoableState& operator=(const UndoableState& other) = delete;
+
+		void restore()
 		{
 			_undoable.importState(_data);
 		}
@@ -45,7 +48,8 @@ class Operation
 	std::string _command;
 
 public:
-	// Constructor
+    using Ptr = std::shared_ptr<Operation>;
+
 	Operation(const std::string& command) :
 		_command(command)
 	{}
@@ -60,21 +64,26 @@ class Operation
 		_command = name;
 	}
 
+    bool empty() const
+    {
+        return _snapshot.empty();
+    }
+
 	void save(IUndoable& undoable)
 	{
 		// Record the state of the given undable and push it to the snapshot
-		// The order is relevant, we use push_front()
-		_snapshot.push_front(UndoableState(undoable));
+		// The order is relevant, we add to the front
+		_snapshot.emplace_front(undoable);
 	}
 
 	void restoreSnapshot()
 	{
-		for (auto& undoablePlusMemento : _snapshot)
+        // Walk through the snapshot front-to-back, the most recently added one is at the front
+		for (auto& state : _snapshot)
 		{
-			undoablePlusMemento.restoreState();
+            state.restore();
 		}
 	}
 };
-typedef std::shared_ptr<Operation> OperationPtr;
 
-} // namespace undo
+} // namespace

radiantcore/undo/Stack.h

@@ -14,22 +14,19 @@ namespace undo
  * Each named operation in this stack contains a snapshot of the undoable objects
  * that have been touched between start() and finish().
  *
- * When start() is called, a new Operation is allocated  and 
- * on calling save(Undoable*) the Undoable is actually stored within
- * the allocated Operation. The method finish() deallocates the
- * memory used in this timespan.
+ * On start() a new Operation is allocated  and on save(IUndoable) 
+ * the IUndoable's memento is actually stored within the allocated Operation. 
+ * The method finish() commits the operation to the stack.
  */
 class UndoStack
 {
-	//! Note: using std::list instead of vector/deque, to avoid copying of undos
-	typedef std::list<OperationPtr> Operations;
-
+private:
 	// The list of Operations that can be undone
-	Operations _stack;
+	//! Note: using std::list instead of vector/deque, to avoid copying of undos
+    std::list<Operation::Ptr> _stack;
 
-	// The pending undo operation (will be committed as soon as the first
-	// undoable saves its data to the stack)
-	OperationPtr _pending;
+	// The pending undo operation (will be committed on finish, if not empty)
+    Operation::Ptr _pending;
 
 public:
 
@@ -43,12 +40,12 @@ class UndoStack
 		return _stack.size();
 	}
 
-	const OperationPtr& back() const
+	const Operation::Ptr& back() const
 	{
 		return _stack.back();
 	}
 
-	const OperationPtr& front() const 
+	const Operation::Ptr& front() const
 	{
 		return _stack.front();
 	}
@@ -83,38 +80,28 @@ class UndoStack
 	// Finish the current undo operation
 	bool finish(const std::string& command)
 	{
-		if (_pending)
+		if (!_pending || _pending->empty())
 		{
 			// The started operation has not been filled with any data
 			// so just discard it without doing anything
 			_pending.reset();
 			return false;
 		}
 
-		// Reaching this point we don't have a *pending* operation
-		// but we need to make sure we have *any* operation at all
-		ASSERT_MESSAGE(!_stack.empty(), "undo stack empty");
-
 		// Rename the last undo operation (it may be "unnamed" till now)
-		_stack.back()->setName(command);
-		return true;
-	}
+        _pending->setName(command);
 
-	// Store an Undoable into the last snapshot
-	void save(IUndoable& undoable)
-	{
-		// Check, if there is still a pending undo operation waiting to be added to the stack
-		if (_pending)
-		{
-			// Save the pending undo command
-			_stack.push_back(_pending);
-			_pending.reset();
-		}
-
-		// Save the UndoMemento of the most recently added command into the snapshot
-		back()->save(undoable);
+        // Move the pending operation into its place
+        _stack.emplace_back(std::move(_pending));
+		return true;
 	}
 
-}; // class UndoStack
+    // Store an Undoable's state into the active operation
+    void save(IUndoable& undoable)
+    {
+        assert(_pending);
+        _pending->save(undoable);
+    }
+};
 
-} // namespace undo
+} // namespace

radiantcore/undo/StackFiller.h

@@ -38,15 +38,16 @@ class UndoStackFiller final :
 
     void saveState() override
     {
-        if (_stack != nullptr)
-        {
-            // Export the Undoable's memento
-            _stack->save(_undoable);
+        // If the stack reference is empty, the associated undoable 
+        // already submitted its state this round
+        if (_stack == nullptr) return;
 
-            // Make sure the stack is dissociated after saving
-            // to make sure further save() calls don't have any effect
-            _stack = nullptr;
-        }
+        // Export the Undoable's memento
+        _stack->save(_undoable);
+
+        // Make sure the stack is dissociated after saving
+        // to make sure further saveState() calls don't have any effect
+        _stack = nullptr;
     }
 
     IUndoSystem& getUndoSystem() override

radiantcore/undo/UndoSystem.cpp

@@ -87,7 +87,7 @@ void UndoSystem::undo()
         return;
     }
 
-	const OperationPtr& operation = _undoStack.back();
+	const auto& operation = _undoStack.back();
 	rMessage() << "Undo: " << operation->getName() << std::endl;
 
 	startRedo();
@@ -122,7 +122,7 @@ void UndoSystem::redo()
         return;
     }
 
-	const OperationPtr& operation = _redoStack.back();
+	const auto& operation = _redoStack.back();
 	rMessage() << "Redo: " << operation->getName() << std::endl;
 
 	startUndo();

test/UndoRedo.cpp

@@ -24,6 +24,18 @@ TEST_F(UndoTest, UndoSystemFactory)
     EXPECT_TRUE(GlobalUndoSystemFactory().createUndoSystem()) << "Undo system factory must not return empty references";
 }
 
+TEST_F(UndoTest, EmptyOperation)
+{
+    EXPECT_FALSE(GlobalMapModule().isModified()) << "Map modified without doing anything";
+
+    // Open an undo operation and finish it without doing anything
+    {
+        UndoableCommand cmd("test");
+    }
+
+    EXPECT_FALSE(GlobalMapModule().isModified()) << "Map should still be modified";
+}
+
 TEST_F(UndoTest, BrushCreation)
 {
     std::string mapPath = "maps/simple_brushes.map";