Fix sorting of favorite files

The sort comparator checks the "isFavorite" property of the FileInfo
objects to compare. That property is set when the file list is loaded
and the response from the server is parsed, and thus a freshly loaded
file list has the proper sorting for favorite files. However, the
property is not set in other cases, like when the FileInfo objects are
derived from FileInfoModels due to a file being marked as a favorite or
a text editor being closed, which causes the file to be sorted in the
wrong position.

There is no need to add the property in those situations, though; in all
cases the TagsPlugin adds a "tags" array property that contains an
OC.TAG_FAVORITE tag, so that tag can be checked instead of "isFavorite".
Moreover, although "isFavorite" was added by the main "_parseFileInfo"
function it did not really belong there but to the "FileInfoParser" from
the TagsPlugin; however, as that property now is not used anywhere it
was removed altogether.

A cleaner solution would have been to make the sort comparator
extensible by plugins like other behaviours of the file list and then
add the sorting logic related to favorite files to the TagsPlugin.
However, right now only the TagsPlugin would need to alter the main
sorting logic, and it seems like a corner case anyway. Even if it is
implemented as a plugin, favorite files is a core feature, so for the
time being it will be taken into account directly in the main sorting
logic; making the sort comparator extensible by plugins is defered until
there are other use cases for that.

Fixes #5410

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>

2 files changed, 121 insertions, 2 deletions

