228842 - Allow multiple selections in Download Manager

Status: VERIFIED FIXED

(Toolkit :: Downloads API, enhancement)

Description

[Warner Young]

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.

Comment 1

[Worcester12345]

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

Comment 2

[Mike Connor [:mconnor]]

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.

Comment 3

[Worcester12345]

(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

[Worcester12345]

(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

[Markus Keller]

Hooray, blocking bug 298371 has been fixed :-)

Comment 6

[Ed Lee :Mardak]

Attachment: 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.

Comment 7

[Ed Lee :Mardak]

We can try making this for Firefox 3! :)

Comment 8

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

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.

Comment 9

[Ed Lee :Mardak]

Attachment: v1.1

(In reply to comment #8)
> Why not have "Copy Download Link" just copy the group with CRLFs inserted
> between them?
Done.

Comment 10

[Shawn Wilsher :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).

Comment 11

[Ed Lee :Mardak]

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

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

Comment 12

[Ed Lee :Mardak]

Attachment: 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

Comment 13

[Shawn Wilsher :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

Comment 14

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

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

a=beltzner, yay tests, this is hot like a fire.

Comment 15

[Ed Lee :Mardak]

Attachment: 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.

Comment 16

[Shawn Wilsher :sdwilsh]

I guess you got approval without even asking for it :)

Comment 17

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

Comment 18

[Szabolcs Horvát]

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.

Comment 19

[Shawn Wilsher :sdwilsh]

That would be a bug with the richlistbox, not with the download manager.

Comment 20

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

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