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

RESOLVED FIXED in Camino1.0

Status

P2
major
RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: nick.kreeger, Assigned: nick.kreeger)

Tracking

({fixed1.8})

unspecified
Camino1.0
PowerPC
macOS
fixed1.8
Bug Flags:
camino1.0 +

Details

Attachments

(5 attachments, 6 obsolete attachments)

(Assignee)

Description

14 years ago
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.
Priority: -- → P2
Target Milestone: --- → Camino1.0
(Assignee)

Comment 1

14 years ago
Posted patch Propesed patch (obsolete) — Splinter Review
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)
(Assignee)

Comment 2

14 years ago
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
Attachment #195543 - Attachment is obsolete: true
(Assignee)

Comment 3

14 years ago
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.
Attachment #195829 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Summary: Downloads that get moved from download folder still show "reveal in finder" and "open" → Downloads that get moved still enable "reveal in finder" and "open", implement file system notifications
(Assignee)

Comment 4

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

14 years ago
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!
Attachment #198059 - Flags: review-
(Assignee)

Comment 6

14 years ago
Updated patch to meet smfr's comments
Attachment #198059 - Attachment is obsolete: true

Updated

14 years ago
Blocks: 312499
This will block 1.0, per IRC conversation.
Flags: camino1.0+
Attachment #198096 - Flags: review?(sfraser_bugs)
(Assignee)

Comment 8

14 years ago
This patch is bit-rotted now, i will post a new one upon the decision of bug 308680
(Assignee)

Comment 9

14 years ago
Posted patch Updated to current source (obsolete) — Splinter Review
Updated to contain changes from bug 308680 and bug 314616
Attachment #198096 - Attachment is obsolete: true
Attachment #198096 - Flags: review?(sfraser_bugs)

Comment 10

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

Comment 11

14 years ago
Attachment #201655 - Attachment is obsolete: true

Comment 12

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

Comment 13

14 years ago
Yeah, this patch doesn't work for new downloads.
(Assignee)

Comment 14

14 years ago
Simon,
By "new" do you mean downloads that are currently running?
(Assignee)

Comment 15

14 years ago
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)
Attachment #201779 - Attachment is obsolete: true

Comment 16

14 years ago
Posted patch PatchSplinter Review
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?).
Attachment #202210 - Flags: review?(nick.kreeger)
(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 :-)
(Assignee)

Comment 18

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

Comment 19

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

Comment 20

14 years ago
(Assignee)

Comment 21

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

Comment 22

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

Comment 23

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

14 years ago
I checked in a bunch of changes and cleanup incorporating the patch here, and the new icon. Thanks Nick!
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Keywords: fixed1.8
Resolution: --- → FIXED
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....
Attachment #202210 - Flags: review?(nick.kreeger)
You need to log in before you can comment on or make changes to this bug.