setLatLngBounds is poorly named

[1ec5]

#1455 adds the methods -[MLNMapView setLatLngBounds:] and -[MLNMapView clearLatLnBounds], which are poorly named. Effectively, the iOS/macOS SDK now has a method named after the Android SDK’s LatLngBounds class, which makes little sense. That class is analogous to the MLNCoordinateBounds struct on iOS/macOS, so it should be called something like setCoordinateBounds.

Better yet, it should be modeled as a property called something more descriptive like viewableCoordinateBounds. As it is, this is a write-only property, nearly unprecedented in any Objective-C framework. “Clearing” the coordinate bounds could be tricky because MLNCoordinateBounds is a struct, which can’t nullable (optional) in Objective-C. Typically, this library has dealt with that issue by allowing the property to be set to a magic value of some kind. In this case, MLNGeometry.h could define a constant MLNCoordinateBoundsWorld that encompasses the entire coordinate system.

Regardless, there should not be any symbol with “LatLngBounds” in its name. This violates multiple Cocoa naming conventions, including the prohibition on superfluous abbreviations.

[louwers]

Thanks for sharing your perspective. I think the naming comes from the internal C++ API that is called, not the Android SDK. That said your arguments for changing the API make sense and I would be happy to merge an alternate API before the next release if someone make a PR for it.

Related issue which argues to smooth over the differences between the platform SDKs #1462

[1ec5]

I think the naming comes from the internal C++ API that is called, not the Android SDK.

You’re right that it’s a little more complex than I described earlier. mbgl::LatLngBounds was originally called BoundingBox but was renamed to LatLngBounds in mapbox/mapbox-gl-native@78cbab9 as part of mapbox/mapbox-gl-native#1018 mapbox/mapbox-gl-native#1021. Consistency with GL JS was given as the reason.¹ GL JS got the term from Leaflet (mapbox/mapbox-gl-js#369), and Leaflet probably got the term from Google Maps. However, on the mobile platforms, the LatLngBounds terminology was never exposed to external developers until the Android SDK later renamed CoordinateBounds to LatLngBounds, again for consistency with Google Maps: mapbox/mapbox-gl-native#2896 (comment).

So in short, this method is named after something in Google Maps. Consistency with other platforms is one thing, but this method is very inconsistent with other parts of this library and the language it’s written in. After all, MLNCoordinateBounds is named after CLLocationCoordinate2D – the iOS/macOS SDK uses this type from Core Location rather than defining its own LatLng type. @JulianBissekkou, given your work on #1455, would you be open to revisiting that API design?

Footnotes

1. At the time, the Android SDK still called it BoundingBox. That was introduced in Android Geo API Initial Setup mapbox/mapbox-gl-native#878, but you can follow the history all the way back to tobrun/mapbox-android-sdk@4913abb osmdroid/osmdroid@c0449ad. At some point, a redundant CoordinateBounds type was introduced, but it was removed for Remove CoordinateBounds mapbox/mapbox-gl-native#2896. ↩

[JulianBissekkou]

@1ec5
Thanks for your Feedback. Sure we can rename and adjust the naming of the API for the upcoming 6.x.x release.
I will describe the platform consistency goals more in the corresponding issue.

[louwers]

This didn't make the cut because there was a typo in the method name.

A PR with another name is welcome.