363680 - Bookmark Info should be disabled for menu spacers

Status: RESOLVED FIXED

(Camino Graveyard :: Bookmarks, defect)

Description

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

I see no reason why users would want to get info on the spacers, especially since they are lines in the manager now (and show up as Info for ' ' in the panel title :P)

Comment 1

[Chris Lawson (gone)]

Bookmark Info isn't the only thing we need to disable for menu spacers. Take a look at the context menu for a separator in the outline view -- the ONLY two things it should contain, IMO, are "Delete" (so you can delete it) and "New Folder" (which is in every outline view CM anyway). None of the other actions -- "Arrange", "Open In New X", "Copy Location to Clipboard" -- have any meaning for separators, and in fact, opening a separator in a new tab makes Gecko spew junk into the debug console.

cl

Comment 2

[Chris Lawson (gone)]

Attachment: does comment 2

diff -w for ease of review. Will probably apply with offsets as there's a bunch of stuff from bug 338558 in my tree at the moment (but should apply cleanly otherwise).

Comment 3

[Chris Lawson (gone)]

Oh, and this also fixes the situation where you can Get Info on a collection via the menu item, even if there's no selection in the outline view. (Bookmark Info is now disabled in that situation.) If there's a need to have a Get Info-style functionality for collections in the future (for example, bug 287406, which seems to me like it would require an entirely different inspector anyway), we can explicitly enable it at that point.

cl

Comment 4

[Chris Lawson (gone)]

Attachment: all fixes in comment 2, enables Reveal for separators

This applies all the fixes in comment 2 and enables the new Reveal item for separators in the bookmark bar. I've removed |validateMenuItem:| from BookmarkButton because the context menu only has what we need, and has been fixed so it doesn't even give separators CM items that don't make sense (so the validation isn't needed any more).

cl

Comment 5

[froodian (Ian Leue)]

Comment on attachment 248673 [details] [diff] [review]
all fixes in comment 2, enables Reveal for separators

>   if ([items count] == 0) return nil;
> 
>   BOOL itemsContainsFolder = NO;
>   BOOL itemsContainsBookmark = NO;
>   BOOL multipleItems = ([items count] > 1);
>+  BOOL isSingleSeparator = (([items count] == 1) && [[items objectAtIndex:0] isSeparator]);

There are tons of [items count] calls throughout this method - let's keep it in a variable.
 
>   // copy URL(s) to clipboard
>+  // This makes no sense for single separators, since they have no URL.
>+  // We handle the selector-embedded-in-multiple-items case in |copyURLs:|
>+  // where we simply ignore separator items.

This comment is prone to becoming false.  How about just "assumes that copyURLS: ignores embedded separator items"

>-    // space
>+    // space unless selection is a separator, in which case Delete is first menu item
>+    if (!isSingleSeparator)
>     [contextMenu addItem:[NSMenuItem separatorItem]];
>+

We need to let this separator through in the bookmark button case (delete should always be buffered from the other items, even though the only other item in the bm buttons is reveal).  Just drop a "|| !outlineView" in the if.  Hmm... or maybe just check to see if the contextMenu has at least one item in it yet.  That might be more robust.

>-  // Arrange bookmarks items. these may get removed again by the caller, so
>-  // we tag them.
>+  // Arrange selections of multiple bookmark items or folders. Doesn't make sense for single bookmarks.
>+  // These may get removed again by the caller, so we tag them.

Let's lose the "Doesn't make sense for single bookmarks."  I think that's covered by "multiple"

