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

"LatLngZoom" & "Coordinate": do they mean the same thing? #3309

Closed
tmcw opened this issue Dec 15, 2015 · 5 comments
Closed

"LatLngZoom" & "Coordinate": do they mean the same thing? #3309

tmcw opened this issue Dec 15, 2015 · 5 comments
Assignees
Labels
Android Mapbox Maps SDK for Android

Comments

@tmcw
Copy link
Contributor

tmcw commented Dec 15, 2015

Our external-facing API has Map.setCenterCoordinate(LatLngZoom) that dispatches to Map.setLatLng and Map.setLatLngZoom. We don't have a class called 'Coordinate' anywhere.

Should LatLng be derived from a Coordinate class? Should it be renamed Coordinate? The two names seem to correspond to the same thing.

@tmcw tmcw added the Android Mapbox Maps SDK for Android label Dec 15, 2015
@tobrun
Copy link
Member

tobrun commented Dec 18, 2015

Center Coordinate

I'm not really sure where the naming of setCenterCoordinate comes from. Using setLatLng would make more sense over setCenterCoordinate. For parity with iOS and Google maps I'm suggesting to keep LatLng and remove Coordinate references.

Google Maps

I'm seeing that Google Maps SDK is using a Camera object for this.

@bleege

  • Is this something that will be changed with your Camera implementation?
  • What are your thoughts on this subject?
Side note

I want to add to this discussion we are sometimes calling a LatLng instance point:

mMapView.setOnMapLongClickListener(new MapView.OnMapLongClickListener() {
         @Override
         public void onMapLongClick(@NonNull LatLng point) {
             // awesome code here
         }
});

This is something that needs to be changed as well.

@tmcw thank you for this valuable feedback, always good to have some extra 👀

@bleege bleege modified the milestones: ios-v3.1.0, android-v3.1.0 Dec 22, 2015
@bleege
Copy link
Contributor

bleege commented Dec 22, 2015

I'm not really sure where the naming of setCenterCoordinate comes from.

I did some sleuthing and found that it comes from 5-Nov-2014.... aka "the first Android PR". It's odd as usually there's been a one to one mapping between MapView.java and NativeMapView.java for Core GL methods (which will be changing as we continue to mirror the Google Maps API). Perhaps this was just a more "human friendly" name?

Regardless, let's move away from this now. With the Mapbox Camera API now in, we can mirror Google's approach by using moveCamera(). The only thing that will have to be managed is the removal of setCenterCoordinate() from MapView as setLanLng() and setLatLngZoom() are both only in NativeMapView which is not accessible. This is not a big deal at all, but it will cause a SemVer major version to be needed.

@tobrun
Copy link
Member

tobrun commented Jan 8, 2016

This will be resolved if #3145 lands

@bleege
Copy link
Contributor

bleege commented Jan 8, 2016

@tobrun Since #3145 is a bit of a bigger lift that may or may not be ready by the time we're ready to release the next PROD version of the SDK let's knock this issue out first as getting things named consistently is important. Cool?

@tobrun
Copy link
Member

tobrun commented Jan 12, 2016

Agreed, will pick this up now

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

No branches or pull requests

4 participants