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.

Sign up for GitHub

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

[ASDKgram Example] fix crash on startup #566

Merged
merged 6 commits into from
Sep 16, 2017
Merged

[ASDKgram Example] fix crash on startup #566

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6210bdd
Merge remote-tracking branch 'TextureGroup/master'
hannahmbanana Jun 13, 2017
a3af47a
Merge remote-tracking branch 'TextureGroup/master'
hannahmbanana Jun 15, 2017
7ea3481
Merge remote-tracking branch 'TextureGroup/master'
hannahmbanana Jul 3, 2017
0a28259
Merge remote-tracking branch 'TextureGroup/master'
hannahmbanana Sep 14, 2017
30d29f6
fix crash on ASDKgram startup
hannahmbanana Sep 14, 2017
d2a1886
quicker fix
hannahmbanana Sep 14, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions examples/ASDKgram/Sample/PhotoFeedBaseController.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ - (void)refreshFeed

[_activityIndicatorView stopAnimating];

[self insertNewRows:newPhotos];
[self.tableView reloadData];
Copy link
Member

@nguyenhuy nguyenhuy Sep 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I would personally shy away from calling reloadData in a sample project, as we've been asking people to avoid using it and call insert/delete/move instead. Maybe it's time to employ a diffing algorithm (either built-in or from 3rd-party) to show how it's supposed to be done? A bit overkill I know, as the data almost certainly has new rows only.

Copy link
Member

Choose a reason for hiding this comment

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

@nguyenhuy Yes, I agree. A couple thoughts though:

  • We do have the IGListKit version of ASDKgram in place; we could consider making it the only implementation, although I'm not sure how easy it is for a new developer to get set up using it with ASCollectionNode, because we don't have docs about it.

  • This fix addresses a pretty serious crash that has been in place for a while. I asked Hannah offline and she is planning to make other changes to ensure only edit operations are used, but the network / loading code needs a small overhaul to be able to do it.

Fortunately the reloadData only occurs during a full reload (e.g. pull to refresh or first load), so it doesn't affect the loading of new batches / pages while scrolling down.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good 👌

[self requestCommentsForPhotos:newPhotos];

// immediately start second larger fetch
Expand All @@ -74,7 +74,8 @@ - (void)insertNewRows:(NSArray *)newPhotos
NSMutableArray *indexPaths = [NSMutableArray array];

NSInteger newTotalNumberOfPhotos = [_photoFeed numberOfItemsInFeed];
for (NSInteger row = newTotalNumberOfPhotos - newPhotos.count; row < newTotalNumberOfPhotos; row++) {
NSInteger existingNumberOfPhotos = newTotalNumberOfPhotos - newPhotos.count;
for (NSInteger row = existingNumberOfPhotos; row < newTotalNumberOfPhotos; row++) {
NSIndexPath *path = [NSIndexPath indexPathForRow:row inSection:section];
[indexPaths addObject:path];
}
Expand Down