[whatwg] Fixing undo on the Web - UndoManager and Transaction

[Jonas Sicking]

On Tue, Aug 9, 2011 at 3:11 PM, Ryosuke Niwa <rniwa at webkit.org> wrote:
> On Tue, Aug 9, 2011 at 2:55 PM, Jonas Sicking <jonas at sicking.cc> wrote:
>> I don't think it's a matter of which use cases can or can't be solved with
>> either solution. It's pretty clear to me that all scenarios can be solved
>> with either API.
>
> Right, they're isomorphic.
>>
>> It's just a matter of which pattern is more common and so which one we
>> should make more convenient. If almost everyone puts the same code in apply
>> and reapply then we're just creating more work for people.
>>
>> Here's how you'd implement the apply/reapply/unapply syntax using simply
>> apply/unapply
>>
>> apply: function() { if (!this.applied) { action1(); this.applied = true; }
>> else { action2(); }
>> unapply: unapply
>
> One thing I don't like about this approach is that it requires a flag.  With
> my proposal, all you need to do is to call a function. Also, we can make it
> so that when you don't supply a value to reapply (i.e. reapply is null),
> then undoManager automatically calls apply instead (I always intended this
> behavior from the beginning but I've apparently left this details out).

Sure, your API is more convenient in certain situations. But it also
encourages code duplication (I'll note that in the examples you
originally provided in this thread you always ended up duplicating
code between apply/reapply), which easily leads to bugs.

Another point to keep in mind is that we can always end up adding
features that makes certain use cases more convenient. There is
literally an infinite amount of things people will want to do and APIs
we can add to make it easier for them to do it. The question we need
to ask ourselves is where to draw the line. The way to do that is to
look at which use cases requires which features, and how common those
use cases are.

I do definitely agree that making the reapply function optional helps
a lot in that at least pages don't have to worry about the feature if
they're not using it. If we do that though we should probably rename
the 'apply' property for managed transactions since it's semantics are
pretty different. 'apply' for managed transactions are only called
once when the transaction is first added. 'apply' for manual
transactions are potentially called every time the transaction is
(re)applied.

/ Jonas