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

Jump to bottom

[ASCollectionView] Improve performance and behavior of rotation / bounds changes. #431

Merged
merged 9 commits into from
Oct 24, 2017
Merged

[ASCollectionView] Improve performance and behavior of rotation / bounds changes. #431

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
46743ad
[ASCollectionView] Improve performance and behavior of rotation / bou…
appleguy Jul 9, 2017
818b6a8
Edit CHANGELOG.md
appleguy Jul 10, 2017
ef69615
Merge branch 'master' into BoundsInvalidate
appleguy Oct 8, 2017
86b4214
[ASDataController] Implement -relayoutAllNodesWithInvalidationBlock:,…
appleguy Oct 8, 2017
78a5e43
Merge branch 'master' into BoundsInvalidate
nguyenhuy Oct 9, 2017
1d2bc7f
Don't set download results if no longer in preload range. (#606)
garrettmoon Oct 9, 2017
28d5156
Animated WebP support (#605)
garrettmoon Oct 9, 2017
9aa4fd2
[ASDataController] Add nullable specifier to invalidationBlock for re…
appleguy Oct 10, 2017
bbda3c8
Merge branch 'master' into BoundsInvalidate
appleguy Oct 13, 2017
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
8 changes: 5 additions & 3 deletions Source/ASCollectionView.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2156,10 +2156,12 @@ - (void)layer:(CALayer *)layer didChangeBoundsWithOldValue:(CGRect)oldBounds new
BOOL changedInNonScrollingDirection = (fixedHorizontally && newBounds.size.width != lastUsedSize.width) || (fixedVertically && newBounds.size.height != lastUsedSize.height);

if (changedInNonScrollingDirection) {
[_dataController relayoutAllNodes];
[_dataController waitUntilAllUpdatesAreCommitted];
// We need to ensure the size requery is done before we update our layout.
// Because -invalidateLayout doesn't trigger any operations by itself, and we answer queries from UICollectionView using layoutThatFits:,
// we invalidate the layout before we have updated all of the cells. Any cells that the collection needs the size of immediately will get
// -layoutThatFits: with a new constraint, on the main thread, and synchronously calculate them. Meanwhile, relayoutAllNodes will update
// the layout of any remaining nodes on background threads (and fast-return for any nodes that the UICV got to first).
Copy link
Member

Choose a reason for hiding this comment

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

Meanwhile, relayoutAllNodes will update the layout of any remaining nodes on background threads (and fast-return for any nodes that the UICV got to first).

I'm actually not aware of this behavior in the current codebase. The current implementation of -relayoutAllNodes processes all nodes in the UIKit index space on the main thread. That being said, I think this is the right approach that we should pursue. Are you planning to implement it shortly?

Copy link
Member Author

Choose a reason for hiding this comment

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

Heh, you're right. I suppose it is necessary for it to be synchronous in the current codebase without some kind of virtualization support (exposing only a subset of the datasource to UIKit).

[self.collectionViewLayout invalidateLayout];
Copy link
Member

@nguyenhuy nguyenhuy Jul 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I think invalidating the collection layout should be done "in the future", inside -[ASDataController _relayoutAllNodes] here. The reason is that there might be new layouts calculated before the size change, but still stuck on the main serial queue and not yet applied. And those layouts are invalid. But calling invalidating here is too soon and doesn't invalidate them.

So it looks like we need to change -[ASDataController relayotuAllNodes] to accept a block that get called on the main serial queue.

Copy link
Member Author

Choose a reason for hiding this comment

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

@nguyenhuy Interesting; that sounds about right. This seems like a very important issue to fix. Do you have any ideas on how we would test it properly?

I'm a bit worried because I don't have time to dig into this right now. In fact, I haven't synced with upstream for almost 2 months, and I have to focus on some deadlines for Q4 and 2018 planning.

If you would be able to take over this diff, I'd certainly appreciate it. I bet this change would greatly help the Pinterest app, especially on iPad rotation.

Copy link
Member

Choose a reason for hiding this comment

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

@appleguy Yeah, I can take over this diff, but not anytime soon though I'm afraid.

[_dataController relayoutAllNodes];
}
}

Expand Down