From cc9ff15142b41759691d06b50ca4bd91d8bab1e8 Mon Sep 17 00:00:00 2001 From: Andrew Hankinson Date: Wed, 10 Apr 2013 00:04:53 -0400 Subject: [PATCH 1/2] 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 | 11 +++++ AppKit/CPMenuItem/_CPMenuItemStandardView.j | 5 +++ AppKit/CPMenuItem/_CPMenuItemView.j | 2 +- Tests/AppKit/CPMenuTest.j | 47 +++++++++++++++++++++ 4 files changed, 64 insertions(+), 1 deletion(-) diff --git a/AppKit/CPMenu/CPMenu.j b/AppKit/CPMenu/CPMenu.j index 58630f9ab..f7dbb5ca7 100644 --- a/AppKit/CPMenu/CPMenu.j +++ b/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]; diff --git a/AppKit/CPMenuItem/_CPMenuItemStandardView.j b/AppKit/CPMenuItem/_CPMenuItemStandardView.j index 5a71538b2..4eaca118d 100644 --- a/AppKit/CPMenuItem/_CPMenuItemStandardView.j +++ b/AppKit/CPMenuItem/_CPMenuItemStandardView.j @@ -336,6 +336,11 @@ } } +- (BOOL)isHighlighted +{ + return _highlighted; +} + @end @implementation _CPMenuItemSubmenuIndicatorView : CPView diff --git a/AppKit/CPMenuItem/_CPMenuItemView.j b/AppKit/CPMenuItem/_CPMenuItemView.j index 9dc244b44..46d7a62f4 100644 --- a/AppKit/CPMenuItem/_CPMenuItemView.j +++ b/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; diff --git a/Tests/AppKit/CPMenuTest.j b/Tests/AppKit/CPMenuTest.j index 63a7e410a..ad1ef953e 100644 --- a/Tests/AppKit/CPMenuTest.j +++ b/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]; From efa2918ce91fc1841b7406deed104b934205458a Mon Sep 17 00:00:00 2001 From: Andrew Hankinson Date: Sun, 14 Apr 2013 15:40:35 -0400 Subject: [PATCH 2/2] Change how menu items are highlighted This replaces the previous solution with one proposed by @BlairDuncan. It uses the _highlightItemAtIndex: method to maintain the highlight state, rather than setting the _highlightIndex variable directly. --- AppKit/CPMenu/CPMenu.j | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/AppKit/CPMenu/CPMenu.j b/AppKit/CPMenu/CPMenu.j index f7dbb5ca7..0cbf4f763 100644 --- a/AppKit/CPMenu/CPMenu.j +++ b/AppKit/CPMenu/CPMenu.j @@ -328,9 +328,6 @@ var _CPMenuBarVisible = NO, */ - (void)removeItem:(CPMenuItem)aMenuItem { - if ([aMenuItem isHighlighted]) - [[aMenuItem _menuItemView] highlight:NO]; - [self removeItemAtIndex:[_items indexOfObjectIdenticalTo:aMenuItem]]; } @@ -358,12 +355,7 @@ 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; + [self _highlightItemAtIndex:CPNotFound]; // Because we are changing _items directly, be sure to notify KVO [self willChangeValueForKey:@"items"]; @@ -1159,6 +1151,7 @@ var _CPMenuBarVisible = NO, return; [aMenuItem setMenu:self]; + [self _highlightItemAtIndex:CPNotFound]; [_items insertObject:aMenuItem atIndex:anIndex]; [[CPNotificationCenter defaultCenter] @@ -1172,10 +1165,8 @@ var _CPMenuBarVisible = NO, if (anIndex < 0 || anIndex >= [_items count]) return; - if (_highlightedIndex === anIndex) - [[[_items objectAtIndex:anIndex] _menuItemView] highlight:NO]; - [[_items objectAtIndex:anIndex] setMenu:nil]; + [self _highlightItemAtIndex:CPNotFound]; [_items removeObjectAtIndex:anIndex]; [[CPNotificationCenter defaultCenter]