Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix main thread assertion for tint color on text nodes, re-render tint-able nodes on hierarchy changes #1670

Merged
merged 3 commits into from
Sep 16, 2019
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions Source/ASButtonNode.mm
Original file line number Diff line number Diff line change
Expand Up @@ -140,9 +140,20 @@ -(void)tintColorDidChange
// UIButton documentation states that it tints the image and title of buttons when tintColor is set.
// | "The tint color to apply to the button title and image."
// | From: https://developer.apple.com/documentation/uikit/uibutton/1624025-tintcolor
[self lock];
UIColor *tintColor = self.tintColor;
self.imageNode.tintColor = tintColor;
self.titleNode.tintColor = tintColor;
[self unlock];
[self setNeedsDisplay];
}

#pragma mark Interface State

- (void)didEnterHierarchy
{
[super didEnterHierarchy];
[self setNeedsDisplay];
}

- (void)updateImage
Expand Down
6 changes: 5 additions & 1 deletion Source/ASTableView.mm
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#import <AsyncDisplayKit/ASDelegateProxy.h>
#import <AsyncDisplayKit/ASDisplayNodeExtras.h>
#import <AsyncDisplayKit/ASDisplayNode+FrameworkPrivate.h>
#import <AsyncDisplayKit/ASDisplayNodeInternal.h>
#import <AsyncDisplayKit/ASElementMap.h>
#import <AsyncDisplayKit/ASInternalHelpers.h>
#import <AsyncDisplayKit/ASLayout.h>
Expand All @@ -28,6 +29,7 @@
#import <AsyncDisplayKit/ASTableLayoutController.h>
#import <AsyncDisplayKit/ASBatchContext.h>


static NSString * const kCellReuseIdentifier = @"_ASTableViewCell";

//#define LOG(...) NSLog(__VA_ARGS__)
Expand Down Expand Up @@ -113,12 +115,14 @@ - (void)setElement:(ASCollectionElement *)element
self.selectionStyle = node.selectionStyle;
self.focusStyle = node.focusStyle;
self.accessoryType = node.accessoryType;
self.tintColor = node.tintColor;
rahul-malik marked this conversation as resolved.
Show resolved Hide resolved

// the following ensures that we clip the entire cell to it's bounds if node.clipsToBounds is set (the default)
// This is actually a workaround for a bug we are seeing in some rare cases (selected background view
// overlaps other cells if size of ASCellNode has changed.)
self.clipsToBounds = node.clipsToBounds;

// If the cell node has been explicitly configured with a tint color, we can apply that directly to the cell view to preserve the previous behavior
self.tintColor = node->_tintColor;
}

[node __setSelectedFromUIKit:self.selected];
Expand Down
42 changes: 40 additions & 2 deletions Source/ASTextNode.mm
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,7 @@ @implementation ASTextNode {
CGSize _shadowOffset;
CGColorRef _shadowColor;
UIColor *_cachedShadowUIColor;
UIColor *_cachedTintColor;
UIColor *_placeholderColor;
CGFloat _shadowOpacity;
CGFloat _shadowRadius;
Expand Down Expand Up @@ -382,7 +383,7 @@ - (ASTextKitAttributes)_locked_rendererAttributes
.shadowColor = _cachedShadowUIColor,
.shadowOpacity = _shadowOpacity,
.shadowRadius = _shadowRadius,
.tintColor = self.textColorFollowsTintColor ? self.tintColor : nil
.tintColor = _cachedTintColor
};
}

