QUndoCommand calls Redo on initialization?
-
When I use the following code the initialization of the undo command calls redo. In other words, everything functions correctly except that DeleteNote::redo() is ALSO called when undoStack->push(delNote) is called. This routine has been rewritten several ways with the same result. I don't understand why redo is called and this is a problem. What am I doing wrong?
[CODE]
class DeleteNote : public QUndoCommand
{
public:
DeleteNote();
void undo();
void redo();};
[/CODE][CODE]
void Note::deleteNote()
{
ADb* db = new ADb;
int rowid = db->dbGetNotesInsertRowid();
qDebug()<< "Delete row:"<< rowid;
deleteNoteUndoCmd();
}
[/CODE][CODE]
void Note::deleteNoteUndoCmd()
{extern QUndoStack* undoStack; QUndoCommand* delNote = new DeleteNote(); delNote->setText("Delete Note"); undoStack->push(delNote);
}
[/CODE][CODE]
DeleteNote::DeleteNote()
{}
void DeleteNote::undo()
{
qDebug()<< "Undone.";}
void DeleteNote::redo()
{
qDebug()<< "RE-done.";}
[/CODE] -
Nothing: that's exactly how it works. The idea with the command pattern is that you create a Command object that does something when it is executed. So you create a command object that does what you want, then push it onto the QUndoStack, which executes it (by calling redo()).
-
Oh, OK, I guess you lose control over how you name your functions then if you use this method. Doesn't this make your code a lot less readable?
-
I guess that depends on what you mean by "less readable": undo/redo is an added bit of functionality, and you pay a price for having it. I don't quite follow your comment about function naming, however: of course the command object itself only has a few functions, and they are all virtual and inherited from QUndoCommand. But your note class itself can be anything you want, and call its functions whatever you prefer.
For example, I imagine you might have a button in a dialog that is connected to a DeleteNote() function in your class. That function looks like this:
@
void Note::DeleteNote ()
{
DeleteNoteCommand *dnc = new DeleteNoteCommand (this, this->selectedNote());
myUndoStack.push (dnc);
}
@
And that DeleteNoteCommand is something like:
@
void DeleteNoteCommand::redo ()
{
storedNoteDocumentThinger->DeleteANote (selectedNote);
}void DeleteNoteCommand::undo ()
{
storedNoteDocumentThinger->AddNote (selectedNote);
}
@ -
You make good points about the naming, but I guess what is at the core of my issue is that I assumed that it was set up so that I could easily have a separate undo process. For instance, I have a kinda complicated database routine to create a note, and a less complicated routine to delete and store the data for a roll back, and a drop-dead simple way to "redo" the delete.
Now I haven't put any thought into this at all yet but on first impression after reading your post it sounds like I need to have the same routine for creating the note as I have for re-doing the note. I'm sure it's not that way but the structure of this mechanism seems a bit awkward at least compared to the way I was understanding it.
-
It sounds to me like the situation you have is where it is expensive to execute the command the first time, but cheap any subsequent time, is that correct? If so, you could easily write the Command object to store whatever info you need so that if it's executed more than once it uses an optimized version the 2nd through n-th times.
-
Yeah, I'll have to think it through. I just think QT should change to work the way I want it to:0)
-
Of course :) -- modifying your way of thinking about executing commands to use the Command pattern takes some getting used to. But being able to encapsulate an action in an object makes undo/redo a breeze to implement: it's especially valuable if you have a lot of different actions and actions spread out amongst different widgets (and then you have multiple open documents each with their own undo stack, etc. etc.) It's really powerful, but of course that comes at a price.
-
Yes,, and you are right, this is new to me. I'm quite open to rethinking the whole thing. I've not considered the implications as I implement undo over the scope of the whole program, and I find this to be much simpler and more powerful than I thought it would be. There must be a reason for this approach I suppose. The obvious open approach to a free-and-clear redo must have a positive trade-off or you'd think they would have done it that way in the first place.
-
A decade or so ago I was on a team that implemented undo/redo for a complex program (this was before the days of undo being built into the toolkits like we have now): it was amazing how difficult it was to achieve the expected behavior! In the end we wound up with a system almost exactly like what Qt has implemented: it was the only paradigm that we could make work the way our users expected it in the case of multiple widgets and multiple windows. I agree that it's awkward to use for the simple cases, but once things get complex I think you'll see the value.
-
Your experience is very valuable and I appreciate it. I'm looking at my code now and trying to get my head around delete-undo-redo-add and figuring out how to keep my head from spinning. I have to redo the delete to add to the backup database to undo the add... yikes.
It is painful to think about redoing a delete that is undoing an add that hasn't been deleted yet and still needs to be backed up to two different databases!
But I take your points about creating a command object that can be reissued. I'm a noob and have to go slowly but I usually come through. I'll be working on what you've told me for a long time. But I'm still concerned about the flexibility of having a separate undo routine, but it's early yet and I'll figure out this command structure and it'll sort out.
-
If you have access to Eric Gamma et al.'s Design Patterns book, the chapter on the Command pattern may prove useful: that's sort of the definitive reference.
-
Thank you. I'm reserving some of my Christmas money for computer books.
-
Thanks for this interesting discussion. To be honest, I always found the QUndoRedo framework a bit awkward as well. For one, I find it a bit strange that you the first action performed is the redo action. There is nothing to redo before we first did an undo, in my book. And of course, we cannot do an undo before we did a do.
In my simple logic, it would make perfect sense to have three methods in a [[doc:QUndoCommand]]:
do, which would perform the initial action
undo, which would, well, undo it, and
redo, which would by default just call do, but could be reimplemented to be different if that makes sense (like it seems to do for devdon).
Of course, the class itself needs to change name to something like QAbstractCommand. Last, but not least, I think that all Qt widgets that currently have their own private undo/redo, should get a getter & setter for a [[doc:QUndoStack]] pointer, so that you can easily make their undo/redo actions part of an application-global undo/redo stack.
-
I spent all day rewriting to make it work under this mechanism. Now, I'm not complaining, because I did my homework (read up on wikipedia about the command pattern undo) and I see their point about creating an object that sets the present ahead a step then rolls it back. (see the article "Design Patterns ":http://en.wikipedia.org/wiki/Design_Patterns_(book) )
But I was still kinda unhappy and wanted a different way to do it. The wisdom of the philosophy still hasn't sunk in yet, I guess, or my case is different from the ordinary widget wrangle. I'm certainly not afraid of more work, I constantly rewrite this as it is, that's my interest in it. I just feel boxed in because I want a third option and I feel like this gives me only two. Flexibility is key and this is theoretically sound but perhaps more brittle than they think it is...
-
[quote author="Andre" date="1324284223"]Last, but not least, I think that all Qt widgets that currently have their own private undo/redo, should get a getter & setter for a [[doc:QUndoStack]] pointer, so that you can easily make their undo/redo actions part of an application-global undo/redo stack. [/quote]
Amen to that.