307753 - Downloads that get moved still enable "reveal in finder" and "open", implement file system notifications

Status: RESOLVED FIXED

(Camino Graveyard :: Downloading, defect, P2)

Description

[Nick Kreeger]

Before and upon landing the persistent download patch, when a user deletes or
moves a file from its original location, inside the download manager, the
buttons for "reveal in finder" and "open" are still enabled. 

Currently Camino just beeps when you try to attempt any of those options. What
we want is to unenable those buttons so that the user can't click them if they
have been deleted.

Comment 1

[Nick Kreeger]

Attachment: Propesed patch

This patch fixes the bug, it disables open and reveal buttons in the download
manager if the files aren't at the location they are supposed to be in the
download manager. (IE. download was trashed or moved)

Comment 2

[Nick Kreeger]

Attachment: Updated-Reworked patch, now listens for file system changes with carbon

Updated the patch so that we check the file system on start-up and if the file
system changes we update ProgressDlgController to check that when it calls
|shouldAllowOpenRevealAction|. Also if the file system does change, disable
"reveal" and "open" and also set the icon refresh flag so that the icon will
update in the view

Comment 3

[Nick Kreeger]

Comment on attachment 195829 [details] [diff] [review]
Updated-Reworked patch, now listens for file system changes with carbon

Hmm. as smfr pointed out, this patch only keeps track of downloads in the
default download folder. We need to monitor folders that other downloads are
living in. Marking thes patch obsolete.

Comment 4

[Nick Kreeger]

Attachment: Implments Carbon File System Notifications for all downloads

This patch adds Carbon File System Notifications (FSN) to
ProgressViewController. If the subscribed FSN changes, |checkFilePathLocation|
is called, and uses |NSFileManager| to check the destination path. 

Also inside ProgressDlgController, a check call to ProgressViewController is
made via the new |mFileExists| boolean in ProgressViewController. This
check-call is called when checking wheter to enable "show" and "open".

This patch will help problems occuring with Bruce's trash patch (bug 230320)

Comment 5

[Simon Fraser [no longer active]]

Comment on attachment 198059 [details] [diff] [review]
Implments Carbon File System Notifications for all downloads

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

> --(BOOL)shouldAllowOpenAction
> +-(BOOL)shouldAllowOpenRevealAction

Maybe rename this to say what it really does:

-(BOOL)selectedItemsFileExists

or 

-(BOOL)fileExistsForSelectedItems