Expand Down Expand Up @@ -543,7 +544,11 @@ - (NSArray *)exclusionPaths
- (NSObject *)drawParametersForAsyncLayer:(_ASDisplayLayer *)layer
{
ASLockScopeSelf();

if (_textColorFollowsTintColor) {
_cachedTintColor = self.tintColor;
} else {
_cachedTintColor = nil;
}
return [[ASTextNodeDrawParameter alloc] initWithRendererAttributes:[self _locked_rendererAttributes]
backgroundColor:self.backgroundColor
textContainerInsets:_textContainerInset
Expand Down Expand Up @@ -945,6 +950,39 @@ - (CGRect)frameForTextRange:(NSRange)textRange
return ASTextNodeAdjustRenderRectForShadowPadding(frame, self.shadowPadding);
}

#pragma mark - Tint Colors


- (void)tintColorDidChange
{
[super tintColorDidChange];

[self _setNeedsDisplayOnTintedTextColor];
}

- (void)_setNeedsDisplayOnTintedTextColor
{
BOOL textColorFollowsTintColor = NO;
{
AS::MutexLocker l(__instanceLock__);
textColorFollowsTintColor = _textColorFollowsTintColor;
}

if (textColorFollowsTintColor) {
[self setNeedsDisplay];
}
}


#pragma mark Interface State

- (void)didEnterHierarchy
{
[super didEnterHierarchy];

[self _setNeedsDisplayOnTintedTextColor];
}

#pragma mark - Placeholders

- (UIColor *)placeholderColor
Expand Down
60 changes: 46 additions & 14 deletions Source/ASTextNode2.mm
Original file line number Diff line number Diff line change
Expand Up @@ -511,19 +511,6 @@ - (void)prepareAttributedString:(NSMutableAttributedString *)attributedString is
shadow.shadowBlurRadius = _shadowRadius;
[attributedString addAttribute:NSShadowAttributeName value:shadow range:NSMakeRange(0, attributedString.length)];
}

// Apply tint color if needed and foreground color is not already specified
if (self.textColorFollowsTintColor && attributedString.length > 0) {
// Apply tint color if specified and if foreground color is undefined for attributedString
NSRange limit = NSMakeRange(0, attributedString.length);
// Look for previous attributes that define foreground color
UIColor *attributeValue = (UIColor *)[attributedString attribute:NSForegroundColorAttributeName atIndex:limit.location effectiveRange:NULL];
UIColor *tintColor = self.tintColor;
if (attributeValue == nil && tintColor) {
// None are found, apply tint color if available. Fallback to "black" text color
[attributedString addAttributes:@{ NSForegroundColorAttributeName : tintColor } range:limit];
}
}
}

#pragma mark - Drawing
Expand All @@ -540,7 +527,20 @@ - (NSObject *)drawParametersForAsyncLayer:(_ASDisplayLayer *)layer
NSMutableAttributedString *mutableText = [_attributedText mutableCopy] ?: [[NSMutableAttributedString alloc] init];

[self prepareAttributedString:mutableText isForIntrinsicSize:NO];


// After all other attributes are set, apply tint color if needed and foreground color is not already specified
if (self.textColorFollowsTintColor && mutableText.length > 0) {
// Apply tint color if specified and if foreground color is undefined for attributedString
NSRange limit = NSMakeRange(0, mutableText.length);
// Look for previous attributes that define foreground color
UIColor *attributeValue = (UIColor *)[mutableText attribute:NSForegroundColorAttributeName atIndex:limit.location effectiveRange:NULL];
UIColor *tintColor = self.tintColor;
if (attributeValue == nil && tintColor) {
// None are found, apply tint color if available. Fallback to "black" text color
[mutableText addAttributes:@{ NSForegroundColorAttributeName : tintColor } range:limit];
}
}

return @{
@"container": copiedContainer,
@"text": mutableText,
Expand Down Expand Up @@ -577,6 +577,38 @@ + (void)drawRect:(CGRect)bounds withParameters:(NSDictionary *)layoutDict isCanc
[layout drawInContext:context size:bounds.size point:bounds.origin view:nil layer:nil debug:[ASTextDebugOption sharedDebugOption] cancel:isCancelledBlock];
}

#pragma mark - Tint Color

- (void)tintColorDidChange
{
[super tintColorDidChange];

[self _setNeedsDisplayOnTintedTextColor];
}

