Add loading spinner to download icon #17175

MorrisJobke · 2015-06-25T14:28:38Z

vanishes after 2 seconds

Same like #17160 but for the file list.

I still don't like that it vanishes after the hardcoded 7 seconds. I searched a lot and tried many JS events to somehow detect, that the download is started but nothing worked.

cc @owncloud/designers @LukasReschke @craigpg @bobboule

MorrisJobke · 2015-06-25T14:29:43Z

I tested this locally, but it feels very hacky :(

@oparoz @rullzer @Xenopathic Any ideas how to solve this better?

PVince81 · 2015-06-25T14:30:15Z

apps/files/js/fileactions.js

+
+				var icon = $(context.$file).find('.fileactions .action-download img');
+				var sourceImage = icon.attr('src');
+				icon.attr('src', sourceImage.replace('actions/download.svg', 'loading-small.gif'));


Ideal would be to work with CSS classes and have them define the icon.
But I guess you were short on time ?

Right, the action icons, I remember... not flexible.

#17175 (comment)

I don't want to break backwards compatibility here.

PVince81 · 2015-06-25T14:31:53Z

Another idea, still a bit hacky, is to delete the img element and insert a new one with the image.

Or, the elegant but out of scope approach: improve FileActions to be able to show a spinner for ANY action (could be used for when loading the sharing dialog, delete, etc)

MorrisJobke · 2015-06-25T14:32:30Z

I tried events like beforeunload and unload but both are triggered at the redirect in JS. Then the browser window adds the spinner as favicon. Once the download starts the spinner is gone. Does anybody know how I can check for this "loading state" from JS? I tried window.location.href but it is always the old url (even if it is set during OC.redirect).

MorrisJobke · 2015-06-25T14:33:53Z

Or, the elegant but out of scope approach: improve FileActions to be able to show a spinner for ANY action (could be used for when loading the sharing dialog, delete, etc)

Ah ... didn't though about this. But it sounds nice :)

I also thought about adding a element that gets an icon class applied and not adding an <img> tag, because then you only need to switch the css class ;)

MorrisJobke · 2015-06-25T14:46:01Z

To test this add sleep(5) to apps/files/ajax/download.php ;)

oparoz · 2015-06-25T15:32:39Z

Interesting problem... The main problem seems to be that we don't have a 100% control over the sending mechanism. Otherwise, it would be easy to use a cookie.

karlitschek · 2015-06-25T21:40:32Z

i like it 👍

MorrisJobke · 2015-06-26T06:07:45Z

Interesting problem... The main problem seems to be that we don't have a 100% control over the sending mechanism. Otherwise, it would be easy to use a cookie.

There is a somewhat hackish way to check if a download request already came back (and therefore the download is shown in the browser):

send a random string within the download redirect
on the server check for availability of the parameter and set the random string to a predefined cookie name (short TTL - a few seconds up to a minute - needs to be bigger than the time between two checks within the JS code - see next steps)
once the response is read by the browser it will set the cookie
the JS can check periodically if this cookie is set to the previous send value and then knows, that the download started.

PVince81 · 2015-06-26T07:40:40Z

Argh... sounds almost like a project on its own.

MorrisJobke · 2015-06-26T07:41:11Z

Argh... sounds almost like a project on its own.

Yes. Definetely something for a separate PR.

MorrisJobke · 2015-06-26T08:27:14Z

apps/files/css/files.css

@@ -599,6 +605,13 @@ a.action>img {
 	opacity: 1;
 	display:inline;
 }
+#fileList tr a.action.disabled {
+	background: none;


This is because usually disabled links have a grey background.

MorrisJobke · 2015-06-26T08:28:23Z

Now the button has doesn't highlight on hover and can't be clicked a second time. (@craigpg requested this as an UX enhancement)

@owncloud/designers please have a look at this.

MorrisJobke · 2015-06-26T08:29:02Z

We maybe want to decrease the time to two seconds (download then should be started)

MorrisJobke · 2015-06-26T08:34:28Z

