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

kiryldz · 2022-03-17T13:52:39Z

Summary of changes

Add MapboxMap.isValid() and Style.isValid() methods. Map and style become invalid when MapView.onDestroy is called: after that map and style objects are treated as not valid and accessing any method will cause printing error log and introduce a memory leak in user's code.

Another thing worse to mention - for async geojson parsing we do check for style being valid when parsing is over and we're about to send geojson to core.

Additionally going thru all our example activities to fix potential leaks.

User impact (optional)

That PR enhances public StyleInterface with a new method isValid(): Boolean. Technically that's a breaking change but this interface is not intended to be actually overwritten by user (and we do not provide any APIs to register such interfaces) so we treat this as an enhancement or bug fix.

That PR may break existing tests in case mocking StyleInterface in some very specific cases, this should be easily fixed with:

every { style.isValid() } returns true

Pull request checklist:

Briefly describe the changes in this PR.
Include before/after visuals or gifs if this PR includes visual changes.
Write tests for all new functionality. If tests were not written, please explain why.
Optimize code for java consumption (@JvmOverloads, @file:JvmName, etc).
Add example if relevant.
Document any changes to public APIs.
Run make update-api to update generated api files, if there's public API changes, otherwise the verify-api-* CI steps might fail.
Update CHANGELOG.md or use the label 'skip changelog', otherwise check changelog CI step will fail.
If this PR is a v10.[version] release branch fix / enhancement, merge it to main firstly and then port to v10.[version] release branch.

Fixes: < Link to related issues that will be fixed by this pull request, if they exist >

PRs must be submitted under the terms of our Contributor License Agreement CLA.

tobrun

could we validate if having these strings inline doesn't result in a bunch of allocations/de-allocations with each function invocation? Else we need to make them constants instead

sdk/src/main/java/com/mapbox/maps/Style.kt

yunikkk · 2022-03-17T14:16:51Z

Should we also remove WeakRefs from NativeObserver and StyleObserver ?

kiryldz · 2022-03-17T14:24:21Z

@tobrun I do use String.format now as we did in v9 maps here assuming that code there is efficient, how does this sound to you? 😄

tobrun · 2022-03-17T14:34:20Z

@tobrun I do use String.format now as we did in v9 maps here assuming that code there is efficient, how does this sound to you? smile

Yeah I was recently looking at that code and I was yikes this doesn't look good

kiryldz · 2022-03-17T15:38:17Z

@tobrun I do use String.format now as we did in v9 maps here assuming that code there is efficient, how does this sound to you? smile

Yeah I was recently looking at that code and I was yikes this doesn't look good

In any case it should not matter - hitting that if block and writing an error log should happen rarely and signalize that user is leaking style or map object. I guess we should not overthink here how efficiently string is built 😄

sdk/src/main/java/com/mapbox/maps/Style.kt

extension-style/src/main/java/com/mapbox/maps/extension/style/sources/Source.kt

sdk/src/main/java/com/mapbox/maps/MapboxMap.kt

sdk/src/main/java/com/mapbox/maps/Style.kt

CHANGELOG.md

yunikkk · 2022-03-21T18:15:31Z

sdk-base/src/main/java/com/mapbox/maps/extension/style/StyleInterface.kt

