414850 - "Clear List" in download manager should only remove visible downloads

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect)

Description

[Ian Spence]

If I do a search for ".doc" in the download manager, it will show me every .doc file.

If I choose to right click and select "Clear List", I'd expect it to remove only each item matching ".doc" from my history. Instead, it removes everything.

"Clear List" should only remove items that are currently in the list.  Otherwise it should be changed to "Purge Download History" or something to that extent

Comment 1

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

Except that "list" always implies plurality.  If it were "entry" or "selection," I'd be inclined to agree with you.

You want "Remove From List".

Also, please always specify which build you're using.

Comment 2

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

I'm an idiot, sorry; I didn't see that you had a search filter going.

Lack of sleep does wonderful things; reopening.

Comment 3

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

Yeah, so should we dynamically adjust that context-menu item to be "Clear Current List", or something?

(And again, sorry!)

Madhava?

Comment 4

[Ian Spence]

"Clear Current List" sounds great

Comment 5

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

Or just disable it until the search is gone. I'm not sure it's easy to go and remove all the selections.

Not blocking as I don't think it's a super-common case, but it sure will be annoying to those who find it, so really-really-want.

Comment 6

[Ed Lee :Mardak]

Too late for string changes, but now that we have super searching, this might be a bigger issue. Search for "december" and clear list to hide your holiday shopping download plans..

So we could clear the currently shown list and clear the search and show the default list so it lets the user know that s/he didn't actually clear the whole list.

Comment 7

[Ed Lee :Mardak]

Attachment: v1

Clear the whole list if there's no search; otherwise, remove items one by one from the end.

Also clean up a little bit because gSearchTerms will always be an array.

Comment 8

[Ed Lee :Mardak]

Comment on attachment 309787 [details] [diff] [review]
v1

Uh.

Comment 9

[Ed Lee :Mardak]

Attachment: v1.1

"".split(/\s+/).length != 0 but it does == "".

Also, now with more testcase! But it's based on the one that bug 419691 needs to fix.. but at least it works fine on other platforms.

Comment 10

[Ed Lee :Mardak]

Attachment: v1.2

With comments fixed up ;)

Comment 11

[Ed Lee :Mardak]

This build has the latest patch applied and should fix this issue:

https://build.mozilla.org/tryserver-builds/2008-03-16_18:[e-mail]/

Bug 403412 - Download Manager window title fails to clear/update upon download completion
Bug 335418 - Pause downloads when computer is about to go to sleep
Bug 414850 - "Clear List" in download manager should only remove visible downloads
Bug 420482 - Big discrepancy in ETA between Downloads Manager & Downloads status bar panel
Bug 414326 - Use DownloadUtils for software update downloads

Bug 393678 - location bar autocomplete should take word boundaries in account
Bug 407946 - emphasize all matching text in title and url, not just the first match in title and url
Bug 415403 - Show matches for all search words for location bar autocomplete
Bug 418257 - Show what part of which tags match for urlbar autocomplete
Bug 392143 - show keywords as url bar autocomplete choices
Bug 249468 - Add all bookmark keywords to location bar autocomplete drop-down list
Bug 395161 - make it possible to restrict the url bar autocomplete results to bookmarks, tags, or history entries
Bug 407204 - adjust the title and url text sizes
Bug 406257 - reduce the number of rows in url bar autocomplete from 10 to 6
Bug 420437 - Search and emphasize quoted strings with spaces
Bug 422491 - Optimize AwesomeBar if it finished searching and found fewer than maxResults

Comment 12

[Shawn Wilsher :sdwilsh]

Comment on attachment 309791 [details] [diff] [review]
v1.2

