Restore Markers #433
Restore Markers #433
Conversation
… restoration after orientation change
| @@ -5,6 +5,7 @@ | |||
|
|
|||
| import android.graphics.drawable.Drawable; | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
Is this class only used for BitmapMarkers or also for Markers (backed by MapData)? If so, we should clarify the naming.
There was a problem hiding this comment.
Updated to BitmapMarkerOptions and deprecated MarkerOptions !
| return new BitmapMarker(this, marker, styleStringGenerator); | ||
| BitmapMarker bitmapMarker = bitmapMarkerFactory.createMarker(this, marker, | ||
| styleStringGenerator); | ||
| configureMarker(bitmapMarker, markerOptions.getPosition(), markerOptions.getIconDrawable(), |
There was a problem hiding this comment.
Instead of passing every property, can we not pass the markerOptions explicitly?
There was a problem hiding this comment.
I use this method in two places- here when adding markers given MarkerOptions and then when restoring markers when I don't have options but instead only the BitmapMarker which is why I don't have it taking the MarkerOptions explicitly. I'll create a couple convenience methods on top of this one to handle configuring a marker given just the MarkerOptions and have it call through to this one. How does that sound? I could keep a mapping of MarkerOptions -> BitmapMarker and then have just one method to configure a marker but I'd prefer not to store this info if I can get away with it.
There was a problem hiding this comment.
Yup that sounds fine to me.
| markerOptions.getHeight(), markerOptions.isInteractive(), markerOptions.getColorHex(), | ||
| markerOptions.getColorInt(), markerOptions.isVisible(), markerOptions.getDrawOrder(), | ||
| markerOptions.getUserData()); | ||
| restorableMarkers.add(bitmapMarker); |
There was a problem hiding this comment.
I could be missing something here, but this should be synchronized right? As in when a new scene is loaded its listener should have have a synchronized set of markers to be restored.
There was a problem hiding this comment.
Good point. Updated to use Collections.synchronizedList(restorableMarkers)
| /** | ||
| * Creates {@link BitmapMarker} objects. | ||
| */ | ||
| public class BitmapMarkerFactory { |
There was a problem hiding this comment.
I might not be following the usage of the Factory here, but could this functionality not make sense in MarkerManager itself?
There was a problem hiding this comment.
It definitely could be within MarkerManager but the reason I pulled it out was to make testing easier. If I have a separate object in charge of creating the markers that I inject into BitmapMarkerFactory, I can easily have this factory object generate mocks and make assertions on them when testing:
https://github.com/mapzen/android/pull/433/files#diff-f00bf2e8f7b3f14b7d6bc8fbf88ddabfR40
https://github.com/mapzen/android/pull/433/files#diff-f00bf2e8f7b3f14b7d6bc8fbf88ddabfR57
There was a problem hiding this comment.
ohh yes definitely makes sense. 👍
|
|
||
| import java.util.ArrayList; | ||
| import java.util.List; | ||
|
|
||
| /** | ||
| * Manages {@link BitmapMarker} instances on the map. | ||
| */ | ||
| public class MarkerManager { |
There was a problem hiding this comment.
I always get confused with this Marker name usage. I know its because of
multiple reasons including compatibility with how Marker->MapData usage was and how BitmapMarker->Tangram::Marker usage is. But can we try to keep the naming such that it distinguishes between sdk markers (tangram mapdata) and sdk bitmapmarkers (tangram markers)?
There was a problem hiding this comment.
I agree, the naming is not ideal. I'll update to BitmapMarkerManager for clarity
| * Returns the object used to manager markers. | ||
| * {@link com.mapzen.tangram.Marker}. | ||
| */ | ||
| @Provides @Singleton public MarkerManager providesMarkerManager( |
There was a problem hiding this comment.
Does android sdk support multiple map instances (tangram android does!), if so does it make sense to not make MarkerManager singleton? Or will every map instance gets its own MarkerManager.
There was a problem hiding this comment.
The SDK doesn't support map instances, this is something I've had floating around in my head as a TODO. I created a ticket for it and will add support in a future PR #436
| markerOptions.getHeight(), markerOptions.isInteractive(), markerOptions.getColorHex(), | ||
| markerOptions.getColorInt(), markerOptions.isVisible(), markerOptions.getDrawOrder(), | ||
| markerOptions.getUserData()); | ||
| configureMarker(bitmapMarker, markerOptions); |
There was a problem hiding this comment.
Yeah overloading this makes it more cleaner to me.
Overview
This PR uses
SceneLoadListenerto restore markers on scene updates, including scene updates which occur because of orientation changes. This code doesn't checksceneIdin the listener callback because we always want to restore markers when the scene is ready after any update. It also doesn't checksceneErrorbecause the sdk doesn't have first class support for loading custom scenes and ensures that all house styles work with the apis we provide.Proposed Changes
SceneLoadListenertoMapzenMapMarkerManagerMarkerOptionsto include allBitmapMarkerpropertiesBitmapMarkerto provide property gettersStyleStringGeneratorandMarkerManagerto be singletonsCloses #348