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_())
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?
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.
If you love us? You can donate to us via Paypal or buy me a coffee so we can maintain and grow! Thank you!
Donate Us With