Closed Bug 230320 Opened 21 years ago Closed 18 years ago

New features for download manager: trash button, clean only disappeared files

Categories

(Camino Graveyard :: Downloading, enhancement, P3)

PowerPC
macOS
enhancement

Tracking

(Not tracked)

RESOLVED FIXED
Camino1.5

People

(Reporter: pguyot, Assigned: mozilla)

References

Details

(Keywords: fixed1.8.1)

Attachments

(5 files, 21 obsolete files)

38.38 KB, image/tiff
Details
2.75 KB, patch
Details | Diff | Splinter Review
2.32 KB, image/tiff
Details
17.19 KB, patch
nick.kreeger
: review+
mikepinkerton
: superreview+
Details | Diff | Splinter Review
31.89 KB, application/octet-stream
Details
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.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Target Milestone: --- → Camino0.9
Not for 0.9 I don't think. Lots of room for error in terms what users expect
things to do.
Target Milestone: Camino0.9 → Future
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?
Assignee: pinkerton → Bruce.Davidson
Status: ASSIGNED → NEW
Attached image icon for move to trash (obsolete) —
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.
Status: NEW → ASSIGNED
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".
Summary: New features for download manager: trash button, clean only disappeared files and dragging → New features for download manager: trash button, clean only disappeared files
Target Milestone: Future → Camino1.0
Attached file Patch (obsolete) —
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.
Attachment #170295 - Flags: review?(joshmoz)
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.
Attachment #170295 - Attachment is obsolete: true
Attachment #171660 - Flags: review?(joshmoz)
Attachment #170295 - Flags: review?(joshmoz)
Attachment #171660 - Flags: review?(qa-mozilla)
Is there any reason not to put this in 0.9?
Flags: camino0.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.
Attachment #171660 - Flags: review?(qa-mozilla)
Attachment #171660 - Flags: review?(joshmoz)
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.
Flags: camino0.9? → camino0.9-
Attached file Revised patch (obsolete) —
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.
Attachment #171660 - Attachment is obsolete: true
Attachment #179713 - Flags: review?(joshmoz)
Attachment #179713 - Flags: review?(qa-mozilla)
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.
Attachment #179713 - Attachment is obsolete: true
Attachment #181805 - Flags: review?(qa-mozilla)
Attachment #181805 - Flags: review?(joshmoz)
Attachment #179713 - Flags: review?(qa-mozilla)
Attachment #179713 - Flags: review?(joshmoz)
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.
Attachment #181805 - Flags: review?(qa-mozilla) → review+
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.
Attached patch Revised patch (obsolete) — Splinter Review
Revised patch addresses comments raised by smfr on IRC:
- Bullet delete method against being called with nothing selected
- Fix tabs that had crept in
Attachment #181805 - Attachment is obsolete: true
Attachment #183845 - Flags: review?(sfraser_bugs)
Priority: -- → P3
Attachment #181805 - Flags: review?(joshmoz)
Simon, have you had a chance to review this? Maybe wait until after the
pause/resume changes have gone in.

CCing kreeger.
Attached patch Revised patch (obsolete) — Splinter Review
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.
Attachment #183845 - Attachment is obsolete: true
Attachment #191817 - Flags: review?(sfraser_bugs)
Attached file Localized strings file (obsolete) —
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.
Attachment #183845 - Flags: review?(sfraser_bugs)
Attachment #181806 - Attachment is obsolete: true
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.
Attachment #191817 - Attachment is obsolete: true
Attachment #191819 - Flags: review?(sfraser_bugs)
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).
Attached patch Revised patch (obsolete) — Splinter Review
This patch (against latest CVS) factors out the common code in remove: and
delete: into a separate method.
Attachment #191819 - Attachment is obsolete: true
Attachment #191917 - Flags: review?(sfraser_bugs)
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.
Attachment #191917 - Flags: review?(sfraser_bugs) → review-
Attached patch Patch addressing review comments (obsolete) — Splinter Review
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.
Attachment #191917 - Attachment is obsolete: true
Attachment #193345 - Flags: review?(sfraser_bugs)
Attachment #191817 - Flags: review?(sfraser_bugs)
Attachment #191819 - Flags: review?(sfraser_bugs)
Attachment #193345 - Flags: review?(sfraser_bugs) → review+
Attachment #193345 - Flags: superreview?(pinkerton)
Comment on attachment 193345 [details] [diff] [review]
Patch addressing review comments

sr=pink
Attachment #193345 - Flags: superreview?(pinkerton) → superreview+
+  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.
Attached patch Updated patch (obsolete) — Splinter Review
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.
Attachment #193345 - Attachment is obsolete: true
Attachment #196416 - Flags: superreview?(pinkerton)
Attached file Updated localized string files (obsolete) —
An updated localized strings file (why oh why isn't this cvs diffable!?)
Attachment #191818 - Attachment is obsolete: true
Attached patch Updated patch (obsolete) — Splinter Review
Updated patch that initialises isDir variable to YES, based on comments from
Mike in IRC.
Attachment #196416 - Attachment is obsolete: true
Attachment #196642 - Flags: superreview?(pinkerton)
Comment on attachment 196642 [details] [diff] [review]
Updated patch

sr=pink
Attachment #196642 - Flags: superreview?(pinkerton) → superreview+
Attachment #196416 - Flags: superreview?(pinkerton)
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?
Attached patch pbxproj diffSplinter Review
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 #197581 - Attachment is obsolete: true
Attached image dl_trash.tif
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.
Attachment #170143 - Attachment is obsolete: true
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.
Attachment #196417 - Attachment is obsolete: true
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.
Attachment #196642 - Attachment is obsolete: true
This will block 1.0, per IRC conversation.
Flags: camino1.0+
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.
Attachment #199670 - Flags: review+
Attached file Up to date Localizable.strings file (obsolete) —
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
Attachment #199670 - Attachment is obsolete: true
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!)
Attachment #207111 - Flags: review?(nick.kreeger)
Attached file Latest localizable strings file (obsolete) —
Attachment #201113 - Attachment is obsolete: true
Attached patch Updated patch (obsolete) — Splinter Review
Updated patch. The previous one had some cruft from before Nick's file system changes landed.
Attachment #207111 - Attachment is obsolete: true
Attachment #207131 - Flags: review?(nick.kreeger)
Attachment #207111 - Flags: review?(nick.kreeger)
Attached patch Cleaner patchSplinter Review
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
Attachment #207131 - Attachment is obsolete: true
Attachment #207909 - Flags: review+
Attachment #207131 - Flags: review?(nick.kreeger)
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.
Flags: camino1.0+ → camino1.0?
This isn't that big of a patch no?
Attachment #207113 - Attachment is obsolete: true
Comment on attachment 207909 [details] [diff] [review]
Cleaner patch

Requesting sr= from Mike (or Simon if he has more time).
Attachment #207909 - Flags: superreview?(mikepinkerton)
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. ;)
Flags: camino1.0? → camino1.0-
Target Milestone: Camino1.0 → Camino1.1
+  // 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
Attachment #207909 - Flags: superreview?(mikepinkerton) → superreview+
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?
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Flags: camino1.1+
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. ***
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: