Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

Why does QUndoStack.push() execute QUndoCommand.redo()?

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_())
like image 911
musicamante Avatar asked Dec 14 '22 18:12

musicamante


2 Answers

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?

like image 61
Mitch Avatar answered Dec 17 '22 22:12

Mitch


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.

like image 25
dtech Avatar answered Dec 18 '22 00:12

dtech