Skip to content

Commit 42f5059

Browse files
kiryldzkiryldz
authored
CP lts/v10: Remove explicit main thread locking in onMoveBegin for annotations (#2539)
* CP lts/v10: Remove explicit main thread locking in onMoveBegin for annotations * Increase latch timeout * Fix reflection method name
1 parent a96ae0f commit 42f5059

File tree

6 files changed

+261
-8
lines changed

6 files changed

+261
-8
lines changed

‎CHANGELOG.md‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,9 @@
22

33
Mapbox welcomes participation and contributions from everyone.
44

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

69
# 10.18.1 May 30, 2024
710
## Bug fixes 🐞

‎app/src/androidTest/java/com/mapbox/maps/testapp/annotation/AnnotationOnMoveTest.kt‎

Lines changed: 179 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,179 @@
1+
package com.mapbox.maps.testapp.annotation
2+
3+
import android.graphics.Color
4+
import androidx.test.espresso.Espresso
5+
import androidx.test.espresso.matcher.ViewMatchers
6+
import com.mapbox.android.gestures.MoveGestureDetector
7+
import com.mapbox.geojson.Point
8+
import com.mapbox.maps.R
9+
import com.mapbox.maps.dsl.cameraOptions
10+
import com.mapbox.maps.plugin.annotation.annotations
11+
import com.mapbox.maps.plugin.annotation.generated.CircleAnnotation
12+
import com.mapbox.maps.plugin.annotation.generated.CircleAnnotationOptions
13+
import com.mapbox.maps.plugin.annotation.generated.createCircleAnnotationManager
14+
import com.mapbox.maps.plugin.gestures.OnMoveListener
15+
import com.mapbox.maps.plugin.gestures.TopPriorityOnMoveListener
16+
import com.mapbox.maps.plugin.gestures.gestures
17+
import com.mapbox.maps.testapp.BaseMapTest
18+
import com.mapbox.maps.testapp.gestures.GesturesUiTestUtils
19+
import org.junit.Assert
20+
import org.junit.Test
21+
import java.util.concurrent.CountDownLatch
22+
import java.util.concurrent.TimeUnit
23+
24+
class AnnotationOnMoveTest : BaseMapTest() {
25+
26+
private lateinit var circleAnnotation: CircleAnnotation
27+
28+
override fun loadMap() {
29+
super.loadMap()
30+
rule.scenario.onActivity {
31+
mapboxMap.setCamera(INITIAL_CAMERA)
32+
}
33+
}
34+
35+
/**
36+
* Assuming we perform async QRF in annotation manager's `onMoveBegin` we have to be sure that:
37+
* a) map is not moved until QRF is executed (this is verified in gesture plugin implementation)
38+
* b) no user explicit `OnMoveListener.onMove`s are called until QRF is executed
39+
* or after QRF showed that we tapped on draggable annotation.
40+
*/
41+
@Test
42+
fun annotationsOnMoveTest() {
43+
var userDefinedMoveBeginCalled = false
44+
val latch = CountDownLatch(1)
45+
rule.scenario.onActivity {
46+
mapView.annotations.createCircleAnnotationManager().apply {
47+
val circleAnnotationOptions: CircleAnnotationOptions = CircleAnnotationOptions()
48+
.withPoint(INITIAL_CAMERA.center!!)
49+
.withCircleColor(Color.YELLOW)
50+
.withCircleRadius(12.0)
51+
.withDraggable(true)
52+
circleAnnotation = create(circleAnnotationOptions)
53+
}
54+
mapView.getMapboxMap().addOnMapIdleListener {
55+
latch.countDown()
56+
}
57+
mapView.gestures.addOnMoveListener(object : OnMoveListener {
58+
override fun onMoveBegin(detector: MoveGestureDetector) {
59+
userDefinedMoveBeginCalled = true
60+
61+
// simulate situation that gestures plugin received explicit `onMove` during async QRF;
62+
// otherwise QRF in reality works too fast and finishes async before first `onMove` is called by our gestures library
63+
val gesturesImpl = Class.forName("com.mapbox.maps.plugin.gestures.GesturesPluginImpl")
64+
val internalHandleMove = gesturesImpl.getDeclaredMethod(
65+
"handleMove\$plugin_gestures_publicDebug",
66+
MoveGestureDetector::class.java,
67+
Float::class.java,
68+
Float::class.java
69+
)
70+
internalHandleMove.isAccessible = true
71+
internalHandleMove.invoke(
72+
mapView.gestures,
73+
detector,
74+
10.0f,
75+
0f
76+
)
77+
}
78+
79+
override fun onMove(detector: MoveGestureDetector): Boolean {
80+
// Make sure this user defined `onMove` is not called due to the
81+
// `circleAnnotation` consuming all the `onMove` events, either
82+
// because the QRF is being done or because the circle annotation is being dragged
83+
throw RuntimeException("User defined onMove must not be called!")
84+
}
85+
86+
override fun onMoveEnd(detector: MoveGestureDetector) {
87+
}
88+
})
89+
}
90+
Assert.assertTrue(latch.await(10_000, TimeUnit.MILLISECONDS))
91+
// simulate 1-finger pan gesture starting from the center of the MapView
92+
// to make sure we click the annotation
93+
val shiftX = 10f * pixelRatio
94+
Espresso
95+
.onView(ViewMatchers.withId(R.id.mapView))
96+
.perform(
97+
GesturesUiTestUtils.move(
98+
shiftX,
99+
0f
100+
)
101+
)
102+
Assert.assertTrue(userDefinedMoveBeginCalled)
103+
Assert.assertEquals(
104+
24.938827583733797,
105+
circleAnnotation.point.longitude(),
106+
EPS
107+
)
108+
Assert.assertEquals(
109+
INITIAL_CAMERA.center!!.latitude(),
110+
circleAnnotation.point.latitude(),
111+
EPS
112+
)
113+
}
114+
115+
@Test
116+
fun addOnMoveListenerOrderingOneTest() {
117+
val listOfMoveBeginEvents = mutableListOf<String>()
118+
rule.scenario.onActivity {
119+
mapView.gestures.addOnMoveListener(object : OnMoveListener {
120+
override fun onMoveBegin(detector: MoveGestureDetector) {
121+
listOfMoveBeginEvents.add("1_normal")
122+
}
123+
124+
override fun onMove(detector: MoveGestureDetector): Boolean { return false }
125+
126+
override fun onMoveEnd(detector: MoveGestureDetector) { }
127+
})
128+
mapView.gestures.addOnMoveListener(object : OnMoveListener {
129+
override fun onMoveBegin(detector: MoveGestureDetector) {
130+
listOfMoveBeginEvents.add("2_normal")
131+
}
132+
133+
override fun onMove(detector: MoveGestureDetector): Boolean { return false }
134+
135+
override fun onMoveEnd(detector: MoveGestureDetector) { }
136+
})
137+
mapView.gestures.addOnMoveListener(object : TopPriorityOnMoveListener {
138+
override fun onMoveBegin(detector: MoveGestureDetector) {
139+
listOfMoveBeginEvents.add("1_priority")
140+
}
141+
142+
override fun onMove(detector: MoveGestureDetector): Boolean { return false }
143+
144+
override fun onMoveEnd(detector: MoveGestureDetector) { }
145+
})
146+
mapView.gestures.addOnMoveListener(object : TopPriorityOnMoveListener {
147+
override fun onMoveBegin(detector: MoveGestureDetector) {
148+
listOfMoveBeginEvents.add("2_priority")
149+
}
150+
151+
override fun onMove(detector: MoveGestureDetector): Boolean { return false }
152+
153+
override fun onMoveEnd(detector: MoveGestureDetector) { }
154+
})
155+
}
156+
Espresso
157+
.onView(ViewMatchers.withId(R.id.mapView))
158+
.perform(
159+
GesturesUiTestUtils.move(
160+
200f,
161+
0f
162+
)
163+
)
164+
Assert.assertArrayEquals(
165+
arrayOf("1_priority", "2_priority", "1_normal", "2_normal"),
166+
listOfMoveBeginEvents.toTypedArray()
167+
)
168+
}
169+
170+
private companion object {
171+
private val INITIAL_CAMERA = cameraOptions {
172+
center(Point.fromLngLat(24.9384, 60.1699))
173+
zoom(14.0)
174+
pitch(0.0)
175+
bearing(0.0)
176+
}
177+
private const val EPS = 0.0001
178+
}
179+
}

‎plugin-annotation/api/PublicRelease/metalava.txt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ package com.mapbox.maps.plugin.annotation {
5959
method public boolean onMapLongClick(com.mapbox.geojson.Point point);
6060
}
6161

62-
public final class AnnotationManagerImpl.MapMove implements com.mapbox.maps.plugin.gestures.OnMoveListener {
62+
public final class AnnotationManagerImpl.MapMove implements com.mapbox.maps.plugin.gestures.TopPriorityOnMoveListener {
6363
ctor public AnnotationManagerImpl.MapMove();
6464
method public boolean onMove(com.mapbox.android.gestures.MoveGestureDetector detector);
6565
method public void onMoveBegin(com.mapbox.android.gestures.MoveGestureDetector detector);

‎plugin-annotation/src/main/java/com/mapbox/maps/plugin/annotation/AnnotationManagerImpl.kt‎

Lines changed: 57 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package com.mapbox.maps.plugin.annotation
22

33
import android.graphics.PointF
44
import com.mapbox.android.gestures.MoveGestureDetector
5+
import com.mapbox.common.Cancelable
56
import com.mapbox.geojson.Feature
67
import com.mapbox.geojson.FeatureCollection
78
import com.mapbox.geojson.Geometry
@@ -34,6 +35,7 @@ import com.mapbox.maps.plugin.gestures.GesturesPlugin
3435
import com.mapbox.maps.plugin.gestures.OnMapClickListener
3536
import com.mapbox.maps.plugin.gestures.OnMapLongClickListener
3637
import com.mapbox.maps.plugin.gestures.OnMoveListener
38+
import com.mapbox.maps.plugin.gestures.TopPriorityOnMoveListener
3739
import java.util.*
3840
import java.util.concurrent.ConcurrentHashMap
3941
import java.util.concurrent.CountDownLatch
@@ -638,21 +640,32 @@ abstract class AnnotationManagerImpl<G : Geometry, T : Annotation<G>, S : Annota
638640
}
639641

640642
/**
641-
* Class handle the map move event
643+
* Class handle the map move event.
644+
*
645+
* It implements [TopPriorityOnMoveListener] to make sure that this listener
646+
* is always invoked before any other added by the user. That assures user's [OnMoveListener.onMove]
647+
* will not be called until async QRF is executed.
642648
*/
643-
inner class MapMove : OnMoveListener {
649+
inner class MapMove : TopPriorityOnMoveListener {
650+
651+
private var asyncQrfCancelable: Cancelable? = null
652+
644653
/**
645654
* Called when the move gesture is starting.
646655
*/
647656
override fun onMoveBegin(detector: MoveGestureDetector) {
648657
if (detector.pointersCount == 1) {
649-
queryMapForFeatures(
658+
asyncQrfCancelable?.cancel()
659+
asyncQrfCancelable = queryMapForFeaturesAsync(
650660
ScreenCoordinate(
651661
detector.focalPoint.x.toDouble(),
652662
detector.focalPoint.y.toDouble()
653663
)
654-
)?.let {
655-
startDragging(it)
664+
) { annotation ->
665+
annotation?.let {
666+
startDragging(it)
667+
}
668+
asyncQrfCancelable = null
656669
}
657670
}
658671
}
@@ -702,7 +715,9 @@ abstract class AnnotationManagerImpl<G : Geometry, T : Annotation<G>, S : Annota
702715
*/
703716
updateDragSource()
704717
}
705-
return false
718+
// explicitly handle this `onMove` if QRF is in progress so that
719+
// other registered OnMoveListener's do not get notified
720+
return asyncQrfCancelable != null
706721
}
707722

708723
/**
@@ -820,6 +835,42 @@ abstract class AnnotationManagerImpl<G : Geometry, T : Annotation<G>, S : Annota
820835
return annotation
821836
}
822837

838+
private fun queryMapForFeaturesAsync(screenCoordinate: ScreenCoordinate, qrfResult: (T?) -> Unit): Cancelable {
839+
val layerList = mutableListOf<String>()
840+
layer?.let {
841+
layerList.add(it.layerId)
842+
}
843+
dragLayer?.let {
844+
layerList.add(it.layerId)
845+
}
846+
return mapFeatureQueryDelegate.queryRenderedFeatures(
847+
RenderedQueryGeometry(screenCoordinate),
848+
RenderedQueryOptions(
849+
layerList,
850+
literal(true)
851+
)
852+
) { features ->
853+
features.value?.firstOrNull()?.feature?.getProperty(getAnnotationIdKey())
854+
?.let { annotationId ->
855+
val id = annotationId.asLong
856+
when {
857+
annotationMap.containsKey(id) -> {
858+
qrfResult(annotationMap[id])
859+
}
860+
dragAnnotationMap.containsKey(id) -> {
861+
qrfResult(dragAnnotationMap[id])
862+
}
863+
else -> {
864+
logE(
865+
TAG,
866+
"The queried id: $id, doesn't belong to an active annotation."
867+
)
868+
}
869+
}
870+
} ?: qrfResult(null)
871+
}
872+
}
873+
823874
/**
824875
* Static variables and methods.
825876
*/

‎plugin-gestures/src/main/java/com/mapbox/maps/plugin/gestures/GesturesPluginImpl.kt‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1644,7 +1644,17 @@ class GesturesPluginImpl : GesturesPlugin, GesturesSettingsBase, MapStyleObserve
16441644
* Add a callback that is invoked when the map is moved.
16451645
*/
16461646
override fun addOnMoveListener(onMoveListener: OnMoveListener) {
1647-
onMoveListeners.add(onMoveListener)
1647+
if (onMoveListener is TopPriorityOnMoveListener) {
1648+
val (topPriority, normalPriority) = onMoveListeners.partition { it is TopPriorityOnMoveListener }
1649+
with(onMoveListeners) {
1650+
clear()
1651+
addAll(topPriority)
1652+
add(onMoveListener)
1653+
addAll(normalPriority)
1654+
}
1655+
} else {
1656+
onMoveListeners.add(onMoveListener)
1657+
}
16481658
}
16491659

16501660
/**

‎sdk-base/src/main/java/com/mapbox/maps/plugin/gestures/GesturesListeners.kt‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
package com.mapbox.maps.plugin.gestures
22

3+
import androidx.annotation.RestrictTo
34
import com.mapbox.android.gestures.MoveGestureDetector
45
import com.mapbox.android.gestures.RotateGestureDetector
56
import com.mapbox.android.gestures.ShoveGestureDetector
@@ -44,6 +45,15 @@ fun interface OnMapLongClickListener {
4445
fun onMapLongClick(point: Point): Boolean
4546
}
4647

48+
/**
49+
* Represents the top priority [OnMoveListener] which will be always added
50+
* in front of the listener list via [GesturesPlugin.addOnMoveListener].
51+
*
52+
* @suppress
53+
*/
54+
@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP_PREFIX)
55+
interface TopPriorityOnMoveListener : OnMoveListener
56+
4757
/**
4858
* Interface definition for a callback to be invoked when the map is moved.
4959
*/

0 commit comments

Comments
 (0)