Improved bookmarks menu item enabling. Rather than try to recreate the logic for automatic item enabling (which is non-trivial), use a dummy, auto-enabling bookmarks menu, loaded from the nib, to get correct enable states and item titles. This really fixes bug 287339.

camino/resources/localized/English.lproj/MainMenu.nib/classes.nib

@@ -66,6 +66,7 @@
             OUTLETS = {
                 mAddBookmarkMenuItem = NSMenuItem; 
                 mApplication = NSApplication; 
+                mBookmarksHelperMenu = NSMenu; 
                 mBookmarksMenu = NSMenu; 
                 mBookmarksToolbarMenuItem = NSMenuItem; 
                 mCloseTabMenuItem = NSMenuItem; 

camino/resources/localized/English.lproj/MainMenu.nib/info.nib

@@ -3,13 +3,15 @@
 <plist version="1.0">
 <dict>
 	<key>IBDocumentLocation</key>
-	<string>100 49 608 498 0 0 1600 1002 </string>
+	<string>53 17 409 367 0 0 1600 1002 </string>
 	<key>IBEditorPositions</key>
 	<dict>
 		<key>29</key>
-		<string>471 684 433 44 0 0 1600 1002 </string>
+		<string>274 731 433 44 0 0 1600 1002 </string>
 		<key>494</key>
-		<string>507 508 116 61 0 0 1152 848 </string>
+		<string>726 607 116 61 0 0 1600 1002 </string>
+		<key>670</key>
+		<string>398 459 199 118 0 0 1600 1002 </string>
 	</dict>
 	<key>IBFramework Version</key>
 	<string>364.0</string>

camino/src/application/MainController.h

@@ -77,6 +77,8 @@ typedef enum EBookmarkOpenBehavior
     IBOutlet NSMenu*        mBookmarksMenu;
     IBOutlet NSMenu*        mDockMenu;
     IBOutlet NSMenu*        mServersSubmenu;
+
+    IBOutlet NSMenu*        mBookmarksHelperMenu; // not shown, used to get enable state
 	
     IBOutlet NSMenuItem*    mBookmarksToolbarMenuItem;
     IBOutlet NSMenuItem*    mAddBookmarkMenuItem;
@@ -177,7 +179,7 @@ typedef enum EBookmarkOpenBehavior
 - (void)setupBookmarkMenus:(BookmarkManager *)BookmarkManager;
 - (void)loadBookmark:(BookmarkItem*)item withWindowController:(BrowserWindowController*)browserWindowController openBehavior:(EBookmarkOpenBehavior)behavior;
 
-- (void)displayPreferencesWindow:sender;
+- (void)displayPreferencesWindow:(id)sender;
 - (BOOL)isMainWindowABrowserWindow;
 
 // if the main window is a browser window, return its controller, otherwise nil

camino/src/application/MainController.mm

@@ -111,6 +111,7 @@ const int kReuseWindowOnAE = 2;
 - (NSMenu*)bookmarksMenu;
 - (BOOL)bookmarksItemsEnabled;
 - (void)adjustBookmarkMenuItems;
+- (void)doBookmarksMenuEnabling;
 - (void)windowLayeringDidChange:(NSNotification*)inNotifiction;
 - (void)openPanelDidEnd:(NSOpenPanel*)inOpenPanel returnCode:(int)inReturnCode contextInfo:(void*)inContextInfo;
 
@@ -182,12 +183,19 @@ const int kReuseWindowOnAE = 2;
 
   [mFindDialog release];
   [mKeychainService release];
+  
   [super dealloc];
 #if DEBUG
   NSLog(@"Main controller died");
 #endif
 }
 
+- (void)awakeFromNib
+{
+  // Be aware that we load a secondary nib for the accessory views, so this
+  // will get called more than once.
+}
+
 -(void)applicationDidFinishLaunching:(NSNotification *)aNotification
 {
   [self installBookmarksMenuEnableHandler];
@@ -298,9 +306,7 @@ const int kReuseWindowOnAE = 2;
 
 - (void)windowLayeringDidChange:(NSNotification*)inNotifiction
 {
-  // The key window isn't set until after this notification is processed, so we have to delay the actual
-  // menu update.
-  [self performSelectorOnMainThread:@selector(adjustBookmarksMenuItemsEnabling) withObject:nil waitUntilDone:NO];
+  [self adjustBookmarksMenuItemsEnabling];
 }
 
 - (NSMenu *)applicationDockMenu:(NSApplication *)sender
@@ -810,6 +816,14 @@ static OSStatus MenuEventHandler(EventHandlerCallRef inHandlerCallRef, EventRef
                                  menuEventList, (void*)self, NULL);
 }
 
