465242 - Clean up ProgressDlgController.mm

Status: RESOLVED FIXED

(Camino Graveyard :: Downloading, defect)

Description

[Ilya Sherman]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en; rv:1.9.0.4pre) Gecko/2008101818 Camino/2.0a1 (like Firefox/3.0.4pre)
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en; rv:1.9.0.4pre) Gecko/2008101818 Camino/2.0a1 (like Firefox/3.0.4pre)

Unify |remove| and |deleteDownloads|, make selection-update logic more transparent

Make |updateSelectionOfDownload| more robust, i.e. able to handle the case that |selectedDownload| doesn't exist any longer.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Ilya Sherman]

Attachment: Patch to fix comments; some questions in TODOs

Comment 2

[Ilya Sherman]

Attachment: Update to incorporate cl & Wevah's feedback

Comment 3

[Nick Kreeger]

Comment on attachment 348487 [details] [diff] [review]
Update to incorporate cl & Wevah's feedback

-//
-// Take care of selecting a download instance to replace the selection being removed
-//
--(void)setSelectionForRemovalOfItems:(NSArray*) selectionToRemove
-{
-  // take care of selecting a download instance to replace the selection being removed
+// Removes downloads from the view; also deletes corresponding files if |shouldDelete|.
+-(void)removeSelectedDownloads:(BOOL)byDeleting {

Your comment doesn't make sense here, did you mean |byDeleting|. Also, to match the style of the existing code in this function, keep the function's '{' on a new line. Also, if you're going to change the in-param name, I'd go with something like 'aShouldDeleteItems' or something.


+    ProgressViewController* progressController = [selected objectAtIndex:i];
+    if (byDeleting) {
+      // We don't need to check if active; the toolbar/menu validates.
+      // If the file was moved without the controller noticing, the move to trash
+      // will fail, but cause it to notice that the file is missing.  Leave it in
+      // the list (now showing missing) so the user doesn't think it was
+      // successfully trashed.
+      if ([progressController deleteFile])
+        [self removeDownload:progressController suppressRedraw:YES];      
+    }
+    else {
+      if (![progressController isActive])
+        [self removeDownload:progressController suppressRedraw:YES]; 
+    }
+  }

else if (!byDeleting && ![progressController isActive]) {
 ....
}


+  // Select the pivot download.
+  if ([mProgressViewControllers count] != 0) {
+    // Ensure that that pivot index is in the range of the array.
+    mSelectionPivotIndex = MIN(mSelectionPivotIndex, (int)[mProgressViewControllers count] - 1);
+    [(ProgressViewController*)[mProgressViewControllers objectAtIndex:mSelectionPivotIndex] setSelected:YES];
+  } else {
+    mSelectionPivotIndex = -1;
   }

Please follow the style guideline from here:
http://wiki.caminobrowser.org/Development:Coding#Code_Style


+  // TODO is it fine to use |indexOfObjectIdenticalTo| instead of |indexOfObject| here?
+  int indexOfSelectedDownload = (int)[mProgressViewControllers indexOfObjectIdenticalTo:selectedDownload];

Without trying - |indexOfObjectIdenticalTo:| should work since it's just comparing pointers. Please make sure and update/remove that comment if it is true.


While we are here, let's create a helper function that |cleanUpDownloads:| and |clearAllDownloads:| could use for getting an array that already has the elements in revese order (in simular fashion to |selectedProgressViewControllers|).


Good work!
Let's, give this one more spin.

Comment 4

[Ilya Sherman]

(In reply to comment #3)
> (From update of attachment 348487 [details] [diff] [review]DL [details]DM)
> -//
> -// Take care of selecting a download instance to replace the selection being
> removed
> -//
> --(void)setSelectionForRemovalOfItems:(NSArray*) selectionToRemove
> -{
> -  // take care of selecting a download instance to replace the selection being
> removed
> +// Removes downloads from the view; also deletes corresponding files if
> |shouldDelete|.
> +-(void)removeSelectedDownloads:(BOOL)byDeleting {
> 
> Your comment doesn't make sense here, did you mean |byDeleting|.

Oops, good catch.

> Also, if you're going to change the in-param name, I'd go with
> something like 'aShouldDeleteItems' or something.

Not sure what you mean by this.  Are you saying I should change |byDeleting| to |aShouldDeleteItems|?  Other parameters in this file don't seem to have the 'a' prefix --- what's the Camino style convention for this?

> +  // TODO is it fine to use |indexOfObjectIdenticalTo| instead of
> |indexOfObject| here?
> +  int indexOfSelectedDownload = (int)[mProgressViewControllers
> indexOfObjectIdenticalTo:selectedDownload];
> 
> Without trying - |indexOfObjectIdenticalTo:| should work since it's just
> comparing pointers. Please make sure and update/remove that comment if it is
> true.

It works for all of the use cases I tested -- wasn't sure whether there can ever be logically equal copies of ProgressViewControllers made for some reason.

> While we are here, let's create a helper function that |cleanUpDownloads:| and
> |clearAllDownloads:| could use for getting an array that already has the
> elements in revese order (in simular fashion to
> |selectedProgressViewControllers|).

Not sure why this is helpful: we can either make a copy of |mProgressViewControllers|, or iterate through it backward (the problem we want to avoid is that removing a download will shift the indices.  What's the advantage of making a copy and iterating forward over just iterating backward?


(will post updated patch once I've tested it a bit more)

Comment 5

[Stuart Morgan]

(In reply to comment #4)
> Other parameters in this file don't seem to have the 'a'
> prefix --- what's the Camino style convention for this?

We have a fair amount of the 'a' prefix, but I consider that legacy and I'd rather we not introduce any more.

Comment 6

[Ilya Sherman]

Attachment: v2.0, addressing nick's comments

Comment 7

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

I guess we really should confirm this, since there are now two patches and neither Nick nor Stuart have objected to the nature of the cleanup ;)

Comment 8

[Ilya Sherman]

Attachment: v2.1; fix up |removeSuccessfulDownloads|

Avoids enumerating over an array and simultaneously removing elements from it.

Comment 9

[Ilya Sherman]

Attachment: Fix v2.2

Synchronizes pivot with download selection in |didStartDownload|

Comment 10

[Jeff Dlouhy]

Comment on attachment 350036 [details] [diff] [review]
Fix v2.2

26% down, line 107
+    // doesn't think it was successfully trashed.    
+    if ((byDeleting && [progressController deleteFile]) ||
+        (!byDeleting && ![progressController isActive])) {
+      [self removeDownload:progressController suppressRedraw:YES];      
+    }
+  }
+  

Some trailing whitespace here.


55% down, line 184
+      if (mSelectionPivotIndex == i)
+        mSelectionPivotIndex = -1;
+      else if (i < mSelectionPivotIndex) {
+        pivotShift++;
+      }

Why does the if statement not have brackets and the else if have them? Be consistent and use them in both places or not.


58% down, line 192
+  
+  mSelectionPivotIndex -= pivotShift;

trailing whitespace on the blank line.


65% down, line 229
+      [self deselectDownloads:currentlySelectedDownloads];
+      mSelectionPivotIndex = -1;
+    }
+    
+    return;
+  }
+  

More blank lines with trailing whitespace.

Other than those nits, looks good. :-)

r=me

Comment 11

[Ilya Sherman]

Attachment: Fix v2.3, nits addressed

Thanks for the reviews :)

Comment 12

[Mike Pinkerton (not reading bugmail)]

-      // take  instance's frame origin y, subtract content view height, 
+      // take  instance's frame origin y, subtract content view height,

extra space after |take|?

sr=pink

Comment 13

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

Landed on cvs trunk, without the extra space in that line.