Allow multiple selections in Download Manager

VERIFIED FIXED in mozilla1.9

Status

()

Toolkit
Downloads API
--
enhancement
VERIFIED FIXED
14 years ago
9 years ago

People

(Reporter: Warner Young, Assigned: Mardak)

Tracking

(Depends on: 1 bug)

Trunk
mozilla1.9
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9 +
blocking-firefox2 -
in-testsuite +
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

14 years ago
It'd be nice to be able to select multiple files in Download Manager and operate
on the selected ones together (such as removing several files at the same time,
without removing all of them).  The old Download sidebar allowed this.

Steps to reproduce:
1. Select an item in Download Manager.
2. Hold down Ctrl or Shift (on Windows) and select a different item.

Expected results:
For Ctrl, both the old and new items are selected.  For shift, all items from
the old to the new are selected. Any right-click menu operations should affect
all selected items.

Actual results:
The old item is deselected while the new one is selected.

Updated

13 years ago
OS: Windows XP → All
QA Contact: aebrahim
Hardware: PC → All
Summary: [RFE] Allow multiple selections in Download Manager → Allow multiple selections in Download Manager

Updated

13 years ago
Severity: normal → enhancement

Comment 1

13 years ago
Adding Download Manager Tweak to the code could fix this and a WHOLE LOT of the
other download manager "bugs" and "enhancement requests".

Updated

13 years ago
Blocks: 262161

Updated

13 years ago
No longer blocks: 262161

Updated

12 years ago
Flags: blocking-aviary2?
Not going to happen for Fx2, depends on bug 298371 which is a trunk core change.  If that gets implemented in a branch-safe way we can revisit at the appropriate time.
Assignee: bugs → nobody
Depends on: 298371
Flags: blocking-aviary2? → blocking-aviary2-
QA Contact: aebrahim-bmo-507 → download.manager

Comment 3

12 years ago
(In reply to comment #2)
> Not going to happen for Fx2, depends on bug 298371 which is a trunk core
> change.  If that gets implemented in a branch-safe way we can revisit at the
> appropriate time.
> 

It sounds so far out there. 

Should this just be target:future?

Comment 4

11 years ago
(In reply to comment #2)
> Not going to happen for Fx2, depends on bug 298371 which is a trunk core
> change.  If that gets implemented in a branch-safe way we can revisit at the
> appropriate time.
> 
Is it time yet? Thanks, and sorry.

Comment 5

10 years ago
Hooray, blocking bug 298371 has been fixed :-)

Updated

10 years ago
Target Milestone: --- → Future
Version: unspecified → Trunk
Flags: wanted-firefox3+
(Assignee)

Comment 6

9 years ago
Created attachment 312283 [details] [diff] [review]
v1

Hey look! This patch has been conveniently mirrored at http://ed.agadak.net/multiple.dl.patch ... ;) ;)

Perform the command for each item selected if we're just performing a command. This happens when you do stuff like hit enter, hit delete or you select something from the context menu. But *not* if you click a download action button like pause for a particular download.

We still only show the context menu for the first selected item, but that should be fine as the user is either selecting downloads for similar purposes:
* delete all selected items
* retry all selected items
* open referring pages for all selected items
* pause all selected items

But even if you do select "cross action" like a paused download and unpaused, we'll correctly not try to resume a completed download.

One tricky case is "copy download link", so I gave it preference to copy the first selected download's link by going backwards.
Assignee: nobody → edilee
Status: NEW → ASSIGNED
Attachment #312283 - Flags: review?(sdwilsh)
(Assignee)

Comment 7

9 years ago
We can try making this for Firefox 3! :)
Target Milestone: Future → Firefox 3
(Assignee)

Updated

9 years ago
Attachment #312283 - Flags: ui-review?(beltzner)
Comment on attachment 312283 [details] [diff] [review]
v1

Why not have "Copy Download Link" just copy the group with CRLFs inserted between them? I could see that being useful if you were posting your download links to a friend in an email or something.

Otherwise, I'd suggest just disabling that entry on the context menu. The actions need to either apply to all of the selection, or none.
Attachment #312283 - Flags: ui-review?(beltzner) → ui-review+
(Assignee)

Comment 9

9 years ago
Created attachment 312394 [details] [diff] [review]
v1.1

(In reply to comment #8)
> Why not have "Copy Download Link" just copy the group with CRLFs inserted
> between them?
Done.
Attachment #312283 - Attachment is obsolete: true
Attachment #312394 - Flags: review?(sdwilsh)
Attachment #312283 - Flags: review?(sdwilsh)
Comment on attachment 312394 [details] [diff] [review]
v1.1

>+let gPerformAllCallback;
A details comment about what this is used for would be appreciated here.

>+      // If we're performing all, we can save some work by only doing it once
>+      if (gPerformAllCallback === null)
>+        gPerformAllCallback = true;
>+      else if (gPerformAllCallback)
>+        return;
not super happy about overloading this variable, but as long as it's documented well in the file I'm OK with it.

>+    // If we don't have a desired download item, do the command for all
>+    // selected items. Initialize the callback to null so commands know to add
>+    // a callback if they want. We will call the callback with empty arguments
>+    // after performing the command on each selected download item.
This changes behavior of the code, untested code at that.  In what situation was it null before?  What invariants are we expecting to be true now?

>+    gPerformAllCallback = undefined;
Why undefined and not null here?  The states of this variable should be documented.


Still not completely sure what this callback would be used for exactly.  Care to explain more of why you did all this?

Also, I need tests for this for me to even consider giving this r+ at this point.  At a *minimum*, I need to see tests for the following:
Performing each command on a single download.
Performing each command on two selected downloads.

The single download test should land before this patch lands (since it should be valid regardless).
Attachment #312394 - Flags: review?(sdwilsh) → review-
(Assignee)

Updated

9 years ago
Assignee: edilee → nobody
Status: ASSIGNED → NEW
Target Milestone: Firefox 3 → Future
(Assignee)

Comment 11

9 years ago
Here's a build if people want to play around with multiple selections.

https://build.mozilla.org/tryserver-builds/2008-03-31_21:09-edward.lee@engineering.uiuc.edu-multi.select.download/
(Assignee)

Comment 12

9 years ago
Created attachment 314497 [details] [diff] [review]
v1.2

(In reply to comment #10)
> (From update of attachment 312394 [details] [diff] [review])
> >+let gPerformAllCallback;
> A details comment about what this is used for would be appreciated here.
Commented about when it's undefined, null, or a function.

> >+      // If we're performing all, we can save some work by only doing it once
> >+      if (gPerformAllCallback === null)
> >+        gPerformAllCallback = true;
> >+      else if (gPerformAllCallback)
> >+        return;
> not super happy about overloading this variable, but as long as it's documented
> well in the file I'm OK with it.
It's just a dummy check. I made it be a function to keep in line with what it should be.

> Still not completely sure what this callback would be used for exactly.  Care
> to explain more of why you did all this?
The callback is necessary to allow the command operating on individual items to communicate with each other as well as letting the command do something /after/ all items are visited. This allows extensions to replace the default implementation and getting the option to support multiple item actions.

> Also, I need tests for this for me to even consider giving this r+ at this
> point.  At a *minimum*, I need to see tests for the following:
> Performing each command on a single download.
> Performing each command on two selected downloads.
chrome://mochikit/content/browser/toolkit/mozapps/downloads/tests/browser/browser_multi_select.js
	PASS - Paused no downloads
	PASS - Resumed no downloads
	PASS - Canceled no downloads
	PASS - Opened no downloads
	PASS - Showed one download
	PASS - Retried one download
	PASS - Opened one referrer
	PASS - Copied one location
	PASS - Removed one downloads, remaining 3
	PASS - Paused neither download
	PASS - Resumed neither download
	PASS - Canceled neither download
	PASS - Opened neither download
	PASS - Showed both downloads
	PASS - Retried both downloads
	PASS - Opened both referrers
	PASS - Copied both locations
	PASS - Removed both downloads, remaining 1
Assignee: nobody → edilee
Attachment #312394 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #314497 - Flags: review?(sdwilsh)
Comment on attachment 314497 [details] [diff] [review]
v1.2

>+  // We have a callback to use, so use it to add a uri
>+  if (typeof gPerformAllCallback == "function")
>+    gPerformAllCallback(uri);
>+  // It's a plain copy source, so copy it
>+  else
>+    clipboard.copyString(uri);
I'd actually like to see the comment inside the blocks, and that means we need bracing.

>+      // If we're performing all, we can save some work by only doing it once
>+      if (gPerformAllCallback === null)
>+        gPerformAllCallback = function() true;
maybe I'm just not used to the syntax yet, but it almost looks like you want the function to return true.  I think it'd be clearer to just do function() { }

>+  } finally {
>+    stmt.reset();
>+    stmt.finalize();
>+  }
nit:
}
finally {

>+      let getClipboard = function() {
>+        let clip = Cc["@mozilla.org/widget/clipboard;1"].
>+          getService(Ci.nsIClipboard);
>+        let trans = Cc["@mozilla.org/widget/transferable;1"].
>+          createInstance(Ci.nsITransferable);
nit: line up second lines with Cc

r=sdwilsh
Attachment #314497 - Flags: review?(sdwilsh) → review+
Comment on attachment 314497 [details] [diff] [review]
v1.2

a=beltzner, yay tests, this is hot like a fire.
Attachment #314497 - Flags: approval1.9+
(Assignee)

Comment 15

9 years ago
Created attachment 314503 [details] [diff] [review]
v1.3

(In reply to comment #13)
> >+  // It's a plain copy source, so copy it
> >+  else
> >+    clipboard.copyString(uri);
> I'd actually like to see the comment inside the blocks, and that means we need
> bracing.
Moved in.

> >+        gPerformAllCallback = function() true;
> maybe I'm just not used to the syntax yet, but it almost looks like you want
> the function to return true.  I think it'd be clearer to just do function() { }
Good idea.

> >+  } finally {
> nit:
> }
> finally {
Sure.

> >+        let clip = Cc["@mozilla.org/widget/clipboard;1"].
> >+          getService(Ci.nsIClipboard);
> >+        let trans = Cc["@mozilla.org/widget/transferable;1"].
> >+          createInstance(Ci.nsITransferable);
> nit: line up second lines with Cc
Done.
Attachment #314497 - Attachment is obsolete: true
I guess you got approval without even asking for it :)
(Assignee)

Comment 17

9 years ago
Checking in toolkit/mozapps/downloads/content/downloads.js;
/cvsroot/mozilla/toolkit/mozapps/downloads/content/downloads.js,v  <--  downloads.js
new revision: 1.145; previous revision: 1.144
done
Checking in toolkit/mozapps/downloads/content/downloads.xul;
/cvsroot/mozilla/toolkit/mozapps/downloads/content/downloads.xul,v  <--  downloads.xul
new revision: 1.49; previous revision: 1.48
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.24; previous revision: 1.23
done
RCS file: /cvsroot/mozilla/toolkit/mozapps/downloads/tests/browser/browser_multi_select.js,v
done
Checking in toolkit/mozapps/downloads/tests/browser/browser_multi_select.js;
/cvsroot/mozilla/toolkit/mozapps/downloads/tests/browser/browser_multi_select.js,v  <--  browser_multi_select.js
initial revision: 1.1
done
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: Future → Firefox 3

Comment 18

9 years ago
There is a flaw in how multiple selection works in the download manager.  For example:

1. Click item 1  ->  item 1 is selected
2. Shift-click item 4 -> items 1-4 are selected
3. Control click item 6 -> items 1-4, 6 are selected
4. Control-shift click item 8 -> items 1-4, 6, 8 are selected

After step 4 I would have expected that all of items 1-4, 6-8 become selected, like in a multiple selection box (such as the one containing the email addresses on this bugzilla page).  But item 7 is not selected.  It seems that control-shift-click does not work properly.
That would be a bug with the richlistbox, not with the download manager.
(Assignee)

Updated

9 years ago
Depends on: 428530
Flags: in-litmus?
Verified FIXED using:

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

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

-and-

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

in-litmus+:

https://litmus.mozilla.org/show_test.cgi?id=5300
Status: RESOLVED → VERIFIED
Flags: in-litmus? → in-litmus+
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.