Handle undoable operations in a correct way. Low-level algorithm shou…

…ldn't start undoable operations on their own, this is the responsibility of calling code. This fixes the crash occurring in ff33068.

Creating another undoable operation when a previous one is already opened will lead to an assertion now.

radiant/selection/algorithm/Group.cpp

@@ -549,6 +549,7 @@ void deleteAllSelectionGroupsCmd(const cmd::ArgumentList& args)
 		return;
 	}
 
+	UndoableCommand cmd("DeleteAllSelectionGroups");
 	getMapSelectionGroupManager().deleteAllSelectionGroups();
 }
 

radiant/selection/group/SelectionGroupManager.cpp

@@ -9,7 +9,6 @@
 #include "iselection.h"
 #include "ieventmanager.h"
 #include "imainframe.h"
-#include "iundo.h"
 #include "iorthocontextmenu.h"
 #include "modulesystem/StaticModule.h"
 #include "selection/algorithm/Group.h"
@@ -68,13 +67,6 @@ void SelectionGroupManager::setGroupSelected(std::size_t id, bool selected)
 }
 
 void SelectionGroupManager::deleteSelectionGroup(std::size_t id)
-{
-	UndoableCommand cmd("DeleteSelectionGroup");
-
-	doDeleteSelectionGroup(id);
-}
-
-void SelectionGroupManager::doDeleteSelectionGroup(std::size_t id)
 {
 	SelectionGroupMap::iterator found = _groups.find(id);
 
@@ -91,11 +83,9 @@ void SelectionGroupManager::doDeleteSelectionGroup(std::size_t id)
 
 void SelectionGroupManager::deleteAllSelectionGroups()
 {
-	UndoableCommand cmd("DeleteAllSelectionGroups");
-
 	for (SelectionGroupMap::iterator g = _groups.begin(); g != _groups.end(); )
 	{
-		doDeleteSelectionGroup((g++)->first);
+		deleteSelectionGroup((g++)->first);
 	}
 
 	assert(_groups.empty());

radiant/selection/group/SelectionGroupManager.h

@@ -35,8 +35,6 @@ class SelectionGroupManager :
 private:
 	std::size_t generateGroupId();
 	void resetNextGroupId();
-
-	void doDeleteSelectionGroup(std::size_t id);
 };
 
 }

radiant/undo/Stack.h

@@ -27,7 +27,8 @@ class UndoStack
 	// The list of Operations that can be undone
 	Operations _stack;
 
-	// The pending undo operation (a working variable, so to say)
+	// The pending undo operation (will be committed as soon as the first
+	// undoable saves its data to the stack)
 	OperationPtr _pending;
 
 public:
@@ -70,35 +71,39 @@ class UndoStack
 	// Allocate a new Operation to work with
 	void start(const std::string& command)
 	{
-		if (_pending)
-		{
-			_pending.reset();
-		}
+		// When starting an operation, we create one and declare it as pending
+		// It will not be added to the stack, it still might end up empty
+		// We will also replace any previously pending operation with the new one
+		// even though it should not happen by design
+		ASSERT_MESSAGE(!_pending, "undo operation already started");
 
-		_pending.reset(new Operation(command));
+		_pending = std::make_shared<Operation>(command);
 	}
 
 	// Finish the current undo operation
 	bool finish(const std::string& command)
 	{
 		if (_pending)
 		{
+			// The started operation has not been filled with any data
+			// so just discard it without doing anything
 			_pending.reset();
 			return false;
 		}
-		else 
-		{
-			// Rename the last undo operation (it was "unnamed" till now)
-			ASSERT_MESSAGE(!_stack.empty(), "undo stack empty");
-			_stack.back()->setName(command);
-			return true;
-		}
+
+		// 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;
 	}
 
 	// Store an Undoable into the last snapshot
 	void save(IUndoable& undoable)
 	{
-		// Check, if there is still a pending undo command around
+		// Check, if there is still a pending undo operation waiting to be added to the stack
 		if (_pending)
 		{
 			// Save the pending undo command