From f850cbf7807ed3bc85aa99fe8a38fed314d34fb0 Mon Sep 17 00:00:00 2001
From: Francisco Ryan Tolmasky I <francisco@280north.com>
Date: Mon, 30 Nov 2009 22:30:41 -0800
Subject: [PATCH] Better handling of submenu actions.

Reviewed by me.
---
 AppKit/CPMenu/CPMenu.j         | 39 +++++++++++++++++++---------------
 AppKit/CPMenuItem/CPMenuItem.j | 38 ++++++++++++++++++++++++---------
 AppKit/CPPopUpButton.j         |  8 +------
 3 files changed, 51 insertions(+), 34 deletions(-)

diff --git a/AppKit/CPMenu/CPMenu.j b/AppKit/CPMenu/CPMenu.j
index 8d5b01e26..38d354fd7 100644
--- a/AppKit/CPMenu/CPMenu.j
+++ b/AppKit/CPMenu/CPMenu.j
@@ -691,12 +691,28 @@ var aFont = nil;
         [delegate menuWillOpen:aMenu];
 
     [menuWindow orderFront:self];
-    [menuWindow beginTrackingWithEvent:[CPApp currentEvent] callback:function(aMenuWindow, aMenu)
+    [menuWindow
+        beginTrackingWithEvent:[CPApp currentEvent]
+                      callback:[CPMenu trackingCallbackWithCallback:aCallback]];
+}
+
++ (Function)trackingCallbackWithCallback:(Function)aCallback
+{
+    return function(aMenuWindow, aMenu)
     {
         [_CPMenuWindow poolMenuWindow:aMenuWindow];
 
-        aCallback(aMenu);
-    }];
+        if (aCallback)
+            aCallback(aMenu);
+
+        var highlightedItem = [aMenu highlightedItem];
+
+        while ([highlightedItem submenu] && [highlightedItem action] === @selector(submenuAction:))
+            highlightedItem = [[highlightedItem submenu] highlightedItem];
+
+        if (highlightedItem)
+            [CPApp sendAction:[highlightedItem action] to:[highlightedItem target] from:highlightedItem];
+    }
 }
 
 + (void)popUpContextMenu:(CPMenu)aMenu withEvent:(CPEvent)anEvent forView:(CPView)aView
@@ -729,20 +745,9 @@ var aFont = nil;
     [menuWindow setFrameOrigin:[[anEvent window] convertBaseToGlobal:[anEvent locationInWindow]]];
 
     [menuWindow orderFront:self];
-    [menuWindow beginTrackingWithEvent:anEvent callback:function(aMenuWindow, aMenu)
-    {
-        [_CPMenuWindow poolMenuWindow:aMenuWindow];
-
-        aCallback(aMenu);
-
-        var highlightedItem = [aMenu highlightedItem];
-
-        while ([highlightedItem submenu] && [highlightedItem action] === @selector(submenuAction:))
-            highlightedItem = [[highlightedItem submenu] highlightedItem];
-
-        if (highlightedItem)
-            [CPApp sendAction:[highlightedItem action] to:[highlightedItem target] from:highlightedItem];
-    }];
+    [menuWindow
+        beginTrackingWithEvent:anEvent
+                      callback:[CPMenu trackingCallbackWithCallback:nil]];
 }
 
 // Managing Display of State Column
diff --git a/AppKit/CPMenuItem/CPMenuItem.j b/AppKit/CPMenuItem/CPMenuItem.j
index 0aa3b7ad2..1472571b0 100644
--- a/AppKit/CPMenuItem/CPMenuItem.j
+++ b/AppKit/CPMenuItem/CPMenuItem.j
@@ -450,19 +450,31 @@ CPOffState
 - (void)setSubmenu:(CPMenu)aMenu
 {
     var supermenu = [_submenu supermenu];
-    
-    if (supermenu == self)
+
+    if (supermenu === self)
         return;
-    
+
     if (supermenu)
-        return alert("bad");
-    
-    [_submenu setSupermenu:_menu];
-    
+        [CPException raise:CPInvalidArgumentException
+		   reason: @"Can't add submenu \"" + [aMenu title] + "\" to item \"" + [self title] + "\", because it is already submenu of \"" + [[aMenu supermenu] title] + "\""];
+
     _submenu = aMenu;
-    
+
+    if (_submenu)
+    {
+        [_submenu setSupermenu:_menu];
+
+        [self setTarget:_menu];
+        [self setAction:@selector(submenuAction:)];
+    }
+    else
+    {
+        [self setTarget:nil];
+        [self setAction:NULL];
+    }
+
     [_menuItemView setDirty];
-    
+
     [_menu itemChanged:self];
 }
 
@@ -762,6 +774,11 @@ CPControlKeyMask
     return _menuItemView;
 }
 
+- (BOOL)_isSelectable
+{
+    return ![self submenu] || [self action] !== @selector(submenuAction:) || [self target] !== [self menu];
+}
+
 @end
 
 var CPMenuItemIsSeparatorKey                = @"CPMenuItemIsSeparatorKey",
@@ -824,8 +841,9 @@ var CPMenuItemIsSeparatorKey                = @"CPMenuItemIsSeparatorKey",
 //    CPImage         _offStateImage;
 //    CPImage         _mixedStateImage;
 
-        _submenu = DEFAULT_VALUE(CPMenuItemSubmenuKey, nil);
+        // This order matters because setSubmenu: needs _menu to be around.
         _menu = DEFAULT_VALUE(CPMenuItemMenuKey, nil);
+        [self setSubmenu:DEFAULT_VALUE(CPMenuItemSubmenuKey, nil)];
 
         _keyEquivalent = [aCoder decodeObjectForKey:CPMenuItemKeyEquivalentKey] || @"";
         _keyEquivalentModifierMask = [aCoder decodeObjectForKey:CPMenuItemKeyEquivalentModifierMaskKey] || 0;
diff --git a/AppKit/CPPopUpButton.j b/AppKit/CPPopUpButton.j
index c170d5ff4..00cbff004 100644
--- a/AppKit/CPPopUpButton.j
+++ b/AppKit/CPPopUpButton.j
@@ -664,14 +664,8 @@ CPPopUpButtonStatePullsDown = CPThemeState("pulls-down");
 
             var highlightedItem = [aMenu highlightedItem];
 
-            while ([highlightedItem submenu] && [highlightedItem action] === @selector(submenuAction:))
-                highlightedItem = [[highlightedItem submenu] highlightedItem];
-
-            if ([highlightedItem menu] === [self menu])
+            if ([highlightedItem _isSelectable])
                 [self selectItem:highlightedItem];
-
-            if (highlightedItem)
-                [CPApp sendAction:[highlightedItem action] to:[highlightedItem target] from:highlightedItem];
         }];
 /*
     var menu = [self menu],