Skip to content

Conversation

@julien-nc
Copy link
Member

Javascript array sort function must return -1, 0 or 1 😁.

This can be backported to stable22 only.

@julien-nc julien-nc added this to the Nextcloud 23 milestone Jul 26, 2021
@julien-nc julien-nc changed the title Fix sort sort function of files multiple selection actions Fix sort function of files multiple selection actions Jul 26, 2021
@julien-nc
Copy link
Member Author

/backport to stable22

skjnldsv
skjnldsv previously approved these changes Jul 26, 2021
@skjnldsv
Copy link
Member

skjnldsv commented Jul 26, 2021

Yes and no.
Sort allows upper and lower values
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/sort

var numbers = [4, 2, 5, 1, 3];
numbers.sort(function(a, b) {
  return a - b;
});
console.log(numbers);

// [1, 2, 3, 4, 5]

If compareFunction(a, b) returns a value > than 0, sort b before a.
If compareFunction(a, b) returns a value ≤ 0, leave a and b in the same order.

@skjnldsv skjnldsv self-requested a review July 26, 2021 10:25
@skjnldsv skjnldsv dismissed their stale review July 26, 2021 10:25

Not sure it's really necessary

@julien-nc
Copy link
Member Author

Not sure it's really necessary

I think it is because, currently, sort function return values are wrong as a boolean is returned. So the array is actually not sorted, its order stays the same.

And sorry for the poor description of what's expected of the sort function 😁

@skjnldsv
Copy link
Member

then shouldn't it be a.order - b.order

@julien-nc
Copy link
Member Author

julien-nc commented Jul 26, 2021

Well what if the order is undefined? (which is the case for the recent tag multiselect action 😁)

We would need something like

return (a.order || 1000) - (b.order || 1000)

to make sure the ones without order are placed at the end.

@skjnldsv
Copy link
Member

If the order is undefined it's most likely going to break somewhere else, no? xD
But yes, I'm fine with whatever you want, let's get this in! 🚀

@artonge
Copy link
Contributor

artonge commented Jul 26, 2021

What's the state here ?

Just tested and:

[1, 5, -1, undefined, -100].sort((a, b)=> a - b)
Array(5) [ -100, -1, 1, 5, undefined ]

[1, 5, -1, undefined, -100].sort((a, b)=> b - a)
Array(5) [ 5, 1, -1, -100, undefined ]

So this should be fine I guess as undefines are at the end

return b.order - a.order

@julien-nc julien-nc force-pushed the fix/file-multiselect-actions-sort branch from 91cc102 to d6b4944 Compare July 26, 2021 12:08
@julien-nc
Copy link
Member Author

@artonge Very true, thanks. Let's roll.

What's the state here ?

State is now yours to decide 😁.

@julien-nc julien-nc merged commit 4c400a0 into master Jul 26, 2021
@julien-nc julien-nc deleted the fix/file-multiselect-actions-sort branch July 26, 2021 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants