Opened 9 years ago
Closed 9 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)
Change History (33)
Changed 9 years ago by
Attachment: | 14681.patch added |
---|
comment:1 Changed 9 years ago by
Milestone: | tbd → 1.8 |
---|
Changed 9 years ago by
Attachment: | 14681-testcase.patch added |
---|
I missed a testcase in the last patch.
comment:5 Changed 9 years ago by
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 9 years ago by
Priority: | undecided → high |
---|---|
Status: | new → assigned |
comment:7 Changed 9 years ago by
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 9 years ago by
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:10 Changed 9 years ago by
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.
Changed 9 years ago by
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.
Changed 9 years ago by
Attachment: | controller-14681-step1.patch added |
---|
Controller name update, next patch will rename BindTwo? to sync.
Changed 9 years ago by
Attachment: | latestUpdates-tests-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 1 of 2..
Changed 9 years ago by
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:20 Changed 9 years ago by
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 9 years ago by
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 9 years ago by
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 9 years ago by
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 9 years ago by
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 9 years ago by
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 9 years ago by
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!
Patch with the multiattribute, convert, wildcard and direction support. Thanks to Akira Sudoh (IBM, CCLA) for his work on this.