pause/resume download

RESOLVED FIXED in Camino0.9

Status

P2
enhancement
RESOLVED FIXED
16 years ago
10 years ago

People

(Reporter: bugzilla, Assigned: nick.kreeger)

Tracking

unspecified
Camino0.9
PowerPC
macOS

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

16 years ago
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).
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

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

Comment 3

15 years ago
taking
Assignee: sfraser → josha

Comment 4

15 years ago
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

14 years ago
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/

Updated

14 years ago
Priority: -- → P2
Target Milestone: --- → Camino1.0
(Assignee)

Comment 6

14 years ago
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.
(Assignee)

Comment 7

14 years ago
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
(Assignee)

Updated

14 years ago
Attachment #190956 - Flags: review?(sfraser_bugs)
(Assignee)

Comment 8

14 years ago
Contains the extra UI elements for pause/resume
- icons
- updated ProgressView.NIB
- update Localized.strings
Attachment #190958 - Flags: review?(sfraser_bugs)
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.


(Assignee)

Comment 10

14 years ago
Updated to fix annonying tab issue in patch.
Attachment #190956 - Attachment is obsolete: true
Attachment #190978 - Flags: review?(sfraser_bugs)

Comment 11

14 years ago
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.
Attachment #190978 - Flags: review?(sfraser_bugs) → review-
> 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

14 years ago
(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").
(Assignee)

Comment 14

14 years ago
Updated for smfr and pinkertons comments
Attachment #190978 - Attachment is obsolete: true
Attachment #191138 - Flags: review?(sfraser_bugs)
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.
(Assignee)

Comment 16

14 years ago
Posted patch Meets pinks last comment (obsolete) — Splinter Review
Meets mikes suggestion for mDownloader if-check
Attachment #191138 - Attachment is obsolete: true
Attachment #191139 - Flags: review?(sfraser_bugs)
(Assignee)

Comment 17

14 years ago
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.
(Assignee)

Updated

14 years ago
Attachment #191139 - Attachment is obsolete: true
Attachment #191329 - Flags: review?(sfraser_bugs)
+-(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. 



(Assignee)

Comment 19

14 years ago
Posted patch Meets pinks nits (obsolete) — Splinter Review
Attachment #191329 - Attachment is obsolete: true
Attachment #191413 - Flags: review?(pinkerton)
(Assignee)

Comment 20

14 years ago
Attachment #191413 - Attachment is obsolete: true
Attachment #191419 - Flags: review?(pinkerton)
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.
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
Summary: need an easier way to restart/retry canceled or interrupted downloads from d/l mgr → pause/resume download
Target Milestone: Camino1.0 → Camino0.9
Should we add a relnote mentioning that not all servers support resuming downloads?
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
Attachment #191413 - Flags: review?(pinkerton)
Attachment #191419 - Flags: review?(pinkerton)
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.
Attachment #191329 - Flags: review?(sfraser_bugs)
You need to log in before you can comment on or make changes to this bug.