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

3145 introduce mapbox class #3500

Merged
merged 1 commit into from
Jan 29, 2016
Merged

3145 introduce mapbox class #3500

merged 1 commit into from
Jan 29, 2016

Conversation

tobrun
Copy link
Member

@tobrun tobrun commented Jan 11, 2016

closes #3145.

As discussed with @bleege and @zugaldia, the introduction of MapboxMap must be completely backwards compatible in next release.
This has resulted in following decisions:

@tobrun
Copy link
Member Author

tobrun commented Jan 11, 2016

To be discussed:

Change packages when we deprecate MapView in future release

This change is required to hide private API with package visibility,

Let me explain this with Google's implementation:

  • Google has two packages:

    screen shot 2016-01-11 at 10 00 57

  • com.google.android.gms.maps contains both the GoogleMap as the MapView class:

    screen shot 2016-01-11 at 10 24 34

  • They leverage the java feature of package visibility to hide methods from their public API/javadoc.

    -> this is feature that we need for MapboxMap <-> MapView interaction.

Above system doesn't match with our current package system because MapView is located in com.mapbox.mapboxsdk.views.

To leverage package visibility we need to move MapboxMap and MapView in the same package.

Possible solutions:
  • Move MapboxMap to com.mapbox.mapboxsdk.views
  • Move MapView to com.mapbox.mapboxsdk
  • Introduce new package and move both classes to com.mapbox.mapboxsdk.maps
  • Introduce new package and move both classes to com.mapbox.mapboxsdk.core
  • Instead of using package visibility use interface to define public API (an interface can still be casted..)

cc @bleege @zugaldia

@tobrun tobrun added this to the android-v3.1.0 milestone Jan 11, 2016
@tobrun tobrun added the Android Mapbox Maps SDK for Android label Jan 11, 2016
@tobrun
Copy link
Member Author

tobrun commented Jan 11, 2016

While doing winter cleaning I have identified some API that needs to be deprecated:

I'm removing these methods from MapboxMap.

@bleege
Copy link
Contributor

bleege commented Jan 11, 2016

How does this relate to the branch / PR #3485 ? Should that one be closed now?

@tobrun
Copy link
Member Author

tobrun commented Jan 11, 2016

@bleege
yes, that branch was not backwards compatible, I migrated the MapboxMap class to this branch.
I have closed that PR.

@tobrun tobrun mentioned this pull request Jan 11, 2016
@bleege
Copy link
Contributor

bleege commented Jan 11, 2016

@tobrun Cool. Thanks for clarifying. There's always so many tickets flying around it can be hard to keep track of which are the germane ones.

@bleege
Copy link
Contributor

bleege commented Jan 11, 2016

the introduction of MapboxMap must be completely backwards compatible in next release.

There is some confusion about this. As mentioned and explained here also, the Mapbox API will be mirroring the use of the Google Maps API. This means that Mapbox will have a MapView and a MapboxMap like Google has a MapView and GoogleMap, but the Mapbox MapView will have it's control methods moved the MapboxMap so that it looks and is used more like Google's MapView. This will be a MAJOR API change and definitely require a new SemVer number for release.

@bleege
Copy link
Contributor

bleege commented Jan 11, 2016

To leverage package visibility we need to move MapboxMap and MapView in the same package.

Good catch! FWIW, I'm fine with com.mapbox.mapboxsdk.maps.* as long as we're disciplined about only putting MapView, MapboxMap, and MapFragment in there. I understand what core is trying to say and I really like that, but the word itself is a bit too vague.

@tobrun
Copy link
Member Author

tobrun commented Jan 11, 2016

@bleege
the reason for backwards compatibility was on request from @zugaldia on the chat:

could we leave both the MapboxMap and the MapView coexisting for one release to give devs a better chance to upgrade?

But I will now focus on getting parity with GoogleMap, without the compatibility support.
Thank you for pointing me in the right direction.

@bleege
Copy link
Contributor

bleege commented Jan 11, 2016

@tobrun No problem at all! Always happy to clarify on things. I remember @zugaldia mentioning that too, but I thought we dismissed it as we were going to do a full SemVer rev anyway. It's always fun to play The Telephone Game! 😃

@zugaldia
Copy link
Member

@bleege @tobrun Indeed 3.1.0 is shaping up really nice with 47 closed tickets as of now: https://github.com/mapbox/mapbox-gl-native/issues?q=is%3Aissue+milestone%3Aandroid-v3.1.0+is%3Aclosed

@bleege bleege modified the milestones: android-v3.1.0, android-v4.0.0 Jan 20, 2016
@tobrun tobrun force-pushed the 3145-Introduce-Mapbox-class branch 3 times, most recently from 52cb25b to 7080ce9 Compare January 26, 2016 08:06
@tobrun
Copy link
Member Author

tobrun commented Jan 28, 2016

A lot has happened in this branch, now that we have a 3.2.0 release branch and an release date.
It is time to see what features will be implemented and which I'm going to take up after merging this to master. I'm aiming to merge this into master the same date we release 3.2.0.
I have no other words than this emoji -> 🚀

@tobrun
Copy link
Member Author

tobrun commented Jan 28, 2016

This is the list of items that I want to resolve before merging

  • Subtyping of CameraUpdates (currently implemented as flag)

These are the items that not made the list:

  • LatLngBounds in CameraUpdateFactory Content insets #3583
  • scrollBy pixelX/pixelY in CameraUpdateFactory (new feature)
  • zoom to a Point in CameraUpdateFactory (new feature)
  • move camera classes to maps package to hide CameraUpdate subtypes
  • [ ]

note to self, create issues for items that did not make the cut after merging

@tobrun tobrun force-pushed the 3145-Introduce-Mapbox-class branch 3 times, most recently from fc1f18f to b635fbc Compare January 29, 2016 11:15
@tobrun
Copy link
Member Author

tobrun commented Jan 29, 2016

CI Approved going to rebase and merge

@tobrun tobrun force-pushed the 3145-Introduce-Mapbox-class branch from b635fbc to 8ed1dc9 Compare January 29, 2016 12:16
@tobrun tobrun merged commit 8ed1dc9 into master Jan 29, 2016
@tobrun tobrun deleted the 3145-Introduce-Mapbox-class branch January 29, 2016 12:18
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

Successfully merging this pull request may close these issues.

Integrate MapboxMap class
5 participants