474622 - Hook up delete key to deleting entries in new download manager

Status: RESOLVED FIXED

(SeaMonkey :: Download & File Handling, defect)

Description

[Robert Kaiser]

I haven't hooked up any keys to tree action in bug 472001, but we should at least hook up the DEL key to remove download list entries.

Comment 1

[Robert Kaiser]

test_delete_key_removes.xul and test_select_all.xul have todo()s that are looking for this feature.

Comment 2

[Jens Hatlak (:InvisibleSmiley)]

I think Stop (Cancel for active downloads, Remove for inactive ones) would be even better. Active downloads would first go to the Canceled state (Retry possible) and from there to Removed. Else Delete would do nothing for active downloads.

This bug is very similar in nature to bug 487681 (Space -> Resume) and bug 495545 (Enter/double click -> Open) and affects the same files (downloadmanager.xul, downloadmanager.js and maybe downloadmanager.dtd). I'd fix all three in one go (also faster to review).

Comment 3

[Jens Hatlak (:InvisibleSmiley)]

Attachment: stop/pauseResume/open

One patch for three bugs. Notes:
- only added a key to Open menuitem because we currently don't have menuitems exactly matching the actions bound to the keys (maybe menuitems should be introduced for Pause/Resume and Stop but that's probably another bug)
- removed unused cut/copy/delete keys (or am I wrong?)
- kept the restriction to one selected download (see bug 474619)

Comment 4

[Robert Kaiser]

This patch is wrong, sorry.
1) You don't need to add any commands
2) You can do this from the treeView.js, and probably don't need to touch anything else
3) Tests fail as they mark the deleting action as todo()

Comment 5

[Robert Kaiser]

To be clear, #2 is speculation, I am less and less sure of it the more I investigate it. The others should stand, though.

Comment 6

[[e-mail]]

(In reply to comment #3)
> - only added a key to Open menuitem because we currently don't have menuitems
> exactly matching the actions bound to the keys (maybe menuitems should be
> introduced for Pause/Resume and Stop but that's probably another bug)
I don't know about Stop but for Pause/Resume you'd want to conflate the existing pause and resume menuitems into a single menuitem (either a checkbox menuitem or a label-switching menuitem).

(In reply to comment #4)
> This patch is wrong, sorry.
> 1) You don't need to add any commands
It seems to be a reasonable way to do it, since the tree controller already has the logic to work out whether the selection is suitable.

> 2) You can do this from the treeView.js, and probably don't need to touch
> anything else
As KaiRo mentioned on IRC he read some misleading documentation :-(

Comment 7

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v2

Implemented label switching, changed accesskey for Resume (R currently clashes with Retry) and enabled existing tests (didn't check whether they succeed yet).

Unfortunately Space doesn't show up as the shortcut for Pause/Resume in the menu even though I added the key relationship. Is there another way to specify it?

KaiRo requested/required several additional tests via IRC (that delete key cancels running/paused downloads and Enter/double click opens finished downloads but not in the search field or on the Clear List button) but I need more time for that so let's do the review of the actual changes in parallel.

Comment 8

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #7)
> enabled existing tests (didn't check whether they succeed yet).

I checked now, they don't:

654 INFO TEST-PASS | .../test_delete_key_removes.xul | The database and the number of downloads display matches
655 INFO TEST-PASS | .../test_delete_key_removes.xul | The download was properly removed
656 ERROR TEST-UNEXPECTED-FAIL | .../test_delete_key_removes.xul | The download was properly removed - got 5, expected 4

700 INFO TEST-PASS | .../test_select_all.xul | All downloads selected
701 ERROR TEST-UNEXPECTED-FAIL | .../test_select_all.xul | All downloads removed - got 3, expected 0

704 ERROR TEST-UNEXPECTED-FAIL | .../test_space_key_pauses_resumes.xul | The download was paused, and then resumed to completion

AFAICS the reasons are:
1. We lose focus after after every Stop. I guess we shouldn't. If you agree we'd have to move the selection to the next download in the list. If you don't we'd have to do that in the test.
2. We currently don't handle multiple selections for Stop so I changed the failing test back to todo_is() locally.
3. We don't ever set focus for Resume (typo 'selecttion', corrected locally). But even when this is fixed it doesn't work for me. Maybe a timing issue (since a timer is used)? I have a fast machine; when I see the Download Manager after the single test finishes (see below) the state of the download is Finished. :-/

You can run test case 3 individually (provided you built without --disable-tests of course) like this:

/path/to/objdir/mozilla # python _tests/testing/mochitest/runtests.py --test-path=suite/common/downloads/tests/test_space_key_pauses_resumes.xul --chrome --console-level=DEBUG

Comment 9

[[e-mail]]

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

>     switch (aCommand) {
>-      case "cmd_pause":
>+      case "cmd_pauseResume":
>         return selectionCount == 1 &&
>-               selItemData.isActive &&
>-               selItemData.state != nsIDownloadManager.DOWNLOAD_PAUSED &&
>-               selItemData.resumable;
>-      case "cmd_resume":
>-        return selectionCount == 1 &&
>-               selItemData.state == nsIDownloadManager.DOWNLOAD_PAUSED &&
>                selItemData.resumable;
Hmm, that's not quite the same - can a download be a) inactive b) unpaused c) resumable all at the same time?

>+    <key id="key_stop"
>+         keycode="VK_DELETE"
>+         command="cmd_stop"/>
Need backspace for Macbooks that don't have delete keys.

>+<!ENTITY cmd.pauseResume.label           "Pause">
>+<!ENTITY cmd.pauseResume.accesskey       "P">
Don't need this duplicated here as well as in the properties file...

> <!ENTITY cmd.retry.label                 "Retry">
> <!ENTITY cmd.retry.accesskey             "R">
Maybe we should merge Retry in with Pause and Resume here too. (Mind you, you might want a better command name than "cmd_pauseResumeRetry"!)

Comment 10

[Robert Kaiser]

Hrm, I very much dislike that merging of commands, and if it's done, I'll avoid touching any code related to it in the future (that may even include adding anything to the menus, like a "Properties" entry).

In any case, here are two things that need to be taken care of:
1) You need to write tests to check that we show the correct text in both main and context menus for different states of downloads
2) The case where the item is disabled needs to be carefully thought about: Should the item be displayed at all for for e.g. finished downloads, what text should be on the (disabled) menu item if it's shown?

Additionally, the name of "stop" for something that might remove data sounds misleading to me...

Comment 11

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #8)
> 704 ERROR TEST-UNEXPECTED-FAIL | .../test_space_key_pauses_resumes.xul | The
> download was paused, and then resumed to completion
> 
> 3. We don't ever set focus for Resume (typo 'selecttion', corrected locally).
> But even when this is fixed it doesn't work for me. Maybe a timing issue (since
> a timer is used)? I have a fast machine; when I see the Download Manager after
> the single test finishes (see below) the state of the download is Finished. :-/

Further tests show that it's not a timing issue, it's that the download is never reaching the DOWNLOADING state, at least not when onDownloadStateChange() is fired. Instead is goes like this: NOTSTARTED -> QUEUED -> TYPE_DOWNLOAD -> FINISHED. I don't know where TYPE_DOWNLOAD is coming from but it's not from the addDownload() parameterization (you can put something else there, no difference). Seems like something is messing with the states but I can't find it (probably SM-specific, though since I guess the FF test passes with the same test logic).

Comment 12

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v3

(In reply to comment #9)
> (From update of attachment 380705 [details] [diff] [review])
> >     switch (aCommand) {
> >-      case "cmd_pause":
> >+      case "cmd_pauseResume":
> >         return selectionCount == 1 &&
> >-               selItemData.isActive &&
> >-               selItemData.state != nsIDownloadManager.DOWNLOAD_PAUSED &&
> >-               selItemData.resumable;
> >-      case "cmd_resume":
> >-        return selectionCount == 1 &&
> >-               selItemData.state == nsIDownloadManager.DOWNLOAD_PAUSED &&
> >                selItemData.resumable;
> Hmm, that's not quite the same - can a download be a) inactive b) unpaused c)
> resumable all at the same time?

You're right, I made it worse. The equivalent middle part is this:
  (selItemData.isActive ||
   selItemData.state == nsIDownloadManager.DOWNLOAD_PAUSED)

Together with Retry it looks like this now:
      case "cmd_play":
        return selectionCount == 1 &&
               ((selItemData.resumable &&
                 (selItemData.isActive ||
                  selItemData.state == nsIDownloadManager.DOWNLOAD_PAUSED)) ||
                (selItemData.state == nsIDownloadManager.DOWNLOAD_CANCELED ||
                 selItemData.state == nsIDownloadManager.DOWNLOAD_FAILED));

I could add local variables for the parts and return the result of logic applied to those if you prefer that for better readability.

> >+    <key id="key_stop"
> >+         keycode="VK_DELETE"
> >+         command="cmd_stop"/>
> Need backspace for Macbooks that don't have delete keys.

Added (hope it works, can't check).

> >+<!ENTITY cmd.pauseResume.label           "Pause">
> >+<!ENTITY cmd.pauseResume.accesskey       "P">
> Don't need this duplicated here as well as in the properties file...

This is how it's done for "Check for Updates" (Help menu), cf. utilityOverlay.properties and utilityOverlay.dtd (both label and accesskey). How could I do it cleanly/understandably otherwise?

> > <!ENTITY cmd.retry.label                 "Retry">
> > <!ENTITY cmd.retry.accesskey             "R">
> Maybe we should merge Retry in with Pause and Resume here too. (Mind you, you
> might want a better command name than "cmd_pauseResumeRetry"!)

I chose Play (cf. cycleCell is treeView.js).

Comment 13

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #10)
> Hrm, I very much dislike that merging of commands

On second thought, maybe it's not such a good idea to merge the menu items. If you think about multi-selections (bug 474619) it's probably better to keep the actions separate as both the menu and context menu items could act on multiple selected downloads (in contrast to the buttons/icons which are per download). Those downloads could have very different states. Which labels should be chosen in cases where some states suggest Pause, others Resume and some Retry? With individual menu items we have no problem there and the implementation could trigger the chosen action for only those (selected) downloads where it applies.

Last but not least, it'll spare me writing those label tests. ;-)

> Additionally, the name of "stop" for something that might remove data sounds
> misleading to me...

Again, see cycleCell in treeView.js. The naming is consistent with that (and both essentially do the same) and like that the name is only used internally.

Comment 14

[Robert Kaiser]

(In reply to comment #13)
> (In reply to comment #10)
> > Additionally, the name of "stop" for something that might remove data sounds
> > misleading to me...
> 
> Again, see cycleCell in treeView.js. The naming is consistent with that (and
> both essentially do the same) and like that the name is only used internally.

Heh, OK, you caught me. "stop" seems to be my own invention after all. I'm misleading myself! :P

Comment 15

[Jens Hatlak (:InvisibleSmiley)]

Attachment: Enter/dblclick opens test

This is a new test for "Enter/double click opens" including four test cases (all of which pass) based on a rough implementation KaiRo provided me. I borrowed the idea for mouseDblClickOnCell() from tree_shared.js (avoids using hard-coded coordinates) and addDownload() from test_space_key_pauses_resumes.xul (we need to actually create a file because cmd_open checks that).

No progress on the existing (now failing) tests, need help from someone with more knowledge of download state changes.

Comment 16

[[e-mail]]

Comment on attachment 380940 [details] [diff] [review]
patch v3

>-    <key id="key_copy"/>
[key_/cmd_copy should probably copy the download link at some point.]

>-    <key id="key_delete"/>
>-    <key id="key_delete2"/>
>+    <key id="key_play"
>+         key=" "
>+         command="cmd_play"/>
>+    <key id="key_stop"
>+         keycode="VK_DELETE"
>+         command="cmd_stop"/>
>+#ifdef XP_MACOSX
>+    <key id="key_stop2"
>+         keycode="VK_BACK"
>+         command="cmd_stop"/>
>+#endif
Not sure what the best idea is here... as you might know, I'm not too keen on #ifdef, but there's a couple of choices...
a) just make backspace work like delete on all platforms
b) switch from _stop to _delete which has special Mac support already.

Comment 17

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #16)
> (From update of attachment 380940 [details] [diff] [review])
> >-    <key id="key_copy"/>
> [key_/cmd_copy should probably copy the download link at some point.]

Sure (different discussion, maybe bug 474640), but until then I think it's misleading to include something that's not bound to anything

> >-    <key id="key_delete"/>
> >-    <key id="key_delete2"/>
> >+    <key id="key_play"
> >+         key=" "
> >+         command="cmd_play"/>
> >+    <key id="key_stop"
> >+         keycode="VK_DELETE"
> >+         command="cmd_stop"/>
> >+#ifdef XP_MACOSX
> >+    <key id="key_stop2"
> >+         keycode="VK_BACK"
> >+         command="cmd_stop"/>
> >+#endif
> Not sure what the best idea is here... as you might know, I'm not too keen on
> #ifdef, but there's a couple of choices...
> a) just make backspace work like delete on all platforms
> b) switch from _stop to _delete which has special Mac support already.

I like b) but only for the key id (actually this /is/ the delete key, right?). I'd like to keep cmd_stop because cmd_delete would be misleading (it isn't deleting in all cases). I know key_delete defaults to command="cmd_delete" but I think it would be pretty self-evident that this is not the case here if the <key> redefinition included command="cmd_stop".

Comment 18

[[e-mail]]

(In reply to comment #17)
> (In reply to comment #16)
> > a) just make backspace work like delete on all platforms
> > b) switch from _stop to _delete which has special Mac support already.
> I like b) but only for the key id (actually this /is/ the delete key, right?).
> I'd like to keep cmd_stop because cmd_delete would be misleading (it isn't
> deleting in all cases). I know key_delete defaults to command="cmd_delete" but
> I think it would be pretty self-evident that this is not the case here if the
> <key> redefinition included command="cmd_stop".
Unfortunately it's the platform overlay that provides the command="cmd_delete" - the backend code for <key> elements depends on it being done that way...

