Tighten Rasterization API, Undeprecate It #82
Conversation
|
🚫 CI failed with log |
|
@Adlai-Holler :-D I never thought I'd see the day, but what an awesome surprise! Definitely planning on making use of this. Let me know if I can help, will review this weekend. |
Source/ASDisplayNode+Beta.h
Outdated
| */ | ||
| @property (nonatomic, assign) BOOL shouldRasterizeDescendants ASDISPLAYNODE_DEPRECATED_MSG("Deprecated in version 2.2"); | ||
| - (void)setShouldRasterizeDescendants; |
There was a problem hiding this comment.
@Adlai-Holler I think it's a good idea to make this an enable-only method. However, this name seems kinda sucky because it sounds exactly like a property that expects an argument.
How about something like -enableSubtreeRasterization, -enableRasterizingDescendants, etc? I think coming up with the right term for a subtree of subnodes could help here. .automaticallyManagesSubnodes is one reference point, but it does not apply recursively to all levels below, so Subtree might be better.
There was a problem hiding this comment.
Hmmm OK! I'll change it to enableSubtreeRasterization.
Source/ASDisplayNode.mm
Outdated
| } | ||
|
|
||
| // Ensure no loaded subnodes | ||
| for (ASDisplayNode *subnode in _subnodes) { |
There was a problem hiding this comment.
Is this loop enough to check all subnodes' subnodes? Should we use ASPerformBlockOnEverySubnode, or otherwise recursively call _enableRasterizingToParent or something on each?
| // If this subnode will be rasterized, enter hierarchy if needed | ||
| // TODO: Move this into _setSupernode: ? | ||
| if (isRasterized) { | ||
| if (self.inHierarchy) { |
There was a problem hiding this comment.
I think it may have been important for the original code to enter the Rasterized hierarchy state before __enterHierarchy is called, because it may have invoked delegate methods that check this. I haven't looked closely though, this might not be true - just something to take a close look at.
There was a problem hiding this comment.
Some minor cleanup / naming considerations to do, but overall this looks like a nice step forward :).
There will continue to be a reduced footprint from this feature once the isInHierarchy system is properly merged with interfacestate, and we could also factor out the AsyncDisplay code into a separate category to reduce complexity of the core files.
After playing with Pinterest for a little bit, there are some views that would majorly benefit from rasterization if we could get it going. Not that I'm addicted to it! But here's what this diff does:
@property (readwrite) shouldRasterizeDescendantswith a one-way method-[node setShouldRasterizeDescendants]. So once it's enabled on a node, there's no disabling it. This is breaking deprecated API but nobody will care.So the rasterization feature remains in the Beta header, and we're not going out of our way to support it, but if we can get rid of some old code, and tamp down the API, let's keep it around. Thoughts @maicki @nguyenhuy @appleguy ?