It now looks like this:

jancborchardt · 2015-06-26T13:53:01Z

@MorrisJobke the »disabled« look makes it a bit hard to see. I would keep the same opacity that it has when hovered / just clicked to make the feedback properly visible.

jancborchardt · 2015-06-26T13:53:06Z

Otherwise awesome! :)

MorrisJobke · 2015-06-29T06:53:22Z

@bboule I added the download spinner to the multiselect download button. Please retest.

LukasReschke · 2015-06-29T06:58:25Z

It's a little bit confusing that the spinner also spins when the download has already been completed.

MorrisJobke · 2015-06-29T07:30:42Z

It's a little bit confusing that the spinner also spins when the download has already been completed.

The proper fix for this is #17175 (comment) 🙈

MorrisJobke · 2015-06-29T07:32:42Z

@LukasReschke I reduced the timeout to 2 seconds.

MorrisJobke · 2015-06-29T09:24:49Z

@MorrisJobke the »disabled« look makes it a bit hard to see. I would keep the same opacity that it has when hovered / just clicked to make the feedback properly visible.

Done.

PVince81 · 2015-07-14T15:54:08Z

@MorrisJobke now it looks great ! Sorry that I still found a few things to comment on, but you're close to perfection now 😉

MorrisJobke · 2015-07-14T15:55:17Z

@PVince81 I highly appreciate your comments 😃

MorrisJobke · 2015-07-15T12:53:07Z

@PVince81 Should I move the functions to the proposed places?

PVince81 · 2015-07-15T15:59:03Z

@MorrisJobke yes please. This one #17175 (comment)

scrutinizer-notifier · 2015-07-22T08:35:36Z

A new inspection was created.

MorrisJobke · 2015-07-22T08:35:53Z

The function is now moved :)

@rullzer @Xenopathic @jancborchardt Happy testing & reviewing :)

ghost · 2015-07-22T14:44:01Z

🚀 Test PASSed.🚀

jancborchardt · 2015-07-22T17:48:46Z

File download doesn’t work for me (folders work fine). Get redirected to http://127.0.0.1/owncloud/index.php/apps/files/ajax/download.php?dir=%2F&files=856.pdf&downloadStartSecret=ik90c30y1sn
and this error shows up:

"message":"Call to a member function isReadable() on null
at \/home\/jan\/owncloud\/lib\/private\/files\/storage\/wrapper\/encryption.php#288"
"url":"\/owncloud\/index.php\/apps\/files\/ajax\/download.php?dir=%2F&files=856.pdf&downloadStartSecret=ik90c30y1sn"}

MorrisJobke · 2015-07-22T21:58:58Z

File download doesn’t work for me

Mmmh. Works here. Which browser?

rullzer · 2015-07-27T07:15:07Z

Working as expected here in firefox and chromium.
👍

MorrisJobke · 2015-07-29T18:23:40Z

@jancborchardt ping

@PVince81 Can you have a final look at the changes and maybe also test if this works for you?

PVince81 · 2015-07-30T08:16:47Z

apps/files/js/fileactions.js

+							OCA.Files.FileActions.updateFileActionSpinner(downloadFileaction, false);
+						};
+
+					OCA.Files.FileActions.updateFileActionSpinner(downloadFileaction, true);


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 😄

PVince81 · 2015-07-30T08:17:27Z

The code looks great now, thanks @MorrisJobke.

Something that hacky needs to be tested in IE8 too.

PVince81 · 2015-07-30T08:30:49Z

It looks like the spinner only appears if the browser is configured with an automatic save location. If you configure it to always show the "save as" dialog, no spinner is shown.
I assume the spinner was designed for the former case ?

PVince81 · 2015-07-30T08:35:29Z

Okay, Chromium 43 shows the spinner before showing the save dialog, which is nice.
However Firefox 39 and IE8 never show the spinner.

PVince81 · 2015-07-30T08:39:49Z

IE9 shows the spinner.

Fine by me to merge 👍

@MorrisJobke do you intend to add a spinner later to the "Download" button on the top right of the public link page ?

