User-Agent: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7a) Gecko/20040106 Camino/0.7+ Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7a) Gecko/20040106 Camino/0.7+ New download manager (see bug #223583) is fine. However, there are two features that I would enjoy: - a move to trash button. It would also remove the file from the list. I often open first and then go back to Camino, show in finder, command-delete and empty trash. - a way to only remove files that have disappeared. I want some downloads to stay in the manager until I have handled them and this would prevent me to remove them from the list one by one (not sure I would use this if I had a move to trash button, though) - a way to *drag* a file from the download manager (once it's downloaded, of course) so I could either drag it to some folder or drag it to some application in the Dock (for example). Opening isn't always what I want. Reproducible: Always Steps to Reproduce: 1. 2. 3.

Not for 0.9 I don't think. Lots of room for error in terms what users expect things to do.

I'm going to look at getting the "move to trash" part of this bug implemented. I'm not going to put the button in the default toolbar for the download manager, but people can add it. I think many of us would find this a significant improvement in our workflow of dealing with downloads. I'm not going to look at tackling the drag and drop aspects of this bug, they should probably be moved to a separate bug. I'm also not going to look at the second part of this bug. Whilst I understand the reporter's intent I'd hate to have to describe it in an icon and one word on the toolbar! Josh, which bit of the bug did you see as having the potential for confusion that you noted?

here you go Bruce

Okay, I have the first part (adding a "move to trash" icon to the d/l manager) up and running. The icon isn't in the default set, so you'll need to customise your toolbar to see it. (Jasper's icon is great - worth customising the toolbar just to see it!) Patches coming up shortly. Discussion on the IRC channel is leaning towards not implementing the second part of this feature request. We can't see many people using it (especially once the first bit is implemented), and its not clear how to present it to the user without introducing the potential for confusion. Drag and drop is also a separate (and non-trivial) problem, which I'm going to move to a separate bug.

I've separated the dragging and dropping aspects into bug 276883, and updated the summary of this bug. Also bringing forward the target milestone for this bug, because I think we can get the simple "delete files" icon in earlier than "future".

Propsed patch. This patch also changes the icon used for the cancel download toolbar button, as discussed in bug 230263, to use the same icon as the "Stop" on the main toolbar. I haven't deleted the old image out of the NIB though, and we can either accept this as part of this patch, or back that bit out and do it as a separate checkin. Requesting review from Josh.

As per your request, I'm posting a comment to remind you to change the text on the toolbar button to "Move to Trash" (or something indicating that) and to change the action of the button to move the file to the trash instead of deleting, so you remember when you get back.

More Cocoa Fun: [[NSWorkSpace sharedWorkspace] performFileOperation:NSWorkspaceRecycleOperation source:someDir destination:@"" files:arrayOfFilesRelativeToSomeDir tag:somePointerIfYouWant]; and [someString sringByDeletingLastPathComponent]; [someString lastPathComponent]; as extra incentive points. ;)

This updated patch moves the file to trash rather than deleting it. Following discussion on IRC I've left the toolbar text alone.

Is there any reason not to put this in 0.9?

Comment on attachment 171660 [details] Updated patch addressing review comments from wevah The patch has bit rotted however. New patch coming up later today.

This is not an 0.9 blocker. Bugs do not have to be on the blocker list in order to get fixed for 0.9. The blocker list is for bugs that we will not ship without fixing.

Revised patch to address bit rot in previous patch. Applies cleanly against latest CVS. Requesting review with a view to getting this into 0.9.

Comment on attachment 179713 [details] Revised patch 1) Could you split the bug in patch and .nib+images attachments ? 2) I can't see the new trash icon, it does not show up, could you post detailled instructions on how to get it visible ? 3) When one deletes a downloaded file from the finder and then tries to delete it from camino, , camino just beeps and does no remove the file entry from the list (I think it should).

