Opened 8 years ago
Closed 8 years ago
#16815 closed enhancement (fixed)
Store API support in CheckedMultiSelect
Reported by: | adros | Owned by: | bill |
---|---|---|---|
Priority: | undecided | Milestone: | 1.9 |
Component: | DojoX Form | Version: | 1.8.3 |
Keywords: | Cc: | ||
Blocked By: | Blocking: |
Description
CheckedMultiSelect? still does not support new dojo/store API even if _FormSelectWidget already does.
I have attached patch file which will solve this issue.
Attachments (2)
Change History (10)
Changed 8 years ago by
comment:1 Changed 8 years ago by
comment:2 Changed 8 years ago by
Why would it break support for old dojo.data? 'setStore' method of CheckedMultiSelect? calls inherited method from _FormSelectWidget, which mixes in 'get' and 'query' methods if dojo.data store was passed in.
comment:3 Changed 8 years ago by
Owner: | changed from dante to adros |
---|---|
Status: | new → pending |
Oh OK. See also #16828 though. I imagine that value[0]
won't work for dojo/store. The problem is there's no test case for your setStore() so I can't tell if your change works or not. If you want to add an automated test to dojox/form/tests/test_CheckedMultiSelect.html that would be great.
Changed 8 years ago by
Attachment: | test_CheckedMultiSelect.html.diff added |
---|
Patch for automated test
comment:4 Changed 8 years ago by
Status: | pending → new |
---|
Attachment (test_CheckedMultiSelect.html.diff) added by ticket reporter.
comment:5 Changed 8 years ago by
Thanks for the test file patch. Unfortunately the test file is not really testing setStore(), either for the old dojo.data stores or the new dojo.store support. To prove that, I can erase the whole method and all the tests still pass.
Maybe we just shouldn't be supporting the store having initially selected items?
My other concern is that the query in setStore() is hardcoded to {selected: true}, so it will only work for a certain structure of data, and possibly only for memory stores, not for stores with a different query syntax.
comment:6 Changed 8 years ago by
Maybe the hardcoded query should be replaced for some param in fetchArgs, e.g. 'selectdQuery'. And only if this param is provided in fetchArgs, the store would be queried for selected items.
comment:7 Changed 8 years ago by
Milestone: | tbd → 1.9 |
---|---|
Owner: | changed from adros to bill |
Status: | new → assigned |
Yes, maybe. But then, that option should really be in dijit/form/MultiSelect and dijit/form/Select too.
Note that _FormSelectWidget (the base class) has lots of fancy code for watching changes to the data store and reflecting them into the widget, so if we supported the data store controlling which values were selected, we would probably want changes to the data store about which items were selected to be reflected to the widget, and in reverse too.... anyway it gets complicated and is perhaps a better job for dojox/mvc than for the widgets to do themselves.
So for now I'm just going to remove that method completely. It fixes support for dojo/store and removes code that according to #16828 wasn't working anyway.
Although, that will break support for the old dojo.data stores. Maybe it should branch, or the patch should wait until 2.0?