Add support for tinting layer-backed ASDisplayNode #1617
Conversation
There was a problem hiding this comment.
Great diff! Excited to see that we can utilize the encapsulation provided by ASDisplayNode to support something CALayer doesn't. I have a couple of questions:
- I'm wondering if we can implement this at a lower level (i.e ASDisplayNode) so that layer-backed ASTextNode and ASTextNode2 can support tint color too?
- Now that layer-backed ASImageNode supports tint color, can we turn ASButtonNode’s image node back to be layer-backed (here)?
rahul-malik
changed the title
|
🚫 CI failed with log |
Source/ASButtonNode.mm
Outdated
| #if TARGET_OS_IOS | ||
| // tvOS needs access to the underlying view | ||
| // of the button node to add a touch handler. | ||
| [_titleNode setLayerBacked:YES]; |
There was a problem hiding this comment.
I know this is from the original impl, but the comment and the code don't add up. If the view is needed under tvOS, then this node should not be layer-backed under it (and can be layer-backed otherwise)?
There was a problem hiding this comment.
the comment might be misleading here, I think the intention was to gate this layer backing only for iOS and by doing nothing in tvOS it will automatically be view backed?
There was a problem hiding this comment.
I can make this clearer at least
| if (_loaded(self)) { | ||
| if (_flags.layerBacked) { | ||
| // The first nondefault tint color value in the view’s hierarchy, ascending from and starting with the view itself. | ||
| return _tintColor ?: self.supernode.tintColor; |
There was a problem hiding this comment.
Our locking principles don't allow walking up the hierarchy while holding the node's lock. So you'd need to do it after releasing the lock.
| if (![_tintColor isEqual:color]) { | ||
| _tintColor = color; | ||
| // Tint color has changed. Unlock here before calling subclasses and exit-early | ||
| AS::MutexLocker unlock(__instanceLock__); |
There was a problem hiding this comment.
This might not be safe because the MutexLocker (which basically is std::lock_guard) will try to unlock again when it's destroyed and yield an exception. Instead I think it'd be easier to just manage the lock manually (i.e remove _bridge_prologue_write and this line, and call lock/unlock directly on __instanceLock__.
| [_titleNode setLayerBacked:NO]; | ||
| #else | ||
| [_titleNode setLayerBacked:YES]; | ||
| #endif |
Currently we're hitting an assertion on master because
self.tintColoris read always withinASImageNode.mm. We would either ignore this and only allow tinting on view backed nodes but since we're drawing the contents it feels like we can extend this capability to layer backed nodes as well.