Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tangram 0.8.0-RC2 #428

Merged
merged 3 commits into from
Aug 21, 2017
Merged

Tangram 0.8.0-RC2 #428

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
525631f
Migrate to tangram 0.8
sarahsnow1 Aug 10, 2017
554f17a
Add more test coverage using separate initializer class
sarahsnow1 Aug 10, 2017
fefd874
deprecate scene update methods instead of removing them
sarahsnow1 Aug 11, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion core/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ group = GROUP
version = VERSION_NAME
project.archivesBaseName = POM_ARTIFACT_ID

ext.tangram_version = "0.6.3"
ext.tangram_version = "0.8.0-RC2"

def SDK_VERSION = hasProperty('version') ? '"' + version + '"' : "null";

Expand Down
25 changes: 11 additions & 14 deletions core/src/main/java/com/mapzen/android/graphics/MapInitializer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@
import com.mapzen.android.core.MapzenManager;
import com.mapzen.android.graphics.model.BubbleWrapStyle;
import com.mapzen.android.graphics.model.MapStyle;
import com.mapzen.android.graphics.model.MarkerManager;
import com.mapzen.tangram.MapController;
import com.mapzen.tangram.SceneError;
import com.mapzen.tangram.SceneUpdate;

import android.content.Context;
Expand All @@ -31,6 +31,8 @@ public class MapInitializer {

private Locale locale = Locale.getDefault();

MapReadyInitializer mapReadyInitializer;

/**
* Creates a new instance.
*/
Expand All @@ -42,6 +44,7 @@ public class MapInitializer {
this.mapDataManager = mapDataManager;
this.mapStateManager = mapStateManager;
this.sceneUpdateManager = sceneUpdateManager;
mapReadyInitializer = new MapReadyInitializer();
}

/**
Expand Down Expand Up @@ -71,10 +74,6 @@ public void init(final MapView mapView, MapStyle mapStyle, Locale locale,
loadMap(mapView, mapStyle, true, callback);
}

private TangramMapView getTangramView(final MapView mapView) {
return mapView.getTangramMapView();
}

private void loadMap(final MapView mapView, MapStyle mapStyle, boolean styleExplicitlySet,
final OnMapReadyCallback callback) {
if (mapStateManager.getPersistMapState() && !styleExplicitlySet) {
Expand All @@ -90,15 +89,13 @@ private void loadMap(final MapView mapView, String sceneFile, final OnMapReadyCa
final List<SceneUpdate> sceneUpdates = sceneUpdateManager.getUpdatesFor(apiKey, locale,
mapStateManager.isTransitOverlayEnabled(), mapStateManager.isBikeOverlayEnabled(),
mapStateManager.isPathOverlayEnabled());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not part of this PR, but does it make sense to provide a isBuildingExtrude option to mapStateManager. Our house style offers options to control this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm glad you brought this up and think it would be nice to provide more global options like this. Created tickets in the iOS and Android SDKs for future work to complete this
#431
mapzen/ios#355

getTangramView(mapView).getMapAsync(new com.mapzen.tangram.MapView.OnMapReadyCallback() {
@Override public void onMapReady(MapController mapController) {
mapController.setHttpHandler(mapzenMapHttpHandler.httpHandler());
MapzenManager mapzenManager = MapzenManager.instance(mapView.getContext());
callback.onMapReady(
new MapzenMap(mapView, mapController, new OverlayManager(mapView, mapController,
mapDataManager, mapStateManager), mapStateManager, new LabelPickHandler(mapView),
new MarkerManager(mapController), sceneUpdateManager, locale, mapzenManager));
MapController controller = mapView.getTangramMapView().getMap(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify this controller is only used to load the scene file and the mapController here: https://github.com/mapzen/android/pull/428/files#diff-2b60561f78b9f2ec09d1ebdfd13cd10cR28, will be set for MapzenMap`?

But this mapController in MapReadyInitializer sets a null for the SceneLoadListener so this does not specify any SceneReadyListener for SceneUpdates. Can you clarify where does this listener gets set for a sceneUpdateAsync?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I set the mapController's SceneLoadListener to null is because:

  1. After the initial scene loads, the SDK considers the map "ready" and doesn't want to invoke the OnMapReadyCallback for other calls to change the scene etc (looking at your comment below, I could also update this to check the sceneId)
  2. The SDK provides type safety around scene updates (https://github.com/mapzen/android/blob/master/core/src/main/java/com/mapzen/android/graphics/SceneUpdateManager.java) and is tested to ensure the calls to sceneUpdateAsync don't produce errors. I know there has been talk about supporting external scenes but at the moment, the SDK doesn't have first class support for this case. If developers care about errors we expect them to access the MapController directly and set a SceneLoadListener on this object. There will be future discussion around this expansion of the SDK though.

Does this reasoning make sense, are there things I'm not considering properly?

On a related note, I was planning on setting a SceneLoadListener as part of the marker restoration work here though (#348).

new MapController.SceneLoadListener() {
@Override public void onSceneReady(int sceneId, SceneError sceneError) {
mapReadyInitializer.onMapReady(mapView, mapzenMapHttpHandler, callback, mapDataManager,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably make sense to use sceneId and sceneError here in MapReadyInitializer.

SceneID could be used to check if any subsequent load scene/scene update has not being called and for what sceneID the OnMapReadyCallback should be called.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because of the way our APIs are setup I don't think this is needed.

The flow the user goes through is either:

  1. MapView#getMapAsync(OnMapReadyCallback) or MapFragment#getMapAsync(OnMapReadyCallback)
  2. other calls to the map after receiving it in OnMapReadyCallback

We wrap all calls to MapController so it is only after receiving this callback that the user has a MapzenMap and can make additional calls to MapController. This means that the first call we get to onSceneReady is guaranteed to be the one we care about because it is in response to the only call we have made to MapController (unless the user has accessed MapController directly and made calls to it. But this case can lead to breaking other parts of our API that we don't support).

mapStateManager, sceneUpdateManager, locale);
}
}, sceneFile, sceneUpdates);
});
controller.loadSceneFileAsync(sceneFile, sceneUpdates);
}
}
37 changes: 37 additions & 0 deletions core/src/main/java/com/mapzen/android/graphics/MapReadyInitializer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
package com.mapzen.android.graphics;

import com.mapzen.android.core.MapzenManager;
import com.mapzen.android.graphics.model.MarkerManager;
import com.mapzen.tangram.MapController;

import java.util.Locale;

/**
* Class to handle creating a {@link MapzenMap} and invoking {@link OnMapReadyCallback}.
*/
class MapReadyInitializer {

/**
* Creates a {@link MapzenMap} configured using the provided parameters and invokes the
* {@link OnMapReadyCallback} with this new map.
* @param mapView
* @param mapzenMapHttpHandler
* @param callback
* @param mapDataManager
* @param mapStateManager
* @param sceneUpdateManager
* @param locale
*/
void onMapReady(MapView mapView, MapzenMapHttpHandler mapzenMapHttpHandler,
OnMapReadyCallback callback, MapDataManager mapDataManager, MapStateManager mapStateManager,
SceneUpdateManager sceneUpdateManager, Locale locale) {
MapController mapController = mapView.getTangramMapView().getMap(null);
mapController.setSceneLoadListener(null);
mapController.setHttpHandler(mapzenMapHttpHandler.httpHandler());
MapzenManager mapzenManager = MapzenManager.instance(mapView.getContext());
callback.onMapReady(
new MapzenMap(mapView, mapController, new OverlayManager(mapView, mapController,
mapDataManager, mapStateManager), mapStateManager, new LabelPickHandler(mapView),
new MarkerManager(mapController), sceneUpdateManager, locale, mapzenManager));
}
}
31 changes: 18 additions & 13 deletions core/src/main/java/com/mapzen/android/graphics/MapzenMap.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import android.view.View;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashMap;
import java.util.List;
import java.util.Locale;
Expand Down Expand Up @@ -89,6 +90,8 @@ public class MapzenMap {
private TouchInput.ScaleResponder scaleResponder;
private TouchInput.ShoveResponder shoveResponder;

private List<SceneUpdate> queuedSceneUpdates = new ArrayList<>();

private static final HashMap<CameraType, MapController.CameraType>
CAMERA_TYPE_TO_MAP_CONTROLLER_CAMERA_TYPE = new HashMap<>();
private static final HashMap<MapController.CameraType, CameraType>
Expand Down Expand Up @@ -693,15 +696,21 @@ public void queueEvent(Runnable r) {
* @param componentPath The YAML component path delimited by a '.' (example "scene.animated")
* @param value A YAML valid string (example "{ property: true }" or "true")
*/
@Deprecated
public void queueSceneUpdate(String componentPath, String value) {
mapController.queueSceneUpdate(new SceneUpdate(componentPath, value));
queuedSceneUpdates.add(new SceneUpdate(componentPath, value));
}

/**
* Apply updates queued by queueSceneUpdate; this empties the current queue of updates.
*/
@Deprecated
public void applySceneUpdates() {
mapController.applySceneUpdates();
if (queuedSceneUpdates.isEmpty()) {
return;
}
mapController.updateSceneAsync(queuedSceneUpdates);
queuedSceneUpdates = new ArrayList<>();
}

/**
Expand Down Expand Up @@ -868,9 +877,8 @@ public void setPersistMapState(boolean persistStateOnRecreation) {
*/
public void setTransitOverlayEnabled(boolean transitOverlayEnabled) {
mapStateManager.setTransitOverlayEnabled(transitOverlayEnabled);
mapController.queueSceneUpdate(sceneUpdateManager.getTransitOverlayUpdate(
transitOverlayEnabled));
mapController.applySceneUpdates();
mapController.updateSceneAsync(Arrays.asList(sceneUpdateManager.getTransitOverlayUpdate(
transitOverlayEnabled)));
}

/**
Expand All @@ -880,9 +888,8 @@ public void setTransitOverlayEnabled(boolean transitOverlayEnabled) {
*/
public void setBikeOverlayEnabled(boolean bikeOverlayEnabled) {
mapStateManager.setBikeOverlayEnabled(bikeOverlayEnabled);
mapController.queueSceneUpdate(sceneUpdateManager.getBikeOverlayUpdate(
bikeOverlayEnabled));
mapController.applySceneUpdates();
mapController.updateSceneAsync(Arrays.asList(sceneUpdateManager.getBikeOverlayUpdate(
bikeOverlayEnabled)));
}

/**
Expand All @@ -892,9 +899,8 @@ public void setBikeOverlayEnabled(boolean bikeOverlayEnabled) {
*/
public void setPathOverlayEnabled(boolean pathOverlayEnabled) {
mapStateManager.setPathOverlayEnabled(pathOverlayEnabled);
mapController.queueSceneUpdate(sceneUpdateManager.getPathOverlayUpdate(
pathOverlayEnabled));
mapController.applySceneUpdates();
mapController.updateSceneAsync(Arrays.asList(sceneUpdateManager.getPathOverlayUpdate(
pathOverlayEnabled)));
}

/**
Expand All @@ -912,8 +918,7 @@ public void setOverlaysEnabled(boolean transitOverlayEnabled, boolean bikeOverla
updates.add(sceneUpdateManager.getTransitOverlayUpdate(transitOverlayEnabled));
updates.add(sceneUpdateManager.getBikeOverlayUpdate(bikeOverlayEnabled));
updates.add(sceneUpdateManager.getPathOverlayUpdate(pathOverlayEnabled));
mapController.queueSceneUpdate(updates);
mapController.applySceneUpdates();
mapController.updateSceneAsync(updates);
}

/**
Expand Down
52 changes: 35 additions & 17 deletions core/src/test/java/com/mapzen/android/graphics/MapInitializerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import com.mapzen.android.core.CoreDI;
import com.mapzen.android.core.MapzenManager;
import com.mapzen.android.graphics.model.BubbleWrapStyle;
import com.mapzen.tangram.MapController;
import com.mapzen.tangram.SceneUpdate;

import org.junit.Before;
Expand Down Expand Up @@ -50,20 +51,22 @@ public class MapInitializerTest {

@Test public void init_shouldReturnMapzenMap() throws Exception {
final TestCallback callback = new TestCallback();
final TestMapView mapView = mock(TestMapView.class);
Context context = mock(Context.class);
when(context.getApplicationContext()).thenReturn(mock(Context.class));
when(mapView.getContext()).thenReturn(context);
when(mapView.getTangramMapView()).thenCallRealMethod();
final TestMapView mapView = new TestMapView(callback);
MapzenManager.instance(getMockContext()).setApiKey("fake-mapzen-api-key");
mapInitializer.init(mapView, callback);
assertThat(callback.map).isInstanceOf(MapzenMap.class);
}

@Test public void init_shouldSetDefaultMapLocale() throws Exception {
// Arrange
MapView mapView = mock(MapView.class);
TangramMapView tangramMapView = mock(TangramMapView.class);
TestCallback callback = mock(TestCallback.class);
TestMapView mapView = mock(TestMapView.class);
TestTangramMapView tangramMapView = mock(TestTangramMapView.class);
MapController mapController = mock(MapController.class);
when(tangramMapView.getMap(any(MapController.SceneLoadListener.class))).thenReturn(
mapController);
tangramMapView.mapView = mapView;
tangramMapView.callback = callback;
when(mapView.getTangramMapView()).thenReturn(tangramMapView);
MapzenManager.instance(getMockContext()).setApiKey("fake-mapzen-api-key");

Expand All @@ -77,14 +80,21 @@ public class MapInitializerTest {
expected.add(new SceneUpdate(STYLE_GLOBAL_VAR_TRANSIT_OVERLAY, "false"));
expected.add(new SceneUpdate(STYLE_GLOBAL_VAR_BIKE_OVERLAY, "false"));
expected.add(new SceneUpdate(STYLE_GLOBAL_VAR_PATH_OVERLAY, "true"));
verify(tangramMapView).getMapAsync(any(com.mapzen.tangram.MapView.OnMapReadyCallback.class),
anyString(), argThat(new SceneUpdatesMatcher(expected)));
verify(tangramMapView).getMap(any(MapController.SceneLoadListener.class));
verify(mapController).loadSceneFileAsync(anyString(), argThat(
new SceneUpdatesMatcher(expected)));
}

@Test public void init_shouldSetGivenMapLocale() throws Exception {
// Arrange
MapView mapView = mock(MapView.class);
TangramMapView tangramMapView = mock(TangramMapView.class);
TestCallback callback = mock(TestCallback.class);
TestMapView mapView = mock(TestMapView.class);
TestTangramMapView tangramMapView = mock(TestTangramMapView.class);
MapController mapController = mock(MapController.class);
when(tangramMapView.getMap(any(MapController.SceneLoadListener.class))).thenReturn(
mapController);
tangramMapView.mapView = mapView;
tangramMapView.callback = callback;
when(mapView.getTangramMapView()).thenReturn(tangramMapView);
MapzenManager.instance(getMockContext()).setApiKey("fake-mapzen-api-key");

Expand All @@ -98,14 +108,21 @@ public class MapInitializerTest {
expected.add(new SceneUpdate(STYLE_GLOBAL_VAR_TRANSIT_OVERLAY, "false"));
expected.add(new SceneUpdate(STYLE_GLOBAL_VAR_BIKE_OVERLAY, "false"));
expected.add(new SceneUpdate(STYLE_GLOBAL_VAR_PATH_OVERLAY, "true"));
verify(tangramMapView).getMapAsync(any(com.mapzen.tangram.MapView.OnMapReadyCallback.class),
anyString(), argThat(new SceneUpdatesMatcher(expected)));
verify(tangramMapView).getMap(any(MapController.SceneLoadListener.class));
verify(mapController).loadSceneFileAsync(anyString(), argThat(
new SceneUpdatesMatcher(expected)));
}

@Test public void init_shouldDefaultToOverlaysDisabledExceptPath() throws Exception {
// Arrange
MapView mapView = mock(MapView.class);
TangramMapView tangramMapView = mock(TangramMapView.class);
TestCallback callback = mock(TestCallback.class);
TestMapView mapView = mock(TestMapView.class);
TestTangramMapView tangramMapView = mock(TestTangramMapView.class);
MapController mapController = mock(MapController.class);
when(tangramMapView.getMap(any(MapController.SceneLoadListener.class))).thenReturn(
mapController);
tangramMapView.mapView = mapView;
tangramMapView.callback = callback;
when(mapView.getTangramMapView()).thenReturn(tangramMapView);
MapzenManager.instance(getMockContext()).setApiKey("fake-mapzen-api-key");

Expand All @@ -119,7 +136,8 @@ public class MapInitializerTest {
expected.add(new SceneUpdate(STYLE_GLOBAL_VAR_TRANSIT_OVERLAY, "false"));
expected.add(new SceneUpdate(STYLE_GLOBAL_VAR_BIKE_OVERLAY, "false"));
expected.add(new SceneUpdate(STYLE_GLOBAL_VAR_PATH_OVERLAY, "true"));
verify(tangramMapView).getMapAsync(any(com.mapzen.tangram.MapView.OnMapReadyCallback.class),
anyString(), argThat(new SceneUpdatesMatcher(expected)));
verify(tangramMapView).getMap(any(MapController.SceneLoadListener.class));
verify(mapController).loadSceneFileAsync(anyString(), argThat(
new SceneUpdatesMatcher(expected)));
}
}
79 changes: 79 additions & 0 deletions core/src/test/java/com/mapzen/android/graphics/MapReadyInitializerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
package com.mapzen.android.graphics;

import com.mapzen.tangram.HttpHandler;
import com.mapzen.tangram.MapController;

import org.junit.Test;
import org.junit.runner.RunWith;
import org.powermock.core.classloader.annotations.PowerMockIgnore;
import org.powermock.core.classloader.annotations.SuppressStaticInitializationFor;
import org.powermock.modules.junit4.PowerMockRunner;

import android.content.Context;

import java.util.Locale;

import static org.mockito.Matchers.any;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

@RunWith(PowerMockRunner.class)
@PowerMockIgnore("javax.net.ssl.*")
@SuppressStaticInitializationFor("com.mapzen.tangram.MapController")
public class MapReadyInitializerTest {

MapReadyInitializer initializer = new MapReadyInitializer();

@Test public void onMapReady_shouldSetMapControllerSceneLoadListenerNull() throws Exception {
MapView mapView = mock(MapView.class);
Context context = mock(Context.class);
when(mapView.getContext()).thenReturn(context);
when(context.getApplicationContext()).thenReturn(mock(Context.class));
TangramMapView tangramMapView = mock(TangramMapView.class);
MapController mapController = mock(MapController.class);
when(mapView.getTangramMapView()).thenReturn(tangramMapView);
when(tangramMapView.getMap(any(MapController.SceneLoadListener.class))).thenReturn(
mapController);
initializer.onMapReady(mapView, mock(MapzenMapHttpHandler.class),
mock(OnMapReadyCallback.class), mock(MapDataManager.class), mock(MapStateManager.class),
mock(SceneUpdateManager.class), new Locale("en_us"));
verify(mapController).setSceneLoadListener(null);
}

@Test public void onMapReady_shouldSetMapControllerHttpHandler() throws Exception {
MapView mapView = mock(MapView.class);
Context context = mock(Context.class);
when(mapView.getContext()).thenReturn(context);
when(context.getApplicationContext()).thenReturn(mock(Context.class));
TangramMapView tangramMapView = mock(TangramMapView.class);
MapController mapController = mock(MapController.class);
when(mapView.getTangramMapView()).thenReturn(tangramMapView);
when(tangramMapView.getMap(any(MapController.SceneLoadListener.class))).thenReturn(
mapController);
MapzenMapHttpHandler mapzenHttpHandler = mock(MapzenMapHttpHandler.class);
HttpHandler httpHandler = mock(HttpHandler.class);
when(mapzenHttpHandler.httpHandler()).thenReturn(httpHandler);
initializer.onMapReady(mapView, mapzenHttpHandler,
mock(OnMapReadyCallback.class), mock(MapDataManager.class), mock(MapStateManager.class),
mock(SceneUpdateManager.class), new Locale("en_us"));
verify(mapController).setHttpHandler(httpHandler);
}

@Test public void onMapReady_shouldCallOnMapReady() throws Exception {
MapView mapView = mock(MapView.class);
Context context = mock(Context.class);
when(mapView.getContext()).thenReturn(context);
when(context.getApplicationContext()).thenReturn(mock(Context.class));
TangramMapView tangramMapView = mock(TangramMapView.class);
MapController mapController = mock(MapController.class);
when(mapView.getTangramMapView()).thenReturn(tangramMapView);
when(tangramMapView.getMap(any(MapController.SceneLoadListener.class))).thenReturn(
mapController);
OnMapReadyCallback callback = mock(OnMapReadyCallback.class);
initializer.onMapReady(mapView, mock(MapzenMapHttpHandler.class),
callback, mock(MapDataManager.class), mock(MapStateManager.class),
mock(SceneUpdateManager.class), new Locale("en_us"));
verify(callback).onMapReady(any(MapzenMap.class));
}
}
Loading