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.

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

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

Closed
2 tasks done
jeffbailey opened this issue Dec 21, 2016 · 4 comments
Labels
Milestone

Comments

@jeffbailey
Copy link
Contributor

New issue checklist

General information

  • IGListKit version:
  • iOS version(s): 10.2
  • CocoaPods/Carthage version: master
  • Xcode version: 8.2
  • Devices/Simulators affected:
  • Reproducible in the demo project? (Yes/No): NO
  • Related issues:

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
Copy link
Contributor

rnystrom commented Jan 4, 2017

@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
Copy link
Contributor Author

I'll take a look!

@jeffbailey
Copy link
Contributor Author

@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!

facebook-github-bot pushed a commit that referenced this issue Jan 14, 2017
… numberOrItems before didUpdate is called

Summary:
The reloadData method in IGListStackedSectionController was being called too soon.  It was moved from the constructor to the `didUpdateToObject` method.  Tests were updated to reflect the change.

- [x] All tests pass. Demo project builds and runs.
- [x] I added tests, an experiment, or detailed why my change isn't tested.
- [ ] I added an entry to the `CHANGELOG.md` for any breaking changes, enhancements, or bug fixes.
- [x] I have reviewed the [contributing guide](https://github.com/Instagram/IGListKit/blob/master/.github/CONTRIBUTING.md)
Closes #390

Differential Revision: D4419751

Pulled By: rnystrom

fbshipit-source-id: 7c812d306b23dd251c160425873930eb8022b1a5
jessesquires pushed a commit that referenced this issue Jan 15, 2017
… numberOrItems before didUpdate is called

Summary:
The reloadData method in IGListStackedSectionController was being called too soon.  It was moved from the constructor to the `didUpdateToObject` method.  Tests were updated to reflect the change.

- [x] All tests pass. Demo project builds and runs.
- [x] I added tests, an experiment, or detailed why my change isn't tested.
- [ ] I added an entry to the `CHANGELOG.md` for any breaking changes, enhancements, or bug fixes.
- [x] I have reviewed the [contributing guide](https://github.com/Instagram/IGListKit/blob/master/.github/CONTRIBUTING.md)
Closes #390

Differential Revision: D4419751

Pulled By: rnystrom

fbshipit-source-id: 7c812d306b23dd251c160425873930eb8022b1a5
@jessesquires jessesquires added this to the 2.2.0 milestone Jan 15, 2017
@jessesquires
Copy link
Contributor

done in 8d74b8f

@jessesquires jessesquires modified the milestones: 2.2.0, 3.0.0 Feb 23, 2017
marciomeschini pushed a commit to marciomeschini/IGListKit that referenced this issue May 4, 2017
…d to know numberOrItems before didUpdate is called

Summary:
The reloadData method in IGListStackedSectionController was being called too soon.  It was moved from the constructor to the `didUpdateToObject` method.  Tests were updated to reflect the change.

- [x] All tests pass. Demo project builds and runs.
- [x] I added tests, an experiment, or detailed why my change isn't tested.
- [ ] I added an entry to the `CHANGELOG.md` for any breaking changes, enhancements, or bug fixes.
- [x] I have reviewed the [contributing guide](https://github.com/Instagram/IGListKit/blob/master/.github/CONTRIBUTING.md)
Closes Instagram#390

Differential Revision: D4419751

Pulled By: rnystrom

fbshipit-source-id: 7c812d306b23dd251c160425873930eb8022b1a5

# Conflicts:
#	CHANGELOG.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants