-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Make Cell Node Properties Atomic #74
Changes from 2 commits
b68f10e
a31cdc6
00ecf6b
bc4d4a3
73ace20
eb25898
828f9c9
6749aad
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,3 +4,4 @@ | |
- Add support for IGListKit post-removal-of-IGListSectionType, in preparation for IGListKit 3.0.0 release. (Adlai-Holler)[https://github.com/Adlai-Holler] (#49)[https://github.com/TextureGroup/Texture/pull/49] | ||
- Fix `__has_include` check in ASLog.h [Philipp Smorygo]([email protected]) | ||
- Fix potential deadlock in ASControlNode [Garrett Moon](https://github.com/garrettmoon) | ||
- Make cell node `indexPath` and `supplementaryElementKind` atomic so you can read from any thread. (Adlai-Holler)[https://github.com/Adlai-Holler] (#49)[https://github.com/TextureGroup/Texture/pull/74] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,7 @@ | |
NS_ASSUME_NONNULL_BEGIN | ||
|
||
@class ASCellNode, ASTextNode; | ||
@protocol ASRangeManagedNode; | ||
|
||
typedef NSUInteger ASCellNodeAnimation; | ||
|
||
|
@@ -87,7 +88,7 @@ typedef NS_ENUM(NSUInteger, ASCellNodeVisibilityEvent) { | |
* @return The supplementary element kind, or @c nil if this node does not represent a supplementary element. | ||
*/ | ||
//TODO change this to be a generic "kind" or "elementKind" that exposes `nil` for row kind | ||
@property (nonatomic, copy, readonly, nullable) NSString *supplementaryElementKind; | ||
@property (copy, readonly, nullable) NSString *supplementaryElementKind; | ||
|
||
/* | ||
* The layout attributes currently assigned to this node, if any. | ||
|
@@ -113,10 +114,8 @@ typedef NS_ENUM(NSUInteger, ASCellNodeVisibilityEvent) { | |
/** | ||
* The current index path of this cell node, or @c nil if this node is | ||
* not a valid item inside a table node or collection node. | ||
* | ||
* @note This property must be accessed on the main thread. | ||
*/ | ||
@property (nonatomic, readonly, nullable) NSIndexPath *indexPath; | ||
@property (readonly, nullable) NSIndexPath *indexPath; | ||
|
||
/** | ||
* The backing view controller, or @c nil if the node wasn't initialized with backing view controller | ||
|
@@ -126,10 +125,9 @@ typedef NS_ENUM(NSUInteger, ASCellNodeVisibilityEvent) { | |
|
||
|
||
/** | ||
* The owning node (ASCollectionNode/ASTableNode) of this cell node, or @c nil if this node is | ||
* not a valid item inside a table node or collection node or if those nodes are nil. | ||
* The table- or collection-node that this cell is a member of, if any. | ||
*/ | ||
@property (weak, nonatomic, readonly, nullable) ASDisplayNode *owningNode; | ||
@property (weak, readonly, nullable) id<ASRangeManagedNode> owningNode; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This s technically a breaking API change but it's so minor that we shouldn't worry about it. Virtually nobody uses this property – it was recently added and probably shouldn't be public anyway – and the migration pathway is trivial. |
||
|
||
/* | ||
* ASCellNode must forward touch events in order for UITableView and UICollectionView tap handling to work. Overriding | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
// | ||
// ASRangeManagedNode.h | ||
// AsyncDisplayKit | ||
// | ||
// Created by Adlai Holler on 4/26/17. | ||
// Copyright © 2017 Facebook. All rights reserved. | ||
// | ||
|
||
#import <Foundation/Foundation.h> | ||
#import <AsyncDisplayKit/ASTraitCollection.h> | ||
|
||
@class ASCellNode; | ||
|
||
NS_ASSUME_NONNULL_BEGIN | ||
|
||
/** | ||
* Basically ASTableNode or ASCollectionNode. | ||
*/ | ||
@protocol ASRangeManagedNode <NSObject, ASTraitEnvironment> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe I'm being overly semantic, but the methods these adopt don't seem to match the name. I can imagine nodes that range manage other nodes that don't have index paths. Anyway, wondering if there's a better name but I don't have suggestions. Also, should it be |
||
|
||
/** | ||
* Retrieve the index path for the given node, if it's a member of this container. | ||
* | ||
* @param node The node. | ||
* @return The index path, or nil if the node is not part of this container. | ||
*/ | ||
- (nullable NSIndexPath *)indexPathForNode:(ASCellNode *)node; | ||
|
||
@end | ||
|
||
NS_ASSUME_NONNULL_END |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -80,7 +80,7 @@ - (instancetype)initWithStyle:(UITableViewStyle)style | |
[self setViewBlock:^{ | ||
// Variable will be unused if event logging is off. | ||
__unused __typeof__(self) strongSelf = weakSelf; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the strongSelf necessary? If the viewBlock is running, self shouldn't be nil, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep but we should strongify once, rather than implicitly strongifying each time we use |
||
return [[ASTableView alloc] _initWithFrame:CGRectZero style:style dataControllerClass:nil eventLog:ASDisplayNodeGetEventLog(strongSelf)]; | ||
return [[ASTableView alloc] _initWithFrame:CGRectZero style:style dataControllerClass:nil owningNode:strongSelf eventLog:ASDisplayNodeGetEventLog(strongSelf)]; | ||
}]; | ||
} | ||
return self; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it is implied, but stylistically, I think it would be a good idea to require atomic to be stated explicitly for properties that intend it. Although it might become the more common thing, currently it's uncommon enough that most cases where it is omitted could be confused for mistakes. Any thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm down with it. The explicitness is nice.