From 42c3c7bc48624a688a5903f831a8b312032073cd Mon Sep 17 00:00:00 2001 From: Alexandre Wilhelm Date: Tue, 25 Aug 2015 16:46:00 -0700 Subject: [PATCH 1/3] Fixed: crash when using a viewBased CPTableView with appearance Previously, when using a viewBased CPTableView Cappuccino just crashed. This was due to the new appearance feature. Indeed, at the end of the method initWithCoder: of CPView, we set the appearance of the current view and the themeStates of the view and its subviews. For an unknown reason some of these subviews were not completely initialized (see comment line 3742), this made crashed cappuccino because _themeState was null. To fix this issue, we send the method setAppearance when the current stack was performed. (Magic setTimeout...). This is for sure not the ultimate best fix, but it's prevent to have a broken master. We will need in the future to fix this issue in a better way. --- AppKit/CPView.j | 10 ++++++- AppKit/CPVisualEffectView.j | 53 +++---------------------------------- 2 files changed, 13 insertions(+), 50 deletions(-) diff --git a/AppKit/CPView.j b/AppKit/CPView.j index d40d1ad6f..ca467a1d0 100644 --- a/AppKit/CPView.j +++ b/AppKit/CPView.j @@ -3737,7 +3737,15 @@ var CPViewAutoresizingMaskKey = @"CPViewAutoresizingMask", _themeAttributes[attributeName] = CPThemeAttributeDecode(aCoder, attributeName, attributes[count], _theme, themeClass); } - [self setAppearance:[aCoder decodeObjectForKey:CPViewAppearanceKey]]; + var appearance = [aCoder decodeObjectForKey:CPViewAppearanceKey]; + + // TODO : find a way to fix this horrible things + // We only set the appearance here when everything and every subviews were unarchived. + // Otherwise this causes a crash with some subviews not unarchived but already created (how ? init is not called, neither initWithFrame or initWithCoder) in the coder (I have no idea why these views are present in _objects of the coder) + // To reproduce, uncomment the timeout and open the test Tests/Manual/TableTests/ViewBasedCib + window.setTimeout(function() { + [self setAppearance:appearance]; + },0); [self setNeedsDisplay:YES]; [self setNeedsLayout]; diff --git a/AppKit/CPVisualEffectView.j b/AppKit/CPVisualEffectView.j index ece566562..2618b611f 100644 --- a/AppKit/CPVisualEffectView.j +++ b/AppKit/CPVisualEffectView.j @@ -89,7 +89,8 @@ CPVisualEffectStateInactive = 2; [CPException raise:CPInvalidArgumentException reason:"Appearance can only be CPAppearanceNameVibrantDark or CPAppearanceNameVibrantLight in CPVisualEffectView, but is " + anAppearance]; [super setAppearance:anAppearance]; - [self _applyVibrancyState]; + + [self setNeedsLayout:YES]; } /*! Sets the received effect state. @@ -109,7 +110,7 @@ CPVisualEffectStateInactive = 2; _state = aState; [self didChangeValueForKey:"state"]; - [self _applyVibrancyState]; + [self setNeedsLayout:YES]; } @@ -131,7 +132,7 @@ CPVisualEffectStateInactive = 2; } -- (void)_applyVibrancyState +- (void)layoutSubviews { switch (_state) { @@ -155,52 +156,6 @@ CPVisualEffectStateInactive = 2; } -#pragma mark - -#pragma mark Overrides - -- (BOOL)setThemeState:(ThemeState)aState -{ - if (aState.isa && [aState isKindOfClass:CPArray]) - aState = CPThemeState.apply(null, aState); - - var r = [super setThemeState:aState]; - - if (r) - [self _applyVibrancyState]; - - return r; -} - -- (BOOL)unsetThemeState:(ThemeState)aState -{ - if (aState.isa && [aState isKindOfClass:CPArray]) - aState = CPThemeState.apply(null, aState); - - var r = [super unsetThemeState:aState]; - - if (r) - [self _applyVibrancyState]; - - return r; -} - -- (void)viewDidMoveToSuperview -{ - [super viewDidMoveToSuperview]; - - if (_superview) - [self _applyVibrancyState]; -} - -- (void)viewDidMoveToWindow -{ - [super viewDidMoveToWindow]; - - if (_window) - [self _applyVibrancyState]; -} - - #pragma mark - #pragma mark CPCoding From b30103c790bed4cbb259eaef6c98796c894b5d98 Mon Sep 17 00:00:00 2001 From: Alexandre Wilhelm Date: Fri, 28 Aug 2015 11:11:28 -0700 Subject: [PATCH 2/3] New: added methods viewWillLayout and viewDidLayout Previously, when a view was about to layout, only layoutSubviews was called. Now the methods viewWillLayout and viewDidLayout are called as well. This is like in Cocoa. This PR fixed the issue with the appearance and the view based tableView. We now handle everything related with appearance when making the layout of a view. This PR removed a small optimization in CPView. Now when making setNeedsLayout and setNeedsDisplay, the methods drawRect: and layoutSubviews are always called even if there are not override. (As in cocoa) --- AppKit/CPView.j | 77 ++++++++++++--------------------------- Tests/AppKit/CPViewTest.j | 11 +++++- 2 files changed, 33 insertions(+), 55 deletions(-) diff --git a/AppKit/CPView.j b/AppKit/CPView.j index ca467a1d0..82db0b74c 100644 --- a/AppKit/CPView.j +++ b/AppKit/CPView.j @@ -231,8 +231,6 @@ var CPViewHighDPIDrawingEnabled = YES; CPView _nextKeyView; CPView _previousKeyView; - unsigned _viewClassFlags; - // ToolTips CPString _toolTip @accessors(getter=toolTip); Function _toolTipFunctionIn; @@ -295,27 +293,6 @@ var CPViewHighDPIDrawingEnabled = YES; return CPViewHighDPIDrawingEnabled; } -- (void)_setupViewFlags -{ - var theClass = [self class], - classUID = [theClass UID]; - - if (CPViewFlags[classUID] === undefined) - { - var flags = 0; - - if ([theClass instanceMethodForSelector:@selector(drawRect:)] !== [CPView instanceMethodForSelector:@selector(drawRect:)]) - flags |= CPViewHasCustomDrawRect; - - if ([theClass instanceMethodForSelector:@selector(layoutSubviews)] !== [CPView instanceMethodForSelector:@selector(layoutSubviews)]) - flags |= CPViewHasCustomLayoutSubviews; - - CPViewFlags[classUID] = flags; - } - - _viewClassFlags = CPViewFlags[classUID]; -} - + (CPSet)keyPathsForValuesAffectingFrame { return [CPSet setWithObjects:@"frameOrigin", @"frameSize"]; @@ -331,12 +308,6 @@ var CPViewHighDPIDrawingEnabled = YES; return nil; } -- (void)awakeFromCib -{ - [super awakeFromCib]; - - [self _recomputeAppearance]; -} - (id)init { @@ -391,8 +362,6 @@ var CPViewHighDPIDrawingEnabled = YES; _DOMImageSizes = []; #endif - [self _setupViewFlags]; - [self _loadThemeAttributes]; } @@ -850,8 +819,7 @@ var CPViewHighDPIDrawingEnabled = YES; */ - (void)viewDidMoveToSuperview { - [self _recomputeAppearance]; - + [self setNeedsLayout:YES]; [self setNeedsDisplay:YES]; } @@ -2489,9 +2457,6 @@ setBoundsOrigin: */ - (void)setNeedsDisplayInRect:(CGRect)aRect { - if (!(_viewClassFlags & CPViewHasCustomDrawRect)) - return; - if (CGRectIsEmpty(aRect)) return; @@ -2632,7 +2597,7 @@ setBoundsOrigin: - (void)setNeedsLayout:(BOOL)needLayout { - if (!(_viewClassFlags & CPViewHasCustomLayoutSubviews) || !needLayout) + if (!needLayout) { _needsLayout = NO; return; @@ -2654,12 +2619,31 @@ setBoundsOrigin: { _needsLayout = NO; + [self viewWillLayout]; [self layoutSubviews]; + [self viewDidLayout]; } } +/*! + @ignore +*/ +- (void)viewWillLayout +{ + +} + +/*! + @ignore +*/ +- (void)viewDidLayout +{ + [self _recomputeAppearance]; +} + - (void)layoutSubviews { + } /*! @@ -3544,19 +3528,13 @@ setBoundsOrigin: _appearance = anAppearance; [self didChangeValueForKey:@"appearance"]; - [self _recomputeAppearance]; + [self setNeedsLayout:YES]; } /*! @ignore */ - (void)_recomputeAppearance { - // if we don't have a themeState, it means - // the view is not decoding from a cib, so we just return. - // this method will be called again in awakeFromCib - if (!_themeState) - return; - var effectiveAppearance = [self effectiveAppearance]; if ([effectiveAppearance isEqual:[CPAppearance appearanceNamed:CPAppearanceNameAqua]]) @@ -3718,7 +3696,6 @@ var CPViewAutoresizingMaskKey = @"CPViewAutoresizingMask", _opacity = 1.0; [self setBackgroundColor:[aCoder decodeObjectForKey:CPViewBackgroundColorKey]]; - [self _setupViewFlags]; _theme = [CPTheme defaultTheme]; _themeClass = [aCoder decodeObjectForKey:CPViewThemeClassKey]; @@ -3737,15 +3714,7 @@ var CPViewAutoresizingMaskKey = @"CPViewAutoresizingMask", _themeAttributes[attributeName] = CPThemeAttributeDecode(aCoder, attributeName, attributes[count], _theme, themeClass); } - var appearance = [aCoder decodeObjectForKey:CPViewAppearanceKey]; - - // TODO : find a way to fix this horrible things - // We only set the appearance here when everything and every subviews were unarchived. - // Otherwise this causes a crash with some subviews not unarchived but already created (how ? init is not called, neither initWithFrame or initWithCoder) in the coder (I have no idea why these views are present in _objects of the coder) - // To reproduce, uncomment the timeout and open the test Tests/Manual/TableTests/ViewBasedCib - window.setTimeout(function() { - [self setAppearance:appearance]; - },0); + [self setAppearance:[aCoder decodeObjectForKey:CPViewAppearanceKey]]; [self setNeedsDisplay:YES]; [self setNeedsLayout]; diff --git a/Tests/AppKit/CPViewTest.j b/Tests/AppKit/CPViewTest.j index b3f1b80e2..9c11b4de5 100644 --- a/Tests/AppKit/CPViewTest.j +++ b/Tests/AppKit/CPViewTest.j @@ -773,6 +773,8 @@ var methodCalled; - (void)testAppearanceDefaultvalue { + [[CPRunLoop currentRunLoop] limitDateForMode:CPDefaultRunLoopMode]; + [self assert:nil equals:[view appearance]]; [self assertFalse:[view hasThemeState:CPThemeStateAppearanceVibrantDark]]; [self assertFalse:[view hasThemeState:CPThemeStateAppearanceVibrantLight]]; @@ -782,9 +784,10 @@ var methodCalled; - (void)testAppearanceWithVibrantDark { [view setAppearance:[CPAppearance appearanceNamed:CPAppearanceNameVibrantDark]]; - [self assert:[CPAppearance appearanceNamed:CPAppearanceNameVibrantDark] equals:[view appearance]]; [self assert:[CPAppearance appearanceNamed:CPAppearanceNameVibrantDark] equals:[view effectiveAppearance]]; + + [[CPRunLoop currentRunLoop] limitDateForMode:CPDefaultRunLoopMode]; [self assertTrue:[view hasThemeState:CPThemeStateAppearanceVibrantDark]]; [self assertFalse:[view hasThemeState:CPThemeStateAppearanceVibrantLight]]; } @@ -795,6 +798,8 @@ var methodCalled; [self assert:[CPAppearance appearanceNamed:CPAppearanceNameVibrantLight] equals:[view appearance]]; [self assert:[CPAppearance appearanceNamed:CPAppearanceNameVibrantLight] equals:[view effectiveAppearance]]; + + [[CPRunLoop currentRunLoop] limitDateForMode:CPDefaultRunLoopMode]; [self assertTrue:[view hasThemeState:CPThemeStateAppearanceVibrantLight]]; [self assertFalse:[view hasThemeState:CPThemeStateAppearanceVibrantDark]]; } @@ -806,6 +811,8 @@ var methodCalled; [self assert:nil equals:[view appearance]]; [self assert:nil equals:[view effectiveAppearance]]; + + [[CPRunLoop currentRunLoop] limitDateForMode:CPDefaultRunLoopMode]; [self assertFalse:[view hasThemeState:CPThemeStateAppearanceVibrantLight]]; [self assertFalse:[view hasThemeState:CPThemeStateAppearanceVibrantDark]]; } @@ -833,12 +840,14 @@ var methodCalled; [viewA addSubview:view]; + [[CPRunLoop currentRunLoop] limitDateForMode:CPDefaultRunLoopMode]; [self assert:[CPAppearance appearanceNamed:CPAppearanceNameVibrantLight] equals:[view effectiveAppearance]]; [self assertTrue:[view hasThemeState:CPThemeStateAppearanceVibrantLight]]; [self assertFalse:[view hasThemeState:CPThemeStateAppearanceVibrantDark]]; [viewB addSubview:view]; + [[CPRunLoop currentRunLoop] limitDateForMode:CPDefaultRunLoopMode]; [self assert:[CPAppearance appearanceNamed:CPAppearanceNameVibrantDark] equals:[view effectiveAppearance]]; [self assertFalse:[view hasThemeState:CPThemeStateAppearanceVibrantLight]]; [self assertTrue:[view hasThemeState:CPThemeStateAppearanceVibrantDark]]; From f2f4fa1b98f625fab2dddd0c1e5b5c9b3ce6c64e Mon Sep 17 00:00:00 2001 From: Alexandre Wilhelm Date: Wed, 23 Sep 2015 15:00:40 -0700 Subject: [PATCH 3/3] Fixed: readded _viewClassFlags. We only displayRect the view if drawRect: or viewWillDraw is implemented. layoutSubviews is still called even if the subclass does not implement it --- AppKit/CPView.j | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/AppKit/CPView.j b/AppKit/CPView.j index e1b0fcaa2..2821a8147 100644 --- a/AppKit/CPView.j +++ b/AppKit/CPView.j @@ -231,6 +231,8 @@ var CPViewHighDPIDrawingEnabled = YES; CPView _nextKeyView; CPView _previousKeyView; + unsigned _viewClassFlags; + // ToolTips CPString _toolTip @accessors(getter=toolTip); Function _toolTipFunctionIn; @@ -308,6 +310,24 @@ var CPViewHighDPIDrawingEnabled = YES; return nil; } +- (void)_setupViewFlags +{ + var theClass = [self class], + classUID = [theClass UID]; + + if (CPViewFlags[classUID] === undefined) + { + var flags = 0; + + if ([theClass instanceMethodForSelector:@selector(drawRect:)] !== [CPView instanceMethodForSelector:@selector(drawRect:)] + || [theClass instanceMethodForSelector:@selector(viewWillDraw)] !== [CPView instanceMethodForSelector:@selector(viewWillDraw)]) + flags |= CPViewHasCustomDrawRect; + + CPViewFlags[classUID] = flags; + } + + _viewClassFlags = CPViewFlags[classUID]; +} - (id)init { @@ -362,6 +382,7 @@ var CPViewHighDPIDrawingEnabled = YES; _DOMImageSizes = []; #endif + [self _setupViewFlags]; [self _loadThemeAttributes]; } @@ -2457,6 +2478,9 @@ setBoundsOrigin: */ - (void)setNeedsDisplayInRect:(CGRect)aRect { + if (!(_viewClassFlags & CPViewHasCustomDrawRect)) + return; + if (CGRectIsEmpty(aRect)) return;