Make Cell Node Properties Atomic #74
Conversation
Source/ASCellNode.h
Outdated
| */ | ||
| @property (weak, nonatomic, readonly, nullable) ASDisplayNode *owningNode; | ||
| @property (weak, readonly, nullable) id<ASRangeManagedNode> owningNode; |
There was a problem hiding this comment.
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.
|
Hmm I wonder if |
Source/ASCellNode.h
Outdated
| @@ -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; | |||
There was a problem hiding this comment.
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.
I'm down with it. The explicitness is nice.
|
@Adlai-Holler Yes, I still think having a true bass class would be a very good thing. It would be a substantial project to build - I attempted it over Christmas holiday the year before this most recent, and it got complicated. I think it would be an architectural gift that keeps on giving, though :). In particular, it could even provide some APIs that are appropriate to offer publicly, and allow advanced apps to create custom range-managed scrollers. |
Source/ASRangeManagedNode.h
Outdated
| /** | ||
| * Basically ASTableNode or ASCollectionNode. | ||
| */ | ||
| @protocol ASRangeManagedNode <NSObject, ASTraitEnvironment> |
There was a problem hiding this comment.
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 ASRangeManagingNode instead? That would better align with ASHierarchyStateRangeManaged state naming.
| @@ -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.
Is the strongSelf necessary? If the viewBlock is running, self shouldn't be nil, right?
There was a problem hiding this comment.
Yep but we should strongify once, rather than implicitly strongifying each time we use weakSelf.
| */ | ||
| @property (nonatomic, strong, readonly) ASElementMap *visibleMap; | ||
| @property (atomic, copy, readonly) ASElementMap *visibleMap; |
There was a problem hiding this comment.
Are there asserts that ensure this?
There was a problem hiding this comment.
We don't need asserts to check this. We only change it internally. And we can't override atomic accessors so it's impossible to assert.
|
🚫 CI failed with log |
This way if you do logging or other tasks on background threads, you can read
indexPathandsupplementaryElementKindsafely.It kind of spiraled out of control, so:
ASRangeManagedNodeprotocol to coverASCollectionNodeandASTableNode.ASDataControllerEnvironmentDelegate– instead pass inASRangeManagedNodeto init.ASDataController.visibleMap/pendingMapatomic.ASCellNode.owningNodeatomic.