387253 - Bookmark not removed in list when removing it while filtering

Status: RESOLVED FIXED

(Camino Graveyard :: Bookmarks, defect)

Description

[Markus Amalthea Magnuson]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; sv; rv:1.8.1.4) Gecko/20070501 Camino/1.5
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; sv; rv:1.8.1.4) Gecko/20070501 Camino/1.5

If I filter the bookmarks view, and remove a bookmark in the filtered list, it is not immediately removed from the list. If I clear the filter and then filter again, the bookmark is gone.

Reproducible: Always

Steps to Reproduce:
1. Create a bookmark
2. Show bookmarks view
3. Filter the bookmarks so that the bookmark created in step 1 will show up
4. Delete that bookmark from the context-menu
Actual Results:  
The bookmark remains in the filtered list

Expected Results:  
The bookmark should immediately be removed from the list

Comment 1

[Stuart Morgan]

Confirming, and raising severity since if you keep trying to delete a deleted item it throws an exception which, at least in my test, locked the whole UI completely.

Comment 2

[Chris Lawson (gone)]

It didn't lock things up in my testing, but it does happen with either the CM item or just the delete key.

2007-07-07 20:52:43.678 Camino[5424] *** -[NSCFSet addObject:]: attempt to insert nil

is the error logged to console. The first time I tried to reproduce this, Camino actually crashed entirely, but I think that was due to some interaction with a previously viewed Java applet, as I was unable to repro the crash.

As soon as I have a fresh tree again, I can look into this, unless you were planning to, Stuart.

Comment 3

[Chris Lawson (gone)]

For my own reference:

http://mxr.mozilla.org/mozilla/source/camino/src/bookmarks/BookmarkViewController.mm#431

Need to trigger a rebuild of the search results in there somewhere in addition to the other cleanup we already do. (Unless there's a better place to be doing this, that is.)

Comment 4

[cannedbrain+mozilla]

I'm using Version 2007073000 (2.0a1pre) and I can confirm this. Except, the bookmark is actually gone. It's just it still shows up in the list until you refresh the search (do another search).

Comment 5

[Markus Amalthea Magnuson]

Attachment: Initial patch, needs testing

Quite a quick fix which seems to work as expected, at least in the case I first tried when filing this bug. Could use some testing though.

Comment 6

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

--> Markus

Comment 7

[Stuart Morgan]

Comment on attachment 293455 [details] [diff] [review]
Initial patch, needs testing

It would be much more efficient to just remove the deleted bookmarks directly from the search results array, rather than doing the search again. You'll also want to do it before the code that tries to fix the selection for the new state of the table.

Comment 8

[Markus Amalthea Magnuson]

Attachment: Better patch

This patch is much better, it checks if we're actually filtering at the moment and only takes action if we are. Also, it removes the deleted bookmarks from the search array instead of doing a new search, as Stuart suggests.

Comment 9

[Stuart Morgan]

Comment on attachment 293552 [details] [diff] [review]
Better patch

>+    currentSearchArray = [NSMutableArray arrayWithArray:mSearchResultArray];

[mSearchResultArray mutableCopy] would probably be a little clearer here.

>+    // Remember which bookmarks that were deleted so they can be
>+    // removed from the search result array as well.
>+    [currentSearchArray removeObject:doomedBookmark];

The comment is a bit confusing here (it says remember, but it's removing from the array); I'd say the code speaks for itself and the comment could just be removed.

>     BookmarkFolder *currentParent = [doomedBookmark parent];
>     [parentsToNotify addObject:currentParent];
>     [currentParent deleteChild:doomedBookmark];
>   }
>+  
>+  // Make sure the outline view is up to date after deleting while filtering
>+  if (currentSearchArray)
>+    [self setSearchResultArray:currentSearchArray];

There should be a reloadData here (there may be an implicit one somewhere, but having an explicit call can't hurt).

Also, please strip the whitespace from the blank lines you're adding.

Other than the nits, looks good; r=me with those changes. Thanks for the patch!

Comment 10

[Markus Amalthea Magnuson]

Attachment: Third version

Attaching a third patch with the changes suggested by Stuart, this one should be final.

Comment 11

[Stuart Morgan]

Comment on attachment 294213 [details] [diff] [review]
Third version

>+    currentSearchArray = [mSearchResultArray mutableCopy];

This needs to be
      currentSearchArray = [[mSearchResultArray mutableCopy] autorelease];

r=me with that (no need to re-request review from me next time around; you just carry the r+ forward).

Comment 12

[Markus Amalthea Magnuson]

Attachment: Fourth version

Doh! Adding a fourth version of the patch, with an autorelease. Requesting review from Chris Lawson, as he is listed under "Bookmarks" at the reviewer page.

Comment 13

[Chris Lawson (gone)]

Comment on attachment 294375 [details] [diff] [review]
Fourth version

Totally unnecessary second-r=me :) Requesting sr from pink, since smorgan already gave this r+.

Comment 14

[Markus Amalthea Magnuson]

I thought all patches needed two reviews before a super-review, at least that's what the wiki page on reviewing says. If that is not the case, please enlighten me.

(And edit the wiki :p)

Comment 15

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

The wiki is correct; two reviews before super-review is the requirement in the Camino module. 

We enforce this strictly for a period of time on all non-trivial patches from all new contributors, but because of resource (people, time) limitations, we have for the past couple of years been waiving the requirement for second review after consensus that the new contributor has demonstrated competence and is comfortable with our codebase and norms ;)

Comment 16

[Markus Amalthea Magnuson]

Should I ask someone else for superreview on this?

Comment 17

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 294375 [details] [diff] [review]
Fourth version

sr=pink

Comment 18

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

Landed on the trunk and MOZILLA_1_8_BRANCH in advance of b1.  Thanks again, Markus!