Clean Up ASDisplayLayer #trivial #315
Conversation
Generated by 🚫 Danger |
Adlai-Holler
changed the title
There was a problem hiding this comment.
These are some great improvements. Thanks @Adlai-Holler!
My preference would be to remove +defaultValueForKey and restore a two-line init method. You can decide and land either way.
| @@ -179,13 +138,14 @@ + (id)defaultValueForKey:(NSString *)key | |||
| { | |||
| if ([key isEqualToString:@"displaysAsynchronously"]) { | |||
| return @YES; | |||
| } else if ([key isEqualToString:@"opaque"]) { | |||
There was a problem hiding this comment.
I'm not very familiar with the semantics of defaultValueForKey when used with standard layer properties like this. That said:
- I think having the code in init is more visible for a user exploring where a potentially-unexpected default opacity is coming from.
- This implementation will perform isEqualToString: on potentially thousands of accesses to unrelated properties. Although unlikely to be a focused performance hotspot, it probably would create distributed cost.
- What do you think about removing this method entirely and setting both self.opaque = YES and self.displaysAsynchronously = YES in the init method?
There was a problem hiding this comment.
@appleguy In my testing, CA only queries this once per class and stores the result, and it also correctly handles overriding standard layer properties as far as I can tell. So the overall performance is probably better with this implementation, which for me overrides the point about a user understanding where an unexpected opacity came from. Thoughts?
| @@ -209,7 +169,7 @@ - (void)display | |||
| ASDisplayNodeAssertMainThread(); | |||
| [self _hackResetNeedsDisplay]; | |||
There was a problem hiding this comment.
It's time to rename this method. YYAsyncLayer does the same thing :). Maybe just _clearNeedsDisplayFlag
There was a problem hiding this comment.
Agreed but too lazy to do it now 😴
|
Since CA caches the default values, the current implementation should be faster than overriding init so I'll land this in its current state. We can always revise later. |
asyncDelegatean atomic property, rather than using a mutex.NS_VALID_UNTIL_END_OF_SCOPEcode has been there since the original version of this file in 2014. Originally, the async delegate was accessed using a block, so I guess this was extra safety since they didn't know what you would do in the block. Anyway, ARC will retain the delegate throughout the method call so there's no behavior change here.displaySuspendedmain-thread-only. This isn't actually a behavior/API change, since changing the value off-main in the past would trigger an assertion failure downstream (in-setNeedsDisplayor-cancelAsyncDisplay) anyway. Better to just bite the bullet.