Re: [UndoManager] Re-introduce DOMTransaction interface?

On Wed, Jul 11, 2012 at 3:47 PM, Ryosuke Niwa <rniwa@webkit.org> wrote:

> On Wed, Jul 11, 2012 at 3:35 PM, Ojan Vafai <ojan@chromium.org> wrote:
>
>> On Wed, Jul 11, 2012 at 3:23 PM, Ryosuke Niwa <rniwa@webkit.org> wrote:
>>
>>> On Wed, Jul 11, 2012 at 3:12 PM, Ojan Vafai <ojan@chromium.org> wrote:
>>>
>>>> We don't have this problem with the dictionary interface because we
>>>> don't store the actual dictionary. We take the dictionary and construct a
>>>> C++ (DOMTransaction?) entry into the undomanager. So, if you reuse the same
>>>> dictionary, you get a new C++ object stored for each transact call. If you
>>>> modify the dictionary after the transact call, it does not affect the
>>>> stored C++ object.
>>>>
>>>
>>> I don't follow. Your second point is that it's ambiguous as to what
>>> should happen when undo/redo event listeners are modified. I'm saying that
>>> the same ambiguity rises when the script modifies undo/redo properties of
>>> the pure JS object that implements DOMTransaction because we don't store
>>> undo/redo properties in a separate C++ (or whatever language you're
>>> implementing UA with) object inside transact().
>>>
>>> By the way, let us refrain from using the word dictionary here because
>>> there is a specific object named Dictionary in WebIDL and multiple folks on
>>> IRC have told me that our use of the term "dictionary" is confusing. For
>>> further clarify, the "dictionary" in the current specification is an user
>>> object implementing the DOMTransaction callback interface as defined in:
>>> http://www.w3.org/TR/WebIDL/#es-user-objects It can't be a Dictionary
>>> because execute, executeAutomatic, undo, and redo need to be called on the
>>> object; e.g. if we had t = {execute: function () { this.undo = function ()
>>> { ~}; }}}, then execute will change the "undo" IDL attribute of t.
>>>
>>
>> I was specifically talking about WebIDL Dictionaries. In which case,
>> this.undo would set the undo property on the window. Why would you want to
>> be able to set the undo function from the execut function? My whole point
>> has been that we should not keep the actual object.
>>
>
> Oh, then you're proposing something different here. There is a use case
> for being able to add custom properties (for extra information used by
> library, etc...) and store that in the undo manager. If we had used
> Dictionary for, say, the constructor of DOMTransaction, then you'd have to
> add those properties after creating DOMTransaction. If you're proposing to
> use Dictionary for transact() so that we don't keep any objects, then that
> just doesn't work.
>

What are the cases where you need this? Closures over your
execute/undo/redo methods seem to me like it would address this use-case
fine.

  Speaking of merge, why is it on transact and not on the transaction
>>>>>> dictionary. The latter makes more sense to me.
>>>>>>
>>>>>
>>> Because of the fact DOMTransaction had been a user object. It would be
>>> awfully confusing if the script could override the value of "merge". If we
>>> had re-introduced the DOMTransaction interface, then we can make "merge" a
>>> readonly attribute on the object.
>>>
>>
>> To clarify my position here, I think transact should take in a
>> Dictionary. Then the useragent converts that into a DOMTransaction and
>> stores it in the UndoManager's history. So, item would return a
>> DOMTransaction. In that case, merge could be a value in the Dictionary and
>> readonly on the DOMTransaction.
>>
>
> So one big use pattern we're predicting is something along the line of:
>
> undoManager.transact(transactionFactory(~~));
>
> where transactionFactory can create a custom DOM transaction object. Now
> all properties set on the object returned by transactionFactory will be
> lost when execute/executeAutomatic/undo/redo are called in your proposal,
> and that's very counter intuitive and weird in my opinion.
>

We disagree on what's counterintuitive.


>
> - Ryosuke
>
>