Instagram / IGListKit

A data-driven UICollectionView framework for building fast and flexible lists.
https://instagram.github.io/IGListKit/
MIT License
12.87k stars 1.54k forks source link

IGListStackedSectionController's children need to know numberOrItems before didUpdate is called #348

Closed jeffbailey closed 7 years ago

jeffbailey commented 7 years ago

New issue checklist

General information

IGListStackedSectionController is incredibly useful for decomposing section controllers, but I'm running into an issue that appears to be a bug when one of the child section controllers has a variable number of cells, based on the object injected into it.

A child section controller's func numberOfItems() -> Int method is getting called before func didUpdate(to object: Any) That results in the section controller having to know how many items it contains BEFORE it has its data.

I tracked the issue down to the stacked section controller constructor calling it's reloadData method:

- (instancetype)initWithSectionControllers:(NSArray <IGListSectionController<IGListSectionType> *> *)sectionControllers {
    if (self = [super init]) {
        for (IGListSectionController<IGListSectionType> *sectionController in sectionControllers) {
            sectionController.collectionContext = self;
            sectionController.viewController = self.viewController;
        }

        _visibleSectionControllers = [[NSCountedSet alloc] init];
        _sectionControllers = [NSOrderedSet orderedSetWithArray:sectionControllers];

        self.displayDelegate = self;
        self.scrollDelegate = self;

        [self reloadData];
    }
    return self;
}

The reloadData method is asking its children for the number of items, but the children don't have their data injected yet.

I can easily workaround the issue by explicitly setting the data (i.e. view model in my case) on the child section controller when I build up stacked section controller, but that seems like a hack unless I'm missing something.

Let me know what you think. If you agree its a bug, I'd be happy to create a unit test and fix the issue, but I probably need a little guidance as to the appropriate fix.

rnystrom commented 7 years ago

@jeffbailey doing some cleanup, if you're still interested in tackling this, that'd be awesome! As far as guidance goes, my bet is that we:

What do you think?

jeffbailey commented 7 years ago

I'll take a look!

jeffbailey commented 7 years ago

@rnystrom You're guidance on the fix was spot on. I submitted a pull request. The main thing to review is how I updated the unit tests to trigger the reloadData method before performing some of the tests. I called perform batch updates. Let me know if there's a better way!

jessesquires commented 7 years ago

done in 8d74b8f778d7104df7e3cf0dd513ceae1a69d880