Closed Bug 414850 Opened 13 years ago Closed 13 years ago

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

Categories

(Toolkit :: Downloads API, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla1.9beta5

People

(Reporter: ispence, Assigned: Mardak)

Details

(Keywords: dataloss, uiwanted)

Attachments

(1 file, 3 obsolete files)

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
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.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → WONTFIX
I'm an idiot, sorry; I didn't see that you had a search filter going.

Lack of sleep does wonderful things; reopening.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Yeah, so should we dynamically adjust that context-menu item to be "Clear Current List", or something?

(And again, sorry!)

Madhava?
Keywords: uiwanted
OS: Linux → All
Hardware: PC → All
"Clear Current List" sounds great
Flags: blocking-firefox3?
Keywords: dataloss
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.
Flags: wanted-firefox3+
Flags: blocking-firefox3?
Flags: blocking-firefox3-
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.
Attached patch v1 (obsolete) — Splinter Review
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.
Assignee: nobody → edilee
Status: REOPENED → ASSIGNED
Attachment #309787 - Flags: review?(sdwilsh)
Comment on attachment 309787 [details] [diff] [review]
v1

Uh.
Attachment #309787 - Flags: review?(sdwilsh)
Attached patch v1.1 (obsolete) — Splinter Review
"".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.
Attachment #309787 - Attachment is obsolete: true
Attachment #309790 - Flags: review?(sdwilsh)
Attached patch v1.2 (obsolete) — Splinter Review
With comments fixed up ;)
Attachment #309790 - Attachment is obsolete: true
Attachment #309791 - Flags: review?(sdwilsh)
Attachment #309790 - Flags: review?(sdwilsh)
This build has the latest patch applied and should fix this issue:

https://build.mozilla.org/tryserver-builds/2008-03-16_18:52-edward.lee@engineering.uiuc.edu-sleepPause.clearSearch.syncTime.keyIcon/

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 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!
Attachment #309791 - Flags: review?(sdwilsh) → review+
(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.
(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.
Attached patch v1.3Splinter Review
(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.
Attachment #309791 - Attachment is obsolete: true
Attachment #310180 - Flags: approval1.9?
Comment on attachment 310180 [details] [diff] [review]
v1.3

a1.9=beltzner
Attachment #310180 - Flags: approval1.9? → approval1.9+
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.
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
Status: ASSIGNED → RESOLVED
Closed: 13 years ago13 years ago
Flags: in-testsuite+
Flags: in-litmus?
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3 beta5
(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.
(In reply to comment #6)
> Too late for string changes

Also not true... we're taking string changes after b5 for other things.
File a new bug please?  I was unaware that'd we be accepting any string changes after this beta.
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
Status: RESOLVED → VERIFIED
(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.
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.