341701 - "When Camino Quits" removes cancelled downloads, not just successful one

Status: VERIFIED FIXED

(Camino Graveyard :: Downloading, defect)

Description

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Some time ago Kreeger fixed things so that cancelled downloads didn't get tossed from the downloads list, in preparation for using them as a placeholder for the url for a complete restart/retry (bug 303158) and for true resume-from-interrupt-point-using-.part (bug 215343).

Bug 308942 broke this (and I forgot to test this scenario); cancelled downloads are now removed on quit (was this pink's bug 308942 comment 57?).

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

When we fix this, we should also reword the pref text to include the word "successful" (or similar) to clarify, and change the "Upon successful download" option text to be "When the download finishes" (or "is complete" or similar) to eliminate redundancy.

Comment 2

[Nick Kreeger]

(In reply to comment #0)
> Some time ago Kreeger fixed things so that cancelled downloads didn't get
> tossed from the downloads list, in preparation for using them as a placeholder
> for the url for a complete restart/retry (bug 303158) and for true
> resume-from-interrupt-point-using-.part (bug 215343).
> 
> Bug 308942 broke this (and I forgot to test this scenario); cancelled downloads
> are now removed on quit (was this pink's bug 308942 comment 57?).
> 

Should "tossed from the downloads list" refer to clean up or remove?
Either way, the clean up action removes downloads that are cancelled.

Comment 3

[Nick Kreeger]

Taking

Comment 4

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

"Tossed from the downloads list" in comment 0 refers to the bug you fixed, pre-pause/resume, to make the downloads list persist on quit.

The issue here is that one of the automatic removal options removes only successfully completed downloads ("on completion") whereas the other automatic option removes all downloads ("on quit"), even cancelled/interrupted ones you might want to keep to retry or later resume.

"Remove" and "Clean Up" are both manual operations (and don't apply here), and you're specifically telling Camino to either remove a single item or all of them, regardless of state (well, not active ones).  You manually initiated it, and you know it's going to happen (and there's a way to not delete cancelled/interrupted ones you want to retry later).  With the automatic pref, you don't know it's going to delete cancelled/interrupted ones, at least not consistently, and there's no easy way to prevent it in the one case where it does delete them all.

Comment 5

[Nick Kreeger]

Attachment: Patch

Proposed patch, rather than deleting the plist, just remove the successful downloads from the list and then save.

Comment 6

[Håkan Waara]

Comment on attachment 227408 [details] [diff] [review]
Patch

>+    // If the download either failed or was cancelled, add it to the list to save

This comment would be easier to understand if it was "Remove successful downloads..." etc.

>+-(BOOL)isFailed
>+{
>+  return mDownloadingError;
>+}

I had to check if this was a NSString* (which I think it sounds like). How about renaming it to mDownloadFailed since it's a boolean?

r+ with the first and optionally the last comment addressed.

Comment 7

[Nick Kreeger]

Attachment: Updated Patch

This patch address the two comments from hwarra

Comment 8

[Mike Pinkerton (not reading bugmail)]

+  while  ((curProgView = [downloadsEnum nextObject]))
+  {
+    // Remove successful downloads from the list
+    if (![curProgView isFailed] && ![curProgView isCanceled])

fix spacing on while.

should we be asking failed or succeeded? usually the affirmative is easier to understand but that's the opposite logic of iscancelled...

Comment 9

[Nick Kreeger]

Attachment: Option B

Pink this moves the logic to another method called |hasSucceeded:| in ProgressViewController to make the call in ProgressDlgController cleaner.

Also fixed the space issue on the while loop

Comment 10

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 227418 [details] [diff] [review]
Option B

flows better, i think. sr=pink

Comment 11

[Nick Kreeger]

Fixed trunk und Branch

Comment 12

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

v. on the trunk