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

Allow to overwrite update camera via delegate #11

Merged
merged 3 commits into from
Jan 3, 2024

Conversation

boldtrn
Copy link
Collaborator

@boldtrn boldtrn commented Dec 26, 2023

Make it possible to customize the camera update through a delegate for implementing apps.

@boldtrn boldtrn added the enhancement New feature or request label Dec 26, 2023
- parameter routeProgress: The current route progress
*/
@objc(navigationMapView:location:routeProgrss:)
optional func updateCamera(_ mapView: NavigationMapView, location: CLLocation, routeProgress: RouteProgress) -> Bool
Copy link
Contributor

@ianthetechie ianthetechie Jan 2, 2024

Choose a reason for hiding this comment

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

Just one minor nitpick on the way that delegate APIs are usually designed on Apple platforms. See UITextViewDelegate for an example of how these are usually named. The logic is also usually inverted so that if the method returns true (or is unimplemented), the user accepts the defaults.

 @objc(navigationMapView:shouldUpdateCameraTo:userLocation:routeProgress:)
    optional func navigationMapView(_ mapView: NavigationMapView, shouldUpdateCameraTo suggestedCamera: MGLCamera, userLocation: CLLocation, routeProgress: RouteProgress) -> Bool

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for looking into this :)

The delegates are already somewhat inconsistently renamed in this project. Some methods use the naming you propose.

For readability I prefer properly named methods instead of tons of the same methods names with different parameter names. Do you think this will be irritating for iOS devs?

If I look into UITextViewDelegate, there are also methods with different naming, for example.

If this is something else, please let me know, it would be great to learn more about Swift 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for looking into this :)

Sure thing! I'll get to the other one about rerouting logic at some point this week as well.

The delegates are already somewhat inconsistently renamed in this project. Some methods use the naming you propose.

For readability I prefer properly named methods instead of tons of the same methods names with different parameter names. Do you think this will be irritating for iOS devs?

Yeah, I gathered that 😂 In which case, as long as it's reasonably consistent, I guess it's at least not new entropy. I would however still fix the typo in the objc signature (Progrss).

If I look into UITextViewDelegate, there are also methods with different naming, for example.

This is actually following the same convention. The general pattern is something like <object>should<Action> for cases where there is only a single parameter, and <object>:should<Action><preposition or something>:<otherParameters>. The conventions are admittedly not intuitive at first, and is very much "legacy" going back to the NEXT days and obj-c, but Apple did codify some of it a few years ago in the Swift API Design Guidelines. Not the easiest reading but if you're bored and want to get deeper into Swift it might be helpful to read at some point (I think it's most helpful to just write a bunch of code, using a bunch of Apple's APIs in practice, and then read the guidelines later as it's hard to just sit down and read, but is a lot easier to skim to solidify the patterns that you'd pick up on subconsciously).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks a lot for the input. I will try to improve my Swift skills :). I have updated the typo and will go ahead and merge this PR.

@boldtrn boldtrn merged commit 50a997f into maplibre:main Jan 3, 2024
1 check passed
@boldtrn boldtrn deleted the overwrite_camera_update branch January 3, 2024 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants