Advanced Photos Integration

[Adlai-Holler]

This PR adds a new class, ASPhotosImageRequest, which formalizes the PHAsset <-> NSURL transformation which was previously done ad-hoc in ASMultiplexImageNode and adds options to configure the request.

The change is backwards-compatible with URLs generated the old way (simply ph://assetID) except that instead of CGSize(2048, 2048), requests that don't specify a target size will get PHImageManagerMaximumSize instead. Apparently this used to result in video image requests returning empty responses.

My goal is to get ASMultiplexImageNode working with the opportunistic delivery method of PHImageManager, and ideally even to get PHCachingImageManager involved with ASRangeController but I'm honestly intimidated about how to pull those off and could use a teammate.

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

[Adlai-Holler]

Sent an email to the CLA folks to update my username

[Adlai-Holler]

Obviously this introduces the requirement of the Photos framework which is bad. I wanted to know whether y'all are even interested in this before taking the time to fix that. @appleguy Is it too dirty to put this in a subspec? Is there a cleaner way?

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[appleguy]

Let's improve this comment, now that a client would have to use the new class manually. Probably break it up into two lines, and expand into a sentence like "Photos.framework URLs (ph://) may be provided to an ASPhotosImageRequest, which can generate a URL to return here" ?

Have you considered making ASMultiplexImageNode create the ASPhotosImageRequest internally by parsing the ph:// URLs? Particularly because the ASPhotosImageRequest object can't be passed to Multiplex directly, this API feels odd to me (I may be misunderstanding it).

[Adlai-Holler]

That's my bad I probably could have explained it more clearly. Photos.framework URLs aren't really "a thing," it's something made-up so the only way to form/parse them is through ASPhotosImageRequest. So I expect the typical use pattern will be 1. create and configure an ASPhotosImageRequest using an asset ID & target size 2. return its URL 3. ASMultiplexImageNode will attempt to parse the URL into an ASPhotosImageRequest like you mentioned

[appleguy]

Cool new area, @Adlai-Holler! There are a number of image handling conveniences I would love to see added to ASDK, and improved Photos.framework integration is definitely a good area.

This is very close to mergable. In fact, if we could make ASPhotosImageRequest something that most ASDK users would not need to know about (by parsing URLs directly handed to Multiplex), then we shouldn't even worry about the name at this point.

For a more visible API, the ASPhotos prefix sounds possibly ambiguous to me. I jumped into reading the patch before finishing your intro, and didn't immediately notice that the class was Photos.framework specific, as opposed to a general image downloader. What do you think about calling it ASPhotosAssetRequest, or ASPhotosFrameworkImageRequest? If you feel the current name is better, keep it — just some ideas.

For the other classes you mentioned, let's definitely talk! There are some other folks at Pinterest who would like to discuss image loading and caching, as we are working in similar areas and may be able to share a solution. You may have seen PINRemoteImage and PINCache on GitHub. We are hoping to integrate at least the former with ASDK, eventually with Progressive Scan JPEG support. @garrettmoon @rcancro

[Adlai-Holler]

Sweet! I agree with you about the name – the old name never really sat right with me but I've been coding with Swift so long, I forgot that long names are part of Objective-C's charm. Name changed.

Thanks for pointing me to the Pinterest repos/folks! Are you guys interested in local assets?

Also, I don't want to get too far off-topic but would you be interested in making ASRangeController a little more public-facing? It's crazy powerful and surely other users want to warm caches/reprioritize operations with those ranges.

[appleguy]

@Adlai-Holler - I am intensely interested in making ASRangeController, and possibly ASDataController, more public-facing. The Paper app used "working ranges" in at least 5 or 6 major areas — the section scroller, the story scroller, vertically within a story for attachments and text, in the back-stack...it is probably the most successful "new" UI programming pattern I have seen on the platform in the past couple years.

There are several things standing in the way, though. 1st, a Visible Range is essential for most third party usages. I want to provide "visible" notifications for ASCellNodes in Table / Collection too.

Second, ASDataController is pretty rough, particularly with the batch handling stuff you ran into recently. I had to change focus before I was able to start testing batches, so I'm embarrassed about its problems there, but they are among the most important in the project right now & fortunately have increasing interest from a couple folks.

Third, I want to make sure we are careful in considering what makes sense for a supportive, powerful third-party API vs something catered to work with UIKit classes. It may well be the case that there should be a superclass that is public, with UITableView and UICollectionView-specific subclasses that aren't intended to be used directly by third parties.

Thanks for the update on this diff, will check it out!

[appleguy]

I realize that this pattern (.asyncdisplaykit_) is used in a few places, but I think those usages are usually quite necessary (like extensions to CALayer) without a clean way to avoid it.

In this case, is it not correct to say the flow is:

1. User creates a PhotosRequest object
2. User manually requests a URL from that object, and provides it to ASMultiplexImageNode
3. User discards reference to PhotosRequest
4. Internals of ASMultiplexImageNode revive the reference by grabbing associated object through magically-extended NSURL object

This is the only part of the diff that doesn't feel right to me. Even parsing ph:// seems a bit better :). Are there any real-world cases where Photos framework images would be mixed with regular remote URLs? If not, what do you think about providing a load method that takes the ASPhotosFrameworkImageRequest directly?

[Adlai-Holler]

That's not quite the flow — the category on NSURL is just a convenience wrapper for ASPhotosFrameworkRequest.requestWithURL which does the ph:// parsing like you mentioned. I'd be fine with ditching it since it only saves you like one line tops. But there's no magic here! Except the sublime magic of creation.

And yes! There is at least one use case for remote images intermingled with local images – I'm working on an app that lets you create and configure photo-based content offline, and then it is synced in the background. So we'll have a flow of photos where some are local and some are remote and that might even change while the image is onscreen.

Sent from my iPhone

On Sep 27, 2015, at 12:01 PM, appleguy notifications@github.com wrote:

In AsyncDisplayKit/ASMultiplexImageNode.mm:

@@ -456,8 +455,8 @@ - (void)_loadNextImage
}];
}
// Likewise, if it's a iOS 8 Photo asset, we need to fetch it accordingly.

• else if (AS_AT_LEAST_IOS8 && [[nextImageURL scheme] isEqualToString:kPHAssetURLScheme]) {
• [self _loadPHAssetWithIdentifier:nextImageIdentifier URL:nextImageURL completion:^(UIImage *image, NSError *error) {
• else if (ASPhotosFrameworkImageRequest *request = nextImageURL.asyncdisplaykit_photosRequest) {
  I realize that this pattern (.asyncdisplaykit_) is used in a few places, but I think those usages are usually quite necessary (like extensions to CALayer) without a clean way to avoid it.

In this case, is it not correct to say the flow is:

1. User creates a PhotosRequest object
2. User manually requests a URL from that object, and provides it to ASMultiplexImageNode
3. User discards reference to PhotosRequest
4. Internals of ASMultiplexImageNode revive the reference by grabbing associated object through magically-extended NSURL object

This is the only part of the diff that doesn't feel right to me. Even parsing ph:// seems a bit better :). Are there any real-world cases where Photos framework images would be mixed with regular remote URLs? If not, what do you think about providing a load method that takes the ASPhotosFrameworkImageRequest directly?

—
Reply to this email directly or view it on GitHub.

[appleguy]

@Adlai-Holler great idea, I didn't think of that! Just make the category nice & visible (probably put it in ASMultiplexImageNode.h, and really explicitly call it out in the comment for the data source method).

I would also just leave out the as_ prefix. We have used asyncdisplaykit_ for areas where collision is plausible, but that's long for an already-long method name, and I would say there is not a reasonable probability that this full method signature could collide.

Ready to merge as soon as you integrate these changes — thanks for taking the time to discuss, iterate, and get this one really right.

[Adlai-Holler]

@appleguy Cool! Glad we dialed it in. I left the ASPhotosFrameworkImageRequest class public in the Details group, but removed reference to it in ASMultiplexImageNode. It's a pretty harmless class and I think users who find it by digging into the Details group will understand that they can use it if they want but we're not supporting it for undocumented use. If that seems too ad-hoc for you I'd be cool walling it off.

I think the code's ready for final review. The last remaining question is the annoying one… Photos framework. I think our two choices are

• Strongly link the Photos framework. Requires us to raise the min deployment target to iOS 8 and increases the memory footprint.
• Make a subspec that has the Photos integration. Adds complexity to the code and the podspec.

If you're thinking the next release will be 2.0 then the ideal move may be remove support for the Assets Library to offset the memory footprint and replace it with this. Thoughts?

[Adlai-Holler]

Oops looks like I broke a test. Investigating…

[appleguy]

Love the ad-hoc "Details" strategy. I like adding little nuggets of awesome for people to find if they wander deeper into the framework :-P

Is there a possibility of dynamically linking Photos.framework using dlopen() or something? It's unfortunately not an option to increase the minimum deployment target to 8.0, as several big clients (including Pinterest, Facebook, etc) support back to 7.0.

[appleguy]

@Adlai-Holler The next release will indeed be 2.0, which I'm thinking about pushing out this weekend — there are too many people using 1.2.3, and I don't anticipate breaking API changes with the new 2.0 stuff (with the possible exception of the naming of properties in ASStackLayoutable, being evaluated now).

However, 2.0 wasn't planned to drop support for 7.0, so I'm not sure about the implication on dropping AssetsLibrary. To be honest, I hadn't realized the OSS framework was linking AssetsLibrary in the first place...

[Adlai-Holler]

Understood. We may be good to go – I created a sample iOS project with deployment target 7.0 and brought this pod in without issues. If I try to use our NSURL constructor, the compiler errors with PHImageRequestOptions is unavailable in iOS 7.

To cover the case that someone targeting iOS 7 gives ASMultiplexImageNode a ph:// URL (maybe their own protocol) I've configured ASPhotosFrameworkImageRequest.requestWithURL to return nil if iOS < 8 rather than crashing when it tries to instantiate PHImageRequestOptions.

If anyone has an actual iOS 7 device to test this on that'd be great, but otherwise I think it's covered.

[appleguy]

Thanks @Adlai-Holler , I really appreciate your consideration of that case and provided safer runtime behavior! It can be difficult to catch iOS 7-specific bugs.