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

Force camera change when specifying edge padding #14790

Closed
wants to merge 3 commits into from

Conversation

1ec5
Copy link
Contributor

@1ec5 1ec5 commented May 29, 2019

As in #14664 on iOS, the macOS implementation of MGLMapView should only force a camera change when the content insets are modified by edge padding. Fortunately, the macOS implementation always adds edge padding to the content insets instead of replacing them, so we can check the insets in a consistent way.

Also copyedited changelogs.

/cc @astojilj @fabian-guerra

@1ec5 1ec5 added bug iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS documentation labels May 29, 2019
@1ec5 1ec5 added this to the release-oolong milestone May 29, 2019
@1ec5 1ec5 requested review from astojilj, fabian-guerra and a team May 29, 2019 02:05
@1ec5 1ec5 self-assigned this May 29, 2019
Copy link
Contributor

@astojilj astojilj left a comment

Choose a reason for hiding this comment

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

Thanks.
LGTM for CHANGELOG commits.

@@ -1311,7 +1311,7 @@ - (void)setVisibleCoordinateBounds:(MGLCoordinateBounds)bounds edgePadding:(NSEd
}

MGLMapCamera *camera = [self cameraForCameraOptions:cameraOptions];
if ([self.camera isEqualToMapCamera:camera]) {
if ([self.camera isEqualToMapCamera:camera] && NSEdgeInsetsEqual(insets, NSEdgeInsetsZero)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

There should be no need for this change: cameraForLatLngBounds is still baking in specified paddingto cameraOptions adjusted center (that is ##14664 didn't fix cameraForLatLngBounds as we don't have padding in public MGLMapCamera API - padding is in view) and this should keep current behaviour since it is unchanged.

@@ -1198,7 +1198,7 @@ - (void)setCamera:(MGLMapCamera *)camera withDuration:(NSTimeInterval)duration a
};
}

if ([self.camera isEqualToMapCamera:camera]) {
if ([self.camera isEqualToMapCamera:camera] && NSEdgeInsetsEqual(edgePadding, NSEdgeInsetsZero)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

edgePadding specified here is self.contentInsets when specified in L.1181.
The documentation doesn't state that this is additional padding, like in #12818 (comment), additional to self.contentInsets.
I believe that we need to check NSEdgeInsetsEqual(edgePadding, self.contentInsets)) here instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oof, thanks, somehow I missed that the macOS implementation is also affected by #12818.

@julianrex julianrex removed the request for review from fabian-guerra June 3, 2019 21:06
@friedbunny
Copy link
Contributor

friedbunny commented Jun 3, 2019

As in #14827, are there tests that can be written to ensure that this functionality does not regress again?

@fabian-guerra
Copy link
Contributor

This was addressed in #14813 and #14795

@1ec5 1ec5 deleted the 1ec5-center-offset-14664 branch June 19, 2019 23:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug documentation iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants