Merge pull request #9341 from owncloud/sharing-fileactions-fix

Fix fileActionsReady event after deferred file actions update

5 files changed, 106 insertions, 53 deletions

diff --git a/apps/files/js/fileactions.js b/apps/files/js/fileactions.js
index fc7c9ccacef..e06d2912274 100644
--- a/apps/files/js/fileactions.js
+++ b/apps/files/js/fileactions.js
@@ -181,11 +181,12 @@
 				return;
 			}
 			this.currentFile = parent;
+			var $tr = parent.closest('tr');
 			var self = this;
 			var actions = this.getActions(this.getCurrentMimeType(), this.getCurrentType(), this.getCurrentPermissions());
 			var file = this.getCurrentFile();
 			var nameLinks;
-			if (parent.closest('tr').data('renaming')) {
+			if ($tr.data('renaming')) {
 				return;
 			}
 
@@ -278,7 +279,7 @@
 			}
 
 			if (triggerEvent){
-				fileList.$fileList.trigger(jQuery.Event("fileActionsReady", {fileList: fileList}));
+				fileList.$fileList.trigger(jQuery.Event("fileActionsReady", {fileList: fileList, $files: $tr}));
 			}
 		},
 		getCurrentFile: function () {
diff --git a/apps/files/js/filelist.js b/apps/files/js/filelist.js
index 0477a657035..c7fccc5dd66 100644
--- a/apps/files/js/filelist.js
+++ b/apps/files/js/filelist.js
@@ -473,6 +473,7 @@
 		/**
 		 * Appends the next page of files into the table
 		 * @param animate true to animate the new elements
+		 * @return array of DOM elements of the newly added files
 		 */
 		_nextPage: function(animate) {
 			var index = this.$fileList.children().length,
@@ -483,7 +484,7 @@
 				isAllSelected = this.isAllSelected();
 
 			if (index >= this.files.length) {
-				return;
+				return false;
 			}
 
 			while (count > 0 && index < this.files.length) {
@@ -496,12 +497,17 @@
 				}
 				if (animate) {
 					tr.addClass('appear transparent');
-					newTrs.push(tr);
 				}
+				newTrs.push(tr);
 				index++;
 				count--;
 			}
 
+			// trigger event for newly added rows
+			if (newTrs.length > 0) {
+				this.$fileList.trigger($.Event('fileActionsReady', {fileList: this, $files: newTrs}));
+			}
+
 			if (animate) {
 				// defer, for animation
 				window.setTimeout(function() {
@@ -510,6 +516,7 @@
 					}
 				}, 0);
 			}
