Closures for undo: bad idea?

[Jeremy]

I'm working on an application that does some DOM manipulation. I want
to add a mechanism for "Undo" so that the user can revert to the
previous state after performing a mistaken action. Simple idea, not
necessarily simple to implement.

My first idea for this is to use closures and an array that tracks the
inverse of the user's actions. Here's a simple example in which the
user can only remove DOM nodes and then undo the removal repeatedly
until they've reached their desired state (or the original state):

var App = {

undo: [],

removeNode: function(node)
{
//remove the node
var parent = node.parentNode;
var nextSibling = node.nextSibling;
parent.removeChild(node);

//inverse action goes into undo buffer
App.undo.push(function() {
if(nextSibling)
parent.insertBefore(node, nextSibling);
else
parent.appendChild(node);
});
},

undoLast: function()
{
if(App.undo.length)
(App.undo.pop())();
}
};

(Obviously the removeNode and undoLast functions are invoked from
elsewhere in response to user actions.)

My concern is with the closures created in the removeNode function.
This enables the undo to take place quite easily, but I am worried about
memory leakage.

Let's say the user undoes a few actions, and then performs a few more
actions. Are the nodes associated with the actions that were undone
still around in memory? There is now no way to access the nodes, but
will they be garbage collected or are they just leaking memory like a sieve?

Has anyone implemented undo this way? Any other strategies I should
consider?

Thanks,
Jeremy

 

[Justin Johansson]

Jeremy,
It's difficult to be definitive about this and the outcome might well be
browser dependent.

On one hand, when the use performs an undo, your undo function is pulled
of the undo stack and executed. After that,
there are no outstanding references to the function and you would think
that the garbage collector wold clean up the function and in turn
release references to state variables (such as nextSibling) help by the
closure.

On the other hand, I've been caught with closure induced memory leak
esp. in IE before and so I would probably use a slightly different
approach. Still keep the undo stack but change what you store on it.
Instead of pushing a closure, push an object containing arguments and
a reference to a statically generated function to call to effect the
undo.

The undo function facility is invoked like the following then:

var App = {

undo: [], // an array of "undo" Objects

undoLast: function()
{
var oUndo = App.undo.pop();

if ( oUndo !== undefined) {
oUndo.fnUndo.apply( oUndo.thisp, oUndo.args);
}
}

};

What you need to complete is the code to record the undo Objects.

Cheers
Justin Johansson