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

[ASCollectionView] Fix index space translation of Flow Layout Delegate methods. #467

Merged
merged 4 commits into from
Oct 9, 2017

Conversation

appleguy
Copy link
Member

This includes a few other cleanups, including overflow of signed integer indices.

It will take a couple PRs to fully sync both upstream and downstream in this case, but I am happy to integrate feedback along the way. Just a heads up that it may be easier to do in subsequent PRs, depending on the importance of changes (stylistic vs functional).

…e methods.

This includes a few other cleanups, including overflow of signed integer indices.
Copy link
Member

@Adlai-Holler Adlai-Holler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love it. Couple notes and then it'll be landing time.


for (ASCollectionElement *element in self.dataController.pendingMap.itemElements) {
ASCellNode *node = element.node;
if (node.shouldUseUIKitCell) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. It's probably inconsequential, but maybe we should just ignore calls where the layout isn't a flow layout, rather than using size 0?

  2. Instead of building a giant array – I don't really like the itemElements property and may propose removing it – it's better to enumerate the map directly and if (element.supplementaryElementKind) { continue; } Note to self: add a UICollectionElementCategory elementCategory property to ASCollectionElement to simplify this distinction.

  3. Shouldn't we also refetch header/footer reference sizes here? Meshes well with 2

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Adlai-Holler some merge conflicts have arisen, and I'm afraid my time availability is nearing a local minimum. :( Do you think this PR is acceptable to land as-is if the conflicts are fixed?

  1. I think it's better to have the 0 default, but indeed maybe no one will ever know :)
  2. The iteration suggestion sounds like an improvement for sure. I'm not sure I can commit to doing it only because I would have to get some test cases running to exercise the change.
  3. This seems potentially important, and indeed might motivate me to do Test PR #2.

Could you give me an Accept if you are comfortable with this, and then if I do 2 or 3, I can land without waiting?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Adlai-Holler It took me a while, but I finally got around to implementing your advice — and went quite a bit further. Thanks for suggesting these things, as they are improvements.

In particular, I was intentionally skipping over handling the update of supplementary element sizes, which is not great. That's now fixed and the code is better factored.

If you or @nguyenhuy can give me a sanity check review soon, I'd appreciate that - would like to merge this in pretty soon.

}
return ASFlowLayoutDefault(l, minimumInteritemSpacing, 10.0); // Default is documented as 10.0
}

- (CGFloat)collectionView:(UICollectionView *)cv layout:(UICollectionViewLayout *)l
minimumLineSpacingForSectionAtIndex:(NSInteger)section
minimumLineSpacingForSectionAtIndex:(NSInteger)section
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙂

#define ASDisplayNodeCAssertPositiveReal(description, num) ASDisplayNodeCAssert(num >= 0 && num <= CGFLOAT_MAX, @"%@ must be a real positive integer.", description)
#define ASDisplayNodeCAssertInfOrPositiveReal(description, num) ASDisplayNodeCAssert(isinf(num) || (num >= 0 && num <= CGFLOAT_MAX), @"%@ must be infinite or a real positive integer.", description)
#define ASDisplayNodeCAssertPositiveReal(description, num) ASDisplayNodeCAssert(num >= 0 && num <= CGFLOAT_MAX, @"%@ must be a real positive integer: %f.", description, (CGFloat)num)
#define ASDisplayNodeCAssertInfOrPositiveReal(description, num) ASDisplayNodeCAssert(isinf(num) || (num >= 0 && num <= CGFLOAT_MAX), @"%@ must be infinite or a real positive integer: %f.", description, (CGFloat)num)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙌

@ghost
Copy link

ghost commented Oct 8, 2017

🚫 CI failed with log

…nsure delegate invalidation re-fetches supplementary sizes too.
@appleguy
Copy link
Member Author

appleguy commented Oct 8, 2017

Hmm, this erroneous test failure is striking again.

ASCollectionViewTests
testThatMultipleBatchFetchesDontHappenUnnecessarily, failed - Too many batch fetches!
/Users/Shared/buildkite/builds/iosf-garrett-VMrmmvuqKsO4.dyn.pinadmin.com-1/pinterest/texture/Tests/ASCollectionViewTests.mm:894

  if (batchFetchCount > 1) {
    XCTFail(@"Too many batch fetches!");

@nguyenhuy nguyenhuy merged commit c6e3dd7 into master Oct 9, 2017
@appleguy appleguy deleted the ASCV_Passthrough branch October 10, 2017 06:10
@appleguy
Copy link
Member Author

Thanks for reviewing and merging!

bernieperez pushed a commit to AtomTickets/Texture that referenced this pull request Apr 25, 2018
…e methods. (TextureGroup#467)

* [ASCollectionView] Fix index space translation of Flow Layout Delegate methods.

This includes a few other cleanups, including overflow of signed integer indices.

* [ASCollectionView] Improve code sharing of UIKit size method calls; ensure delegate invalidation re-fetches supplementary sizes too.

* [ASCollectionView] Final method ordering and doc-comment for new _sizeForUIKitCellWithKind:atIndexPath: method.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants