Camino only - Bug 363680:Context menu overhaul for menu spacers. Patch by cl <bugzilla@chrislawson.net>. r=me sr=pink

camino/src/bookmarks/BookmarkButton.mm

@@ -238,20 +238,6 @@
   return [[BookmarkManager sharedBookmarkManager] contextMenuForItems:theItemArray fromView:nil target:self];
 }
 
-//
-// context menu has only what we need
-//
-- (BOOL)validateMenuItem:(NSMenuItem*)aMenuItem
-{
-  if ([[self bookmarkItem] isKindOfClass:[Bookmark class]] && [(Bookmark *)[self bookmarkItem] isSeparator]) {
-    SEL action = [aMenuItem action];
-    if (action != @selector(deleteBookmarks:))
-      return NO;
-  }
-
-  return YES;
-}
-
 - (void)showFolderPopupAction:(id)aSender
 {
   [self showFolderPopup:[NSApp currentEvent]];

camino/src/bookmarks/BookmarkManager.mm

@@ -719,10 +719,12 @@ static BookmarkManager* gBookmarkManager = nil;
 // or from a bookmark button, which should pass a nil outlineView
 - (NSMenu *)contextMenuForItems:(NSArray*)items fromView:(BookmarkOutlineView *)outlineView target:(id)target
 {
-  if ([items count] == 0) return nil;
+  if ([items count] == 0)
+    return nil;
 
   BOOL itemsContainsFolder = NO;
   BOOL itemsContainsBookmark = NO;
+  BOOL itemsAllSeparators = YES;
   BOOL multipleItems = ([items count] > 1);
 
   NSEnumerator* itemsEnum = [items objectEnumerator];
@@ -730,28 +732,34 @@ static BookmarkManager* gBookmarkManager = nil;
   while ((curItem = [itemsEnum nextObject])) {
     itemsContainsFolder   |= [curItem isKindOfClass:[BookmarkFolder class]];
     itemsContainsBookmark |= [curItem isKindOfClass:[Bookmark class]];
+    itemsAllSeparators    &= [curItem isSeparator];
   }
 
   // All the methods in this context menu need to be able to handle > 1 item
-  // being selected, and the selected items containing a mixture of folders
+  // being selected, and the selected items containing a mixture of folders,
-  // and bookmarks.
+  // bookmarks, and separators.
   NSMenu* contextMenu = [[[NSMenu alloc] initWithTitle:@"notitle"] autorelease];
   NSString* menuTitle = nil;
+  NSMenuItem* menuItem = nil;
+  NSMenuItem* shiftMenuItem = nil;
 
+  // Selections with only separators shouldn't have these CM items at all.
+  // We rely on the called selectors to do the Right Thing(tm) with embedded separators.
+  if (!itemsAllSeparators) {
     // open in new window(s)
-  if (itemsContainsFolder && [items count] == 1)
+    if (itemsContainsFolder && !multipleItems)
       menuTitle = NSLocalizedString(@"Open Tabs in New Window", nil);
     else if (multipleItems)
       menuTitle = NSLocalizedString(@"Open in New Windows", nil);
     else
       menuTitle = NSLocalizedString(@"Open in New Window", nil);
 
-  NSMenuItem *menuItem = [[[NSMenuItem alloc] initWithTitle:menuTitle action:@selector(openBookmarkInNewWindow:) keyEquivalent:@""] autorelease];
+    menuItem = [[[NSMenuItem alloc] initWithTitle:menuTitle action:@selector(openBookmarkInNewWindow:) keyEquivalent:@""] autorelease];
     [menuItem setTarget:target];
     [menuItem setKeyEquivalentModifierMask:0]; //Needed since by default NSMenuItems have NSCommandKeyMask
     [contextMenu addItem:menuItem];
 
-  NSMenuItem *shiftMenuItem = [NSMenuItem alternateMenuItemWithTitle:menuTitle action:@selector(openBookmarkInNewWindow:) target:target modifiers:NSShiftKeyMask];
+    shiftMenuItem = [NSMenuItem alternateMenuItemWithTitle:menuTitle action:@selector(openBookmarkInNewWindow:) target:target modifiers:NSShiftKeyMask];
     [contextMenu addItem:shiftMenuItem];
 
     // open in new tabs in new window
@@ -780,13 +788,14 @@ static BookmarkManager* gBookmarkManager = nil;
 
     shiftMenuItem = [NSMenuItem alternateMenuItemWithTitle:menuTitle action:@selector(openBookmarkInNewTab:) target:target modifiers:NSShiftKeyMask];
     [contextMenu addItem:shiftMenuItem];
+  }
 
   BookmarkFolder* collection = [target isKindOfClass:[BookmarkViewController class]] ? [target activeCollection] : nil;
   // 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])) ||
+      (!multipleItems && (([self searchActive] && !(collection == [self historyFolder])) ||
                           (collection == [self top10Folder]))))
   {
     menuTitle = NSLocalizedString(@"Reveal in Bookmark Manager", nil);
@@ -795,25 +804,32 @@ static BookmarkManager* gBookmarkManager = nil;
     [contextMenu addItem:menuItem];
   }
 
+  if (!itemsAllSeparators) {
     [contextMenu addItem:[NSMenuItem separatorItem]];
 
-  if (!outlineView || ([items count] == 1)) {
+    if (!outlineView || !multipleItems) {
       menuTitle = NSLocalizedString(@"Bookmark Info", nil);
       menuItem = [[[NSMenuItem alloc] initWithTitle:menuTitle action:@selector(showBookmarkInfo:) keyEquivalent:@""] autorelease];
       [menuItem setTarget:target];
       [contextMenu addItem:menuItem];
     }
+  }
 
   // copy URL(s) to clipboard
+  // This makes no sense for separators, which have no URL.
+  // We rely on |copyURLs:| to handle the selector-embedded-in-multiple-items case.
+  if (!itemsAllSeparators) {
     if (itemsContainsFolder || multipleItems)
       menuTitle = NSLocalizedString(@"Copy URLs to Clipboard", nil);
     else
       menuTitle = NSLocalizedString(@"Copy URL to Clipboard", nil);
+
     menuItem = [[[NSMenuItem alloc] initWithTitle:menuTitle action:@selector(copyURLs:) keyEquivalent:@""] autorelease];
     [menuItem setTarget:target];
     [contextMenu addItem:menuItem];
+  }
 
-  if (([items count] == 1) && itemsContainsFolder) {
+  if (!multipleItems && itemsContainsFolder) {
     menuTitle = NSLocalizedString(@"Use as Dock Menu", nil);
     menuItem = [[[NSMenuItem alloc] initWithTitle:menuTitle action:@selector(toggleIsDockMenu:) keyEquivalent:@""] autorelease];
     [menuItem setTarget:[items objectAtIndex:0]];
@@ -828,11 +844,13 @@ static BookmarkManager* gBookmarkManager = nil;
 
   // if we're not in a smart collection (other than history)
   if (!outlineView ||
-      ![target isKindOfClass:[BookmarkViewController class]] ||
       ![[target activeCollection] isSmartFolder] ||
-      ([target activeCollection] == [self historyFolder])) {
+      ([target activeCollection] == [self historyFolder]))
-    // space
+  {
+    if ([contextMenu numberOfItems] != 0)
+      // only add a separator if it won't be the first item in the menu
       [contextMenu addItem:[NSMenuItem separatorItem]];
+
     // delete
     menuTitle = NSLocalizedString(@"Delete", nil);
     menuItem = [[[NSMenuItem alloc] initWithTitle:menuTitle action:@selector(deleteBookmarks:) keyEquivalent:@""] autorelease];
@@ -850,10 +868,12 @@ static BookmarkManager* gBookmarkManager = nil;
     [contextMenu addItem:menuItem];
   }
 
-  // Arrange bookmarks items. these may get removed again by the caller, so
+  // Arrange selections of multiple bookmark items or folders.
-  // we tag them.
+  // These may get removed again by the caller, so we tag them.
   if ([target isKindOfClass:[BookmarkViewController class]] &&
-      ![[target activeCollection] isSmartFolder])
+      ![[target activeCollection] isSmartFolder] &&
+      (multipleItems || itemsContainsFolder) &&
+      !itemsAllSeparators)
   {
     NSMenuItem* separatorItem = [NSMenuItem separatorItem];
     [separatorItem setTag:kBookmarksContextMenuArrangeSeparatorTag];
@@ -906,6 +926,7 @@ static BookmarkManager* gBookmarkManager = nil;
 //
 // Copy a set of bookmarks URLs to the specified pasteboard.
 // We don't care about item titles here, nor do we care about format.
+// Separators have no URL and are ignored.
 //
 - (void)copyBookmarksURLs:(NSArray*)bookmarkItems toPasteboard:(NSPasteboard*)aPasteboard
 {
@@ -917,8 +938,7 @@ static BookmarkManager* gBookmarkManager = nil;
   NSEnumerator* bookmarkItemsEnum = [bookmarkItems objectEnumerator];
   BookmarkItem* curItem;
   while (curItem = [bookmarkItemsEnum nextObject]) {
-    // if it's a bookmark and we haven't seen it yet
+    if ([curItem isKindOfClass:[Bookmark class]] && ![curItem isSeparator] && ![seenBookmarks containsObject:curItem]) {
-    if (([curItem isKindOfClass:[Bookmark class]]) && (![seenBookmarks containsObject:curItem])) {
       [seenBookmarks addObject:curItem]; // now we've seen it
       [urlList addObject:[(Bookmark*)curItem url]];
     }
@@ -928,8 +948,7 @@ static BookmarkManager* gBookmarkManager = nil;
       NSEnumerator* childrenEnum = [children objectEnumerator];
       Bookmark* curChild;
       while (curChild = [childrenEnum nextObject]) {
-        // if we haven't seen it yet
+        if (![seenBookmarks containsObject:curChild] && ![curItem isSeparator]) {
-        if (![seenBookmarks containsObject:curChild]) {
           [seenBookmarks addObject:curChild]; // now we've seen it
           [urlList addObject:[curChild url]];
         }

camino/src/bookmarks/BookmarkViewController.mm

@@ -507,7 +507,7 @@ const int kOutlineViewLeftMargin = 19; // determined empirically, since it doesn
       else
         [mBookmarksOutlineView expandItem:curItem];
     }
-    else {
+    else if (![curItem isSeparator]) {
       // otherwise follow the standard bookmark opening behavior
       BOOL shiftKeyDown = ([[NSApp currentEvent] modifierFlags] & NSShiftKeyMask) != 0;
       EBookmarkOpenBehavior behavior = eBookmarkOpenBehavior_Preferred;
@@ -536,7 +536,7 @@ const int kOutlineViewLeftMargin = 19; // determined empirically, since it doesn
       [[NetworkServices sharedNetworkServices] attemptResolveService:[(RendezvousBookmark*)curItem serviceID] forSender:curItem];
       mOpenActionFlag = eOpenInNewTabAction;
     }
-    else {
+    else if (![curItem isSeparator]) {
       // otherwise follow the standard bookmark opening behavior
       BOOL reverseBackgroundPref = NO;
       if ([aSender isAlternate])
@@ -570,7 +570,7 @@ const int kOutlineViewLeftMargin = 19; // determined empirically, since it doesn
       mOpenActionFlag = eOpenInNewTabAction;
     }
     else {
-      if ([curItem isKindOfClass:[Bookmark class]])
+      if ([curItem isKindOfClass:[Bookmark class]] && ![curItem isSeparator])
         [urlArray addObject:[curItem url]];
       else if ([curItem isKindOfClass:[BookmarkFolder class]])
         [urlArray addObjectsFromArray:[curItem childURLs]];
@@ -603,7 +603,7 @@ const int kOutlineViewLeftMargin = 19; // determined empirically, since it doesn
       [[NetworkServices sharedNetworkServices] attemptResolveService:[(RendezvousBookmark*)curItem serviceID] forSender:curItem];
       mOpenActionFlag = eOpenInNewWindowAction;
     }
-    else {
+    else if (![curItem isSeparator]) {
       // otherwise follow the standard bookmark opening behavior
       BOOL reverseBackgroundPref = NO;
       if ([aSender isAlternate])
@@ -1605,20 +1605,18 @@ const int kOutlineViewLeftMargin = 19; // determined empirically, since it doesn
 
     BookmarkItem* selItem = [self selectedBookmarkItem];
 
-    if (action == @selector(openBookmark:))
+    if ((action == @selector(openBookmark:)) ||
+        (action == @selector(openBookmarkInNewTab:)) ||
+        (action == @selector(openBookmarkInNewWindow:)) ||
+        (action == @selector(deleteBookmarks:)) ||
+        (action == @selector(showBookmarkInfo:)))
+    {
       return (selItem != nil);
+    }
 
-    if (action == @selector(openBookmarkInNewTab:))
+    // getInfo: is passed here from BrowserWindowController
-      return (selItem != nil);
+    if (action == @selector(getInfo:))
-
+      return ((selItem != nil) && ![selItem isSeparator]);
-    if (action == @selector(openBookmarkInNewWindow:))
-      return (selItem != nil);
-
-    if (action == @selector(deleteBookmarks:))
-      return (selItem != nil);
-
-    if (action == @selector(showBookmarkInfo:))
-      return (selItem != nil);
 
     if (action == @selector(arrange:)) {
       BookmarkFolder* activeCollection = [self activeCollection];

camino/src/browser/BrowserWindowController.mm

@@ -1686,8 +1686,11 @@ enum BWCOpenDest {
   }
 
   if (action == @selector(getInfo:)) {
-    if([self bookmarkManagerIsVisible])
+    if ([self bookmarkManagerIsVisible]) {
       [aMenuItem setTitle:NSLocalizedString(@"Bookmark Info", nil)];
+      // let the BookmarkViewController validate based on selection
+      return [[self bookmarkViewControllerForCurrentTab] validateMenuItem:aMenuItem];
+    }
     else
       [aMenuItem setTitle:NSLocalizedString(@"Page Info", nil)];
   }