RFC: Design for undo/redo support in drgeo/* code...

["Hilaire Fernandes" <hilaire@ofset.org>]

Sorry to reply to this message a bit late.


 > figures.  We want undo/redo for all kinds of operations on a geometric
 > figure, and most notably:
 > 
 >  * Creation of new item in the figure.
 >  * Changing the attributes of one item (color, style, name).
 >  * Moving an item on the figure.
 >  * Removing an item from the figure.
 >  * Running a macro.
 > 
 > We also want to support potentially unlimited number of undo/redo.
 > 
 > What I first recommend is that the following methods are implemented
 > in the drgeoFigure class:
 > 
 >  * createItem ()
 > 
 >  * setItemColor ()
 > 
 >  * setItemDrawStyle ()
 > 
 >  * setItemName ()
 > 
 >  * runMacro ()
 > 
 >  * removeItem ()
 > 

Should a method moveToItem be implemented also or do you left it out
for some reason? Also some of the action below (macro, shape stuff)
actually take place outside the drgeoFigure class in a drgeoDrawable
class, so there is design issue there. The action performed in the
drgeoDrawable methods should be move inside these functions so that
there is no dependences against the drawable concerning this methods.
We just need to revert the dependencie (take a look in
drgeo_gtkmacro.cc around line 590 in method setColor)

But what happen when a user edit a free value? We need a setItemValue
method there. So instead of setItemColor, setItemDrawStyle,
setItemName & setItemValue why not using a more generic function
setItemAttribute. For exemple setItemAttribute ("color", "red"),
setItemAttribute ("value", "3.2") , setItemAttribute ("shape",
"cross"), etc... 
 

 > Implementing these methods will also help support scripting as all the
 > functionality of the Dr. Geo code is accessible through these methods.
 
Sure and we need to talk about that. A couple of days ago I got a
discussion with George to remplement genius using CLISP (not CLIPS,
but I will take to you about this one in a short future). It's look
George may be remotivated working with CLISP and this will allow the
drgeo code top be scriptable with CLIPS.

 
 > When running one of these commands we must in some way remember the
 > state of the figure before the execution of the command, so that we
 > are able to undo the change.  We also need to remember the parameters
 > of the method, so that we are able to redo the change.
 
If I get it right with your design, the prameters of the method will
be kept in the object reference corresponding to the method
(removeItem method and drgeoCommandRemoveItem object below) ?

 
 > As a complement to these public methods corresponding private methods
 > implementing just the functionality without taking care of undo/redo.
 > These private methods are used to perform the real work.  I propose
 > that they are named as follow:
 > 
 >  * _createItem ()
 > 
 >  * _setItemColor ()
 > 
 >  * _setItemDrawStyle ()
 > 
 >  * _setItemName ()
 > 
 >  * _runMacro ()
 > 
 >  * _removeItem ()
 > 
 > Based on an idea exposed in the book I cited in the introduction I
 > suggest that we define the following class:
 > 
 >   class drgeoCommand {
 >   public:
 >     virtual ~drgeoCommand ();
 > 
 >     virtual void Execute ();
 >     virtual void Unexecute ();
 >   protected:
 >     drgeoCommand ();
 >   };
 > 
 > And now here is an example of how to use that class based on the
 > example of the command "removeItem()".  First we have to sub-class the
 > drgeoCommand class:
 > 
 >   class drgeoRemoveItemCommand {
 >   public:
 >     ~drgeoRemoveItemCommand ();
 >     drgeoRemoveItemCommand (drgeoFigure *aFigure, const char *aItem);
 > 
 >     void Execute ();
 >     void Unexecute ();
 >   private:
 >     // We keep all the information for undo/redo in the private part.
 >     drgeoFigure *mFigure;
 >     char *mItem;
 >     XXX *mBackup;
 >   };
 > 
 > Here is now a start of implementation of the methods of this class:
 > 
 >   drgeoRemoveItemCommand::~drgeoRemoveItemCommand ()
 >   {
 >     if (mBackup)
 >       delete mBackup;
 >   }
 > 
 >   drgeoRemoveItemCommand::drgeoRemoveItemCommand (drgeoFigure *aFigure,
 > 						  const char *aItem)
 >   {
 >     mFigure = aFigure;
 >     mItem = aItem;
 >     mBackup = NULL;
 >   }
 > 
 >   void drgeoRemoveItemCommand::Execute ()
 >   {
 >     // Before removing the item, we must save data about it, so that
 >     // it can be restored later.  Is suggest to keep information about
 >     // it as an XML data structure.
 >     mBackup = mFigure->saveItem (mItem);
 > 
 >     mFigure->_removeItem (mItem);
 >   }
 > 
 >   void drgeoRemoveItemCommand::Unexecute ()
 >   {
 >     // We have to re-create the item from the data we have previously
 >     // saved.  I suggest that we use an XML data structure.
 > 
 >     mFigure->restoreItem (mBackup);
 >   }
 > 


What is the meaning of m in mItem, mFigure, ...

Do we need a reference to the drgeoFigure, it can be passed as a
parameter. This is just to keep size to a minimum of the undoItem.
Well this is 4/8 octets, I one have an undo list of 100 items, this
make just 400/800 of difference, not a big deal...  About the backup
we can probably use the save() method of the geometricObject



 > And finally here is how it is used in the implementation of
 > drgeoFigure::removeItem():
 > 
 >   void drgeoFigure::removeItem (char *aItem)
 >   {
 >     // Create the command object.
 >     drgeoCommand *command = new drgeoRemoveItemCommand (this, aItem);
 > 
 >     // Run the command.
 >     command->Execute ();
 > 
 >     // We'll have to add code here to keep track of this command in an
 >     // history.  Read more about the history mechanism below.
 >   }
 > 

One issue I saw there is about  the aItem argument



 > When implementing the saveItem() and restoreItem() methods special
 > care must be taken of some details.  In particular let's take the
 > example of the following sequence of event starting with an empty
 > figure:

One solution can be to change all id to the new one in the undoItem
list.


 > 
 >   * create point A.
 > 
 >   * create point B.
 > 
 >   * create segment [AB].
 > 
 >   * create C the middle of segment [AB].
 > 
 >   * undo creation of C.
 > 
 >   * undo creation of [AB].
 > 
 >   * redo [AB].
 > 
 >   * redo C.
 > 
 >     At this point when recreating C the delicate point is that the
 >     reference to [AB] in the backup data of the creation command even
 >     when [AB] has been undone/recreated (and possibly has a new id).
 > 
 > To implement undo/redo for macros, what can be done is to create a
 > drgeoMacroCommand class that will perform undo/redo on a list of
 > subcommands.
 > 
 > To support an unlimited number of undo we must keep an history of the
 > commands executed on the figure.  The history data-structure is a kind
 > of LIFO queue (like a stack with a push/pop like mechanism).
 > 
 > When doing several undos we must keep the full history so that we are
 > able to redo operation, but if instead of redoing an operation we
 > perform a new operation on the figure, then we must discard all the
 > "end" of the command history.
 

Right

> 
 > Additional idea:
 > ================
 > 
 > Maybe we could save the undo/redo history as XML so that when loading
 > a figure with an history we can play a scenario, just by pressing the
 > "redo" button several times.


Oh yeah, this should be quite simple to add it in the actual save
format.


Hilaire