From 586b3a9683421181b7cd66aff2d3fabd0f34531e Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Fri, 27 Jun 2014 13:36:18 +0200 Subject: [PATCH] Sync file list with file actions Whenever file actions are registered later, now the file lists are automatically notified. Added FileActions.addUpdateListener() to be able to receive such notifications. This removes the need for apps to manually call FileActions.display() after registering new actions. This fixes issues with race conditions when file actions are registered after the file list was already rendered. --- apps/files/js/app.js | 4 ++ apps/files/js/fileactions.js | 55 +++++++++++++++++-- apps/files/js/filelist.js | 22 ++++++++ apps/files/tests/js/fileactionsSpec.js | 53 +++++++++++++++++- apps/files/tests/js/filelistSpec.js | 32 +++++++++++ apps/files_external/tests/appSpec.js | 1 + .../tests/js/mountsfilelistSpec.js | 1 + apps/files_sharing/tests/js/appSpec.js | 2 + apps/files_sharing/tests/js/shareSpec.js | 2 + .../tests/js/sharedfilelistSpec.js | 1 + apps/files_trashbin/tests/js/filelistSpec.js | 1 + 11 files changed, 168 insertions(+), 6 deletions(-) diff --git a/apps/files/js/app.js b/apps/files/js/app.js index 71802948a5..45b6b6a0e1 100644 --- a/apps/files/js/app.js +++ b/apps/files/js/app.js @@ -32,6 +32,10 @@ // regular actions fileActions.merge(OCA.Files.fileActions); + // in case apps would decide to register file actions later, + // replace the global object with this one + OCA.Files.fileActions = fileActions; + this.files = OCA.Files.Files; // TODO: ideally these should be in a separate class / app (the embedded "all files" app) diff --git a/apps/files/js/fileactions.js b/apps/files/js/fileactions.js index 47a6ab2f04..fc7c9ccace 100644 --- a/apps/files/js/fileactions.js +++ b/apps/files/js/fileactions.js @@ -22,9 +22,51 @@ defaults: {}, icons: {}, currentFile: null, + + /** + * List of handlers to be notified whenever a register() or + * setDefault() was called. + */ + _updateListeners: [], + initialize: function() { this.clear(); }, + + /** + * Adds an update listener to be notified whenever register() + * or setDefault() has been called. + * + * @param Function callback + */ + addUpdateListener: function(callback) { + if (!_.isFunction(callback)) { + throw 'Argument passed to FileActions.addUpdateListener must be a function'; + } + this._updateListeners.push(callback); + }, + + /** + * Removes an update listener. + * + * @param Function callback + */ + removeUpdateListener: function(callback) { + if (!_.isFunction(callback)) { + throw 'Argument passed to FileActions.removeUpdateListener must be a function'; + } + this._updateListeners = _.without(this._updateListeners, callback); + }, + + /** + * Notifies the registered update listeners + */ + _notifyUpdateListeners: function() { + for (var i = 0; i < this._updateListeners.length; i++) { + this._updateListeners[i](this); + } + }, + /** * Merges the actions from the given fileActions into * this instance. @@ -59,15 +101,18 @@ this.actions[mime][name]['permissions'] = permissions; this.actions[mime][name]['displayName'] = displayName; this.icons[name] = icon; + this._notifyUpdateListeners(); }, clear: function() { this.actions = {}; this.defaults = {}; this.icons = {}; this.currentFile = null; + this._updateListeners = []; }, setDefault: function (mime, name) { this.defaults[mime] = name; + this._notifyUpdateListeners(); }, get: function (mime, type, permissions) { var actions = this.getActions(mime, type, permissions); @@ -133,8 +178,7 @@ display: function (parent, triggerEvent, fileList) { if (!fileList) { console.warn('FileActions.display() MUST be called with a OCA.Files.FileList instance'); - // using default list instead, which could be wrong - fileList = OCA.Files.App.fileList; + return; } this.currentFile = parent; var self = this; @@ -309,9 +353,10 @@ window.FileActions, mime, name, permissions, icon, action, displayName ); }; - window.FileActions.setDefault = function (mime, name) { - console.warn('FileActions.setDefault() is deprecated, please use OCA.Files.fileActions.setDefault() instead', mime, name); - OCA.Files.FileActions.prototype.setDefault.call(window.FileActions, mime, name); + window.FileActions.display = function (parent, triggerEvent, fileList) { + fileList = fileList || OCA.Files.App.fileList; + console.warn('FileActions.display() is deprecated, please use OCA.Files.fileActions.register() which automatically redisplays actions', mime, name); + OCA.Files.FileActions.prototype.display.call(window.FileActions, parent, triggerEvent, fileList); }; })(); diff --git a/apps/files/js/filelist.js b/apps/files/js/filelist.js index 55afedb206..82aa29670d 100644 --- a/apps/files/js/filelist.js +++ b/apps/files/js/filelist.js @@ -168,12 +168,22 @@ this.$container.on('scroll', _.bind(this._onScroll, this)); }, + /** + * Destroy / uninitialize this instance. + */ + destroy: function() { + // TODO: also unregister other event handlers + this.fileActions.removeUpdateListener(this._onFileActionsUpdated); + }, + _initFileActions: function(fileActions) { this.fileActions = fileActions; if (!this.fileActions) { this.fileActions = new OCA.Files.FileActions(); this.fileActions.registerDefaultActions(); } + this._onFileActionsUpdated = _.debounce(_.bind(this._onFileActionsUpdated, this), 100); + this.fileActions.addUpdateListener(this._onFileActionsUpdated); }, /** @@ -487,6 +497,18 @@ } }, + /** + * Event handler for when file actions were updated. + * This will refresh the file actions on the list. + */ + _onFileActionsUpdated: function() { + console.log('onFileActionsUpdated'); + var self = this; + this.$fileList.find('tr td.filename').each(function() { + self.fileActions.display($(this), true, self); + }); + }, + /** * Sets the files to be displayed in the list. * This operation will re-render the list and update the summary. diff --git a/apps/files/tests/js/fileactionsSpec.js b/apps/files/tests/js/fileactionsSpec.js index 355761afa0..f464800730 100644 --- a/apps/files/tests/js/fileactionsSpec.js +++ b/apps/files/tests/js/fileactionsSpec.js @@ -21,7 +21,7 @@ describe('OCA.Files.FileActions tests', function() { var $filesTable, fileList; - var FileActions; + var FileActions; beforeEach(function() { // init horrible parameters @@ -36,6 +36,7 @@ describe('OCA.Files.FileActions tests', function() { }); afterEach(function() { FileActions = null; + fileList.destroy(); fileList = undefined; $('#dir, #permissions, #filestable').remove(); }); @@ -192,4 +193,54 @@ describe('OCA.Files.FileActions tests', function() { context = actionStub.getCall(0).args[1]; expect(context.dir).toEqual('/somepath'); }); + describe('events', function() { + var clock; + beforeEach(function() { + clock = sinon.useFakeTimers(); + }); + afterEach(function() { + clock.restore(); + }); + it('notifies update event handlers once after multiple changes', function() { + var actionStub = sinon.stub(); + var handler = sinon.stub(); + FileActions.addUpdateListener(handler); + FileActions.register( + 'all', + 'Test', + OC.PERMISSION_READ, + OC.imagePath('core', 'actions/test'), + actionStub + ); + FileActions.register( + 'all', + 'Test2', + OC.PERMISSION_READ, + OC.imagePath('core', 'actions/test'), + actionStub + ); + expect(handler.calledTwice).toEqual(true); + }); + it('does not notifies update event handlers after unregistering', function() { + var actionStub = sinon.stub(); + var handler = sinon.stub(); + FileActions.addUpdateListener(handler); + FileActions.removeUpdateListener(handler); + FileActions.register( + 'all', + 'Test', + OC.PERMISSION_READ, + OC.imagePath('core', 'actions/test'), + actionStub + ); + FileActions.register( + 'all', + 'Test2', + OC.PERMISSION_READ, + OC.imagePath('core', 'actions/test'), + actionStub + ); + expect(handler.notCalled).toEqual(true); + }); + }); }); diff --git a/apps/files/tests/js/filelistSpec.js b/apps/files/tests/js/filelistSpec.js index dea7c48e05..7100a2667c 100644 --- a/apps/files/tests/js/filelistSpec.js +++ b/apps/files/tests/js/filelistSpec.js @@ -1623,6 +1623,38 @@ describe('OCA.Files.FileList tests', function() { expect(context.fileActions).toBeDefined(); expect(context.dir).toEqual('/subdir'); }); + it('redisplays actions when new actions have been registered', function() { + var actionStub = sinon.stub(); + var clock = sinon.useFakeTimers(); + var debounceStub = sinon.stub(_, 'debounce', function(callback) { + return function() { + // defer instead of debounce, to make it work with clock + _.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.fileActions.register( + 'text/plain', + 'Test', + OC.PERMISSION_ALL, + function() { + // Specify icon for hitory button + return OC.imagePath('core','actions/history'); + }, + actionStub + ); + var $tr = fileList.findFileEl('One.txt'); + expect($tr.find('.action-test').length).toEqual(0); + // update is delayed + clock.tick(100); + expect($tr.find('.action-test').length).toEqual(1); + clock.restore(); + debounceStub.restore(); + }); }); describe('Sorting files', function() { it('Sorts by name by default', function() { diff --git a/apps/files_external/tests/appSpec.js b/apps/files_external/tests/appSpec.js index b00442384d..43902d1c1d 100644 --- a/apps/files_external/tests/appSpec.js +++ b/apps/files_external/tests/appSpec.js @@ -42,6 +42,7 @@ describe('OCA.External.App tests', function() { }); afterEach(function() { App.fileList = null; + fileList.destroy(); fileList = null; }); diff --git a/apps/files_external/tests/js/mountsfilelistSpec.js b/apps/files_external/tests/js/mountsfilelistSpec.js index 96a6b622a4..b599df77aa 100644 --- a/apps/files_external/tests/js/mountsfilelistSpec.js +++ b/apps/files_external/tests/js/mountsfilelistSpec.js @@ -54,6 +54,7 @@ describe('OCA.External.FileList tests', function() { afterEach(function() { OCA.Files.FileList.prototype = oldFileListPrototype; testFiles = undefined; + fileList.destroy(); fileList = undefined; fileActions = undefined; diff --git a/apps/files_sharing/tests/js/appSpec.js b/apps/files_sharing/tests/js/appSpec.js index 9c46b7caf1..d0480ad1aa 100644 --- a/apps/files_sharing/tests/js/appSpec.js +++ b/apps/files_sharing/tests/js/appSpec.js @@ -47,6 +47,8 @@ describe('OCA.Sharing.App tests', function() { afterEach(function() { App._inFileList = null; App._outFileList = null; + fileListIn.destroy(); + fileListOut.destroy(); fileListIn = null; fileListOut = null; }); diff --git a/apps/files_sharing/tests/js/shareSpec.js b/apps/files_sharing/tests/js/shareSpec.js index 455addaccc..3d4fc75482 100644 --- a/apps/files_sharing/tests/js/shareSpec.js +++ b/apps/files_sharing/tests/js/shareSpec.js @@ -70,6 +70,8 @@ describe('OCA.Sharing.Util tests', function() { OCA.Files.FileList.prototype = oldFileListPrototype; delete OCA.Sharing.sharesLoaded; delete OC.Share.droppedDown; + fileList.destroy(); + fileList = null; OC.Share.statuses = {}; OC.Share.currentShares = {}; }); diff --git a/apps/files_sharing/tests/js/sharedfilelistSpec.js b/apps/files_sharing/tests/js/sharedfilelistSpec.js index 0f6d0a0ba4..4e13088550 100644 --- a/apps/files_sharing/tests/js/sharedfilelistSpec.js +++ b/apps/files_sharing/tests/js/sharedfilelistSpec.js @@ -55,6 +55,7 @@ describe('OCA.Sharing.FileList tests', function() { afterEach(function() { OCA.Files.FileList.prototype = oldFileListPrototype; testFiles = undefined; + fileList.destroy(); fileList = undefined; fileActions = undefined; diff --git a/apps/files_trashbin/tests/js/filelistSpec.js b/apps/files_trashbin/tests/js/filelistSpec.js index 11eeff68df..fd479234b3 100644 --- a/apps/files_trashbin/tests/js/filelistSpec.js +++ b/apps/files_trashbin/tests/js/filelistSpec.js @@ -96,6 +96,7 @@ describe('OCA.Trashbin.FileList tests', function() { }); afterEach(function() { testFiles = undefined; + fileList.destroy(); fileList = undefined; $('#dir').remove();