Review: Resubmit
> I hinted at it yesterday, I'll say it in full today: the renaming of
> Widget#stop to Widget#destroy brings absolutely no value and no
> justifications, does not make anything clearer and gratuitously breaks 6.1
> code. Even on aesthetic grounds it is indefensible since it breaks the current
> symmetry between Widget#start and Widget#stop. Worse, it implies destructor or
> finalizer semantics which don't exist (the object still exists in a known
> state after the end of #stop, and there are little to no restrictions on what
> it can do in its own #stop).
For the "it could break stuff" argument, I agree, but it was clearly stated by
al that we can break compatibility and that the API should be clean for 6.1 .
As far as I understand that, that's not because we chose a bad name at first
that we should keep it. That should be al to decide on this.
As I explained, now the ParentedMixin is totally independent to Widget and so
there is no purpose to conceive it with Widget in mind. No matter how you turn
it, there is not point considering start() as the opposite of the new
destroy(). destroy() is the opposite of init().
Finally, OF COURSE it implies destructor semantic, and that's exactly what we
want and what we need. Don't think because 99% of destructors in C++ free
memory it means that's the only purpose of a destructor, as proven by the RAII
idiom in that language. We need a destructor semantic to free any resource
owned by the objects (in the case of Widget, it's the part of the dom it
occupies). The only problem in languages that use a garbage collector is that
freeing the resources and freeing the memory can't be done at the same time
(because most if not all garbage collectors are non-deterministic, and freeing
the resources must be deterministic). If you compare to C++, in that language
we create a destructor to free resources AND memory at the same time, in
javascript we create a destructor to free only resources, since the garbage
collector will do the rest.
> For the rest, why not with two caveats:
>
> * Not fond of the Java getter/setter patterns in Java, even less so in
> javascript
> *
> `openerp.web.CallbackEnabled.extend(openerp.web.ParentedMixin).extend(other)`
> creates some sort of subclass to CallbackEnabled then re-extends it
> immediately and throws it out
> * Why the double-underscore prefixes when a single underscore will do?
> * _.each(this.getChildren(), function(el) {
> el.destroy();
> });
>
> why not _.invoke?
>
> * _.each(_.clone(this.getChildren()), function(el) {
> el.destroy();
> });
>
> why clone the children collection (which was already cloned before being
> returned, too) when you're not altering it, merely stopping all the children?
>
> * if (!parent.getChildren())
>
> will not work correctly, and neither will the implementation of
> #getChildren(): empty arrays are truty in js, so there is no way the code will
> ever enter this conditional.
>
> * furthermore, on the line below `parent.getChildren().push(this);` just
> wastes a bunch of cycle: #getChildren() never returns the actual childrens
> array, it either returns a brand new empty array or returns a shallow clone of
> the childrens array in the parent, so this line 1. creates a new array with
> the parent's children (#getChildren); 2. adds an element to the array (#push);
> 3. throws the edited array in the garbage. It's a noop as far as the parent
> object is concerned.
Those are fixed.
> * Why are there start/stop semantics independent of the parent/child
> relationship in the "ParentedMixin"? Namely, what do `__parented_stopped` and
> `isDestroyed` have to do with parent/child relationships which the mixin name
> loosely implies is its job?
Not sure I understand. Could you rephrase by taking into account my previous
argument about the fact I don't think destroy() has anything to do with start()
in Widget?
--
https://code.launchpad.net/~openerp-dev/openerp-web/trunk-core-extraction-1-niv/+merge/94021
Your team OpenERP R&D Team is subscribed to branch
lp:~openerp-dev/openerp-web/trunk-core-extraction-1-niv.
_______________________________________________
Mailing list: https://launchpad.net/~openerp-dev-gtk
Post to : [email protected]
Unsubscribe : https://launchpad.net/~openerp-dev-gtk
More help : https://help.launchpad.net/ListHelp