+			return newTrs;
 		},
 
 		/**
@@ -518,9 +525,16 @@
 		 */
 		_onFileActionsUpdated: function() {
 			var self = this;
-			this.$fileList.find('tr td.filename').each(function() {
-				self.fileActions.display($(this), true, self);
+			var $files = this.$fileList.find('tr');
+			if (!$files.length) {
+				return;
+			}
+
+			$files.each(function() {
+				self.fileActions.display($(this).find('td.filename'), false, self);
 			});
+			this.$fileList.trigger($.Event('fileActionsReady', {fileList: this, $files: $files}));
+
 		},
 
 		/**
@@ -532,7 +546,6 @@
 			// detach to make adding multiple rows faster
 			this.files = filesArray;
 
-			this.$fileList.detach();
 			this.$fileList.empty();
 
 			// clear "Select all" checkbox
@@ -541,10 +554,7 @@
 			this.isEmpty = this.files.length === 0;
 			this._nextPage();
 
-			this.$el.find('thead').after(this.$fileList);
-
 			this.updateEmptyContent();
-			this.$fileList.trigger($.Event('fileActionsReady', {fileList: this}));
 
 			this.fileSummary.calculate(filesArray);
 
@@ -1282,16 +1292,16 @@
 								// reinsert row
 								self.files.splice(tr.index(), 1);
 								tr.remove();
-								self.add(fileInfo, {updateSummary: false, silent: true});
-								self.$fileList.trigger($.Event('fileActionsReady', {fileList: self}));
+								tr = self.add(fileInfo, {updateSummary: false, silent: true});
+								self.$fileList.trigger($.Event('fileActionsReady', {fileList: self, $files: $(tr)}));
 							}
 						});
 					} else {
 						// add back the old file info when cancelled
 						self.files.splice(tr.index(), 1);
 						tr.remove();
-						self.add(oldFileInfo, {updateSummary: false, silent: true});
-						self.$fileList.trigger($.Event('fileActionsReady', {fileList: self}));
+						tr = self.add(oldFileInfo, {updateSummary: false, silent: true});
+						self.$fileList.trigger($.Event('fileActionsReady', {fileList: self, $files: $(tr)}));
 					}
 				} catch (error) {
 					input.attr('title', error);
diff --git a/apps/files/tests/js/filelistSpec.js b/apps/files/tests/js/filelistSpec.js
index a699177767a..713cd5468d8 100644
--- a/apps/files/tests/js/filelistSpec.js
+++ b/apps/files/tests/js/filelistSpec.js
@@ -797,13 +797,24 @@ describe('OCA.Files.FileList tests', function() {
 			fileList.$fileList.on('fileActionsReady', handler);
 			fileList.setFiles(testFiles);
 			expect(handler.calledOnce).toEqual(true);
+			expect(handler.getCall(0).args[0].$files.length).toEqual(testFiles.length);
 		});
 		it('triggers "fileActionsReady" event after single add', function() {
 			var handler = sinon.stub();
+			var $tr;
 			fileList.setFiles(testFiles);
 			fileList.$fileList.on('fileActionsReady', handler);
-			fileList.add({name: 'test.txt'});
+			$tr = fileList.add({name: 'test.txt'});
+			expect(handler.calledOnce).toEqual(true);
+			expect(handler.getCall(0).args[0].$files.is($tr)).toEqual(true);
+		});
+		it('triggers "fileActionsReady" event after next page load with the newly appended files', function() {
+			var handler = sinon.stub();
+			fileList.setFiles(generateFiles(0, 64));
+			fileList.$fileList.on('fileActionsReady', handler);
+			fileList._nextPage();
 			expect(handler.calledOnce).toEqual(true);
+			expect(handler.getCall(0).args[0].$files.length).toEqual(fileList.pageSize);
 		});
 		it('does not trigger "fileActionsReady" event after single add with silent argument', function() {
 			var handler = sinon.stub();
@@ -1630,6 +1641,7 @@ describe('OCA.Files.FileList tests', function() {
 		});
 		it('redisplays actions when new actions have been registered', function() {
 			var actionStub = sinon.stub();
+			var readyHandler = sinon.stub();
 			var clock = sinon.useFakeTimers();
 			var debounceStub = sinon.stub(_, 'debounce', function(callback) {
 				return function() {
@@ -1637,11 +1649,15 @@ describe('OCA.Files.FileList tests', function() {
 					_.defer(callback);
 				};
 			});
+
 			// need to reinit the list to make the debounce call
 			fileList.destroy();
 			fileList = new OCA.Files.FileList($('#app-content-files'));
 
 			fileList.setFiles(testFiles);
+
+			fileList.$fileList.on('fileActionsReady', readyHandler);
+
 			fileList.fileActions.register(
 				'text/plain',
 				'Test',
@@ -1654,9 +1670,13 @@ describe('OCA.Files.FileList tests', function() {
 			);
 			var $tr = fileList.findFileEl('One.txt');
 			expect($tr.find('.action-test').length).toEqual(0);
+			expect(readyHandler.notCalled).toEqual(true);
+
 			// update is delayed
 			clock.tick(100);
 			expect($tr.find('.action-test').length).toEqual(1);
+			expect(readyHandler.calledOnce).toEqual(true);
+
 			clock.restore();
 			debounceStub.restore();
 		});
diff --git a/apps/files_sharing/js/share.js b/apps/files_sharing/js/share.js
index 2efed5310bc..e46be4ada46 100644
--- a/apps/files_sharing/js/share.js
+++ b/apps/files_sharing/js/share.js
@@ -36,54 +36,34 @@
 					}
 					return tr;
 				};
-
-				var oldRenderRow = OCA.Files.FileList.prototype._renderRow;
-				OCA.Files.FileList.prototype._renderRow = function(fileData) {
-					var $tr = oldRenderRow.apply(this, arguments);
-					// if the statuses are loaded already, use them for the icon
-					// (needed when scrolling to the next page)
-					var shareStatus = OC.Share.statuses[fileData.id];
-					if (fileData.shareOwner || fileData.recipientsDisplayName || shareStatus) {
-						var permissions = $tr.data('permissions');
-						var hasLink = !!(shareStatus && shareStatus.link);
-						if (permissions & OC.PERMISSION_SHARE) {
-							OC.Share.markFileAsShared($tr, true, hasLink);
-						} else {
-							// if no share action exists because the admin disabled sharing for this user
-							// we create a share notification action to inform the user about files
-							// shared with him otherwise we just update the existing share action.
-							// TODO: make this work like/with OC.Share.markFileAsShared()
-							var shareNotification = '<a class="action action-share-notification permanent"' +
-									' data-action="Share-Notification" href="#" original-title="">' +
-									' <img class="svg" src="' + OC.imagePath('core', 'actions/share') + '"></img>';
-							$tr.find('.fileactions').append(function() {
-								var shareBy = t('files_sharing', 'Shared by {owner}', {owner: escapeHTML(fileData.shareOwner)});
-								var $result = $(shareNotification + '<span> ' + shareBy + '</span></span>');
-								$result.on('click', function() {
-									return false;
-								});
-								return $result;
-							});
-						}
-					}
-					return $tr;
-				};
 			}
 
 			// use delegate to catch the case with multiple file lists
-			$('#content').delegate('#fileList', 'fileActionsReady',function(ev){
+			$('#content').delegate('#fileList', 'fileActionsReady', function(ev){
 				var fileList = ev.fileList;
+				var $files = ev.$files;
+
+				function updateIcons($files) {
+					if (!$files) {
+						// if none specified, update all
+						$files = fileList.$fileList.find('tr');
+					}
+					_.each($files, function(file) {
+						OCA.Sharing.Util.updateFileActionIcon($(file));
+					});
+				}
+
 				if (!OCA.Sharing.sharesLoaded){
-					OC.Share.loadIcons('file', fileList);
+					OC.Share.loadIcons('file', fileList, function() {
+						// since we don't know which files are affected, just refresh them all
+						updateIcons();
+					});
 					// assume that we got all shares, so switching directories
 					// will not invalidate that list
 					OCA.Sharing.sharesLoaded = true;
 				}
 				else{
-					// this will update the icons for all the currently visible elements
-					// additionally added elements when scrolling down will be
-					// updated in the _renderRow override
-					OC.Share.updateIcons('file', fileList);
+					updateIcons($files);
 				}
 			});
 
@@ -141,6 +121,40 @@
 		},
 
 		/**
+		 * Update the file action share icon for the given file
+		 *
+		 * @param $tr file element of the file to update
+		 */
+		updateFileActionIcon: function($tr) {
+			// if the statuses are loaded already, use them for the icon
+			// (needed when scrolling to the next page)
+			var shareStatus = OC.Share.statuses[$tr.data('id')];
+			if (shareStatus || $tr.attr('data-share-recipients') || $tr.attr('data-share-owner')) {
+				var permissions = $tr.data('permissions');
+				var hasLink = !!(shareStatus && shareStatus.link);
+				OC.Share.markFileAsShared($tr, true, hasLink);
+				if ((permissions & OC.PERMISSION_SHARE) === 0) {
+					// if no share action exists because the admin disabled sharing for this user
+					// we create a share notification action to inform the user about files
+					// shared with him otherwise we just update the existing share action.
+					// TODO: make this work like/with OC.Share.markFileAsShared()
+					$tr.find('.fileactions .action-share-notification').remove();
+					var shareNotification = '<a class="action action-share-notification permanent"' +
+							' data-action="Share-Notification" href="#" original-title="">' +
+							' <img class="svg" src="' + OC.imagePath('core', 'actions/share') + '"></img>';
+					$tr.find('.fileactions').append(function() {
+						var shareBy = t('files_sharing', 'Shared by {owner}', {owner: escapeHTML($tr.attr('data-share-owner'))});
+						var $result = $(shareNotification + '<span> ' + shareBy + '</span></span>');
+						$result.on('click', function() {
+							return false;
+						});
+						return $result;
+					});
+				}
+			}
+		},
+
+		/**
 		 * Formats a recipients array to be displayed.
 		 * The first four recipients will be shown and the
 		 * other ones will be shown as "+x" where "x" is the number of
diff --git a/apps/files_sharing/tests/js/shareSpec.js b/apps/files_sharing/tests/js/shareSpec.js
index 3d4fc754821..600859b4e7d 100644
--- a/apps/files_sharing/tests/js/shareSpec.js
+++ b/apps/files_sharing/tests/js/shareSpec.js
@@ -80,6 +80,14 @@ describe('OCA.Sharing.Util tests', function() {
 		// TODO: test data-permissions, data-share-owner, etc
 	});
 	describe('Share action icon', function() {
+		beforeEach(function() {
+			OC.Share.statuses = {1: {link: false, path: '/subdir'}};
+			OCA.Sharing.sharesLoaded = true;
+		});
+		afterEach(function() {
+			OC.Share.statuses = {};
+			OCA.Sharing.sharesLoaded = false;
+		});
 		it('do not shows share text when not shared', function() {
 			var $action, $tr;
 			OC.Share.statuses = {};