319491 - Reset Camino does not clear top 10 list in bookmarks

Status: RESOLVED FIXED

(Camino Graveyard :: General, defect)

Description

[Chris Lawson (gone)]

Separate issue from bug 300408, which is for UI.

"Reset Camino" should nuke the top 10 group in the Bookmarks Manager. Right now, it doesn't, and as the top 10 group is our default Dock menu source, it's causing confusion when users reset Camino and find their old bookmarks still in the Dock menu.

Comment 1

[Chris Lawson (gone)]

Nominating to block 1.0. Patch will be forthcoming.

cl

Comment 2

[Chris Lawson (gone)]

OK, what seems like the most logical behaviour here:

a) reset all bookmark visit counts to 0 (in the absence of wiping out bookmarks entirely, which I assume we avoid in Reset Camino for a reason)

b) reset the counts of the top 10 items to 0, forcing a rebuild of the top 10 group (which will leave the *next* 10 bookmarks as the new top 10, potentially alleviating user confusion not at all)

c) something else entirely

I prefer option (a) -- this is "reset", after all.

Josh's bug 257281 comment 15 said we should leave bookmarks alone, but I disagree. I'm with Mike's bug 257281 comment 16 -- if we leave bookmarks alone entirely, we end up with issues like the confusion demonstrated by the filing of bug 319476.

CCing Simon, Josh, and Smokey for further discussion.

cl

Comment 3

[Simon Fraser [no longer active]]

(In reply to comment #2)
> OK, what seems like the most logical behaviour here:
> 
> a) reset all bookmark visit counts to 0 (in the absence of wiping out bookmarks
> entirely, which I assume we avoid in Reset Camino for a reason)

Yes, if we choose to do anything. I guess visit counts reaveal browsing behavior, so should be cleared by Reset.

> b) reset the counts of the top 10 items to 0, forcing a rebuild of the top 10
> group (which will leave the *next* 10 bookmarks as the new top 10, potentially
> alleviating user confusion not at all)

No, very confusing.

Comment 4

[Chris Lawson (gone)]

Attachment: clear all visit counts when resetting

Clear all bookmarks visit counts when resetting Camino, per discussions on IRC, as well as Stuart's comment on MZ.

Fixes an NSArray in KindaSmartFolderManager.mm to use NSEnumerator instead, too, per Simon's request.

Comment 5

[Stuart Morgan]

Last visit is history information as well, and should also be cleared--whatever "cleared" means, since I'm guessing a nil date will cause issues.  Maybe setting the date to the epoch and special casing the display of that value in the info window?

Comment 6

[Simon Fraser [no longer active]]

Comment on attachment 205279 [details] [diff] [review]
clear all visit counts when resetting

>Index: KindaSmartFolderManager.mm
>===================================================================

>+  NSEnumerator* bookmarksEnum = [[[BookmarkManager sharedBookmarkManager] rootBookmarks];

This should have given you a build warning, and thrown an exception at runtime. Did you test it?  ;)

>Index: MainController.mm


>     
>+    // re-set all bookmarks visit counts to zero
>+    NSEnumerator* bookmarksEnum = [[[BookmarkManager sharedBookmarkManager] rootBookmarks];
>+    BookmarkItem* curItem;
>+    while ((curItem = [bookmarksEnum nextObject]))
>+    {
>+      [curItem setNumberOfVisits:0];
>+    }

Please move this to a method on the bookmark manager. (clearVisitCounts or something).

Comment 7

[Chris Lawson (gone)]

Attachment: fixes silly mistake and moves clearAllVisits to Bookmark Manager

Contains part of the fix for bug 254313 as both modify BookmarkManager.mm and I couldn't figure out a clean way to split the patches into two diffs without pulling fresh source between diffs.

cl

Comment 8

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

(In reply to comment #5)
> Last visit is history information as well, and should also be cleared--whatever
> "cleared" means, since I'm guessing a nil date will cause issues.  Maybe
> setting the date to the epoch and special casing the display of that value in
> the info window?

Now bug 319649.

Comment 9

[Simon Fraser [no longer active]]

Comment on attachment 205364 [details] [diff] [review]
fixes silly mistake and moves clearAllVisits to Bookmark Manager

>Index: BookmarkManager.h
>===================================================================

> - (void)writeSafariFile:(NSString *)pathToFile;
> 
>+// Clear visit counts
>+-(void)clearAllVisits;

Please follow the spacing of the methods above.


>Index: KindaSmartFolderManager.mm
>===================================================================

>+  NSEnumerator* bookmarksEnum = [[BookmarkManager sharedBookmarkManager] rootBookmarks];

This still ain't gonna compile without warnings, or even run properly. You need:

  NSEnumerator* bookmarksEnum = [[[BookmarkManager sharedBookmarkManager] rootBookmarks] objectEnumerator];

>+  BookmarkItem* curItem;
>+  while ((curItem = [bookmarksEnum nextObject]))
>+  {
>+    [self checkForNewTop10:curItem];

Not your code, but -checkForNewTop10 looks for the item in the array twice (up to 2 * 10 comparisons), then compares with the array items (10 comparisons), then does a bunch of work to possibly insert. This could be made much more efficient.

>Index: BookmarkManager.mm
>===================================================================

>+//
>+// -clearAllVisits:
>+//
>+// resets all bookmarks visit counts to zero as part of Reset Camino
>+//
>+
>+-(void)clearAllVisits
>+{
>+    NSEnumerator* bookmarksEnum = (BookmarkManager*)[self rootBookmarks];
>+    BookmarkItem* curItem;
>+    while (curItem = [bookmarksEnum nextObject])
>+    {
>+	  if ([curItem isKindOfClass:[Bookmark class]]) {
>+        [(Bookmark*)curItem setNumberOfVisits:0];
>+	  }

Bad spacing here (and we tend not to use {} for 1-liners).

Comment 10

[Chris Lawson (gone)]

Attachment: fixes spacing as requested

KindaSmartFolderManager.mm dropped entirely, so Simon can do some reworking of the file.

cl

Comment 11

[Simon Fraser [no longer active]]

Fixed.