4

I created a custom QPushButton that allows to pick a color from a menu or a QColorDialog. Since it's part of a "theme" editor, I also added support for a QUndoStack: every time the color of a custom button is changed, it creates a subclass of QUndoCommand, which is pushed to the QUndoStack.
Then I realized (according to this) that every time I do a QUndoStack.push(cmd), that command is executed (which obviously created a recursion, automatically "ignored" by PyQt, but still reported in the stdout).

I solved the problem by blocking signals on the target widget when redo() is called, but the question still remains: why is a pushed command executed at first?

From my point of view, if I push a command to the undo stack, it has already been executed.
What is the case scenario for which a (theoretically already) executed [subclassed] QUndoCommand has to be executed "again"?
Is that scenario that common that it requires this kind of implementation as the default behaviour?

Following, a minimal and incomplete (but enough to show the issue) example; it doesn't support signal handling for undo/redo actions, but that's not the point (I think?). As far as I can understand, the problem comes when the signal calling the QUndoCommand creation coincides with the slot creating the signal itself:

#!/usr/bin/env python2

import sys
from PyQt5 import QtCore, QtGui, QtWidgets

class UndoCmd(QtWidgets.QUndoCommand):
    def __init__(self, widget, newColor, oldColor):
        QtWidgets.QUndoCommand.__init__(self)
        self.widget = widget
        self.newColor = newColor
        self.oldColor = oldColor

    def redo(self):
        self.widget.color = self.newColor


class ColorButton(QtWidgets.QPushButton):
    colorChanged = QtCore.pyqtSignal(QtGui.QColor)
    def __init__(self, parent=None):
        QtWidgets.QPushButton.__init__(self, 'colorButton', parent)
        self.oldColor = self._color = self.palette().color(self.palette().Button)
        self.clicked.connect(self.changeColor)

    @QtCore.pyqtProperty(QtGui.QColor)
    def color(self):
        return self._color

    @color.setter
    def color(self, color):
        self._color = color
        palette = self.palette()
        palette.setColor(palette.Button, color)
        self.setPalette(palette)
        self.colorChanged.emit(color)

    def changeColor(self):
        dialog = QtWidgets.QColorDialog()
        if dialog.exec_():
            self.color = dialog.selectedColor()

class Window(QtWidgets.QWidget):
    def __init__(self):
        QtWidgets.QWidget.__init__(self)
        layout = QtWidgets.QHBoxLayout()
        self.setLayout(layout)
        self.colorButton = ColorButton()
        layout.addWidget(self.colorButton)
        self.undoStack = QtWidgets.QUndoStack()
        self.colorButton.colorChanged.connect(lambda: self.colorChanged(self.colorButton.oldColor))

    def colorChanged(self, oldColor):
        self.undoStack.push(UndoCmd(self.colorButton, oldColor, self.colorButton._color))


app = QtWidgets.QApplication(sys.argv)
w = Window()
w.show()
sys.exit(app.exec_())
Community
  • 1
  • 1
