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

RESOLVED FIXED in Camino1.5

Status

Camino Graveyard
Bookmarks
--
enhancement
RESOLVED FIXED
13 years ago
11 years ago

People

(Reporter: Wevah, Assigned: Chris Lawson (gone))

Tracking

({fixed1.8.1.1})

unspecified
Camino1.5
PowerPC
Mac OS X
fixed1.8.1.1

Details

Attachments

(1 attachment, 2 obsolete attachments)

15.70 KB, patch
froodian (Ian Leue)
: review+
Mike Pinkerton (not reading bugmail)
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

13 years ago
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.
why just for folders? 

maybe call it "reveal in bookmark manager" after the similar function in the finder?
Target Milestone: --- → Camino1.1
(Reporter)

Comment 2

13 years ago
(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.

Updated

12 years ago
Depends on: 229513
Assignee: mikepinkerton → nobody
QA Contact: bookmarks
Target Milestone: Camino1.1 → Camino2.0

Comment 3

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

Updated

11 years ago
Summary: Add "View in Bookmark Manager" contextual menu item for folders in the Bookmark Bar. → Add "View (reveal) in Bookmark Manager" contextual menu item for folders in the Bookmark Bar.
(Assignee)

Comment 4

11 years ago
Created attachment 247855 [details] [diff] [review]
fix

Rips out some dead code and makes this thing work properly.
Assignee: nobody → bugzilla
Status: NEW → ASSIGNED
Attachment #247855 - Flags: review?(stridey)
(Assignee)

Comment 5

11 years ago
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
No longer depends on: 229513
Target Milestone: Camino2.0 → Camino1.1
(Assignee)

Updated

11 years ago
Blocks: 229513

Comment 6

11 years ago
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.
Attachment #247855 - Flags: review?(stridey) → review-
(Assignee)

Comment 7

11 years ago
Created attachment 247877 [details] [diff] [review]
updated, also fixes 229513

Whee. That was a fun afternoon.
Attachment #247855 - Attachment is obsolete: true
Attachment #247877 - Flags: review?(stridey)

Comment 8

11 years ago
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).
Attachment #247877 - Flags: review?(stridey) → review-
(Assignee)

Comment 9

11 years ago
Created attachment 247915 [details] [diff] [review]
review comments addressed
Attachment #247877 - Attachment is obsolete: true
Attachment #247915 - Flags: review?(stridey)

Comment 10

11 years ago
(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

11 years ago
Comment on attachment 247915 [details] [diff] [review]
review comments addressed

r=me.  shiny.
Attachment #247915 - Flags: superreview?(mikepinkerton)
Attachment #247915 - Flags: review?(stridey)
Attachment #247915 - Flags: review+
Comment on attachment 247915 [details] [diff] [review]
review comments addressed

sr=pink
Attachment #247915 - Flags: superreview?(mikepinkerton) → superreview+

Comment 13

11 years ago
Checked in on 1.8branch and trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.1.1
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.