[ASCollectionView] Repopulate supplementary views on item-level changes #1629

erichoracek · 2016-05-05T23:30:23Z

Currently within ASCollectionView, there is an assumption that there will always be a static number of supplementary views per section—even when additional items are added or removed from that section. This is evidenced by the fact that when you invoke -[ASCollectionView insertItemsAtIndexPaths:], the data source method -[ASCollectionDataSource collectionView:nodeForSupplementaryElementOfKind:atIndexPath:] is not invoked, preventing consumers from specifying a new number of supplementary nodes for the new set of items.

With this change, the set of supplementary nodes for a section is now recalculated not only on section-level mutations, but also on item-level mutations as well. This adds item-level counterparts to the section-level -prepareFor... subclassing hooks in ASDataController+Subclasses.h to make this possible.

This should fix #1278 and #1322

This has been tested in my project and seems to fix the assertion. Open to suggestions on how to test in a more universal way.

ekurutepe · 2016-05-06T09:38:49Z

AsyncDisplayKit/Details/ASCollectionDataController.mm

+    [self batchLayoutNodesFromContexts:contexts ofKind:kind completion:^(NSArray<ASCellNode *> *nodes, NSArray<NSIndexPath *> *indexPaths) {
+      [self insertNodes:nodes ofKind:kind atIndexPaths:indexPaths completion:nil];
+    }];
+    [_pendingContexts removeObjectForKey:kind];


You are mutating _pendingContexts while enumerating over it. Shouldn't this be throwing an exception?

It seems like it doesn't throw an exception but does not do what you expect either. Try this in a playground:

var mutableArray = NSMutableArray(array: [1,2,3,4,5,6]) mutableArray.enumerateObjectsUsingBlock { (object, idx, stop) in print("\(idx): \(object)") mutableArray.removeObject(object) }

Oh, I just realized that _pendingContexts is a dictionary not an array. Sorry about that.

While I definitely agree that this is an issue, this is the precedent set by the code above in this file at https://github.com/facebook/AsyncDisplayKit/blob/master/AsyncDisplayKit/Details/ASCollectionDataController.mm#L90-L101

@erichoracek please do fix that, as it seems to be an oversight.

appleguy · 2016-05-07T01:36:41Z

This is really great, thanks Eric! Great to e-meet you by the way, would love to talk with you sometime soon to see if I can help out with any of your projects.

As you no doubt noticed while working in this code, supplementary node support is scheduled to be revisited. This work is a valuable contribution to that effort and takes it in the right direction.

One thing we’d like to do is better integrate supplementary node handling with regular cell handling, so that we can also use “supplementary” elements to support section header and footer nodes in ASTableView. That said, you shouldn’t worry about this, simply some context on what we’d like to see happen here in the next ~3 months.

Send me a note at asyncdisplaykit@gmail.com mailto:asyncdisplaykit@gmail.com if you’re dealing with any blockers or have some highly desired features. I’ll try to get this reviewed and merged ASAP / in the next few days. We’re not too far from 1.9.8 as well.

    — Scott

On May 5, 2016, at 4:30 PM, Eric Horacek <notifications@github.com mailto:notifications@github.com> wrote:

Currently within ASCollectionView, there is an assumption that there will always be a static number of supplementary views per section—even when additional items are added or removed from that section. This is evidenced by the fact that when you invoke -[ASCollectionView insertItemsAtIndexPaths:], the data source method -[ASCollectionDataSource collectionView:nodeForSupplementaryElementOfKind:atIndexPath:] is not invoked, preventing consumers from specifying a new number of supplementary nodes for the new set of items.

With this change, the set of supplementary nodes for a section is now recalculated not only on section-level mutations, but also on item-level mutations as well. This adds item-level counterparts to the section-level -prepareFor... subclassing hooks in ASDataController+Subclasses.h to make this possible.

This should fix #1278 #1278 and #1322 #1322
This has been tested in my project and seen to fix the assertion. Open to suggestions on how to test in a more universal way.

