Added `-[IGListAdapter visibleCellsForObject:]` API #442

Sherlouk · 2017-01-22T10:17:07Z

Changes in this pull request

Added -[IGListAdapter visibleCellsForObject:] API

Approach

Get all visible cells
Get the section for the object
Filter cells where indexPath.section of cell is equal to the object
Return

Pull request checklist

All tests pass. Demo project builds and runs.
I added test(s), 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.
I have reviewed the contributing guide

Purely additive, can go in 2.2.0 release? Hold off on changelog until agreed.
Wanting to learn some more ObjC so gave this a shot, would appreciate some nit picking/feedback

Closes #437

zhubofei

LGTM 👍

Sherlouk · 2017-01-22T16:27:55Z

Turns out there's already a method for -[IGListCollectionContext visibleCellsForSectionController:]

https://instagram.github.io/IGListKit/Protocols/IGListCollectionContext.html#/c:objc(pl)IGListCollectionContext(im)visibleCellsForSectionController:

https://github.com/Instagram/IGListKit/blob/master/Source/IGListAdapter.m#L777-L788

Considering you can quite easily get the section controller from object using -[IGListAdapter sectionControllerForObject:] it doesn't seem too difficult to do this already?

@rnystrom Suggestions?

rnystrom · 2017-01-22T19:25:28Z

@Sherlouk doh. Ya, maybe we should just recommend people use that API instead...

Sherlouk · 2017-01-22T19:30:24Z

@rnystrom Was your API request, if you think the need is no longer necessary then close this and that issue 😄

rnystrom · 2017-01-22T19:33:35Z

Lol I know, forgot that API existed. Might still be valuable to cut out the middle-layer steps. Could also return early if sectionController is nil.

Sherlouk · 2017-01-22T19:37:41Z

Sorry, lost track here...

We saying in the old API to return early?
What steps do we want to take here

(Also pardon the test name 😂, only just realised I didn't change it)

rnystrom · 2017-01-22T22:01:31Z

The old API will end up asserting if the section controller is nil. If you used this method by first getting the object, then the SC (in ObjC) you might assert:

id controller = [self.adapter sectionControllerForObject:object]; // can be nil
NSArray *cells = [self.adapter visibleCellsForSectionController:controller]; // assert if nil

Which means all your callsites have to look like:

id controller = [self.adapter sectionControllerForObject:object]; // can be nil
if (controller != nil) {
  NSArray *cells = [self.adapter visibleCellsForSectionController:controller]; // assert if nil
}

Which is kind of gross. Swift will be just as bad. Options are:

Remove the assert
Add visibleCellsForObject: method that returns early if a section controller isn't found for the object

We should also add a unit test that sends nil to visibleCellsForSectionController: even tho the param marked nonnull, in ObJC-land that can still happen and the compiler wont help you.

rnystrom · 2017-01-23T15:11:28Z

@Sherlouk ok thought about it more. Let's do option 2 and move forward w/ this for 2.2 if you think that's doable 😄

Sherlouk · 2017-01-23T15:14:44Z

Option 2 is good, want to raise a new issue to track progress on amending old functionality?
Will add another unit test as well testing if an object is not found that it returns empty

rnystrom · 2017-01-23T15:11:50Z

Source/IGListAdapter.h

+
+ @return An array of collection view cells.
+ */
+- (NSArray<UICollectionViewCell *> *)visibleCellsForObject:(id)item;


nit: object as param name

rnystrom · 2017-01-23T15:13:29Z

Source/IGListAdapter.m

+    IGAssert(section != NSNotFound, @"Section not found for object %@", item);
+
+    NSPredicate *controllerPredicate = [NSPredicate predicateWithBlock:^BOOL(UICollectionViewCell* cell, NSDictionary* bindings) {
+        NSIndexPath *indexPath = [self.collectionView indexPathForCell:cell];


nit: calling self.collectionView inside this block will execute an extra objc_msgSend(...) every execution. Instead let's capture the collection view in a local variable that the block uses:

UICollectionView *collectionView = self.collectionView; NSPredicate *predicate = [... ^{ NSIndexPath *path = [collectionView indexPathForCell:cell]; }];

rnystrom · 2017-01-23T15:14:00Z

Source/IGListAdapter.m

+
+    NSArray<UICollectionViewCell *> *visibleCells = [self.collectionView visibleCells];
+    NSInteger section = [self.sectionMap sectionForObject:item];
+    IGAssert(section != NSNotFound, @"Section not found for object %@", item);


Actually let's remove this. A use might be holding a ref to an expired object. I think it's ok to return an empty array in that case.

rnystrom · 2017-01-23T15:14:04Z

Source/IGListAdapter.m

+    IGParameterAssert(item != nil);
+
+    NSArray<UICollectionViewCell *> *visibleCells = [self.collectionView visibleCells];
+    NSInteger section = [self.sectionMap sectionForObject:item];


nit: const

rnystrom · 2017-01-23T15:14:46Z

Source/IGListAdapter.m

+    }];
+
+    NSArray<UICollectionViewCell *> *filteredCells = [visibleCells filteredArrayUsingPredicate:controllerPredicate];
+    return filteredCells;


nit: IMO just return [visibleCells filteredArrayUsingPredicate:controllerPredicate];

Sherlouk · 2017-01-23T15:18:21Z

All valid comments, will work on it tonight!

facebook-github-bot · 2017-01-23T17:22:19Z

@Sherlouk updated the pull request - view changes

Sherlouk · 2017-01-23T17:23:37Z

@rnystrom Just updated, should address all of your points!

Not sure if this is the best code to return an empty array, but it does seem to work?

return [NSArray new];

jessesquires

Looks good to me.

Let's add a changelog for 2.2.0

facebook-github-bot · 2017-01-23T17:42:12Z

@Sherlouk updated the pull request - view changes

facebook-github-bot · 2017-01-23T18:27:00Z

@jessesquires has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2017-01-23T18:27:17Z

@jessesquires has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Summary: - Added `-[IGListAdapter visibleCellsForObject:]` API 1. Get all visible cells 2. Get the section for the object 3. Filter cells where indexPath.section of cell is equal to the object 4. Return - [x] All tests pass. Demo project builds and runs. - [x] I added test(s), 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) ------------- Purely additive, can go in 2.2.0 release? Hold off on changelog until agreed. Wanting to learn some more ObjC so gave this a shot, would appreciate some nit picking/feedback Closes #437 Closes #442 Differential Revision: D4450259 Pulled By: jessesquires fbshipit-source-id: 521583c669fc1160212a7c46a50d62d951cafa2e

