187483 - pause/resume download

Status: RESOLVED FIXED

(Camino Graveyard :: Downloading, enhancement, P2)

Description

[sairuh (rarely reading bugmail)]

would it be possible to implement an easier way to restart (or just retry) a
canceled or interrupted download from the download manager?

right now it's a multistep process:

1. initiate a download (eg, d/l a chimera or mozilla build) which brings up the
download manager window.

2. stop the download: hit the Cancel button (or somehow disconnect your network
connection ;).

3. retry the download: you need to first copy the contents of the Source field
(expanded d/l entry)...

4. ...then paste the source url into the urlbar (Location field) of a browser
window, then hit return.

results: the download starts again (expected), but another entry is added to the
d/l mgr window (kind of cluttering).

Comment 1

[Henri Sivonen (:hsivonen)]

Necko already supports HTTP 1.1 Range requests when resuming downloading of
partially cached cacheable HTTP objects. Considering that the Range support is
already there, it should be possible to support resuming interrupted downloads
(as in Mac IE 5 and Opera).

I'd like to see a "Resume" button in place of "Open" on interrupted download
manager items.

On click Camino would need to do the following:
1) Check whether the partially saved file is still on disk. (If not, redownload
in entirety.)
2) Get the number of bytes in the partially saved files.
3) Issue a HTTP request with a Range header for skipping the number of bytes
already downloaded.
4) If the server replies with 206 Partial Content, append to the partial file.
5) If the server replies with 200 OK, overwrite the previous partial file.

Comment 2

[Prachi Gauriar]

*** Bug 196558 has been marked as a duplicate of this bug. ***

Comment 3

[Josh Aas]

taking

Comment 4

[Kurt Weinschenker]

I have to agree with comment #1.  Camino *really* could benefit from the
download manager having the ability to restart a download owing to a busted
connection.

Alas, my coding skills are few.  Hopefully, we might see this feature by Camino 1.0!

Comment 5

[Jasper]

Josh I found some pretty extensive documentation on the API that allows
pause/resume in Mozilla. It's called libxpnet. The file can be found here.

http://www.mozilla.org/projects/xpnet/

Comment 6

[Nick Kreeger]

I am currently working on this and have a working pause/resume function done.
Japser and I are tidying up the UI as well while this process is worked on.

Comment 7

[Nick Kreeger]

Attachment: Patch that contains the diff for the source code also requires zip file attached as well

Note that this diff only contains the changes to the source code, the extra
localized.strings icons and updated progressview.nib is also required from the
zip file that has been attached to this bug as well

Comment 8

[Nick Kreeger]

Attachment: Contains extra UI elements for this feature (icons, nib, localized.strings)

Contains the extra UI elements for pause/resume
- icons
- updated ProgressView.NIB
- update Localized.strings

Comment 9

[Mike Pinkerton (not reading bugmail)]

please, 2 spaces instead of a tab.

+	unsigned count = [selected count];

be specific, |unsigned int|. you could also make this |const| if you wanted ;)