- (void)_setNeedsDisplayOnTintedTextColor
{
BOOL textColorFollowsTintColor = NO;
{
AS::MutexLocker l(__instanceLock__);
textColorFollowsTintColor = _textColorFollowsTintColor;
}

if (textColorFollowsTintColor) {
[self setNeedsDisplay];
}
}


#pragma mark Interface State

- (void)didEnterHierarchy
{
[super didEnterHierarchy];

[self _setNeedsDisplayOnTintedTextColor];
}

#pragma mark - Attributes

- (id)linkAttributeValueAtPoint:(CGPoint)point
Expand Down
1 change: 1 addition & 0 deletions Source/Private/ASDisplayNode+UIViewBridge.mm
Original file line number Diff line number Diff line change
Expand Up @@ -823,6 +823,7 @@ - (void)setTintColor:(UIColor *)color
}
}
} else {
_tintColor = color;
_setToViewOnly(tintColor, color);
}
__instanceLock__.unlock();
Expand Down
32 changes: 32 additions & 0 deletions Tests/ASButtonNodeSnapshotTests.mm
Original file line number Diff line number Diff line change
Expand Up @@ -80,4 +80,36 @@ - (void)testTintColorWithForegroundColorSet
ASSnapshotVerifyNode(node, nil);
}

- (void)testTintColorWithInheritedTintColor
{
ASDisplayNode *container = [[ASDisplayNode alloc] init];
container.tintColor = UIColor.redColor;

// Add to hierarchy, assert new tint is picked up
ASButtonNode *node = [[ASButtonNode alloc] init];
[container addSubnode:node];
[node setImage:[[self testImage] imageWithRenderingMode:UIImageRenderingModeAlwaysTemplate] forState:UIControlStateNormal];
[node setTitle:@"Press Me"
withFont:[UIFont systemFontOfSize:48]
withColor:nil
forState:UIControlStateNormal];
node.imageNode.style.width = ASDimensionMake(200);
node.imageNode.style.height = ASDimensionMake(200);
container.style.preferredSize = CGSizeMake(1000,1000);
ASDisplayNodeSizeToFitSize(node, CGSizeMake(1000, 1000));
// Force load so the superview is set for the button _ASDisplayView
__unused UIView *v = container.view;
ASSnapshotVerifyNode(node, @"red_inherited_tint");

// Change hierarchy, assert new tint is picked up
ASDisplayNode *container2 = [[ASDisplayNode alloc] init];
container2.tintColor = UIColor.greenColor;
container2.style.preferredSize = CGSizeMake(1000,1000);
[container2 addSubnode:node];
ASDisplayNodeSizeToFitSize(node, CGSizeMake(1000, 1000));
// Force load so the superview is set for the button _ASDisplayView
__unused UIView *v2 = container2.view; // Force load
ASSnapshotVerifyNode(node, @"green_inherited_tint");
}

@end
21 changes: 21 additions & 0 deletions Tests/ASTableViewTests.mm
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,7 @@ - (ASCellNodeBlock)tableView:(ASTableView *)tableView nodeBlockForRowAtIndexPath
ASTestTextCellNode *textCellNode = [ASTestTextCellNode new];
textCellNode.text = [NSString stringWithFormat:@"{%d, %d}", (int)indexPath.section, (int)indexPath.row];
textCellNode.backgroundColor = [UIColor whiteColor];
textCellNode.tintColor = [UIColor yellowColor];
return textCellNode;
};
}
Expand Down Expand Up @@ -886,6 +887,26 @@ - (void)testTableViewReloadDoesReloadIfEditableTextNodeIsFirstResponder
XCTAssertEqual([node.view numberOfRowsInSection:0], 2);
}


