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

Question about using IGListKit with Photos framework #454

Closed
2 tasks done
andreamazz opened this issue Jan 30, 2017 · 16 comments
Closed
2 tasks done

Question about using IGListKit with Photos framework #454

andreamazz opened this issue Jan 30, 2017 · 16 comments
Labels

Comments

@andreamazz
Copy link

andreamazz commented Jan 30, 2017

I'm trying to use IGListKit to present a library picker that fetches the user's pictures from the camera roll, so my datasource is basically a PHFetchResult<PHAsset> instance, that fetches the single images asynchronously:

class LibraryItem: IGListDiffable {
  var id = 0
  var assets: PHFetchResult<PHAsset>?

  convenience init(id: Int, with assets: PHFetchResult<PHAsset>?) {
    self.init()
    self.id = id
    self.assets = assets
  }

  public func diffIdentifier() -> NSObjectProtocol {
    return NSNumber(value: id)
  }

  public func isEqual(toDiffableObject object: IGListDiffable?) -> Bool {
    guard let object = object as? LibraryItem else { return false }
    return id != object.id
  }
}

For the id I use a timestamp of the latest fetch.

The section controller is something along these lines:

extension LibrarySectionController: IGListSectionType {
  func numberOfItems() -> Int {
    return libraryItem?.assets?.count ?? 0
  }

  func cellForItem(at index: Int) -> UICollectionViewCell {
    let cell = collectionContext!.dequeueReusableCell(of: LibraryCell.self, for: self, at: index)

    if let cell = cell as? LibraryCell, let asset = libraryItem?.assets?.object(at: index), let imageManager = imageManager {
      let requestOptions = PHImageRequestOptions()
      requestOptions.isNetworkAccessAllowed = true
      imageManager.requestImage(for: asset, targetSize: sizeForItem(at: index), contentMode: .aspectFill, options: requestOptions) { image, _ in
        cell.setup(with: image)
      }
    }
    return cell
  }

  func didUpdate(to object: Any) {
    libraryItem = object as? LibraryItem
  }
}

This works fine, but when an update to the camera roll is triggered, the whole collection view is refreshed (and you can see the cell "flashing" a couple of times the re-render) since the diffing considers the whole library changed.

I was wondering if there is a better approach to this problem. Thanks.

@zhubofei
Copy link

zhubofei commented Jan 30, 2017

@andreamazz I think the last line of the function,

public func isEqual(toDiffableObject object: IGListDiffable?) -> Bool {
  guard let object = object as? LibraryItem else { return false }
  return id != object.id
}

should be id == object.id.

@andreamazz
Copy link
Author

Hi @zhubofei
Yes, you're right, it's id == object.id, sorry about that. In any case, even with that change the result is the same, the whole library is refreshed, and there is a strange 'flashing' behaviour.
This is how I update the adapter:

extension LibraryViewController: PHPhotoLibraryChangeObserver {
  public func photoLibraryDidChange(_ changeInstance: PHChange) {
    assets = PHAsset.fetchAssets(with: sortDescriptor)
    data = [LibraryItem(id: Int(Date().timeIntervalSince1970), with: assets)]
    DispatchQueue.main.async {
      self.adapter.performUpdates(animated: true, completion: nil)
    }
  }
}

@Sherlouk
Copy link
Contributor

Unless I'm mistaken IGLK will refresh on a section by section basis by default, your implementation is designed around one section which has many cells when in reality the data structure should be changed to be 1 section per asset (in your case).

The reason it's always refreshing is because your library item ID is the time, which is always changing. So from IGLK point of view, it's a completely different item and will do a full refresh.

@andreamazz
Copy link
Author

andreamazz commented Jan 30, 2017

@Sherlouk thanks for your answer. This leads to the question that I had in mind when I decided to proceed with the single section: since the datasource needs to provide a [IGListDiffable], I'll need to cycle through all the PHFetchResults and store each PHAsset in its own diffable object, correct? My fear is that this approach might be highly inefficient.

Also, with an item per section would it be possible to achieve a grid layout? (like the stock photo library).

Thank you for you time.

@Sherlouk
Copy link
Contributor

