187619 - d/l mgr should display downloads from previous sessions

Status: RESOLVED FIXED

(Camino Graveyard :: Downloading, defect, P3)

Description

[sairuh (rarely reading bugmail)]

the download manager doesn't display download entries from previous chimera
sessions (whether completed or canceled).

couldn't find an existing bug for this, but dup as needed.

Comment 1

[sairuh (rarely reading bugmail)]

fwiw, ppembed (2003.01.03.05-1.0) behaves the same way.

Comment 2

[Ludovic Hirlimann [:Usul]]

still true with 2003090102 on 10.2.6.

Comment 3

[Josh Aas]

taking

Comment 4

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

Josh, status update? Is this hard to implement?

(Also, lowering the severity to normal, this isn't major.)

Comment 5

[Josh Aas]

I don't know how hard it is - I haven't tried. I doubt it is trivial.

Comment 6

[Nick Kreeger]

Taking this, should be part of the updates for the dload manager after the
pause/resume patch lands. Will be fixed with the cancel/restart fix. Since we
need to show a cancled download after a quit->restart. 

Comment 7

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

Will fixing this bug and bug 303158 allow one to pause a download, quit Camino,
restart Camino, and resume the dowload, or does that require yet another bug? 
(Currently, when a download is paused, quitting asks if you want to "stop" the
download and cancels it if you say yes.)

Or are pause/resume strictly "within browser session" activities and
cancel/restart are the "cross-session" equivalent activities?

Comment 8

[Nick Kreeger]

The idea is to retain the sessions after the app quits

Comment 9

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

-> kreeger

Comment 10

[Nick Kreeger]

(In reply to comment #7)
> Will fixing this bug and bug 303158 allow one to pause a download, quit Camino,
> restart Camino, and resume the dowload, or does that require yet another bug? 
> (Currently, when a download is paused, quitting asks if you want to "stop" the
> download and cancels it if you say yes.)
> 
> Or are pause/resume strictly "within browser session" activities and
> cancel/restart are the "cross-session" equivalent activities?

Pause/resume sessions are browser session activities, the request kills itself
when camino (gecko/app shutdown calls) quits.

Right now I have developed a working solution that fixes this bug only, after a
patch gets done and lands, that should be the time to work on restarting a
download (bug 303158).

Comment 11

[Nick Kreeger]

Attachment: First version of patch awaiting nits and comments..

First version of patch, saves a plist for the instances.

Comment 12

[Simon Fraser [no longer active]]

Comment on attachment 194127 [details] [diff] [review]
First version of patch awaiting nits and comments..

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


>+-(void)saveProgressViewControllers
>+{
>+  NSMutableArray* instances = [[NSMutableArray alloc] init];
>+  unsigned int arraySize = [mProgressViewControllers count];

Since you know the size, it's slightly faster to do

    NSMutableArray* instances = [[NSMutableArray alloc]
initWithCapacity:arraySize];

I'd prefer a better var name than "instances". How about "downloads" or
"downloadArray"?

>+  for (unsigned int i = 0; i < arraySize; i++) 
>+  {
>+    [instances addObject: [[mProgressViewControllers objectAtIndex:i] progressViewDictionary]];
>+  }

I'm a fan of enumerators:

    NSEnumerator* downloadsEnum = [mProgressViewControllers objectEnumerator];
    ProgressViewController* curController;
    while ((curController = [downloadsEnum nextObject]))
    {
      [downloadArray addObject:[curController progressViewDictionary]];
    }

>+  // now save
>+  NSString *profileDir = [[PreferenceManager sharedInstance] newProfilePath];
>+  [instances writeToFile: [profileDir stringByAppendingPathComponent:@"downloads.plist"] atomically: YES];

You've leaked instances. You should autorelease it on the init line.

>+-(void)loadProgressViewControllers
>+{
>+  NSString *profileDir = [[PreferenceManager sharedInstance] newProfilePath];
>+  NSArray* instances = [NSArray arrayWithContentsOfFile: [profileDir stringByAppendingPathComponent:@"downloads.plist"]];

I'd prefer this as:
    NSString* downloadsPath = [[[PreferenceManager sharedInstance]
newProfilePath] stringByAppendingPathComponent:@"downloads.plist"];
    NSArray*  downloads     = [NSArray arrayWithContentsOfFile:downloadsPath];

That way you've separated out the two tasks onto separate lines
(1. get the path to the file, 2. load the file).

>+  if (instances)
>+  {
>+    unsigned int numValues = [instances count];
>+    for (unsigned int i = 0; i < numValues; i++)

Could use an enumerator here too.

>+    {
>+      ProgressViewController* aView = [[ProgressViewController alloc] init];
>+      [aView setProgressViewFromDictionary:[instances objectAtIndex:i]];

It makes sense now to give ProgressViewController an -initWithDictionary.

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

>+-(void)setProgressViewFromDictionary:(NSDictionary*)aDict
>+{
>+  // assign values from the dict
>+  mDestPath = [aDict valueForKey: @"destPath"];   
>+  mSourceURL = [aDict valueForKey: @"sourceURL"]; 
>+  mDownloadSize = [[aDict valueForKey: @"downloadSize"] longLongValue];
>+  mCurrentProgress = [[aDict valueForKey: @"currentProgress"] longLongValue];
>+  mDownloadTime = [[aDict valueForKey: @"downloadTime"] doubleValue];

>+  // have to retain these or else SIGTERM 11's

This makes it sound like you don't understand the refcounting, and you've
debugged the code into existence. It would be better to do:


    mDestPath = [[aDict valueForKey: @"destPath"] retain];

etc. above.

>+  mDownloadDone = YES; // always either completed of cancelled
>+  
>+  // set as cancel if sizes dont match up
>+  if (mDownloadSize != mCurrentProgress)
>+    mUserCancelled = YES; 

I guess this is OK, but it seems a bit hacky.

>+  [mProgressBar stopAnimation: self];
>+  [[NSWorkspace sharedWorkspace] noteFileSystemChanged:mDestPath];

Why are these here?

>+-(NSDictionary*)progressViewDictionary

Maybe rename this "downloadInfoDictionary" or something.

>+{
>+  NSMutableDictionary* dict = [[NSMutableDictionary alloc] init];
>+  [dict setObject: mDestPath forKey:@"destPath"];
>+  [dict setObject: mSourceURL forKey:@"sourceURL"];

No space after : please.

>+  [dict setObject:[NSNumber numberWithLongLong: mDownloadSize] forKey:@"downloadSize"];
>+  [dict setObject:[NSNumber numberWithLongLong: mCurrentProgress] forKey:@"currentProgress"];
>+  [dict setObject:[NSNumber numberWithDouble: mDownloadTime] forKey:@"downloadTime"];
>+  
>+  return dict;
>+}
>+

> -(void)onEndDownload:(BOOL)completedOK
> {
>   mDownloadingError = !completedOK;
>   
>+  // this may work, but it opens a file if the file can be opened.
>+  // need to find a better way of updating the outlets.
>   [self downloadDidEnd];
>   [mProgressWindowController didEndDownload:self withSuccess:completedOK];
> }

Not sure what that comment refers to.

Comment 13

[Nick Kreeger]

Attachment: Updated to meet smfrs comments...

Updated, changed according to smfrs comments, also per his comments added a
-initWithDictionary:(NSDictionary*)aDict for alloc-ing ProgressViewControllers

Comment 14

[Simon Fraser [no longer active]]

Before we land this, I'd like to know what happens if you accumulate hundreds of
downloads. Do we slow to a crawl?

Comment 15

[Nick Kreeger]

This patch/bug will depend on the resolution of bug 293995. The fear is that
100's of downloads will acumulate in the manager, causing the CPU to thrash to
keep up with updating each Progress View. 

Comment 16

[Nick Kreeger]

Attachment: Updated this fixes refresh thrasing from bug 293995

This is updated to contain the fix for bug 293995 (refresh thrasing). It simply
wont check the icon type unless the boolean is true. We only check on the start
up so saved downloads can get a icon and when a download starts and ends.

Comment 17

[Nick Kreeger]

Comment on attachment 194202 [details] [diff] [review]
Updated to meet smfrs comments...

removing review sorry for spam

Comment 18

[Nick Kreeger]

Comment on attachment 194202 [details] [diff] [review]
Updated to meet smfrs comments...

removing review sorry for spam

Comment 19

[Mike Pinkerton (not reading bugmail)]

+-(void)setProgressViewFromDictionary:(NSDictionary*)aDict;

does this need to be in the public API?

also, do we really want to be writing the file on every change? What if there
are like 100 items in there and the user deletes them 1 by 1? that could take a
long time.

Comment 20

[Simon Fraser [no longer active]]

(In reply to comment #19)
> also, do we really want to be writing the file on every change? What if there
> are like 100 items in there and the user deletes them 1 by 1? that could take a
> long time.

I think saving the plist is pretty damn fast. I don't think this is an issue. If
it is, then we can put it on a zero-delay timer to coalesce saves or something.

Comment 21

[Nick Kreeger]

Attachment: Fixes multiple-saves when removing downloads from the list

Removed the call to |saveProgressViewControllers| in |removeDownload| in
ProgressDlgController.mm. This will prevent multiple saving when removing
multiple downloads.

Comment 22

[Nick Kreeger]

Comment on attachment 194605 [details] [diff] [review]
Updated this fixes refresh thrasing from bug 293995

removing review from old patch

Comment 23

[Nick Kreeger]

Comment on attachment 195289 [details] [diff] [review]
Fixes multiple-saves when removing downloads from the list

removing review sorry

Comment 24

[Nick Kreeger]

Attachment: Fixes a couple of declaration issues for ProgressViewController

Moved |setProgressViewFromDictionary| to a private declaration and added
|initWithDictionary| to the header file.

Comment 25

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 195290 [details] [diff] [review]
Fixes a couple of declaration issues for ProgressViewController

r=pink i'll let simon give it a final once-over.

Comment 26

[Simon Fraser [no longer active]]

Comment on attachment 195290 [details] [diff] [review]
Fixes a couple of declaration issues for ProgressViewController

>+-(NSDictionary*)downloadInfoDictionary
>+{
>+  NSMutableDictionary* dict = [[NSMutableDictionary alloc] init];
>+  [dict setObject: mDestPath forKey:@"destPath"];
>+  [dict setObject: mSourceURL forKey:@"sourceURL"];
>+  [dict setObject:[NSNumber numberWithLongLong: mDownloadSize] forKey:@"downloadSize"];
>+  [dict setObject:[NSNumber numberWithLongLong: mCurrentProgress] forKey:@"currentProgress"];
>+  [dict setObject:[NSNumber numberWithDouble: mDownloadTime] forKey:@"downloadTime"];
>+  
>+  return dict;

This should return an autoreleased dict; currently, it leaks.
sr=sfraser with this fixed.

Comment 27

[Nick Kreeger]

Attachment: Fixes smfr's request for autorelease

Comment 28

[Nick Kreeger]

Attachment: Oops wrong declaration with |autorelease| correct initialization is in this patch...

Phew. long day, wrong initilization of NSMutableDictionary, I now have to
correct version here.

Comment 29

[Nick Kreeger]

Attachment: Updated so that when you quit with active downloads, they are saved.

When a user quits with active downloads in que, the app doesn't send any cancel
requests, so therefore the downloads wont save automatically. I fixed this by
adding a call to |saveProgressViewControllers| in |allowTerminate| in
ProgressDlgController.

Comment 30

[Mark Mentovai]

Checked in on trunk.  Preheat oven to 350o and bake for 25 minutes.  Then we'll
hit the branch too.

Comment 31

[Mark Mentovai]

Checked in on the branch, mmm.

Comment 32

[Jon Hicks]

Mmmm indeed! 

Comment 33

[Paul Borokhov (lensovet)]

Now that this is active, can we get an option that will automatically clear the
list on quit?