Comment 19

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #18)
> > I'd like to keep cmd_stop because cmd_delete would be misleading (it isn't
> > deleting in all cases). I know key_delete defaults to command="cmd_delete" but
> > I think it would be pretty self-evident that this is not the case here if the
> > <key> redefinition included command="cmd_stop".
> Unfortunately it's the platform overlay that provides the command="cmd_delete"
> - the backend code for <key> elements depends on it being done that way...

Just to be sure I checked myself, you're right: it calls cmd_delete no matter what the command attribute in downloadmanager.xul says.

How about correcting the binding using JS? We already do this for the Bookmark Manager:
<http://mxr.mozilla.org/comm-central/source/suite/common/bookmarks/bookmarksManager.js#78>
<http://mxr.mozilla.org/comm-central/source/suite/common/bookmarks/bm-panel.js#45>

Of course it would be easier to just call the command cmd_delete but as I already said I think it would be misleading. Your call.

Comment 20

[Jens Hatlak (:InvisibleSmiley)]

Since currently neither double click nor Return nor Del work people almost certainly will stumble over this, requesting blocking Beta (you choose which ;-)).

Comment 21

[[e-mail]]

(In reply to comment #19)
> How about correcting the binding using JS? We already do this for the Bookmark Manager:
OK, that sounds reasonable.

Comment 22

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v4

The new patch doesn't remove Pause/Resume/Retry from the menus anymore (see comment 13) but keeps cmd_play for the Space key and resolves the Retry/Resume accesskey clash. The Delete keys trigger cmd_stop instead of cmd_delete (see comment 19).

I removed the changes to tests from the patch to make it easier to complete review: Some tests are still failing (see comment 8) and I partly don't know what the actual cause is (see comment 11) so I think it's better to handle the actual implementation, changes to existing tests, and new tests in separate patches (like the "Enter/dblclick opens test").

Comment 23

[[e-mail]]

Comment on attachment 384151 [details] [diff] [review]
patch v4

>+  if (key.getAttribute("command"))
Nit: use hasAttribute, it's minusculely faster ;-)

>-    <key id="key_cut"/>
>-    <key id="key_copy"/>
Don't remove these, that's a separate bug.

Comment 24

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v4a, r=neil

Comment 25

[Jens Hatlak (:InvisibleSmiley)]

Attachment: Enter/dblclick opens test v2

Turns out the original version of the test failed randomly because the download wasn't always finished when the tests were started. The new version works correctly but maybe it's a bit too complicated, I don't know.

Comment 26

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #11)
> Further tests show that it's not a timing issue, it's that the download is
> never reaching the DOWNLOADING state, at least not when onDownloadStateChange()
> is fired. Instead is goes like this: NOTSTARTED -> QUEUED -> TYPE_DOWNLOAD ->
> FINISHED. I don't know where TYPE_DOWNLOAD is coming from

Forget what I said. I misinterpreted that aState/aDownload.state being 0 would mean Ci.nsIDownloadManager.DOWNLOAD_TYPE_DOWNLOAD when in fact it means Ci.nsIDownloadManager.DOWNLOAD_DOWNLOADING (both are 0, see nsIDownloadManager.idl).

In other words, it /is/ a timing issue. It seems the download state can change so fast that it reaches DOWNLOAD_FINISHED before synthesizeKey() can pause/resume the download. Maybe my PCs are too fast or the machines running the automatic tests are (artificially?) slow, I don't know. Any ideas?

Comment 27

[[e-mail]]

Comment on attachment 384234 [details] [diff] [review]
Enter/dblclick opens test v2

Presumably this was roughly based on toolkit's test_space_key_pauses_resumes.xul?

Comment 28

[Robert Kaiser]

(In reply to comment #27)
> (From update of attachment 384234 [details] [diff] [review])
> Presumably this was roughly based on toolkit's
> test_space_key_pauses_resumes.xul?

AFAIK, I started with our version of that one before handing a wireframe over to Jens, yes.

Comment 29

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #27)
> (From update of attachment 384234 [details] [diff] [review])
> Presumably this was roughly based on toolkit's
> test_space_key_pauses_resumes.xul?

Yes, see comment 15.

Open issues:
- test_delete_key_removes.xul fails due to lost focus after Stop (see comment 8)
- test_space_key_pauses_resumes.xul randomly fails (see comment 26)
- additional tests requested by KaiRo:
  - delete key cancels running/paused downloads
  - Enter/double click/Del/Space don't trigger actions on downloads if the focus is in the search field or on the Clear List button.

Sigh.

Comment 30

[Jens Hatlak (:InvisibleSmiley)]

Attachment: enable/fix Delete removes test

In comment 8 I asked whether I should fix the root cause or just the test. The attached patch fixes both.

1. the tree view is updated through calls made in retryDownload() and gDownloadObserver.observe.

2. Toolkit uses removeFromView() to update the view. That function also updates the selection:
<http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/mozapps/downloads/content/downloads.js#1094>

3. We in contrast use removeDownload() on our tree view which until now doesn't update the selection:
<http://mxr.mozilla.org/comm-central/source/suite/common/downloads/treeView.js#353>

-> Adding the selection logic solves the problem without having to fix the test (note: removeDownload() is also called if you click the Stop icon on a download, cf. treeView.js cycleCell).

BTW: I'll probably create a single patch for checkin once all issues from comment 29 have been addressed. Until then I'll add one patch per problem and stop including diffs for Makefile.in, it'll be in the final patch where I'll request review once again to be sure I didn't miss a part.

Comment 31

[[e-mail]]

Comment on attachment 384741 [details] [diff] [review]
enable/fix Delete removes test

>+    // Update selection
>+    this.selection.select(Math.min(index, this.itemCount - 1));
We should only do this if the selection is empty as a result of the deletion (so not if a different row is deleted).

Comment 32

[Robert Kaiser]

(In reply to comment #30)
> 3. We in contrast use removeDownload() on our tree view which until now doesn't
> update the selection:
> <http://mxr.mozilla.org/comm-central/source/suite/common/downloads/treeView.js#353>

1) I had updating the selection in the original dlmgr patch but Neil told me that the tree code does that automatically.
2) Your patch assumes that we just never have selection on a random collection of multiple tree rows, or do I get that wrong?

Comment 33

[Jens Hatlak (:InvisibleSmiley)]

Attachment: enable/fix Delete removes test v2

(In reply to comment #31)
> (From update of attachment 384741 [details] [diff] [review])
> >+    // Update selection
> >+    this.selection.select(Math.min(index, this.itemCount - 1));
> We should only do this if the selection is empty as a result of the deletion
> (so not if a different row is deleted).

OK.

(In reply to comment #32)
> 1) I had updating the selection in the original dlmgr patch but Neil told me
> that the tree code does that automatically.

Maybe he confused it with the way it works in the Cookie Manager: DeleteSelectedItemFromTree() from treeUtils.js handles selection updates there while performing removes.

> 2) Your patch assumes that we just never have selection on a random collection
> of multiple tree rows, or do I get that wrong?

Well, I was thinking of gDownloadObserver.observe which specifically discriminates between single and multiple selection but on second thought I agree with you. At least for now we should play safe and only handle the obvious case: single selection of the download that is removed. I updated the patch accordingly.

Comment 34

[[e-mail]]

Comment on attachment 384914 [details] [diff] [review]
enable/fix Delete removes test v2

>+      this.selection.select(Math.min(index, this.itemCount - 1));
itemCount?

Comment 35

[[e-mail]]

Comment on attachment 384914 [details] [diff] [review]
enable/fix Delete removes test v2

Oh, you mean rowCount? r=me with that fixed.

Comment 36

[Jens Hatlak (:InvisibleSmiley)]

Attachment: enable/fix Delete removes test v2a, r=neil

(In reply to comment #35)
> (From update of attachment 384914 [details] [diff] [review])
> Oh, you mean rowCount?

Yes, too much copy & paste.

Comment 37

[Jens Hatlak (:InvisibleSmiley)]

Attachment: Action keys respect focus test

As announced this doesn't include the (trivial) Makefile.in changes needed to enable the test.

Comment 38

[Ian Neal]

Not going to block beta 1 on this but if patch makes it in, brilliant.

Comment 39

[Jens Hatlak (:InvisibleSmiley)]

Regarding "test_space_key_pauses_resumes.xul randomly fails" (comment 26):

I found the reason. There are two problems involved:

1. the DOWNLOAD_DOWNLOADING case doesn't use a timer so the Space key press happens at the wrong time. The fix is simple: just do it like in the DOWNLOAD_PAUSED case.

2. the Space key press doesn't pause or resume the download. This is because the DownloadTreeView incorrectly states that the download is not resumable while in fact it is if you ask the download directly.

After using a timer for both Space key presses and inserting the following line at the beginning of the test's onDownloadStateChange() function the test passed:
this.mWin.gDownloadTreeView.getRowData(0).resumable = aDownload.resumable;

Analysis for 2:
When pressing the Space key cmd_play is invoked correctly but selItemData.resumable is FALSE in the cmd_play case in downloadmanager.js's isCommandEnabled() (with the patch from this bug applied). At the same time, aDownload.resumable in the test is TRUE. selItemData is retrieved using DownloadTreeView.getRowData() which looks it up in DownloadTreeView's internal _dlList array. As far as I can see the "resumable" property of a download stored in that array is only set by DownloadTreeView's addDownload(), updateDownload() and initTree() functions and none of those is called directly when a download changes its state.

Toolkit downloads.js's isCommandEnabled() on the other hand directly asks the download whether it's resumable, paused or inProgress. I guess we should do the same. Otherwise we'd have to find a way to update the view live.

Neil, what do you think?

Comment 40

[Robert Kaiser]

(In reply to comment #39)
> As far as I can see the "resumable" property of a download
> stored in that array is only set by DownloadTreeView's addDownload(),
> updateDownload() and initTree() functions and none of those is called directly
> when a download changes its state.

We should always update the line correctly when the state changes but I wonder if any download can change .resumable after being initally added. In any case, isn't updateDownload() called from the download listener when the state changes (didn't check but I thought it was)?

Comment 41

[[e-mail]]

(In reply to comment #40)
> (In reply to comment #39)
> > As far as I can see the "resumable" property of a download
> > stored in that array is only set by DownloadTreeView's addDownload(),
> > updateDownload() and initTree() functions and none of those is called directly
> > when a download changes its state.
> We should always update the line correctly when the state changes but I wonder
> if any download can change .resumable after being initally added.
Downloads are never resumable while they are download or paused, so they can change resumability when their state changes.

Comment 42

[[e-mail]]

Sorry, I rewrote part of that and forgot to finish. What I meant was:
Downloads are only ever resumable while they are downloading or paused, so they can change resumability when their state changes.

Comment 43

[[e-mail]]

>if (this._dlList[row].currBytes != aDownload.amountTransferred) {
>  this._dlList[row].endTime = Date.now();
>  this._dlList[row].currBytes = aDownload.amountTransferred;
>  this._dlList[row].maxBytes = aDownload.size;
>  this._dlList[row].progress = aDownload.percentComplete;
>  this._dlList[row].resumable = aDownload.resumable;
>}
>if (this._dlList[row].state != aDownload.state) {
>  this._dlList[row].state = aDownload.state;
So maybe it would be better to update resumable in the second block.

Comment 44

[Robert Kaiser]

(In reply to comment #43)
> So maybe it would be better to update resumable in the second block.

Right. Nice how working on a test catches an error in the implementation ;-)

Comment 45

[Jens Hatlak (:InvisibleSmiley)]

Attachment: enable/fix Space pauses/resumes test

(In reply to comment #43)
> So maybe it would be better to update resumable in the second block.

Yes, that works. :-)

I only included the treeView.js change for this test in this patch, the changes for the Delete removes test will be back in the final patch.

Comment 46

[Jens Hatlak (:InvisibleSmiley)]

Attachment: Delete key cancels test

Again, as announced this doesn't include the (trivial) Makefile.in changes needed to enable the test.

Checking my list of open issues (comment 29) the only thing left would be to make sure that a double click doesn't trigger actions on downloads if the focus is in the search field or on the Clear List button. I don't think this is worth a test though because the ondblclick event handler is defined on <tree id="downloadTree"> so it can hardly affect anything else. Let me know if you require the test.

One last thing that should have a test is Retry which is triggered by Space if the download is in the canceled state. I'll attach a patch for that shortly. Other than that I think we're through.

Comment 47

[Jens Hatlak (:InvisibleSmiley)]

Attachment: Space key retries test

Again, as announced this doesn't include the (trivial) Makefile.in changes needed to enable the test.

Theoretically this test could be merged with the Space key pauses/resumes test but I think it's better to keep them separate (not only for comparability with Toolkit but also for KISS reasons / one test per issue).

Comment 48

[[e-mail]]

(In reply to comment #46)
> Checking my list of open issues (comment 29) the only thing left would be to
> make sure that a double click doesn't trigger actions on downloads if the focus
> is in the search field or on the Clear List button.
The click would focus the tree anyway.

Comment 49

[Robert Kaiser]

Right, double click focuses the item anyways, so you don't need to test that. I was more worried about the DEL, Enter or space keys in the search field and Enter or space on the clear button when requesting the tests stated in comment #7.

Comment 50

[Jens Hatlak (:InvisibleSmiley)]

Attachment: complete patch

(In reply to comment #36)
> Created an attachment (id=385419) [details]
> enable/fix Delete removes test v2a, r=neil

>+    // Update selection if only removed download was selected
>+    if (wasSingleSelection && this.selection.count == 0)
>+      this.selection.select(Math.min(index, this.rowCount - 1));

Actually this is wrong when there is nothing to select (this.rowCount==0) because select() is then called with -1. In the equivalent Toolkit code this is no problem because the result of Math.min() is only used as the new value of gDownloadsView.selectedIndex there and selectedIndex==-1 means deselect. select() on the other hand expects an actual index. Proposal:

if (wasSingleSelection && this.selection.count == 0) {
  index = Math.min(index, this.rowCount - 1);
  if (index >= 0)
    this.selection.select(index);
}

(or != -1, if you want). Since the other review requests are still pending, please understand the review request for this patch as mainly for the above.

I found this because the new tests suddenly started to fail after I reintegrated the fix for the above test with "this._dlList[row] is undefined" (file: .../treeView.js line 619). Then, after I had undone the changes to test_multi_select.xul included in patch v3, all Download Manager tests passed. Time for a complete patch. :-)

Comment 51

[[e-mail]]

I tried the patch interactively (i.e. by invoking runtests.py) and although test_action_keys_respect_focus passed, test_delete_key_cancels failed both tests (with a cancelled download, so presumably a failure in the harness) AND left the download manager open; while the weird award goes to test_enter_dblclick_opens which passes it if I tab to the test and hit enter but fails if I click it.

Comment 52

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #51)
> I tried the patch interactively (i.e. by invoking runtests.py) and although
> test_action_keys_respect_focus passed, test_delete_key_cancels failed both
> tests (with a cancelled download, so presumably a failure in the harness) AND
> left the download manager open; while the weird award goes to
> test_enter_dblclick_opens which passes it if I tab to the test and hit enter
> but fails if I click it.

All tests pass for me 100% with the complete.patch on both Windows and Linux (without manual intervention of course) when run from the command line [1]. On which platform did you test? Did you use the complete patch (recommended) or the individual patches? Please also note comment 50, maybe it has an impact on more tests.

A Download Manager window left open should be no problem, all existing tests close open Download Manager windows during startup.

[1]
ALL: cd /path/to/objdir/mozilla; make mochitest-chrome
ONE: see comment 8

Comment 53

[Serge Gautherie (:sgautherie)]

Comment on attachment 386767 [details] [diff] [review]
complete patch


[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1.1pre) Gecko/20090705 SeaMonkey/2.0b1pre] (comm-1.9.1-win32-unittest/1246823309) (W2Ksp4)
(http://hg.mozilla.org/releases/mozilla-1.9.1/rev/7b9cc253bcb8
 +http://hg.mozilla.org/comm-central/rev/035fc7c4ed38)

All (6) tests pass, either as a whole (suite) or individually.

Though, at least when run individually, 3 tests leave the DM open:
test_delete_key_cancels.xul
test_space_key_pauses_resumes.xul
test_space_key_retries.xul

Comment 54

[Serge Gautherie (:sgautherie)]

(In reply to comment #53)
> Though, at least when run individually, 3 tests leave the DM open:

Fwiw, test_clear_button_disabled.xul too.

And, with this test (only), if I then move the mouse in/out of the tree area, I get:
{
Error: uncaught exception: [Exception... "Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIDownloadManager.getDownload]"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "JS frame :: chrome://communicator/content/downloads/treeView.js :: anonymous :: line 175"  data: no]

Error: uncaught exception: [Exception... "Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIDownloadManager.getDownload]"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "JS frame :: chrome://communicator/content/downloads/treeView.js :: anonymous :: line 191"  data: no]
}

***

Ftr, test_select_all.xul reports 2 'ok' and 1 'todo'...

Comment 55

[Jens Hatlak (:InvisibleSmiley)]

Attachment: complete patch w/ close win

(In reply to comment #53)
> Though, at least when run individually, 3 tests leave the DM open:
> test_delete_key_cancels.xul
> test_space_key_pauses_resumes.xul
> test_space_key_retries.xul

Fixed (had to do it on a timer because else SM crashes upon this.mWin.close(), at least on Windows).

(In reply to comment #54)
> (In reply to comment #53)
> > Though, at least when run individually, 3 tests leave the DM open:
> 
> Fwiw, test_clear_button_disabled.xul too.
> 
> And, with this test (only), if I then move the mouse in/out of the tree area, I
> get (...)

Both are out of scope of this bug. Please file a new one if you care (you because I don't exactly understand what you mean with the latter, but please only explain in that new bug if you need).

> Ftr, test_select_all.xul reports 2 'ok' and 1 'todo'...

This is expected, see comment 8 point 2.

Comment 56

[[e-mail]]

Comment on attachment 388121 [details] [diff] [review]
complete patch w/ close win

For whatever reason these tests all pass for me now :-)

Comment 57

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 388121 [details] [diff] [review]
complete patch w/ close win

I read 2.0b1 freeze is today but neither of you is on IRC right now. 
Asking for approval mainly because this is so late in the game; the l10n change is only concerning the accesskey (no functionality lost if localizers don't notice immediately but then they should probably have chosen another key anyway).
Asking for sr to make sure Neil's r+ was including everything (the pending reviews concern tests included in the reviewed patch).

A final note: Whenever this has all flags set appropriately please go ahead an check in, just make sure to include a better commit message than the bug summary, e.g. "Enable action keys (Del, Enter, Space) and double click in new Download Manager". Thanks!

Comment 58

[Robert Kaiser]

Comment on attachment 388121 [details] [diff] [review]
complete patch w/ close win

a=me for 2.0b1 if checked in before the strict freeze, L10n is OK because it doesn't (and don't need to) change the string ID.

Comment 59

[Robert Kaiser]

Pushed as http://hg.mozilla.org/comm-central/rev/d3b58b4f7a3f

Jens, please mark all patches as obsolete that are included in the version that was pushed and mark the bug fixed if this is done (as well as resolving any other bugs this may have fixed).