+
+- (void)adjustBookmarksMenuItemsEnabling
+{
+  // we do this after a delay to ensure that window layer state has been set by the time
+  // we do the enabling.
+  [self performSelectorOnMainThread:@selector(doBookmarksMenuEnabling) withObject:nil waitUntilDone:NO];
+}
+
 //
 // -adjustBookmarksMenuItemsEnabling
 //
@@ -817,35 +831,21 @@ static OSStatus MenuEventHandler(EventHandlerCallRef inHandlerCallRef, EventRef
 // number of bookmarks in the list so we have to manage it manually. This routine
 // should be called whenever a window goes away, becomes main, or is no longer main.
 //
-- (void)adjustBookmarksMenuItemsEnabling
+- (void)doBookmarksMenuEnabling
 {
-  NSResponder* firstResponder = [[NSApp keyWindow] firstResponder];
-  [mAddBookmarkMenuItem               setEnabled:([firstResponder responderForAction:[mAddBookmarkMenuItem action]] != nil)];
-  [mCreateBookmarksFolderMenuItem     setEnabled:([firstResponder responderForAction:[mCreateBookmarksFolderMenuItem action]] != nil)];
-  [mCreateBookmarksSeparatorMenuItem  setEnabled:([firstResponder responderForAction:[mCreateBookmarksSeparatorMenuItem action]] != nil)];
+  // update our stand-in menu by hand (because it doesn't get autoupdated)
+  [mBookmarksHelperMenu update];
 
-  BOOL browserWindowIsMain  = [[[NSApp mainWindow] delegate] isMemberOfClass:[BrowserWindowController class]];
-  BOOL showBookmarksEnabled = [self bookmarksItemsEnabled];
-  BOOL useShowLabel = YES;
-  if (browserWindowIsMain)
-  {
-    BrowserWindowController* browserController = (BrowserWindowController*)[[NSApp mainWindow] delegate];
-    if ([browserController bookmarkManagerIsVisible])
-    {
-      useShowLabel = NO;
-      showBookmarksEnabled = [browserController canHideBookmarks];
-    }    
-    else
-    {
-      useShowLabel = YES;
-    }
-  }
+  [mAddBookmarkMenuItem               takeStateFromItem:[mBookmarksHelperMenu itemWithTarget:[mAddBookmarkMenuItem target]
+                                                                                   andAction:[mAddBookmarkMenuItem action]]];
+  [mCreateBookmarksFolderMenuItem     takeStateFromItem:[mBookmarksHelperMenu itemWithTarget:[mCreateBookmarksFolderMenuItem target]
+                                                                                   andAction:[mCreateBookmarksFolderMenuItem action]]];
+  [mCreateBookmarksSeparatorMenuItem  takeStateFromItem:[mBookmarksHelperMenu itemWithTarget:[mCreateBookmarksSeparatorMenuItem target]
+                                                                                   andAction:[mCreateBookmarksSeparatorMenuItem action]]];
+  [mShowAllBookmarksMenuItem          takeStateFromItem:[mBookmarksHelperMenu itemWithTarget:[mShowAllBookmarksMenuItem target]
+                                                                                   andAction:[mShowAllBookmarksMenuItem action]]];
 
-  NSString* showBMLabel = useShowLabel ? NSLocalizedString(@"Show All Bookmarks", @"")
-                                       : NSLocalizedString(@"Hide All Bookmarks", @"");
-                                       
-  [mShowAllBookmarksMenuItem setTitle:showBMLabel];
-  [mShowAllBookmarksMenuItem setEnabled:showBookmarksEnabled];
+  // We enable bookmark items themselves from the carbon event handler that fires before the menu is shown.
 }
 
 - (NSMenu*)bookmarksMenu
@@ -855,34 +855,9 @@ static OSStatus MenuEventHandler(EventHandlerCallRef inHandlerCallRef, EventRef
 
 - (BOOL)bookmarksItemsEnabled
 {
-  BOOL enableItems = YES;
-  
-  // NSLog(@"Main window %@, key window %@", [NSApp mainWindow], [NSApp keyWindow]);
-  
-  // I can't help thinking that there's an easier way, via NSResponder-type logic
-  // XXX this isn't quite right yet. It disables stuff when the toolbar customization
-  // sheet is up, which is unnecessary.
-  NSEnumerator* windowEnum = [[NSApp windows] objectEnumerator];
-  NSWindow* curWindow;
-  while ((curWindow = [windowEnum nextObject]))
-  {
-    if (![curWindow isVisible])
-      continue;
-
-    if ([curWindow level] == NSModalPanelWindowLevel)
-    {
-      enableItems = NO;
-      break;
-    }
-    
-    if ([curWindow isSheet])
-    {
-      enableItems = NO;
-      break;
-    }
-  }
-    
-  return enableItems;
+  // since this menu is not in the menu bar, we have to update it by hand
+  [mBookmarksHelperMenu update];
+  return [[mBookmarksHelperMenu itemWithTarget:self andAction:@selector(openMenuBookmark:)] isEnabled];
 }
 
 - (void)adjustBookmarkMenuItems
@@ -1079,7 +1054,7 @@ static OSStatus MenuEventHandler(EventHandlerCallRef inHandlerCallRef, EventRef
 //
 // toggle the bookmark manager (creating a new window if needed)
 //
--(IBAction)manageBookmarks: (id)aSender
+-(IBAction)manageBookmarks:(id)aSender
 {
   NSWindow* browserWindow = [self getFrontmostBrowserWindow];
   if (!browserWindow) {
@@ -1090,7 +1065,7 @@ static OSStatus MenuEventHandler(EventHandlerCallRef inHandlerCallRef, EventRef
   [[browserWindow windowController] manageBookmarks: aSender];
 }
 
-- (void)displayPreferencesWindow:sender
+- (void)displayPreferencesWindow:(id)sender
 {
   [[MVPreferencesController sharedInstance] showPreferences:nil];
 }
@@ -1171,7 +1146,7 @@ static OSStatus MenuEventHandler(EventHandlerCallRef inHandlerCallRef, EventRef
   }
 }
 
--(BOOL)validateMenuItem: (NSMenuItem*)aMenuItem
+-(BOOL)validateMenuItem:(NSMenuItem*)aMenuItem
 {
   BrowserWindowController* browserController = [self getMainWindowBrowserController];
 
@@ -1258,9 +1233,11 @@ static OSStatus MenuEventHandler(EventHandlerCallRef inHandlerCallRef, EventRef
   {
     return (browserController && [[browserController getTabBrowser] numberOfTabViewItems] > 1);
   }
-
+  
+#if 0
   if (action == @selector(addBookmark:))
     return (browserController && ![[browserController getBrowserWrapper] isEmpty]);
+#endif
   
   if (action == @selector(biggerTextSize:))
     return (browserController &&
@@ -1291,7 +1268,17 @@ static OSStatus MenuEventHandler(EventHandlerCallRef inHandlerCallRef, EventRef
       return NO;
   }
   
-  if (action == @selector(sendURL:)) {
+  if (action == @selector(manageBookmarks:))
+  {
+    BOOL showingBookmarks = (browserController && [browserController bookmarkManagerIsVisible]);
+    NSString* showBMLabel = showingBookmarks ? NSLocalizedString(@"Hide All Bookmarks", @"")
+                                             : NSLocalizedString(@"Show All Bookmarks", @"");
+    [aMenuItem setTitle:showBMLabel];
+    return showingBookmarks ? [browserController canHideBookmarks] : YES;
+  }
+  
+  if (action == @selector(sendURL:))
+  {
     NSString* titleString = nil;
     NSString* urlString = nil;
     [[[self getMainWindowBrowserController] getBrowserWrapper] getTitle:&titleString andHref:&urlString];

camino/src/browser/BrowserWindowController.mm

@@ -2265,7 +2265,7 @@ enum BWCOpenDest {
   }
 }
 
-- (void)createNewTab:(ENewTabContents)contents;
+- (void)createNewTab:(ENewTabContents)contents
 {
     BrowserTabViewItem* newTab  = [self createNewTabItem];
     BrowserWrapper*     newView = [newTab view];
@@ -2317,8 +2317,6 @@ enum BWCOpenDest {
   {
     [[[mTabBrowser selectedTabViewItem] view] windowClosed];
     [mTabBrowser removeTabViewItem:[mTabBrowser selectedTabViewItem]];
-
-    [[NSApp delegate] adjustBookmarksMenuItemsEnabling];
   }
 }
 
@@ -2336,6 +2334,8 @@ enum BWCOpenDest {
     [mTabBrowser selectFirstTabViewItem:sender];
   else
     [mTabBrowser selectNextTabViewItem:sender];
+
+  [[NSApp delegate] adjustBookmarksMenuItemsEnabling];
 }
 
 - (IBAction)closeSendersTab:(id)sender
@@ -2433,6 +2433,8 @@ enum BWCOpenDest {
 
   if (![self userChangedLocationField] && [[self window] isKeyWindow])
     [mBrowserView setBrowserActive:YES];
+
+  [[NSApp delegate] adjustBookmarksMenuItemsEnabling];
 }
 
 - (void)tabView:(NSTabView *)aTabView willSelectTabViewItem:(NSTabViewItem *)aTabViewItem
@@ -2506,7 +2508,7 @@ enum BWCOpenDest {
 // open a new tab, but doesn't load anything into it. Must be matched
 // with a call to do that.
 //
-- (BrowserTabViewItem*)openNewTab:(BOOL)aLoadInBG;
+- (BrowserTabViewItem*)openNewTab:(BOOL)aLoadInBG
 {
   BrowserTabViewItem* newTab  = [self createNewTabItem];
 
@@ -2534,13 +2536,13 @@ enum BWCOpenDest {
   return newTab;
 }
 
--(void)openNewWindowWithDescriptor:(nsISupports*)aDesc displayType:(PRUint32)aDisplayType loadInBackground:(BOOL)aLoadInBG;
+-(void)openNewWindowWithDescriptor:(nsISupports*)aDesc displayType:(PRUint32)aDisplayType loadInBackground:(BOOL)aLoadInBG
 {
   BrowserWindowController* browser = [self openNewWindow:aLoadInBG];
   [[[browser getBrowserWrapper] getBrowserView] setPageDescriptor:aDesc displayType:aDisplayType];
 }
 
--(void)openNewTabWithDescriptor:(nsISupports*)aDesc displayType:(PRUint32)aDisplayType loadInBackground:(BOOL)aLoadInBG;
+-(void)openNewTabWithDescriptor:(nsISupports*)aDesc displayType:(PRUint32)aDisplayType loadInBackground:(BOOL)aLoadInBG
 {
   BrowserTabViewItem* newTab = [self openNewTab:aLoadInBG];
   [[[newTab view] getBrowserView] setPageDescriptor:aDesc displayType:aDisplayType];
@@ -3107,7 +3109,7 @@ enum BWCOpenDest {
   return sInsecureIcon;
 }
 
-+ (NSImage*) secureIcon;
++ (NSImage*) secureIcon
 {
   static NSImage* sSecureIcon = nil;
   if (!sSecureIcon)
@@ -3115,7 +3117,7 @@ enum BWCOpenDest {
   return sSecureIcon;
 }
 
-+ (NSImage*) brokenIcon;
++ (NSImage*) brokenIcon
 {
   static NSImage* sBrokenIcon = nil;
   if (!sBrokenIcon)
@@ -3282,7 +3284,6 @@ enum BWCOpenDest {
   return handled;
 }
 
-
 @end
 
 #pragma mark -

camino/src/extensions/NSMenu+Utils.h

@@ -19,7 +19,7 @@
  * the Initial Developer. All Rights Reserved.
  *
  * Contributor(s):
- *   Simon Fraser <sfraser@netscape.com>
+ *   Simon Fraser <smfr@smfr.org>
  *
  * Alternatively, the contents of this file may be used under the terms of
  * either the GNU General Public License Version 2 or later (the "GPL"), or
@@ -50,6 +50,9 @@
 // optionally recursing into submenus.
 - (void)setAllItemsEnabled:(BOOL)inEnable startingWithItemAtIndex:(int)inFirstItem includingSubmenus:(BOOL)includeSubmenus;
 
+// return the first item (if any) with the given target and action.
+- (id<NSMenuItem>)itemWithTarget:(id)anObject andAction:(SEL)actionSelector;
+
 @end
 
 
@@ -57,4 +60,7 @@
 
 - (int)tagRemovingMask:(int)tagMask;
 
+// copy the title and enabled state from the given item
+- (void)takeStateFromItem:(id<NSMenuItem>)inItem;
+
 @end

camino/src/extensions/NSMenu+Utils.m

@@ -35,8 +35,9 @@
  *
  * ***** END LICENSE BLOCK ***** */
 
-#import "NSMenu+Utils.h"
+#import "NSResponder+Utils.h"
 
+#import "NSMenu+Utils.h"
 
 @implementation NSMenu(ChimeraMenuUtils)
 
@@ -71,7 +72,7 @@
 {
   NSArray* menuItems = [self itemArray];
 
-  int i;
+  unsigned int i;
   for (i = inFirstItem; i < [menuItems count]; i ++)
   {
     id<NSMenuItem> curItem = [self itemAtIndex:i];
@@ -83,6 +84,12 @@
   }
 }
 
+- (id<NSMenuItem>)itemWithTarget:(id)anObject andAction:(SEL)actionSelector
+{
+  int itemIndex = [self indexOfItemWithTarget:anObject andAction:actionSelector];
+  return [self itemAtIndex:itemIndex];
+}
+
 @end
 
 
@@ -93,4 +100,10 @@
   return ([self tag] & ~tagMask);
 }
 
+- (void)takeStateFromItem:(id<NSMenuItem>)inItem
+{
+  [self setTitle:[inItem title]];
+  [self setEnabled:[inItem isEnabled]];
+}
+
 @end