Opened 10 years ago

Closed 10 years ago

#14681 closed enhancement (fixed)

Add MVC support to bind to multiple attributes (in addition to value).

Reported by: Ed Chatelain Owned by: Ed Chatelain
Priority: high Milestone: 1.8
Component: DojoX MVC Version: 1.7.1
Keywords: Cc:
Blocked By: Blocking:

Description

dojox.mvc in Dojo 1.7 supports “value” attribute in widget side and “value” property in data-model side (dojox.mvc.StatefulModel?/dojo.Stateful). This works very well with form widgets (dijit.form.*), as most of them use “value” attribute to keep track of the value.

On the other hand, there has been a desire to support more (actually, any) widgets for data binding. It crates a need to support widget attributes beyond “value”. For example:

dijit.form.CheckBox? uses “checked” attribute for its checked/unchecked state. dijit.form.Button uses “label” attribute for its text, and “iconClass” attribute for its icon. dijit.MenuItem? uses “disabled” attribute for its enabled/disabled state, “selected” attribute for its selection state, “active” attribute for its mouse/key press state, etc. It also uses “label” and “iconClass” attributes like dijit.form.Button.

We should also add "wildcard" support for binding all matching properties at once, and "convert" and "direction" support as well.

Attachments (6)

14681.patch (79.5 KB) - added by Ed Chatelain 10 years ago.
Patch with the multiattribute, convert, wildcard and direction support. Thanks to Akira Sudoh (IBM, CCLA) for his work on this.
14681-testcase.patch (5.7 KB) - added by Ed Chatelain 10 years ago.
I missed a testcase in the last patch.
multiattrib-14681.patch (209.0 KB) - added by Ed Chatelain 10 years ago.
The next set of updates for dojox.mvc. Includes many updates for better controller support and additional tests for the new mvc APIs.
controller-14681-step1.patch (121.5 KB) - added by Ed Chatelain 10 years ago.
Controller name update, next patch will rename BindTwo? to sync.
latestUpdates-tests-14681.patch (220.8 KB) - added by Ed Chatelain 10 years ago.
Next set of updates from work done in github, added many testcases and inline comments and examples, in addition to code updates and bug fixes - patch 1 of 2..
latestUpdates-14681.patch (152.1 KB) - added by Ed Chatelain 10 years ago.
Next set of updates from work done in github, added many testcases and inline comments and examples, in addition to code updates and bug fixes - patch 2 of 2..

Download all attachments as: .zip

Change History (33)

Changed 10 years ago by Ed Chatelain

Attachment: 14681.patch added

Patch with the multiattribute, convert, wildcard and direction support. Thanks to Akira Sudoh (IBM, CCLA) for his work on this.

comment:1 Changed 10 years ago by cjolif

Milestone: tbd1.8

comment:2 Changed 10 years ago by cjolif

In [27695]:

refs #14681. Add multi-attribute binding (might need to be cleaned-up a bit in a subsequent commit to get it works only with one syntax for simplicity reasons), wildcard & direction support. As well some additional very small clean-ups. Thanks edchat & Akira Sudoh (IBM, CCLA) for this patch. !strict.

Changed 10 years ago by Ed Chatelain

Attachment: 14681-testcase.patch added

I missed a testcase in the last patch.

comment:3 Changed 10 years ago by cjolif

In [27698]:

refs #14681. Missing test case. Thanks edchat.

comment:4 Changed 10 years ago by ben hockey

could we remove the null check from the initial copy?

comment:5 Changed 10 years ago by Ed Chatelain

Hi Ben, where is the null check you would like to have removed? We are working on another large set of updates in github, so I will have to make sure it is updated there. The next drop should happen in the next day or so. Thanks

comment:6 Changed 10 years ago by Ed Chatelain

Priority: undecidedhigh
Status: newassigned

comment:7 Changed 10 years ago by Ed Chatelain

We are in the process of migrating the old testcases to the new api, so hopefully we will catch any bugs which had been reported and fixed previously.

comment:8 Changed 10 years ago by ben hockey

hi ed,

the change is already in a branch in github (https://github.com/asudoh/dojox_mvc/commit/b50d4b8d5a6c369d7a1fe6f082c83b39c88079b2) which is why i was making sure it was coming here too. it should probably be a little bit different though because of the way copy works. once the check for null is gone, the initial call to copy should maybe be something like this (change 6th arg to undefined) for it to work:

copy(parseFunc, target, targetProp, source, sourceProp, undefined, value);

this has the effect that something with an undefined value will not do an initial copy but null will be copied. there are times where i'm intentionally setting initial values to null and i want that to propagate when i first bind.

also, in bindTwo, direction is missing a var declaration

comment:9 Changed 10 years ago by cjolif

In [27904]:

refs #14681. Reorganize test files. !strict.

comment:10 Changed 10 years ago by Ed Chatelain

Ben I will work on the updates for the null checks in BindTwo? after the current set of updates is completed to get the latest code from github into svn.

comment:11 Changed 10 years ago by cjolif

In [27916]:

refs #14681. Updates for multiple attribute support, along with array support & follow ups on tests re-organization. Thanks edchat & Akira Sudoh (IBM, CCLA). !strict.

comment:12 Changed 10 years ago by cjolif

In [27917]:

refs #14681. New directories for more tests content to come.

comment:13 Changed 10 years ago by cjolif

In [27932]:

refs #14681. More tests + some clean ups (including removing some svn:executable properties on js files!). Thanks edchat (IBM, CCLA). !strict.

comment:14 Changed 10 years ago by cjolif

In [27935]:

refs #14681. More clean-ups. Thanks edchat (IBM, CCLA). !strict.

Changed 10 years ago by Ed Chatelain

Attachment: multiattrib-14681.patch added

The next set of updates for dojox.mvc. Includes many updates for better controller support and additional tests for the new mvc APIs.

comment:15 Changed 10 years ago by cjolif

In [27968]:

refs #14681.
1/ Have all controller inheriting _Controller and make mixin versions to be used with widgets
2/ Have getStatefulOptions by default
3/ various other changes
!strict.

Changed 10 years ago by Ed Chatelain

Controller name update, next patch will rename BindTwo? to sync.

comment:16 Changed 10 years ago by cjolif

In [27980]:

refs #14681. Remove the duality between regular class & Dijit mixins by merging both. Thanks edchat & Akirah Sudoh (IBM, CCLA). !strict.

comment:17 Changed 10 years ago by cjolif

In [27981]:

refs #14681. Rename BindTwo? -> sync after dicussion with Ed & Ben. !strict.

Changed 10 years ago by Ed Chatelain

Next set of updates from work done in github, added many testcases and inline comments and examples, in addition to code updates and bug fixes - patch 1 of 2..

Changed 10 years ago by Ed Chatelain

Attachment: latestUpdates-14681.patch added

Next set of updates from work done in github, added many testcases and inline comments and examples, in addition to code updates and bug fixes - patch 2 of 2..

comment:18 Changed 10 years ago by cjolif

In [28174]:

refs #14681. Next set of updates from github. Mostly testcaes & inline documentation. Also added EditStoreRefListController? (mixin of EditStoreRef? & ListController?) as well as equals to compare models. Thanks Akira Sudoh & Ed Chatelain (IBM, CCLA). !strict.

comment:19 Changed 10 years ago by cjolif

In [28175]:

refs #14681. Tests for the latest features. Thanks Akira Sudoh & Ed Chatelain (IBM, CCLA).

comment:20 Changed 10 years ago by ben hockey

is anyone reviewing these changes?

i know we don't have a formal review process with dojo but with very little effort i keep finding small issues that cause me to wonder what i might find if i had the time to look more thoroughly. most of the problems i find are arguably minor but they lead me to doubt the quality of the code.

i'm happy that all my concerns are addressed quickly but there is a lot of code being added and i'm concerned that the problems i'm able to see easily might only be the tip of the iceberg.

comment:21 Changed 10 years ago by Ed Chatelain

Hi Ben, yes it is being reviewed by cjolif before he commits, but I guess the review is not as close as it should be due to the large number of changes. Sorry about the large drops of code, we will try to keep them smaller from now on, so that it will be easier to review the changes better. Do you have specific areas you think we need to take a closer look at? Thanks

comment:22 Changed 10 years ago by cjolif

Replying to neonstalwart: Ben, thanks for your interest in this. I'm doing a high level review of this code both when it lands on github and when i'm committing it here. I do not go in a thorough review of looking for small bugs here and there but as you said I don't think we have process for that. That said I trust Ed and Akira for using the remaining time until the release (3 months?) to track down small issues and fix them. If you have time for more thorough reviews than I do I guess you should look at the github to get advance warning of what's coming. As a final note I would add that dojox/mvc probably have nothing to be blamed for compare to most of other dojox modules in term of quality!

comment:23 Changed 10 years ago by ben hockey

ed - the few comments i've already made on github at (https://github.com/edchat/dojox_mvc/pull/29) are all i have for now. i've only skimmed the code.

christophe - poor quality in existing code does not justify poor quality in new code. that said, i'll give ed and akira time to find issues and try to withhold my comments until closer to the release. btw, currently the tests don't run due to a couple of small typos (see #15011). do you include running the tests as part of your high level review?

comment:24 Changed 10 years ago by Ed Chatelain

Ben all of the tests ran fine for me on Chrome on Windows. But I should have caught that and gotten the fix for 15011 in, sorry I missed it. I will fix that now. Thanks,

comment:25 Changed 10 years ago by ben hockey

ed, yes, the tests will pass on chrome after #15011 is fixed but did christophe notice that the tests wouldn't load when he reviewed the code?

once #15011 is fixed, you'll notice there is a failure in IE for dojox.mvc.tests.doh_mvc_mobile-demo - i only tried IE7 & 8 and i'm not sure if it's expected or not. it seems the inputs with id="nameInput${this.index}" don't get created and the "Repeat Data Binding Example page" tests fail.

comment:26 Changed 10 years ago by cjolif

Ben, I would have hoped that you did not think I wanted to justify poor quality with poor quality! I wanted to raise the fact that in my mind even if you seem to think differently dojox/mvc has a relatively good quality (even if certainly not perfect I agree) and that most other dojox modules have not that level of quality so blaming dojox/mvc in particular sounded a bit unfair to me even if obviously errors have been made! How many dojox projects even in the most used ones have DOH tests running? So it sounded strange to me to blame the quality of one of the most dojox tested project?

When it comes to running tests, I was usually running the full suite when I started the first commits on this work and was never getting issues so I started to trust more and more Ed and Akira and started just running a few tests instead of the full test suite to save time considering Ed did the testing before. Errare humanun est, from now on I will run the full test suite!

comment:27 Changed 10 years ago by Ed Chatelain

Resolution: fixed
Status: assignedclosed

This is done.

Note: See TracTickets for help on using tickets.