Add Android API for scene loading with scene updates #1173

tallytalwar · 2016-12-13T23:21:52Z

MapView provides an overloaded getMapAsync which accepts an ArrayList of SceneUpdates (new
Class).
MapController::loadSceneFile provides an overloaded method to accept an ArrayList of SceneUpdates
MapController::queueSceneUpdates provides an overloaded method to accept an ArrayList of
SceneUpdates

karimnaaji · 2016-12-14T15:45:34Z

android/tangram/src/com/mapzen/tangram/MapController.java

+    public void queueSceneUpdate(ArrayList<SceneUpdate> sceneUpdates) {
+        checkPointer(mapPointer);
+        for(SceneUpdate sceneUpdate : sceneUpdates) {
+            nativeQueueSceneUpdate(mapPointer, sceneUpdate.path, sceneUpdate.value);


Don't we want to make it a single call here nativeQueueSceneUpdates(long mapPtr, String[] sceneUpdatePaths, String[] sceneUpdateValues)? It would reduce jumping between platform code and core library.

Good point, will update.

Recommend using the SceneUpdate type in the older queueSceneUpdate method as well.

karimnaaji · 2016-12-14T15:50:27Z

android/tangram/src/com/mapzen/tangram/SceneUpdate.java

+        set(path, value);
+    }
+
+    public SceneUpdate set(String p, String v) {


If we make this public we should be more explicit on the variable names p and v.
I'm not sure we need the default constructor SceneUpdate() and this setter since the variable are public.

I was just being more formal here. Will remove the constructor and directly assign.

Recommend removing the setter and only setting members in the constructor.

karimnaaji · 2016-12-14T15:51:04Z

android/tangram/src/com/mapzen/tangram/SceneUpdate.java

+    public SceneUpdate set(String p, String v) {
+        path = p;
+        value = v;
+        return this;


Why the return?

I was trying something else before. Will update this.

matteblair · 2016-12-14T17:43:45Z

android/tangram/jni/jniExports.cpp

@@ -118,6 +118,25 @@ extern "C" {
        jniEnv->ReleaseStringUTFChars(path, cPath);
    }

+    JNIEXPORT void JNICALL Java_com_mapzen_tangram_MapController_nativeLoadSceneWithUpdates(JNIEnv* jniEnv, jobject obj, jlong mapPtr, jstring path, jobjectArray jsceneUpdatePaths, jobjectArray jsceneUpdateValues) {


Seems like we could merge this with the method above and just pass a null array when no updates are given.

matteblair · 2016-12-14T17:54:56Z

android/tangram/src/com/mapzen/tangram/MapController.java

+     * @param path Location of the YAML scene file within the application assets
+     * @param sceneUpdates List of path:value pairs to be applied when loading this scene
+     */
+    public void loadSceneFile(String path, ArrayList<SceneUpdate> sceneUpdates) {


These methods should take a List<SceneUpdate>, the List interface should be preferred to concrete types when we don't need the specific type.

We should also check this for null.

matteblair · 2016-12-14T17:58:18Z

android/tangram/src/com/mapzen/tangram/SceneUpdate.java

+    public String path;
+    public String value;
+
+    public SceneUpdate() { this("", ""); }


Recommend removing this constructor.

matteblair · 2016-12-14T17:59:04Z

android/tangram/src/com/mapzen/tangram/SceneUpdate.java

+public class SceneUpdate {
+
+    public String path;
+    public String value;


Recommend making these members private and adding getters.

matteblair · 2016-12-14T17:59:45Z

android/tangram/src/com/mapzen/tangram/SceneUpdate.java

+        set(path, value);
+    }
+
+    public SceneUpdate set(String p, String v) {


Recommend removing the setter and only setting members in the constructor.

matteblair · 2016-12-14T18:02:52Z

android/tangram/src/com/mapzen/tangram/MapController.java

+    public void queueSceneUpdate(ArrayList<SceneUpdate> sceneUpdates) {
+        checkPointer(mapPointer);
+        for(SceneUpdate sceneUpdate : sceneUpdates) {
+            nativeQueueSceneUpdate(mapPointer, sceneUpdate.path, sceneUpdate.value);


Recommend using the SceneUpdate type in the older queueSceneUpdate method as well.

matteblair · 2016-12-14T18:03:30Z

android/tangram/src/com/mapzen/tangram/SceneUpdate.java

+
+    public SceneUpdate() { this("", ""); }
+
+    public SceneUpdate(String path, String value) {


Recommend adding documentation explaining the meaning of these arguments.

matteblair · 2016-12-14T18:04:39Z

android/tangram/src/com/mapzen/tangram/MapView.java

@@ -65,7 +81,11 @@ public void getMapAsync(@NonNull final OnMapReadyCallback callback,
            @SuppressWarnings("WrongThread")
            protected Boolean doInBackground(Void... params) {
                mapInstance.init();
-                mapInstance.loadSceneFile(sceneFilePath);
+                if (sceneUpdates.size() > 0) {


This branch should be unnecessary if we allow empty or null lists as the second argument to loadSceneFile.

tallytalwar · 2016-12-14T21:01:30Z

Addressed all comments here: 7353bbe

karimnaaji · 2016-12-19T17:39:50Z

Comments got removed of the pull request: 7353bbe#commitcomment-20205669. Merging once updated.

tallytalwar · 2016-12-19T17:51:06Z

Ohh, thanks @karimnaaji . Updated.

matteblair · 2016-12-19T18:54:41Z

I have changes to suggest for this - don't merge quite yet please.

karimnaaji · 2016-12-21T15:31:15Z

@ecgreb this one needs your approval! It's good to merge otherwise.

ecgreb

Public API looks good to me. Two suggestions inline regarding input validation and duplicate code.

ecgreb · 2016-12-21T18:32:01Z

android/tangram/src/com/mapzen/tangram/MapController.java

+     * @param sceneUpdates List of {@code SceneUpdate}
+     */
+    public void queueSceneUpdate(List<SceneUpdate> sceneUpdates) {
+        checkPointer(mapPointer);


It would be nice to provide some input validation on sceneUpdates param. Perhaps throw an IllegalArgumentException if it is empty or null?

Hmm, tangram will be handling a null or empty sceneUpdates gracefully, through the jni code here:

tangram-es/android/tangram/jni/jniExports.cpp

Lines 470 to 483 in 4b54361

JNIEXPORT void JNICALL Java_com_mapzen_tangram_MapController_nativeQueueSceneUpdates(JNIEnv* jniEnv, jobject obj, jlong mapPtr, jobjectArray updateStrings) {

assert(mapPtr > 0);

size_t nUpdateStrings = (updateStrings == NULL)? 0 : jniEnv->GetArrayLength(updateStrings);

std::vector<Tangram::SceneUpdate> sceneUpdates;

for (size_t i = 0; i < nUpdateStrings;) {

jstring path = (jstring) (jniEnv->GetObjectArrayElement(updateStrings, i++));

jstring value = (jstring) (jniEnv->GetObjectArrayElement(updateStrings, i++));

sceneUpdates.emplace_back(stringFromJString(jniEnv, path), stringFromJString(jniEnv, value));

jniEnv->DeleteLocalRef(path);

jniEnv->DeleteLocalRef(value);

}

if (sceneUpdates.empty()) { return; }

.

Am I missing java thingy, which will help here?

No the code won't break but it could be helpful to the developer if we provided some explicit feedback rather than failing silently in the case of invalid input.

Also passing data across the JNI is expensive. If we can avoid that by checking the input in Java first that would be preferable.

Ok Both are valid points. Updated here: 8c1ff13

ecgreb · 2016-12-21T18:32:11Z

android/tangram/src/com/mapzen/tangram/MapController.java

+        String[] updateStrings = null;
+
+        if (sceneUpdates != null) {
+            updateStrings = new String[sceneUpdates.size() * 2];


The logic to build updateStrings is repeated again below in queueSceneUpdate(...). Could be extracted into a private method?

Addressed here: c362285

tallytalwar · 2016-12-22T04:26:57Z

With an update to arguments for queueSceneUpdates, this now is an API breaking change (for the next release).

Will rebase this with master and merge.

- MapView provides an overloaded `getMapAsync` which accepts an ArrayList of SceneUpdates (new Class). - MapController::loadSceneFile provides an overloaded method to accept an ArrayList of SceneUpdates - MapController::queueSceneUpdates provides an overloaded method to accept an ArrayList of SceneUpdates

path -> getPath value -> getValue

- BundleSceneUpdate private method bundles list of scene updates as an array of strings alternating scene yaml path and corresponding value

tallytalwar requested review from ecgreb and karimnaaji December 13, 2016 23:22

tallytalwar assigned karimnaaji Dec 13, 2016

tallytalwar added the ready for review label Dec 13, 2016

karimnaaji reviewed Dec 14, 2016

View reviewed changes

matteblair self-requested a review December 14, 2016 16:49

matteblair requested changes Dec 14, 2016

View reviewed changes

matteblair mentioned this pull request Dec 14, 2016

Add description on how to set API key per style tangrams/cartography-docs#2

Closed

tallytalwar force-pushed the android-sceneLoadUpdates branch from 7353bbe to a710b43 Compare December 18, 2016 21:40

karimnaaji approved these changes Dec 19, 2016

View reviewed changes

matteblair approved these changes Dec 21, 2016

View reviewed changes

tallytalwar force-pushed the android-sceneLoadUpdates branch from e77febb to 4b54361 Compare December 21, 2016 00:17

ecgreb approved these changes Dec 21, 2016

View reviewed changes

tallytalwar and others added 6 commits December 21, 2016 23:32

Addressed Comments

4e7f2a6

Be more Java like!!

a163835

path -> getPath value -> getValue

Clean up scene update lists on Android

de69c20

Addressed Comments - code reuse

7575409

- BundleSceneUpdate private method bundles list of scene updates as an array of strings alternating scene yaml path and corresponding value

Throw exception if illegal arguments are passed to queueSceneUpdates

67eb847

tallytalwar force-pushed the android-sceneLoadUpdates branch from 8c1ff13 to 67eb847 Compare December 22, 2016 04:47

tallytalwar merged commit 75f31f2 into master Dec 22, 2016

tallytalwar deleted the android-sceneLoadUpdates branch December 22, 2016 05:26

ecgreb mentioned this pull request Jan 11, 2017

Set tile API key in scene config and update stylesheets mapzen/android#114

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Android API for scene loading with scene updates #1173

Add Android API for scene loading with scene updates #1173

tallytalwar commented Dec 13, 2016 •

edited

Loading

karimnaaji Dec 14, 2016

tallytalwar Dec 14, 2016

matteblair Dec 14, 2016

karimnaaji Dec 14, 2016 •

edited

Loading

tallytalwar Dec 14, 2016

matteblair Dec 14, 2016

karimnaaji Dec 14, 2016

tallytalwar Dec 14, 2016

matteblair Dec 14, 2016

matteblair Dec 14, 2016

matteblair Dec 14, 2016

matteblair Dec 14, 2016

matteblair Dec 14, 2016

matteblair Dec 14, 2016

matteblair Dec 14, 2016

matteblair Dec 14, 2016

matteblair Dec 14, 2016

tallytalwar commented Dec 14, 2016

karimnaaji commented Dec 19, 2016 •

edited

Loading

tallytalwar commented Dec 19, 2016

matteblair commented Dec 19, 2016 •

edited

Loading

karimnaaji commented Dec 21, 2016

ecgreb left a comment

ecgreb Dec 21, 2016

tallytalwar Dec 21, 2016

ecgreb Dec 21, 2016

tallytalwar Dec 22, 2016

ecgreb Dec 22, 2016

ecgreb Dec 21, 2016

tallytalwar Dec 21, 2016

tallytalwar commented Dec 22, 2016 •

edited

Loading


		public SceneUpdate() { this("", ""); }

		public SceneUpdate(String path, String value) {

	JNIEXPORT void JNICALL Java_com_mapzen_tangram_MapController_nativeQueueSceneUpdates(JNIEnv* jniEnv, jobject obj, jlong mapPtr, jobjectArray updateStrings) {
	assert(mapPtr > 0);
	size_t nUpdateStrings = (updateStrings == NULL)? 0 : jniEnv->GetArrayLength(updateStrings);

	std::vector<Tangram::SceneUpdate> sceneUpdates;
	for (size_t i = 0; i < nUpdateStrings;) {
	jstring path = (jstring) (jniEnv->GetObjectArrayElement(updateStrings, i++));
	jstring value = (jstring) (jniEnv->GetObjectArrayElement(updateStrings, i++));
	sceneUpdates.emplace_back(stringFromJString(jniEnv, path), stringFromJString(jniEnv, value));
	jniEnv->DeleteLocalRef(path);
	jniEnv->DeleteLocalRef(value);
	}

	if (sceneUpdates.empty()) { return; }

Add Android API for scene loading with scene updates #1173

Add Android API for scene loading with scene updates #1173

Conversation

tallytalwar commented Dec 13, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karimnaaji Dec 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tallytalwar commented Dec 14, 2016

karimnaaji commented Dec 19, 2016 • edited Loading

tallytalwar commented Dec 19, 2016

matteblair commented Dec 19, 2016 • edited Loading

karimnaaji commented Dec 21, 2016

ecgreb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tallytalwar commented Dec 22, 2016 • edited Loading

tallytalwar commented Dec 13, 2016 •

edited

Loading

karimnaaji Dec 14, 2016 •

edited

Loading

karimnaaji commented Dec 19, 2016 •

edited

Loading

matteblair commented Dec 19, 2016 •

edited

Loading

tallytalwar commented Dec 22, 2016 •

edited

Loading