- (void)testTintColorIsPropagatedToTableViewCell
{
// If a tint color is explicitly defined on an ASCellNode, we should
CGSize tableViewSize = CGSizeMake(100, 500);
ASTestTableView *tableView = [[ASTestTableView alloc] initWithFrame:CGRectMake(0, 0, tableViewSize.width, tableViewSize.height)
style:UITableViewStylePlain];
ASTableViewFilledDataSource *dataSource = [ASTableViewFilledDataSource new];

tableView.asyncDelegate = dataSource;
tableView.asyncDataSource = dataSource;

[tableView reloadData];
[tableView waitUntilAllUpdatesAreCommitted];
NSIndexPath *indexPath = [NSIndexPath indexPathForRow:0 inSection:0];
UITableViewCell *uikitCell = [tableView cellForRowAtIndexPath:indexPath];
BOOL areColorsEqual = CGColorEqualToColor(uikitCell.tintColor.CGColor, UIColor.yellowColor.CGColor);
XCTAssertTrue(areColorsEqual);
}

@end

@implementation UITableView (Testing)
Expand Down
23 changes: 23 additions & 0 deletions Tests/ASTextNode2SnapshotTests.mm
Original file line number Diff line number Diff line change
Expand Up @@ -331,4 +331,27 @@ - (void)testTextTintColor_ASTextNode2
ASSnapshotVerifyNode(textNode, nil);
}


- (void)testTintColorHierarchyChange
{
ASDisplayNode *containerNode = [[ASDisplayNode alloc] init];
containerNode.tintColor = [UIColor greenColor];

ASTextNode *node = [[ASTextNode alloc] init];
[containerNode addSubnode:node];
[node setLayerBacked:YES];
node.textColorFollowsTintColor = YES;
node.attributedText = [[NSAttributedString alloc] initWithString:@"Hello" attributes:@{}];
ASDisplayNodeSizeToFitSizeRange(node, ASSizeRangeMake(CGSizeZero, CGSizeMake(INFINITY, INFINITY)));
containerNode.style.preferredSize = node.bounds.size;
ASSnapshotVerifyNode(node, @"green_tint_from_parent");

ASDisplayNode *containerNode2 = [[ASDisplayNode alloc] init];
containerNode2.tintColor = [UIColor redColor];
[containerNode2 addSubnode:node];
containerNode2.style.preferredSize = node.bounds.size;
ASSnapshotVerifyNode(node, @"red_tint_from_parent");
}


@end
25 changes: 25 additions & 0 deletions Tests/ASTextNodeSnapshotTests.mm
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,28 @@ - (void)testUIGraphicsRendererDrawingExperiment
ASSnapshotVerifyNode(textNode, nil);
}

- (void)testTintColorHierarchyChange
{
ASDisplayNode *containerNode = [[ASDisplayNode alloc] init];
containerNode.tintColor = [UIColor greenColor];

ASTextNode *node = [[ASTextNode alloc] init];
[containerNode addSubnode:node];
[node setLayerBacked:YES];
node.textColorFollowsTintColor = YES;
node.attributedText = [[NSAttributedString alloc] initWithString:@"Hello" attributes:@{}];
ASDisplayNodeSizeToFitSizeRange(node, ASSizeRangeMake(CGSizeZero, CGSizeMake(INFINITY, INFINITY)));
containerNode.style.preferredSize = node.bounds.size;
ASSnapshotVerifyNode(node, @"green_tint_from_parent");


ASDisplayNode *containerNode2 = [[ASDisplayNode alloc] init];
containerNode2.tintColor = [UIColor redColor];
[containerNode2 addSubnode:node];
containerNode2.style.preferredSize = node.bounds.size;
ASSnapshotVerifyNode(node, @"red_tint_from_parent");
}

#if AS_AT_LEAST_IOS13

- (void)testUserInterfaceStyleSnapshotTesting
Expand Down Expand Up @@ -248,6 +270,9 @@ - (void)testUserInterfaceStyleSnapshotTestingTintColor
}
}




#endif // #if AS_AT_LEAST_IOS13

@end
Binary file added ...deSnapshotTests/[email protected]
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added ...NodeSnapshotTests/[email protected]
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added ...xtNode2SnapshotTests/[email protected]
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added ...TextNode2SnapshotTests/[email protected]
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added ...extNodeSnapshotTests/[email protected]
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added ...STextNodeSnapshotTests/[email protected]
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.