diff --git a/apps/files/js/filelist.js b/apps/files/js/filelist.js
index b1e7c3f5f8c..086e148e102 100644
--- a/apps/files/js/filelist.js
+++ b/apps/files/js/filelist.js
@@ -1566,11 +1566,16 @@
 			this._sort = sort;
 			this._sortDirection = (direction === 'desc')?'desc':'asc';
 			this._sortComparator = function(fileInfo1, fileInfo2) {
-				if(fileInfo1.isFavorite && !fileInfo2.isFavorite) {
+				var isFavorite = function(fileInfo) {
+					return fileInfo.tags && fileInfo.tags.indexOf(OC.TAG_FAVORITE) >= 0;
+				};
+
+				if (isFavorite(fileInfo1) && !isFavorite(fileInfo2)) {
 					return -1;
-				} else if(!fileInfo1.isFavorite && fileInfo2.isFavorite) {
+				} else if (!isFavorite(fileInfo1) && isFavorite(fileInfo2)) {
 					return 1;
 				}
+
 				return direction === 'asc' ? comparator(fileInfo1, fileInfo2) : -comparator(fileInfo1, fileInfo2);
 			};
 
diff --git a/apps/files/tests/js/filelistSpec.js b/apps/files/tests/js/filelistSpec.js
index b7ee9c8554e..ddd9a45d153 100644
--- a/apps/files/tests/js/filelistSpec.js
+++ b/apps/files/tests/js/filelistSpec.js
@@ -2476,6 +2476,120 @@ describe('OCA.Files.FileList tests', function() {
 
 			sortStub.restore();
 		});
+		describe('with favorites', function() {
+			it('shows favorite files on top', function() {
+				testFiles.push(new FileInfo({
+					id: 5,
+					type: 'file',
+					name: 'ZZY Before last file in ascending order',
+					mimetype: 'text/plain',
+					mtime: 999999998,
+					size: 9999998,
+					// Tags would be added by TagsPlugin
+					tags: [OC.TAG_FAVORITE],
+				}), new FileInfo({
+					id: 6,
+					type: 'file',
+					name: 'ZZZ Last file in ascending order',
+					mimetype: 'text/plain',
+					mtime: 999999999,
+					size: 9999999,
+					// Tags would be added by TagsPlugin
+					tags: [OC.TAG_FAVORITE],
+				}));
+
+				fileList.setFiles(testFiles);
+
+				// Sort by name in ascending order (default sorting is by name
+				// in ascending order, but setFiles does not trigger a sort, so
+				// the files must be sorted before being set or a sort must be
+				// triggered afterwards by clicking on the header).
+				fileList.$el.find('.column-name .columntitle').click();
+				fileList.$el.find('.column-name .columntitle').click();
+
+				expect(fileList.findFileEl('ZZY Before last file in ascending order').index()).toEqual(0);
+				expect(fileList.findFileEl('ZZZ Last file in ascending order').index()).toEqual(1);
+				expect(fileList.findFileEl('somedir').index()).toEqual(2);
+				expect(fileList.findFileEl('One.txt').index()).toEqual(3);
+				expect(fileList.findFileEl('Three.pdf').index()).toEqual(4);
+				expect(fileList.findFileEl('Two.jpg').index()).toEqual(5);
+
+				// Sort by size in ascending order
+				fileList.$el.find('.column-size .columntitle').click();
+				fileList.$el.find('.column-size .columntitle').click();
+
+				expect(fileList.findFileEl('ZZY Before last file in ascending order').index()).toEqual(0);
+				expect(fileList.findFileEl('ZZZ Last file in ascending order').index()).toEqual(1);
+				expect(fileList.findFileEl('One.txt').index()).toEqual(2);
+				expect(fileList.findFileEl('somedir').index()).toEqual(3);
+				expect(fileList.findFileEl('Two.jpg').index()).toEqual(4);
+				expect(fileList.findFileEl('Three.pdf').index()).toEqual(5);
+
+				// Sort by modification time in ascending order
+				fileList.$el.find('.column-mtime .columntitle').click();
+				fileList.$el.find('.column-mtime .columntitle').click();
+
+				expect(fileList.findFileEl('ZZY Before last file in ascending order').index()).toEqual(0);
+				expect(fileList.findFileEl('ZZZ Last file in ascending order').index()).toEqual(1);
+				expect(fileList.findFileEl('One.txt').index()).toEqual(2);
+				expect(fileList.findFileEl('somedir').index()).toEqual(3);
+				expect(fileList.findFileEl('Three.pdf').index()).toEqual(4);
+				expect(fileList.findFileEl('Two.jpg').index()).toEqual(5);
+			});
+			it('shows favorite files on top also when using descending order', function() {
+				testFiles.push(new FileInfo({
+					id: 5,
+					type: 'file',
+					name: 'AAB Before last file in descending order',
+					mimetype: 'text/plain',
+					mtime: 2,
+					size: 2,
+					// Tags would be added by TagsPlugin
+					tags: [OC.TAG_FAVORITE],
+				}), new FileInfo({
+					id: 6,
+					type: 'file',
+					name: 'AAA Last file in descending order',
+					mimetype: 'text/plain',
+					mtime: 1,
+					size: 1,
+					// Tags would be added by TagsPlugin
+					tags: [OC.TAG_FAVORITE],
+				}));
+
+				fileList.setFiles(testFiles);
+
+				// Sort by name in descending order
+				fileList.$el.find('.column-name .columntitle').click();
+
+				expect(fileList.findFileEl('AAB Before last file in descending order').index()).toEqual(0);
+				expect(fileList.findFileEl('AAA Last file in descending order').index()).toEqual(1);
+				expect(fileList.findFileEl('Two.jpg').index()).toEqual(2);
+				expect(fileList.findFileEl('Three.pdf').index()).toEqual(3);
+				expect(fileList.findFileEl('One.txt').index()).toEqual(4);
+				expect(fileList.findFileEl('somedir').index()).toEqual(5);
+
+				// Sort by size in descending order
+				fileList.$el.find('.column-size .columntitle').click();
+
+				expect(fileList.findFileEl('AAB Before last file in descending order').index()).toEqual(0);
+				expect(fileList.findFileEl('AAA Last file in descending order').index()).toEqual(1);
+				expect(fileList.findFileEl('Three.pdf').index()).toEqual(2);
+				expect(fileList.findFileEl('Two.jpg').index()).toEqual(3);
+				expect(fileList.findFileEl('somedir').index()).toEqual(4);
+				expect(fileList.findFileEl('One.txt').index()).toEqual(5);
+
+				// Sort by modification time in descending order
+				fileList.$el.find('.column-mtime .columntitle').click();
+
+				expect(fileList.findFileEl('AAB Before last file in descending order').index()).toEqual(0);
+				expect(fileList.findFileEl('AAA Last file in descending order').index()).toEqual(1);
+				expect(fileList.findFileEl('Two.jpg').index()).toEqual(2);
+				expect(fileList.findFileEl('Three.pdf').index()).toEqual(3);
+				expect(fileList.findFileEl('somedir').index()).toEqual(4);
+				expect(fileList.findFileEl('One.txt').index()).toEqual(5);
+			});
+		});
 	});
 	describe('create file', function() {
 		var deferredCreate;