[ASTextNode, ASImageNode, ASVideoNode] Use ASDisplayNode base class lock for subclass property synchronization #1877
Conversation
…ass property synchronization
| @@ -110,17 +109,11 @@ - (instancetype)initWithViewBlock:(ASDisplayNodeViewBlock)viewBlock didLoadBlock | |||
| return nil; | |||
| } | |||
|
|
|||
| - (void)dealloc | |||
There was a problem hiding this comment.
@johnepinterest do you remember why this was removed? I think we'll add it back in.
There was a problem hiding this comment.
We need to add that back in! That was a refactor to remove the dealloc from the ASImageNode category and move it into ASImageNode
There was a problem hiding this comment.
Although I haven't tracked it down exactly, I strongly suspect this is probably an artifact of the branch that I originally made these changes too (332ab23), which was current at the time. Due to the nature in which we tried to get this change in (e.g., I sent you the file as-is directly), this context was lost as a result.
I believe the correct thing to do is remove this part of the diff - restore the -dealloc code removed - and verify that the additional -dealloc code that is "apparently" deleted here was in fact added as a result of a change / merged PR that happened some time after 332ab23.
There was a problem hiding this comment.
Ok, tracked it down. The -dealloc deletion by this commit is a mistake and is a result of loosing the context and commit that my original changes were applied to (332ab23).
Commit b2810ed, which took place after 332ab23, is the commit that added this -dealloc code.
IMPORTANT: As @maicki says, this -dealloc code needs to go back in (if it isn't already).
For reference, the reason why this showed up as a delete is because I gave @appleguy a copy of ASImageNode.mm with my changes (relative to 332ab23) directly, and I suspect that @applyguy copied that file in to his repo at a "more current than 332ab23, and after b2810ed" point in time.
There was a problem hiding this comment.
@johnepinterest: -dealloc was added back in before it was merged. If that's the only difference then we are good.
ghost
added
the
| @@ -134,11 +127,11 @@ - (CGSize)calculateSizeThatFits:(CGSize)constrainedSize | |||
|
|
|||
| - (void)setImage:(UIImage *)image | |||
| { | |||
| _imageLock.lock(); | |||
There was a problem hiding this comment.
I actually remember now why this method had to use .lock() and .unlock() manually. It was to try to avoid interleaving with the superclass _propertyLock, which was grabbed inside invalidateCalculatedLayout and other methods below.
I think now that this is addressed, we should make this case a simple MutexLocker. The else case that unlocks can be deleted.
appleguy
changed the title
|
@hannahmbanana thank you for doing this! There is a lot of careful stuff here, and this is a very important change that I didn't have time for tonight. |
* master_up: (43 commits) Do not expose tgmath.h to all clients of Texture (facebookarchive#1900) Call will / did display node for ASTextNode. Fixes facebookarchive#1680 (facebookarchive#1893) Remove background deallocation helper code (facebookarchive#1890) [Accessibility] Ship ASExperimentalDoNotCacheAccessibilityElements (facebookarchive#1888) 🎉 3.0.0 (facebookarchive#1883) Upgrade to Xcode 11.5 (facebookarchive#1877) Renames AS_EXTERN and ASViewController (facebookarchive#1876) Improve ThreeMigrationGuide.md (facebookarchive#1878) Add a 3.0 migration guide (facebookarchive#1875) I forgot this in the last PR and I'm pushing to master, I'm a bad person. Update for 3.0.0-rc.2 (facebookarchive#1874) Update RELEASE.md (facebookarchive#1873) Fix all the warnings and re-enable on CI (facebookarchive#1872) Prepare for 3.0.0-rc.1 release (facebookarchive#1870) -[ASNetworkImageNode setURL:resetToDefault:] forget to reset animatedImage (facebookarchive#1861) [ASDisplayNode] Implement accessibilityElementsHidden (facebookarchive#1859) Fix documentation for ASCornerRoundingTypeClipping (facebookarchive#1863) Add iOS13 UIContextMenu api to ASCommonCollectionDelegate (facebookarchive#1860) [ASDisplayNode] Implement accessibilityViewIsModal (facebookarchive#1858) Update FBSnapshotTestCase to iOSSnapshotTestCase (=6.2) (facebookarchive#1855) ...
No description provided.