musicamante
  • 41,230
  • 6
  • 33
  • 58
  • The best way to understand and solve this type of problem is to provide a [mcve] :D – eyllanesc Mar 05 '18 at 03:02
  • I'm preparing an example that I'll add to the answer, but I don't think it's an "actual" problem: as far as I understand, that's an implementation choice, don't you think? – musicamante Mar 05 '18 at 03:27
  • I think you have a typical problem: why my code does not work?, you are describing a problem that arises from your implementation, I do not know if your implementation is good or bad, so I ask you to publish a [mcve], in rare cases you must use blockSignals, so I ask for a [mcve] – eyllanesc Mar 05 '18 at 03:32
  • 2
    According to [the example](http://doc.qt.io/qt-5/qtwidgets-tools-undoframework-example.html): _"An undo command class knows how to both redo() - or just do the first time - and undo() an action."_, i.e. redo is equal to "do", so to say. To be honest I was also surprised when I first used the undo framework (a couple of years ago), but that's the way it is. So, to answer your question - because it is designed like that. I presume it has something to do with a particullar design pattern. Take a look at the [command pattern](https://www.youtube.com/watch?v=9qA5kw8dcSU). – scopchanov Mar 05 '18 at 03:33
  • @eyllanesc: I updated the answer with an example, as minimal as possible, thank you. @scopchanov I could understand that, my question is not really about "how to fix it" (I can easily understand it), but *why*. I get that a signal calling an QUndoCommand should (could?) not be the same as the slot that the command calls, but, I still don't understand the necessity of executing the command after a `push`. – musicamante Mar 05 '18 at 04:19
  • 2
    _"From my point of view, if I push a command to the undo stack, it has already been executed."_ I've checked my code and the [example](http://doc.qt.io/qt-5/qtwidgets-tools-undoframework-example.html) to be sure that I remember correctly. In both places I see that new commands are being pushed, e.g. `undoStack->push(new MoveCommand(movedItem, oldPosition));`. So, the command is intended to be executed by being pushed to the undo stack and not beforehand, hence _"the necessity of executing the command after a push."_ – scopchanov Mar 05 '18 at 06:36
  • 1
    @scopchanov Ok, I think I'm getting it. Thank you for the video. When I previously used the QUndoStack it was a simpler situation (spinbox changes) and didn't care much about the execution after push, so I completely forgot about that. I can understand the pattern, I simply didn't considered it in that way. – musicamante Mar 06 '18 at 10:17

2 Answers2

7

This is described in the overview documentation:

Qt's Undo Framework is an implementation of the Command pattern, for implementing undo/redo functionality in applications.

The Command pattern is based on the idea that all editing in an application is done by creating instances of command objects. Command objects apply changes to the document and are stored on a command stack. Furthermore, each command knows how to undo its changes to bring the document back to its previous state. As long as the application only uses command objects to change the state of the document, it is possible to undo a sequence of commands by traversing the stack downwards and calling undo on each command in turn. It is also possible to redo a sequence of commands by traversing the stack upwards and calling redo on each command.

To use the undo framework, you should ensure that any action that should be undoable is only performed by a QUndoCommand subclass.

So I'm assuming that you're performing the action twice: once directly and then as a result of that, once through the undo framework. Instead, you should only perform the action through the undo framework.

The reason why pushed actions are immediately redone is probably for simplicity: the redo() function already conveniently performs the action, so why add another function to do the same thing?

Mitch
  • 23,716
  • 9
  • 83
  • 122
  • The "problem" is that the widget itself performs the action of changing its colour, but I was actually thinking it in a much simpler (and somehow wrong) way. Now I'm getting the concept much better, and makes sense. Thanks. – musicamante Mar 06 '18 at 10:29
  • I had this same confusion. I think for it to make any sense you have to have a model / database / state variable / document that exists separate from the UI. Whenever changes are made in the UI, a command is generated that updates the state. Then whenever the state is changed, like with an Undo operation, the UI would need to be synchronized with the current application state. I think this is kinda like the Model-View stuff Qt and other UI libs use to separate UI from data. This at least gives the command something to do and stores your state somewhere besides the UI itself. – flutefreak7 Sep 19 '18 at 16:51
6

redo() is synonymous to do(), don't let the "re" part confuse you. The API has two key functions, one that applies a command and one that reverses it.

There is also a suboptimal choice of wording, as the undo stack should really be called the command stack.

Therefore it is only logical that a command is applied as it is added to the command stack. The command should not be applied manually before that point.

dtech
  • 47,916
  • 17
  • 112
  • 190
  • 1
    Yes. What confused me is that I imagined in a simpler way, such as a text editing, for which you ideally create undo actions once the text has already been changed by the user typing, and the change is not performed by the pushed undo command. – musicamante Mar 06 '18 at 10:24