Improve ASMapNode to get ready for 2.0

[aaronschubert0]

Hey @appleguy, here are changes based on your latest feedback
Notable changes:

• Changed init method to -initWithRegion: instead of initWithCoordinate:
• You can now call setLiveMap before ASMapNode has been loaded to start off with a live map view.
• Smaller syntax changes and clean up.

Just had a thought though, would it be useful for people to be able to change the region after the initialisation of ASMapNode, should we provide a method for this?

[appleguy]

@aaronschubert0 super awesome, thank you! I was just thinking about this earlier in the morning.

It would indeed be ideal to proved a property for region, and allow it to be set. More likely than anything else it may be used to read the value before the node is loaded, but it seems useful to be able to set it too.

I might even suggest that we kill initWithRegion entirely (I know, I know ... I had suggested it), and just rely on the property. This way if a user's app doesn't know the region at init time (seems plausible), they could set it in -fetchData or wherever else is appropriate.

[aaronschubert0]

@appleguy Hmm this is a tricky one. I've been thinking about this for a few days now. I'm not sure how comfortable I would be in having no custom init method that clearly specifies an action. If we did get rid of initWithRegion then we would have to provide a suitable default so that ASMapNode works out of the box. Although [[ASMapNode alloc] init] doesn't produce anything great either at the moment. I'm still 50/50. The layout spec (as well as ASImage/Text/ControlNode etc) conforms to a standard -init and then setting properties so maybe we should actually go this way to keep in line with the framework. Should we provide a default in this case?

[appleguy]

@aaronschubert0 I think it's similar to UIView initWithFrame:, which actually is a bad practice / pattern to use, and so that's why ASDisplayNode does not have initWithFrame: even though it is basically a superset of the UIView API.

Let's go ahead and make it a plain -init with the region property. Defaulting the region to "whole world" would make sense (not US, please!) - simply so that people creating a map node for the first time can see something show up if they don't have a region to set yet.

[appleguy]

Make sure all indentation throughout the file is 2 spaces, to match ASDK code style. You can set this in your editor and then use the "re-indent" function if you select all in the file. Keep the else on the same line - } else {

[aaronschubert0]

@appleguy I've made the specified changes. The major breaking change is getting rid of the custom init method in favour of altering the region property. Internally, I got rid of _mapNode to work directly with the MKMapView, change isLiveMap to be a 'computed' property and generally improved the code quality. The only snag left is the possibility of a retain cycle in the -addLiveMap method, I've commented on a previous comment asking how this may be solved, happy to change anything regarding this in a follow up diff.

[appleguy]

Add to the comment - "this property may be set from a background thread before the node is loaded, and will automatically be applied to define the region of the static snapshot (if liveMap = NO) or the internal MKMapView (otherwise)."

[appleguy]

@aaronschubert0 this is really getting there! Did we add a test case for this, at least an example app? I wonder if we could add some unit tests to make sure coverage stays high and we don't break it?

I'm going to land this one because it looks like a definitive improvement / step forward. I do think we should bring back the BOOL and look for any other ways to make sure the internal logic / state is consistently handled for all the possible ordering of calls and use cases that nodes can go through. One followup there is correct handling of layerBacked mode — at a minimum we'd need to add the mapView's sublayer as a sublayer of the node's layer when the node is layerBacked, but it's very possible that won't work correctly. Yet we should support layerBacking for the default snapshotter mode. So I'd suggest overriding setLayerBacked: and setLiveMap: for each to check for the other property being set and throw an assertion if at any point they are both enabled.

[aaronschubert0]

@appleguy Yeah!! So much better than when we started. Thanks for all the amazing feedback! I haven't added test cases yet but will add those along with an example app soon. Good point on the layer backing, can't forget about that! I'll add this in a future diff as well.

In terms of the BOOL, yeah I agree, it's tricky trying to minimise the internal state but will keep working on that and bringing back the BOOL does make it clearer.

Due to work I'll probably be picking this up a the start of the new year, once we've had some rest.