Last Comment Bug 288980 - Add "View (reveal) in Bookmark Manager" contextual menu item for folders in the Bookmark Bar.
: Add "View (reveal) in Bookmark Manager" contextual menu item for folders in t...
Status: RESOLVED FIXED
: fixed1.8.1.1
Product: Camino Graveyard
Classification: Graveyard
Component: Bookmarks (show other bugs)
: unspecified
: PowerPC Mac OS X
-- enhancement (vote)
: Camino1.5
Assigned To: Chris Lawson (gone)
:
:
Mentors:
: 359584 (view as bug list)
Depends on:
Blocks: 229513
  Show dependency treegraph
 
Reported: 2005-04-04 09:56 PDT by Wevah
Modified: 2006-12-08 09:24 PST (History)
7 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
fix (7.65 KB, patch)
2006-12-07 10:51 PST, Chris Lawson (gone)
froodian: review-
Details | Diff | Splinter Review
updated, also fixes 229513 (15.15 KB, patch)
2006-12-07 14:14 PST, Chris Lawson (gone)
froodian: review-
Details | Diff | Splinter Review
review comments addressed (15.70 KB, patch)
2006-12-07 16:59 PST, Chris Lawson (gone)
froodian: review+
mikepinkerton: superreview+
Details | Diff | Splinter Review

Description User image Wevah 2005-04-04 09:56:46 PDT
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 User image Mike Pinkerton (not reading bugmail) 2005-04-19 05:01:33 PDT
why just for folders? 

maybe call it "reveal in bookmark manager" after the similar function in the finder?
Comment 2 User image Wevah 2005-04-19 16:16:14 PDT
(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 User image froodian (Ian Leue) 2006-11-05 09:10:16 PST
*** Bug 359584 has been marked as a duplicate of this bug. ***
Comment 4 User image Chris Lawson (gone) 2006-12-07 10:51:35 PST
Created attachment 247855 [details] [diff] [review]
fix

Rips out some dead code and makes this thing work properly.
Comment 5 User image Chris Lawson (gone) 2006-12-07 10:54:28 PST
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 User image froodian (Ian Leue) 2006-12-07 12:52:06 PST
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 User image Chris Lawson (gone) 2006-12-07 14:14:07 PST
Created attachment 247877 [details] [diff] [review]
updated, also fixes 229513

Whee. That was a fun afternoon.
Comment 8 User image froodian (Ian Leue) 2006-12-07 16:33:29 PST
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 User image Chris Lawson (gone) 2006-12-07 16:59:46 PST
Created attachment 247915 [details] [diff] [review]
review comments addressed
Comment 10 User image Stuart Morgan 2006-12-07 17:17:01 PST
(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 User image froodian (Ian Leue) 2006-12-08 00:01:32 PST
Comment on attachment 247915 [details] [diff] [review]
review comments addressed

r=me.  shiny.
Comment 12 User image Mike Pinkerton (not reading bugmail) 2006-12-08 05:40:38 PST
Comment on attachment 247915 [details] [diff] [review]
review comments addressed

sr=pink
Comment 13 User image froodian (Ian Leue) 2006-12-08 09:24:44 PST
Checked in on 1.8branch and trunk.

Note You need to log in before you can comment on or make changes to this bug.