Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose more MGLMapViewDelegate methods #685

Closed
7 tasks
frederoni opened this issue Oct 4, 2017 · 7 comments · Fixed by #1601
Closed
7 tasks

Expose more MGLMapViewDelegate methods #685

frederoni opened this issue Oct 4, 2017 · 7 comments · Fixed by #1601
Assignees

Comments

@frederoni
Copy link
Contributor

frederoni commented Oct 4, 2017

We only delegate mapView(_:viewFor:) and mapView(_:imageFor:) to the NavigationViewControllerDelegate currently. Let's add a few more requested (#666) delegate methods.

  • mapView(_:regionWillChangeAnimated:)
  • mapView(_:regionDidChangeAnimated:)
  • mapView(_:didFinishLoading:) (both methods)
  • mapView(_:viewForAnnotation:)
  • mapView(_:didAddAnnotationViews:)
  • mapView(_:annotationCanShowCallout:)
  • mapView(_:tapOnCalloutForAnnotation:)

@bsudekum @1ec5

@1ec5
Copy link
Contributor

1ec5 commented Oct 4, 2017

NavigationMapView currently seems to implement only mapView(_:imageFor:) and mapView(_:viewFor:), and it does so by turning around and ultimately asking the NavigationViewControllerDelegate’s implementation of those methods. So does NavigationMapViewController even need to be NavigationMapView’s delegate? If not, we can remove all these pass-throughs and let the developer set the map view’s delegate to their own view controller.

@frederoni
Copy link
Contributor Author

@1ec5 It would be great if we could expose all of MGLMapViewDelegate but the RouteMapViewController needs mapView:didFinishLoadingStyle: and mapView:regionDidChangeAnimated:. 🤔

@1ec5
Copy link
Contributor

1ec5 commented Oct 16, 2017

We could imitate -mapView:didFinishLoadingStyle: by key-value observing the style key path. We could move RouteMapViewController off -mapView:regionDidChangeAnimated: using didSet handlers on various properties like tracksUserCourse.

@akitchen
Copy link
Contributor

see also #1526

@mapbox mapbox deleted a comment from lemwerks Jun 28, 2018
@1ec5
Copy link
Contributor

1ec5 commented Jul 16, 2018

Edited the original post with some more MGLMapViewDelegates that developers have told us they’d like to see exposed through NavigationMapViewDelegate and/or NavigationViewControllerDelegate.

@akitchen
Copy link
Contributor

Is there a reason why we don't offer message passing of the entire MGLMapViewDelegate family of delegate methods? Are there any which would negatively impact the navigation experience or be otherwise disruptive?

@1ec5
Copy link
Contributor

1ec5 commented Jul 18, 2018

Passing the entire public interface of MGLMapViewDelegate is the most straightforward solution. The downsides would be that NavigationViewControllerDelegate becomes even more bloated and that we still rely on manually plumbing delegate methods through both RouteMapViewController and NavigationViewController as they’re added to the map SDK. (We’ll need to keep a close eye on the map SDK for any such additions.)

We could expose RouteMapViewController publicly as the go-to class for customizing the map view that’s embedded in NavigationViewController, removing one level of indirection. But there will still be the same issue of NavigationViewController hogging access to the RouteMapViewController’s delegate methods.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants