Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[ios] add content inset to camera edge padding #14081

Closed
wants to merge 1 commit into from

Conversation

frederoni
Copy link
Contributor

Fixes #12818

For consistency with other camera methods, -[MGLMapView setCamera:withDuration:animationTimingFunction:edgePadding:completionHandler:] now adds the contentInset to the edge padding when updating the camera.

cc @fabian-guerra @julianrex @1ec5

@frederoni frederoni requested a review from 1ec5 as a code owner March 11, 2019 13:47
@frederoni frederoni requested a review from a team March 11, 2019 13:47
@frederoni frederoni added bug iOS Mapbox Maps SDK for iOS navigation For the Mapbox Navigation SDK for Android or iOS or navigation use cases in general ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Mar 11, 2019
Copy link
Contributor

@julianrex julianrex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Can you add some change log blurbs please?

Copy link
Contributor

@fabian-guerra fabian-guerra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for fixing this. Could you please add this fix to macOS as well.

@friedbunny friedbunny added this to the release-l milestone Mar 11, 2019
Copy link
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned above, the macOS implementation of MGLMapView also has the same bug.

mbgl::CameraOptions cameraOptions = [self cameraOptionsObjectForAnimatingToCamera:camera edgePadding:edgePadding];

mbgl::EdgeInsets padding = MGLEdgeInsetsFromNSEdgeInsets(edgePadding);
padding += MGLEdgeInsetsFromNSEdgeInsets(self.contentInset);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some callers are already adding the same content inset to the padding:

padding += MGLEdgeInsetsFromNSEdgeInsets(self.contentInset);

To avoid the redundant change, those callers also need to be updated.

@frederoni
Copy link
Contributor Author

I haven't been able to figure out how to write a test for this case yet. Edge padding is thrown away after transform_state is finished. Perhaps a snapshot test could work. Any ideas are very much welcome.

@friedbunny friedbunny removed this from the release-liquid milestone Apr 29, 2019
@julianrex
Copy link
Contributor

Is this still valid?

@friedbunny
Copy link
Contributor

Indeed, there’s been a fair bit of work done on this recently (#14664, #14795, #14813, etc.) — is this still valid, @fabian-guerra @astojilj?

@friedbunny friedbunny requested a review from fabian-guerra June 11, 2019 23:37
@fabian-guerra
Copy link
Contributor

This was fixed in #14813

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold iOS Mapbox Maps SDK for iOS navigation For the Mapbox Navigation SDK for Android or iOS or navigation use cases in general
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Content insets should be added when setting camera with custom edge padding
5 participants