What's wrong with this code? (undo/redo architecture)

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;

public:
  CMyCommand* GetCommand(int a_iCommandID); // instantiates and returns appropriate type
  void Execute(CMyCommand* a_pCommand);
  void Undo();
  void Redo();

private:
  void CCommandInterface::ClearHistoryForward();

  CMyCommandList   m_CommandHistory;
  POSITION         m_HistoryPOS;


-- code ---

CCommandInterface::CCommandInterface()
{
  m_instance  = this;
  m_HistoryPOS = m_CommandHistory.GetHeadPosition();
}

void CCommandInterface::Execute(CMyCommand* a_pCommand)
{
  if (!m_CommandHistory.IsEmpty())
    ClearHistoryForward();

  m_CommandHistory.AddTail(a_pCommand);
  m_HistoryPOS = m_CommandHistory.GetTailPosition();
  a_pCommand->Execute();
}

void CCommandInterface::ClearHistoryForward()
{
  // 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); )
  {  
    delete m_CommandHistory.GetTail();
  }
}

void CCommandInterface::Undo()
{
  if (!m_CommandHistory.IsEmpty())
  {
    CMyCommand* pLastCommand = m_CommandHistory.GetPrev(m_HistoryPOS);
    if (pLastCommand)
      pLastCommand->Undo();
  }
}

void CCommandInterface::Redo()
{
  if (!m_CommandHistory.IsEmpty())
  {
    CMyCommand* pLastCommand = m_CommandHistory.GetNext(m_HistoryPOS);
    if (pLastCommand)
      pLastCommand->Redo();
  }
}


PMH4514Asked:
Who is Participating?
 
itsmeandnobodyelseCommented:
>>>> m_HistoryPOS

The problems might come because of this. A POSITION is a temporary that shouldn't be stored as it might get invalid when the list grows.

I would recommend to use an array and array indices rather than a list and positions. Even when using dynamic array containers like std::vector or CPtrArray, indices are valid as long as you are not deleting previous elements. In case of removing items you easily could update saved indices as well.

Regards, Alex
0
 
PMH4514Author Commented:
ok I'll go give that a try

-Paul
0
 
PMH4514Author Commented:
perfect, thanks!
0
Cloud Class® Course: MCSA MCSE Windows Server 2012

This course teaches how to install and configure Windows Server 2012 R2.  It is the first step on your path to becoming a Microsoft Certified Solutions Expert (MCSE).

 
PMH4514Author Commented:
actually if I could just followup to this - I'll make a new thread/question if you like, but I changed to use a vector, and an int to represent the position. my undo/redo is working well, but I'm having a problem figuring out how to implement my "clear from index forward":


      std::vector<CMyCommand*> m_CommandHistory;

void CCommandInterface::ClearHistoryForward()
{
      for(int i=(m_iHistoryIndex+1);i<m_CommandHistory.size();)
      {
            delete m_CommandHistory[i++];
      }
}

that deletes the CMyCommand instances, but it doesn't erase from the list.  It looks like vector.erase() requires an iterator? do I have to use an iterator object or am I just missing something here?

thanks!
-Paul
0
 
itsmeandnobodyelseCommented:
I think

     m_CommandHistory.resize(m_iHistoryIndex+1);

should do it;

Regards, Alex
 
0
 
PMH4514Author Commented:
will there be any performance hits using resize(x)  when you keep in mind that following this, new items will continue to be pushed on the back?
0
 
itsmeandnobodyelseCommented:
>>>> will there be any performance hits using resize(x)  

No, all implementations I have knowledge of simply set the length counter to the required size. The allocation size and the internal buffer will not be changed.

Regards, Alex
0
 
PMH4514Author Commented:
thanks alex
0
Question has a verified solution.

Are you are experiencing a similar issue? Get a personalized answer when you ask a related question.

Have a better answer? Share it in a comment.

All Courses

From novice to tech pro — start learning today.