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

Fixes a bug where the animated parameter of -[MGLMapView selectAnnotation:animated:] was ignored. #13689

Merged
merged 6 commits into from
Jan 9, 2019

Conversation

julianrex
Copy link
Contributor

Fixes: #13055

Also:

  • renames related variables/parameters for clarity (e.g. moveOnscreen -> moveIntoView)
  • Adds -[MGLMapView selectAnnotation:moveIntoView:animateSelection:] to the public header, but marked as undocumented.

Julian Rex added 3 commits January 8, 2019 12:21
@julianrex julianrex added the iOS Mapbox Maps SDK for iOS label Jan 8, 2019
@julianrex julianrex added this to the release-java milestone Jan 8, 2019
@julianrex julianrex requested a review from friedbunny January 8, 2019 17:32
@julianrex julianrex requested a review from 1ec5 as a code owner January 8, 2019 17:32
@julianrex julianrex requested a review from a team January 8, 2019 17:32
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.

Thanks, this change looks good. Just one question that jumped out at me; not sure whether we need to do anything about it.

CGRect positioningRect = [self positioningRectForAnnotation:annotation defaultCalloutPoint:CGPointZero];
[self selectAnnotation:annotation moveOnscreen:animated animateSelection:YES calloutPositioningRect:positioningRect];
[self selectAnnotation:annotation moveIntoView:moveIntoView animateSelection:animateSelection calloutPositioningRect:positioningRect];
Copy link
Contributor

Choose a reason for hiding this comment

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

Should deselection in response to another annotation being selected also honor the animated flag, for consistency?

[self deselectAnnotation:self.selectedAnnotation animated:NO];

[annotationView setSelected:NO animated:animated];

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the quick review!

I had the very same question 🙂 I can imagine situations where different applications would want different results. I think leaving as is makes the most sense at present, and we can tackle in a subsequent PR if necessary.

platform/macos/src/MGLMapView.mm Outdated Show resolved Hide resolved
platform/ios/src/MGLMapView.h Show resolved Hide resolved
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.

LGTM, thanks for clarifying your intent. Let’s be sure to circle back to these docs when we’re more certain about the implementation.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
iOS Mapbox Maps SDK for iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants