From 282735772b7f7fe59a16dede9ccb1a5d5af3195a Mon Sep 17 00:00:00 2001 From: liyanjin <36877356+grab-liyanjin@users.noreply.github.com> Date: Tue, 24 Jun 2025 20:38:22 +0800 Subject: [PATCH] Expose sync methods in GeoJsonSource (#3560) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> --- .../src/cpp/style/sources/geojson_source.cpp | 39 ++++++++ .../src/cpp/style/sources/geojson_source.hpp | 9 ++ .../android/style/sources/GeoJsonSource.kt | 75 +++++++++++++++ .../testapp/style/GeoJsonSourceTests.java | 94 +++++++++++++++++++ 4 files changed, 217 insertions(+) diff --git a/platform/android/MapLibreAndroid/src/cpp/style/sources/geojson_source.cpp b/platform/android/MapLibreAndroid/src/cpp/style/sources/geojson_source.cpp index bc37a5f5c..4188db872 100644 --- a/platform/android/MapLibreAndroid/src/cpp/style/sources/geojson_source.cpp +++ b/platform/android/MapLibreAndroid/src/cpp/style/sources/geojson_source.cpp @@ -69,6 +69,16 @@ void GeoJSONSource::setGeoJSONString(jni::JNIEnv& env, const jni::String& jStrin setAsync(converterFn); } +void GeoJSONSource::setGeoJSONStringSync(jni::JNIEnv& env, const jni::String& jString) { + std::shared_ptr json = std::make_shared(jni::Make(env, jString)); + + Update::Converter converterFn = [this, json](ActorRef _callback) { + converter->self().ask(&FeatureConverter::convertJson, json, _callback).wait(); + }; + + setAsync(converterFn); +} + void GeoJSONSource::setFeatureCollection(jni::JNIEnv& env, const jni::Object& jFeatures) { setCollectionAsync(env, jFeatures); } @@ -81,6 +91,19 @@ void GeoJSONSource::setGeometry(jni::JNIEnv& env, const jni::Object& jFeatures) { + setCollectionSync(env, jFeatures); +} + +void GeoJSONSource::setFeatureSync(jni::JNIEnv& env, const jni::Object& jFeature) { + setCollectionSync(env, jFeature); +} + +void GeoJSONSource::setGeometrySync(jni::JNIEnv& env, const jni::Object& jGeometry) { + setCollectionSync(env, jGeometry); +} + void GeoJSONSource::setURL(jni::JNIEnv& env, const jni::String& url) { // Update the core source source.as()->setURL(jni::Make(env, url)); @@ -207,6 +230,18 @@ void GeoJSONSource::setAsync(Update::Converter converterFn) { update->converterFn(update->callback->self()); } +template +void GeoJSONSource::setCollectionSync(jni::JNIEnv& env, const jni::Object& jObject) { + auto global = jni::NewGlobal(env, jObject); + auto object = std::make_shared(std::move(global)); + + Update::Converter converterFn = [this, object](ActorRef _callback) { + converter->self().ask(&FeatureConverter::convertObject, object, _callback).wait(); + }; + + setAsync(converterFn); +} + void GeoJSONSource::registerNative(jni::JNIEnv& env) { // Lookup the class static auto& javaClass = jni::Class::Singleton(env); @@ -225,6 +260,10 @@ void GeoJSONSource::registerNative(jni::JNIEnv& env) { METHOD(&GeoJSONSource::setFeatureCollection, "nativeSetFeatureCollection"), METHOD(&GeoJSONSource::setFeature, "nativeSetFeature"), METHOD(&GeoJSONSource::setGeometry, "nativeSetGeometry"), + METHOD(&GeoJSONSource::setGeoJSONStringSync, "nativeSetGeoJsonStringSync"), + METHOD(&GeoJSONSource::setFeatureCollectionSync, "nativeSetFeatureCollectionSync"), + METHOD(&GeoJSONSource::setFeatureSync, "nativeSetFeatureSync"), + METHOD(&GeoJSONSource::setGeometrySync, "nativeSetGeometrySync"), METHOD(&GeoJSONSource::setURL, "nativeSetUrl"), METHOD(&GeoJSONSource::getURL, "nativeGetUrl"), METHOD(&GeoJSONSource::querySourceFeatures, "querySourceFeatures"), diff --git a/platform/android/MapLibreAndroid/src/cpp/style/sources/geojson_source.hpp b/platform/android/MapLibreAndroid/src/cpp/style/sources/geojson_source.hpp index 6a360484b..6722cb7cb 100644 --- a/platform/android/MapLibreAndroid/src/cpp/style/sources/geojson_source.hpp +++ b/platform/android/MapLibreAndroid/src/cpp/style/sources/geojson_source.hpp @@ -54,6 +54,12 @@ private: void setFeatureCollection(jni::JNIEnv&, const jni::Object&); void setFeature(jni::JNIEnv&, const jni::Object&); void setGeometry(jni::JNIEnv&, const jni::Object&); + + void setGeoJSONStringSync(jni::JNIEnv&, const jni::String&); + void setFeatureCollectionSync(jni::JNIEnv&, const jni::Object&); + void setFeatureSync(jni::JNIEnv&, const jni::Object&); + void setGeometrySync(jni::JNIEnv&, const jni::Object&); + void setURL(jni::JNIEnv&, const jni::String&); jni::Local>> querySourceFeatures(jni::JNIEnv&, @@ -80,6 +86,9 @@ private: void setAsync(Update::Converter); + template + void setCollectionSync(jni::JNIEnv&, const jni::Object&); + }; // class GeoJSONSource } // namespace android diff --git a/platform/android/MapLibreAndroid/src/main/java/org/maplibre/android/style/sources/GeoJsonSource.kt b/platform/android/MapLibreAndroid/src/main/java/org/maplibre/android/style/sources/GeoJsonSource.kt index 68f2161bb..380f37ff1 100644 --- a/platform/android/MapLibreAndroid/src/main/java/org/maplibre/android/style/sources/GeoJsonSource.kt +++ b/platform/android/MapLibreAndroid/src/main/java/org/maplibre/android/style/sources/GeoJsonSource.kt @@ -275,6 +275,55 @@ class GeoJsonSource : Source { } } + + /** + * Updates the GeoJson with a single feature. The update is performed synchronously, + * so the data will be immediately visible and available to query when this method returns. + * + * @param feature the GeoJSON [Feature] to set + */ + fun setGeoJsonSync(feature: Feature?) { + if (detached) { + return + } + checkThread() + nativeSetFeatureSync(feature) + } + + /** + * Updates the GeoJson with a single geometry. The update is performed synchronously, + * so the data will be immediately visible and available to query when this method returns. + * + * @param geometry the GeoJSON [Geometry] to set + */ + fun setGeoJsonSync(geometry: Geometry?) { + if (detached) { + return + } + checkThread() + nativeSetGeometrySync(geometry) + } + + /** + * Updates the GeoJson. The update is performed synchronously, + * so the data will be immediately visible and available to query when this method returns. + * + * @param featureCollection the GeoJSON FeatureCollection + */ + fun setGeoJsonSync(featureCollection: FeatureCollection?) { + if (detached) { + return + } + checkThread() + if (featureCollection != null && featureCollection.features() != null) { + val features = featureCollection.features() + val featuresCopy: List = ArrayList(features) + nativeSetFeatureCollectionSync(FeatureCollection.fromFeatures(featuresCopy)) + } else { + nativeSetFeatureCollectionSync(featureCollection) + } + } + /** * Updates the GeoJson. The update is performed asynchronously, * so the data won't be immediately visible or available to query when this method returns. @@ -289,6 +338,20 @@ class GeoJsonSource : Source { nativeSetGeoJsonString(json) } + /** + * Updates the GeoJson. The update is performed synchronously, + * so the data will be immediately visible and available to query when this method returns. + * + * @param json the raw GeoJson FeatureCollection string + */ + fun setGeoJsonSync(json: String) { + if (detached) { + return + } + checkThread() + nativeSetGeoJsonStringSync(json) + } + /** * Updates the url * @@ -480,6 +543,18 @@ class GeoJsonSource : Source { @Keep private external fun nativeSetGeometry(geometry: Geometry?) + @Keep + private external fun nativeSetGeoJsonStringSync(geoJson: String) + + @Keep + private external fun nativeSetFeatureCollectionSync(geoJson: FeatureCollection?) + + @Keep + private external fun nativeSetFeatureSync(feature: Feature?) + + @Keep + private external fun nativeSetGeometrySync(geometry: Geometry?) + @Keep private external fun querySourceFeatures(filter: Array?): Array diff --git a/platform/android/MapLibreAndroidTestApp/src/androidTest/java/org/maplibre/android/testapp/style/GeoJsonSourceTests.java b/platform/android/MapLibreAndroidTestApp/src/androidTest/java/org/maplibre/android/testapp/style/GeoJsonSourceTests.java index 9f75618c0..9de93454c 100644 --- a/platform/android/MapLibreAndroidTestApp/src/androidTest/java/org/maplibre/android/testapp/style/GeoJsonSourceTests.java +++ b/platform/android/MapLibreAndroidTestApp/src/androidTest/java/org/maplibre/android/testapp/style/GeoJsonSourceTests.java @@ -91,6 +91,26 @@ public class GeoJsonSourceTests extends EspressoTest { }); } + @Test + public void testUpdateCoalescingSync() { + validateTestSetup(); + MapLibreMapAction.invoke(maplibreMap, (uiController, maplibreMap) -> { + GeoJsonSource source = new GeoJsonSource("source"); + maplibreMap.getStyle().addSource(source); + maplibreMap.getStyle().addLayer(new CircleLayer("layer", source.getId())); + + source.setGeoJsonSync(Point.fromLngLat(0, 0)); + source.setGeoJsonSync(Point.fromLngLat(-25, -25)); + source.setGeoJsonSync(ResourceUtils.readRawResource(rule.getActivity(), R.raw.test_feature_properties)); + + source.setGeoJsonSync(Point.fromLngLat(20, 55)); + TestingAsyncUtils.INSTANCE.waitForLayer(uiController, mapView); + assertEquals(1, maplibreMap.queryRenderedFeatures( + maplibreMap.getProjection().toScreenLocation( + new LatLng(55, 20)), "layer").size()); + }); + } + @Test public void testClearCollectionDuringConversion() { // https://github.com/mapbox/mapbox-gl-native/issues/14565 @@ -157,6 +177,56 @@ public class GeoJsonSourceTests extends EspressoTest { }); } + @Test + public void testPointFeatureSync() { + testFeatureFromResourceSync(R.raw.test_point_feature); + } + + @Test + public void testLineStringFeatureSync() { + testFeatureFromResourceSync(R.raw.test_line_string_feature); + } + + @Test + public void testPolygonFeatureSync() { + testFeatureFromResourceSync(R.raw.test_polygon_feature); + } + + @Test + public void testPolygonWithHoleFeatureSync() { + testFeatureFromResourceSync(R.raw.test_polygon_with_hole_feature); + } + + @Test + public void testMultiPointFeatureSync() { + testFeatureFromResourceSync(R.raw.test_multi_point_feature); + } + + @Test + public void testMultiLineStringFeatureSync() { + testFeatureFromResourceSync(R.raw.test_multi_line_string_feature); + } + + @Test + public void testMultiPolygonFeatureSync() { + testFeatureFromResourceSync(R.raw.test_multi_polygon_feature); + } + + protected void testFeatureFromResourceSync(final @RawRes int resource) { + validateTestSetup(); + MapLibreMapAction.invoke(maplibreMap, (uiController, maplibreMap) -> { + GeoJsonSource source = new GeoJsonSource("source"); + maplibreMap.getStyle().addSource(source); + Layer layer = new CircleLayer("layer", source.getId()); + maplibreMap.getStyle().addLayer(layer); + + source.setGeoJsonSync(Feature.fromJson(ResourceUtils.readRawResource(rule.getActivity(), resource))); + + maplibreMap.getStyle().removeLayer(layer); + maplibreMap.getStyle().removeSource(source); + }); + } + @Test @Ignore("https://github.com/maplibre/maplibre-native/issues/3493") public void testDuplicateSourceDuringAsyncSetGeoJson() { @@ -181,6 +251,30 @@ public class GeoJsonSourceTests extends EspressoTest { }); } + @Test + @Ignore("https://github.com/maplibre/maplibre-native/issues/3493") + public void testDuplicateSourceDuringSyncSetGeoJson() { + // regression test for segfault when setting GeoJSON contents + // for a source that has been deleted + // https://github.com/maplibre/maplibre-native/issues/3493 + validateTestSetup(); + MapLibreMapAction.invoke(maplibreMap, (uiController, maplibreMap) -> { + String geoJsonString = "{\"type\":\"Feature\",\"geometry\":{\"type\":\"Point\",\"coordinates\":[0,0]}}"; + for (int i = 0; i < 1000; i++) { + // Create a GeoJSON source with the same ID each iteration + GeoJsonSource source = new GeoJsonSource("source"); + try { + maplibreMap.getStyle().addSource(source); + } catch (CannotAddSourceException ex) { + // ignore, this is expected + } + + // Schedule a sync update via setGeoJson(String) + source.setGeoJsonSync(geoJsonString); + } + }); + } + public abstract class BaseViewAction implements ViewAction { @Override