Re: [UndoManager] Re-introduce DOMTransaction interface?

On Thu, Jul 5, 2012 at 4:27 PM, Ojan Vafai <ojan@chromium.org> wrote:

> On Thu, Jul 5, 2012 at 1:02 PM, Ryosuke Niwa <rniwa@webkit.org> wrote:
>
>> On Thu, Jul 5, 2012 at 12:45 PM, James Graham <jgraham@opera.com> wrote:
>>
>>> On Thu, 5 Jul 2012, Ryosuke Niwa wrote:
>>>
>>>> On Thu, Jul 5, 2012 at 8:08 AM, James Graham <jgraham@opera.com> **
>>>> wrote:
>>>>
>>>>>       On 07/05/2012 12:38 AM, Ryosuke Niwa wrote:
>>>>>>       After this change, authors can write:
>>>>>>       scope.undoManager.transact(new AutomaticDOMTransaction{**function
>>>>>> () {
>>>>>>            scope.appendChild("foo");
>>>>>>       }, 'append "foo"'));
>>>>>>
>>>>>> [...]
>>>
>>>>
>>>>>>       document.undoManager.transact(**new DOMTransaction({function
>>>>>> () {
>>>>>>                // Draw a line on canvas
>>>>>>            }, function () {
>>>>>>                // Undraw a line
>>>>>>            }, function () { this.execute(); },
>>>>>>            'Draw a line'
>>>>>>       }));
>>>>>>
>>>>>
>>>>> I think this proposed API looks particularly verbose and ugly. I
>>>>> thought we wanted to make new APIs more author
>>>>> friendly and less like refugees from Java-land.
>>>>>
>>>>
>>>>
>>>> What makes you say so? If anything, you don't have to have labels like
>>>> execute, undo, and redo. So it's less verbose. If you
>>>> don't like the long name like AutomaticDOMTransaction, we can come up
>>>> with a shorter name.
>>>>
>>>
>>> I think having to call a particular constructor for an object that is
>>> just passed straight into a DOM function is verbose, and difficult to
>>> internalise (because you have to remember the constructor name and case and
>>> so on). I think the design with three positional arguments is much harder
>>> to read than the design with three explicitly "named arguments" implemented
>>> as object properties.
>>
>>
>> But the alternative is having to remember labels like execute, undo, &
>> redo.
>>
>
> In your version, you need to remember the order of the arguments, which
> requires you looking it up each time. If we do decide to add the
> DOMTransaction constructor back, we should keep passing it a dictionary as
> it's argument. Or maybe take the label and a dictionary as arguments.
>

That's true of almost all other Web APIs when used in ECMAScript 5. I'm not
sympathetic to the argument that you have to remember orders in which
arguments appear because that's true of all functions in ES5, C++, and
various other programming languages. If we were to solve this problem (I'm
not even convinced that this is a problem), then TC39 is the right place to
discuss this issue. I do not want to turn the undo manager API spec into a
scientific experiment for new syntax sugar in ES5.

On Thu, Jul 5, 2012 at 4:40 PM, Yuval Sadan <sadan.yuval@gmail.com> wrote:
>
>  > t = undoManager.transact("foobar", function() { ... });
>> > t.onredo = function() { /* custom redo */ };
>> > t.execute();
>> >
>> > Whether onundo/onredo are assigned upon execute() is well defined, so
>> basing behavior on that is clear enough for me. Plus, I wonder - what is a
>> non-automatic transaction without an undo? A freak?
>>
>> Also, the interface you proposed behaves somewhat oddly when the undo
>> manager from which the transaction was created is disconnected since
>> execute() will fail in such a case.
>>
> What I find appealing in this example is that the transaction is an
> operation which is indeed disconnected from the UndoManager. Perhaps I'm
> missing something here, but what do we gain by having UndoManager being the
> place of both creation and execution of the DOMTransaction object? If I may
> take the notion further, it might as well originate elsewhere, and connect
> to an UndoManager only on execute(), e.g.
>
> t = document.transaction("foo", function() {  }); // or new
> DOMTransaction(), actually
> t.onundo = function() {   };
> undoManager.execute(t);
>
> I realize as I'm writing that this is a hybrid suggestion between new
> DOMTransaction() and the event handler version. It gives reason for
> introduction of the DOMTransaction() standalone instantiation. Perhaps I'm
> going to far. It just seems logical.
>

Right, I like the separation of concerns. Your proposal here is almost
identical to mine. I just made it easier to specify undo & redo as an
argument to transaction (in my case of new DOMTransaction) so that instead
of

t = new DOMTransaction("foo", function () { });
t.onundo = function() { };

I can do just do:

t = new DOMTransaction("foo", function () { }, function () { });
undoManager.execute(t);

It encapsulates the action as well as the reverse action. I find this to be
> explicit and simple enough to write without confusion. Every idea is
> specified once and in a clear place - (a) what to do, (b) how to undo and
> (c) execute. More specific than this would be verbose imho.
>

I'm totally fine with the constructor of document.transact just take the
initial execute callback and undo/redo optionally specified as ES5
properties.

>  Another approach will be to make the second argument optional and treat
>> all transactions created without a callback function as manual transactions
>> since there's no need for manual transaction's execute to be a callback.
>>
> I fear that changing placement of the execute function in different
> circumstances will cause confusion and that it is indeed too implicit.
>

Okay.

- Ryosuke