474619 - Handle multiple selections more intelligently in new download manager

Status: RESOLVED FIXED

(SeaMonkey :: Download & File Handling, defect)

Description

[Robert Kaiser]

The first version of the new toolkit-based download manager UI doesn't work on multiple selections for any other command than copy source URL. We should be able to handle it on at least some other commands as well.

Comment 2

[Robert Kaiser]

test_multi_select.xul has tests for those actions, so needs to be adjusted if
we do that.

Comment 3

[Jens Hatlak (:InvisibleSmiley)]

Attachment: proposed patch

This adds support for multiple selections to:
- play/pause/resume/retry
- stop/cancel/remove
and adapts the test.

Notes:
- Only consistent selections enable a command, i.e. all selected items must fulfill the same requirements as a single selected download had to fulfill before.
- Due to the above, the command enabling for the changed commands is now mostly using the negation of the old logic to find a way to escape the loop.
- First I wanted to let the code handle inconsistent selections (which would also have allowed to break early from the checking loops) but KaiRo convinced me on IRC that we shouldn't do/allow something that might contravene the user's expectations. The user can still use column sorting to group similar downloads and select those.

Comment 4

[[e-mail]]

Comment on attachment 395149 [details] [diff] [review]
proposed patch

>+          m_selIdx.push(row)
Nit: missing ;

>+          if (dldata.state == nsIDownloadManager.DOWNLOAD_CANCELED ||
>+              dldata.state == nsIDownloadManager.DOWNLOAD_FAILED)
>+            continue;
>+          if (!dldata.resumable ||
>+              !((dldata.isActive ||
>+               dldata.state == nsIDownloadManager.DOWNLOAD_PAUSED)))
>+            return false;
I'd prefer these two to be combined if possible i.e.
if (state != CANCELED && state != FAILED &&
(!resumable || (!isActive && state != paused)))

>+          if (!dldata.isActive ||
>+             dldata.state == nsIDownloadManager.DOWNLOAD_PAUSED ||
>+             !dldata.resumable)
Nit: needs one space more indentation on the continuation lines

>     var m_selIdx = [];
>     if (selectionCount) {
>       m_selIdx = [];
Whoops ;-)

>+        var rmDlids = []; // removeDownload alters the selection, delay execution
It might be worth making selItemData an array of the row data, since that's what we actually use everywhere. Any remaining consumers of selItemData could probably then be simply converted to selItemData[0]. (Does restart not alter the selection?)

Comment 5

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v2

(In reply to comment #4)
> Does restart not alter the selection?

I guess you mean "retry"... You are right, it calls removeDownload so needs the same handling. Surprisingly "cancel" also needs special handling. I don't know why but without it strange things happen (testcase: start some larger downloads, e.g. Eclipse and OpenOffice, selected both and press Space and Del as needed to cycle the states).

Comment 6

[[e-mail]]

Comment on attachment 395362 [details] [diff] [review]
patch v2

Sorry, you misunderstood - selItemData[] should have the actual dldata objects so that you don't have to fetch them again or cache them or such like.

Comment 7

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v3 [Checkin: Comment 8]

(In reply to comment #6)
> (From update of attachment 395362 [details] [diff] [review])
> Sorry, you misunderstood - selItemData[] should have the actual dldata objects
> so that you don't have to fetch them again or cache them or such like.

That makes a lot of sense, actually... :-) Obviously I also accessed the wrong data through selItemData[0] before.

Comment 8

[Serge Gautherie (:sgautherie)]

Comment on attachment 395588 [details] [diff] [review]
patch v3
[Checkin: Comment 8]


http://hg.mozilla.org/comm-central/rev/27fe0942ca30

Comment 9

[Serge Gautherie (:sgautherie)]

(In reply to comment #8)

Needs an additional positive follow-up:
{
TEST-UNEXPECTED-PASS | ... | All downloads removed - got 0, expected 0
}
;->

Comment 10

[Jens Hatlak (:InvisibleSmiley)]

Attachment: fix (enable) test [Checkin: Comment 11]

Comment 11

[Serge Gautherie (:sgautherie)]

Comment on attachment 395882 [details] [diff] [review]
fix (enable) test
[Checkin: Comment 11]


http://hg.mozilla.org/comm-central/rev/7f233fe1983a
(forwarding previous Neil's review)