Fixed: Removing an item from a menu also removes its highlight state

Previously when a menu item was instantiated outside of the context of the menu itself, its highlight state was maintained even after it was removed from the menu. If the menu was dismissed with the item highlighted, and then the item was used in another menu (e.g., re-added to a context menu for a table row) it would appear highlighted, even though the menu highlight index was not set.

This commit ensures that when removing items from a menu that the highlight state of the underlying view is set to NO as well.

Tests for this behaviour are included as well.

Fixes #1899

AppKit/CPMenu/CPMenu.j

@@ -328,6 +328,9 @@ var _CPMenuBarVisible               = NO,
 */
 - (void)removeItem:(CPMenuItem)aMenuItem
 {
+    if ([aMenuItem isHighlighted])
+        [[aMenuItem _menuItemView] highlight:NO];
+
     [self removeItemAtIndex:[_items indexOfObjectIdenticalTo:aMenuItem]];
 }
 
@@ -355,6 +358,11 @@ var _CPMenuBarVisible               = NO,
     while (count--)
         [_items[count] setMenu:nil];
 
+    // See Issue 1899. Ensure the highlight state of the underlying
+    // _CPMenuItemView is set to NO as well.
+    if (_highlightedIndex !== CPNotFound)
+        [[_items[_highlightedIndex] _menuItemView] highlight:NO];
+
     _highlightedIndex = CPNotFound;
 
     // Because we are changing _items directly, be sure to notify KVO
@@ -1164,6 +1172,9 @@ var _CPMenuBarVisible               = NO,
     if (anIndex < 0 || anIndex >= [_items count])
         return;
 
+    if (_highlightedIndex === anIndex)
+        [[[_items objectAtIndex:anIndex] _menuItemView] highlight:NO];
+
     [[_items objectAtIndex:anIndex] setMenu:nil];
     [_items removeObjectAtIndex:anIndex];
 

AppKit/CPMenuItem/_CPMenuItemStandardView.j

@@ -336,6 +336,11 @@
     }
 }
 
+- (BOOL)isHighlighted
+{
+    return _highlighted;
+}
+
 @end
 
 @implementation _CPMenuItemSubmenuIndicatorView : CPView

AppKit/CPMenuItem/_CPMenuItemView.j

@@ -34,7 +34,7 @@
 @implementation _CPMenuItemView : CPView
 {
     CPMenuItem              _menuItem;
-    CPView                  _view;
+    CPView                  _view       @accessors(property=view, readonly);
 
     CPFont                  _font;
     CPColor                 _textColor;

Tests/AppKit/CPMenuTest.j

@@ -15,6 +15,8 @@
     BOOL    saveDocumentWasCalled;
     BOOL    saveDocumentAsWasCalled;
     BOOL    undoWasCalled;
+
+    CPMenuItem anInstantiatedMenuItem;
 }
 
 - (void)setUp
@@ -67,6 +69,10 @@
 
     [menu addItem:editMenuItem];
     [menu addItem:[CPMenuItem separatorItem]];
+
+    // Test Issue 1899
+    anInstantiatedMenuItem = [[CPMenuItem alloc] initWithTitle:@"Highlight" action:nil keyEquivalent:@""];
+    [menu addItem:anInstantiatedMenuItem];
 }
 
 - (void)_retarget:(CPMenuItem)aMenu
@@ -82,6 +88,47 @@
     }
 }
 
+- (void)testRemoveAllItemsHighlighting
+{
+    // hack it so that this menu item is highlighted
+    [menu _highlightItemAtIndex:[menu indexOfItem:anInstantiatedMenuItem]];
+
+    // test both the public isHighlighted method, as well as the underlying view highlighting
+    [self assertTrue:[anInstantiatedMenuItem isHighlighted]];
+    [self assertTrue:[[[anInstantiatedMenuItem _menuItemView] view] isHighlighted] message:@"Underlying view was not highlighted in removeAll"];
+
+    [menu removeAllItems];
+
+    [self assertFalse:[anInstantiatedMenuItem isHighlighted]];
+    [self assertFalse:[[[anInstantiatedMenuItem _menuItemView] view] isHighlighted] message:@"Underlying view was still highlighted after removeAll"];
+}
+
+- (void)testRemoveOneItemHighlighting
+{
+    [menu _highlightItemAtIndex:[menu indexOfItem:anInstantiatedMenuItem]];
+
+    [self assertTrue:[anInstantiatedMenuItem isHighlighted]];
+    [self assertTrue:[[[anInstantiatedMenuItem _menuItemView] view] isHighlighted] message:@"Underlying view was not highlighted in removeItem"];
+
+    [menu removeItem:anInstantiatedMenuItem];
+
+    [self assertFalse:[anInstantiatedMenuItem isHighlighted]];
+    [self assertFalse:[[[anInstantiatedMenuItem _menuItemView] view] isHighlighted] message:@"Underlying view was still highlighted after removeItem"];
+}
+
+- (void)testRemoveOneItemByIndexHighlighting
+{
+    [menu _highlightItemAtIndex:[menu indexOfItem:anInstantiatedMenuItem]];
+
+    [self assertTrue:[anInstantiatedMenuItem isHighlighted]];
+    [self assertTrue:[[[anInstantiatedMenuItem _menuItemView] view] isHighlighted] message:@"Underlying view was not highlighted in removeItemAtIndex"];
+
+    [menu removeItemAtIndex:[menu indexOfItem:anInstantiatedMenuItem]];
+
+    [self assertFalse:[anInstantiatedMenuItem isHighlighted]];
+    [self assertFalse:[[[anInstantiatedMenuItem _menuItemView] view] isHighlighted] message:@"Underlying view was still highlighted after removeItemAtIndex"];
+}
+
 - (void)testKeyEquivalent
 {
     [self _retarget:menu];