Opened 7 years ago

Closed 7 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)

cms.diff (1.5 KB) - added by adros 7 years ago.
test_CheckedMultiSelect.html.diff (2.0 KB) - added by adros 7 years ago.
Patch for automated test

Download all attachments as: .zip

Change History (10)

Changed 7 years ago by adros

Attachment: cms.diff added

comment:1 Changed 7 years ago by bill

Although, that will break support for the old dojo.data stores. Maybe it should branch, or the patch should wait until 2.0?

comment:2 Changed 7 years ago by adros

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 7 years ago by bill

Owner: changed from dante to adros
Status: newpending

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 7 years ago by adros

Patch for automated test

comment:4 Changed 7 years ago by adros

Status: pendingnew

Attachment (test_CheckedMultiSelect.html.diff) added by ticket reporter.

comment:5 Changed 7 years ago by bill

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 7 years ago by adros

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 7 years ago by bill

Milestone: tbd1.9
Owner: changed from adros to bill
Status: newassigned

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.

comment:8 Changed 7 years ago by bill

Resolution: fixed
Status: assignedclosed

In [30865]:

Remove setStore() override method. It was there to let the store indicate which items were originally selected, but didn't seem to work correctly (refs #16828), and broke support for the new dojo/store API. It's probably something that should be handled by dojox/mvc anyway. Fixes #16815 !strict. Thanks to adros (CLA on file) for the patch to the test.

Note: See TracTickets for help on using tickets.