Updated patch that addresses Ludo's comment about deleting files that have already been deleted via the finder. To get the image to show up in the toolbar I take the following steps: 1. Copy the image (dl_trash.tif) from this bug into <caminoBuildPlace>/mozilla/camino/resources/images/toolbar. 2. In XCode double click AccessoryViews.nib (the outer one in the tree view) to open it. 3. In InterfaceBuilder select the images pane of AccessoryViews.nib 4. Drag the dl_trash.tif file from where you placed it in step 1 onto the images tab. 5. When the sheet appears asking you to which targets the file should be added, select Camino and CaminoStatic. (These should be the two ticked by default). 6. Save and close the nib file. 7. XCode should have resave the project file automatically. 8. Click "Build" in XCode. That's worked for me every time. Any time that I get a fresh .xcode file down I have to repeat these steps (apart from step 1, obviously) - so it seems to matter that the file exists both in the nib and the project file. If you have any problems please shout via IRC or e-mail. I'll attach a tar.gz of the latest nib and the image itself in a moment. Requesting review on this patch.

Comment on attachment 181805 [details] [diff] [review] Updated patch with Ludo's comments address (NIB & image to follow) >+ else if ([itemIdentifier isEqualToString:@"movetotrashbutton"]) { I added the following code between those two lines : NSLog(@"Yo !"); >+ [theItem setToolTip:NSLocalizedString(@"dlTrashButtonTooltip", @"Move selected download(s) to Trash")]; >+ [theItem setLabel:NSLocalizedString(@"dlTrashButtonLabel", @"Delete File(s)")]; >+ [theItem setPaletteLabel:NSLocalizedString(@"dlTrashButtonLabel", @"Delete File(s)")]; >+ [theItem setAction:@selector(deleteDownloads:)]; >+ [theItem setImage:[NSImage imageNamed:@"dl_trash.tif"]]; >+ } I've followed the steps described in the "how-to do it comment", the image is now part of the .nib file. However when I run a cleaned build I : 1) never see the trash icon. 2) Don't see any "Yo !" messages in my logs. Bruce is the patch complete ?

Comment on attachment 181805 [details] [diff] [review] Updated patch with Ludo's comments address (NIB & image to follow) IMHO the trash button should be visible by default.

Screenshot. Obviously the arrangement of the toolbar buttons depends on how you customise your toolbar.

Images don't belong in nib files; IB just caches them there locally on your machine. The images should be added to the project.

Revised patch addresses comments raised by smfr on IRC: - Bullet delete method against being called with nothing selected - Fix tabs that had crept in

Simon, have you had a chance to review this? Maybe wait until after the pause/resume changes have gone in. CCing kreeger.

Revised patch following the landing of the pause and resume stuff. Mainly identical to previous patch, but changed getting of localised strings to pass nil as the default text rather than an English string to match the rest of the code.

Updated localized strings file to go with latest patch (as diff seems to be convinced these files are binary). Apart from this you will need to add the image Jasper attached to this bug into your project in xcode.

This patch prevents the user accidently clicking on the delete button if the D/L manager isn't the top window. Based on Nick's patch yesterday for the other buttons.

Now that patch has disabled clickthrough update in, the patch looks ok to me.

Can we get a revised patch to the trunk?

Comment on attachment 191819 [details] [diff] [review] Patch with click-through disabled This patch is against the latest trunk code (unaffected by all the bookmark checkins made late yesterday).

This patch (against latest CVS) factors out the common code in remove: and delete: into a separate method.

