Support cells created from nibs #56

svenbacia · 2016-10-12T12:28:54Z

Changes in this pull request

I started working on adding support for dequeuing cells created from nibs (issue #1). Additionally I extended IGListSingleSectionController so that it can be used with nibs too. I don't know if you had this also in mind.

Missing

I'm currently thinking about the best way to test these changes.
I was not able to update the documentation (issue Error when running ./build_docs.sh #55).

Pull request checklist

All tests pass. Demo project builds and runs.
I added tests, an experiment, or detailed why my change isn't tested.
I have reviewed the contributing guide

facebook-github-bot · 2016-10-12T12:29:38Z

Thank you for your pull request. As you may know, we require contributors to sign our Contributor License Agreement, and we don't seem to have you on file and listed as active anymore. In order for us to review and merge your code, please email cla@fb.com with your details so we can update your status.

facebook-github-bot · 2016-10-12T12:32:57Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

rnystrom · 2016-10-12T14:12:07Z

Source/IGListAdapter.m

+                                               cellClass:(Class)cellClass
+                                    forSectionController:(IGListSectionController<IGListSectionType> *)sectionController
+                                                 atIndex:(NSInteger)index
+{


nit: Can you put the curly on the same line?

rnystrom · 2016-10-12T14:14:59Z

Source/IGListCollectionContext.h

+ @note This method uses a string representation of the cell class as the identifier.
+ */
+- (__kindof UICollectionViewCell *)dequeueReusableCellFromNibName:(NSString *)nibName
+                                                           bundle:(NSBundle * _Nullable)bundle


nit: we've been using the format (nullable NSBundle *) elsewhere in the app

rnystrom · 2016-10-12T14:16:24Z

Source/IGListSingleSectionController.h

+                         bundle:(NSBundle * _Nullable)bundle
+                      cellClass:(Class)cellClass
+                 configureBlock:(IGListSingleSectionCellConfigureBlock)configureBlock
+                      sizeBlock:(IGListSingleSectionCellSizeBlock)sizeBlock;


cc @jessesquires on adding another initializer. I prefer having a single init, but that'll mean a breaking change.

It's early yet, I think we should not be afraid of a few breaking changes to start with?

Ya, let's break it. @svenbacia mind changing this so there's only a single init? Note that this will break the sample apps and unit tests, but chasing the compiler errors should be easy.

another option:

we could have 2 initializers for a 1.1.0 non-breaking release. then, open a task to track combining these into 1 initializer for a 2.0.0 (breaking) release.

rnystrom · 2016-10-12T14:16:31Z

Source/IGListSingleSectionController.h

+ (usually "self") or the IGListAdapter. Pass in locally scoped objects or use weak references!
+ */
+- (instancetype)initWithNibName:(NSString *)nibName
+                         bundle:(NSBundle * _Nullable)bundle


(nullable NSBundle *)

rnystrom · 2016-10-12T14:17:14Z

Source/Internal/IGListAdapterInternal.h

@@ -18,8 +18,8 @@
 NS_ASSUME_NONNULL_BEGIN

 /// Generate a string representation of a reusable view class when registering with a UICollectionView.
-NS_INLINE NSString *IGListReusableViewIdentifier(Class viewClass, NSString * _Nullable kind) {
-  return [NSString stringWithFormat:@"%@%@", kind ?: @"", NSStringFromClass(viewClass)];
+NS_INLINE NSString *IGListReusableViewIdentifier(Class viewClass, NSString * _Nullable nibName, NSString * _Nullable kind) {


nullable NSString * x2

FYI: nullable isn't available in C functions :)

Bikeshed: what's the thinking on why this should be inline and header-defined? Any possible computational speed benefits are nullified by the first msgSend you do inside the function body.

@kylehickinson TIL, maybe we should change all of them to _Nullable then so we're consistent.... sounds like another good starter-task.

@ocrickard We could extern it, but its exposed in the internal header for unit testing in IGListAdapterTests.m

@rnystrom

maybe we should change all of them to _Nullable

Eh, I don't think so. nullable is preferred for ObjC contexts, _Nullable is preferred for C contexts

rnystrom · 2016-10-12T14:22:35Z

This is really, really great. I know the community is going to want this feature ASAP. Aside from my notes on the review, a couple other things we need to do:

Gen the documentation (jazzy is sort of broken w/ the dir structure, see Error when running ./build_docs.sh #55 for a temp workaround)
Add unit tests
- Add a test nib as a resource to the tests target
- Unit test using cells (test that they are created and configured)
- Unit test the single item controller (which I totally didn't think about!)
Optional: Add an example to the samples app
- This could be something really simple, but maybe a cell w/ some autolayout labels or w/e
Add a line to the changelog
- Since we don't have any examples, take a look at @jessesquires's CHANGELOG for an idea of the format/content

facebook-github-bot · 2016-10-12T15:48:51Z

@svenbacia updated the pull request - view changes

jessesquires · 2016-10-12T16:01:48Z

Gen the documentation

I'd personally prefer this in a separate PR. We should do this right before we push a new release with these changes. Otherwise, our docs and code are inconsistent. (That is, most people will be using CocoaPods/Carthage, pinned to a release tag, not using master. I think published docs should be for a release, not for master.)

rnystrom · 2016-10-12T16:18:40Z

@jessesquires interesting, I'm down w/ that workflow. I should detail that in the contributing docs. Should we revert it here?

jessesquires · 2016-10-12T16:24:33Z

Up to you.

I probably would, but this is also a minor change -- and it looks like we're going to push 2.0.0 out soon? (like probably this week or next?)

facebook-github-bot · 2016-10-12T17:51:59Z

@svenbacia updated the pull request - view changes

svenbacia · 2016-10-12T18:34:56Z

Thanks for the feedback. I think I covered all open points. Please let me know in case there are still something missing/wrong.

jessesquires · 2016-10-12T21:16:10Z

CHANGELOG.md

@@ -2,6 +2,17 @@

 The changelog for `IGListKit`. Also see the [releases](https://github.com/instagram/IGListKit/releases) on GitHub.

+x.y.z


this will be 2.0.0

facebook-github-bot · 2016-10-13T06:40:38Z

@svenbacia updated the pull request - view changes

rnystrom · 2016-10-13T13:26:11Z

CHANGELOG.md

+-----
+### Breaking Changes
+
+- Extended the initializer for `IGListSingleSectionController` to be able to support single sections created from nibs. Therefore the initializer now takes two new arguments `nibName` and `bundle`. An example can be found [here](Example/IGListKitExamples/ViewControllers/SingleSectionViewController.swift).


rnystrom · 2016-10-13T13:27:35Z

Example/IGListKitExamples/ViewControllers/SingleSectionViewController.swift

+            data.append(index)
+        }
+
+        collectionView.backgroundColor = .white


We can remove this, IGListCollectionView sets the background to white by default

rnystrom · 2016-10-13T13:28:04Z

Example/IGListKitExamples/ViewControllers/SingleSectionViewController.swift

+    }()
+    let collectionView = IGListCollectionView(frame: CGRect.zero, collectionViewLayout: UICollectionViewFlowLayout())
+
+    let rowCount = 20


nit: Let's remove this variable and just do for index in 0..<20 below, simpler

rnystrom · 2016-10-13T13:30:00Z

Example/IGListKitExamples/ViewControllers/SingleSectionViewController.swift

+
+    func didSelect(_ sectionController: IGListSingleSectionController) {
+        let section = adapter.section(for: sectionController) + 1
+        let alert = UIAlertController(title: "Section \(section) was selected 🎉", message: nil, preferredStyle: .alert)


I love this!

rnystrom · 2016-10-13T13:32:25Z

Source/IGListAdapter.m

+    NSIndexPath *indexPath = [self indexPathForSectionController:sectionController index:index];
+    if (![self.registeredCellClasses containsObject:cellClass]) {
+        [self.registeredCellClasses addObject:cellClass];
+        UINib *nib = [UINib nibWithNibName:nibName bundle:bundle];


Noting to myself that we don't need an assert to check if the nib exists b/c according to the docs:

An exception is thrown if there were errors during initialization or the nib file could not be located.

rnystrom · 2016-10-13T13:38:00Z

Source/IGListCollectionContext.h

+ Dequeues a cell from the UICollectionView reuse pool.
+
+ @param nibName           The name of the nib file.
+ @param bundle            The bundle in which to search for the nib file.


nit: Add what happens when this is nil:

If nil, this method looks for the nib file in the main bundle.

rnystrom · 2016-10-13T13:42:46Z

Source/IGListCollectionContext.h

+ */
+- (__kindof UICollectionViewCell *)dequeueReusableCellFromNibName:(NSString *)nibName
+                                                           bundle:(nullable NSBundle *)bundle
+                                                        cellClass:(Class)cellClass


Can we omit this parameter? I know you're using it to lookup registration in the registeredCellClasses and create the cell identifier, but we could:

Add another registered set NSMutableSet<NSString *> *registeredNibNames

Use the nib name string to lookup if the nib has been registered

Use the nib name string as the cell identifier

That'd reduce the complexity and number of parameters for this method, making a simpler API. What do you think? cc @jessesquires

You are right. Given that we are returning a UICollectionViewCell in either case we don't actually need the class 👍

I started implementing this and I realized that the initializer for IGListSingleSectionController now has 3 nullable parameters since we no longer need the cellClass for nib cells. Is this too confusing for the user? Maybe we should split the initializer.

- (instancetype)initWithCellClass:(nullable Class)cellClass nibName:(nullable NSString *)nibName bundle:(nullable NSBundle *)bundle configureBlock:(IGListSingleSectionCellConfigureBlock)configureBlock sizeBlock:(IGListSingleSectionCellSizeBlock)sizeBlock NS_DESIGNATED_INITIALIZER;

rnystrom · 2016-10-13T13:44:12Z

Making really great progress on this. This and storyboard support are easily the most wanted features right now. Going to make a lot of people happy!

facebook-github-bot · 2016-10-14T16:43:09Z

@svenbacia updated the pull request - view changes

facebook-github-bot · 2016-10-14T16:57:19Z

@svenbacia updated the pull request - view changes

rnystrom · 2016-10-14T17:45:51Z

Awesome!! @svenbacia last couple things then I think we're g2g:

Looks like it needs a rebase to fix the conflicts in CHANGELOG.md
~~Can you squash all the commits into a single commit?~~

edit: nvm that last one, I'll try to import as-is and see what happens

svenbacia · 2016-10-14T18:10:32Z

Great! Let me know in case you need all commits in a single one.

svenbacia · 2016-10-14T18:18:22Z

I just realized that due to the latest changes to the IGListSingleSectionController there are no breaking changes. I will update the CHANGELOG.

facebook-github-bot · 2016-10-14T18:21:48Z

@svenbacia updated the pull request - view changes

rnystrom · 2016-10-14T19:41:58Z

CHANGELOG.md

+2.0.0
+-----
+
+### Enhancements


Can you rebase and put this all under "Master"? I'm going to change the version in another diff

facebook-github-bot · 2016-10-14T20:28:07Z

@svenbacia updated the pull request - view changes

svenbacia · 2016-10-14T20:29:10Z

First time I used rebase. I hope this is what you expected. At least the conflict is gone.

rnystrom · 2016-10-14T20:39:50Z

@svenbacia last request (I promise) is to revert the changes to docs/*. I'm going to run the script in a separate diff.

Rebase looks like it worked!

facebook-github-bot · 2016-10-14T20:47:22Z

@svenbacia updated the pull request - view changes

svenbacia · 2016-10-14T20:47:26Z

@rnystrom Done :)

facebook-github-bot · 2016-10-14T20:48:46Z

Thanks for importing. If you are a Facebook employee, you can view this diff on Phabricator.

rnystrom · 2016-10-14T21:22:40Z

Import is landing! 💪

I made a couple changes off of @dshahidehpour's internal review. You can check them out once the commit is pushed, but notably:

Renamed method from dequeueReusableCellFromNibName: to dequeueReusableCellWithNibName:
Changelog version to 1.1.0
Checking string length instead of nil string
Store property used twice in method in a local var

facebook-github-bot added the CLA Signed label Oct 12, 2016

rnystrom reviewed Oct 12, 2016

View reviewed changes

facebook-github-bot added the CLA Signed label Oct 12, 2016

rnystrom suggested changes Oct 12, 2016

View reviewed changes

rnystrom assigned svenbacia Oct 12, 2016

facebook-github-bot added CLA Signed labels Oct 12, 2016

facebook-github-bot added the CLA Signed label Oct 12, 2016

jessesquires reviewed Oct 12, 2016

View reviewed changes

jessesquires modified the milestone: 2.0.0 Oct 12, 2016

facebook-github-bot added the CLA Signed label Oct 12, 2016

facebook-github-bot added the CLA Signed label Oct 13, 2016

rnystrom suggested changes Oct 13, 2016

View reviewed changes

facebook-github-bot added the CLA Signed label Oct 13, 2016

jessesquires mentioned this pull request Oct 13, 2016

Added CHANGELOG entry for #51 #67

Closed

3 tasks

facebook-github-bot added the CLA Signed label Oct 14, 2016

ElizabethmParker approved these changes Oct 14, 2016

View reviewed changes

facebook-github-bot added the CLA Signed label Oct 14, 2016

rnystrom suggested changes Oct 14, 2016

View reviewed changes

facebook-github-bot added the CLA Signed label Oct 14, 2016

adds support for cells created from nibs

195a785

facebook-github-bot added the CLA Signed label Oct 14, 2016

facebook-github-bot closed this in ce4b851 Oct 15, 2016

This was referenced Oct 20, 2016

Support supplementaryViews created from nibs #90

Closed

Add storyboard support #39 #92

Closed

Cannot create cells from Nib in Cocoapods release #104

Closed

Sherlouk mentioned this pull request Dec 6, 2016

Fix "tbd" revisions in changelog just before 2.0 #278

Closed

svenbacia deleted the feature/support-cells-from-nibs branch December 12, 2016 22:06

		@@ -2,6 +2,17 @@

		The changelog for `IGListKit`. Also see the [releases](https://github.com/instagram/IGListKit/releases) on GitHub.

		x.y.z

Support cells created from nibs #56

Support cells created from nibs #56

Conversation

svenbacia commented Oct 12, 2016 • edited Loading

Changes in this pull request

Missing

Pull request checklist

facebook-github-bot commented Oct 12, 2016

facebook-github-bot commented Oct 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnystrom Oct 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnystrom commented Oct 12, 2016 • edited Loading

facebook-github-bot commented Oct 12, 2016

jessesquires commented Oct 12, 2016

rnystrom commented Oct 12, 2016

jessesquires commented Oct 12, 2016

facebook-github-bot commented Oct 12, 2016

svenbacia commented Oct 12, 2016

Choose a reason for hiding this comment

facebook-github-bot commented Oct 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svenbacia Oct 13, 2016 • edited Loading

Choose a reason for hiding this comment

rnystrom commented Oct 13, 2016

facebook-github-bot commented Oct 14, 2016

facebook-github-bot commented Oct 14, 2016

rnystrom commented Oct 14, 2016 • edited Loading

svenbacia commented Oct 14, 2016

svenbacia commented Oct 14, 2016

facebook-github-bot commented Oct 14, 2016

Choose a reason for hiding this comment

facebook-github-bot commented Oct 14, 2016

svenbacia commented Oct 14, 2016

rnystrom commented Oct 14, 2016

facebook-github-bot commented Oct 14, 2016

svenbacia commented Oct 14, 2016

facebook-github-bot commented Oct 14, 2016

rnystrom commented Oct 14, 2016

svenbacia commented Oct 12, 2016 •

edited

Loading

rnystrom Oct 12, 2016 •

edited

Loading

rnystrom commented Oct 12, 2016 •

edited

Loading

svenbacia Oct 13, 2016 •

edited

Loading

rnystrom commented Oct 14, 2016 •

edited

Loading