?
Solved

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

Posted on 2005-05-10
8
Medium Priority
?
340 Views
Last Modified: 2008-02-01
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();
  }
}


0
Comment
Question by:PMH4514
  • 5
  • 3
8 Comments
 
LVL 39

Accepted Solution

by:
itsmeandnobodyelse earned 500 total points
ID: 13969847
>>>> 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
 

Author Comment

by:PMH4514
ID: 13970018
ok I'll go give that a try

-Paul
0
 

Author Comment

by:PMH4514
ID: 13971343
perfect, thanks!
0
Technology Partners: We Want Your Opinion!

We value your feedback.

Take our survey and automatically be enter to win anyone of the following:
Yeti Cooler, Amazon eGift Card, and Movie eGift Card!

 

Author Comment

by:PMH4514
ID: 13977783
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
 
LVL 39

Expert Comment

by:itsmeandnobodyelse
ID: 13978128
I think

     m_CommandHistory.resize(m_iHistoryIndex+1);

should do it;

Regards, Alex
 
0
 

Author Comment

by:PMH4514
ID: 13978818
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
 
LVL 39

Expert Comment

by:itsmeandnobodyelse
ID: 13979454
>>>> 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
 

Author Comment

by:PMH4514
ID: 13979873
thanks alex
0

Featured Post

How to Use the Help Bell

Need to boost the visibility of your question for solutions? Use the Experts Exchange Help Bell to confirm priority levels and contact subject-matter experts for question attention.  Check out this how-to article for more information.

Question has a verified solution.

If you are experiencing a similar issue, please ask a related question

Written by John Humphreys C++ Threading and the POSIX Library This article will cover the basic information that you need to know in order to make use of the POSIX threading library available for C and C++ on UNIX and most Linux systems.   [s…
What is C++ STL?: STL stands for Standard Template Library and is a part of standard C++ libraries. It contains many useful data structures (containers) and algorithms, which can spare you a lot of the time. Today we will look at the STL Vector. …
The goal of the video will be to teach the user the concept of local variables and scope. An example of a locally defined variable will be given as well as an explanation of what scope is in C++. The local variable and concept of scope will be relat…
The viewer will learn additional member functions of the vector class. Specifically, the capacity and swap member functions will be introduced.
Suggested Courses

755 members asked questions and received personalized solutions in the past 7 days.

Join the community of 500,000 technology professionals and ask your questions.

Join & Ask a Question