[dojo-contributors] MVC support for dijit based on dojo.Stateful

Rahul Akolkar rahul.akolkar at gmail.com
Sat Mar 5 15:17:19 EST 2011


Hi Bill,

Inline.

On Fri, Mar 4, 2011 at 10:33 PM, Bill Keese <bill at dojotoolkit.org> wrote:
> Glad you are getting good feedback from Ben.
<snip/>

Yup, good to have more pairs of eagle eyes on this. I am in the middle
of some minor refactorings based on all the comments so far. I expect
to have a refreshed patch by the end of the weekend.


> Here's a reply to some of the stuff from my original note.
>
<snap/>

Thanks, I was hoping it'd come in before my refreshed patch became ready :-)


>>
>> > 2. DataBindingMixin
>> >
>> > a) file structure
>> > The root of the patch is _DataBindingMixin.js, which gets mixed into
>> > _WidgetBase.   Current code for that is split across two files as:
>> >
>> > dojo.declare("dijit._DataBindingMixin", null, { ... });
>> >
>> > dojo.extend(dijit._WidgetBase, new dijit._DataBindingMixin());
>> >
>> > It would be simpler like this:
>> >
>> > dojo.extend(dijit._WidgetBase, {...});
>> >
>> > You can still pull _DataBindingMixin.js into StatefulModel.js if you
>> > don't
>> > want apps to need to explicitly dojo.require(DataBindingMixin).
>> >
>> <snap/>
>>
>> Correct, thats indeed the objective. The data binding mixin is not
>> something we'd want apps to explicitly dojo.require(). By "pull into",
>> I'm thinking you mean inline the source into the other file? Thats an
>> option.
>
>
> Sorry, I meant that you could list _DataBindingMixin in the list of requires
> for StatefulModel, in the first line define() call.
<snip/>

Oh, got it.


> But I agree with Ben's comment, it seems wrong to make a dijit dependency on
> StatefulModel just so apps can avoid an extra dojo.require() call
> Would StatefulModel ever be useful outside of widgets?    A recursive
> dojo.Stateful sounds useful in general, not sure about the other stuff in
> there.
>
<snap/>

Right, the dijit.StatefulModel class is already geared to deal with
specifics of a view layer (all the valid, readOnly, relevant etc.
meta-data lends well to view layer bindings). In theory, one could
capture the essence of the recursive model for reuse elsewhere. But
while it may be possible to craft a base class that is purely a
recursive dojo.Stateful model and then subclass it to provide what is
effectively dijit.StatefulModel functionality now, I can't say I have
many usecases at hand. Ofcourse, such refactoring can also happen down
the road if deemed essential.


>> > e) _initControl()
>> >
>> > This is run from postCreate(), yet it assumes that the widget is
>> > anchored to
>> > the document.   You shouldn't assume that until startup() is called; in
>> > particular it's not true for programatically created widgets ex:  new
>> > dijit.form.TextBox().placeAt(...)
>> >
>> <snip/>
>>
>> OK, I'll look into (d) and (e) above.
>>
>
> About having every widget trace up the tree to find a parent with a binding
> specified, or if there's no such parent then tracing all the way to
> <body>...    we faced that same problem with the bidi group and their
> lobbying for every widget to support a dir parameter (possibly) being
> inherited from an ancestor .  We didn't want to have every single widget
> calling getParent() repeatedly to inspect it's ancestors as that seemed like
> it would degrade performance on page load.   So we ended up adding code to
> the parser to pass down the dir inherited from the ancestor.   I wonder if
> that makes sense here.
<snip/>

It may make sense, but let me step back a bit. Some thoughts, in no
particular order:

I assume by getParent() you mean dijit.getEnclosingWidget() which is
what is used here. The number of these calls is a function of: (a)
Number of data-bound widgets, (b) Topology of binds i.e. amount of
relative refs implied by nesting and (c) Average DOM separation (along
parent axis) between nested data-bound dijits.

