[Patch][CLA] ComboBox sort functionality enhancement

[mdknapp@…]

Allow sorting of a combobox's data store.

Patch for ComboBox?.js and testcases attached.

CLA on file: Matthew Knapp

[guest]

ComboBox?.js patch to allow sorting of data store

[guest]

test_ComboBox.html patch to test ComboBox? data store sort

[doughays]

The provided patch does not implement sort for the built-in dijit.form._ComboBoxDataStore defined in ComboBox?.js.
I see at least 4 alternatives:
1) implemement sort for the internal store (hard due to the multi-field capabilities of the sort object)
2) instead of sort, just support descending:boolean attribute which would be much easier to implement in home grown data stores
3) instead of sort, support a constraints type object that is just mixin'ed to the data.store parameter currently being passed, then the internal store wouldn't have to support sort and also other data.store attributes could be passed like queryOptions
4) don't support sort for the internal store and let the web page author presort the data manually

I'm deferring this for now pending more discussion.
I currently favor 2), but 3) and 4) seem OK.

[guest]

Replying to doughays:

The provided patch does not implement sort for the built-in dijit.form._ComboBoxDataStore defined in ComboBox??.js.

This is really a ticketable issue with the built-in dijit.form._ComboBoxDataStore. The documentation in the dojo.data.read api specifies (emphasis added):

All implementations should accept keywordArgs objects with any of the 9 standard properties: query, onBegin, onItem, onComplete, onError, scope, sort, start, and count. Some implementations may accept additional properties in the keywordArgs object as valid parameters, such as {includeOutliers:true}.

As per standard definitions of should and may the _ComboBoxDataStore chooses not to implement the sort at its own peril. Perhpas just documenting that sort is only valid on underlying data stores that properly and fully implement the data.read api? That said, a sort should be a valid arguement to any fetch on underlying data objects and there shouldn't be any problem with it being part of the combobox.

1) implemement sort for the internal store (hard due to the multi-field capabilities of the sort object)

1) In light of the requirements for a read from a data store, out of the 4 solutions, this is the only solution that seems to make sense for the _ComboBoxDataStore, complex or not.

2) instead of sort, just support descending:boolean attribute which would be much easier to implement in home grown data stores

2) People don't always just want a support on displayed values. If the underlying data is stored in a data store we should expose the features of that data store. Home grown data stores still need to support the read functionality as outlined in dojo.data.read

3) instead of sort, support a constraints type object...

3) Although making a datastore constraints field may be preferred to exposing all the individual data store options on the combobox, this doesn't address the fact that the _ComboBoxDataStore chooses not to adhere to the data.read api (for good or bad causes). Should we work around other data stores that choose not to support a "query"?

4) don't support sort for the internal store and let the web page author presort the data manually

4) Is fine if internal store refers to the "built-in" dijit.form._ComboBoxDataStore. Otherwise it will not capture the dynamic nature of data stores. If the data store changes the sort may be changed which is the reason for the data store being queried and returning a result set.

Regardless, it appears the issue is with the relatively new (it doesn't exist in 1.0.2) _ComboBoxDataStore deviating from what a data store should be by choosing not to incorporate sort capability.

Perhaps the solution is to not even try to make the _ComboBoxDataStore present itself as a data store but to treat it as something else entirely. Is there a need for a Data Store "light" that has a much simplified contract to encompass these "home grown" data stores?

[dante]

(In [13627]) refs #6021 - typo

[haysmark]

Bill, I need an architecture decision from you.

Supporting all of these dojo.data attributes is becoming a maintenance problem. First it was 'deep', now this 'sort', and probably something else sooner or later. I think it would be great if we did doughays's #3 and exposed some kind of dojo.data "constraints" object on the ComboBox?. I was thinking we could mix that into the dojo.data query (not to be confused with the 'query' attribute already on ComboBox?) so we don't have to hardcode these dojo.data attributes into the ComboBox? anymore. What do you think?

Also, what do you want to do about the ComboBox? internal data store for option tags? Obviously it doesn't support sort ATM. Should we defer that for another release?

[haysmark]

Heh meant to change the owner for now.

[bill]

Hmm, I'm not sure how that would work, given that "deep" and "sort" are separate args to fetch:

 store.fetch({queryOptions:{ignoreCase:this.ignoreCase, deep:true}, 
query: query, sort: this.sort, ...
, start:0, count:this.pageSize});

I can see queryOptions being an option like "constraints" but don't know how sort fits into that?

Also, what other possible future parameters are there?

[haysmark]

Bill, I was thinking that we would mix in the constraints with the outer object we pass to fetch. So if the user wanted to set deep and sort, they could set the constraints to:

mycombobox.dojodataconstraints={queryOptions:{deep:false,ignoreCase:true}, sort:mysort, myattr:true}

Then we would do a dojo.mixin to copy these values over the defaults we hardcode into the ComboBox?:

var defaultfetch={queryOptions:{ignoreCase:this.ignoreCase, deep:true}, 
query: query, sort: this.sort, ...
, start:0, count:this.pageSize};
var newfetch=dojo.mixin(defaultfetch, this.dojodataconstraints); // or whatever the syntax is
store.fetch(newfetch);

With this model, users have the power to add a custom attribute, myattr, to the fetch. Their data stores can then respond to this custom attr and return the appropriate results, without directly filtering on it like it would with the query attribute. For example, we have an IBM product team that uses a modified ItemFileReadStore? and wants to pass the language to display the ComboBox? menu in, and right now they are changing the ComboBox? source to add a lang attribute to the fetch, which is silly. They don't want to pass it to the query because that's what the ItemFileReadStore? filters on.

As for other attributes. the dojo.data Read API requires yet another one: scope. It also says, "Some implementations may accept additional properties in the keywordArgs object as valid parameters, such as {includeOutliers:true}."

So we can just hardcode a sort param like the patch suggests, but I forsee that it will be one of a series of updates we will have to make as dojo.data evolves.

[haysmark]

Fixes #6021. Added fetchProperties mixin attribute to ComboBox? to support both present and future dojo.data attributes. Added tests to test_ComboBox demonstrating sort both in markup and programmatically.

[haysmark]

Here is a new patch that adds broader support for dojo.data and also adds sort to the ComboBoxDataStore? (sort comes for free with an existing dojo.data require in ComboBox?.js).

[bill]

(In [14219]) Add ability to pass sort flag or other parameters to ComboBox?'s data store. Also adds sort capability to ComboBoxDataStore? built in data store for inlined data (sort comes for free with an existing dojo.data require in ComboBox?.js).

Patch from Mark Hays. Fixes #6021. !strict