>+      if (gSearchTerms == "") {
>+        gDownloadManager.cleanUp();
>+      } else {
nit: |else {| on a new line please

>+        // Try to remove each download one by one from the end, but we might
>+        // fail if there's an active download, but if we hit an active
>+        // download, we would want to stop anyway
>+        try {
>+          while (gDownloadsView.hasChildNodes())
>+            removeDownload(gDownloadsView.lastChild);
>+        } catch (e) { }
not sure we shouldn't be checking e.result and if it's not what we get when we try to remove an active one, at least log it, if not propagate it.

>+  for each (let site in ["delete.me", "i.live"]) {
>+    stmt.bindStringParameter(0, "Super Pimped Download");
>+    stmt.bindStringParameter(1, "file://dummy/file");
>+    stmt.bindStringParameter(2, "http://" + site + "/file");
>+    stmt.bindInt32Parameter(3, dm.DOWNLOAD_FINISHED);
>+    stmt.bindInt64Parameter(4, new Date(1985, 7, 2) * 1000);
>+    stmt.bindInt64Parameter(5, 111222333444);
>+
>+    // Add it!
>+    stmt.execute();
>+  }
>+
>+  stmt.finalize();
This should actually be in a try-finally block so we always finalize the statement

r=sdwilsh - thanks for the test!

Comment 13

[Ed Lee :Mardak]

(In reply to comment #12)
> >+        // Try to remove each download one by one from the end, but we might
> >+        // fail if there's an active download, but if we hit an active
> >+        // download, we would want to stop anyway
> >+        try {
> >+          while (gDownloadsView.hasChildNodes())
> >+            removeDownload(gDownloadsView.lastChild);
> >+        } catch (e) { }
> not sure we shouldn't be checking e.result and if it's not what we get when we
> try to remove an active one, at least log it, if not propagate it.
Would it also prevent the search from being cleared and focus to the list?

> >+  stmt.finalize();
> This should actually be in a try-finally block so we always finalize the
> statement
It would finalize when it goes out of scope anyway, no? If anything throws, the test is over and should cause it to be finalized anyway.

Comment 14

[Shawn Wilsher :sdwilsh]

(In reply to comment #13)
> Would it also prevent the search from being cleared and focus to the list?
I didn't say you had to throw it immediately ;)

> It would finalize when it goes out of scope anyway, no? If anything throws, the
> test is over and should cause it to be finalized anyway.
When it get's garbage collected, sure.  It could leave the database in a read only state until then.

Comment 15

[Ed Lee :Mardak]

Attachment: v1.3

(In reply to comment #12)
> >+      } else {
> nit: |else {| on a new line please
Not sure when we switched to this style, but sure.

> >+        try {
> >+          while (gDownloadsView.hasChildNodes())
> >+            removeDownload(gDownloadsView.lastChild);
> >+        } catch (e) { }
> not sure we shouldn't be checking e.result and if it's not what we get when we
> try to remove an active one, at least log it, if not propagate it.
Changed it to check for inProgress and not have it throw for active downloads.

> >+  stmt.finalize();
> This should actually be in a try-finally block so we always finalize the
> statement
Done.

Comment 16

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

Comment on attachment 310180 [details] [diff] [review]
v1.3

a1.9=beltzner

Comment 17

[Shawn Wilsher :sdwilsh]

Sorry - it's stmt.reset() that needs to be in the finally block (finalize is OK there after you reset()).  Please add that before checking in.

Comment 18

[Ed Lee :Mardak]

Checking in toolkit/mozapps/downloads/content/downloads.js;
/cvsroot/mozilla/toolkit/mozapps/downloads/content/downloads.js,v  <--  downloads.js
new revision: 1.140; previous revision: 1.139
done
Checking in toolkit/mozapps/downloads/tests/browser/Makefile.in;
/cvsroot/mozilla/toolkit/mozapps/downloads/tests/browser/Makefile.in,v  <--  Makefile.in
new revision: 1.20; previous revision: 1.19
done
RCS file: /cvsroot/mozilla/toolkit/mozapps/downloads/tests/browser/browser_cleanup_search.js,v
done
Checking in toolkit/mozapps/downloads/tests/browser/browser_cleanup_search.js;
/cvsroot/mozilla/toolkit/mozapps/downloads/tests/browser/browser_cleanup_search.js,v  <--  browser_cleanup_search.js
initial revision: 1.1
done

Comment 19

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

in-litmus+

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

Comment 20

[dynamis (Tomoya ASAI)]

(In reply to comment #6)
> Too late for string changes
As you say this may be too late for string changes but IMHO strings should be added/changed properly when the feature is changed.

Localizers don't only translate literally but we sometimes write/select better expression for users. In some case l10n depends on the behavior of the feature and localizers will have trouble not only with late l10n change but also with late behavior change.
When the behavior change, it's natural to change strings and we should not be afraid about late-l10n change I think.

In this case, to make users understand clearer/easier, we (ja locale) had translated "clear list" menu like "clear all download history" in Japanese but now we have to change translation since the behavior was changed.

(In reply to comment #3)
> Yeah, so should we dynamically adjust that context-menu item to be "Clear
> Current List", or something?
I think it's better to use "Clear Current List" dynamically for users and localizer can accept late-l10n change for users.

Comment 21

[Reed Loden [:reed]]

(In reply to comment #6)
> Too late for string changes

Also not true... we're taking string changes after b5 for other things.

Comment 22

[Shawn Wilsher :sdwilsh]

File a new bug please?  I was unaware that'd we be accepting any string changes after this beta.

Comment 23

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

Verified FIXED using:

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

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b5pre) Gecko/2008031904 Minefield/3.0b5pre

-and-

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

Comment 24

[dynamis (Tomoya ASAI)]

(In reply to comment #22)
> File a new bug please?  I was unaware that'd we be accepting any string changes
> after this beta.

I filed a new bug 423942. Thanks.