The observed rule of thumb is that at page load you'd expect roughly
2x the number of dijit.getEnclosingWidget() calls as the number of
data-bound widgets. This is somewhat less pervasive than bidi as it
only applies to data-bound dijits (clearly, a view may have data-bound
dijits and others that aren't data-bound).

There are two scenarios where bindings are calculated (or recalculated):
 * Initial page load
 * Programmatic binding update (example: when user chooses to list
details for a particular search result in the master-detail demos in
the patch, causing the bindings in the detail portion of the view to
be updated)

Ben had a suggestion about an optional parent binding param to
_setupBinding(), which effectively removes the need for any DOM
operations or parent widget lookups in the second case.

For the initial page load, the roughly 2x dijit.getEnclosingWidget()
calls may be avoided if we can get the parent binding to be
"inherited" through the parser. OTOH, these calls aren't terribly
expensive either (operations therein include .parentNode references,
getting the widgetId attribute values and a lookups into the widget
registry), so some of this could be done as incremental improvements.


> In your response to Ben you gave this example:
> model:
>  { foo : { bar : { baz : { freddy : { frog : "prince" } } } } }
>
>
> markup:
>  <div dojoType="dijit.mvc.Group" ref="model.foo">
>  <div dojoType="dijit.mvc.Group" ref="bar">
>   <div dojoType="dijit.mvc.Group" ref="baz">
>    <div dojoType="dijit.mvc.Group" ref="freddy">
>     <div dojoType="dijit.form.TextBox" ref="frog">
>
>
> Hopefully that doesn't execute in O(n^2), where every widget needs to trace
> to the top of the tree, does it?
<snap/>

In the above example:
 * There are exactly 5 getEnclosingWidget() calls
 * The total number of node.parentNode references looked up (including
those within these 5 calls) is exactly equal to the DOM depth of the
textbox on the page

Let me try to understand why you say O(n^2), since every widget isn't
trying to reach every other widget on the page. Worst case performance
is linear to the total number of data-bound widgets (n) where the
constant in the linear equation is the average DOM separation across
all data-bound widgets (k). I guess you are assuming n tends to k on a
given page? (not usually the case). Also, if total number of dijits on
page is N, n < N is common, if not outrightly n << N.


> I don't really understand the code that
> executes for the TextBox figures out that it's connected to
> model.foo.bar.baz.freddy.frog.   (What info is cached inside of the
> innermost Group?)
<snip/>

There is no caching per se, direct references to nodes within the
StatefulModel are stored as "binding" property on appropriate dijits.
Here is how things work in above scenario:
1. Outermost group finds no data-bound parent, evals "model.foo" to
obtain its ref StatefulModel node
2. The next three groups locate their data-bound parent immediately
i.e. on first parent lookup (as its also the DOM parent), and each
resolves their ref expression as relative to parent. So, in the
absolute sense, 2nd group refs model.foo.bar, 3rd refs
model.foo.bar.baz and so on.
3. The textbox similarly (relatively) obtains its "binding" as a ref
to the model.foo.bar.baz.freddy.frog node (parent:
model.foo.bar.baz.freddy, relative ref: frog). At which point, the
data (i.e. "value") and meta-data associations are set up between that
model node and the textbox, thereby the textbox displays the value
from the model i.e. "prince".


> Anyway, the parser has an "inherited" parameter that could be used to pass
> down the ref from the parent.  I'm not sure if using it makes sense or not.
>
<snap/>

See above.


>>
>> > 4. NumberTextBox, ValidationTextBox
>> >
>> > These methods are now assuming a super-class isValid() method, but that
>> > method only exists when _DataBindingMixin is included.   I guess we need
>> > to
>> > define an empty isValid() method in _WidgetBase?   I don't know what
>> > happens
>> > when you call this.inherited(arguments) and there's no such super-class
>> > function, but even if it works, it seems fragile.
>> <snap/>
>>
>> Nothing happens, see line 190 (linking to a few lines before that for
>> context):
>> http://bugs.dojotoolkit.org/browser/dojo/trunk/_base/declare.js?rev=23778#L186
>>
>> That aside, yes, this was slightly tricky because there was no
>> corresponding isValid() in _WidgetBase whereas I think there should
>> be. Also, not all dijits check base class validity (where applicable)
>> before deciding their isValid() status and once its added to
>> _WidgetBase, they should (and thereby, the model validity will be
>> accounted for in cases where the data binding mixin is applied).
>>
>
> OK, adding it to _WidgetBase is something to consider.    But again, there
> would be the "problem" where your mixin is simply overwriting the
> isValid()method in _WidgetBase.js, like it's overwriting postCreate() now.
>  The isValid() problem is harder to solve since dojo.connect() doesn't work
> since you need to return a value, I guess you should be doing something
> like:
> var oldIsValid = dijit._WidgetBase.isValid;
> dijit._WidgetBase.isValid = function(){ return this.oldIsValid() &&
> your-code-here; }
>
<snip/>

OK, that would cover all bases. OTOH, isValid() would simply return
true in _WidgetBase, can't imagine much else going on there.


>>
>> > 5. dijit.mvc._Container
>> >
>> > This has >100 lines of cut-pasted code from _Templated and doesn't
>> > support
>> > the new data-dojo-attach-point etc. syntax.   Can you do better?   See
>> > dijit.Declaration, maybe you can have similar code.
>> <snip/>
>>
>> Yes, its listed as a TODO in code -- it'd be good to remove that
>> duplication. The issue as I remember was that I wasn't able to reuse
>> _Templated directly as the way it deals with templates and template
>> caches is different from, say repeat, where the template is inline
>> (body). I'll take another look at dijit.Declaration, but ISTR some
>> other differences there.
>
>
> This inheritance from _Container to both Group and Repeater confuses me.
>  While Group is just a plain wrapper widget, similar to dijit.form.Form or
> dijit.layout.ContentPane, Repeater is a different beast that generates from
> a template.
<snap/>

It sure would be confusing if that were the case :-) Group does not
inherit from _Container, it inherits from _WidgetBase. Its the
simplest data-bound widget that is very useful in setting up
hierarchical data bindings (ref'ing to intermediate model nodes).


>   I guess my question is, why does mvc._Container have any code
> about templates?
>
<snip/>

In hindsight, somewhat unnecessary. I've refactored that out of
_Container (please see next patch in a day or two).


>>
>> > b) the declaredClass references here and in other parts of the code
>> > aren't
>> > future-proof.   declaredClass will likely go away in 2.0.
>> <snap/>
>>
>> OK, any future-proof equivalent? I'll investigate or use a bit more of
>> the duck typing sauce if needed.
>
> Yes, if there isn't an obvious way to ducktype (ex: testing if a certain
> method like then() is available), then in dijit for example we have
> isContainer and isLayoutContainer flags on widgets that indicate they have a
> certain functionality.
>
<snap/>

Thanks, there is, toPlainObject() is used as the duck typing signature.

-Rahul


More information about the dojo-contributors mailing list