[ios] MGLAnnotationView annotation property made writable

[fabian-guerra]

Fixes #7321

[mention-bot]

@fabian-guerra, thanks for your PR! By analyzing this pull request, we identified @boundsj, @1ec5 and @incanus to be potential reviewers.

[1ec5]

What is the effect of changing this property? Does it accomplish the use case envisioned in #7321, given that MGLAnnotationView’s initializer doesn’t take an MGLAnnotation parameter?

[fabian-guerra]

If setAnnotation exists publicly, then they can override it in a subclass and have the annotation model change the behavior of the view.

[1ec5]

Even though MKAnnotationView.annotation is read-write, it also comes with this note in the documentation:

You should not change the value of this property directly.

which we also state above, redundantly. What is our reason for making the property read-write but advising developers not to write to it?

[fabian-guerra]

Changing the annotation in mapView:viewForAnnotation: will not affect annotationViewForAnnotation: (the caller) it's meant to be a convenient way to set up any UI update code. This is also how Mapkit behaves.
From a conversation with @boundsj

The developer setting it in the delegate first is redundant but facilitates them writing code to update the view when the annotation model property is set.

[incanus]

If setting an annotation view's annotation to nil is opened up to the public, since it's still nullable, that means that it could go away at any time. If that's the case, it means instances such as this and this could crash.

Beyond fixing just these instances, we should come up good tests around various points in the lifecycle to ensure that setting an annotation to nil is safe.

Note also that as @eimantas points out in the OP in #7321 (comment), MapKit provides an initializer argument, but also has a note on the property itself:

You should not change the value of this property directly. This property contains a non-nil value only while the annotation view is visible on the map. If the view is queued and waiting to be reused, the value is nil

We should determine if MapKit has any guards around this (for example, gating the public setter and only using a private one internally for its own management) and implement that.

[fabian-guerra]

@incanus #8139 (comment) does this makes sense?

[incanus]

@fabian-guerra Yes, but I think we either need to guard against, or at very least warn against, setting the annotation to nil since it's technically now possible. Especially if that can result in a crash.

[fabian-guerra]

@incanus I'm thinking a NSAssert clause would be better since it won't hide the failing reason nor creating unexpected behavior, thoughts?

[eimantas]

One more approach would be to create a designated initialiser that accepts the annotation as an argument (just like in MapKit). This would also increase the MapKit parity for the mapbox' darwin SDKs. With this it'd be possible to mark the current initialiser as deprecated and leave it like that for a couple of releases until people who update the SDK, would know to update the initialisers too.

[incanus]

Made a minor grammar tweak in 590c6ce @fabian-guerra, but otherwise this feels ok to me. I'm unsure how sustainable it will be in the future, but I think that just comes with the territory of this sort of change.

[boundsj]

@fabian-guerra please cherry pick this to the new release branch release-ios-v3.5.0-android-v5.0.0 thanks!

[1ec5]

When you want to indicate an error on the developer’s part, raise a named NSException instead of calling NSAssert(). See #8026 (comment) for more details. @fabian-guerra, can you change these assertions into named exceptions?