QUndoStack versus Kate’s Undo System

I’m currently using QUndoStack in a project of mine. A QUndoStack contains a list of QUndoCommands which then can be executed with QUndoStack::undo() and QUndoStack:.redo().

Now I thought when deleting an instance of the class Node, I can just hock the creation of the respective UndoDeleteNode class (derived from QUndoCommand) into the destructor, like this:

class Node {
public:
  // ...
  ~Node() {
    undoStack()->push(new UndoDeleteNode(this));
  }
};

class UndoDeleteNode : public QUndoCommand {
public:
  // ...
  void undo() {
    // create node again
  }
  void redo() {
    // delete node again
  }
};

Now the problem with this code is, that executing UndoDeleteNode::redo() will delete the node through the undo/redo system. In this case, the destructor ~Node() will create another UndoDeleteNode item. So what I was looking for a way to only create the undo item when the undo/redo system is not active, i.e.:

class Node {
public:
  // ...
  ~Node() {
    if (!undoStack()->undoOrRedoRunning()) {
      undoStack()->push(new UndoDeleteNode(this));
    } 
  }
};

I found QUndoStack::isActive(), but isActive() does something completely different. In fact, it looks as if there is no way to tell that the QUndoStack is currently operating. This finally also gets clear by reading the API docs of QUndoStack::push(QUndoCommand* cmd):

[...] Executes cmd by calling its redo() function. [...]

In other words, each QUndoCommand you push on the stack is executed immediately. This obviously is a design decision: Following this paradigm, you should not just call “delete node;” Instead, you should simply create a UndoDeleteNode() instance and push it to the stack. The undo command is then immediately executed by calling redo().

This design has the advantage that developers are forced to go this route. Following this paradigm, you very easily get macro support, since all actions are undo/redo items. This is cool.

However, what I dislike about this approach is the fact that it seems you loose the nice API to simply delete the node. You cannot write “delete node;”. Instead, you have to have some helper class such as a Document, that provides a Document::deleteNode(Node* node) function. This function then just creates a new UndoDeleteNode and pushes it to the stack.

Is this how the QUndoStack is supposed to be used? Does it imply that the destructor should be protected, and UndoDeleteNode must be a friend class of Node, to avoid other classes from just calling delete Node?

In Kate, we indeed go the other way: We have a KateUndoManager::isActive(), which indicates whether the edit actions should be tracked by the undo manager or not…

I’m not yet convinced that the approach of QUndoStack is ultimately cool. To me it looks like I’m forced into some design decision I didn’t want to take. Maybe I’m missing something?

QUndoStack versus Kate’s Undo System

See also: