Skip to content

Commit

Permalink
CP lts/v10: Remove explicit main thread locking in onMoveBegin for an…
Browse files Browse the repository at this point in the history
…notations (#2539) (#2541)

* CP lts/v10: Remove explicit main thread locking in onMoveBegin for annotations

* Increase latch timeout

* Fix reflection method name
  • Loading branch information
kiryldz authored Jun 13, 2024
1 parent ebde675 commit 8480784
Show file tree
Hide file tree
Showing 6 changed files with 261 additions and 8 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@

Mapbox welcomes participation and contributions from everyone.

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

# 10.18.1 May 30, 2024
## Bug fixes 🐞
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.getMapboxMap().addOnMapIdleListener {
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_publicDebug",
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(10_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/PublicRelease/metalava.txt
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,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
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package com.mapbox.maps.plugin.annotation

import android.graphics.PointF
import com.mapbox.android.gestures.MoveGestureDetector
import com.mapbox.common.Cancelable
import com.mapbox.geojson.Feature
import com.mapbox.geojson.FeatureCollection
import com.mapbox.geojson.Geometry
Expand Down Expand Up @@ -34,6 +35,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.*
import java.util.concurrent.ConcurrentHashMap
import java.util.concurrent.CountDownLatch
Expand Down Expand Up @@ -638,21 +640,32 @@ abstract class AnnotationManagerImpl<G : Geometry, T : Annotation<G>, S : Annota
}

/**
* 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 @@ -702,7 +715,9 @@ abstract class AnnotationManagerImpl<G : Geometry, T : Annotation<G>, S : Annota
*/
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 @@ -820,6 +835,42 @@ abstract class AnnotationManagerImpl<G : Geometry, T : Annotation<G>, S : Annota
return annotation
}

private fun queryMapForFeaturesAsync(screenCoordinate: ScreenCoordinate, qrfResult: (T?) -> Unit): Cancelable {
val layerList = mutableListOf<String>()
layer?.let {
layerList.add(it.layerId)
}
dragLayer?.let {
layerList.add(it.layerId)
}
return mapFeatureQueryDelegate.queryRenderedFeatures(
RenderedQueryGeometry(screenCoordinate),
RenderedQueryOptions(
layerList,
literal(true)
)
) { features ->
features.value?.firstOrNull()?.feature?.getProperty(getAnnotationIdKey())
?.let { annotationId ->
val id = annotationId.asLong
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)
}
}

/**
* Static variables and methods.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1644,7 +1644,17 @@ class GesturesPluginImpl : GesturesPlugin, GesturesSettingsBase, MapStyleObserve
* 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
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 8480784

Please sign in to comment.