@@ -15,6 +15,12 @@ interface StyleInterface : StyleManagerInterface {
   */
  val pixelRatio: Float

+  /**


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

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

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.

@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.

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

sdk/src/main/java/com/mapbox/maps/Style.kt

sdk-base/src/main/java/com/mapbox/maps/extension/style/StyleInterface.kt

Chaoba

Left a question, others LGTM. Thank you for taking this.

sdk/src/main/java/com/mapbox/maps/StyleObserver.kt

pengdev · 2022-03-23T09:38:36Z

extension-style/src/main/java/com/mapbox/maps/extension/style/sources/Source.kt

+          if (!styleDelegate.isValid()) {
+            delegate = null
+            return@let
+          }


Would it make sense to add some error logs here?

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

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

pengdev · 2022-03-23T09:39:37Z

extension-style/src/main/java/com/mapbox/maps/extension/style/sources/SourceExt.kt

@@ -17,11 +17,11 @@ import com.mapbox.maps.extension.style.utils.silentUnwrap
 fun StyleManagerInterface.getSource(sourceId: String): Source? {


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

pengdev

LGTM with 1 comment, really like the documentation updates to the MapboxMap and Style, great work!

kiryldz added the skip changelog label Mar 17, 2022

kiryldz self-assigned this Mar 17, 2022

kiryldz requested a review from a team as a code owner March 17, 2022 13:52

tobrun reviewed Mar 17, 2022

View reviewed changes

yunikkk reviewed Mar 17, 2022

View reviewed changes

sdk/src/main/java/com/mapbox/maps/Style.kt Outdated Show resolved Hide resolved

alexander-kulikovskii reviewed Mar 18, 2022

View reviewed changes

sdk/src/main/java/com/mapbox/maps/Style.kt Show resolved Hide resolved

pengdev reviewed Mar 21, 2022

View reviewed changes

extension-style/src/main/java/com/mapbox/maps/extension/style/sources/Source.kt Outdated Show resolved Hide resolved

pengdev reviewed Mar 21, 2022

View reviewed changes

extension-style/src/main/java/com/mapbox/maps/extension/style/sources/Source.kt Outdated Show resolved Hide resolved

Chaoba reviewed Mar 21, 2022

View reviewed changes

sdk/src/main/java/com/mapbox/maps/MapboxMap.kt Show resolved Hide resolved

kiryldz added 6 commits March 21, 2022 13:39

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

9ca4163

Fix api and changelog

2bb16f0

Use string format instead of raw string

0ffb3ba

Remove remaining weak refs

da0a2c8

Fixes

9fa1ae5

Workaround to reset delegate from geojson source

5ec87cb

kiryldz force-pushed the kdz-refactor-native-map-style-ref branch from 0ef4224 to 1bacb77 Compare March 21, 2022 11:39

Fix tests and check example activities

01fd8f2

kiryldz force-pushed the kdz-refactor-native-map-style-ref branch from 1bacb77 to 01fd8f2 Compare March 21, 2022 11:43

alexander-kulikovskii reviewed Mar 21, 2022

View reviewed changes

sdk/src/main/java/com/mapbox/maps/Style.kt Show resolved Hide resolved

kiryldz requested review from tobrun, yunikkk, pengdev, alexander-kulikovskii and Chaoba March 21, 2022 13:40

Get rid of reflection and enhance interface

e59e356

kiryldz force-pushed the kdz-refactor-native-map-style-ref branch from 6978eee to e59e356 Compare March 21, 2022 15:32

yunikkk reviewed Mar 21, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

yunikkk reviewed Mar 21, 2022

View reviewed changes

sdk/src/main/java/com/mapbox/maps/Style.kt Outdated Show resolved Hide resolved

kiryldz added 2 commits March 22, 2022 11:13

Rework style validness

9112623

Revert style validness as it was before

3cd7be1

kiryldz requested a review from yunikkk March 22, 2022 10:29

yunikkk reviewed Mar 22, 2022

View reviewed changes

sdk-base/src/main/java/com/mapbox/maps/extension/style/StyleInterface.kt Show resolved Hide resolved

yunikkk approved these changes Mar 22, 2022

View reviewed changes

Chaoba approved these changes Mar 22, 2022

View reviewed changes

sdk/src/main/java/com/mapbox/maps/StyleObserver.kt Outdated Show resolved Hide resolved

sdk/src/main/java/com/mapbox/maps/StyleObserver.kt Show resolved Hide resolved

Add more docs

f26c591

kiryldz force-pushed the kdz-refactor-native-map-style-ref branch from 28cdc18 to f26c591 Compare March 22, 2022 14:42

kiryldz merged commit fde9cba into main Mar 22, 2022

kiryldz deleted the kdz-refactor-native-map-style-ref branch March 22, 2022 19:53

pengdev reviewed Mar 23, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kiryldz commented Mar 17, 2022 •

edited

Loading

tobrun left a comment

yunikkk commented Mar 17, 2022

kiryldz commented Mar 17, 2022 •

edited

Loading

tobrun commented Mar 17, 2022

kiryldz commented Mar 17, 2022

yunikkk Mar 21, 2022

kiryldz Mar 22, 2022

pengdev Mar 22, 2022

kiryldz Mar 22, 2022

pengdev Mar 23, 2022

Chaoba left a comment

pengdev Mar 23, 2022

kiryldz Mar 23, 2022

kiryldz May 5, 2022

pengdev Mar 23, 2022

kiryldz Mar 23, 2022

pengdev left a comment

		@@ -17,11 +17,11 @@ import com.mapbox.maps.extension.style.utils.silentUnwrap
		fun StyleManagerInterface.getSource(sourceId: String): Source? {

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

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

Conversation

kiryldz commented Mar 17, 2022 • edited Loading

Summary of changes

User impact (optional)

Pull request checklist:

tobrun left a comment

Choose a reason for hiding this comment

yunikkk commented Mar 17, 2022

kiryldz commented Mar 17, 2022 • edited Loading

tobrun commented Mar 17, 2022

kiryldz commented Mar 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chaoba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pengdev left a comment

Choose a reason for hiding this comment

kiryldz commented Mar 17, 2022 •

edited

Loading

kiryldz commented Mar 17, 2022 •

edited

Loading