Skip to content
This repository was archived by the owner on Feb 2, 2023. It is now read-only.

Conversation

Adlai-Holler
Copy link
Contributor

This resolves #897. I added two test cases, and also posted a toy project https://github.com/Adlai-Holler/ASDKPlainNodeTest.

Changes
  • Add new interface state ASInterfaceStateInHierarchy that unites all others. This is the interface state for non cell-hosted nodes that are in the window hierarchy. I don't like the name – I considered Unknown but that seems like none more than all, and I considered All but that's not very informative.
Questions
  • What's the safest way to push the inCellNodeness down the hierarchy? It's sort of unsafe to walk up the node hierarchy to check during willEnterHierarchy and didExitHierarchy
  • I added checkViewHierarchy: param to _supernodeWithClass but honestly I wish I had just removed that behavior altogether. We don't call _supernodeWithClass anywhere in our code. But I wanted to ask because I haven't thought through the possible benefits of walking up the view hierarchy.

@appleguy What do you think?

@appleguy
Copy link
Contributor

appleguy commented Dec 5, 2015

@Adlai-Holler I would like to introduce a new type, ASHierarchyState, that includes things such as inRasterizedTree or inCellNode (those latter names aren't exactly excellent, open to ideas on a more pattern-friendly nomenclature).

I think there would have to be a mechanism to recursively flow updates to the ASHierarchyState down the tree when there is a change, such as enabling or disabling rasterization.

This is actually my preference for how we resolve the deadlock, but I haven't yet studied it in enough detail to be confident this can be done without another possible deadlocking moment. However, I think it can be.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sadly I think the behavior of this method is subtle. It actually varies a bit for layer-backed nodes, which rely on kCAOrderIn (CAAction) rather than the UIView-unique hierarchy notifications. This needs better documentation, and in fact, I'm pretty sure a Github issue is filed for this purpose.

@appleguy
Copy link
Contributor

appleguy commented Dec 6, 2015

I am happy to land this as soon as I can start on the deadlock alleviation measures, worst case should be tomorrow.

@appleguy
Copy link
Contributor

appleguy commented Dec 6, 2015

@Adlai-Holler I agree we need to rethink the name for the combined state, but this is easy to rename after 1.9.3 has stabilized some of the active issues.

I think _supernodeWithClass is used by internal FB extensions to ASDK that were broken out during the prep for open sourcing. However, they're almost certainly irrelevant now. Don't worry about this method for the moment and I will look into possible usages / consider removing it in the next couple months.

appleguy added a commit that referenced this pull request Dec 6, 2015
Provide Basic Interface State Support for Nodes Outside of Cells
@appleguy appleguy merged commit 2989729 into facebookarchive:master Dec 6, 2015
peter-iakovlev pushed a commit to peter-iakovlev/AsyncDisplayKit that referenced this pull request Jul 21, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[ASInterfaceState] Regression: plain view-hosted nodes don't automatically fetch data anymore

3 participants