Skip to content

Commit

Permalink
[ASDisplayNode] -didEnterPreloadState does not need to call -layoutIf…
Browse files Browse the repository at this point in the history
…Needed #trivial (TextureGroup#411)

This was originally added for ASCollectionNode and ASTableNode preloading to work
as intended when nested inside of another ASRangeController-powered node. Indeed,
it is still necessary to trigger layout on these UIKit-backed components in order
for their own ASRangeControllers to start preparing content within them.

However, unlike the comment suggests, it is *not* necessary to do this for image
nodes. ASNetworkImageNode has only one .URL, and does not use the .bounds or
.calculatedLayout at all during loading. Even the ASMultiplexImageNode does not
use the .bounds, and the ASMapNode uses .calculatedLayout instead of .bounds.

This change has important performance benefits. In particular, it avoids
layouts that would occur on the main thread, often including text sizing,
and also can result in redundant layout passes (even during a layout pass that
triggers a node to enter the preload range, it may force its own layout early).

It would be great to test this change with Pinterest to confirm its safety, but
based on a full audit of the framework codebase, the only possibility that I
see for a regression is if app implementations of -didEnterPreloadState make
direct use of .bounds instead of .calculatedLayout (which is easy to fix).
  • Loading branch information
appleguy authored and bernieperez committed Apr 25, 2018
1 parent cc10d56 commit a82136c
Show file tree
Hide file tree
Showing 3 changed files with 6 additions and 10 deletions.
6 changes: 3 additions & 3 deletions Source/ASCollectionNode.mm
Original file line number Diff line number Diff line change
Expand Up @@ -213,10 +213,10 @@ - (void)interfaceStateDidChange:(ASInterfaceState)newState fromState:(ASInterfac

- (void)didEnterPreloadState
{
// Intentionally allocate the view here so that super will trigger a layout pass on it which in turn will trigger the intial data load.
// We can get rid of this call later when ASDataController, ASRangeController and ASCollectionLayout can operate without the view.
[self view];
[super didEnterPreloadState];
// Intentionally allocate the view here and trigger a layout pass on it, which in turn will trigger the intial data load.
// We can get rid of this call later when ASDataController, ASRangeController and ASCollectionLayout can operate without the view.
[[self view] layoutIfNeeded];
}

#if ASRangeControllerLoggingEnabled
Expand Down
4 changes: 0 additions & 4 deletions Source/ASDisplayNode.mm
Original file line number Diff line number Diff line change
Expand Up @@ -2938,10 +2938,6 @@ - (void)didEnterPreloadState
ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__);
[_interfaceStateDelegate didEnterPreloadState];

// Trigger a layout pass to ensure all subnodes have the correct size to preload their content.
// This is important for image nodes, as well as collection and table nodes.
[self layoutIfNeeded];

if (_methodOverrides & ASDisplayNodeMethodOverrideFetchData) {
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wdeprecated-declarations"
Expand Down
6 changes: 3 additions & 3 deletions Source/ASTableNode.mm
Original file line number Diff line number Diff line change
Expand Up @@ -142,10 +142,10 @@ - (void)interfaceStateDidChange:(ASInterfaceState)newState fromState:(ASInterfac

- (void)didEnterPreloadState
{
// Intentionally allocate the view here so that super will trigger a layout pass on it which in turn will trigger the intial data load.
// We can get rid of this call later when ASDataController, ASRangeController and ASCollectionLayout can operate without the view.
[self view];
[super didEnterPreloadState];
// Intentionally allocate the view here and trigger a layout pass on it, which in turn will trigger the intial data load.
// We can get rid of this call later when ASDataController, ASRangeController and ASCollectionLayout can operate without the view.
[[self view] layoutIfNeeded];
}

#if ASRangeControllerLoggingEnabled
Expand Down

0 comments on commit a82136c

Please sign in to comment.