Skip to content

Commit

Permalink
Remove explicit main thread locking in onMoveBegin for annotations (m…
Browse files Browse the repository at this point in the history
…apbox#2530)

* Remove explicit main thread locking in onMoveBegin for annotations

* Cover corner-cases

* PR fixes

* Cancel ongoing qrf when needed

* Add instrument test

* Simulate onMove during QRD

* PR fixes

* Simplify test to make pass on CI

* Try make test stable
  • Loading branch information
kiryldz authored Jun 10, 2024
1 parent 722fedb commit e3968a7
Show file tree
Hide file tree
Showing 8 changed files with 257 additions and 9 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ Mapbox welcomes participation and contributions from everyone.

## Features ✨ and improvements 🏁
* Expose `TerrainState` and `AtmosphereState` properties as `MutableState`.
* Remove explicit main thread locking when using `CircleAnnotationManager`, `PointAnnotationManager`, `PolygonAnnotationManager`, `PolylineAnnotationManager` and dragging the map that could lead to an ANR.

## Bug fixes 🐞
* `Snapshotter` methods throw `SnapshotterDestroyedException` if `destroy` was already called.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,179 @@
package com.mapbox.maps.testapp.annotation

import android.graphics.Color
import androidx.test.espresso.Espresso
import androidx.test.espresso.matcher.ViewMatchers
import com.mapbox.android.gestures.MoveGestureDetector
import com.mapbox.geojson.Point
import com.mapbox.maps.R
import com.mapbox.maps.dsl.cameraOptions
import com.mapbox.maps.plugin.annotation.annotations
import com.mapbox.maps.plugin.annotation.generated.CircleAnnotation
import com.mapbox.maps.plugin.annotation.generated.CircleAnnotationOptions
import com.mapbox.maps.plugin.annotation.generated.createCircleAnnotationManager
import com.mapbox.maps.plugin.gestures.OnMoveListener
import com.mapbox.maps.plugin.gestures.TopPriorityOnMoveListener
import com.mapbox.maps.plugin.gestures.gestures
import com.mapbox.maps.testapp.BaseMapTest
import com.mapbox.maps.testapp.gestures.GesturesUiTestUtils
import org.junit.Assert
import org.junit.Test
import java.util.concurrent.CountDownLatch
import java.util.concurrent.TimeUnit

class AnnotationOnMoveTest : BaseMapTest() {

private lateinit var circleAnnotation: CircleAnnotation

override fun loadMap() {
super.loadMap()
rule.scenario.onActivity {
mapboxMap.setCamera(INITIAL_CAMERA)
}
}

/**
* Assuming we perform async QRF in annotation manager's `onMoveBegin` we have to be sure that:
* a) map is not moved until QRF is executed (this is verified in gesture plugin implementation)
* b) no user explicit `OnMoveListener.onMove`s are called until QRF is executed
* or after QRF showed that we tapped on draggable annotation.
*/
@Test
fun annotationsOnMoveTest() {
var userDefinedMoveBeginCalled = false
val latch = CountDownLatch(1)
rule.scenario.onActivity {
mapView.annotations.createCircleAnnotationManager().apply {
val circleAnnotationOptions: CircleAnnotationOptions = CircleAnnotationOptions()
.withPoint(INITIAL_CAMERA.center!!)
.withCircleColor(Color.YELLOW)
.withCircleRadius(12.0)
.withDraggable(true)
circleAnnotation = create(circleAnnotationOptions)
}
mapView.mapboxMap.subscribeMapIdle {
latch.countDown()
}
mapView.gestures.addOnMoveListener(object : OnMoveListener {
override fun onMoveBegin(detector: MoveGestureDetector) {
userDefinedMoveBeginCalled = true

// simulate situation that gestures plugin received explicit `onMove` during async QRF;
// otherwise QRF in reality works too fast and finishes async before first `onMove` is called by our gestures library
val gesturesImpl = Class.forName("com.mapbox.maps.plugin.gestures.GesturesPluginImpl")
val internalHandleMove = gesturesImpl.getDeclaredMethod(
"handleMove\$plugin_gestures_debug",
MoveGestureDetector::class.java,
Float::class.java,
Float::class.java
)
internalHandleMove.isAccessible = true
internalHandleMove.invoke(
mapView.gestures,
detector,
10.0f,
0f
)
}

override fun onMove(detector: MoveGestureDetector): Boolean {
// Make sure this user defined `onMove` is not called due to the
// `circleAnnotation` consuming all the `onMove` events, either
// because the QRF is being done or because the circle annotation is being dragged
throw RuntimeException("User defined onMove must not be called!")
}

override fun onMoveEnd(detector: MoveGestureDetector) {
}
})
}
Assert.assertTrue(latch.await(2_000, TimeUnit.MILLISECONDS))
// simulate 1-finger pan gesture starting from the center of the MapView
// to make sure we click the annotation
val shiftX = 10f * pixelRatio
Espresso
.onView(ViewMatchers.withId(R.id.mapView))
.perform(
GesturesUiTestUtils.move(
shiftX,
0f
)
)
Assert.assertTrue(userDefinedMoveBeginCalled)
Assert.assertEquals(
24.938827583733797,
circleAnnotation.point.longitude(),
EPS
)
Assert.assertEquals(
INITIAL_CAMERA.center!!.latitude(),
circleAnnotation.point.latitude(),
EPS
)
}

@Test
fun addOnMoveListenerOrderingOneTest() {
val listOfMoveBeginEvents = mutableListOf<String>()
rule.scenario.onActivity {
mapView.gestures.addOnMoveListener(object : OnMoveListener {
override fun onMoveBegin(detector: MoveGestureDetector) {
listOfMoveBeginEvents.add("1_normal")
}

override fun onMove(detector: MoveGestureDetector): Boolean { return false }

override fun onMoveEnd(detector: MoveGestureDetector) { }
})
mapView.gestures.addOnMoveListener(object : OnMoveListener {
override fun onMoveBegin(detector: MoveGestureDetector) {
listOfMoveBeginEvents.add("2_normal")
}

override fun onMove(detector: MoveGestureDetector): Boolean { return false }

override fun onMoveEnd(detector: MoveGestureDetector) { }
})
mapView.gestures.addOnMoveListener(object : TopPriorityOnMoveListener {
override fun onMoveBegin(detector: MoveGestureDetector) {
listOfMoveBeginEvents.add("1_priority")
}

override fun onMove(detector: MoveGestureDetector): Boolean { return false }

override fun onMoveEnd(detector: MoveGestureDetector) { }
})
mapView.gestures.addOnMoveListener(object : TopPriorityOnMoveListener {
override fun onMoveBegin(detector: MoveGestureDetector) {
listOfMoveBeginEvents.add("2_priority")
}

override fun onMove(detector: MoveGestureDetector): Boolean { return false }

override fun onMoveEnd(detector: MoveGestureDetector) { }
})
}
Espresso
.onView(ViewMatchers.withId(R.id.mapView))
.perform(
GesturesUiTestUtils.move(
200f,
0f
)
)
Assert.assertArrayEquals(
arrayOf("1_priority", "2_priority", "1_normal", "2_normal"),
listOfMoveBeginEvents.toTypedArray()
)
}

private companion object {
private val INITIAL_CAMERA = cameraOptions {
center(Point.fromLngLat(24.9384, 60.1699))
zoom(14.0)
pitch(0.0)
bearing(0.0)
}
private const val EPS = 0.0001
}
}
2 changes: 1 addition & 1 deletion plugin-annotation/api/Release/metalava.txt
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ package com.mapbox.maps.plugin.annotation {
method public boolean onMapLongClick(com.mapbox.geojson.Point point);
}

public final class AnnotationManagerImpl.MapMove implements com.mapbox.maps.plugin.gestures.OnMoveListener {
public final class AnnotationManagerImpl.MapMove implements com.mapbox.maps.plugin.gestures.TopPriorityOnMoveListener {
ctor public AnnotationManagerImpl.MapMove();
method public boolean onMove(com.mapbox.android.gestures.MoveGestureDetector detector);
method public void onMoveBegin(com.mapbox.android.gestures.MoveGestureDetector detector);
Expand Down
2 changes: 1 addition & 1 deletion plugin-annotation/api/plugin-annotation.api
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ public final class com/mapbox/maps/plugin/annotation/AnnotationManagerImpl$MapLo
public fun onMapLongClick (Lcom/mapbox/geojson/Point;)Z
}

public final class com/mapbox/maps/plugin/annotation/AnnotationManagerImpl$MapMove : com/mapbox/maps/plugin/gestures/OnMoveListener {
public final class com/mapbox/maps/plugin/annotation/AnnotationManagerImpl$MapMove : com/mapbox/maps/plugin/gestures/TopPriorityOnMoveListener {
public fun <init> (Lcom/mapbox/maps/plugin/annotation/AnnotationManagerImpl;)V
public fun onMove (Lcom/mapbox/android/gestures/MoveGestureDetector;)Z
public fun onMoveBegin (Lcom/mapbox/android/gestures/MoveGestureDetector;)V
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import androidx.annotation.MainThread
import androidx.annotation.VisibleForTesting
import com.mapbox.android.gestures.MoveGestureDetector
import com.mapbox.bindgen.Value
import com.mapbox.common.Cancelable
import com.mapbox.geojson.Feature
import com.mapbox.geojson.FeatureCollection
import com.mapbox.geojson.Geometry
Expand Down Expand Up @@ -44,6 +45,7 @@ import com.mapbox.maps.plugin.gestures.GesturesPlugin
import com.mapbox.maps.plugin.gestures.OnMapClickListener
import com.mapbox.maps.plugin.gestures.OnMapLongClickListener
import com.mapbox.maps.plugin.gestures.OnMoveListener
import com.mapbox.maps.plugin.gestures.TopPriorityOnMoveListener
import java.util.UUID
import java.util.concurrent.CountDownLatch
import java.util.concurrent.TimeUnit
Expand Down Expand Up @@ -620,21 +622,32 @@ internal constructor(
}

/**
* Class handle the map move event
* Class handle the map move event.
*
* It implements [TopPriorityOnMoveListener] to make sure that this listener
* is always invoked before any other added by the user. That assures user's [OnMoveListener.onMove]
* will not be called until async QRF is executed.
*/
inner class MapMove : OnMoveListener {
inner class MapMove : TopPriorityOnMoveListener {

private var asyncQrfCancelable: Cancelable? = null

/**
* Called when the move gesture is starting.
*/
override fun onMoveBegin(detector: MoveGestureDetector) {
if (detector.pointersCount == 1) {
queryMapForFeatures(
asyncQrfCancelable?.cancel()
asyncQrfCancelable = queryMapForFeaturesAsync(
ScreenCoordinate(
detector.focalPoint.x.toDouble(),
detector.focalPoint.y.toDouble()
)
)?.let {
startDragging(it)
) { annotation ->
annotation?.let {
startDragging(it)
}
asyncQrfCancelable = null
}
}
}
Expand Down Expand Up @@ -682,7 +695,9 @@ internal constructor(
*/
updateDragSource()
}
return false
// explicitly handle this `onMove` if QRF is in progress so that
// other registered OnMoveListener's do not get notified
return asyncQrfCancelable != null
}

/**
Expand Down Expand Up @@ -793,6 +808,36 @@ internal constructor(
return annotation
}

private fun queryMapForFeaturesAsync(screenCoordinate: ScreenCoordinate, qrfResult: (T?) -> (Unit)): Cancelable =
mapFeatureQueryDelegate.queryRenderedFeatures(
RenderedQueryGeometry(screenCoordinate),
RenderedQueryOptions(
listOf(layer.layerId, dragLayer.layerId),
literal(true)
)
) { features ->
features.value?.firstOrNull()?.queriedFeature?.feature?.getProperty(getAnnotationIdKey())
?.let { annotationId ->
val id = annotationId.asString
when {
annotationMap.containsKey(id) -> {
qrfResult(annotationMap[id])
}

dragAnnotationMap.containsKey(id) -> {
qrfResult(dragAnnotationMap[id])
}

else -> {
logE(
TAG,
"The queried id: $id, doesn't belong to an active annotation."
)
}
}
} ?: qrfResult(null)
}

protected fun setLayerProperty(value: Value, propertyName: String) {
try {
with(delegateProvider.mapStyleManagerDelegate) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1535,7 +1535,17 @@ internal class GesturesPluginImpl : GesturesPlugin, GesturesSettingsBase, MapSty
* Add a callback that is invoked when the map is moved.
*/
override fun addOnMoveListener(onMoveListener: OnMoveListener) {
onMoveListeners.add(onMoveListener)
if (onMoveListener is TopPriorityOnMoveListener) {
val (topPriority, normalPriority) = onMoveListeners.partition { it is TopPriorityOnMoveListener }
with(onMoveListeners) {
clear()
addAll(topPriority)
add(onMoveListener)
addAll(normalPriority)
}
} else {
onMoveListeners.add(onMoveListener)
}
}

/**
Expand Down
3 changes: 3 additions & 0 deletions sdk-base/api/sdk-base.api
Original file line number Diff line number Diff line change
Expand Up @@ -1872,6 +1872,9 @@ public abstract interface class com/mapbox/maps/plugin/gestures/OnShoveListener
public abstract fun onShoveEnd (Lcom/mapbox/android/gestures/ShoveGestureDetector;)V
}

public abstract interface class com/mapbox/maps/plugin/gestures/TopPriorityOnMoveListener : com/mapbox/maps/plugin/gestures/OnMoveListener {
}

public final class com/mapbox/maps/plugin/gestures/generated/GesturesSettings : android/os/Parcelable {
public static final field CREATOR Landroid/os/Parcelable$Creator;
public synthetic fun <init> (ZZZZZLcom/mapbox/maps/plugin/ScrollMode;ZZZLcom/mapbox/maps/ScreenCoordinate;ZZZZZFZLkotlin/jvm/internal/DefaultConstructorMarker;)V
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package com.mapbox.maps.plugin.gestures

import androidx.annotation.RestrictTo
import com.mapbox.android.gestures.MoveGestureDetector
import com.mapbox.android.gestures.RotateGestureDetector
import com.mapbox.android.gestures.ShoveGestureDetector
Expand Down Expand Up @@ -44,6 +45,15 @@ fun interface OnMapLongClickListener {
fun onMapLongClick(point: Point): Boolean
}

/**
* Represents the top priority [OnMoveListener] which will be always added
* in front of the listener list via [GesturesPlugin.addOnMoveListener].
*
* @suppress
*/
@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP_PREFIX)
interface TopPriorityOnMoveListener : OnMoveListener

/**
* Interface definition for a callback to be invoked when the map is moved.
*/
Expand Down

0 comments on commit e3968a7

Please sign in to comment.