Closed
Bug 187483
Opened 22 years ago
Closed 19 years ago
pause/resume download
Categories
(Camino Graveyard :: Downloading, enhancement, P2)
Tracking
(Not tracked)
RESOLVED
FIXED
Camino0.9
People
(Reporter: bugzilla, Assigned: nick.kreeger)
References
Details
Attachments
(2 files, 6 obsolete files)
19.69 KB,
application/zip
|
Details | |
33.68 KB,
patch
|
Details | Diff | Splinter Review |
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•21 years ago
|
||
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•21 years ago
|
||
*** Bug 196558 has been marked as a duplicate of this bug. ***
Comment 4•20 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!
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•19 years ago
|
Priority: -- → P2
Target Milestone: --- → Camino1.0
Assignee | ||
Comment 6•19 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•19 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•19 years ago
|
Attachment #190956 -
Flags: review?(sfraser_bugs)
Assignee | ||
Comment 8•19 years ago
|
||
Contains the extra UI elements for pause/resume - icons - updated ProgressView.NIB - update Localized.strings
Attachment #190958 -
Flags: review?(sfraser_bugs)
Comment 9•19 years ago
|
||
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•19 years ago
|
||
Updated to fix annonying tab issue in patch.
Attachment #190956 -
Attachment is obsolete: true
Attachment #190978 -
Flags: review?(sfraser_bugs)
Comment 11•19 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-
Comment 12•19 years ago
|
||
> 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•19 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•19 years ago
|
||
Updated for smfr and pinkertons comments
Attachment #190978 -
Attachment is obsolete: true
Attachment #191138 -
Flags: review?(sfraser_bugs)
Comment 15•19 years ago
|
||
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•19 years ago
|
||
Meets mikes suggestion for mDownloader if-check
Attachment #191138 -
Attachment is obsolete: true
Attachment #191139 -
Flags: review?(sfraser_bugs)
Assignee | ||
Comment 17•19 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•19 years ago
|
Attachment #191139 -
Attachment is obsolete: true
Attachment #191329 -
Flags: review?(sfraser_bugs)
Comment 18•19 years ago
|
||
+-(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•19 years ago
|
||
Attachment #191329 -
Attachment is obsolete: true
Attachment #191413 -
Flags: review?(pinkerton)
Assignee | ||
Comment 20•19 years ago
|
||
Attachment #191413 -
Attachment is obsolete: true
Attachment #191419 -
Flags: review?(pinkerton)
Comment 21•19 years ago
|
||
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
Closed: 19 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?
Comment 23•19 years ago
|
||
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
Updated•19 years ago
|
Attachment #191413 -
Flags: review?(pinkerton)
Updated•19 years ago
|
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)
Attachment #191139 -
Flags: review?(sfraser_bugs)
Attachment #191138 -
Flags: review?(sfraser_bugs)
Attachment #190958 -
Flags: review?(sfraser_bugs)
Attachment #190956 -
Flags: review?(sfraser_bugs)
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
Assignee: joshmoz → nick.kreeger
QA Contact: chrispetersen → downloading
You need to log in
before you can comment on or make changes to this bug.
Description
•