Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor native Map and Style to log error and not throw exception #1230

Merged
merged 11 commits into from
Mar 22, 2022
Merged

Refactor native Map and Style to log error and not throw exception #1230

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
9ca4163
Refactor native Map and Style to log error and not throw exception
kiryldz Mar 17, 2022
2bb16f0
Fix api and changelog
kiryldz Mar 17, 2022
0ffb3ba
Use string format instead of raw string
kiryldz Mar 17, 2022
da0a2c8
Remove remaining weak refs
kiryldz Mar 17, 2022
9fa1ae5
Fixes
kiryldz Mar 18, 2022
5ec87cb
Workaround to reset delegate from geojson source
kiryldz Mar 18, 2022
01fd8f2
Fix tests and check example activities
kiryldz Mar 21, 2022
e59e356
Get rid of reflection and enhance interface
kiryldz Mar 21, 2022
9112623
Rework style validness
kiryldz Mar 22, 2022
3cd7be1
Revert style validness as it was before
kiryldz Mar 22, 2022
f26c591
Add more docs
kiryldz Mar 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,7 @@ Mapbox welcomes participation and contributions from everyone.
# main
## Features ✨ and improvements 🏁
* Optimize how plugins handle settings changes. Call `applySettings` only when settings value changes. ([#1189](https://github.com/mapbox/mapbox-maps-android/pull/1189))
* Add exception type `MapboxMapMemoryLeakException` which will be thrown when users try to invoke functions related with `nativeMap` in `MapboxMap` and `Style` after onDestroy.
Users check the status of MapboxMap and Style by new method MapboxMap.isValid() and Style.isValid() to avoid such exception.
Automatically unsubscribe all observers in MapboxMap when `MapboxMap.onDestroy()` is invoked. ([1193](https://github.com/mapbox/mapbox-maps-android/pull/1193)) ([1202](https://github.com/mapbox/mapbox-maps-android/pull/1202))
* Add `MapboxMap.isValid()` and `Style.isValid()` methods. `MapboxMap` or `Style` become invalid when `MapView.onDestroy()` is called. Accessing any method on invalid object will print an error log. Also unsubscribe map observers automatically when `MapboxMap.onDestroy()` is invoked. ([1193](https://github.com/mapbox/mapbox-maps-android/pull/1193)) ([1202](https://github.com/mapbox/mapbox-maps-android/pull/1202) ([1230](https://github.com/mapbox/mapbox-maps-android/pull/1230)))
* Add `MapboxMap#coordinateBoundsForCameraUnwrapped` method for API consistency. ([1222](https://github.com/mapbox/mapbox-maps-android/pull/1222))
* Add `LocationIndicatorLayer.bearingTransition` API to control transition of bearing property. (([1207](https://github.com/mapbox/mapbox-maps-android/pull/1207)))

Expand Down
5 changes: 0 additions & 5 deletions app/src/main/java/com/mapbox/maps/testapp/examples/InsetMapActivity.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,11 +133,6 @@ class InsetMapActivity : AppCompatActivity(), OnCameraChangeListener {
)
}

override fun onDestroy() {
super.onDestroy()
mainMapboxMap.removeOnCameraChangeListener(this)
}

companion object {
private const val STYLE_URL = "mapbox://styles/mapbox/cj5l80zrp29942rmtg0zctjto"
private const val INSET_FRAGMENT_TAG = "com.mapbox.insetMapFragment"
Expand Down
2 changes: 1 addition & 1 deletion app/src/main/java/com/mapbox/maps/testapp/examples/SimulateNavigationRouteActivity.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,13 +61,13 @@ class SimulateNavigationRouteActivity : AppCompatActivity() {

override fun onDestroy() {
super.onDestroy()
handler.removeCallbacksAndMessages(null)
if (this::navigationSimulator.isInitialized) {
navigationSimulator.onDestroy()
}
}

companion object {
private const val TAG = "NavigationRouteActivity"
private const val NAVIGATION_ROUTE_JSON_NAME = "navigation_route.json"
private const val INITIAL_OVERVIEW_DELAY_MS = 3000L
private const val FIRST_FOLLOW_MODE_DELAY_MS = 8000L
Expand Down
4 changes: 3 additions & 1 deletion app/src/main/java/com/mapbox/maps/testapp/examples/SpaceStationLocationActivity.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package com.mapbox.maps.testapp.examples
import android.graphics.BitmapFactory
import android.os.Bundle
import android.os.Handler
import android.os.Looper
import android.widget.Toast
import androidx.appcompat.app.AppCompatActivity
import com.mapbox.common.Logger
Expand Down Expand Up @@ -35,7 +36,7 @@ import retrofit2.http.GET
*/
class SpaceStationLocationActivity : AppCompatActivity() {

private val handler = Handler()
private val handler = Handler(Looper.getMainLooper())
private var runnable: (() -> Unit) = {}
private var call: Call<IssModel>? = null
private lateinit var mapboxMap: MapboxMap
Expand Down Expand Up @@ -181,6 +182,7 @@ class SpaceStationLocationActivity : AppCompatActivity() {
// When the user leaves the activity, there is no need in calling the API since the map
// isn't in view.
handler.removeCallbacksAndMessages(null)
call?.cancel()
}

companion object {
Expand Down
4 changes: 2 additions & 2 deletions app/src/main/java/com/mapbox/maps/testapp/examples/fragment/MapFragment.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ class MapFragment : Fragment() {
inflater: LayoutInflater,
container: ViewGroup?,
savedInstanceState: Bundle?
): View? {
): View {
mapView = MapView(
inflater.context,
MapInitOptions(inflater.context)
Expand All @@ -39,7 +39,7 @@ class MapFragment : Fragment() {
callback.invoke(mapboxMap)
} else this.onMapReady = callback

fun getMapView(): MapView? {
fun getMapView(): MapView {
return mapView
}
}
12 changes: 10 additions & 2 deletions extension-style/src/main/java/com/mapbox/maps/extension/style/sources/Source.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,10 @@ import android.util.Log
import com.mapbox.bindgen.Value
import com.mapbox.common.Logger
import com.mapbox.maps.MapboxStyleException
import com.mapbox.maps.StyleManagerInterface
import com.mapbox.maps.extension.style.StyleContract
import com.mapbox.maps.extension.style.StyleInterface
import com.mapbox.maps.extension.style.layers.properties.PropertyValue
import com.mapbox.maps.extension.style.sources.generated.GeoJsonSource
import com.mapbox.maps.extension.style.utils.unwrap

/**
Expand Down Expand Up @@ -46,7 +46,7 @@ abstract class Source(
HashMap<String, PropertyValue<*>>()
}

internal var delegate: StyleManagerInterface? = null
internal var delegate: StyleInterface? = null

/**
* Add the source to the Style.
Expand Down Expand Up @@ -93,6 +93,14 @@ abstract class Source(
private fun updateProperty(property: PropertyValue<*>, throwRuntimeException: Boolean = true) {
delegate?.let { styleDelegate ->
try {
// checking for validness makes sense only for geojson as it uses async parsing
if (this is GeoJsonSource) {
// explicitly reset native reference and return if map is not valid anymore
if (!styleDelegate.isValid()) {
delegate = null
return@let
}
Comment on lines +99 to +102
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to add some error logs here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's non-error situation - map destroyed while we were parsing so see no need in logs here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that code is not correct when using persistent layers, rollbacked in #1324

}
val expected = styleDelegate.setStyleSourceProperty(
sourceId,
property.propertyName,
Expand Down
10 changes: 5 additions & 5 deletions extension-style/src/main/java/com/mapbox/maps/extension/style/sources/SourceExt.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,11 @@ import com.mapbox.maps.extension.style.utils.silentUnwrap
fun StyleManagerInterface.getSource(sourceId: String): Source? {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not in this release, but would consider to directly extend StyleInterface in the next major release.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

return this.getStyleSourceProperty(sourceId, "type").silentUnwrap<String>()?.let { type ->
when (type) {
"vector" -> VectorSource.Builder(sourceId).build().also { it.delegate = this }
"geojson" -> GeoJsonSource.Builder(sourceId).build().also { it.delegate = this }
"image" -> ImageSource.Builder(sourceId).build().also { it.delegate = this }
"raster-dem" -> RasterDemSource.Builder(sourceId).build().also { it.delegate = this }
"raster" -> RasterSource.Builder(sourceId).build().also { it.delegate = this }
"vector" -> VectorSource.Builder(sourceId).build().also { it.delegate = this as StyleInterface }
"geojson" -> GeoJsonSource.Builder(sourceId).build().also { it.delegate = this as StyleInterface }
"image" -> ImageSource.Builder(sourceId).build().also { it.delegate = this as StyleInterface }
"raster-dem" -> RasterDemSource.Builder(sourceId).build().also { it.delegate = this as StyleInterface }
"raster" -> RasterSource.Builder(sourceId).build().also { it.delegate = this as StyleInterface }
else -> {
Logger.e("StyleSourcePlugin", "Source type: $type unknown.")
null
Expand Down
1 change: 1 addition & 0 deletions ...tyle/src/test/java/com/mapbox/maps/extension/style/sources/generated/GeoJsonSourceTest.kt
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions plugin-annotation/src/main/java/com/mapbox/maps/plugin/annotation/AnnotationManagerImpl.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -543,6 +543,8 @@ abstract class AnnotationManagerImpl<G : Geometry, T : Annotation<G>, S : Annota
gesturesPlugin.removeOnMapClickListener(mapClickResolver)
gesturesPlugin.removeOnMapLongClickListener(mapLongClickResolver)
gesturesPlugin.removeOnMoveListener(mapMoveResolver)
annotationMap.clear()
dragAnnotationMap.clear()
dragListeners.clear()
clickListeners.clear()
longClickListeners.clear()
Expand Down
8 changes: 1 addition & 7 deletions sdk-base/api/metalava.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,6 @@ package com.mapbox.maps {
ctor public MapboxCameraAnimationException(String? exceptionText);
}

public final class MapboxExceptionsKt {
}

@kotlin.RequiresOptIn(level=kotlin.RequiresOptIn.Level, message="This API is experimental. It may be changed in the future without notice.") @kotlin.annotation.Retention(kotlin.annotation.AnnotationRetention) @kotlin.annotation.Target(allowedTargets={kotlin.annotation.AnnotationTarget, kotlin.annotation.AnnotationTarget, kotlin.annotation.AnnotationTarget}) public @interface MapboxExperimental {
}

Expand All @@ -36,10 +33,6 @@ package com.mapbox.maps {
ctor public MapboxMapException(String? exceptionText);
}

public final class MapboxMapMemoryLeakException extends java.lang.IllegalStateException {
ctor public MapboxMapMemoryLeakException(String exceptionText = "You are accessing MapboxMap or Style after MapView is destroyed.");
}

public final class MapboxStyleException extends java.lang.RuntimeException {
ctor public MapboxStyleException(String? exceptionText);
}
Expand Down Expand Up @@ -530,6 +523,7 @@ package com.mapbox.maps.extension.style {
public interface StyleInterface extends com.mapbox.maps.StyleManagerInterface {
method public com.mapbox.bindgen.Expected<java.lang.String,com.mapbox.bindgen.None> addImage(String imageId, android.graphics.Bitmap bitmap);
method public float getPixelRatio();
method public boolean isValid();
property public abstract float pixelRatio;
}

Expand Down
7 changes: 1 addition & 6 deletions sdk-base/api/sdk-base.api
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,6 @@ public final class com/mapbox/maps/MapboxMapException : java/lang/RuntimeExcepti
public fun <init> (Ljava/lang/String;)V
}

public final class com/mapbox/maps/MapboxMapMemoryLeakException : java/lang/IllegalStateException {
public fun <init> ()V
public fun <init> (Ljava/lang/String;)V
public synthetic fun <init> (Ljava/lang/String;ILkotlin/jvm/internal/DefaultConstructorMarker;)V
}

public final class com/mapbox/maps/MapboxStyleException : java/lang/RuntimeException {
public fun <init> (Ljava/lang/String;)V
}
Expand Down Expand Up @@ -520,6 +514,7 @@ public abstract interface class com/mapbox/maps/extension/style/StyleContract$St
public abstract interface class com/mapbox/maps/extension/style/StyleInterface : com/mapbox/maps/StyleManagerInterface {
public abstract fun addImage (Ljava/lang/String;Landroid/graphics/Bitmap;)Lcom/mapbox/bindgen/Expected;
public abstract fun getPixelRatio ()F
public abstract fun isValid ()Z
}

public abstract interface class com/mapbox/maps/module/MapTelemetry {
Expand Down
11 changes: 1 addition & 10 deletions sdk-base/src/main/java/com/mapbox/maps/MapboxExceptions.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,13 +40,4 @@ class MapboxCameraAnimationException(
*/
class MapboxStyleException(
exceptionText: String?
) : RuntimeException(exceptionText)

/**
* Mapbox exception class flagging a memory leak did occur.
*/
class MapboxMapMemoryLeakException(
exceptionText: String = MEMORY_LEAK_EXCEPTION_TEXT
) : IllegalStateException(exceptionText)

private const val MEMORY_LEAK_EXCEPTION_TEXT = "You are accessing MapboxMap or Style after MapView is destroyed."
) : RuntimeException(exceptionText)
12 changes: 12 additions & 0 deletions sdk-base/src/main/java/com/mapbox/maps/extension/style/StyleInterface.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,18 @@ interface StyleInterface : StyleManagerInterface {
*/
val pixelRatio: Float

/**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're 100% sure that interface is not implemented with any customer, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, it's our abstractions. While for plugin we potentially have a way to overwrite our default plugins - we do not have that for extensions and this interface is used with style extension only.
@pengdev @tobrun to confirm my understanding is correct here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StyleInterface is implemented in the SDK module itself, we allow user to overwrite the plugin implementations, but the SDK module itself shouldn't and couldn't be implemented by the end users.

The cases I can think of user use the StyleInterface is that they could mock it in their tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pengdev sounds a bit strange to me still as we're operating with Style in our public API. Mostly it's one place:

/**
   * Callback to be invoked when a style has finished loading.
   */
  fun interface OnStyleLoaded {
    /**
     * Invoked when a style has finished loading.
     *
     * @param style the style that has finished loading
     */
    fun onStyleLoaded(style: Style)
  }

so for an end user guess it makes sense to mock Style then... In any case I'll drop instructions in PR description for that case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 didn't remember we return Style async instead of StyleInterface

* Whether the Style instance is valid.
*
* Style becomes invalid after MapView.onDestroy() is invoked,
* calling any method then could result in undefined behaviour and will print an error log.
*
* Keeping the reference to an invalid Style instance introduces significant native memory leak.
kiryldz marked this conversation as resolved.
Show resolved Hide resolved
*
* @return True if Style is valid and false otherwise.
*/
fun isValid(): Boolean

/**
* Adds an image to be used in the style. This API can also be used for updating
* an image. If the image \a id was already added, it gets replaced by the new image.
Expand Down
2 changes: 1 addition & 1 deletion sdk/api/sdk.api
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -410,7 +410,7 @@ public final class com/mapbox/maps/Style : com/mapbox/maps/extension/style/Style
public fun invalidateStyleCustomGeometrySourceTile (Ljava/lang/String;Lcom/mapbox/maps/CanonicalTileID;)Lcom/mapbox/bindgen/Expected;
public fun isStyleLayerPersistent (Ljava/lang/String;)Lcom/mapbox/bindgen/Expected;
public fun isStyleLoaded ()Z
public final fun isValid ()Z
public fun isValid ()Z
public fun moveStyleLayer (Ljava/lang/String;Lcom/mapbox/maps/LayerPosition;)Lcom/mapbox/bindgen/Expected;
public fun removeStyleImage (Ljava/lang/String;)Lcom/mapbox/bindgen/Expected;
public fun removeStyleLayer (Ljava/lang/String;)Lcom/mapbox/bindgen/Expected;
Expand Down
3 changes: 1 addition & 2 deletions sdk/src/main/java/com/mapbox/maps/MapController.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ import com.mapbox.maps.plugin.viewport.ViewportPluginImpl
import com.mapbox.maps.renderer.MapboxRenderer
import com.mapbox.maps.renderer.OnFpsChangedListener
import com.mapbox.maps.renderer.widget.Widget
import java.lang.ref.WeakReference

internal class MapController : MapPluginProviderDelegate, MapControllable {

Expand All @@ -68,7 +67,7 @@ internal class MapController : MapPluginProviderDelegate, MapControllable {
mapInitOptions,
renderer,
)
this.nativeObserver = NativeObserver(WeakReference(nativeMap))
this.nativeObserver = NativeObserver(nativeMap)
this.mapboxMap =
MapProvider.getMapboxMap(nativeMap, nativeObserver, mapInitOptions.mapOptions.pixelRatio)
this.mapboxMap.renderHandler = renderer.renderThread.renderHandlerThread.handler
Expand Down
4 changes: 4 additions & 0 deletions sdk/src/main/java/com/mapbox/maps/MapView.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,10 @@ open class MapView : FrameLayout, MapPluginProviderDelegate, MapControllable {

/**
* Returns a [MapboxMap] object that can be used to interact with the map.
*
* Note: keeping the reference to an invalid [MapboxMap] instance introduces significant native memory leak,
* see [MapboxMap.isValid] for more details.
*
* @return [MapboxMap] object to interact with the map.
*/
override fun getMapboxMap(): MapboxMap = mapController.getMapboxMap()
Expand Down
Loading