Redo and returning true if undo/redo did anything.

[roasm]

I added two features that we needed for our app.

The larger one is redo. It doesn't pop states off the stack, but instead keeps an index to the next state that should be returned by undo. If a store() is called while the index is in the middle of the stack, it truncates the array to remove the remain states that should no longer be accessible.

Because store() is usually called before an action is performed on the model, there are generally some unsaved changes to the model when you call undo. These unsaved changes are stored in a temporary attributes variable so they can be redone (redo'ed?).

I also return true if an undo() or redo() call actually applies attributes to the model. If nothing is applied, it returns false.

[mxriverlynn]

the ideas sound good, but i'm a bit confused by the implementation and by some of the detail in how you describe it. can you provide some examples of how this works, and why? it would be best to get them in the form of jasmine specs, with the test suite, if you can.

[roasm]

No problem! I'll write some tests this morning...

Thanks,

Thai

On 1/9/12 4:13 PM, Derick Bailey wrote:

the ideas sound good, but i'm a bit confused by the implementation and by some of the detail in how you describe it. can you provide some examples of how this works, and why? it would be best to get them in the form of jasmine specs, with the test suite, if you can.

---

Reply to this email directly or view it on GitHub:
#11 (comment)

[roasm]

I added a redo spec to the project.

Here are some notes on the details of the implementation:

• Because we're redo-ing, we can't pop states off the attribute stack, so we keep a pointer (index) into the stack with the next state that would be returned by an undo.
• store(), restore(), redo() all update that pointer and return the correct model state.
• If you've called restore() multiple times (the pointer is somewhere in the middle of the attribute stack), and call store(), all the attribute states "above" the pointer in the stack are truncated, so you can't redo() to those states anymore.
• The tricky part is the finalRedoState. Generally, the user calls store() before a change to the model is made (so if you call restore(), it undoes that last change). That means that we don't have a stored state for the model that includes that last change.
  • To store that last change, if restore() is called when we're at the top of the stack (i.e., it's the first undo since a change), we store the current state in a separate variable.
  • We keep that out of the MementoStack because it's a special "virtual" undo state. And keeping it out of the MementoStack keeps the math in the MementoStack simpler.
• Because I added a redo() method, I aliased restore() to undo() for symmetry.
• And returning false/true for the methods is self-explanatory.

Does that make sense?

Thanks!

[mxriverlynn]

Thanks for the explanation and the specs! Your explanation helped a lot.

I still didn't quite follow what the finalRedoState was for until I looked at the specs. That last spec where you make a change, do a restore, then a redo makes it clear now.

A few suggestions on that...

The name 'finalRedoState' seems a little odd to me. When I read the spec, I keep thinking about tracking dirty state. What do you think of changing that to "dirtyState"?

And the functionality may not be clearly defined for all the cases I can think of. What happens in these scenarios:

• store(), set something, set something else, restore(), redo()
• store(), set something, restore(), set something to another value, redo()
• store(), set something, store(), set something else, restore(), restore(), redo()

I don't really know what I would expect these to do, off-hand... but these types of scenarios (any others, too?) need to at least be accounted for in the specs and documentation so that we know what to expect from code like this.

Thanks again for the work on this. It's looking great!

[roasm]

dirtyState makes sense to me. I just pushed that.

As for the scenarios you mentioned:

1. I think the "set something" and "set something else" are basically a single atomic undo state, so the final "redo()" call should re-apply both of those changes. I added a test for that.
2. In the way I assume Memento should be used, you should not "set something to another value" without a call to "store()" first. My implementation will re-apply the value of the first "set something", which makes sense because you're re-applying the last thing that was undone ("undo()"ed?). I added a test for that.
3. This case is covered in the test "when undoing twice and redoing twice".

[mxriverlynn]

1 & 3 - that makes sense.

2 ... hmm... in thinking about standard undo / redo functionality, if i undo something and then make a change, my path through the changes is now different and there is no more redo functionality because I'm now travelling a different path. would it make sense for this to work the same way?

so... in this example:

• set{foo: "bar"}
• store()
• set{foo: "baz"}
• restore()
• set{foo: "quux"}
• redo()

Right now when I call restore, it changes foo back to "bar". then after I have made changes and call redo, it sets bar to "baz".

In the standard undo/redo path travelling, calling restore would set foo back to "bar" and then the set{foo: "quux"} would wipe out the remaining states in the queue because I started down another path. then, "redo" would not change anything because there is no where to go.

I can see reason to do it the current way, and to change it to what i just described. my inclination is to change it because most people are going to expect undo/redo to work in this manner.

what do you think?

[roasm]

If any change to the model is preceded by a store(), then it would work
the way you describe (and I intended) because the store() clears off the
rest of the redo stack above it. The specific case here is that the
code is changing the model without store()ing the state before the change.

Right now, Memento doesn't know when the model changes and implementing
things the way you suggest would require that Memento hook into "set" or
the "change" event to clear the redo end of the stack. My opinion is
that would cross a boundary that I wouldn't cross.

Similarly, if calling "set" without a store() is going to impact
Memento's behavior then what would happen in this case?

• set{foo: "bar"}
• store()
• set{foo: "baz"}
• restore()
• set{foo: "quux"}
• restore()

Should the second "restore()" set foo back to "baz" or leave it at
"quux". With the current (your) implementation of Memento, the second
"restore()" would do nothing because there are no attributes left on the
stack. Basically, that last "set{foo: 'quux'}" is invisible to Memento.

Ultimately, I'm assuming that users of Memento will call store() before
every change to the model that should be undo-able. If there is a
change made without a store(), they must have a particular reason for
doing so (e.g., several "set()"s and "add()"s together make an atomic
change that the user can undo/redo).

If you do choose to make that Memento aware of that last "set()" and
somehow affect Memento's behavior, how do you propose doing it?

On 1/10/12 6:05 PM, Derick Bailey wrote:

1& 3 - that makes sense.

2 ... hmm... in thinking about standard undo / redo functionality, if i undo something and then make a change, my path through the changes is now different and there is no more redo functionality because I'm now travelling a different path. would it make sense for this to work the same way?

so... in this example:

• set{foo: "bar"}
• store()
• set{foo: "baz"}
• restore()
• set{foo: "quux"}
• redo()

Right now when I call restore, it changes foo back to "bar". then after I have made changes and call redo, it sets bar to "baz".

In the standard undo/redo path travelling, calling restore would set foo back to "bar" and then the set{foo: "quux"} would wipe out the remaining states in the queue because I started down another path. then, "redo" would not change anything because there is no where to go.

I can see reason to do it the current way, and to change it to what i just described. my inclination is to change it because most people are going to expect undo/redo to work in this manner.

what do you think?

---

Reply to this email directly or view it on GitHub:
#11 (comment)

[roasm]

Quick ping to see if you're still interested in this pull request. Is there anything else you suggest? Thanks.

[mxriverlynn]

d'oh! i completely forgot about this. :( sorry. i'll get this pulled in soon. i've been buried under my email lately, and digging myself out slowly.

[jlloydcurious]

+1

[brihogan]

Looks like good stuff. I'm in need of a library like Memento and the redo functionality would be a great addition. Would love to see it merged, when you get the chance.

[ashnur]

Anyone knows an alternative to this?