From 72e3d6906deee2384c1610e13ac6f5cb0ee1ddc5 Mon Sep 17 00:00:00 2001 From: Tobrun Date: Tue, 20 Sep 2016 03:03:49 +0200 Subject: [PATCH] [android] - do not update marker while creating (#6314) * [android] - do not update marker while creating, add activity to prevent regression * [core] - add asserts to incorrect usage of update and remove annotations api * [android] - fix marker tests --- .../mapbox/mapboxsdk/annotations/Marker.java | 7 + .../mapboxsdk/annotations/MarkerOptions.java | 10 +- .../src/main/AndroidManifest.xml | 10 + .../annotation/AddRemoveMarkerActivity.java | 171 ++++++++++++++++++ .../res/layout/activity_add_remove_marker.xml | 19 ++ .../src/main/res/values/strings.xml | 2 + .../mapbox/mapboxsdk/maps/MapboxMapTest.java | 10 +- src/mbgl/annotation/annotation_manager.cpp | 28 ++- 8 files changed, 239 insertions(+), 18 deletions(-) create mode 100644 platform/android/MapboxGLAndroidSDKTestApp/src/main/java/com/mapbox/mapboxsdk/testapp/activity/annotation/AddRemoveMarkerActivity.java create mode 100644 platform/android/MapboxGLAndroidSDKTestApp/src/main/res/layout/activity_add_remove_marker.xml diff --git a/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/annotations/Marker.java b/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/annotations/Marker.java index ea9d9a62e..e1f692a6f 100644 --- a/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/annotations/Marker.java +++ b/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/annotations/Marker.java @@ -49,6 +49,13 @@ public class Marker extends Annotation { title = baseMarkerViewOptions.title; } + Marker(LatLng position, Icon icon, String title, String snippet) { + this.position = position; + this.icon = icon; + this.title = title; + this.snippet = snippet; + } + public LatLng getPosition() { return position; } diff --git a/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/annotations/MarkerOptions.java b/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/annotations/MarkerOptions.java index e0f3b86fc..328ee1894 100644 --- a/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/annotations/MarkerOptions.java +++ b/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/annotations/MarkerOptions.java @@ -22,14 +22,10 @@ import com.mapbox.mapboxsdk.geometry.LatLng; */ public final class MarkerOptions extends BaseMarkerOptions implements Parcelable { - private Marker marker; - public MarkerOptions() { - marker = new Marker(); } protected MarkerOptions(Parcel in) { - marker = new Marker(); position((LatLng) in.readParcelable(LatLng.class.getClassLoader())); snippet(in.readString()); title(in.readString()); @@ -75,11 +71,7 @@ public final class MarkerOptions extends BaseMarkerOptions + + + + + diff --git a/platform/android/MapboxGLAndroidSDKTestApp/src/main/java/com/mapbox/mapboxsdk/testapp/activity/annotation/AddRemoveMarkerActivity.java b/platform/android/MapboxGLAndroidSDKTestApp/src/main/java/com/mapbox/mapboxsdk/testapp/activity/annotation/AddRemoveMarkerActivity.java new file mode 100644 index 000000000..811a87c0a --- /dev/null +++ b/platform/android/MapboxGLAndroidSDKTestApp/src/main/java/com/mapbox/mapboxsdk/testapp/activity/annotation/AddRemoveMarkerActivity.java @@ -0,0 +1,171 @@ +package com.mapbox.mapboxsdk.testapp.activity.annotation; + +import android.os.Bundle; +import android.support.v7.app.ActionBar; +import android.support.v7.app.AppCompatActivity; +import android.support.v7.widget.Toolbar; +import android.util.Log; +import android.view.MenuItem; + +import com.mapbox.mapboxsdk.annotations.Icon; +import com.mapbox.mapboxsdk.annotations.IconFactory; +import com.mapbox.mapboxsdk.annotations.Marker; +import com.mapbox.mapboxsdk.annotations.MarkerOptions; +import com.mapbox.mapboxsdk.camera.CameraPosition; +import com.mapbox.mapboxsdk.camera.CameraUpdateFactory; +import com.mapbox.mapboxsdk.constants.MapboxConstants; +import com.mapbox.mapboxsdk.geometry.LatLng; +import com.mapbox.mapboxsdk.maps.MapView; +import com.mapbox.mapboxsdk.maps.MapboxMap; +import com.mapbox.mapboxsdk.maps.OnMapReadyCallback; +import com.mapbox.mapboxsdk.testapp.R; + +public class AddRemoveMarkerActivity extends AppCompatActivity { + + public static final double THRESHOLD = 5.0; + + private MapView mapView; + private MapboxMap mapboxMap; + private double lastZoom; + private boolean isShowingHighThresholdMarker; + private boolean isShowingLowThresholdMarker; + + private MarkerOptions lowThresholdMarker; + private MarkerOptions highThresholdMarker; + private Marker activeMarker; + + @Override + protected void onCreate(Bundle savedInstanceState) { + super.onCreate(savedInstanceState); + setContentView(R.layout.activity_add_remove_marker); + + Toolbar toolbar = (Toolbar) findViewById(R.id.toolbar); + setSupportActionBar(toolbar); + + ActionBar actionBar = getSupportActionBar(); + if (actionBar != null) { + actionBar.setDisplayHomeAsUpEnabled(true); + actionBar.setDisplayShowHomeEnabled(true); + } + + final Icon icon1 = IconFactory.getInstance(this).fromResource(R.drawable.ic_arsenal); + final Icon icon2 = IconFactory.getInstance(this).fromResource(R.drawable.ic_chelsea); + + lowThresholdMarker = new MarkerOptions() + .icon(icon1) + .position(new LatLng(-0.1, 0)); + + highThresholdMarker = new MarkerOptions() + .icon(icon2) + .position(new LatLng(0.1, 0)); + + mapView = (MapView) findViewById(R.id.mapView); + mapView.onCreate(savedInstanceState); + mapView.getMapAsync(new OnMapReadyCallback() { + @Override + public void onMapReady(MapboxMap mapboxMap) { + AddRemoveMarkerActivity.this.mapboxMap = mapboxMap; + updateZoom(mapboxMap.getCameraPosition().zoom); + mapboxMap.moveCamera(CameraUpdateFactory.zoomTo(4.9f)); + mapboxMap.setOnCameraChangeListener(new MapboxMap.OnCameraChangeListener() { + @Override + public void onCameraChange(CameraPosition position) { + updateZoom(position.zoom); + } + }); + } + }); + } + + private void updateZoom(double zoom) { + if (lastZoom == zoom) { + return; + } + + lastZoom = zoom; + if (zoom > THRESHOLD) { + showHighThresholdMarker(); + } else { + showLowThresholdMarker(); + } + } + + private void showLowThresholdMarker() { + if (isShowingLowThresholdMarker) { + return; + } + + isShowingLowThresholdMarker = true; + isShowingHighThresholdMarker = false; + + if (activeMarker != null) { + Log.d(MapboxConstants.TAG, "Remove marker with " + activeMarker.getId()); + mapboxMap.removeMarker(activeMarker); + } else { + Log.e(MapboxConstants.TAG, "active marker is null"); + } + + activeMarker = mapboxMap.addMarker(lowThresholdMarker); + Log.d(MapboxConstants.TAG, "showLowThresholdMarker() " + activeMarker.getId()); + } + + private void showHighThresholdMarker() { + if (isShowingHighThresholdMarker) { + return; + } + + isShowingLowThresholdMarker = false; + isShowingHighThresholdMarker = true; + + if (activeMarker != null) { + Log.d(MapboxConstants.TAG, "Remove marker with " + activeMarker.getId()); + mapboxMap.removeMarker(activeMarker); + } else { + Log.e(MapboxConstants.TAG, "active marker is null"); + } + + activeMarker = mapboxMap.addMarker(highThresholdMarker); + Log.d(MapboxConstants.TAG, "showHighThresholdMarker() " + activeMarker.getId()); + } + + + @Override + protected void onResume() { + super.onResume(); + mapView.onResume(); + } + + @Override + protected void onPause() { + mapView.onPause(); + super.onPause(); + } + + @Override + protected void onSaveInstanceState(Bundle outState) { + super.onSaveInstanceState(outState); + mapView.onSaveInstanceState(outState); + } + + @Override + public void onLowMemory() { + super.onLowMemory(); + mapView.onLowMemory(); + } + + @Override + protected void onDestroy() { + super.onDestroy(); + mapView.onDestroy(); + } + + @Override + public boolean onOptionsItemSelected(MenuItem item) { + switch (item.getItemId()) { + case android.R.id.home: + onBackPressed(); + return true; + } + return super.onOptionsItemSelected(item); + } +} \ No newline at end of file diff --git a/platform/android/MapboxGLAndroidSDKTestApp/src/main/res/layout/activity_add_remove_marker.xml b/platform/android/MapboxGLAndroidSDKTestApp/src/main/res/layout/activity_add_remove_marker.xml new file mode 100644 index 000000000..596f52aab --- /dev/null +++ b/platform/android/MapboxGLAndroidSDKTestApp/src/main/res/layout/activity_add_remove_marker.xml @@ -0,0 +1,19 @@ + + + + + + + + \ No newline at end of file diff --git a/platform/android/MapboxGLAndroidSDKTestApp/src/main/res/values/strings.xml b/platform/android/MapboxGLAndroidSDKTestApp/src/main/res/values/strings.xml index b6c19300c..52c30bebd 100644 --- a/platform/android/MapboxGLAndroidSDKTestApp/src/main/res/values/strings.xml +++ b/platform/android/MapboxGLAndroidSDKTestApp/src/main/res/values/strings.xml @@ -19,6 +19,7 @@ Press Map For Marker View Marker API Scaling in the View Marker API + Add/Remove marker Standard InfoWindow @@ -112,6 +113,7 @@ Hightligh buildings in box MyLocationView follow map update example Android video view on top of the map view + Based on zoom level Concurrent Open InfoWindows Deselect Markers On Tap diff --git a/platform/android/MapboxGLAndroidSDKTestApp/src/test/java/com/mapbox/mapboxsdk/maps/MapboxMapTest.java b/platform/android/MapboxGLAndroidSDKTestApp/src/test/java/com/mapbox/mapboxsdk/maps/MapboxMapTest.java index 33fd77521..9d0a1c0ad 100644 --- a/platform/android/MapboxGLAndroidSDKTestApp/src/test/java/com/mapbox/mapboxsdk/maps/MapboxMapTest.java +++ b/platform/android/MapboxGLAndroidSDKTestApp/src/test/java/com/mapbox/mapboxsdk/maps/MapboxMapTest.java @@ -590,10 +590,10 @@ public class MapboxMapTest { MarkerOptions markerOptions2 = new MarkerOptions().position(new LatLng()).title("b"); markerList.add(markerOptions1); markerList.add(markerOptions2); - mMapboxMap.addMarkers(markerList); + List markers = mMapboxMap.addMarkers(markerList); assertEquals("Markers size should be 2", 2, mMapboxMap.getMarkers().size()); - assertTrue(mMapboxMap.getMarkers().contains(markerOptions1.getMarker())); - assertTrue(mMapboxMap.getMarkers().contains(markerOptions2.getMarker())); + assertTrue(mMapboxMap.getMarkers().contains(markers.get(0))); + assertTrue(mMapboxMap.getMarkers().contains(markers.get(1))); } @Test @@ -608,9 +608,9 @@ public class MapboxMapTest { List markerList = new ArrayList<>(); MarkerOptions markerOptions = new MarkerOptions().title("a").position(new LatLng()); markerList.add(markerOptions); - mMapboxMap.addMarkers(markerList); + List markers = mMapboxMap.addMarkers(markerList); assertEquals("Markers size should be 1", 1, mMapboxMap.getMarkers().size()); - assertTrue(mMapboxMap.getMarkers().contains(markerOptions.getMarker())); + assertTrue(mMapboxMap.getMarkers().contains(markers.get(0))); } @Test diff --git a/src/mbgl/annotation/annotation_manager.cpp b/src/mbgl/annotation/annotation_manager.cpp index 28a97d292..63adf9de2 100644 --- a/src/mbgl/annotation/annotation_manager.cpp +++ b/src/mbgl/annotation/annotation_manager.cpp @@ -40,7 +40,9 @@ AnnotationManager::~AnnotationManager() = default; AnnotationID AnnotationManager::addAnnotation(const Annotation& annotation, const uint8_t maxZoom) { AnnotationID id = nextID++; - updateAnnotation(id, annotation, maxZoom); + Annotation::visit(annotation, [&] (const auto& annotation_) { + this->add(id, annotation_, maxZoom); + }); return id; } @@ -57,6 +59,8 @@ void AnnotationManager::removeAnnotation(const AnnotationID& id) { } else if (shapeAnnotations.find(id) != shapeAnnotations.end()) { obsoleteShapeAnnotationLayers.insert(shapeAnnotations.at(id)->layerID); shapeAnnotations.erase(id); + } else { + assert(false); // Should never happen } } @@ -85,13 +89,14 @@ void AnnotationManager::add(const AnnotationID& id, const StyleSourcedAnnotation } Update AnnotationManager::update(const AnnotationID& id, const SymbolAnnotation& annotation, const uint8_t maxZoom) { + Update result = Update::Nothing; + auto it = symbolAnnotations.find(id); if (it == symbolAnnotations.end()) { - removeAndAdd(id, annotation, maxZoom); - return Update::AnnotationData | Update::AnnotationStyle; + assert(false); // Attempt to update a non-existent symbol annotation + return result; } - Update result = Update::Nothing; const SymbolAnnotation& existing = it->second->annotation; if (existing.geometry != annotation.geometry) { @@ -110,16 +115,31 @@ Update AnnotationManager::update(const AnnotationID& id, const SymbolAnnotation& } Update AnnotationManager::update(const AnnotationID& id, const LineAnnotation& annotation, const uint8_t maxZoom) { + auto it = shapeAnnotations.find(id); + if (it == shapeAnnotations.end()) { + assert(false); // Attempt to update a non-existent shape annotation + return Update::Nothing; + } removeAndAdd(id, annotation, maxZoom); return Update::AnnotationData | Update::AnnotationStyle; } Update AnnotationManager::update(const AnnotationID& id, const FillAnnotation& annotation, const uint8_t maxZoom) { + auto it = shapeAnnotations.find(id); + if (it == shapeAnnotations.end()) { + assert(false); // Attempt to update a non-existent shape annotation + return Update::Nothing; + } removeAndAdd(id, annotation, maxZoom); return Update::AnnotationData | Update::AnnotationStyle; } Update AnnotationManager::update(const AnnotationID& id, const StyleSourcedAnnotation& annotation, const uint8_t maxZoom) { + auto it = shapeAnnotations.find(id); + if (it == shapeAnnotations.end()) { + assert(false); // Attempt to update a non-existent shape annotation + return Update::Nothing; + } removeAndAdd(id, annotation, maxZoom); return Update::AnnotationData | Update::AnnotationStyle; }