781762 - Add the ability to delete all downloaded files at one time

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Download Manager, enhancement)

Description

[Cristian Nicolae (:xti)]

At this point, there is no way to delete all downloaded files at one time. If there are 15+ downloads, it could be really annoying to delete them one by one.

A possible solution could be to add a dedicated option in Clear Private Data list.

Another solution, which is should be very easier to be implemented, is just to clear the Download list by long tapping on any entry form the list and selecting the Clear All option.

Comment 1

[Mark Capella [:capella]]

Attachment: Patch (v1)

Asking margaret for feedback on the first revision of this possible solution.

Comment 2

[:Margaret Leibovic]

Comment on attachment 658046 [details] [diff] [review]
Patch (v1)

Review of attachment 658046 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good! I have some comments, but this is definitely on the right track.

::: mobile/android/chrome/content/aboutDownloads.js
@@ +110,5 @@
>        function (aTarget) {
>          Downloads.cancelDownload(aTarget);
>        }
>      );
> +    

Nit: whitespace.

@@ +114,5 @@
> +    
> +    // Clear List is shown always
> +    Downloads.clearListMenuItem = contextmenus.add(gStrings.GetStringFromName("downloadAction.clearList"),
> +                                                   contextmenus.SelectorContext("li"),
> +      function () {

I realize you're being consistent with the rest of the file, which is generally the best way to go, but you should give this function a name, something like clearListCallback().

We generally don't like anonymous functions, since they can be harder to debug, so if you want write a separate patch to name the other anonymous functions in here, that would be nice :) (separate patch to avoid scope creep here).

@@ +116,5 @@
> +    Downloads.clearListMenuItem = contextmenus.add(gStrings.GetStringFromName("downloadAction.clearList"),
> +                                                   contextmenus.SelectorContext("li"),
> +      function () {
> +        Downloads.clearListDownload();
> +      }

Also, I'm not really sure why we use |Downloads| all over the place instead of |this|, but it's fine to keep it as is, to stay consistent with the rest of the file.

@@ +418,5 @@
>        if (f) f.remove(false);
>      } catch(ex) { }
>    },
>  
> +  clearListDownload: function () {

This function name doesn't really make sense, we can probably just make it clearList(). Also, it would be good to name these member functions as well (more material for that follow-up patch).

@@ +427,5 @@
> +    if (elements.length == 0) {
> +      let message = gStrings.GetStringFromName("downloadMessageEmpty.clearList");
> +      let flags = Services.prompt.BUTTON_POS_0 * Services.prompt.BUTTON_TITLE_OK;
> +      let choice = Services.prompt.confirmEx(null, title, message, flags,
> +                                             null, null, null, null, {});

Instead of doing this check for 0 elements then showing an error message, we should change the SelectorContext for the context menu item to avoid showing this option when there aren't downloads to clear. It can be a chance for you to practice your CSS selector fu :)

@@ +435,5 @@
> +                  Services.prompt.BUTTON_POS_1 * Services.prompt.BUTTON_TITLE_CANCEL;
> +      let choice = Services.prompt.confirmEx(null, title, message, flags,
> +                                             null, null, null, null, {});
> +      if (choice == 0) {
> +        for (var i = 0; i < elements.length; i++) {

s/var/let

::: mobile/android/locales/en-US/chrome/aboutDownloads.properties
@@ +6,5 @@
>  downloadAction.remove=Delete
> +downloadAction.clearList=Clear List
> +downloadTitle.clearList=Clear List
> +downloadMessage.clearList=Remove (%1$S) completed, canceled, or failed downloads from the list?
> +downloadMessageEmpty.clearList=No completed, canceled, or failed downloads to remove

We should get some UX input on these strings. The last two feel a little verbose. Also, if you're using the first two in the same context (that is, you couldn't imagine a localizer wanting to treat them differently), then you can just make one string instead of two.

Comment 3

[:Margaret Leibovic]

Adding Ian, in case he has ideas about these strings (see previous comment).

If we avoid showing the context menu item when there aren't downloads to clear, we won't need the last string anymore, so that would make things easier :)

Comment 4

[Mark Capella [:capella]]

Thanks! I tried that context selector logic to avoid the (nothing to download) message ... I'll take another look because I really don't like that message either, and not having a context entry "Clear List" in a place where it won't do anything anyhow :)

btw: I love that I'm not the only one who knows the meaning of the phrase "scope creep" ... I've worked with other areas where changing the reqs and changing the reqs, and broadening the scope all on the fly, has led me to put things down, rather than get dragged into a never ending making it-up-as-we-go-along situation ... I'd rather fix the bug, then file more if we need to.

The verbose messages were cloned from the base browser tooltip when hovering over the download manager "Clear List" button. Will wait for Ian to weigh in there.

I'll tweak it, and get back to you, and thanks again -- mark

Comment 5

[Wesley Johnston (:wesj)]

A big distinction between us and desktop, this will actually delete the files from the sdcard. I think we'll want different language than "Clear List".

Comment 6

[Mark Capella [:capella]]

Attachment: Patch (v2)

This is a little bit cleaner, addressing most, if not all, of your concerns. Let me know if it's what you had in mind.

We haven't heard from Ian yet, but I changed "Clear List" to be "Delete All", considering the wesj idea and the original bug description.

Comment 7

[:Margaret Leibovic]

Comment on attachment 660407 [details] [diff] [review]
Patch (v2)

Review of attachment 660407 [details] [diff] [review]:
-----------------------------------------------------------------

I'm giving this an r- to sort out the string. Besides a few other comments, it's looking good!

::: mobile/android/chrome/content/aboutDownloads.js
@@ +118,5 @@
> +                                                                                "li[state='" + this._dlmgr.DOWNLOAD_CANCELED + "']," +
> +                                                                                "li[state='" + this._dlmgr.DOWNLOAD_FAILED + "']"),
> +      function deleteAllCallback() {
> +        Downloads.deleteAll();
> +      }

Instead of making an new function like this, you can just use |this.deleteAll.bind(this)|. You need to use bind [1] to make sure the function gets called in the correct context, since you use |this| in deleteAll.

[1] https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/Function/bind

@@ +420,5 @@
>        if (f) f.remove(false);
>      } catch(ex) { }
>    },
>  
> +  deleteAll: function () {

Give this function a name. Something like |function dl_deleteAll()| would be good.

::: mobile/android/locales/en-US/chrome/aboutDownloads.properties
@@ +4,5 @@
>  
>  downloadAction.open=Open
>  downloadAction.remove=Delete
> +downloadAction.deleteAll=Delete All
> +downloadMessage.deleteAll=Delete (%1$S) completed, canceled, or failed downloads from the list?

You should use plural forms to provide different strings for the singular/plural cases. For an example, see: http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/downloads/downloads.properties#70
Actually, is there a way to hide this menuitem altogether when there's only one download? That might take some real SelectorContext hackery, so it's okay if you want to punt on that (but if you do, you should make sure the string handles the singular case).

I also don't think that we need parentheses around the number, and I'm not sure that we need "completed, canceled, or failed". And as wesj mentioned, this deletes downloads from the system, not just from the list, so maybe we can keep it really simple and say something like "Delete 4 downloads?".

Comment 8

[Mark Capella [:capella]]

Attachment: Patch (v3)

Thanks for the info re: .bind() ! I've bumped into that before but the code sample helped clarify it for me ...

This version works on my device ...

Comment 9

[:Margaret Leibovic]

Comment on attachment 663737 [details] [diff] [review]
Patch (v3)

Review of attachment 663737 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/chrome/content/aboutDownloads.js
@@ +424,5 @@
> +    let elements = this._list.querySelectorAll("li[state='" + this._dlmgr.DOWNLOAD_FINISHED + "']," +
> +                                               "li[state='" + this._dlmgr.DOWNLOAD_CANCELED + "']," +
> +                                               "li[state='" + this._dlmgr.DOWNLOAD_FAILED + "']");
> +    let message = gStrings.formatStringFromName("downloadMessage.deleteAll",
> +                                                [elements.length, (elements.length > 1 ? "s" : "")],

This isn't an l10n-friendly way to handle plural forms. Not all languages pluarlize with an s, and there can be more than (or fewer than) 2 plural forms. See the example I linked to in comment 7, and see https://developer.mozilla.org/en-US/docs/Localization_and_Plurals.

::: mobile/android/locales/en-US/chrome/aboutDownloads.properties
@@ +4,5 @@
>  
>  downloadAction.open=Open
>  downloadAction.remove=Delete
> +downloadAction.deleteAll=Delete All
> +downloadMessage.deleteAll=Delete %1$S download%2$S ?

Nit: Don't put a space between the last word and the question mark.

Comment 10

[Mark Capella [:capella]]

Wow! I hadn't expected that much work :P I initially placed the number into the string to help me with debugging and I'm not really attached to it ...

What if I changed the confirmation message to be >really< simple:

+downloadMessage.deleteAll=Are you sure?

Comment 11

[:Margaret Leibovic]

I don't think it's that much work. Basically you just need to make your string have two forms (since English only has two forms):

downloadMessage.deleteAll=Delete one download?;Delete #1 downloads?

Then in JS, you need to do:

let downloadCount = elements.length;
let messageForm = gStrings.GetStringFromName("downloadMessage.deleteAll");
let message = PluralForm.get(downloadCount, messageForm).replace("#1", downloadCount);

We can also get Ian's input about what string would be best here. Ian, we're talking about a string for a confirmation dialog that appears when you select "Delete All" in the downloads manager.

Comment 12

[Ian Barlow (:ibarlow)]

This looks good. I would just tweak it to read either

Delete this download?
Delete #1 downloads?

Comment 13

[Mark Capella [:capella]]

Attachment: Patch (v4)

( Laughs ... no it's not hard ... my A.D.D. just kicked in and I was distracted by Bug 741654 )

Here's the latest version ... with a more localizable confirmation string ...

Comment 14

[:Margaret Leibovic]

Comment on attachment 665195 [details] [diff] [review]
Patch (v4)

Nice, looks good.

Comment 15

[Mark Capella [:capella]]

Always do my TRY
https://tbpl.mozilla.org/?tree=Try&rev=283e7022282c

Comment 16

[Mark Capella [:capella]]

And on to inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/85a1897826b5

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/85a1897826b5

Comment 18

[Cristian Nicolae (:xti)]

Delete All option will delete all existing downloads from about:downloads. Also they are correctly removed from sdcard/downloads too.
Closing bug as verified fixed on:

Firefox 18.0a1 (2012-09-30)
Device: Galaxy Note
OS: Android 4.0.4

Comment 19

[Aaron Train [:aaronmt]]

I couldn't find the Delete All button. I don't think it makes any sense to have to long-tap to invoke the context-menu for a given item to access a function that applies to greater context.