>  {
>    NSMutableArray* selectedArray = [self getSelectedProgressViewControllers];

NSArray. Not need for mutability here. I thought we fixed all those?

>    unsigned int selectedCount = [selectedArray count];
> -  // if no selections are are active or canceled then allow open
> +  // if no selections are are active, canceled, or moved then allow open and reveal
>    for (unsigned int i = 0; i < selectedCount; i++) {
> -    if ([[selectedArray objectAtIndex:i] isActive] || [[selectedArray objectAtIndex:i] isCanceled]) {
> +    if ([[selectedArray objectAtIndex:i] isActive] || [[selectedArray objectAtIndex:i] isCanceled] ||
> +        ![[selectedArray objectAtIndex:i] fileExists]) {
>        return NO;
>      }
>    }
>    return YES;
>  }

Better to get [selectedArray objectAtIndex:i] once at the top of the loop:


for (unsigned int i = 0; i < selectedCount; i++) {
  ProgressViewController* curController = [selectedArray objectAtIndex:i];

  etc.

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

>    BOOL            mIsFileSave;
>    BOOL            mUserCancelled;
>    BOOL            mDownloadingError;
>    BOOL            mDownloadDone;
>    BOOL            mRefreshIcon;
> +  BOOL            mFileExists;
> +  
> +  FNSubscriptionRef subRef;
> +  FNSubscriptionUPP subUPP;

Please use the "m" prefix for member vars.

>  -(IBAction)open:(id)sender;
>  -(IBAction)pause:(id)sender;
>  -(IBAction)resume:(id)sender;
>  
>  -(BOOL)isActive;
>  -(BOOL)isCanceled;
>  -(BOOL)isSelected;
>  -(BOOL)isPaused;
> +-(BOOL)fileExists;
> +
> +-(void)checkFilePathLocation;

Rename this to "checkFileExists" or something. I'm not sure what
"checkFilePathLocation" means.

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

>  -(void)viewDidLoad;
> +-(void)subscribeDownloadPath;

Rename to
- (void)setupFileSystemNotification
or something.

> +-(void)subscribeDownloadPath
> +{
> +  // listen to the downloads folder
> +  NSString* dir = [mDestPath stringByDeletingLastPathComponent];
> +  if (dir)
> +  {
> +    subUPP = NewFNSubscriptionUPP((FNSubscriptionProcPtr)ProgressViewControllerProc);
> +    NSLog([@"subscribed " stringByAppendingString:dir]);

Remove NSLog for checkin.

> +    OSStatus err = FNSubscribeByPath(((const UInt8*)[dir UTF8String]), subUPP, (void*)self, nil, &subRef);

You should use [NSString fileSystemRepresentation] here.


> +-(void)checkFilePathLocation
> +{
> +  // if dest path doesn't exist or download is not done, don't check
> +  if (!mDestPath || !mDownloadDone)
> +    return;
> +    
> +  if (![[NSFileManager defaultManager] fileExistsAtPath:mDestPath])
> +  {
> +    NSLog([[mDestPath stringByDeletingPathExtension] stringByAppendingString:@" has moved."]);

Remove the NSLog.

Almost there!

Comment 6

[Nick Kreeger]

Attachment: Updated to meet smfr's comments

Updated patch to meet smfr's comments

Comment 7

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

This will block 1.0, per IRC conversation.

Comment 8

[Nick Kreeger]

This patch is bit-rotted now, i will post a new one upon the decision of bug 308680

Comment 9

[Nick Kreeger]

Attachment: Updated to current source

Updated to contain changes from bug 308680 and bug 314616

Comment 10

[Mark Mentovai]

Comment on attachment 201655 [details] [diff] [review]
Updated to current source

ProgressViewController has both init and initWithDictionary:.  Now that they're both getting heavier, you should make the latter call the former.

Comment 11

[Nick Kreeger]

Attachment: Updated to include changes discussed in IRC with mento

Comment 12

[Simon Fraser [no longer active]]

Hm. I applied the patch, but "Show" is disabled for a download that still exists on disk.

Comment 13

[Simon Fraser [no longer active]]

Yeah, this patch doesn't work for new downloads.

Comment 14

[Nick Kreeger]

Simon,
By "new" do you mean downloads that are currently running?

Comment 15

[Nick Kreeger]

Attachment: Fixes subscription prob, Enables Reveal on active downloads

The problem was that new downloads sometimes don't exist when |setupFileSubscription| is called, so add a flag to set it the file to be subscribed is coming from either a new download (YES) or a dictionary (NO)

Comment 16

[Simon Fraser [no longer active]]

Attachment: Patch

This patch simplifies the file checking stuff quite a bit, as well as doing some other bug fixes and cleanup:
* avoid loading the nib twice via -initWithDictionary:
* fix a memory leak in -loadProgressViewControllers
* clean up a bunch of ProgressDlgController methods

The d/l view maintenance is complicated by the fact that there are 2 views, with the "current view" depending on whether you are active or stopped. It might be simpler to just keep both views in sync; it's too easy to have bugs where a view isn't updated before being shown, so shows the string values from the nib.

The other odd thing is that when a file is deleted, we don't clear the icon. We need the "missing icon" image (isn't there a bug for that?).

Comment 17

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

(In reply to comment #16)
> We need the "missing icon" image (isn't there a bug for that?).

It's the bug listed in the blocking field :-)

Comment 18

[Nick Kreeger]

Patch looks good, I was wondering one day if we should switch to enums, glad we did. I only noticed a couple of items:

Are not going to track downloads that get loaded from the plist? And if not, do we want a default icon, or just leave the view blank? If we are, we are not doing so in this patch. The ProgressView loaded from a completed download looks kinda funny with out a icon for it.

Either way, we probly need to update the comment in |initWithDictionary|:

 -(id)initWithDictionary:(NSDictionary*)aDict
 {
-  if ((self = [super init]))
+  if ((self = [self init]))
   {
-    [NSBundle loadNibNamed:@"ProgressView" owner:self];
-    [self viewDidLoad];
     [self setProgressViewFromDictionary:aDict];
+    // since we are creating a progressview from a dict, the file
+    // will not be restarted, so setup file system notifications here
   }
   
   return self;
-  
 }
 
The bug that has the missing download icon is: bug 312499.

Comment 19

[Nick Kreeger]

One other item, do we still want to enable "reveal:" for downloads that are currently running? If so we need to modify ProgressDlgController.

Comment 20

[Nick Kreeger]

Attachment: screen shot of loaded missing icons

Comment 21

[Nick Kreeger]

Hmmm. There still seems to be some bugs concerning a file where you file->download link target as action. If you save a download in a distinct folder where you have not saved a download, the show/reveal/icon actions are strange. Say you save a download to a location where you have not saved before, then look at the download manager, you can see no icon, reveal and open are disabled (well i was testing with an updated patch that allows reveal to work with active downloads). When you then start another download while allowing the current one to run, all of sudden the icon is loaded properly and reveal is now enabled.

I'm busy until tonight, but I will update the patch then.

Comment 22

[Nick Kreeger]

Attachment: Updated to include changes

Just posting this here that contains work that is waiting to be checked in, just here to show progress. This also contains a setup to mRefreshIcon in |init| to make sure we refresh that icon at least once.

This patch works for me, I have tested it out every way could think of and so far it works for me. (Granted i'm not the best tester monkey). 

I'm also attaching the missing file icon that I have used.

Comment 23

[Nick Kreeger]

Attachment: Missing file icon from bug 312499

Missing file icon, don't know if this is the actual name of the icon that we want to use, but it's the name that is used in the latest updated patch (202851)

Comment 24

[Simon Fraser [no longer active]]

I checked in a bunch of changes and cleanup incorporating the patch here, and the new icon. Thanks Nick!

Comment 25

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

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

Clearing review request on fixed bug.  Down the road it *might* be useful to know which of these patches were obsolete and which were checked-in....