What's wrong with this code? (undo/redo architecture)
Posted on 2005-05-10
I'm using a CTypedPtrList to implement a list of instnaces of CMyCommand - basically the heart of my undo/redo architecture.
It seems to work almost right, but I get weird crashes at times, I'll be able to undo/redo a couple times and execute new, but then it will crash, the POSITION gets nulled out for some reason, so I wanted to get some extra eyes on my code to see what I'm missing.
As an overview of my thought process:
CCommandInterface is the singleton from which my other classes request instances of CMyCommand based on UI actions. The CMyCommand instances are provided paramaters by the calling code as /if necessary, and are pushed back to CCommandInterface::Execute(CMyCommand* pCommand) for execution. CCommandInterface::Undo() and CCommandInterface::Redo() exist to go back and forth on my list executing Undo() or Redo() on each CMyCommand.
So to make this work, my thought is that the list starts empty. When a command is executed, I add it to the tail position. When Undo() is called, I need to call Undo() on the CMyCommand instance in the tail position, and move the cursor back one. I do this with GetPrv() (I'll show all code in a bit) - an immediate call to Redo() would simply call Redo() on the CMyCommand instance at the cursor.
Now, if my POSITION cursor is somewhere in the middle of the list (ie. several successive Undo()'s were executed) and an Execute() is requested, I need to clear everything from the cursor to the end of the list and then add the newly executed instance to the tail position.
So that is my thought. I think the premise is correct, I just must not be checking for the right things and such. Here is the code:
--- header ---
typedef CTypedPtrList<CPtrList, CMyCommand*> CMyCommandList;
CMyCommand* GetCommand(int a_iCommandID); // instantiates and returns appropriate type
void Execute(CMyCommand* a_pCommand);
-- code ---
m_instance = this;
m_HistoryPOS = m_CommandHistory.GetHeadPosition();
void CCommandInterface::Execute(CMyCommand* a_pCommand)
m_HistoryPOS = m_CommandHistory.GetTailPosition();
// private helper method to empty everything off
// the end of the list that has been undone, now that we
// are executing something new.
POSITION pos = NULL;
for( pos = m_CommandHistory.GetTailPosition(); (pos != NULL && pos != m_HistoryPOS); )
CMyCommand* pLastCommand = m_CommandHistory.GetPrev(m_HistoryPOS);
CMyCommand* pLastCommand = m_CommandHistory.GetNext(m_HistoryPOS);