Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sorting items is not "stable" #185

Open
plequang opened this issue Jun 8, 2018 · 3 comments
Open

Sorting items is not "stable" #185

plequang opened this issue Jun 8, 2018 · 3 comments
Assignees

Comments

@plequang
Copy link
Contributor

plequang commented Jun 8, 2018

Description

Sorting items in an omnitable is not stable according to this definition https://en.wikipedia.org/wiki/Sorting_algorithm#Stability

First, on chrome, when sorting on a column for the first time with no grouping, the original items order is not preserved.
This is not a Chrome bug, since array sort does not have to be stable https://www.ecma-international.org/ecma-262/6.0/#sec-array.prototype.sort

The sort is not necessarily stable (that is, elements that compare equal do not necessarily remain in their original order).

Also, since we sort the items array in place, sorting on column c1, then c2, then c1 again will produce a different output.

Steps to reproduce

  1. sort on column c1
  2. observer order of items with equal c1 value
  3. sort on column c2
  4. sort on column c1 again
  5. observer order of items, which will be different for items with equal c1 value

Additionally, you can compare sort stability on different browser using:

var array = [
    {label: '2', value: 2},
    {label: '3.1', value: 3},
    {label: '3.2', value: 3},
    {label: '4', value: 4},
    {label: '3.3', value: 3},
    {label: '5', value: 5},
    {label: '3.4', value: 3},
    {label: '3.5', value: 3},
    {label: '6', value: 6},
    {label: '3.6', value: 3},
    {label: '1', value: 1},
];

array.sort((a,b) => { return a.value - b.value;});

console.log(array);

Expected results

Sorting should be stable, i.e. elements that compare equal when sorting should have their original order in the data array passed to the omnitable

@plequang
Copy link
Contributor Author

plequang commented Jun 8, 2018

I was thinking we could add __position property to elements we receive in the data property, and use that property to make sorting stable.
But I'm wondering if it is generally accepted for a public component to alter data that is passed to it.

@nomego
Copy link
Contributor

nomego commented Jun 8, 2018

Well I hope we never manipulate the object (or order) in data?
So we should be able to easily do this by changing sorter()

sorter(a, b) {
  const v1 = this.sortOnColumn.getComparableValue(a, this.sortOnColumn.sortOn),
    v2 = this.sortOnColumn.getComparableValue(b, this.sortOnColumn.sortOn),
    output = this._genericSorter(v1, v2);

  if (output === 0) {
    return this.data.indexOf(a) - this.data.indexOf(b);
  }

  return output;
},

@plequang
Copy link
Contributor Author

plequang commented Jun 8, 2018

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants