Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add loading spinner to download icon #17175

Merged
merged 10 commits into from
Jul 30, 2015
11 changes: 11 additions & 0 deletions apps/files/ajax/download.php
Original file line number Diff line number Diff line change
Expand Up @@ -39,4 +39,15 @@
$files_list = array($files);
}

/**
* this sets a cookie to be able to recognize the start of the download
* the content must not be longer than 32 characters and must only contain
* alphanumeric characters
*/
if(isset($_GET['downloadStartSecret'])
&& !isset($_GET['downloadStartSecret'][32])
&& preg_match('!^[a-zA-Z0-9]+$!', $_GET['downloadStartSecret']) === 1) {
setcookie('ocDownloadStarted', $_GET['downloadStartSecret'], time() + 20, '/');
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means we won't be able to use WebDAV download in the future, unless the WebDAV endpoint also sets cookies, super ugly. Or we keep a special ajax download endpoint only for this logic.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or hmmmm a Sabre plugin that detects that the WebDAV upload was done from a web UI and sets the cookie, might be cleaner.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What kind of magic is this 🙊

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hehe. This is the only way to detect if a download has started and to remove the download spinner animation. Because the download is handled in a separate request the browsers sandbox doesn't allow to check if the request already finished or not. With this we set the cookie and the browser then sets the cookie for the whole domain. If the cookie is set, the JS part knows, that the download has started and can remove the loading spinner.

@PVince81 Yes ... we need to find a way to do this in WebDAV too :/ But user feedback for such stuff is also quite essential I think ... otherwise users click really often on the download button just because the backend needs some time to fetch a file 🙈


OC_Files::get($dir, $files_list, $_SERVER['REQUEST_METHOD'] == 'HEAD');
25 changes: 23 additions & 2 deletions apps/files/css/files.css
Original file line number Diff line number Diff line change
Expand Up @@ -583,14 +583,23 @@ a.action>img {
#fileList tr:focus a.action,
#fileList a.action.permanent,
#fileList tr:hover a.action.no-permission:hover,
#fileList tr:focus a.action.no-permission:focus
/*#fileList .name:focus .action*/ {
#fileList tr:focus a.action.no-permission:focus,
/*#fileList .name:focus .action,*/
/* also enforce the low opacity for disabled links that are hovered/focused */
.ie8 #fileList a.action.disabled:hover img,
#fileList tr:hover a.action.disabled:hover,
#fileList tr:focus a.action.disabled:focus,
#fileList .name:focus a.action.disabled:focus,
#fileList a.action.disabled img {
-ms-filter: "progid:DXImageTransform.Microsoft.Alpha(Opacity=50)";
filter: alpha(opacity=50);
opacity: .5;
display:inline;
}
.ie8 #fileList a.action:hover img,
#fileList tr a.action.disabled.action-download,
#fileList tr:hover a.action.disabled.action-download:hover,
#fileList tr:focus a.action.disabled.action-download:focus,
#fileList tr:hover a.action:hover,
#fileList tr:focus a.action:focus,
#fileList .name:focus a.action:focus {
Expand All @@ -599,6 +608,18 @@ a.action>img {
opacity: 1;
display:inline;
}
#fileList tr a.action.disabled {
background: none;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is because usually disabled links have a grey background.

}

#selectedActionsList a.download.disabled,
#fileList tr a.action.action-download.disabled {
color: #000000;
}

#fileList tr:hover a.action.disabled:hover * {
cursor: default;
}

.summary {
-ms-filter: "progid:DXImageTransform.Microsoft.Alpha(Opacity=30)";
Expand Down
35 changes: 34 additions & 1 deletion apps/files/js/fileactions.js
Original file line number Diff line number Diff line change
Expand Up @@ -478,15 +478,48 @@
}, function (filename, context) {
var dir = context.dir || context.fileList.getCurrentDirectory();
var url = context.fileList.getDownloadUrl(filename, dir);

var downloadFileaction = $(context.$file).find('.fileactions .action-download');

// don't allow a second click on the download action
if(downloadFileaction.hasClass('disabled')) {
return;
}

if (url) {
OC.redirect(url);
var disableLoadingState = function(){
OCA.Files.FileActions.updateFileActionSpinner(downloadFileaction, false);
};

OCA.Files.FileActions.updateFileActionSpinner(downloadFileaction, true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note, here you can use "self.updateFileActionSpinner(...)". But you need to add var self = this; at the top of the original method. See an example here: https://github.com/owncloud/core/blob/master/apps/files/js/filelist.js#L619

Feel free to leave it now. Just a hint for next time 😄

OCA.Files.Files.handleDownload(url, disableLoadingState);
}
}, t('files', 'Download'));
}
};

OCA.Files.FileActions = FileActions;

/**
* Replaces the download icon with a loading spinner and vice versa
* - also adds the class disabled to the passed in element
*
* @param downloadButtonElement download fileaction
* @param {boolean} showIt whether to show the spinner(true) or to hide it(false)
*/
OCA.Files.FileActions.updateFileActionSpinner = function(downloadButtonElement, showIt) {
var icon = downloadButtonElement.find('img'),
sourceImage = icon.attr('src');

if(showIt) {
downloadButtonElement.addClass('disabled');
icon.attr('src', sourceImage.replace('actions/download.svg', 'loading-small.gif'));
} else {
downloadButtonElement.removeClass('disabled');
icon.attr('src', sourceImage.replace('loading-small.gif', 'actions/download.svg'));
}
};

/**
* File action attributes.
*
Expand Down
16 changes: 15 additions & 1 deletion apps/files/js/filelist.js
Original file line number Diff line number Diff line change
Expand Up @@ -417,7 +417,21 @@
else {
files = _.pluck(this.getSelectedFiles(), 'name');
}
OC.redirect(this.getDownloadUrl(files, dir));

var downloadFileaction = $('#selectedActionsList').find('.download');

// don't allow a second click on the download action
if(downloadFileaction.hasClass('disabled')) {
event.preventDefault();
return;
}

var disableLoadingState = function(){
OCA.Files.FileActions.updateFileActionSpinner(downloadFileaction, false);
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate code with above, but I guess this is only your POC and you intended to refactor later ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is a good place to put this? in the filelist class?


OCA.Files.FileActions.updateFileActionSpinner(downloadFileaction, true);
OCA.Files.Files.handleDownload(this.getDownloadUrl(files, dir), disableLoadingState);
return false;
},

Expand Down
27 changes: 26 additions & 1 deletion apps/files/js/files.js
Original file line number Diff line number Diff line change
Expand Up @@ -276,8 +276,33 @@
FileList.scrollTo(getURLParameter('scrollto'));
}
*/
},

/**
* Handles the download and calls the callback function once the download has started
* - browser sends download request and adds parameter with a token
* - server notices this token and adds a set cookie to the download response
* - browser now adds this cookie for the domain
* - JS periodically checks for this cookie and then knows when the download has started to call the callback
*
* @param {string} url download URL
* @param {function} callback function to call once the download has started
*/
handleDownload: function(url, callback) {
var randomToken = Math.random().toString(36).substring(2),
checkForDownloadCookie = function() {
if (!OC.Util.isCookieSetToValue('ocDownloadStarted', randomToken)){
return false;
} else {
callback();
return true;
}
};

OC.redirect(url + '&downloadStartSecret=' + randomToken);
OC.Util.waitFor(checkForDownloadCookie, 500);
}
}
};

Files._updateStorageStatisticsDebounced = _.debounce(Files._updateStorageStatistics, 250);
OCA.Files.Files = Files;
Expand Down
4 changes: 2 additions & 2 deletions apps/files/tests/js/fileactionsSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ describe('OCA.Files.FileActions tests', function() {
$tr.find('.action-download').click();

expect(redirectStub.calledOnce).toEqual(true);
expect(redirectStub.getCall(0).args[0]).toEqual(
expect(redirectStub.getCall(0).args[0]).toContain(
OC.webroot +
'/index.php/apps/files/ajax/download.php' +
'?dir=%2Fsubdir&files=testName.txt');
Expand All @@ -129,7 +129,7 @@ describe('OCA.Files.FileActions tests', function() {
$tr.find('.action-download').click();

expect(redirectStub.calledOnce).toEqual(true);
expect(redirectStub.getCall(0).args[0]).toEqual(
expect(redirectStub.getCall(0).args[0]).toContain(
OC.webroot + '/index.php/apps/files/ajax/download.php' +
'?dir=%2Fanotherpath%2Fthere&files=testName.txt'
);
Expand Down
10 changes: 5 additions & 5 deletions apps/files/tests/js/filelistSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,8 @@ describe('OCA.Files.FileList tests', function() {
'<th id="headerName" class="hidden column-name">' +
'<input type="checkbox" id="select_all_files" class="select-all">' +
'<a class="name columntitle" data-sort="name"><span>Name</span><span class="sort-indicator"></span></a>' +
'<span class="selectedActions hidden">' +
'<a href class="download">Download</a>' +
'<span id="selectedActionsList" class="selectedActions hidden">' +
'<a href class="download"><img src="actions/download.svg">Download</a>' +
'<a href class="delete-selected">Delete</a></span>' +
'</th>' +
'<th class="hidden column-size"><a class="columntitle" data-sort="size"><span class="sort-indicator"></span></a></th>' +
Expand Down Expand Up @@ -1774,7 +1774,7 @@ describe('OCA.Files.FileList tests', function() {
var redirectStub = sinon.stub(OC, 'redirect');
$('.selectedActions .download').click();
expect(redirectStub.calledOnce).toEqual(true);
expect(redirectStub.getCall(0).args[0]).toEqual(OC.webroot + '/index.php/apps/files/ajax/download.php?dir=%2Fsubdir&files=%5B%22One.txt%22%2C%22Three.pdf%22%2C%22somedir%22%5D');
expect(redirectStub.getCall(0).args[0]).toContain(OC.webroot + '/index.php/apps/files/ajax/download.php?dir=%2Fsubdir&files=%5B%22One.txt%22%2C%22Three.pdf%22%2C%22somedir%22%5D');
redirectStub.restore();
});
it('Downloads root folder when all selected in root folder', function() {
Expand All @@ -1783,15 +1783,15 @@ describe('OCA.Files.FileList tests', function() {
var redirectStub = sinon.stub(OC, 'redirect');
$('.selectedActions .download').click();
expect(redirectStub.calledOnce).toEqual(true);
expect(redirectStub.getCall(0).args[0]).toEqual(OC.webroot + '/index.php/apps/files/ajax/download.php?dir=%2F&files=');
expect(redirectStub.getCall(0).args[0]).toContain(OC.webroot + '/index.php/apps/files/ajax/download.php?dir=%2F&files=');
redirectStub.restore();
});
it('Downloads parent folder when all selected in subfolder', function() {
$('.select-all').click();
var redirectStub = sinon.stub(OC, 'redirect');
$('.selectedActions .download').click();
expect(redirectStub.calledOnce).toEqual(true);
expect(redirectStub.getCall(0).args[0]).toEqual(OC.webroot + '/index.php/apps/files/ajax/download.php?dir=%2F&files=subdir');
expect(redirectStub.getCall(0).args[0]).toContain(OC.webroot + '/index.php/apps/files/ajax/download.php?dir=%2F&files=subdir');
redirectStub.restore();
});
});
Expand Down
32 changes: 31 additions & 1 deletion core/js/js.js
Original file line number Diff line number Diff line change
Expand Up @@ -1513,8 +1513,38 @@ OC.Util = {
}
}
return aa.length - bb.length;
},
/**
* Calls the callback in a given interval until it returns true
* @param {function} callback
* @param {integer} interval in milliseconds
*/
waitFor: function(callback, interval) {
var internalCallback = function() {
if(callback() !== true) {
setTimeout(internalCallback, interval);
}
};

internalCallback();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

},
/**
* Checks if a cookie with the given name is present and is set to the provided value.
* @param {string} name name of the cookie
* @param {string} value value of the cookie
* @return {boolean} true if the cookie with the given name has the given value
*/
isCookieSetToValue: function(name, value) {
var cookies = document.cookie.split(';');
for (var i=0; i < cookies.length; i++) {
var cookie = cookies[i].split('=');
if (cookie[0].trim() === name && cookie[1].trim() === value) {
return true;
}
}
return false;
}
}
};

/**
* Utility class for the history API,
Expand Down