Yes that would mean some sort of iterative creation of items from the results you get, without a much bigger look into your implementation/actually how the photo library stuff works I couldn't give a very good answer on how best to do it

It's possible to do a grid layout if an item per section yes, in fact @zhubofei created a custom layout just for this (which is packaged with IGListKit). See https://github.com/Instagram/IGListKit/blob/master/Examples/Examples-iOS/IGListKitExamples/ViewControllers/StoryboardViewController.swift. (Uses the layout)

@andreamazz
Copy link
Author

Great, thank you @Sherlouk
I'll try to switch to a single cell per section and check if the performances are ok. I hope to share the results, since accessing the photo library is a fairly common task.
I'll close this for now. Thanks again.

@zhubofei
Copy link

zhubofei commented Jan 30, 2017

@andreamazz

  • I think each PHAsset object already has its own localIdentifier, you can use that as your diffIdentifier.
  • For performance, you can load a few photos first and load more as user scrolling down. There is an example here.
  • Not sure if IGListKit is the best choice for this use case. In PHPhotoLibraryChangeObserver, Apple has already provided us with a diffing method sample code.

@andreamazz
Copy link
Author

@zhubofei
The problem with Apple's code is that it's prone to crash, a lot :)
I've implemented it in a couple of apps, you can test it for weeks, but once the app goes live the crash reports start pouring in (all due to inconsistency exceptions raised by the collection view). That's why this time I wanted to try a different approach.

@Sherlouk
Copy link
Contributor

The problem with Apple's code is that it's prone to crash, a lot :)

😆

@zhubofei
Copy link

@andreamazz I SEE 😂. Also worth noting, there is a much better grid layout to be released soon. Check out #450.

@rnystrom
Copy link
Contributor

@andreamazz awesome question! @Sherlouk and @zhubofei summed everything up perfectly.

I'll need to cycle through all the PHFetchResults and store each PHAsset in its own diffable object, correct?

I'd recommend just extending PHAsset to conform to IGListDiffable:

extension PHAsset: IGListDiffable {
  func diffIdentifier -> NSObjectProtocol {
    return localIdentifier
  }

  func isEqual(toDiffableObject object: IGListDiffable?) -> Bool {
    return isEqual(object: object) // PHObject implements this for you
  }
}

My fear is that this approach might be highly inefficient.

Don't worry about this part. As long as your equality method is performant, the framework will be fast. In Instagram we diff hundreds/thousands of items all the time, on the main queue, and don't skip miss a frame 😄

@MichaelLuoSyd
Copy link

Would be much appreciated if we can use Hashable instead of IGListDiffable in swift, as we have to include IGListKit into our core framework in my project, which is really unnecessary.

@rnystrom
Copy link
Contributor

Hey @MichaelLuoSyd! We’re brainstorming some ways to make IGListKit and Swift better. A new issue to discuss this as an option would be great!

You should be able to add all conformance via extensions, hopefully solving any import issues. Obviously IGListKit has to be imported somewhere in order to use it, though.

Sent with GitHawk

@MichaelLuoSyd
Copy link

@rnystrom Thanks very much for your quick response. The data models in the core frameworks are all RealmSwiftObjects. In my project, I tried to add extensions to the models as below:

import Foundation
import CoreFramework
import IGListKit

extension CompanyProfile: ListDiffable {
public func diffIdentifier() -> NSObjectProtocol {
return companyID as NSObjectProtocol
}
public func isEqual(toDiffableObject object: ListDiffable?) -> Bool {
guard self !== object else { return true }
guard let object = object as? CompanyProfile else { return false }
return companyID == object.companyID
}
}

Swift compiler failed with error message "Cannot define category for undefined class 'CompanyProfile'", but the 'CompanyProfile' class does exist in the core framework

Any idea what went wrong? Thanks

@rnystrom
Copy link
Contributor

That’s really bizarre, sounds like some sort of project setup issue? I’d imagine that extending a class in another framework should work just fine 🤔

Sent with GitHawk

@MichaelLuoSyd
Copy link

that's what i thought. The temporary solution is to import IGListKit to the core framework to make the compiler happy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants