308942 - Add options to remove finished download list items automatically (clearing completed downloads)

Status: VERIFIED FIXED

(Camino Graveyard :: Preferences, enhancement)

Description

[Eiichi]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20050915 Camino/1.0+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20050915 Camino/1.0+

Now Bug 187619 has been fixed, please add option to remove download list items.

Reproducible: Always

Steps to Reproduce:

Comment 1

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

That's what the "Clean Up" icon does.  There's also another bug out there to add
a "Clean only missing items" icon, too.

Comment 2

[Eiichi]

(In reply to comment #1)

I'm sorry I didn't give an appropriate explanation, and change Summary
correspondingly.

Safari has an option "Remove download list items" in Preferences.
1. Manually
2. When Safari Quits
3. Uopn Successful Download

Comment 3

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

Ah, ok.  Considering the issues with removing large numbers of saved downloads,
this sounds like something worth considering.

Comment 4

[Nick Kreeger]

Taking

Comment 5

[Nick Kreeger]

Attachment: Proposed patch

Here is a proposed patch, it adds a new IBOutlet to the Downloads pref pane, and  a new .js pref to go with it. 

When the pref is set to true, and |downloadDidEnd| in ProgressViewController is executed, we check the pref, then remove the download depending on the pref state.

Mento, Pink, Simon, and/or Stuart care to take a look?

Comment 6

[Nick Kreeger]

Attachment: Updated Downloads.nib

Here is the updated pref pane.

Comment 7

[Nick Kreeger]

Attachment: Updated Patch

Catches typo in ProgressViewController, using the wrong pref in the new method, patch fixes this.

Comment 8

[Bruce Davidson]

Comment on attachment 222436 [details] [diff] [review]
Updated Patch

Patch works fine and code looks good.

However I think the default for this option should be false rather than true. I initially thought this was a great idea, but actually watching a file appear and disappear in the download manager is weird; novice users would have no way to open a downloaded file other than manually locating it in the finder.

Nit: opening braces should follow the dominant "at end of condition line" style rather than being on a line by themselves. With that nit fixed and default=false r+=me.

Comment 9

[Jasper]

I agree that this feature should be disabled by default as it's destructive by nature. "Raison de etre" for the download manager list is so that users can find previous downloads.

Comment 10

[Stuart Morgan]

Comment on attachment 222436 [details] [diff] [review]
Updated Patch

>+  if (!mUserCancelled && !mDownloadingError) 
>+  {
>+    if ([[PreferenceManager sharedInstance] getBooleanPref:"browser.download.autoRemoveDownload" withSuccess:NULL])
>+    {
>+      [mProgressWindowController removeDownload:self];
>+    }
>+  }

Why the double if instead of just using another && ?

Comment 11

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

If we're not actually using common Gecko/XPFE/tookit prefs with matching values, beahviors, and so forth, shouldn't we be naming them camino.download.* instead?  Or is browser.* open to all types of prefs for all apps that are browsers?

And yes, it should be off by default ;)

Comment 12

[Stuart Morgan]

(In reply to comment #11)
> If we're not actually using common Gecko/XPFE/tookit prefs with matching
> values, beahviors, and so forth, shouldn't we be naming them camino.download.*
> instead?

browser.download.autoDownload and browser.download.autoDispatch are both nonstandard, and already there.  Changing them at this point would be a pain, and doing something different for this pref probably doesn't make much sense.

Comment 13

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

The former is an XPFE pref (according to LXR), although the latter appears to be wholly our invention (or was historically XPFE but has been removed).  No argument about the pain of changing; it was more a question of what is proper pref-naming protocol.

Comment 14

[Nick Kreeger]

Attachment: Updated patch

Here is an updated patch, it sets the pref to false to start with, and combines the two if statements into one.

As to the nit, I think code is much easier to read when the braces are on blank lines. I know that there is some different opinions here, do we have a official style guidline?

Comment 15

[Nick Kreeger]

Comment on attachment 222808 [details] [diff] [review]
Updated patch

Stuart can you take a look?

Comment 16

[Bruce Davidson]

I agree with Nick that braces on their own line are far easier, but the style guideline (http://www.mozilla.org/hacking/mozilla-style-guide.html#Visual) doesn't. I'm sure I've been corrected on it before in Camino patches, though can't find the comments right now (and the code in that file isn't totally consistent, though most are same-line style).

Comment 17

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

We talked about this a little on irc and there was some agreement that we should copy Safari entirely (comment 2), as the one option we're implementing here is somewhat disconcerting for people who keep the download manager open (though it makes some sense for people who have it auto-close).

Comment 18

[Nick Kreeger]

Attachment: Updated patch to make the removal options tri-state

Here is an updated patch that gives the user the tri-state download list item removal option like Safari.

This patch will require the new Downloads.nib

Comment 19

[Nick Kreeger]

Attachment: Updated Downloads.nib

Contains the new nib for the Downloads pref pane. Included is a NSPopUpButton simular to the one that Safari uses for their tri-state option.

Comment 20

[Nick Kreeger]

Attachment: Updated to address BruceD's comments via IRC

This is an updated patch to address some comments from BruceD via IRC.

We moved the pref const values to ProgressViewController.h so they are defined in the same place. Also I added some more comments to |removeDownloadListIfAppropriate:| to explain why we need to have that method return a boolean value.

Comment 21

[Bruce Davidson]

Comment on attachment 223051 [details] [diff] [review]
Updated to address BruceD's comments via IRC

On testing this patch I'm finding that the pref value gets saved okay (by examining prefs.js). However every time I restart Camino and look at the download prefs the option shown is "Manually" rather than whatever option was set when I last quit Camino (and which was saved to prefs.js).

Also two very small nits:

In the comment on RemoveDownloadList: The pref isn't a boolean, so remove "to true". Also its not very clear that you're talking about the return value. Personally I prefer big bold comments about return values, e.g.
// Return YES if the plist is deleted (to avoid double checking the pref in
// |allowTerminate:|)


In removeFromDownloadListIfAppropriate: indent the dependent statement of the if two rather than four spaces.

Comment 22

[Nick Kreeger]

Attachment: Updated patch

Fix the problem with the popupbutton not selecting the correct pref first by selecting the value in |mainViewDidLoad:|

Comment 23

[Nick Kreeger]

Attachment: Oops catch nits

Sorry brain-fart and I forgot to cover Bruce's nits.

Comment 24

[Nick Kreeger]

Attachment: Oops part2

I updated the comment in ProgressViewController (which probably needed it as well), this one updates the one Bruce mentions.

Comment 25

[Nick Kreeger]

Attachment: Cleaner patch

Forgot to clean up the patch before posting. Sorry. Again.

Comment 26

[Bruce Davidson]

Comment on attachment 223388 [details] [diff] [review]
Cleaner patch

r+=me - thanks Nick for churning through the nits.

Comment 27

[Nick Kreeger]

Attachment: Screen shot of the nib

Comment 28

[Stuart Morgan]

Comment on attachment 222926 [details]
Updated Downloads.nib

Items and Quits shouldn't be capitalized.

Comment 29

[Stuart Morgan]

Comment on attachment 223388 [details] [diff] [review]
Cleaner patch

>+  IBOutlet NSPopUpButton* mRemoveDownloadListOption;
...
>+- (IBAction)chooseRemoveDownloadListOption:(id)sender;

I don't like the name RemoveDownloadListOption--the download list isn't being removed. mDownloadClearingPolicyOptions and chooseDownloadClearingPolicy, maybe?

>+// Set the download removal policy

Or maybe DownloadRemovalPolicy, since that's better :)

>+-(BOOL)removeDownloadListIfAppropriate;

clearDownloadListIfAppropriate. However:

>+      // If the pref is not set to remove items when Camino quits,
>+      // save the downloads here because the downloads that were 
>+      // downloading are not cancelled, just terminated.
>+      if (![self removeDownloadListIfAppropriate])
>+        [self saveProgressViewControllers];
>+

Given that it will be only called from this one method anyway, and the bool return is awkward to the point that it needed significant comments at both the call site and the definition, I think the method should be folded into this one.  The logic of allowTerminate will need some rearranging, but since it has early returns that's probably a good thing; perhaps something like:

BOOL shouldTerminate = YES
BOOL downloadsInProgress = <blah>
if (downloadsInProgress) {
  <do all the prompting>
  if (pressed cancel) {
    ...
    shouldTerminate = NO
  }
}
if (shouldTerminate) {
  if (<download policy pref> set to clear things)
    <clear things>
  else if (downloadsInProgress)
    <save things>
}
return shouldTerminate ? NSTerminateNow : NSTerminateCancel;
  

>+const int kRemoveDownloadsOnQuitPrefValue = 1;
>+const int kRemoveUponSuccessfulDownloadPrefValue = 2;

Maybe a comment here that 0 == manual

Comment 30

[Stuart Morgan]

Oops, forgot the most important part--as mentioned on IRC, the pref for clearing on completion can in some cases cause visual artifacts (an illusory almost-done download) when the list is otherwise empty.

Comment 31

[Nick Kreeger]

Attachment: Update Patch

Here is an updated patch, the reason why the download would get "stuck" in the view is because we were not calling |[self rebuildViews]| in |removeDownload| in ProgressDlgController.

I tweaked |allowTerminate| per Stuart's comments as well.

This will require a new nib...

Comment 32

[Nick Kreeger]

Attachment: Updated Downloads.nib

Here is the updated Downloads.nib, it is required since some naming was changed in Downloads.h/.m 

Stuart can you take a look at both of these items when you get a chance?

Comment 33

[Nick Kreeger]

Attachment: Updated Patch

Oops, I forgot to remove a stale comment.

Comment 34

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

Comment on attachment 223657 [details]
Updated Downloads.nib

r- on the nib.

Issues:
1) the i in "Items" needs to be lower-case
2) the q in "Quits" needs to be lower-case
3) The newly-added menu is 1px too far to the right and is 2px too high
4) The baselines of the menu and its descriptive text need to line up (they don't now, and won't when you move the menu down 2px)...enable the Layout rectangles to help you line them up
5) The new menu is not in the keyboard loop when you enable FKA...it just skips from the location menu to "close" checkbox.

Since you're touching the nib already, if you can fix bug 327203 for this prefPane, too (not sure if it's a nib issue or a code issue though).

Comment 35

[Nick Kreeger]

Attachment: Updated patch

Updated patch address's some comments from hwaara.

Comment 36

[Stuart Morgan]

Comment on attachment 224600 [details] [diff] [review]
Updated patch

r=me

Comment 37

[Stuart Morgan]

Comment on attachment 223657 [details]
Updated Downloads.nib

Same issues as raised above (except that the menu is actual 2 pixels too far to the right, and the one above it 1 pixel too far (using the checkboxes as the baseline)).

Let's not bring other bugs into this though, especially ones that affect all of the pref panes.

Comment 38

[Nick Kreeger]

Attachment: Fixed NIB (non-AHIG)

Here is an updated NIB that should address all the comments from reviewers

Comment 39

[Nick Kreeger]

Attachment: Another nib (AHIG-compliant)

This is a nib that smokey and myself worked on to match the AHIG dimensions. Screenshot coming.

Comment 40

[Nick Kreeger]

Attachment: Screen shot of the new nib

Here is the nib as it meets AHIG requirements

Comment 41

[Nick Kreeger]

Attachment: Another SS

Here is a screenshot that fixes the spacing at the bottom, and spaces 10px between the comboboxes and the checkboxes rather than 8px.

Comment 42

[Nick Kreeger]

Attachment: Nib from "Another SS"

This is the NIB from screen shot 2, from discussion in #camino, we seem to like it the best.

Comment 43

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

Comment on attachment 224656 [details]
Fixed NIB (non-AHIG)

This nib fixes all the issues I raised before and still looks like the rest of the Camino prefPanes (non-AHIG-compliant); see additional comments...

Comment 44

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

OK, all of the last 3 nibs (non-obsolete ones) have r=me for the layout issues I mentioned in my review from comment 34.

We need some sr guidance here, though, on the spacing.  Chris and I noticed, in the home page bug, that our nibs are 1) wildly inconsistent and 2) out-of-wack with the AHIG spacing rules at http://tinyurl.com/e6dfe

The first nib "Fixed NIB (non-AHIG)" essentially looks like all the other nibs do right now, with wide spacing.  The second "Another nib (AHIG-compliant)" is strictly AHIG spacing-compliant, and the 3rd nib (Nib from "Another SS") adds 2 extra px between the last drop-down and the first checkbox to make that less cramped.

If we take the AHIG-compliant nib (or even the one with the 2px extra), we need to fix all the other prefPanes soon because the AHIG-compliant one will stick out like a sore thumb, since our current spacing is much larger than AHIG.  If we don't take the AHIG-compliant one, we need to establish some guidelines of our own to sync all our prefPanes and at least make things internally consistent.

To me stock AHIG tends to make spacing between certain controls too cramped-looking (8px is fine for checkboxes and radio buttons, but tight with the larger controls and cramped when moving between a large control and a smaller one)....

Mike, Simon, Mark, Japser, please come to a conclusion on what we should do ;)

Comment 45

[Mike Pinkerton (not reading bugmail)]

can i get some screenshots of the three? would make it a lot faster to comment.

Comment 46

[Nick Kreeger]

Attachment: Option 1 Screen Shot

Too keep this simple, I will post the 3 options.

Comment 47

[Nick Kreeger]

Attachment: Option 2 Screen Shot

Option 2.

Comment 48

[Nick Kreeger]

Attachment: Option 3 Screen Shot

Option 3.

Comment 49

[Mike Pinkerton (not reading bugmail)]

what do apple apps do in their prefs, like safari or iphoto? do they respect the alignment guides or not?

Comment 50

[Nick Kreeger]

Attachment: Safari Pref Screen Shot

Here is a screen shot from a safari pref pane.

Comment 51

[Nick Kreeger]

(In reply to comment #49)
> what do apple apps do in their prefs, like safari or iphoto? do they respect
> the alignment guides or not?
> 

They seem to be inconsistent to me, Smokey what do you think?

Comment 52

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

I looked carefully at a couple of prefPanes in Safari 1.3.foo (General and Security) and the spacing seemed to vary anywhere between 6px and 10px between controls, and then extended spaces to separate "groups" (which the AHIG doesn't mention there at all, only talking about using separator lines and the spacing on either side of them).  They aren't consistent on the top margin, either :P

A cursory look at Mail and iPhoto 6 seemed to reveal similar variety in adherence  (or lack thereof) to the rules :/ 

Comment 53

[Nick Kreeger]

Attachment: Proposed Nib Screen Shot

Comment 54

[Nick Kreeger]

Attachment: Proposal Deuce

Here is another, instead of 10px spaceing between the checkboxes and the popups, i changed it to 15px.

Comment 55

[Mike Pinkerton (not reading bugmail)]

i like Proposal Deuce

+    // Check the download item removal policy here to see if downloads should be removed when camino quits.
+    if ([self shouldRemoveDownloadsOnQuit])
+      [self removeSavedDownloadPlist];

why are we doing work to remove things in allowTerminate? Shouldn't we wait until we actually are shutting down?

looks good otherwise.

Comment 56

[Nick Kreeger]

Attachment: Updated Patch

Here is an updated patch to address Mike's catch.

I added the method |applicationWillTerminate:| to ProgressDlgController, and moved the save/delete action out of |allowTerminate:| to the new method. In MainController I added a call to our new method in ProgressDlgController so that we only save or delete when we know for sure that we are about to terminate.

Comment 57

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 224868 [details] [diff] [review]
Updated Patch

looks good. did  you test everything to ensure that moving it didn't affect saving the downloads between restarts either?

sr=pink

Comment 58

[Nick Kreeger]

Pink,

Yes I tested everything, save/delete works in its new location just as it did in the old location.

Comment 59

[Nick Kreeger]

Fixed with check-in, branch (1.8) and trunk.

Comment 60

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

It's fixed1.8.1 for the 1.8(.1) branch these days ;)

Also, kreeger, will you please document the various margins and spacings from the final prefPane on the wiki somewhere?  Thanks!

Comment 61

[Eiichi]

kreeger, thanks a lot for your effort.
Thanks again.

Comment 62

[[e-mail]]

Verified on 1.8 branch and trunk