From 8a0946358586ee54a42d7d73193b4cc1791ac5e2 Mon Sep 17 00:00:00 2001 From: ank27 Date: Fri, 25 Nov 2022 12:57:56 +0200 Subject: [PATCH] address review comments --- .../mapbox/maps/ViewAnnotationManagerImpl.kt | 51 +++++++++++-------- .../mapbox/maps/ViewAnnotationManagerTest.kt | 10 ++-- .../maps/shadows/ShadowCoordinateBounds.java | 8 +-- 3 files changed, 39 insertions(+), 30 deletions(-) diff --git a/sdk/src/main/java/com/mapbox/maps/ViewAnnotationManagerImpl.kt b/sdk/src/main/java/com/mapbox/maps/ViewAnnotationManagerImpl.kt index 334ee76ac8..688242093f 100644 --- a/sdk/src/main/java/com/mapbox/maps/ViewAnnotationManagerImpl.kt +++ b/sdk/src/main/java/com/mapbox/maps/ViewAnnotationManagerImpl.kt @@ -27,7 +27,7 @@ internal class ViewAnnotationManagerImpl( ) : ViewAnnotationManager, ViewAnnotationPositionsUpdateListener { private val mapboxMap: MapboxMap = mapView.getMapboxMap() private val renderThread = mapView.mapController.renderer.renderThread - private var pixelRatio = mapView.resources.displayMetrics.density + private val pixelRatio = mapView.resources.displayMetrics.density init { viewAnnotationsLayout.layoutParams = FrameLayout.LayoutParams( @@ -213,36 +213,42 @@ internal class ViewAnnotationManagerImpl( val zoom = cameraOptionForCoordinates.zoom isCorrectBound = true viewAnnotationOptions.forEach { options -> - val frame = getViewAnnotationOptionsFrame(options) - val annotationBounds = calculateCoordinateBoundForAnnotation(options, zoom) - if (north == null || calculateCoordinateBoundForAnnotation(north!!.first, zoom).north() < annotationBounds.north()) { + val frame = getViewAnnotationOptionsFrame(options) ?: Rect(0, 0, 0, 0) + val annotationBounds = calculateCoordinateBoundForAnnotation(options, frame, zoom) + if (north == null || calculateCoordinateBoundForAnnotation(north!!.first, frame, zoom).north() < annotationBounds.north()) { north = Pair(options, frame) isCorrectBound = false } - if (east == null || calculateCoordinateBoundForAnnotation(east!!.first, zoom).east() < annotationBounds.east()) { + if (east == null || calculateCoordinateBoundForAnnotation(east!!.first, frame, zoom).east() < annotationBounds.east()) { east = Pair(options, frame) isCorrectBound = false } - if (south == null || calculateCoordinateBoundForAnnotation(south!!.first, zoom).south() > annotationBounds.south()) { + if (south == null || calculateCoordinateBoundForAnnotation(south!!.first, frame, zoom).south() > annotationBounds.south()) { south = Pair(options, frame) isCorrectBound = false } - if (west == null || calculateCoordinateBoundForAnnotation(west!!.first, zoom).west() > annotationBounds.west()) { + if (west == null || calculateCoordinateBoundForAnnotation(west!!.first, frame, zoom).west() > annotationBounds.west()) { west = Pair(options, frame) isCorrectBound = false } } + // adding extra checks for nullability (this shouldn't execute normally as we are checking nullability in loop of viewannotations). + if (north == null || east == null || south == null || west == null) { + logW(TAG, "ViewAnnotation options framing is null. Returning null camera option") + return null + } + val coordinateBoundsForCamera = CoordinateBounds( - Point.fromLngLat((west!!.first.geometry as Point).longitude(), (south!!.first.geometry as Point).latitude()), - Point.fromLngLat((east!!.first.geometry as Point).longitude(), (north!!.first.geometry as Point).latitude()) + Point.fromLngLat(west!!.first.coordinate().longitude(), south!!.first.coordinate().latitude()), + Point.fromLngLat(east!!.first.coordinate().longitude(), north!!.first.coordinate().latitude()) ) val paddings = EdgeInsets( - (edgeInsets?.top ?: 0).toDouble().plus(abs((north!!.second?.top ?: 0.0).toDouble())), - (edgeInsets?.left ?: 0).toDouble().plus(abs((west!!.second?.left ?: 0.0).toDouble())), - (edgeInsets?.bottom ?: 0).toDouble().plus(abs((south!!.second?.bottom ?: 0.0).toDouble())), - (edgeInsets?.right ?: 0).toDouble().plus(abs((east!!.second?.right ?: 0.0).toDouble())) + (edgeInsets?.top ?: 0).toDouble() + abs((north!!.second?.top ?: 0.0).toDouble()), + (edgeInsets?.left ?: 0).toDouble() + abs((west!!.second?.left ?: 0.0).toDouble()), + (edgeInsets?.bottom ?: 0).toDouble() + abs((south!!.second?.bottom ?: 0.0).toDouble()), + (edgeInsets?.right ?: 0).toDouble() + abs((east!!.second?.right ?: 0.0).toDouble()) ) cameraOptionForCoordinates = mapboxMap.cameraForCoordinateBounds( @@ -255,6 +261,8 @@ internal class ViewAnnotationManagerImpl( return cameraOptionForCoordinates } + private fun ViewAnnotationOptions.coordinate() = geometry as Point + /** * Function to calculate coordinatebound associated with annotation option. * this uses [MapboxMap.projectedMetersForCoordinate] and [MapboxMap.coordinateForProjectedMeters] @@ -263,19 +271,19 @@ internal class ViewAnnotationManagerImpl( @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) internal fun calculateCoordinateBoundForAnnotation( viewAnnotationOptions: ViewAnnotationOptions, + annotationFrame: Rect, zoom: Double? ): CoordinateBounds { - val frame = getViewAnnotationOptionsFrame(viewAnnotationOptions) val metersPerPixelAtLatitude = - if (zoom == null) mapboxMap.getMetersPerPixelAtLatitude((viewAnnotationOptions.geometry as Point).latitude()) else - mapboxMap.getMetersPerPixelAtLatitude((viewAnnotationOptions.geometry as Point).latitude(), zoom) + if (zoom == null) mapboxMap.getMetersPerPixelAtLatitude(viewAnnotationOptions.coordinate().latitude()) else + mapboxMap.getMetersPerPixelAtLatitude(viewAnnotationOptions.coordinate().latitude(), zoom) - val projectedMeterForCoordinate = mapboxMap.projectedMetersForCoordinate(viewAnnotationOptions.geometry as Point) + val projectedMeterForCoordinate = mapboxMap.projectedMetersForCoordinate(viewAnnotationOptions.coordinate()) val metersPerPixelDensity = metersPerPixelAtLatitude / pixelRatio - val northing = projectedMeterForCoordinate.northing + (abs(frame?.top ?: 0).toDouble() * metersPerPixelDensity) - val easting = projectedMeterForCoordinate.easting + (abs(frame?.right ?: 0).toDouble() * metersPerPixelDensity) - val southing = projectedMeterForCoordinate.northing - (abs(frame?.bottom ?: 0).toDouble() * metersPerPixelDensity) - val westing = projectedMeterForCoordinate.easting - (abs(frame?.left ?: 0).toDouble() * metersPerPixelDensity) + val northing = projectedMeterForCoordinate.northing + (abs(annotationFrame.top).toDouble() * metersPerPixelDensity) + val easting = projectedMeterForCoordinate.easting + (abs(annotationFrame.right).toDouble() * metersPerPixelDensity) + val southing = projectedMeterForCoordinate.northing - (abs(annotationFrame.bottom).toDouble() * metersPerPixelDensity) + val westing = projectedMeterForCoordinate.easting - (abs(annotationFrame.left).toDouble() * metersPerPixelDensity) val projectedMeterNorthEast = ProjectedMeters(northing, easting) val projectedMeterSouthWest = ProjectedMeters(southing, westing) @@ -653,6 +661,7 @@ internal class ViewAnnotationManagerImpl( internal const val EXCEPTION_TEXT_GEOMETRY_IS_NULL = "Geometry can not be null!" internal const val EXCEPTION_TEXT_ASSOCIATED_FEATURE_ID_ALREADY_EXISTS = "View annotation with associatedFeatureId=%s already exists!" + private const val TAG = "ViewAnnotationImpl" @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) internal fun needToReorderZ( diff --git a/sdk/src/test/java/com/mapbox/maps/ViewAnnotationManagerTest.kt b/sdk/src/test/java/com/mapbox/maps/ViewAnnotationManagerTest.kt index 744622ee5f..82c4bcb06c 100644 --- a/sdk/src/test/java/com/mapbox/maps/ViewAnnotationManagerTest.kt +++ b/sdk/src/test/java/com/mapbox/maps/ViewAnnotationManagerTest.kt @@ -488,10 +488,10 @@ class ViewAnnotationManagerTest { ) } answers { expectedCameraOptions } val coordinateBounds = mockk(relaxUnitFun = true) - every { coordinateBounds.north() } returns 0.0 - every { coordinateBounds.east() } returns 0.0 - every { coordinateBounds.west() } returns 0.0 - every { coordinateBounds.south() } returns 0.0 + every { coordinateBounds.north() } returns 20.0 + every { coordinateBounds.east() } returns 20.0 + every { coordinateBounds.west() } returns 10.0 + every { coordinateBounds.south() } returns 10.0 every { mapboxMap.coordinateBoundsForCamera(any()) } returns coordinateBounds every { mapboxMap.getMetersPerPixelAtLatitude(any(), any()) } returns 1.0 @@ -549,7 +549,7 @@ class ViewAnnotationManagerTest { CoordinateBounds(Point.fromLngLat(0.0, 0.0), Point.fromLngLat(0.0, 0.0)) annotationManagerSpyk.addViewAnnotation(view, viewAnnotationOptions) val actualCoordinateBounds = - annotationManagerSpyk.calculateCoordinateBoundForAnnotation(viewAnnotationOptions, 1.0) + annotationManagerSpyk.calculateCoordinateBoundForAnnotation(viewAnnotationOptions, rect,1.0) assertEquals(expectedCoordinateBounds, actualCoordinateBounds) } diff --git a/sdk/src/test/java/com/mapbox/maps/shadows/ShadowCoordinateBounds.java b/sdk/src/test/java/com/mapbox/maps/shadows/ShadowCoordinateBounds.java index fb834d65da..1ebb260e03 100644 --- a/sdk/src/test/java/com/mapbox/maps/shadows/ShadowCoordinateBounds.java +++ b/sdk/src/test/java/com/mapbox/maps/shadows/ShadowCoordinateBounds.java @@ -12,18 +12,18 @@ public class ShadowCoordinateBounds { @Implementation public static double north() { - return 0.0; + return 20.0; } @Implementation public static double east() { - return 0.0; + return 20.0; } @Implementation public static double south() { - return 0.0; + return 10.0; } @Implementation public static double west() { - return 0.0; + return 10.0; } }