Closed Bug 781762 Opened 12 years ago Closed 12 years ago

Add the ability to delete all downloaded files at one time

Categories

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

ARM
Android
enhancement
Not set
normal

Tracking

(firefox18 verified)

VERIFIED FIXED
Firefox 18
Tracking Status
firefox18 --- verified

People

(Reporter: xti, Assigned: capella)

Details

Attachments

(1 file, 3 obsolete files)

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.
Component: General → Download Manager
Attached patch Patch (v1) (obsolete) — Splinter Review
Asking margaret for feedback on the first revision of this possible solution.
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Attachment #658046 - Flags: feedback?(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.
Attachment #658046 - Flags: feedback?(margaret.leibovic) → feedback+
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 :)
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
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".
Attached patch Patch (v2) (obsolete) — Splinter Review
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.
Attachment #658046 - Attachment is obsolete: true
Attachment #660407 - Flags: review?(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?".
Attachment #660407 - Flags: review?(margaret.leibovic) → review-
Attached patch Patch (v3) (obsolete) — Splinter Review
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 ...
Attachment #660407 - Attachment is obsolete: true
Attachment #663737 - Flags: review?(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.
Attachment #663737 - Flags: review?(margaret.leibovic) → review-
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?
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.
Keywords: uiwanted
This looks good. I would just tweak it to read either

Delete this download?
Delete #1 downloads?
Attached patch Patch (v4)Splinter Review
( 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 ...
Attachment #663737 - Attachment is obsolete: true
Attachment #665195 - Flags: review?(margaret.leibovic)
Comment on attachment 665195 [details] [diff] [review]
Patch (v4)

Nice, looks good.
Attachment #665195 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/mozilla-central/rev/85a1897826b5
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
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
Status: RESOLVED → VERIFIED
Keywords: uiwanted
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.
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.