Mapbox-gl-native: Content insets should be added when setting camera with custom edge padding

[1ec5]

The -[MGLMapView setCamera:withDuration:animationTimingFunction:edgePadding:completionHandler:] method added in #9651 uses the edgePadding parameter verbatim. By contrast, -setVisibleCoordinates:count:edgePadding:direction:duration:animationTimingFunction:completionHandler:, -cameraThatFitsCoordinateBounds:edgePadding:, and other camera-related methods all add the MGLMapView.contentInset property to the passed-in edge padding:

https://github.com/mapbox/mapbox-gl-native/blob/372611de0fc3f82134671d8177c83e4d96f2186e/platform/ios/src/MGLMapView.mm#L3175

This inconsistency forces callers of -setCamera:withDuration:animationTimingFunction:edgePadding:completionHandler: to manually add the content insets.

/cc @frederoni @JThramer

[stale[bot]]

This issue has been automatically detected as stale because it has not had recent activity and will be archived. Thank you for your contributions.

[1ec5]

This is definitely still a bug. Nice try, stalebot.

[astojilj]

There is discrepancy in code and documentation between the methods, when it comes to interpretation of edgePadding/insets. Documentation "could" be interpreted as inline with the implementation - setVisiblecoordinates and "fitting" methods documentation uses wording "additional padding" while setcamera doesn't:

1. 
   
   • (void)setCamera... edgePadding:(UIEdgeInsets)edgePadding
   
   

@param edgePadding The minimum padding (in screen points) that would be visible around the returned camera object if it were set as the receiver’s camera.

1. 
   
   • (void)setVisibleCoordinates ... edgePadding:(UIEdgeInsets)insets animated:(BOOL)animated;
   
   

Changes the receiver’s viewport to fit all of the given coordinates and optionally some additional padding on each side.

However, documentation doesn't specify additional to contentInset* value. Additional tocontentInset`value, when automatically adjusted by deprecated automaticallyAdjustsScrollViewInsets (btw, we should port to usage of contentInsetAdjustmentBehavior since deprecating automaticallyAdjustsScrollViewInsets), is also overwritten by automatic adjustment.

I would suggest changing the implementation so that:
a) when automaticallyAdjustsScrollViewInsets is YES (default), all methods, including setContentInsets and setCamera, define additional content inset to the one read from viewController layout guides (deprecation topLayoutGuide in favour of topLayoutGuide.bottomAnchor there).
b) when automaticallyAdjustsScrollViewInsets is NO, all methods, so fitting, setCamera and setContentInsets, there is no automatic content inset (or it is (0, 0, 0, 0)) and all APIs define the same content inset value. Additional word can stay in documentation as it is adding to zero content inset.

[1ec5]

Documentation "could" be interpreted as inline with the implementation - setVisiblecoordinates and "fitting" methods documentation uses wording "additional padding" while setcamera doesn't

To me, the -setCamera:… documentation remains mum about the edgePadding behavior when there is a content inset; it doesn’t say one way or another whether the content inset is added to the edge padding.

I would suggest changing the implementation so that:
a) when automaticallyAdjustsScrollViewInsets is YES (default), all methods, including setContentInsets and setCamera, define additional content inset to the one read from viewController layout guides (deprecation topLayoutGuide in favour of topLayoutGuide.bottomAnchor there).
b) when automaticallyAdjustsScrollViewInsets is NO, all methods, so fitting, setCamera and setContentInsets, there is no automatic content inset (or it is (0, 0, 0, 0)) and all APIs define the same content inset value. Additional word can stay in documentation as it is adding to zero content inset.

Agreed. -[MGLMapView setCamera:withDuration:animationTimingFunction:edgePadding:completionHandler:] should add contentInsets to edgePadding, and -[MGLMapView setCamera:withDuration:animationTimingFunction:completionHandler:] should pass in an edge padding of NSEdgeInsetsZero/UIEdgeInsetsZero, not contentInsets.

The current behavior is clearly a bug, but it has been present for a while, and some clients such as the iOS navigation SDK have been working around it. So can we make the change in place, or do we have to create new methods, deprecating the old ones, to get around semver requirements?

[fabian-guerra]

The current behavior is clearly a bug, but it has been present for a while, and some clients such as the iOS navigation SDK have been working around it. So can we make the change in place, or do we have to create new methods, deprecating the old ones, to get around semver requirements?

Technically this is a bug. What happens if we treat it as such? and specify the proper functionality in the docs? We shouldn't have to wait for a semver to fix this problem nor complicate the api further. From a bug perspective (and even tho it has been this way for a while, and devs work around this issue) I'm in favor to release it as part of a "minor" version.

cc @mapbox/maps-ios

[1ec5]

If we fix this bug, we need to fix it in time for v5.1.0 (release-oolong). That release will also include a significant change to vanishing point behavior (#14664), essentially a fix for an even older bug. The navigation SDK is currently working around both bugs. (mapbox/mapbox-navigation-ios#2134 tracks removing the vanishing point workaround.)

The navigation SDK is pinning to v5._x_ of the map SDK. If the map SDK team commits to fixing this bug in v5.1.0, then I’ll modify today’s release of the navigation SDK to pin to v5.0._x_, and we can relax the dependency the next time around once v5.1.0 is out. Otherwise, developers on a current version of the navigation SDK will wind up with a broken application when they routinely run pod install or carthage bootstrap. This is the pitfall that semver is supposed to protect clients of the map SDK from, but if we’re going to bend the rules, then we’ll need to at least protect the navigation SDK.

[astojilj]

Is this still an issue? If so, let's reopen to get it fixed.

From the #15232 and #15233 it is not clear to me if this is still an issue.

[astojilj]

Is this still an issue? If so, let's reopen to get it fixed.

From the #15232 and #15233 it is not clear to me if this is still an issue.

Please ignore - it seems fine https://github.com/mapbox/mapbox-gl-native/issues/15232#issuecomment-521195215