setSorting doesn't properly overwrite/remove comparator for client collections

[toekneestuck]

Currently at the end of the setSorting function, this is happening:

if (delComp) delete this.comparator;
if (delFullComp && fullCollection) delete fullCollection.comparator;

However, this doesn't work as expected if a default comparator is defined on the collection. Instead, it will fallback to inheriting the default comparator, which has interesting results (and could be desirable in some cases). What effectively ends up happening is a sorting of the sorted data. It will sort the fullCollection properly and then apply the default comparator to the already sorted data.

What I would propose is explicitly setting this.comparator to null when there is a sortKey present, and removing all together when there is no sortKey (which restores the 'default' sort).

if (delComp) !sortKey ? delete this.comparator : this.comparator = null;
if (delFullComp && fullCollection) !sortKey ? delete fullCollection.comparator : fullCollection.comparator = null;

[wyuenho]

Under what circumstances would you have a default comparator on a PageableCollection? Why would you make your own comparator instead of using the one PageableCollection makes? If you attach a default comparator to a collection, is removing it silently and automatically necessary the right thing to do?

[toekneestuck]

In my particular instance, I have a default comparator on the collection because the API I'm using doesn't guarantee the results in a consistent order (and there is no 'order' parameter to the API unfortunately). I could also see this being useful where you want the default sorting to be a calculation of more than one attribute.

I think the main value in this is for people migrating from a standard Backbone.Collection to a PageableCollection where previously they used a comparator. This would make the migration more seamless, preventing any unexpected results (as it relates to sorting).

To answer your last question, I think it's the right thing to do primarily because while using the sorting feature of PageableCollection, I don't think the expectation is performing a multi-sort of the data just because you have a default comparator defined. While the effects can be pretty cool, if you're trying to perform a multi-sort, don't you think it should be dealt with in a different/clearer way?

[wyuenho]

I still fail to understand what the issue is. The default comparator on the current page will be automatically removed under the default settings under client mode already, and the code that you produced is exactly equivalent in any case.

The only time it will be left in place is when you have a default comparator on a server mode pageable collection, which you can replace by calling setSorting with an option {full: false}. The reason options.full is not false by default under server mode is because under most circumstances the server side collection will be fully sorted when sent down.

[toekneestuck]

Since you asked for a test case before you edited your latest comment, here is one: http://jsfiddle.net/8xVh6/

Side by side you'll see the paginated collection and the full collection rendered. Sort by "priority" on either a couple times and you'll notice that the paginated collection sorts by priority, and then again by "type", which essentially just sorts them by "priority" within the already sorted "type". When you look at the full collection, you'll see that it properly sorts by priority, rearranging the entire list.

The code I suggested does not produced the equivalent of this, as setting the comparator to null (instead of deleting it on the instance) will prevent the paginated collection from inheriting the default comparator defined on the collection class. Here is an example where I edited setSorting to include the code I suggested, and sorting works as expected: http://jsfiddle.net/8YbJb/

Are you expecting the following code in the constructor to remove the comparator for the instance?

if (comparator && options.full) {
      delete this.comparator;
      fullCollection.comparator = comparator;
}

If so, this does not actually work. It merely removes the comparator method on the instance, but the original comparator (defined in the extend) is still inherited through the prototype chain. You would need to set this.comparator = null (or some equivalent) so this.comparator will be falsy.

Either way, I would not recommend doing this, as it unnecessarily forces people to adhere to the Backbone.Pageable way of configuring sorting, when using Backbone's built in configuration can suit just as well.

[wyuenho]

Ah right I see. In that case I think instead of delete, just setting the comparators to null regardless of sortKey will be fine. Automatic multi-sort was never an intention, and it is best done explicitly by the caller anyway. I'm going to bed, my brain's been fried. If you sent me a PR before I get to it later this weekend, I'll merge.

Thanks for the heads up!

[toekneestuck]

No problem—I'll get a PR setup soon and send it your way