Fixed JS sort comparator to be consistent between JS and PHP #7254

PVince81 · 2014-02-18T11:34:07Z

added OC.Util.naturalSortComparator that uses the same algo that was
used for the user list
changed user list and files list to use OC.Util.naturalSortComparator
removed toLowerCase() and changed the comparator to use
String.localeCompare()
added unit tests

Fixes for #7059, #3804

PVince81 · 2014-02-18T11:52:32Z

Looks like it doesn't work correctly when sorting file names like "a.txt", "a10.txt", "a2.txt"

PVince81 · 2014-02-18T12:06:43Z

common sort function for users and file list
natural sort on JS side
natural sort on PHP side (need to produce the same results as the JS function, according to the requirements from Sorting is off for filenames with (2) numbers in parentheses #7059)

PVince81 · 2014-02-18T15:52:57Z

Fix issue with sorting with capitalized umlauts

This is needed to make the sort order exactly the same as in JS.

PVince81 · 2014-02-19T09:04:44Z

Try and make it consistent across platforms 😦

PVince81 · 2014-02-19T10:41:40Z

Change the browser locale: run unit tests, check that the sort order after creating files/folders (JS sort) is still the same as after refreshing the page (PHP sort)

bantu · 2014-02-19T13:04:05Z

I don't like how a single class is used for everything.

bantu · 2014-02-19T13:07:57Z

.jshintrc

@@ -17,7 +17,7 @@
 	"globals": {
 		"console": true,
 		"it": true,
-		"itx": true,
+		"xit": true,


Is this a related change?

Yes, 'itx' was a mistake and one of the test uses "xit" (the phantomJS exclusion) which reminded me that this was wrong.

bantu · 2014-02-19T13:09:22Z

👎

Please extract the PHP comperator into its own class instead of using the Util class. Have a look at https://github.com/owncloud/core/pull/6778/files for inspiration. Is this maybe even the same or a related comperator?

bantu · 2014-02-19T13:13:28Z

You are saying this is performing a "natural sort". There are also the functions natsort() and natcasesort(), so it would be useful to document the differences between your implementation and these in the class description.
http://php.net/natsort

PVince81 · 2014-02-19T13:38:41Z

lib/private/util.php

+		// Needed because PHP doesn't sort correctly when numbers are enclosed in
+		// parenthesis, even with NUMERIC_COLLATION enabled.
+		// For example it gave ["test (2).txt", "test.txt"]
+		// instead of ["test.txt", "test (2).txt"]


@bantu documented here, that's one of the differences with natsort.
Also natsort didn't sort the umlauts properly.

PVince81 · 2014-02-19T13:43:10Z

@bantu I'll extract the comparator into a new class since it's already having three functions which is a bit too much for the Util class.
Would it be ok to keep Util::naturalSortComparator() and make that one use the new class ?

This sort is not the same as the one you linked (already discussed with @blizzz)

bantu · 2014-02-19T13:44:11Z

@PVince81 Sounds good.

PVince81 · 2014-02-20T11:36:30Z

Rebased+squashed.

I've moved the sorting algo into a separate class "OC_NaturalSort".

Still need to test this branch on IE, Safari and other browsers.

bantu · 2014-02-20T13:13:27Z

lib/private/naturalsort.php

+ *
+ */
+class OC_NaturalSort {
+	private static $collator;


Everything (methods too) should be non-static here. You should create instances of classes. In general, the number of "good use cases" for static is very limited.

bantu · 2014-02-20T13:21:40Z

@PVince81 More work is required here.

From an archetictural standpoint it might be a good idea to have multiple Comparator implementation, and a single Sorter that takes an instance of a Comparator and does the sorting. Not sure whether that is easily possible, though.

PVince81 · 2014-02-20T13:23:28Z

Opps, the file was supposed to be naturalsort.php, I might have committed it twice.
Will fix.

I wasn't sure whether to use a comparator instance or class, but ok I can change it to be an instance.
Is a static method getInstance() ok ? (PHP pattern-wise)

bantu · 2014-02-20T13:26:09Z

I wasn't sure whether to use a comparator instance or class, but ok I can change it to be an instance.
Is a static method getInstance() ok ? (PHP pattern-wise)

Not sure why that would be required. I would suggest to just use the new operator.

PVince81 · 2014-02-20T13:27:56Z

@bantu I've just deleted "naturalsortcomparator.php" which was the wrong class, the correct one is "naturalsort.php"

I don't want to "new" the class multiple times in the same php call.
getInstance() would create an instance once and returned the cached one.

@icewind1991 any comments about this pattern ?

bantu · 2014-02-20T13:38:31Z

I don't want to "new" the class multiple times in the same php call.
getInstance() would create an instance once and returned the cached one.

Okay. I see. Not sure whether it would be possible to have this in the container where it would be ensured that it is only created once.

icewind1991 · 2014-02-20T15:06:06Z

core/js/js.js

+				if (aNum == aa[x] && bNum == bb[x]) {
+					return aNum - bNum;
+				} else {
+					return aa[x].localeCompare(bb[x], 'en');


shouldn't the locale be determined from the used language?

Normally yes, but in our case we want it to match the sorting of PHP which locale is always set to English.
From my test cases it worked quite well even for German umlauts and Chinese characters.

Add a comment explaining that?

icewind1991 · 2014-02-20T15:07:41Z

I don't want to "new" the class multiple times in the same php call.
getInstance() would create an instance once and returned the cached one.

Okay. I see. Not sure whether it would be possible to have this in the container where it would be ensured that it is only created once.

We have the server container for that (OC::$server)

scrutinizer-notifier · 2014-08-11T11:30:04Z

A new inspection was created.

PVince81 · 2014-08-11T11:30:08Z

Rebased onto master, should run on Travis now.

PVince81 · 2014-08-11T12:07:48Z

@DeepDiver1975 @MorrisJobke segfault on Jenkins.
Shouldn't it run on Travis now ?

MorrisJobke · 2014-08-11T12:21:53Z

@PVince81 They run in parallel. Travis is just too slow yet. (just 5 jobs at the same time isn't enough to manage our workload)

ghost · 2014-08-19T23:04:53Z

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/6802/

PVince81 · 2014-08-20T08:06:19Z

Please review, unit tests finally pass!

MorrisJobke · 2014-08-20T08:42:20Z

👎 This doesn't fix the order after the reload.

MorrisJobke · 2014-08-20T08:44:13Z

Sorry: This fixes #7059 but not the sorting of the umlauts (correct: A Ä Z - this branch and master: A Z Ä) #3804

PVince81 · 2014-08-20T08:49:20Z

Hmm strange, it should. Will try again.

PVince81 · 2014-09-11T09:27:27Z

@MorrisJobke works for me.
I created three folders, "A", "Ä" and "Z" in the web UI. They are sorted correctly.
Then I refresh the page, they are still sorted correctly.
They do in Chrome.

Please provide more details and make sure you have the latest version of this branch that was rebased multiple times already.

Thanks.

MorrisJobke · 2014-09-15T13:25:08Z

I just checked out the latest change, and tried with firefox and chromium: https://cloud.morrisjobke.de/public.php?service=files&t=cac3ab8f16cca86e1e9b6bd4f09b3a3b

Both sort in the wrong way after the refresh. Are any special PHP modules required?

MorrisJobke · 2014-09-15T13:27:18Z

Are any special PHP modules required?

I just enabled the intl module -.- I will try again dum-di-dum

MorrisJobke · 2014-09-15T13:28:20Z

Nice one: Works excellent now 👍

PVince81 · 2014-09-16T15:18:05Z

@jancborchardt have you had a look at this ?

This needs another reviewer @th3fallen @LukasReschke

LukasReschke · 2014-09-16T15:28:59Z

👍

Fixed JS sort comparator to be consistent between JS and PHP

MorrisJobke added Bug labels Feb 19, 2014

bantu reviewed Feb 19, 2014
View reviewed changes

PVince81 reviewed Feb 19, 2014
View reviewed changes

bantu reviewed Feb 20, 2014
View reviewed changes

bantu closed this Feb 20, 2014

bantu reopened this Feb 20, 2014

icewind1991 reviewed Feb 20, 2014
View reviewed changes

Fixed sort algo for additional cases

f2001a4

MorrisJobke added 5 - To review and removed 4 - Developing labels Aug 20, 2014

MorrisJobke modified the milestones: ownCloud 7.0.2, 2014-sprint-02-current Aug 20, 2014

craigpg modified the milestones: 2014-sprint-03-next, 2014-sprint-02-current Sep 2, 2014

craigpg modified the milestones: 2014-sprint-04-current, 2014-sprint-03 Sep 15, 2014

LukasReschke added a commit that referenced this pull request Sep 16, 2014

Merge pull request #7254 from owncloud/core-sortalgo

d2743e6

Fixed JS sort comparator to be consistent between JS and PHP

LukasReschke merged commit d2743e6 into master Sep 16, 2014

LukasReschke deleted the core-sortalgo branch September 16, 2014 15:29

This was referenced Sep 16, 2014

Sorting is off for filenames with (2) numbers in parentheses #7059

Closed

Folders starting with UPPERCASE Umlaut (e.g. Ä, Ö, Ü) are not sorted correctly #3804

Closed

lock bot locked as resolved and limited conversation to collaborators Aug 16, 2019

Fixed JS sort comparator to be consistent between JS and PHP #7254

Fixed JS sort comparator to be consistent between JS and PHP #7254

Conversation

PVince81 commented Feb 18, 2014

PVince81 commented Feb 18, 2014

PVince81 commented Feb 18, 2014

PVince81 commented Feb 18, 2014

PVince81 commented Feb 19, 2014

PVince81 commented Feb 19, 2014

bantu commented Feb 19, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bantu commented Feb 19, 2014

bantu commented Feb 19, 2014

Choose a reason for hiding this comment

PVince81 commented Feb 19, 2014

bantu commented Feb 19, 2014

PVince81 commented Feb 20, 2014

Choose a reason for hiding this comment

bantu commented Feb 20, 2014

PVince81 commented Feb 20, 2014

bantu commented Feb 20, 2014

PVince81 commented Feb 20, 2014

bantu commented Feb 20, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

icewind1991 commented Feb 20, 2014

scrutinizer-notifier commented Aug 11, 2014

PVince81 commented Aug 11, 2014

PVince81 commented Aug 11, 2014

MorrisJobke commented Aug 11, 2014

ghost commented Aug 19, 2014

PVince81 commented Aug 20, 2014

MorrisJobke commented Aug 20, 2014

MorrisJobke commented Aug 20, 2014

PVince81 commented Aug 20, 2014

PVince81 commented Sep 11, 2014

MorrisJobke commented Sep 15, 2014

MorrisJobke commented Sep 15, 2014

MorrisJobke commented Sep 15, 2014

PVince81 commented Sep 16, 2014

LukasReschke commented Sep 16, 2014