Closed Bug 391863 Opened 14 years ago Closed 14 years ago

"clean up" functionality missing from download manager

Categories

(Toolkit :: Downloads API, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla1.9beta1

People

(Reporter: Peter6, Assigned: Mardak)

References

Details

Attachments

(1 file, 4 obsolete files)

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007081115 Minefield/3.0a8pre ID:2007081115

repro:
download a file
open DM and try to clear the content

result:
no clear button to clean up the window
If you mean the clean up button that existed previously, this was intentional.
Why ?  Users are not going to want to use 'Clear private data' to clear the DM, and using the Delete Key does not 'highlight' the next item in the list for possible Deletion, causing one to click and repeat, not very efficient. 

The 'Mock-ups' also show a Clear all button.

Iteration 1 mock-ups do, however iteration 2 ones do not
(In reply to comment #1)
> If you mean the clean up button that existed previously, this was intentional.
> 
I disagree.
It's plain wrong if you have to go to Options->Privacy to clear the list.
Blocks: 388517
OS: Windows XP → All
Hardware: PC → All
Summary: clear button missing (new DM) → Clean Up button missing from new DM
download list clear button is no more necessary if you can search through the list, order that using places organizer. 

>  Users are not going to want to use 'Clear private data' to clear the DM

Why? It's a privacy data so it should be cleared there, people will be able to find easily their current and older downloads, if they continuously clear the list, that feature is lost.
Ok so users are apparently going to have enough downloaded stuff in the dm to have the need search through but not enough stuff to have a button to clear everything at one time instead of one by one? That is just plain retarded.
A list that's 100-elements long used to take minutes to display, during which firefox was frozen.

Unless performance changed significantly, clearing the DM list often is necessary.
about Comment #7: the dm uses sqlite now, so i think that performance changed significantly and should be no more necessary clear it often as it was before
The 'Clean Up' button has been present in the firefox download manager window since firefox 1.0, and I think people have gotten very used to using it in their work-flow when download files. Certainly I have; downloading, say, a load of images from a webpage - once i can see they have successfully downloaded I 'Clear Up', go to the next page, and continue. This let's me easily keep an eye on only the relevant things that I am downloading from that page/at that particular time. This behaviour is now deeply ingrained in the way I work when downloading things in firefox.

Also, personally, I don't like having a big list of things I've downloaded. Certainly some people might welcome this, but I dislike having a long list of stuff that I can't read in one go (ie: that scrolls off the bottom of the window). So I always used 'Clear Up' to keep the list tidy and managable, so I doubt I will ever use the ability to search my download list.

But the removal of this button is certainly strange and feels at this point in time like a step backwards. (It's a shame we have no stats on how often this button is pressed by how many people.) But I will give it at least a week to try and get used to the button's absence to see if I really feel strongly about it. And I guess if it does remain removed, then some extension author will re-add it :)

Of course we have to remember that firefox is aimed at people who don't even know what a web browser is, so for them it might be useful to have a giant download history. But even if they do, and they want to clean it up, it is a lot of steps to do this (Tools, Clear Private Data, Untick all boxes except download history, click clear now, etc) where as they might just want a button in the relevant DM UI to clean the list up.
(In reply to comment #8)
> about Comment #7: the dm uses sqlite now, so i think that performance changed
> significantly and should be no more necessary clear it often as it was before
 
I just faked a 100-file download window (with sqlite editing tools) and indeed it's much better, it needs only 1 second to open.

However, eventually, it *will* become too big to be efficient. Perhaps using History setting to auto-delete old entries is a good idea, if Clean Up is doomed?
Another option if the "Clean Up" button is gone for good is to have the download manager clean up files that no longer exist from the list. I find it utterly useless to have 10 listings for installers that I downloaded, installed, and then deleted from my disk once I was done. If there isn't a function like this or the "Clean Up" button then I see a major disadvantage to keeping a history of downloads.
Say you have uninstalled the software, but decided that you wanted it again.  You can search for the download, click the information button, and the first button with take you to the referring page where you can download it again (Bug 391870).
Can someone say 1 good reason why didn't we keep "Clean Up" button? For those that say "it's not for any use anymore because of performance fixes", well last time I checked, button said "Clean Up", not "Improve Performance of Download Manager"...

I used it a lot earlier, especially when downloading lots of images because I use Download Manager to see files I'm currently downloading, not those that I downloaded 1 week ago... also I download all files in 1 folder and when download finishes I move them to more suitable folder or delete them so download history is useless for me. Also a lot of my friends used it same way as I did.

So...if you want to make Firefox more user-friendly (as I read everywhere), return that button, if not, well... good luck in future.
I have to concur, this is a really silly decision
(In reply to comment #12)
> Say you have uninstalled the software, but decided that you wanted it again. 
> You can search for the download
etc.

At the risk of adding more "me too" spam, I believe that it's a rare event for the average user to repeat a download of the same file. So I don't believe that it's a good argument for disabling what was a clear, simple and expected UI.

The use of "Clear Private Data" for this, I think, will be unintuitive to anyone other than a power user. I think that most, like me, after some faffing around will discover that they can right click on the downloads to remove them and then become annoyed that they have to do this one at a time, not even realizing there's another (albeit disconnected) way.
You can't try to change the habits of your users by changing the interface... you'll just end up with disgruntled users.
Duplicate of this bug: 393491
Removing the Clean up button is a regression. Intentional or not, users will now have a hard time cleaning up the download list.

I'm developing software, too, and adding circumvulated ways to do simple things would confuse our users and they would switch away from us.

Please keep simple thing simple or better, KISS.

Big changes like this should be sent to Firefox 4, after usability tests. After reading comments here, I did not see a single valid reason to remove the Clean Up button.
fwiw if anyone wants the button back this instant, try the Download Cleanup-1.0 extension from http://webdesigns.ms11.net/chromeditp.html
(In reply to comment #19)
> fwiw if anyone wants the button back this instant, try the Download Cleanup-1.0
> extension from http://webdesigns.ms11.net/chromeditp.html
>

Could you please post that on AMO so we can get download stats for it?
Flags: blocking-firefox3?
(In reply to comment #20)
> Could you please post that on AMO so we can get download stats for it?

Even if I had been through the AMO-submission process before (which I haven't), I certainly wouldn't be happy in submitting someone else's extension to AMO.

And anyway, I personally don't believe it would be productive to do so. The only stats we'd get would be of the number of nightly testers who are scared of change and who aren't prepared to try something for a month to see if they can change their work-flow.

We mustn't forget that Firefox is targeted at new-fish (those people who don't even know what a browser is.) If our target audience is always loosing its downloads (which is what evidence suggests), then making it harder to clear the download history whilst having the ability to maintain a large download history and have search capabilities is A Good Thing and also The Way To Go.

That the majority of nightly testers don't like this change is neither here nor there imo. We are not Firefox's target audience, and we have enough skills to be able to locate and install an extension which brings back such functionality.
Even though I enjoy testing the lastest builds, this nonsense has got to stop.

As a general rule of thumb you do not change this type of thing (KISS).
  The Clear All Button is Simple.

By what logic do you assume that the average user wants a list of the last 1,000 files he has downloaded.  While it is helpful to see a list of downloaded files in some cases, in other cases it would be better to erase the list and move on.

The User Interface should not change in this way.  It does not matter that the new download manager adds great things to Firefox.  Removing the button makes the transition harder for the average user.  Having the button there does not force anyone to use it, however removing the button forces everyone to go to a completely different and unintuitive method.

I can not fathom how such a reasonable request can be drawn out for such a long time.  This type of nonsense probably frustrates many users and never gets dealt with.
How is removing a button making things more confusing for the user? Answer: It's not - it's making things simpler for them. The download history will probably be aged just like browser history - see bug 251337.
Excuse me.
If we would keep every options or UI piece from previous versions we'd even be stuck with a bigger UI mess than we have atm.
I filed this bug because I didn't know it was intentional.
I've lived with it for 3 weeks and some 500 downloads and I'd say leave it out.
I'd be happy if this was WONTFIX-ed, end of discussion.
This is about limiting the users choices.  Removing the button limits the users choices and makes a simple task more complex.  The ability to clear the list is still present, albeit in a different location.  Why not put a shortcut in the download manager?  It does not diminish anything.  A bit of humility is in order here.  
   Why is keeping a huge list of downloaded files so important? 
   Why is the ability to easily clear the list problematic?
   Why is this search functionality so sacrosanct?
   Why is the user forced to keep a history of downloaded files?
   Why do files that were opened by an application listed?
       Does the user need to see a few hundred statement.qif entries in the download manger?
   How about a purge deleted files button?

Please bring the new download manager up to par with the old one.  Searching is great, but give the user a choice.  Performance improvements should not come at the expense of expected behavior.  Look at this as an opportunity to highlight the new search features while preserving the ability to start with a clean slate.

This is *not* the place to be discussing UI changes.  Take it to the newsgroups.

I'd just like to note that most of the issues people are bringing up are non-issues because there is a way to do what you want.  Just because it isn't in the primary UI doesn't mean you get to complain about it and demand that it be fixed.
(In reply to comment #26)
> This is *not* the place to be discussing UI changes.  Take it to the
> newsgroups.

Which newsgroup? (newsgroups?)
mozilla.dev.apps.firefox (where the original UI discussions happened to remove the button in the first place).
I discovered that entries can be easily cleared by scrolling to the bottom of the list, clicking on the last entry and holding down the delete key.  I'd prefer to highlight multiple entries for deletion but the delete key works well enough for me.  There's an extension available to restore the cleanup button which I don't need given the improved functionality of the delete key and automatic highlighting of the next entry.   I'm withdrawing my vote for this bug.  
Blocks: 394597
Fwiw, pressing the delete key to delete download entries is not an option for me, since I try to do everything by mouse, not by keyboard.
Steve England: Thanks for the link to http://webdesigns.ms11.net/extensions/downloadcu-1.0.xpi. It works like a champ!! :)
I'll block on this with a slight morph: I'm not sure that this needs to be a button in the window itself. That said, there needs to be a way to select all of the completed downloads and remove them for the neat-freaks amongst us. A "Remove All Completed" menuItem in the context menu seems like a good way of doing this.
Flags: blocking-firefox3? → blocking-firefox3+
Summary: Clean Up button missing from new DM → "clean up" functionality missing from download manager
I agree Mike; we do not need a Clean all button if there are other ways to replace it.

Since pressing the Delete (Backspace) key now erases the currently selected download item, I would suggest to add to this and add a context menu item "Select All". This would be enough for the neat-freak like me.

Event greater, control-A (Mac command-A) key combination would make things purfect. I vote for Command-A -> Delete (Backspace).
Ok, this patch adds the "Clean Up" functionality back as a context menu-item.
I still would prefer button in the download manager, but you can't have it all, I guess.
Attachment #281764 - Flags: review?
Attachment #281764 - Attachment is patch: true
Attachment #281764 - Attachment mime type: application/octet-stream → text/plain
Attachment #281764 - Flags: ui-review?(beltzner)
Attachment #281764 - Flags: review?(comrade693+bmo)
Attachment #281764 - Flags: review?
Comment on attachment 281764 [details] [diff] [review]
patch to add "Clean Up" context menu-item

>+    document.getElementById("cmd_cleanUp").removeAttribute("disabled");
I don't see the need for this - please explain

>+  if (popup.childNodes.length > 0)
>+    return true;
>+
Why is this here exactly?

>+    if (!dl && aCommand != "cmd_cleanUp")
I'd prefer to have a switch statement above this if for any commands that don't require a download object to determine if it should be enabled or not.

>+    cmd_cleanUp: function() {
>+      gDownloadManager.cleanUp();
>+
>+      // Update UI
>+      for (var i = gDownloadsView.children.length - 1; i >= 0; --i) {
>+        var elm = gDownloadsView.children[i];
>+        var state = parseInt(elm.getAttribute("state"));
s/var/let/ for these three lines

>+        if (state != Ci.nsIDownloadManager.DOWNLOAD_NOTSTARTED &&
>+            state != Ci.nsIDownloadManager.DOWNLOAD_DOWNLOADING &&
>+            state != Ci.nsIDownloadManager.DOWNLOAD_PAUSED)
>+          gDownloadsView.removeChild(gDownloadsView.children[i]);
>+      }
>+        gDownloadsView.selectedIndex = gDownloadsView.itemCount - 1;
nit: spacing
Also, I'm not so certain this loop is correct.  Please see what we do when building the views in general:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/mozapps/downloads/content/downloads.js&rev=1.89#705
Attachment #281764 - Flags: review?(comrade693+bmo) → review-
Target Milestone: --- → Firefox 3 M9
Assignee: nobody → martijn.martijn
Attached patch patchv2 (obsolete) — Splinter Review
(In reply to comment #35)
> (From update of attachment 281764 [details] [diff] [review])
> >+    document.getElementById("cmd_cleanUp").removeAttribute("disabled");
> I don't see the need for this - please explain

Sorry, it wasn't needed, so I've removed it now.

> >+  if (popup.childNodes.length > 0)
> >+    return true;
> >+
> Why is this here exactly?

There can be 0 menu-items in the popup when there is nothing in the downloads window. You wouldn't want to get a empty popup appearing in that case.

> Also, I'm not so certain this loop is correct.  Please see what we do when
> building the views in general:

I added a check for 'richlistitem' as tagName, but I'm not sure if that's what you meant. I don't see in what way the loop is incorrect.

Thanks for the review.
Attachment #281764 - Attachment is obsolete: true
Attachment #282121 - Flags: review?(comrade693+bmo)
Attachment #281764 - Flags: ui-review?(beltzner)
Comment on attachment 282121 [details] [diff] [review]
patchv2

>+  if (gDownloadManager.canCleanUp) {
>+    popup.appendChild(document.getElementById("menuitem_cleanUp").cloneNode(true));
nit: line wrapping @ 80 chars

>+    switch (aCommand) {
>+      case "cmd_cleanUp":
>+        return gDownloadManager.canCleanUp;
>+    }
Please add a comment stating that this switch statement is for commands that do not require a download object.

>+      for (let i = gDownloadsView.children.length - 1; i >= 0; --i) {
>+        let elm = gDownloadsView.children[i];
>+        let state = parseInt(elm.getAttribute("state"));
>+
>+        if (elm.tagName == "richlistitem" &&
>+            state != Ci.nsIDownloadManager.DOWNLOAD_NOTSTARTED &&
>+            state != Ci.nsIDownloadManager.DOWNLOAD_DOWNLOADING &&
>+            state != Ci.nsIDownloadManager.DOWNLOAD_PAUSED)
>+          gDownloadsView.removeChild(gDownloadsView.children[i]);
>+      }
This still isn't technically correct.  Anything that isn't a richlistitem would not have a state property, so you should check that condition after you get elm.  Of course, .children is always supposed to be returning richlistitems only, so I suppose this is a moot point altogether :/
You can probably disregard my previous comment about this since it was functionally correct before.  (I don't quite agree with overriding a DOM attribute like richlistbox does in this case, but that's not my decision).

r=sdwilsh with these fixed.
Attachment #282121 - Flags: review?(comrade693+bmo) → review+
Attached patch patchv3 (obsolete) — Splinter Review
Updated with suggestions from comment 37.
Mike, this is what you wanted, right?

Your comment 32 suggests there still might be a chance you might consider the big button back in the download manager.
Attachment #282244 - Flags: ui-review?(beltzner)
Can't we just call buildDefaultView with a new concept of a "default view" -- anything more recent than the last cleanup time.
moving out bugs that don't need to block b1
Target Milestone: Firefox 3 M9 → Firefox 3 M10
Comment on attachment 282244 [details] [diff] [review]
patchv3

We'll need to file a follow-up to change the label as per the new DM design review mockups in bug 397655, but this is OK for addressing the immediate need.
Attachment #282244 - Flags: ui-review?(beltzner) → ui-review+
Comment on attachment 282244 [details] [diff] [review]
patchv3

This is low risk.
Attachment #282244 - Flags: review+
Attachment #282244 - Flags: approval1.9?
Filed Bug 398619 for the string change.  If someone wants to pick that up, it'd be greatly appreciated!
Attachment #282244 - Flags: approval1.9? → approval1.9+
Comment on attachment 282244 [details] [diff] [review]
patchv3

>+    cmd_cleanUp: function() {
>+      gDownloadManager.cleanUp();
>+
>+      // Update UI
>+      for (let i = gDownloadsView.children.length - 1; i >= 0; --i) {
>+        let elm = gDownloadsView.children[i];
>+        let state = parseInt(elm.getAttribute("state"));
>+
>+        if (state != Ci.nsIDownloadManager.DOWNLOAD_NOTSTARTED &&
>+            state != Ci.nsIDownloadManager.DOWNLOAD_DOWNLOADING &&
>+            state != Ci.nsIDownloadManager.DOWNLOAD_PAUSED)
>+          gDownloadsView.removeChild(gDownloadsView.children[i]);
>+      }
>+      gDownloadsView.selectedIndex = gDownloadsView.itemCount - 1;
The download manager cleanup will do the query to delete entries which will then notify with "download-manager-clear-history" now that bug 392152 is fixed. So there shouldn't be an need to manually iterate through and remove child nodes.
Attached patch patchv3rev2 (obsolete) — Splinter Review
Thanks for letting me know. Indeed, it works fine now if I remove that.
I'll just let Shawn rereview this, but I'll just assume this has still got ui-review and approval1.9 because it's basically still patchv3 (but without the code you referred to).
Attachment #282121 - Attachment is obsolete: true
Attachment #282244 - Attachment is obsolete: true
Attachment #283717 - Flags: review?(comrade693+bmo)
Comment on attachment 283717 [details] [diff] [review]
patchv3rev2

>@@ -547,20 +547,27 @@ function buildContextMenu(aEvent)
...
>+  if (gDownloadManager.canCleanUp) {
>+    popup.appendChild(document.getElementById("menuitem_cleanUp")
>+                      .cloneNode(true));
>     return true;
>   }
Might as well reuse the gContextMenus if we're following the download manager mockups that /don't/ have the cleanup menu item for all downloads -- only completed ones. Removes the need for the extra logic to make sure we have items for a context menu.

>   isCommandEnabled: function(aCommand)
...
>+    //This switch statement is for commands that do not need a download object
Space after //
Comment on attachment 283717 [details] [diff] [review]
patchv3rev2

(In reply to comment #46)
> Might as well reuse the gContextMenus if we're following the download manager
> mockups that /don't/ have the cleanup menu item for all downloads -- only
> completed ones. Removes the need for the extra logic to make sure we have items
> for a context menu.
I like this idea a lot better actually.  Just add it to the appropriate places here
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/mozapps/downloads/content/downloads.js&rev=1.92#512
Attachment #283717 - Flags: review?(comrade693+bmo) → review-
Attachment #282244 - Flags: approval1.9+
Blocks: 397655, 398619
Attached patch v4Splinter Review
Use gContextMenus and switch to Clear List for bug 398619.
Attachment #283717 - Attachment is obsolete: true
Attachment #285569 - Flags: review?(comrade693+bmo)
Blocks: 400495
Assignee: martijn.martijn → nobody
Comment on attachment 285569 [details] [diff] [review]
v4

r=sdwilsh

This is a low risk patch.
Attachment #285569 - Flags: review?(comrade693+bmo)
Attachment #285569 - Flags: review+
Attachment #285569 - Flags: approval1.9?
I agree that the Clean up button should be there.
Some people including myself find it useful.
It doesn't disturb anyway, does it? So why not adding it back?
Lior, the plan is to readd the Clean up button, at least, that was the plan, last time I checked.
Attachment #285569 - Flags: approvalM9+
Attachment #285569 - Flags: approval1.9?
Attachment #285569 - Flags: approval1.9+
Assignee: nobody → edilee
Checking in toolkit/locales/en-US/chrome/mozapps/downloads/downloads.dtd;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/mozapps/downloads/downloads.dtd,v  <--  downloads.dtd
new revision: 1.12; previous revision: 1.11
done
Checking in toolkit/mozapps/downloads/content/downloads.js;
/cvsroot/mozilla/toolkit/mozapps/downloads/content/downloads.js,v  <--  downloads.js
new revision: 1.95; previous revision: 1.94
done
Checking in toolkit/mozapps/downloads/content/downloads.xul;
/cvsroot/mozilla/toolkit/mozapps/downloads/content/downloads.xul,v  <--  downloads.xul
new revision: 1.31; previous revision: 1.30
done
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: in-testsuite?
Flags: in-litmus?
Resolution: --- → FIXED
Target Milestone: Firefox 3 M10 → Firefox 3 M9
Verified FIXED using

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a9pre) Gecko/2007102720 Minefield/3.0a9pre

and

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a9pre) Gecko/2007102721 Minefield/3.0a9pre

There is now a "Clear List" context-menu item that clears multiple completed/failed/cancelled downloads.

The bug to implement/decide whether to implement the "Clear List" button is bug 400495, but please don't spam it.
Status: RESOLVED → VERIFIED
Duplicate of this bug: 405175
Flags: in-testsuite? → in-testsuite-
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.