>-    // if it's a bookmark and we haven't seen it yet
>-    if (([curItem isKindOfClass:[Bookmark class]]) && (![seenBookmarks containsObject:curItem])) {
>+    // if it's a bookmark that's not a separator and we haven't seen it yet
>+    if (([curItem isKindOfClass:[Bookmark class]]) && (![curItem isSeparator]) && (![seenBookmarks containsObject:curItem])) {

That parenthesizing seems a little excessive (i know you were just going with what was there).

>@@ -1612,18 +1596,19 @@
>       return (selItem != nil);
> 
>     if (action == @selector(openBookmarkInNewWindow:))
>       return (selItem != nil);
> 
>     if (action == @selector(deleteBookmarks:))
>       return (selItem != nil);
> 
>-    if (action == @selector(showBookmarkInfo:))
>-      return (selItem != nil);
>+    // getInfo: is passed here from BrowserWindowController
>+    if (action == @selector(showBookmarkInfo:) || action == @selector(getInfo:))
>+      return ((selItem != nil) && (![selItem isSeparator]));

Let's collapse all of these "return (selItem != nil)" validations into one if while we're here.  Also, if I understand it correctly, |showBookmarkInfo| isn't relevant when the item is a separator, so maybe that clause of the if condition should go above with the others too.

>-    if([self bookmarkManagerIsVisible])
>+    if ([self bookmarkManagerIsVisible]) {
>       [aMenuItem setTitle:NSLocalizedString(@"Bookmark Info", nil)];
>+      // let the BookmarkViewController validate so we disable Get Info for separators
>+      return [[self bookmarkViewControllerForCurrentTab] validateMenuItem:aMenuItem];
>+    }

how about "// let the BookmarkViewController validate based on selection"

Comment 6

[Chris Lawson (gone)]

Attachment: still diff -w for ease of sr

All review comments addressed, asking for sr from pink. This version also deletes a couple seemingly unnecessary comments where the code is self-documenting.

cl

Comment 7

[Chris Lawson (gone)]

Attachment: patch for checkin, assuming -w patch passes sr

Here's the version for checkin.

Comment 8

[froodian (Ian Leue)]

Comment on attachment 248849 [details] [diff] [review]
patch for checkin, assuming -w patch passes sr

>   if ([items count] == 0) return nil;

I know this isn't you, but let's get the return on its own line

>+  if (!isSingleSeparator) {
>+  
>+    // open in new window(s)

No whitespace on the blank line.

>   // 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]))))
>+      (!multipleItems && (([self searchActive] && !(collection == [self historyFolder])) ||
>+                        (collection == [self top10Folder]))))
>   {

The (collection == [self top10Folder])))) line needs to be two spaces farther to the right, given the change in the line above it.


>-    if (action == @selector(deleteBookmarks:))
>-      return (selItem != nil);
>+    if ((action == @selector(openBookmark:)) ||
>+        (action == @selector(openBookmarkInNewTab:)) ||
>+        (action == @selector(openBookmarkInNewWindow:)) ||
>+        (action == @selector(deleteBookmarks:)) ||
>+        (action == @selector(showBookmarkInfo:)))
>+      {
>+        return (selItem != nil);
>+      }

The curlies and return line are one tab too far in.

These are all nits, and would be r+... but there's a functionality problem.  If there are only two separators selected, we give them the regurlar cm items.  Instead of having |isSingleSeparator| we should have |onlySeparatorItems| or something, which is true if every selected item is a separator.

Comment 9

[Chris Lawson (gone)]

Attachment: Doesn't break when selection is only separators

Comment 10

[froodian (Ian Leue)]

Comment on attachment 248855 [details] [diff] [review]
Doesn't break when selection is only separators

>+  // Single separators shouldn't have these CM items at all.
>+  // We rely on the called selectors to do the Right Thing(tm) with embedded separators.

Given the new logic, s/Single/solitary

>-  [contextMenu addItem:[NSMenuItem separatorItem]];
>+  if (!itemsAllSeparators)
>+    [contextMenu addItem:[NSMenuItem separatorItem]];
> 
>-  if (!outlineView || ([items count] == 1)) {
>+  if ((!outlineView || !multipleItems) && !itemsAllSeparators) {
>     menuTitle = NSLocalizedString(@"Bookmark Info", nil);
>     menuItem = [[[NSMenuItem alloc] initWithTitle:menuTitle action:@selector(showBookmarkInfo:) keyEquivalent:@""] autorelease];
>     [menuItem setTarget:target];
>     [contextMenu addItem:menuItem];
>   }

I think it's safe to think of the separator created here as "belonging" to the Bookmark Info item - let's move the !itemsAllSeparators check to wrap around both bits.

r=me with those changes.

Comment 11

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 248855 [details] [diff] [review]
Doesn't break when selection is only separators

sr=pink

Comment 12

[froodian (Ian Leue)]

Checked in on trunk and MOZILLA_1_8_BRANCH with checkin loving for comment 10.

Comment 13

[Samuel Sidler (old account; do not CC)]

Moving fixed "1.2" bugs to 1.1 where they were really fixed. Filter on CaminoFixed1.1 for bugmail purposes.