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

[example/CustomCollectionView] Implement MosaicCollectionLayoutDelegate #28

Merged
merged 4 commits into from May 14, 2017

Conversation

nguyenhuy
Copy link
Member

@nguyenhuy nguyenhuy commented Apr 18, 2017

This is the first step in turning CustomCollectionView example into a playground for the coming async pager layout. It also helps identify components in the current system that don't play well with ASCollectionLayout and need to be improved/by-passed, such as ASRangeController, layout inspector, etc.

Ticket: #186

// This source code is licensed under the BSD-style license found in the
// LICENSE file in the root directory of this source tree. An additional grant
// of patent rights can be found in the PATENTS file in the same directory.
// Modifications to this file made after 4/13/2017 are: Copyright (c) 2017-present,
Copy link
Member Author

Choose a reason for hiding this comment

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

This new file uses many logics from MosaicCollectionViewLayout.m, hence this version of Copyright Notice.

return [ASInsetLayoutSpec insetLayoutSpecWithInsets:UIEdgeInsetsZero child:_imageNode];
return [ASInsetLayoutSpec insetLayoutSpecWithInsets:UIEdgeInsetsZero
child:[ASRatioLayoutSpec ratioLayoutSpecWithRatio:self.image.size.height/self.image.size.width
child:_imageNode]];
Copy link
Member

Choose a reason for hiding this comment

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

Just to demonstrate best practice, might be worth having a CGSize imageSize = at the top

ASElementMap *elements = context.elements;
CGFloat top = 0;

NSMapTable<ASCollectionElement *, UICollectionViewLayoutAttributes *> *attrsMap = [NSMapTable mapTableWithKeyOptions:(NSMapTableObjectPointerPersonality | NSMapTableWeakMemory) valueOptions:NSMapTableStrongMemory];
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit complex for sample code -- add a comment above for why it isn't just a dictionary?


attrs.frame = frame;
[attrsMap setObject:attrs forKey:element];
columnHeights[section][columnIndex] = @(CGRectGetMaxY(frame) + _interItemSpacing.bottom);
Copy link
Member

Choose a reason for hiding this comment

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

Vector might be worthwhile to avoid boxing - try profiling this first and look for object retain / release overhead. Apps will probably borrow this sample code and ship it :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point! Since this method is only called once for each data set, and in many cases on a background thread, I left a TODO instead of optimizing now.

Copy link
Member

@appleguy appleguy left a comment

Choose a reason for hiding this comment

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

@nguyenhuy this is so exciting! I can't wait to try running it locally.

I think it would be worth a couple more performance optimizations for demonstrating best practices in core code, but this is subjective and not a correctness issue.

@ghost
Copy link

ghost commented May 3, 2017

🚫 CI failed with log

@garrettmoon
Copy link
Member

Looks like this needs a rebase.

@ghost
Copy link

ghost commented May 4, 2017

1 Warning
⚠️ This is a big PR, please consider splitting it up to ease code review.

Generated by 🚫 Danger

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.

None yet

4 participants