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

[ios] Fix incorrect center coordinate after pinch gesture #15097

Merged
merged 2 commits into from
Jul 14, 2019

Conversation

astojilj
Copy link
Contributor

@astojilj astojilj commented Jul 10, 2019

To changelogs:
Fixed incorrect center coordinate after pinch regression caused by edge insets fix (#14664).

While working on #14664, missed to understand the logic used in

                CLLocationCoordinate2D centerCoordinate = _previousPinchCenterCoordinate;
                mbgl::EdgeInsets padding { centerPoint.y, centerPoint.x, self.size.height - centerPoint.y, self.size.width - centerPoint.x };
                self.mbglMap.jumpTo(mbgl::CameraOptions()
                                        .withCenter(MGLLatLngFromLocationCoordinate2D(centerCoordinate))
                                        .withPadding(padding));

Replacing this code by moveBy achieves the required translation.

Fixes #14977 and fixes #15082.

@astojilj astojilj requested review from a team and 1ec5 July 10, 2019 22:02
@astojilj astojilj self-assigned this Jul 10, 2019
@astojilj astojilj added high priority needs changelog Indicates PR needs a changelog entry prior to merging. labels Jul 10, 2019
@friedbunny friedbunny changed the title [ios][macos] Fix center coordinate incorrect after pinch gesture [ios] Fix incorrect center coordinate after pinch gesture Jul 10, 2019
@friedbunny friedbunny added iOS Mapbox Maps SDK for iOS UX labels Jul 10, 2019
@friedbunny friedbunny added this to the release-picklejuice milestone Jul 10, 2019
@friedbunny friedbunny requested review from julianrex and removed request for fabian-guerra July 10, 2019 22:23
@friedbunny
Copy link
Contributor

friedbunny commented Jul 10, 2019

Thanks for jumping on this, @astojilj — this does indeed fix the rotation issue I was seeing, at the very least. (I haven’t looked wider yet.)

To confirm that this fixes the full range of the reported problems (annotation selection, wrong center coordinate, etc.) are there tests that we can add?

@chloekraw chloekraw added the needs backport Indicates PR needs to be cherrypicked into a previous release branch. label Jul 10, 2019
@julianrex julianrex requested review from friedbunny and removed request for 1ec5, frederoni and julianrex July 12, 2019 14:08
@julianrex julianrex removed the needs changelog Indicates PR needs a changelog entry prior to merging. label Jul 12, 2019
@julianrex
Copy link
Contributor

@friedbunny I have added a changelog for this PR and have tested manually. I added some rotation/selection tests in https://github.com/mapbox/mapbox-gl-native/tree/jrex/rotation-tests, but they do not repro this particular issue. I will try to add another for this.

Can you please review and merge? (or I'll merge given that @astojilj is on vacation).

@astojilj does this same fix need to be added to the macOS's MGLMapView?

@friedbunny
Copy link
Contributor

Regarding tests: MGLMapViewDirectionTests is the basic sort of thing I had in mind, apologies for not linking to that sooner. Annotation selection and such is likely a bit trickier, though...

@friedbunny friedbunny self-assigned this Jul 12, 2019
astojilj and others added 2 commits July 12, 2019 12:47
To changelog:
Fixed incorrect center coordinate after pinch regression caused by edge insets fix (#14664).

While working on #14664, missed to understand the logic used in

```
                CLLocationCoordinate2D centerCoordinate = _previousPinchCenterCoordinate;
                mbgl::EdgeInsets padding { centerPoint.y, centerPoint.x, self.size.height - centerPoint.y, self.size.width - centerPoint.x };
                self.mbglMap.jumpTo(mbgl::CameraOptions()
                                        .withCenter(MGLLatLngFromLocationCoordinate2D(centerCoordinate))
                                        .withPadding(padding));

```

Replacing this code by moveBy achieves the required translation.

Fixes: #14977, #15082
Copy link
Contributor

@friedbunny friedbunny left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this, @astojilj — sorry we didn’t catch it in the original review. 🙇

I’m adding a regression test for this over in #15117.

@astojilj
Copy link
Contributor Author

@astojilj does this same fix need to be added to the macOS's MGLMapView?

No. Thanks - it is only for iOS.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
high priority iOS Mapbox Maps SDK for iOS needs backport Indicates PR needs to be cherrypicked into a previous release branch. UX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Annotations are not selected when map is rotated .centerCoordinate incorrect after zoom gesture
4 participants