+	  if ([self shouldAllowPauseAction]) {
+		  [theItem setToolTip:NSLocalizedString(@"dlPauseButtonTooltip", @"Pause
selected download(s)";)];
+			[theItem setLabel:NSLocalizedString(@"dlPauseButtonLabel", @"Pause")];
+		  [theItem setAction:@selector(pause:)];
+		  [theItem setImage:[NSImage imageNamed:@"dl_pause.tif"]];
+		  return YES;
+	  }
+	  else if ([self shouldAllowResumeAction]) {
+		  [theItem setToolTip:NSLocalizedString(@"dlResumeButtonTooltip", @"Resume
selected download(s)";)];
+			[theItem setLabel:NSLocalizedString(@"dlResumeButtonLabel", @"Resume")];
+		  [theItem setAction:@selector(resume:)];
+		  [theItem setImage:[NSImage imageNamed:@"dl_resume.tif"]];
+		  return YES;
+	  }

move the code to set the item attributes into separate methods so that they can
be re-used. I noticed you have this same code elsewhere.

-  return [NSArray arrayWithObjects:@"cleanupbutton", @"removebutton",
@"cancelbutton", @"openbutton", NSToolbarFlexibleSpaceItemIdentifier,
@"revealbutton", nil];
+  return [NSArray arrayWithObjects:@"cleanupbutton", @"removebutton",
@"cancelbutton", @"pauseresumebutton", @"openbutton",
NSToolbarFlexibleSpaceItemIdentifier, @"revealbutton", nil];

for people that have modified their toolbar, do we have to change anything so
that these buttons will show up? change a string id for prefs or something?
otherwise people will never see this feature.

+	if (mDownloadPaused || mDownloadDone)
+		return mCompletedView;
+	else
+		return mProgressView;

don't need the |else|, just do:

if (...)
  return mCompletedView
return mProgressView;

+	if (!mUserCancelled && !mDownloadDone) 
+	{
+		if (mDownloader) 
+		{

why not put the check for |mDownloader| in the |if| as well, at the end? Gets
rid of a level of indenting and improves readability.

+			NSString* statusString = NSLocalizedString(@"DownloadPausedStatusString",
@"Paused at %@ of %@");

just use |NSLocalizedString(...,nil)| as we don't ever use the description for
anything. Just takes up extra data space.

+		  mRequest->Resume();
+		  mDownloadPaused = PR_FALSE;

is there any way to ask the request whether it's paused, rather than having to
keep track of it ourselves? Just one more thing to screw up....

also, are we sure it's ok to call Resume() on a request that's already complete?
Can this ever happen in the UI? I'm pretty sure you checked for it, just making
sure.

Comment 10

[Nick Kreeger]

Attachment: Contains the source code patch with fixed tab sizing

Updated to fix annonying tab issue in patch.

Comment 11

[Simon Fraser [no longer active]]

Comment on attachment 190978 [details] [diff] [review]
Contains the source code patch with fixed tab sizing

>Index: download/ProgressDlgController.mm
>===================================================================

>+-(IBAction)pause:(id)sender
>+{
>+  NSMutableArray* selected = [self getSelectedProgressViewControllers];
>+  unsigned count = [selected count];
>+  for (unsigned i = 0; i < count; i++) 
>+  {
>+    [[selected objectAtIndex:i] pause:self];

You should pass on 'sender', rather than using 'self'.

>+  }
>+
>+  [self rebuildViews];
>+}
>+
>+-(IBAction)resume:(id)sender
>+{
>+  NSMutableArray* selected = [self getSelectedProgressViewControllers];
>+  unsigned count = [selected count];
>+  for (unsigned i = 0; i < count; i++)
>+  {
>+    [[selected objectAtIndex:i] resume:self];

Ditto.

> -(NSArray *)toolbarAllowedItemIdentifiers:(NSToolbar *)toolbar
> {
>-  return [NSArray arrayWithObjects:@"removebutton", @"cleanupbutton", @"cancelbutton", @"openbutton", @"revealbutton", NSToolbarFlexibleSpaceItemIdentifier, nil];
>+  return [NSArray arrayWithObjects:@"removebutton", @"cleanupbutton", @"cancelbutton", @"pauseresumebutton", @"openbutton", @"revealbutton", NSToolbarFlexibleSpaceItemIdentifier, nil];
> }
> 
> -(NSArray *)toolbarDefaultItemIdentifiers:(NSToolbar *)toolbar
> {
>-  return [NSArray arrayWithObjects:@"cleanupbutton", @"removebutton", @"cancelbutton", @"openbutton", NSToolbarFlexibleSpaceItemIdentifier, @"revealbutton", nil];
>+  return [NSArray arrayWithObjects:@"cleanupbutton", @"removebutton", @"cancelbutton", @"pauseresumebutton", @"openbutton", NSToolbarFlexibleSpaceItemIdentifier, @"revealbutton", nil];
> }

Will the new buttons show up for someone who's customized the toolbar?

>Index: download/ProgressViewController.h
>===================================================================
>RCS file: /cvsroot/mozilla/camino/src/download/ProgressViewController.h,v
>retrieving revision 1.5
>diff -u -8 -r1.5 ProgressViewController.h
>--- download/ProgressViewController.h	21 Apr 2005 01:25:42 -0000	1.5
>+++ download/ProgressViewController.h	29 Jul 2005 15:15:13 -0000
>@@ -70,16 +70,17 @@
> 
>   IBOutlet ProgressView *mProgressView;      // in-progress view, STRONG ref
>   IBOutlet ProgressView *mCompletedView;     // completed view, STRONG ref
>   
>   BOOL            mIsFileSave;
>   BOOL            mUserCancelled;
>   BOOL            mDownloadingError;
>   BOOL            mDownloadDone;
>+  BOOL	           mDownloadPaused;

You have a tab here. And I think we might want to remove this variable (see
below)


>+-(BOOL)isPaused
>+{
>+  return mDownloadPaused;
>+}

I think this should just call down to the mDownloader.


>Index: download/nsDownloadListener.h
>===================================================================
>RCS file: /cvsroot/mozilla/camino/src/download/nsDownloadListener.h,v
>retrieving revision 1.14
>diff -u -8 -r1.14 nsDownloadListener.h
>--- download/nsDownloadListener.h	24 Apr 2005 21:16:21 -0000	1.14
>+++ download/nsDownloadListener.h	29 Jul 2005 15:15:13 -0000
>@@ -43,16 +43,17 @@
> #import <Appkit/Appkit.h>
> 
> #import "CHDownloadProgressDisplay.h"
> 
> #include "nsString.h"
> #include "nsIDownload.h"
> #include "nsIWebBrowserPersist.h"
> #include "nsIURI.h"
>+#include "nsIRequest.h"
> #include "nsILocalFile.h"
> 
> #include "nsIExternalHelperAppService.h"
> 
> 
> // maybe this should replace nsHeaderSniffer too?
> 
> class nsDownloadListener :  public CHDownloader,
>@@ -76,21 +77,22 @@
>     virtual void ResumeDownload();
>     virtual void CancelDownload();
>     virtual void DownloadDone(nsresult aStatus);
>     virtual void DetachDownloadDisplay();
>     
> private:
> 
>     nsCOMPtr<nsICancelable>         mCancelable;        // Object to cancel the download
>-
>+    nsCOMPtr<nsIRequest>            mRequest;           // Request to hook on status change, allows pause/resume
>     nsCOMPtr<nsIURI>                mURI;               // The URI of our source file. Null if we're saving a complete document.
>     nsCOMPtr<nsIURI>                mDestination;       // Our destination URL.
>     nsCOMPtr<nsILocalFile>          mDestinationFile;   // Our destination file.
>     nsresult                        mDownloadStatus;		// status from last nofication
>     PRInt64                         mStartTime;         // When the download started
>     PRPackedBool                    mBypassCache;       // Whether we should bypass the cache or not.
>     PRPackedBool                    mNetworkTransfer;     // true if the first OnStateChange has the NETWORK bit set
>     PRPackedBool                    mGotFirstStateChange; // true after we've seen the first OnStateChange
>     PRPackedBool                    mUserCanceled;        // true if the user canceled the download
>     PRPackedBool                    mSentCancel;          // true when we've notified the backend of the cancel
>+    PRPackedBool		     mDownloadPaused;      // true when download is paused
> };

You have tabs here.

I think you should add an inline accessor for mDownloadPaused
as a public method:

 PRBool IsDownloadPaused() const { return mDownloadPaused; }

and call that from the ProgressViewController code. This avoids 2 variables
that record the same thing.

Let's have one more round.

Comment 12

[Samuel Sidler (old account; do not CC)]

> Will the new buttons show up for someone who's customized the toolbar?

No. We had a discussion about this in IRC. The most easy "fix" for that is to
add a date customized pref to org.mozilla.Navigator.plist, and if prior to the
build this is put in, clear the customization and require the user to re-customize.

Does that sound reasonable and easy enough? No one else could think of a way
this could be done.

Comment 13

[Simon Fraser [no longer active]]

(In reply to comment #12)

> Does that sound reasonable and easy enough? No one else could think of a way
> this could be done.

There is a way. Just change the identifier used by the toolbar (currently
@"dlmanager").

Comment 14

[Nick Kreeger]

Attachment: Meets smfrs and pinkertons comments

Updated for smfr and pinkertons comments

Comment 15

[Mike Pinkerton (not reading bugmail)]

funny how simon and i had many of the same comments :)

+-(BOOL)isPaused
+{
+  return mDownloader->IsDownloadPaused();
+}

are you guaranteed |mDownloader| is always valid? you check for it in other
places, might as well be safe here as well.

looks good otherwise.

Comment 16

[Nick Kreeger]

Attachment: Meets pinks last comment

Meets mikes suggestion for mDownloader if-check

Comment 17

[Nick Kreeger]

Attachment: Update to patch fixes palette labels and fixes more |NSLocalizedStrings(@"", nil)|

Update to patch fixes the palette labels for the dload buttons toolbar, and
fixes all |NSLocalizedStrings(@"", nil)| for all the buttons in the dload
manager.

Comment 18

[Mike Pinkerton (not reading bugmail)]

+-(BOOL)isPaused
+{
+  if (mDownloader)
+    return mDownloader->IsDownloadPaused();
+  else
+    return NO;

no |else| needed

+  if (![self isPaused]) {
+    pauseResumeItem = [[NSMenuItem alloc]
initWithTitle:NSLocalizedString(@"dlPauseCMLabel", nil)
+                                   action:@selector(pause:) keyEquivalent:@""];
+  }
+  else {
+    pauseResumeItem = [[NSMenuItem alloc]
initWithTitle:NSLocalizedString(@"dlResumeCMLabel", nil) 
+                                   action:@selector(resume:) keyEquivalent:@""];
+  }

reverse the logic here so the |if| case is "paused". Positive expressions are
easier to read than negative ones. 

Comment 19

[Nick Kreeger]

Attachment: Meets pinks nits

Comment 20

[Nick Kreeger]

Attachment: missed one file in patch

Comment 21

[Mike Pinkerton (not reading bugmail)]

this bug has morphed too much into pause/resume. changing summary, marking
fixed. i spun off bug 303158 for the original purpose of this bug. sorry for the
hassle.

Comment 22

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

Should we add a relnote mentioning that not all servers support resuming downloads?

Comment 23

[Mike Pinkerton (not reading bugmail)]

if that's how nsRequest ends up working (and i assume it does), then yeah, we
probably should add something to the release notes and the help pages/faq on cb.org

Comment 24

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

Comment on attachment 191329 [details] [diff] [review]
Update to patch fixes palette labels and fixes more |NSLocalizedStrings(@"", nil)|

Clearing requests on obsolete patches; apologies for the bugspam.

Comment 25

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

Since I had to look up this bug today, tidying it up.

For reference, the checkin was http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=Camino&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=pinkerton%25aol.net&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-08-01&maxdate=2005-08-05&cvsroot=%2Fcvsroot