sortBy reverses the order of items when they are considered equal

[AdamWillden]

I'm not sure if this is only on the change-sort-order feature branch or if it exists in the current distribution.

I can toggle which columns are sorted which populates the mapping array. When no columns are sorted I return an empty array array.sortBy(function(entity, descending) { /*...*/ return []; }). I've only just noticed that the original array gets reversed.

The trick to fix this is to change if (comparefn(items[mid], newItem) < 0) { to if (comparefn(items[mid], newItem) <= 0) { in findInsertionIndex:

function findInsertionIndex(items, newItem, comparefn) {
        var left = -1,
            right = items.length,
            mid;
        while (right - left > 1) {
            mid = (left + right) >>> 1;
            if (comparefn(items[mid], newItem) <= 0) {
                left = mid;
            } else {
                right = mid;
            }
        }
        return right;
    }

I notice a similar line in binarySearch but am not sure if that needs changing too? I don't think it does.

---

I noticed this issue most when I had two chained sortBy projections. The first sorted the array in one direction as desired then the second (with an empty mapping array) was reversing the result.

[AdamWillden]

In fact this may need more depending on whether you think the following is a bug:

Upon returning an empty mapping array I would expect the order to be the same as the underlying array. So long as the data has never been sorted by the projection this works using the above fix. However if I sort and then stop sorting the array the data will remain sorted rather than returning to the underlying order.

[sunesimonsen]

Sorry for the radio silence. I'll see if I get the time tonight to look into the issue.

[AdamWillden]

No problem at all @sunesimonsen it's the holiday season, I just figured you were away with better things to do! Thanks for working on the projections, your additions are incredibly useful to me.

[sunesimonsen]

@AdamWillden hi again, now I read your issue. I glad you fund our work useful. We use projections increasingly at One.com, mainly for views with 5000+ items.

I totally understand your intuition about an empty mapping would result in the original sort order. It would be cool if it worked that way, but the projection rely heavily on the items being sorting in a deterministic order that is independent of the original arrays order. That is one of the reasons why it is so fast. If we defined the empty mapping to result in the original sorting order, we would need to update the output array every time the underlying array changes - that is expensive. I don't know if it is possible to just special case this situation and forward the underlying array and do a complete resort when a new sort order is introduced. I don't have time to look into that unless it is a use case needed by One.com - @mwoc, @gertsonderby do you need this feature for our products?

A simpler solution for your specific problem is to use the original array when no sorting is selected and use the sorted projection when sorting is enabled. Here is the pseudo code:

var items = ko.observableArray([...]);
var sortedItems = items.sortBy(function (entity, descending) { ... });
var itemsView = ko.computed({
    return sorted ? sortedItems : items;
})

I know that is not too sexy. Are you sure there isn't a default sort order you can use in the case where no order has been selected? I don't know your use case, but it seems kind of arbitrary to use the insertion order. Creation time?, last changed? sequence id? I don't know.

Kind regards Sune

[AdamWillden]

@sunesimonsen thanks for the detailed response. It's much appreciated!

If you'd consider fixing the initial array order as per the first post that would be of great help to me and seems no more than a one character change. It means I can return the data from the server in a particular order. What the user does from there regarding sort order is up to them.

The matter of maintaining the array sorted as per the underlying array isn't too much of an issue; I understood it may not have been considered a bug and that's no problem :-)

[sunesimonsen]

@AdamWillden I know it is a little favor you are asking for. But I'm a little hesitant to change this algorithm to make an unsupported behavior work for you. The reason is that I think the solution will come back and bite you for some edge case. My first instinct was that the code should throw an exception if the mapping returned an empty array, but that would block you even further.

Would it be possible to add a sequence number to each of the entities? Then you could sort by that in the default case.

[AdamWillden]

Okay fair enough, I understand hesitancy in changing something that works for the sake of something unsupported.

Also at the end of the day, with your supported usage, if two items are equal it shouldn't matter if they're reversed. So it is not a bug, I'll just make a local build with that change and pull your changes in as you make them.

[sunesimonsen]

Sorry that I couldn't be of more help, I'm still considering if supporting having no specific sort order: I created an issue for that #5, I'll let you know if we are going to implement it. Till then it sounds like you have a backup plan.

[AdamWillden]

No problem at all @sunesimonsen I watch/subscribe to this repository so will keep an eye out. As you say I have the backup plan I need for my project in the short term :-)