MorrisJobke · 2015-07-30T14:33:59Z

@MorrisJobke do you intend to add a spinner later to the "Download" button on the top right of the public link page ?

Yes. Separate PR

Add loading spinner to download icon

MorrisJobke added design 3 - To Review labels Jun 25, 2015

PVince81 reviewed Jun 25, 2015
View reviewed changes

MorrisJobke added 2 - Developing and removed 3 - To Review labels Jun 26, 2015

MorrisJobke reviewed Jun 26, 2015
View reviewed changes

MorrisJobke added 3 - To Review and removed 2 - Developing labels Jun 26, 2015

DeepDiver1975 added this to the 8.2-next milestone Jun 29, 2015

MorrisJobke mentioned this pull request Jun 29, 2015

[Jenkins] add download feedback #17227

Closed

refactor this to proper separation of concerns

43afa4c

MorrisJobke force-pushed the add-download-feedback branch from 44190ee to 43afa4c Compare July 15, 2015 09:14

MorrisJobke added 2 - Developing and removed 3 - To Review labels Jul 16, 2015

move updateFileActionSpinner to OCA.Files.FileActions

009d1f3

MorrisJobke added 3 - To Review and removed 2 - Developing labels Jul 22, 2015

PVince81 reviewed Jul 30, 2015
View reviewed changes

MorrisJobke added a commit that referenced this pull request Jul 30, 2015

Merge pull request #17175 from owncloud/add-download-feedback

5699fff

Add loading spinner to download icon

MorrisJobke merged commit 5699fff into master Jul 30, 2015

MorrisJobke deleted the add-download-feedback branch July 30, 2015 14:34

MorrisJobke removed their assignment Oct 5, 2015

PVince81 mentioned this pull request Oct 8, 2015

Set download cookie on public download #19643

Merged

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

Add loading spinner to download icon #17175

Add loading spinner to download icon #17175

Conversation

MorrisJobke commented Jun 25, 2015

MorrisJobke commented Jun 25, 2015

PVince81 Jun 25, 2015

Choose a reason for hiding this comment

PVince81 Jun 25, 2015

Choose a reason for hiding this comment

MorrisJobke Jun 25, 2015

Choose a reason for hiding this comment

PVince81 commented Jun 25, 2015

MorrisJobke commented Jun 25, 2015

MorrisJobke commented Jun 25, 2015

MorrisJobke commented Jun 25, 2015

oparoz commented Jun 25, 2015

karlitschek commented Jun 25, 2015

MorrisJobke commented Jun 26, 2015

PVince81 commented Jun 26, 2015

MorrisJobke commented Jun 26, 2015

MorrisJobke Jun 26, 2015

Choose a reason for hiding this comment

MorrisJobke commented Jun 26, 2015

MorrisJobke commented Jun 26, 2015

MorrisJobke commented Jun 26, 2015

jancborchardt commented Jun 26, 2015

jancborchardt commented Jun 26, 2015

MorrisJobke commented Jun 29, 2015

LukasReschke commented Jun 29, 2015

MorrisJobke commented Jun 29, 2015

MorrisJobke commented Jun 29, 2015

MorrisJobke commented Jun 29, 2015

PVince81 commented Jul 14, 2015

MorrisJobke commented Jul 14, 2015

MorrisJobke commented Jul 15, 2015

PVince81 commented Jul 15, 2015

scrutinizer-notifier commented Jul 22, 2015

MorrisJobke commented Jul 22, 2015

ghost commented Jul 22, 2015

jancborchardt commented Jul 22, 2015

MorrisJobke commented Jul 22, 2015

rullzer commented Jul 27, 2015

MorrisJobke commented Jul 29, 2015

PVince81 Jul 30, 2015

Choose a reason for hiding this comment

PVince81 commented Jul 30, 2015

PVince81 commented Jul 30, 2015

PVince81 commented Jul 30, 2015

PVince81 commented Jul 30, 2015

MorrisJobke commented Jul 30, 2015