You can view, comment on, or merge this pull request online at:

#1629 #1629
Commit Summary

Repopulate supplementary views on item-level changes
File Changes

M AsyncDisplayKit/Details/ASCollectionDataController.mm https://github.com/facebook/AsyncDisplayKit/pull/1629/files#diff-0 (100)
M AsyncDisplayKit/Details/ASDataController+Subclasses.h https://github.com/facebook/AsyncDisplayKit/pull/1629/files#diff-1 (66)
M AsyncDisplayKit/Details/ASDataController.mm https://github.com/facebook/AsyncDisplayKit/pull/1629/files#diff-2 (42)
M AsyncDisplayKit/Private/ASMultidimensionalArrayUtils.h https://github.com/facebook/AsyncDisplayKit/pull/1629/files#diff-3 (7)
M AsyncDisplayKit/Private/ASMultidimensionalArrayUtils.mm https://github.com/facebook/AsyncDisplayKit/pull/1629/files#diff-4 (29)
Patch Links:

https://github.com/facebook/AsyncDisplayKit/pull/1629.patch https://github.com/facebook/AsyncDisplayKit/pull/1629.patch
https://github.com/facebook/AsyncDisplayKit/pull/1629.diff https://github.com/facebook/AsyncDisplayKit/pull/1629.diff
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub #1629

levi · 2016-05-10T21:18:14Z

@erichoracek really strong stuff! Thanks so much for this patch. Would you be able to add some test cases to the supplementary node example project? This will help outline the given scenario and prove the solution.

I'm looking forward to getting this in!

erichoracek · 2016-05-11T06:54:14Z

@levi Thanks! The one example project I see with supplementary nodes is the CustomCollectionView project with the MosaicCollectionViewLayout. However, as far as I can tell, this collection view layout doesn't draw supplementary views for each item in the collection—rather there is only a single header supplementary view for each section. This PR is specifically attempting to fix the case where there is a supplementary view for each item in a collection view (e.g. inter-item separators) and batch updates are made to the items in the section. As such, does it make sense to add a new example project?

appleguy · 2016-05-14T06:49:02Z

@erichoracek Could you update to fix conflicts? Sorry I didn't get to this in time to avoid them! I will land this as soon as I notice (feel free to send me a Slack)

appleguy · 2016-05-14T06:49:31Z

@maicki take a review pass here if you get a chance as you know this code as well as anyone :)

maicki · 2016-05-16T07:36:57Z

AsyncDisplayKit/Details/ASCollectionDataController.mm

    [self batchLayoutNodesFromContexts:contexts ofKind:kind completion:^(NSArray<ASCellNode *> *nodes, NSArray<NSIndexPath *> *indexPaths) {
      [self insertNodes:nodes ofKind:kind atIndexPaths:indexPaths completion:nil];
    }];
-    [_pendingContexts removeObjectForKey:kind];


Do we remove the kind of pending contexts in some other place?

This change is due to the fact that _pendingContexts is being enumerated in this block so mutating it has no effect. What change are you asking for here?

maicki · 2016-05-16T07:46:36Z

@appleguy One small comment, but overall looks good to me.

appleguy · 2016-06-06T01:23:55Z

@erichoracek Hi Eric - I'm back from Vietnam and Cambodia! I believe the same conflicts are still present. If you can ping me on Slack once they're fixed, I'll merge straight away. Thanks for working on this!

Currently within `ASCollectionView`, there is an assumption that there will always be a static number of supplementary views per section—even when additional items are added or removed from that section. This is evidenced by the fact that when you invoke -[ASCollectionView insertItemsAtIndexPaths:], the data source method -[ASCollectionDataSource collectionView:nodeForSupplementaryElementOfKind:atIndexPath:] is not invoked, preventing consumers from specifying a new number of supplementary nodes for the new set of items. With this change, the set of supplementary nodes for a section is now recalculated not only on section-level mutations, but also on item-level mutations as well. This adds item-level counterparts to the section-level `-prepareFor...` subclassing hooks in `ASDataController+Subclasses.h` to make this possible. This should fix facebookarchive#1278 and facebookarchive#1322 This has been tested in my project and seen to fix the assertion. Open to suggestions on how to test in a more universal way.

