From 2e66601c19faf21416671a88886a00fa2dbd9bf3 Mon Sep 17 00:00:00 2001 From: Alexandre Wilhelm Date: Mon, 1 Dec 2014 14:22:39 -0800 Subject: [PATCH] Fixed: memory leak in CPComboBox Previously, when assigning a listDelegate to a CPComboBox, we never deleted the observers added by this method. Now we add and delete these observers in the method addObservers and removeObservers. This PR fix another issue. Previously, the behavior of the panel of the comboBox wasn't the same as the one in Cocoa. Now, when closing the panel, the hit view won't be the first responder as it was. Cocoa works like this as well. --- AppKit/CPComboBox.j | 100 +++++++++++++++++++++++++++++------------- AppKit/_CPPopUpList.j | 34 +++++++++----- 2 files changed, 94 insertions(+), 40 deletions(-) diff --git a/AppKit/CPComboBox.j b/AppKit/CPComboBox.j index 844149d4b..f313e38de 100644 --- a/AppKit/CPComboBox.j +++ b/AppKit/CPComboBox.j @@ -160,7 +160,7 @@ var CPComboBoxTextSubview = @"text", - (void)setIntercellSpacing:(CGSize)aSize { - if (_intercellSpacing && CGSizeEqualToSize(aSize, _intercellSpacing)) + if (!aSize || (_intercellSpacing && CGSizeEqualToSize(aSize, _intercellSpacing))) return; _intercellSpacing = aSize; @@ -396,49 +396,53 @@ var CPComboBoxTextSubview = @"text", if (_listDelegate === aDelegate) return; - var defaultCenter = [CPNotificationCenter defaultCenter]; - - if (_listDelegate) - { - [defaultCenter removeObserver:self name:_CPPopUpListWillPopUpNotification object:_listDelegate]; - [defaultCenter removeObserver:self name:_CPPopUpListWillDismissNotification object:_listDelegate]; - [defaultCenter removeObserver:self name:_CPPopUpListDidDismissNotification object:_listDelegate]; - [defaultCenter removeObserver:self name:_CPPopUpListItemWasClickedNotification object:_listDelegate]; - - var oldTableView = [_listDelegate tableView]; - - if (oldTableView) - { - [defaultCenter removeObserver:self name:CPTableViewSelectionIsChangingNotification object:oldTableView]; - [defaultCenter removeObserver:self name:CPTableViewSelectionDidChangeNotification object:oldTableView]; - } - } + [self _removeObserversForListDelegate:_listDelegate]; _listDelegate = aDelegate; + // We only add the observers if the CPComboBox is displayed + if ([self window]) + [self _addObserversForListDelegate:_listDelegate] + + // Apply our text style to the list + [_listDelegate setFont:[self font]]; + [_listDelegate setAlignment:[self alignment]]; + + [self setHasVerticalScroller:_hasVerticalScroller]; + [self setIntercellSpacing:_intercellSpacing]; + [self setItemHeight:_itemHeight]; +} + +- (void)_addObserversForListDelegate:(_CPPopUpList)aDelegate +{ + if (!aDelegate) + return; + + var defaultCenter = [CPNotificationCenter defaultCenter]; + [defaultCenter addObserver:self selector:@selector(comboBoxWillPopUp:) name:_CPPopUpListWillPopUpNotification - object:_listDelegate]; + object:aDelegate]; [defaultCenter addObserver:self selector:@selector(comboBoxWillDismiss:) name:_CPPopUpListWillDismissNotification - object:_listDelegate]; + object:aDelegate]; [defaultCenter addObserver:self selector:@selector(listDidDismiss:) name:_CPPopUpListDidDismissNotification - object:_listDelegate]; + object:aDelegate]; [defaultCenter addObserver:self selector:@selector(itemWasClicked:) name:_CPPopUpListItemWasClickedNotification - object:_listDelegate]; + object:aDelegate]; - [[_listDelegate scrollView] setHasVerticalScroller:_hasVerticalScroller]; + [[aDelegate scrollView] setHasVerticalScroller:_hasVerticalScroller]; - var tableView = [_listDelegate tableView]; + var tableView = [aDelegate tableView]; [defaultCenter addObserver:self selector:@selector(comboBoxSelectionIsChanging:) @@ -449,13 +453,27 @@ var CPComboBoxTextSubview = @"text", selector:@selector(comboBoxSelectionDidChange:) name:CPTableViewSelectionDidChangeNotification object:tableView]; +} - // Apply our text style to the list - [_listDelegate setFont:[self font]]; - [_listDelegate setAlignment:[self alignment]]; - [[_listDelegate scrollView] setHasVerticalScroller:_hasVerticalScroller]; - [[_listDelegate tableView] setIntercellSpacing:_intercellSpacing]; - [[_listDelegate tableView] setRowHeight:_itemHeight]; +- (void)_removeObserversForListDelegate:(_CPPopUpList)aDelegate +{ + if (!aDelegate) + return; + + var defaultCenter = [CPNotificationCenter defaultCenter]; + + [defaultCenter removeObserver:self name:_CPPopUpListWillPopUpNotification object:aDelegate]; + [defaultCenter removeObserver:self name:_CPPopUpListWillDismissNotification object:aDelegate]; + [defaultCenter removeObserver:self name:_CPPopUpListDidDismissNotification object:aDelegate]; + [defaultCenter removeObserver:self name:_CPPopUpListItemWasClickedNotification object:aDelegate]; + + var oldTableView = [aDelegate tableView]; + + if (oldTableView) + { + [defaultCenter removeObserver:self name:CPTableViewSelectionIsChangingNotification object:oldTableView]; + [defaultCenter removeObserver:self name:CPTableViewSelectionDidChangeNotification object:oldTableView]; + } } - (int)indexOfItemWithObjectValue:(id)anObject @@ -982,6 +1000,28 @@ var CPComboBoxTextSubview = @"text", } } + +#pragma mark - +#pragma mark Observers method + +- (void)_addObservers +{ + if (_isObserving) + return; + + [super _addObservers]; + [self _addObserversForListDelegate:_listDelegate]; +} + +- (void)_removeObservers +{ + if (!_isObserving) + return; + + [super _removeObservers]; + [self _removeObserversForListDelegate:_listDelegate]; +} + @end @implementation CPComboBox (CPComboBoxDelegate) diff --git a/AppKit/_CPPopUpList.j b/AppKit/_CPPopUpList.j index 3dd357132..845768bd6 100644 --- a/AppKit/_CPPopUpList.j +++ b/AppKit/_CPPopUpList.j @@ -96,28 +96,41 @@ var ListColumnIdentifier = @"1"; return [super sendEvent:anEvent]; } -- (void)orderFront:(id)sender -{ - [self _trapNextMouseDown]; - [super orderFront:sender]; -} - - (void)_mouseWasClicked:(CPEvent)anEvent { + // This is needed, when the user close the list with the key enter + if (![self isVisible]) + { + [CPApp sendEvent:anEvent]; + return; + } + var mouseWindow = [anEvent window], - rect = [[[self delegate] dataSource] bounds], + rect = CGRectInsetByInset([[[self delegate] dataSource] bounds], [[[self delegate] dataSource] currentValueForThemeAttribute:@"content-inset"]), point = [[[self delegate] dataSource] convertPoint:[anEvent locationInWindow] fromView:nil]; + // If we click somewhere else than the comboBox or the panel we close the panel if (mouseWindow != self && !CGRectContainsPoint(rect, point)) + { [[self delegate] close]; + } else - [self _trapNextMouseDown]; + { + // If we click on the panel, the app will know what to do + if (mouseWindow == self) + [CPApp sendEvent:anEvent]; + + // If we click on the comboBox field, we will trap the next mouse down + if (CGRectContainsPoint(rect, point)) + [self _trapNextMouseDown]; + } + } - (void)_trapNextMouseDown { - // Don't dequeue the event so clicks in controls will work - [CPApp setTarget:self selector:@selector(_mouseWasClicked:) forNextEventMatchingMask:CPLeftMouseDownMask untilDate:nil inMode:CPDefaultRunLoopMode dequeue:NO]; + // Dequeue the event and mouseWasClicked will do what it needs to do + [CPApp setTarget:self selector:@selector(_mouseWasClicked:) forNextEventMatchingMask:CPLeftMouseDownMask untilDate:nil inMode:CPDefaultRunLoopMode dequeue:YES]; } @end @@ -336,6 +349,7 @@ var ListColumnIdentifier = @"1"; [self listWillPopUp]; + [_panel _trapNextMouseDown]; [[aView window] addChildWindow:_panel ordered:CPWindowAbove]; }