Adds a delegate to notify when a gesture is canceled.

[fabian-guerra]

Fixes mapbox/mapbox-gl-native#14686. Added a delegate to gracefully manipulate the camera once a gesture is canceled programatically.

[1ec5]

We can add this delegate method if necessary, but did we try out the approach suggested in mapbox/mapbox-gl-native#14686 (comment)? It seems pretty elegant:

Probably the best place to edit it would be method shouldChangeFromCamera:toCamera:reason:.
You could be passing newCamera as a reference - so it'd be editable for those who want it, or not, for those who don't want it. Or make another delegate method to eventually edit it - if you want to keep in sync with the old version.

We can’t pass in newCamera by reference without changing the signature in a backwards-compatible change. However, the camera is mutable, so after calling -_shouldChangeFromCamera:toCamera:, we could check if any of newCamera’s properties have changed; if they have, we could bail out of the usual gesture handling and set that camera instead (animated or unanimated depending on the call site).

[julianrex]

However, the camera is mutable, so after calling -_shouldChangeFromCamera:toCamera:, we could check if any of newCamera’s properties have changed

@fabian-guerra any reason why we couldn't try this first?

[fabian-guerra]

@1ec5 @julianrex I didn't consider following their suggestion because:

1. The delegate method already returns a value and changing this to also modify the camera breaks SRP, and I don't think will actually solve the problem.
2. By letting implement a delegate when the map cancels a gesture gives them more control on what to do beyond just moving the camera to a specific point. For example they could add a warning explaining why customers can move to beyond boundaries and provide their own transition timing to a specific place.

I would encourage you to try the example to see how it works.

[1ec5]

What gives me pause about the proposed addition to the API is that it tells the delegate that the map view “did” cancel the gesture when in fact that hasn’t happened and wouldn’t even happen with an empty implementation of the method. At most, this method is one of a few opportunities for the delegate to cancel the gesture. The naming of this method is critical, because it affects when developers expect it to be called in the order of camera-related delegate methods.

The delegate method already returns a value and changing this to also modify the camera breaks SRP, and I don't think will actually solve the problem.

mapbox/mapbox-gl-native#2457 (comment) proposed following the precedent in -[UIScrollViewDelegate scrollViewWillEndDragging:withVelocity:targetContentOffset:], in which the targetContentOffset parameter is an inout parameter that the implementation can fill in with the desired offset. I liked the idea but considered it to be tail work for mapbox/mapbox-gl-native#5584. That idea came up again in mapbox/mapbox-gl-native#14686 (comment).

Since we’re dealing with an MGLMapCamera object, we don’t need to make it an inout object, which means the method signature would stay the same. There is a semantic difference between a “should” method and a “will” method. But currently only the “should” method tells the application what the new camera will be.

By letting implement a delegate when the map cancels a gesture gives them more control on what to do beyond just moving the camera to a specific point. For example they could add a warning explaining why customers can move to beyond boundaries and provide their own transition timing to a specific place.

My naïve assumption was that a developer could implement -[MGLMapViewDelegate mapView:shouldChangeFromCamera:toCamera:] to display that warning and return true to move to the modified camera or false to cancel the camera change. We also added -shouldChangeFromCamera:toCamera:reason:, which makes it easier to prevent any recursion that might result from changing the camera inside the method.

The example being added to iosapp seems a bit contrived, but it does effectively demonstrate your point that the existing “should” methods aren’t well-suited for the use case in mapbox/mapbox-gl-native#14686. I wonder if it would be feasible to blend your approach with the UIScrollViewDelegate precedent: -mapViewWillEndPanning:targetCamera: would be specific to gestures but still accept a mutable camera parameter, and we could add similar methods for the other gestures. That way developers would know this is specific to the drift behavior and wouldn’t need to distinguish the other camera change reasons. The implementation would look pretty similar to this PR, except that we’d take care of calling mbgl::Map::easeTo() for the developer.

[1ec5]

Suggested change

	if ([self.delegate respondsToSelector:@selector(mapView:didCancelGestureForCamera:)]) {
	[self.delegate mapView:self didCancelGestureForCamera:toCamera];
	}
	MGLMapCamera *targetCamera = toCamera.copy;
	if ([self.delegate respondsToSelector:@selector(mapViewWillEndPanning:targetCamera:)]) {
	[self.delegate mapViewWillEndPanning:self targetCamera:targetCamera];
	if (![toCamera isEqualToCamera:targetCamera]) {
	[self setCamera:targetCamera animated:YES];
	}
	}

[1ec5]

Actually, that would have to happen regardless of -mapView:shouldChangeFromCamera:toCamera:, so we’d move it out one level so it’s only conditionalized on drift.

[julianrex]

As the PR stands, I think a developer would want to know the type of gesture that was beginning to drift. Also I'm not sure that cancel is the correct verb here.

Another option (!) might be to add a sibling to mapView:shouldChangeFromCamera:toCamera: that returns a new (or existing camera). Naming is hard, but something along the lines of cameraForMapView:changingFromCamera:toCamera:. Returning nil could be treated as the same as returning NO from the should variant.

However I like @1ec5's explicit willEnd..., which is a little more explicit and less of a mouthful. How would you feel if this was mapViewWillEndGesture:targetCamera: (that could even return a new camera).

[1ec5]

I think a developer would want to know the type of gesture that was beginning to drift.

Yes, the idea with -mapViewWillEndPanning:targetCamera: is that we’d have a different method for each distinct drift animation. I think for the purpose of this feature, we don’t need a separate method for each UIGestureRecognizer, because only a few of the gesture recognizers result in a drift animation. The original goal is to give the developer more control over the built-in drift animations, of which there are only a few.

[habibahabiba77]

I need to get my way to working with my iPhone