erichoracek · 2016-06-06T04:58:35Z

@appleguy Awesome! Should be rebased with master—be sure to let me know if there's anything else that looks like it needs to be changed. Thanks!

appleguy · 2016-06-06T05:36:49Z

@erichoracek Diving save to join the 1.9.8 release!! Thanks for the super quick response, you are in :)

I'd love to hear about your experiences with the framework sometime -- if you are on Slack, let's see if a call (or meet at WWDC!) would equip both of us with new information about our craft.

…ve#1629) * Allow full color tinting on grayscale template images. * Signing a commit with the correct email address. * Add snapshot test for a grayscale image with ASExperimentalDrawingGlobal enabled.

ghost added the CLA Signed label May 5, 2016

erichoracek changed the title ~~Repopulate supplementary views on item-level changes~~ [ASCollectionView] Repopulate supplementary views on item-level changes May 6, 2016

ekurutepe reviewed May 6, 2016
View reviewed changes

maicki reviewed May 16, 2016
View reviewed changes

appleguy added ASCollectionView labels Jun 6, 2016

appleguy added this to the 1.9.8 milestone Jun 6, 2016

erichoracek force-pushed the recalculate-supplementaries-on-item-changes branch from b7e7bfc to 6f0a15e Compare June 6, 2016 02:49

erichoracek added 6 commits June 5, 2016 21:38

Fix warning

8f3e511

Fix index path arithmetic error

d89504e

Populate more aggresively

dc7f67c

Remove _pendingContexts dictionary mutation during enumeration

30dba28

Rebase

ed9fcdc

erichoracek force-pushed the recalculate-supplementaries-on-item-changes branch from 6f0a15e to ed9fcdc Compare June 6, 2016 04:53

Whitespace

70fbbe0

appleguy merged commit 4c1f88e into facebookarchive:master Jun 6, 2016

hannahmbanana mentioned this pull request Jun 18, 2016

[ASCollectionView] Deleting first item - inconsistency with associated supplementary node header #1771

Closed

appleguy mentioned this pull request Jun 18, 2016

Revert "[ASCollectionView] Repopulate supplementary views on item-level changes" #1772

Closed

nguyenhuy mentioned this pull request Feb 11, 2017

[ASDataController] Refactor ASDataController #3017

Merged

7 tasks

Uh oh!

[ASCollectionView] Repopulate supplementary views on item-level changes #1629

[ASCollectionView] Repopulate supplementary views on item-level changes #1629

Uh oh!

Conversation

erichoracek commented May 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ekurutepe May 6, 2016

Choose a reason for hiding this comment

Uh oh!

ekurutepe May 6, 2016

Choose a reason for hiding this comment

Uh oh!

ekurutepe May 6, 2016

Choose a reason for hiding this comment

Uh oh!

erichoracek May 6, 2016

Choose a reason for hiding this comment

Uh oh!

levi May 10, 2016

Choose a reason for hiding this comment

Uh oh!

appleguy commented May 7, 2016

Uh oh!

levi commented May 10, 2016

Uh oh!

erichoracek commented May 11, 2016

Uh oh!

appleguy commented May 14, 2016

Uh oh!

appleguy commented May 14, 2016

Uh oh!

maicki May 16, 2016

Choose a reason for hiding this comment

Uh oh!

erichoracek May 20, 2016

Choose a reason for hiding this comment

Uh oh!

maicki commented May 16, 2016

Uh oh!

appleguy commented Jun 6, 2016

Uh oh!

erichoracek commented Jun 6, 2016

Uh oh!

appleguy commented Jun 6, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

erichoracek commented May 5, 2016 •

edited

Loading