Sherlock added 3 commits January 21, 2017 20:54

Attempt at visibleCellsForObject

4f41c9e

Added test for visibleCellsForObject function

50f2289

Added param to doc

595b65d

Sherlouk added this to the 2.2.0 milestone Jan 22, 2017

Sherlouk requested review from rnystrom and zhubofei January 22, 2017 10:17

facebook-github-bot added the CLA Signed label Jan 22, 2017

zhubofei approved these changes Jan 22, 2017

View reviewed changes

rnystrom suggested changes Jan 23, 2017

View reviewed changes

Return empty array on not found, add extra test

816339e

jessesquires approved these changes Jan 23, 2017

View reviewed changes

jessesquires added the enhancement label Jan 23, 2017

Added changelog entry

9fd6b02

facebook-github-bot closed this in 528bd33 Jan 23, 2017

jessesquires removed this from the 2.2.0 milestone Feb 23, 2017

jessesquires modified the milestones: 3.0.0, 2.2.0 Feb 23, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added `-[IGListAdapter visibleCellsForObject:]` API #442

Added `-[IGListAdapter visibleCellsForObject:]` API #442

Sherlouk commented Jan 22, 2017

zhubofei left a comment

Sherlouk commented Jan 22, 2017

rnystrom commented Jan 22, 2017

Sherlouk commented Jan 22, 2017

rnystrom commented Jan 22, 2017

Sherlouk commented Jan 22, 2017

rnystrom commented Jan 22, 2017

rnystrom commented Jan 23, 2017

Sherlouk commented Jan 23, 2017

rnystrom Jan 23, 2017

rnystrom Jan 23, 2017

rnystrom Jan 23, 2017

rnystrom Jan 23, 2017

rnystrom Jan 23, 2017

Sherlouk commented Jan 23, 2017

facebook-github-bot commented Jan 23, 2017

Sherlouk commented Jan 23, 2017

jessesquires left a comment

facebook-github-bot commented Jan 23, 2017

facebook-github-bot commented Jan 23, 2017

facebook-github-bot commented Jan 23, 2017

Added -[IGListAdapter visibleCellsForObject:] API #442

Added -[IGListAdapter visibleCellsForObject:] API #442

Conversation

Sherlouk commented Jan 22, 2017

Changes in this pull request

Approach

Pull request checklist

zhubofei left a comment

Choose a reason for hiding this comment

Sherlouk commented Jan 22, 2017

rnystrom commented Jan 22, 2017

Sherlouk commented Jan 22, 2017

rnystrom commented Jan 22, 2017

Sherlouk commented Jan 22, 2017

rnystrom commented Jan 22, 2017

rnystrom commented Jan 23, 2017

Sherlouk commented Jan 23, 2017

rnystrom Jan 23, 2017

Choose a reason for hiding this comment

rnystrom Jan 23, 2017

Choose a reason for hiding this comment

rnystrom Jan 23, 2017

Choose a reason for hiding this comment

rnystrom Jan 23, 2017

Choose a reason for hiding this comment

rnystrom Jan 23, 2017

Choose a reason for hiding this comment

Sherlouk commented Jan 23, 2017

facebook-github-bot commented Jan 23, 2017

Sherlouk commented Jan 23, 2017

jessesquires left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Jan 23, 2017

facebook-github-bot commented Jan 23, 2017

facebook-github-bot commented Jan 23, 2017

Added `-[IGListAdapter visibleCellsForObject:]` API #442

Added `-[IGListAdapter visibleCellsForObject:]` API #442