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?

3 thoughts on “QUndoStack versus Kate’s Undo System”

  1. Imo, doing anything non-trivial in a dtor is a code smell. Esp. consider what would happen if you create a class inheriting from Node, lets say FancyNode. ~Node will be called _after_ ~FancyNode and you’ll thus never be able to “redo” the special functionality of FancyNode.

    Also, I doubt you _always_ want to add every Node to the stack you delete. There are quite probably cases where you rather want aggregated top-level items in there for example. This is e.g. something which is really bad in Kate, as tons of items are added to the undo stack without being aggregated which leads to overhead. Example: Remove ten lines, you get ten undo actions to remove a line instead of one action that removed ten lines.

    So yeah, I think having an explicit API is better here. The developer knows best where/when to create such undo/redo actions.

    1. With respect to your example: on deletion, ~FancyNode puts itself on the undo stack, then ~Node: so the stack is:
      set-fancy-properties, delete-node. In this case, undo would first create the node again, and than the fancy-properties are set again. So in this example the order even matches what you want.

      With respect to the deletion: Right, I don’t want a node to always go to the undo-stack: Only when it belongs to a Document. That is, in ~Node():
      if (document() && !document()->undoManager()->isRunning()) { /* push deletion to undo stack */ }
      So I have both: just always delete a node, and it does exactly what I want.

      Having an explicit API that only allows one way to use it is probably indeed good. However, it could also have been
      QUndoStack::push(QUndoCommand * cmd, bool execute = false);
      Further, QUndoStack::isRunning() is trivial to implement, just some very few lines of code.
      But yeah, it was simply decided to implement it this way, and I’m not saying it’s wrong. But it forces me to use the undo stack in a different way I wanted. And it also has implications for more stuff of my code. So it’s simply not super-cool ;)

      I’m also not saying that Kate’s undo system is the way to go. But it works perfectly fine and we have everywhere good control over what happens and if the undo stack is operating. In fact, that’s heavily used in quite some places.

Leave a Reply