Comment on attachment 191917 [details] [diff] [review] Revised patch >Index: src/download/ProgressDlgController.mm >=================================================================== >+// >+// Take care of selecting a download instance to replace the selection being removed >+// >+-(void)getNewSelectionToReplaceSelection:(NSMutableArray*) selectionToRemove This should be renamed to "setSelectionForRemovalOfItems:(NSArray*)doomedItems and the param should be an NSArray; no need for it to be mutable. >+ unsigned int selectedCount = [selectionToRemove count]; >+ if (selectedCount > 0) { >+ unsigned indexOfLastSelection = [mProgressViewControllers indexOfObject:[selectionToRemove objectAtIndex:(((int)selectedCount) - 1)]]; All this swapping between indices and ProgressViewControllers is clumsy and inefficient. Might it not be easier to have the param be an array of NSNumber indices? >+ // if dl instance after last selection exists, select it or look for something else to select >+ if ((indexOfLastSelection + 1) < [mProgressViewControllers count]) { >+ [[((ProgressViewController*)[mProgressViewControllers objectAtIndex:(indexOfLastSelection + 1)]) view] setSelected:YES]; A little helper method - (ProgressViewController*)progressViewControllerAtIndex:(unsigned)inIndex would help a lot here. >+ } >+ else { // find the first unselected DL instance before the last one marked for removal and select it >+ // use an int in the loop, not unsigned because we might make it negative >+ for (int i = ([mProgressViewControllers count] - 1); i >= 0; i--) { >+ if (![((ProgressViewController*)[mProgressViewControllers objectAtIndex:i]) isSelected]) { > [[((ProgressViewController*)[mProgressViewControllers objectAtIndex:i]) view] setSelected:YES]; > break; Why do we have an -isSelected method on ProgressViewController, but no -setSelected: method? That should be fixed. >+// remove all selected instances, don't remove anything that is active as a guard against bad things >+-(IBAction)remove:(id)sender >+{ >+ NSMutableArray* selected = [self getSelectedProgressViewControllers]; getSelectedProgressViewControllers should return an NSArray*, not a mutable array.

This revised patch addresses all bar one of the review comments (see below), and cleans up the rest of the code to use the new methods where possible. > All this swapping between indices and ProgressViewControllers > is clumsy and inefficient. > Might it not be easier to have the param be an array of > NSNumber indices? All other callers of getSelectedProgressViewControllers use the fact that they get an array of PVCs. I think the slight untidyness in this one method is better than changing all the other places to do an extra [self getPVCAtIndex] call to get the underlying PVC each time; however I'm willing to change it if preferred.

Comment on attachment 193345 [details] [diff] [review] Patch addressing review comments sr=pink

+ NSString* fileName = [mDestPath lastPathComponent]; + NSString* path = [mDestPath stringByDeletingLastPathComponent]; + [[NSWorkspace sharedWorkspace] performFileOperation:NSWorkspaceRecycleOperation + source:path + destination:@"" + files:[NSArray arrayWithObject:fileName] + tag:nil]; is there anything we should do here to ensure that we don't end up deleting the directory by accident? I remember that was a problem with a safari beta where their d/l cleanup code was deleting the parent directory (which was the home directory) in some cases. should we be double-extra safe and first check that the file we're about to delete is a file (not a directory) and exists?

This patch no longer applies. We need a new one.

This is basically the same patch, updated to apply cleanly against the latest trunk code. The only change is to do the paranoia bullet proofing suggested by Mike in comment 33. Requesting sr= and landing to trunk.

An updated localized strings file (why oh why isn't this cvs diffable!?)

Updated patch that initialises isDir variable to YES, based on comments from Mike in IRC.

Comment on attachment 196642 [details] [diff] [review] Updated patch sr=pink

I was about to commit this, but it doesn't play nicely with the new inter-session download remembering stuff. Bruce and/or Nick, could you take a look?

Attachment 197581 [details] didn't include dl_trash.tif in the CaminoStatic target. (And it was an old-style diff, eew!) This is better.

Attachment 170143 [details] had all sorts of embedded metadata, including RDF crap and Jasper's page setup settings. This is a more optimal TIFF, after a run through GraphicConverter.

Several issues have accured when I have patched and tested the latest patch on the freshest source. I concur with Mark's comments, and also there is speratic behavior when deleting an item, it increments the current selection to n+1. Also if the file gets moved the trash button is still enabled. It seems that the file system figures out where this file is and trashes it? (At least from my expirementation, but i could be wrong getting kinda late here). I would really recommend the completion of file system notifications before this patch to prevent any future headaches and bugs to come with moving files and trying to delete them and etc. For now, I am updating Bruce's patch to see if I can help eliminate some of the behavior issues seen by Mark and myself.

Ok after some more testing it appears that the muliple selection problem occurs on a progress view that was loaded on launch, the download had completed sucessfully, and the associated file was moved. Also on a progress view that displays this state, the 'trash' button is enabled. Maybe an easy fix to this is to check the file's location (as in bug 307753, file system notifications), and set the buttons status through a file exists flag.

(In reply to comment #42) > This is a more optimal TIFF, after a run through GraphicConverter. I've been using tiffutil -cat to shrink PhotoShop-heavy tiffs.

Comment on attachment 196417 [details] Updated localized string files Checkin from bug 299758 obsoletes this Localizable.strings file.

This fixes the selection problem since the landing of the persistent downloads patch. The call in ProgressViewController to |deleteDownload:| called |remove:| in ProgressViewController. Since the selection controlling is all done in ProgressDlgController in |setSelectionForRemovalOfItems:|, the selection for removal is already set when called in |deleteDownloads:|, so just call |removeDownload:| in |deletedDownloads:| in ProgressDlgController. I also created a new method in ProgressDlgController called |shouldAllowMoveToTrashAction:|, this is used to validate the toolbar and context-menu functions. Since this call protects us, I removed the FileManager call to deleted the download in |deleteDownload:| in ProgressViewController. Easy huh? Note that this patch includes the patch for file system notifications for tracking downloads. We need this so we can validate the trash function in the toolbar and context-menu.

This will block 1.0, per IRC conversation.

If this does land (hopefully) before whatever we are calling the 1.0 b/rc release, we need an update localizable.strings and xcode project file, and any other misc item I forgot to mention.

Comment on attachment 199670 [details] [diff] [review] Updated patch, now removal of ProgressViews works with persistent downloads. Not sure whether you wanted this reviewed yet or not Nick. Looks good though. One minor nit: if we don't want to do the "is the file we're about to delete definitely a file not a directory" check, remove the code altogether rather than leaving it commented out. With that nit r+=me, and suggest its ready for Simon or Mike to look at. I'll upload an up-to-date resource file too.

Comment on attachment 199670 [details] [diff] [review] Updated patch, now removal of ProgressViews works with persistent downloads. Unfortunatly again this patch has big rotten, i'll post an update today

Patch against the latest trunk source code. Note that my e-mail address for bugzilla stuff has changed (I've resigned from my job at IPL!)

Updated patch. The previous one had some cruft from before Nick's file system changes landed.

Updated patch, the last one did not apply cleanly. Just one thing: +// Delete the file we downloaded and remove this item from the list +-(IBAction)deleteFile:(id)sender +{ + NSString* fileName = [mDestPath lastPathComponent]; + NSString* path = [mDestPath stringByDeletingLastPathComponent]; + + [[NSWorkspace sharedWorkspace] performFileOperation:NSWorkspaceRecycleOperation + source:path + destination:@"" + files:[NSArray arrayWithObject:fileName] + tag:nil]; +} + You should probably ensure that fileName and path were set correctly before passing off the delete operation. Also, more of feature question, should we play the "clunk" sound that plays when you drop a file from the finder to the trash when someone perfoms the delete operation? r=me

I was thinking of either showing the Poof animation or the cluck sound from the trash would indeed be a good addition to ensure that users get feedback on what is happening.

It's probably too late to take a change this big. Rerequesting blocking for 1.0.

This isn't that big of a patch no?

Comment on attachment 207909 [details] [diff] [review] Cleaner patch Requesting sr= from Mike (or Simon if he has more time).

Pushing to 1.1... Let's get this early on though. (In reply to comment #59) > This isn't that big of a patch no? It's more about the testing required to land this. We'd have to be sure there was no destruction done. "Delete" keys are scary. ;)

+ // now remove stuff, don't need to check if active, the toolbar/menu validates + for (unsigned int i = 0; i < selectedCount; i++) { + [[selected objectAtIndex:i] deleteFile:sender]; + [self removeDownload:[selected objectAtIndex:i]]; + } won't this change the indices while you're iterating? looks ok otherwise for landing on the trunk and 1.8branch.

Comment on attachment 207909 [details] [diff] [review] Cleaner patch sr=pink after talking with BruceD on irc

Dear Mark, please check this in for Bruce soon.

Dear Mark, I agree. Let's bury this one before Localizable.strings changes again. Fixed on the trunk and MOZILLA_1_8_BRANCH. I don't like "Delete File(s)" for dlTrashButtonLabel. What was wrong with "Move to Trash" like dlTrashCMLabel?

Thanks Bruce and everyone else. It's a great addition to Camino. I love it.

*** Bug 335487 has been marked as a duplicate of this bug. ***