391863 - "clean up" functionality missing from download manager

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect)

Description

[Peter van der Woude [:Peter6]]

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

Comment 1

[Shawn Wilsher :sdwilsh]

If you mean the clean up button that existed previously, this was intentional.

Comment 2

[Jim Jeffery not reading bug-mail 1/2/11]

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.

Comment 3

[Shawn Wilsher :sdwilsh]

Iteration 1 mock-ups do, however iteration 2 ones do not

Comment 4

[Peter van der Woude [:Peter6]]

(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.

Comment 5

[Marco Bonardo [:mak]]

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.

Comment 6

[u88484]

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.

Comment 7

[Radek 'sysKin' Czyz]

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.

Comment 8

[Marco Bonardo [:mak]]

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

Comment 9

[Steve England [:stevee]]

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.

Comment 10

[Radek 'sysKin' Czyz]

(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?

Comment 11

[Landon Abney]

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.

Comment 12

[Shawn Wilsher :sdwilsh]

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).

Comment 13

[Nikola Kocić]

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.

Comment 14

[M** A****]

I have to concur, this is a really silly decision

Comment 15

[Wayne Woods]

(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.

Comment 16

[Brian Marshall]

You can't try to change the habits of your users by changing the interface... you'll just end up with disgruntled users.

Comment 18

[jrblier]

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.

Comment 19

[Steve England [:stevee]]

fwiw if anyone wants the button back this instant, try the Download Cleanup-1.0 extension from http://webdesigns.ms11.net/chromeditp.html

Comment 20

[u88484]

(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?

Comment 21

[Steve England [:stevee]]

(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.

Comment 22

[Ian Kester-Haney]

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.

Comment 23

[Steve England [:stevee]]

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.

Comment 24

[Peter van der Woude [:Peter6]]

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.

Comment 25

[Ian Kester-Haney]

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.

Comment 26

[Shawn Wilsher :sdwilsh]

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.

Comment 27

[Martijn Wargers (dead)]

(In reply to comment #26)
> This is *not* the place to be discussing UI changes.  Take it to the
> newsgroups.

Which newsgroup? (newsgroups?)

Comment 28

[Shawn Wilsher :sdwilsh]

mozilla.dev.apps.firefox (where the original UI discussions happened to remove the button in the first place).

Comment 29

[Dan Loomis]

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.  

Comment 30

[Martijn Wargers (dead)]

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.

Comment 31

[greg]

Steve England: Thanks for the link to http://webdesigns.ms11.net/extensions/downloadcu-1.0.xpi. It works like a champ!! :)

Comment 32

[Mike Beltzner [:beltzner, not reading bugmail]]

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.

Comment 33

[jrblier]

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).

Comment 34

[Martijn Wargers (dead)]

Attachment: patch to add "Clean Up" context menu-item

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.

Comment 35

[Shawn Wilsher :sdwilsh]

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

Comment 36

[Martijn Wargers (dead)]

Attachment: patchv2

(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.

Comment 37

[Shawn Wilsher :sdwilsh]

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.

Comment 38

[Martijn Wargers (dead)]

Attachment: patchv3

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.

Comment 39

[Ed Lee :Mardak]

Can't we just call buildDefaultView with a new concept of a "default view" -- anything more recent than the last cleanup time.

Comment 40

[Mike Connor [:mconnor]]

moving out bugs that don't need to block b1

Comment 41

[Mike Beltzner [:beltzner, not reading bugmail]]

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.

Comment 42

[Shawn Wilsher :sdwilsh]

Comment on attachment 282244 [details] [diff] [review]
patchv3

This is low risk.

Comment 43

[Shawn Wilsher :sdwilsh]

Filed Bug 398619 for the string change.  If someone wants to pick that up, it'd be greatly appreciated!

Comment 44

[Ed Lee :Mardak]

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.

Comment 45

[Martijn Wargers (dead)]

Attachment: patchv3rev2

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).

Comment 46

[Ed Lee :Mardak]

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 47

[Shawn Wilsher :sdwilsh]

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

Comment 48

[Ed Lee :Mardak]

Attachment: v4

Use gContextMenus and switch to Clear List for bug 398619.

Comment 49

[Shawn Wilsher :sdwilsh]

Comment on attachment 285569 [details] [diff] [review]
v4

r=sdwilsh

This is a low risk patch.

Comment 50

[Lior Halphon (LIJI)]

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?

Comment 51

[Martijn Wargers (dead)]

Lior, the plan is to readd the Clean up button, at least, that was the plan, last time I checked.

Comment 52

[Ed Lee :Mardak]

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

Comment 53

[Stephen Donner [:stephend] Not actively reading bugmail]

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.

Comment 54

[Stephen Donner [:stephend] Not actively reading bugmail]

https://litmus.mozilla.org/show_test.cgi?id=4727

in-litmus+