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.

Sign up for GitHub

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

Clean Up ASDisplayLayer #trivial #315

Merged
merged 1 commit into from
May 29, 2017
Merged

Clean Up ASDisplayLayer #trivial #315

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions Source/Details/_ASDisplayLayer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,11 +53,11 @@
+ (dispatch_queue_t)displayQueue;

/**
@summary Delegate for asynchronous display of the layer.
@summary Delegate for asynchronous display of the layer. This should be the node (default) unless you REALLY know what you're doing.

@desc The asyncDelegate will have the opportunity to override the methods related to async display.
*/
@property (nonatomic, weak) id<_ASDisplayLayerDelegate> asyncDelegate;
@property (atomic, weak) id<_ASDisplayLayerDelegate> asyncDelegate;

/**
@summary Suspends both asynchronous and synchronous display of the receiver if YES.
Expand Down
70 changes: 8 additions & 62 deletions Source/Details/_ASDisplayLayer.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,64 +28,26 @@

@implementation _ASDisplayLayer
{
ASDN::Mutex _asyncDelegateLock;
// We can take this lock when we're setting displaySuspended and in setNeedsDisplay, so to not deadlock, this is recursive
ASDN::RecursiveMutex _displaySuspendedLock;
BOOL _displaySuspended;
BOOL _attemptedDisplayWhileZeroSized;

struct {
BOOL delegateDidChangeBounds:1;
} _delegateFlags;

id<_ASDisplayLayerDelegate> __weak _asyncDelegate;
}

@dynamic displaysAsynchronously;

#pragma mark -
#pragma mark Lifecycle

- (instancetype)init
{
if ((self = [super init])) {

self.opaque = YES;
}
return self;
}

#pragma mark -
#pragma mark Properties

- (id<_ASDisplayLayerDelegate>)asyncDelegate
{
ASDN::MutexLocker l(_asyncDelegateLock);
return _asyncDelegate;
}
#pragma mark - Properties

- (void)setDelegate:(id)delegate
{
[super setDelegate:delegate];
_delegateFlags.delegateDidChangeBounds = [delegate respondsToSelector:@selector(layer:didChangeBoundsWithOldValue:newValue:)];
}

- (void)setAsyncDelegate:(id<_ASDisplayLayerDelegate>)asyncDelegate
{
ASDisplayNodeAssert(!asyncDelegate || [asyncDelegate isKindOfClass:[ASDisplayNode class]], @"_ASDisplayLayer is inherently coupled to ASDisplayNode and cannot be used with another asyncDelegate. Please rethink what you are trying to do.");
ASDN::MutexLocker l(_asyncDelegateLock);
_asyncDelegate = asyncDelegate;
}

- (BOOL)isDisplaySuspended
{
ASDN::MutexLocker l(_displaySuspendedLock);
return _displaySuspended;
}

- (void)setDisplaySuspended:(BOOL)displaySuspended
{
ASDN::MutexLocker l(_displaySuspendedLock);
ASDisplayNodeAssertMainThread();
if (_displaySuspended != displaySuspended) {
_displaySuspended = displaySuspended;
if (!displaySuspended) {
Expand Down Expand Up @@ -146,8 +108,6 @@ - (void)layoutSublayers
- (void)setNeedsDisplay
{
ASDisplayNodeAssertMainThread();

_displaySuspendedLock.lock();

// FIXME: Reconsider whether we should cancel a display in progress.
// We should definitely cancel a display that is scheduled, but unstarted display.
Expand All @@ -157,7 +117,6 @@ - (void)setNeedsDisplay
if (!_displaySuspended) {
[super setNeedsDisplay];
}
_displaySuspendedLock.unlock();
}

#pragma mark -
Expand All @@ -179,13 +138,14 @@ + (id)defaultValueForKey:(NSString *)key
{
if ([key isEqualToString:@"displaysAsynchronously"]) {
return @YES;
} else if ([key isEqualToString:@"opaque"]) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@Adlai-Holler Adlai-Holler May 29, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

return @YES;
} else {
return [super defaultValueForKey:key];
}
}

#pragma mark -
#pragma mark Display
#pragma mark - Display

- (void)displayImmediately
{
Expand All @@ -209,7 +169,7 @@ - (void)display
ASDisplayNodeAssertMainThread();
[self _hackResetNeedsDisplay];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's time to rename this method. YYAsyncLayer does the same thing :). Maybe just _clearNeedsDisplayFlag

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed but too lazy to do it now 😴


if (self.isDisplaySuspended) {
if (self.displaySuspended) {
return;
}

Expand All @@ -221,29 +181,15 @@ - (void)display:(BOOL)asynchronously
if (CGRectIsEmpty(self.bounds)) {
_attemptedDisplayWhileZeroSized = YES;
}

id<_ASDisplayLayerDelegate> NS_VALID_UNTIL_END_OF_SCOPE strongAsyncDelegate;
{
_asyncDelegateLock.lock();
strongAsyncDelegate = _asyncDelegate;
_asyncDelegateLock.unlock();
}

[strongAsyncDelegate displayAsyncLayer:self asynchronously:asynchronously];
[self.asyncDelegate displayAsyncLayer:self asynchronously:asynchronously];
}

- (void)cancelAsyncDisplay
{
ASDisplayNodeAssertMainThread();

id<_ASDisplayLayerDelegate> NS_VALID_UNTIL_END_OF_SCOPE strongAsyncDelegate;
{
_asyncDelegateLock.lock();
strongAsyncDelegate = _asyncDelegate;
_asyncDelegateLock.unlock();
}

[strongAsyncDelegate cancelDisplayAsyncLayer:self];
[self.asyncDelegate cancelDisplayAsyncLayer:self];
}

// e.g. <MYTextNodeLayer: 0xFFFFFF; node = <MYTextNode: 0xFFFFFFE; name = "Username node for user 179">>
Expand Down