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

Integrate MapboxMap class #3145

Closed
tobrun opened this issue Nov 30, 2015 · 11 comments · Fixed by #3500
Closed

Integrate MapboxMap class #3145

tobrun opened this issue Nov 30, 2015 · 11 comments · Fixed by #3500
Assignees
Labels
Android Mapbox Maps SDK for Android refactor tests

Comments

@tobrun
Copy link
Member

tobrun commented Nov 30, 2015

Copies GoogleMap implementation as indicated by @bleege in #2616 (comment).
This will boil down to applying composition/delegation to our current MapView class.
This is the first step towards making the project more modular and testable.

@tobrun tobrun added the Android Mapbox Maps SDK for Android label Nov 30, 2015
@tobrun tobrun added this to the android-v2.3.0 milestone Nov 30, 2015
@bleege bleege modified the milestones: android-v2.3.0, android-v2.4.0 Dec 4, 2015
@bleege bleege modified the milestones: android-v3.0.0, android-v3.1.0 Dec 21, 2015
@tobrun
Copy link
Member Author

tobrun commented Jan 8, 2016

I have been trying to migrate the whole MapView class to a separate MapboxMap class. This refactor seems a bit big to handle at once. Will granularly move code and start with integrating MapboxMap as a facade interface.

@tobrun
Copy link
Member Author

tobrun commented Jan 8, 2016

Current progress can be viewed here:
https://github.com/mapbox/mapbox-gl-native/tree/3145-Mapbox-class-refactor

@tobrun
Copy link
Member Author

tobrun commented Jan 8, 2016

@bleege @zugaldia
Would love some 👀 on progress above.

Let me explain the why, what changed and whats needs to be done to finish this:

Why

The gist behind the MapboxMap class is having morepartity with Google and more simple feature activities in testapp but most importantly this will allow our code to be more testable because we can test against an non Android SDK component and we can stub out the actual Views eg. MapView.

What changed

What needs to happen to finish

  • actual code needs to migrate from MapView into MapboxMap (current just facading)
  • feature activity for an Activity hosting a MapView (non fragment approach)
  • write tests on the new architecture
  • correctly composes different classes in getMapAsync method
  • correctly refactor annotations to use MapboxMap instead of MapView

Direct links to the classes

@bleege
Copy link
Contributor

bleege commented Jan 8, 2016

@tobrun It looks like the whole refactor is done in 1 commit ( 0dcc5b3 )... is this accurate?

@tobrun
Copy link
Member Author

tobrun commented Jan 8, 2016

@bleege yeah, I shouldn't have rebased before pushing

@bleege
Copy link
Contributor

bleege commented Jan 8, 2016

@tobrun NP... just wanted to make sure I was looking at all the changes.

It looks like the TestApp has been refactored to only use MapFragment. We'll still need some examples that use MapView too. Many developers don't use Fragments and we don't want to give the incorrect impression that they have to use Fragments.

@mourner mourner assigned bleege and unassigned tobrun Jan 8, 2016
@bleege bleege assigned tobrun and unassigned bleege Jan 8, 2016
@tobrun
Copy link
Member Author

tobrun commented Jan 11, 2016

As discussed with @bleege and @zugaldia. We are going for full backwards compatibility for next release (no need for version bump needed for convention semver). More relevant information and discussion points will be added to related PR so we have one place to do discussion and follow up.

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

Successfully merging a pull request may close this issue.

4 participants