fix: row selection not sorted numerically

[muendlein]

• issues: fixes Selected rows are not numerically sorted. #1194

[ghiscoding]

the sort comparer function is the same, you can maybe move it outside as a single sort function, something like this maybe:

const sortNumbers = (a, b) => a - b;
if (!this.arrayEquals(previousSelectedRows.sort(sortNumbers), this.selectedRows.sort(sortNumbers))) {
...

[ghiscoding]

tested that in my own Slickgrid-Universal project and doing this change can potentially cause other arrays to become out of sync (mainly the Ids become out of sync with row indexes). That is actually how I detected the issue, I got a lot more E2E tests in my project and I only use the DataView and got a few failures because of this change. So if you use the DataView then these become out of sync, meaning that row indexes no longer match row ids:

• dataView.mapRowsToIds()
• dataView.getAllSelectedIds()
• dataView.getAllSelectedFilteredIds()
• .... maybe some other SlickGrid functions as well?

We could have them all sort by their ids so they match the row indexes with row ids, that would work if ids are numbers or strings as number... but what if the ids are GUID or other type of strings... then we're out of luck.

So having all of these things out of synch is seriously not ideal and in the end it might be better to just leave that to the end user (like I mentioned before)...

OR the best we could do is maybe to provide an extra argument to the get function to optionally sort but leave it as unsorted by default (so it would still match the ids by default)

getSelectedRows(sortRows = false) {
  ...
  const selectedRows = this.selectedRows().slice(0);
  if (sortRows) {
    return selectedRows.sort((a, b) a - b);
  }
  return selectedRows;
}

[muendlein]

tested that in my own Slickgrid-Universal project and doing this change can potentially cause other arrays to become out of sync (mainly the Ids become out of sync with row indexes). That is actually how I detected the issue, I got a lot more E2E tests in my project and I only use the DataView and got a few failures because of this change. So if you use the DataView then these become out of sync, meaning that row indexes no longer match row ids:

• dataView.mapRowsToIds()
• dataView.getAllSelectedIds()
• dataView.getAllSelectedFilteredIds()
• .... maybe some other SlickGrid functions as well?

We could have them all sort by their ids so they match the row indexes with row ids, that would work if ids are numbers or strings as number... but what if the ids are GUID or other type of strings... then we're out of luck.

So having all of these things out of synch is seriously not ideal and in the end it might be better to just leave that to the end user (like I mentioned before)...

OR the best we could do is maybe to provide an extra argument to the get function to optionally sort but leave it as unsorted by default (so it would still match the ids by default)

getSelectedRows(sortRows = false) {
  ...
  const selectedRows = this.selectedRows().slice(0);
  if (sortRows) {
    return selectedRows.sort((a, b) a - b);
  }
  return selectedRows;
}

If that's the case, it is probably best to leave it as is. Generally however it seems pretty fragile that row indices must not be sorted numerically.