Opened 8 years ago

Closed 8 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 8 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 8 years ago.
I missed a testcase in the last patch.
multiattrib-14681.patch (209.0 KB) - added by Ed Chatelain 8 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 8 years ago.
Controller name update, next patch will rename BindTwo? to sync.
latestUpdates-tests-14681.patch (220.8 KB) - added by Ed Chatelain 8 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 8 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 8 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 8 years ago by cjolif

Milestone: tbd1.8

comment:2 Changed 8 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 8 years ago by Ed Chatelain

Attachment: 14681-testcase.patch added

I missed a testcase in the last patch.

comment:3 Changed 8 years ago by cjolif

In [27698]:

refs #14681. Missing test case. Thanks edchat.

comment:4 Changed 8 years ago by ben hockey

could we remove the null check from the initial copy?

comment:5 Changed 8 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 8 years ago by Ed Chatelain

Priority: undecidedhigh
Status: newassigned

comment:7 Changed 8 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 8 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 8 years ago by cjolif

In [27904]:

refs #14681. Reorganize test files. !strict.

comment:10 Changed 8 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 8 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 8 years ago by cjolif

In [27917]:

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

comment:13 Changed 8 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 8 years ago by cjolif

In [27935]:

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

Changed 8 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 8 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 8 years ago by Ed Chatelain

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

comment:16 Changed 8 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 8 years ago by cjolif

In [27981]:

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

Changed 8 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 8 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 8 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 8 years ago by cjolif

In [28175]:

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

comment:20 Changed 8 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 8 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 8 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 8 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 8 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 8 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 8 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 8 years ago by Ed Chatelain

Resolution: fixed
Status: assignedclosed

This is done.

Note: See TracTickets for help on using tickets.