Add ASMapNode

[aaronschubert0]

I've started implementing an ASMapNode as discussed here #59 this node uses the MKMapSnapshotter API to provide a snapshot of a map. It then supports turning that snapshot into a MKMapView when the user taps on it which fully supports the MKMapView delegate, as well as annotations.

I'm still in two minds whether the snapshot should use an ASImageNode or whether it'd be better to just draw the image directly to the node.

The main issue I'm still facing is a stutter on scrolling when objects are being dealloc'ed in clearFetchedData I'm certain this can be improved, and would love community help on this.

cc @appleguy

[appleguy]

@aaronschubert0 fantastic, thank you! @garrettmoon, what is Bin's github handle?

[garrettmoon]

@binl

[appleguy]

I don't actually know much about this, but can we make this a Weak framework? @Adlai-Holler I think you'd set up the Photos one for us. Curious if we can even do that with AssetsLibrary itself.

[aaronschubert0]

@appleguy happy to change this to weak if that's an option, will wait to hear back from @Adlai-Holler

[appleguy]

Looks like we might not hear back in time for 1.9.2, which I hope to ship at the end of tomorrow. We don't have to include this in that release, although I'd like to, as we'll start using it in Pinterest :).

If you have time to research the Cocoapods feature itself, please do. I bet this is possible, but I'm wondering how this works for a given user. Maybe you could try adding it to weak frameworks locally, and add a map node to one of the example projects that doesn't link MapKit, and see what happens. Presumably you'll get a linker error, and I think that would be totally fine to expect users to have to manually have linked MapKit if they want to use this feature.

[aaronschubert0]

Hey @appleguy will investigate myself, hopefully we can change it to weak if it's not needed. I have time today from 16:30 GMT so will get back to you in time for 1.9.2, then we can make the getter/setter changes in a separate diff (probably tomorrow)

[appleguy]

I'm ready to merge this if we can make the framework weak, because API tweaks / refinements can easily be done in followup diffs (whereas I don't want anyone to ship an app with a new framework linkage as large as MapKit if they don't use this feature). Thanks a lot @aaronschubert0 for grabbing this task and contributing a new component to the community!

[aaronschubert0]

Hey @appleguy change the pod spec to weak and seems to work on my local, would be good to check once it's merged. Still worried about the scroll stutter when clearFetchedData is called, but I guess we can fix this in future diffs?

[aaronschubert0]

@appleguy Anything stopping this from being merged?

[appleguy]

By no means a blocker, but let's add a return before and after the @interface line to match the style of most other ASDK headers.

[appleguy]

Ah, I see _maxSize is set here...not sure what _nodeSize is for but I think that one was used in another method. If we can eliminate some or all of these instance variables and just use self.bounds.size, that might be clearer / more reliable.

[aaronschubert0]

Yeah totally agree, quite messy this way. I was using them mainly to accurately detect orientation changes. Any ideas on that? Although if we abstract to reloading the image on size changes this will be superseded.

[appleguy]

@aaronschubert0 hey there - sorry it took me a long time to get to the review. There have been a lot of PRs waiting in the queue as I've simply fallen behind on stuff. I've mostly caught up, and hope to release 1.9.2 with several improvements including my ASInterfaceState feature soon!

Though a lot of my feedback is just small stylistic nits, there are a handful of API design things to work through. I understand fully if this ended up being more than you bargained for, and so if you don't have a chance to revisit it, rest assured that the code is still very valuable to the project and it will be tweaked and landed soon!

Thanks for your efforts, and let me know if you have any questions.

[aaronschubert0]

Hey @appleguy this feedback is awesome, will go through it and respond to each comment, definitely want to get this right and have it clear for all developers. Pretty busy at work this week but will tend to this very soon.

[aaronschubert0]

Hey @appleguy implemented most of the feedback that you gave. The changes from the previous iteration are:

• ASMapNode now inherits from ASImageNode, rather than ASControlNode. The mapImage property has been removed because of this change.
• The mapView property is now a MKMapView rather than a ASDisplayNode
• The mapSize property has been removed. If developers wish to set an explicit size for ASMapNode, then they can set the preferredFrameSize property.
• "automaticallyReloadsMapImageOnOrientationChange" has been renamed to "needsMapReloadOnBoundsChange"
• "-addAnnotations:" has been renamed "setAnnotations:"
• All scalar size properties (_maxSize & _nodeSize) have been removed (yay!) in favour of self.bounds.size or self.calculatedSize
• "-calculateSizeThatFits:" has been removed as suggested by @nguyenhuy
• ASMapNode now automatically fetches a new map snapshot if it's size is changed, as opposed to only on orientation change.
• A new internal "CLLocationCoordinate2D" property has been introduced. This is to support an ASMapNode (that has liveMap = YES) to return to the correct liveMap view after the user has scrolled from the node and then returned to it at a later occasion.
• Syntax cleanup.

P.S. I don't think Travis CI is running against these commits?

[nguyenhuy]

There are some conflicts against master. I bet Travis will run once they are resolved. Awesome work, can't wait to try it out 👍

Also, it would be great if we have a working sample of this new node. Definitely not a blocking issue though.

[aaronschubert0]

Thanks @nguyenhuy ! That's a good idea! Will need to come up with a good example. I'll work on that once this has been finalised :)

[appleguy]

Big progress, thanks @aaronschubert0! I'll review as soon as I have stabilized a few issues that have come up with the 1.9.2 release. Definitely by Sunday.

[aaronschubert0]

@appleguy Awesome, looking forward to it!

[appleguy]

Aha, doesn't this need to be an MKMapRegion or something else that fully specifies the visible extent?

Later - not necessary for you to do personally @aaronschubert0, more like feature request - we could add amazing helper functions that aid a user who has a CLLocationCoordinate2D and perhaps a radius as an expression of zoom scale, and we could create a region for them.

I think I will merge this diff now, but only because 2.0 is coming up and I feel confident to make a breaking API change in that release. We really should change this though because right now the zoom level seems unspecified, and it would force creation of the mapView if set through there.

[aaronschubert0]

Hey @appleguy yeah I've been wanting to make this change since the zoom level is currently at 1km. This will make it unambiguous and available to control for everyone. It's going to be a MKCoordinateRegion. In terms of the helper functions, what use cases do you envision for this? I'm just asking since you only need a CLLocationCoordinate2D and a radius to create a MKCoordinateRegion so not sure how helpful it would end up being or what advantages it would have over the built in method?

[appleguy]

@aaronschubert0 thanks again - really happy to be merging this. If you have time, I do think one more round of changes is needed - but this should prepare the class to be included as a primary API in the 2.0 release!

[aaronschubert0]

@appleguy Woohoo! Thanks a lot for the thorough code review. I'll definitely give it another pass to give it that polish.