288980 - Add "View (reveal) in Bookmark Manager" contextual menu item for folders in the Bookmark Bar.

Status: RESOLVED FIXED

(Camino Graveyard :: Bookmarks, enhancement)

Description

[Wevah (Nate Weaver)]

I often find myself wanting to reorder or otherwise modify the contents of a
folder in the BM bar; a shortcut like this would be quite handy.

Comment 1

[Mike Pinkerton (not reading bugmail)]

why just for folders? 

maybe call it "reveal in bookmark manager" after the similar function in the finder?

Comment 2

[Wevah (Nate Weaver)]

(In reply to comment #1)
> why just for folders? 
> 
> maybe call it "reveal in bookmark manager" after the similar function in the
finder?

You're right, it shouldn't be just for folders. I just think it would be most
*useful* for folders (for reordering contents and stuff). But yes, not just for
folders.

Comment 3

[froodian (Ian Leue)]

*** Bug 359584 has been marked as a duplicate of this bug. ***

Comment 4

[Chris Lawson (gone)]

Attachment: fix

Rips out some dead code and makes this thing work properly.

Comment 5

[Chris Lawson (gone)]

Targeting back to 1.1 since we have code. This should help with bug 229513 but it doesn't need to depend on that bug. (In fact, the other way around makes more sense, IMO.)

I'll work on that one next.

cl

Comment 6

[froodian (Ian Leue)]

Comment on attachment 247855 [details] [diff] [review]
fix

Per IRC, we should find a way to do this without using |bookmarkViewControllerForCurrentTab|, since it's exposing internals:

[3:46pm] <smorgan> Generally, the solution is to have a chain of methods that pass info down, rather than having a chain of accessors
[3:47pm] <smorgan> See http://en.wikipedia.org/wiki/Law_Of_Demeter

Also, there are some behavioral issues here - if the bookmark manager isn't already visible, it won't select the correct selection or the relevant bookmark.  Also, I would load bookmarks by calling |manageBookmarks|, not by loading "about:bookmarks" manually.  Since you check first to make sure the manager isn't visible, it amounts to the same thing, but if we ever changed our bookmark-loading scheme, calling |manageBookmarks| is more future-proofed.

Comment 7

[Chris Lawson (gone)]

Attachment: updated, also fixes 229513

Whee. That was a fun afternoon.

Comment 8

[froodian (Ian Leue)]

Comment on attachment 247877 [details] [diff] [review]
updated, also fixes 229513

>+  BookmarkFolder* collection = [target activeCollection];
>+  // We only want a "Reveal" menu item if the CM is on a BookmarkButton,
>+  // if the user is searching somewhere other than the History folder,
>+  // or if the Top 10 is the active collection.
>+  if ((!outlineView) ||
>+      (([items count] == 1) && (([self searchActive] && !(collection == [self historyFolder])) ||
>+                               ((collection == [self top10Folder])))))
>+  {

the ((collection == [self top10Folder]... line should start one space farther in, since it's enclosed in the first paren of the clause above it.

Also, I know I said to do it this way... but we can't call |activeCollection| unless we know that target is a BookmarkViewController.  As it is, you can't get a CM for bookmark bar items anymore.  Put the [target activeCollection] calls back in the if, or do something like

BookmarkFolder* collection = [target isKindOfClass:[BookmarkViewController class]] ? [target activeCollection] : nil;

or something.

I thought we'd decided to rearrange the menu items... I don't see that.

>+- (void)revealBookmark:(BookmarkItem*)anItem
>+{
>+  BookmarkViewController* bvc = [self bookmarkViewControllerForCurrentTab];
>+
>+  if (![self bookmarkManagerIsVisible]) {
>+    [bvc setItemToRevealOnLoad:anItem];
>+    [self performSelector:@selector(manageBookmarks:) withObject:nil afterDelay:0];
>+  }
>+  else {
>+    [bvc revealItem:anItem scrollIntoView:YES selecting:YES byExtendingSelection:NO];
>+  }
>+}

If there isn't a particular reason for the performSelector call, I would just do [self manageBookmarks:nil].  Even with a delay of 0, there's still a delay (if the comments sprinkled throughout our code is to be believed).

Comment 9

[Chris Lawson (gone)]

Attachment: review comments addressed

Comment 10

[Stuart Morgan]

(In reply to comment #8)
> Even with a delay of 0, there's still a delay
> (if the comments sprinkled throughout our code is to be believed).

That causes it to be scheduled on the runloop immediately, but it won't actually run until a subsequent pass through the runloop.

Comment 11

[froodian (Ian Leue)]

Comment on attachment 247915 [details] [diff] [review]
review comments addressed

r=me.  shiny.

Comment 12

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 247915 [details] [diff] [review]
review comments addressed

sr=pink

Comment 13

[froodian (Ian Leue)]

Checked in on 1.8branch and trunk.