Split callbacks originating on getStyle and loadStyle requests. #1114

yunikkk · 2022-02-02T17:07:56Z

Summary of changes

Problem this PR addresses : location puck is not displayed after style load #321, #1048.

Background context : at #300 the logic that clears style loaded callbacks added with mapboxMap.getStyle / mapboxMap.loadStyle was introduced.

However, for the getStyle the behaviour doesn't feel right. getStyle name doesn't suggest that it depends on the specific loadStyle call, instead it would be natural to return ANY style whenever it is loaded.

Existing getStyle implementation also leads to the issues (mentioned above) with our own LocationComponent. If location component was triggered before style load was started (possible if MapInitOptions.styleUri = null or style fails to load), it needs style and waits for it to appear at getStyle (in couple places, e.g. here and here. When new style is loaded afterwards - this callbacks of location component are being cleared so it's never initialized, if not triggered manually again after style loaded.

I've changed behaviour of mapboxMap.getStyle callbacks, now they are not cleared anymore after the loadStyle call, while callbacks loadStyle retain current behaviour.

User impact (optional)

Aside from fixing bugs with location puck - when user calls mapboxMap.getStyle they will receive style from the first style that is loaded successfully in future.

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.
Apply changelog label ('breaking change', 'bug 🪲', 'build', 'docs', 'feature 🍏', 'performance ⚡', 'testing 💯') or use the label 'skip changelog'
Add an entry inside this element for inclusion in the mapbox-maps-android changelog: <changelog>Fix location component now shown in certain cases.</changelog>.
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: #321, #1048

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

yunikkk · 2022-02-02T17:08:14Z

Tests coming

yunikkk · 2022-02-02T17:14:24Z

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

  internal var isStyleLoadInitiated = false
-  private val styleObserver = StyleObserver(this, nativeObserver, pixelRatio)
+  private val styleObserver = StyleObserver(


Removed this param from StyleObserver, using reference to the object not yet constructed fully could lead to subtle bugs. Instead only passed lambda that inits style directly.

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

kiryldz · 2022-02-03T11:19:10Z

@yunikkk label and changelog entry are missing, can you add those so CI will pass?

pengdev · 2022-02-03T11:55:00Z

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

@@ -102,9 +106,9 @@ class MapboxMap internal constructor(
  ) {
    initializeStyleLoad(onStyleLoaded, onMapLoadErrorListener)
    if (styleUri.isEmpty()) {
-      nativeMapWeakRef.call { (this as StyleManagerInterface).styleJSON = EMPTY_STYLE_JSON }
+      nativeMapWeakRef.call { styleJSON = EMPTY_STYLE_JSON }


Recommend to use this keyword to limit the scope of the function to be within nativeMap, so that not to be confused with property/functions with the same name in the MapboxMap class. It used to cause trouble since we have the exact same functions both inside the NativeMap and MapboxMap.

Suggested change

nativeMapWeakRef.call { styleJSON = EMPTY_STYLE_JSON }

nativeMapWeakRef.call { this.styleJSON = EMPTY_STYLE_JSON }

Could you explain a bit more? this. is implied anyway,
nativeMapWeakRef.call { someProperty ->
should be always the same as
nativeMapWeakRef.call { this.someProperty

we have had some occurrences where it was calling the function on the outer class versus on the native map. Not much of an issue here though

Ah, got it, it seems.
IMO for such situations when this. context might be confused and outer class may hold similar or same method - better to use explicit named references with let e.g. let { inner -> inner.smth } instead of apply or similar that changes this.

pengdev · 2022-02-03T13:12:00Z

@yunikkk could you verify if the PR fixes #321, #1048 with a snapshot build?

yunikkk · 2022-02-03T13:35:19Z

@yunikkk could you verify if the PR fixes #321, #1048 with a snapshot build?

Yes, will check

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

tobrun · 2022-02-04T16:31:26Z

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

@@ -102,9 +106,9 @@ class MapboxMap internal constructor(
  ) {
    initializeStyleLoad(onStyleLoaded, onMapLoadErrorListener)
    if (styleUri.isEmpty()) {
-      nativeMapWeakRef.call { (this as StyleManagerInterface).styleJSON = EMPTY_STYLE_JSON }
+      nativeMapWeakRef.call { styleJSON = EMPTY_STYLE_JSON }


we have had some occurrences where it was calling the function on the outer class versus on the native map. Not much of an issue here though

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

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

yunikkk · 2022-02-07T21:43:14Z

@yunikkk could you verify if the PR fixes #321, #1048 with a snapshot build?

I've verified that 1tap originated issue is resolved, and the compose actually is not (though it's reproduced only on the emulator).

yunikkk requested a review from a team February 2, 2022 17:07

yunikkk commented Feb 2, 2022

View reviewed changes

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

pengdev reviewed Feb 3, 2022

View reviewed changes

yunikkk added the bug 🪲 Something isn't working label Feb 4, 2022

yunikkk requested a review from pengdev February 4, 2022 15:29

yunikkk force-pushed the yds-fix-location-component-get-style branch 2 times, most recently from fac22c6 to 05fff7e Compare February 4, 2022 16:20

tobrun approved these changes Feb 4, 2022

View reviewed changes

Chaoba approved these changes Feb 7, 2022

View reviewed changes

kiryldz reviewed Feb 7, 2022

View reviewed changes

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

kiryldz reviewed Feb 7, 2022

View reviewed changes

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

yunikkk requested a review from kiryldz February 8, 2022 09:35

yunikkk force-pushed the yds-fix-location-component-get-style branch from 531808f to f7443cd Compare February 8, 2022 10:52

yunikkk added 5 commits February 8, 2022 13:39

Separate callbacks originating on getStyle and loadStyle requests.

6643d0b

Tests.

1a2811d

Update changelog.

6cf29e5

Update test to reflect new getStyle behaviour.

d1397bc

Add Deprecated to isFullyLoaded method.

660d052

yunikkk force-pushed the yds-fix-location-component-get-style branch from f7443cd to 660d052 Compare February 8, 2022 11:40

Remove unnecessary volatile.

38d0082

kiryldz approved these changes Feb 8, 2022

View reviewed changes

yunikkk merged commit 22d06c9 into main Feb 9, 2022

yunikkk deleted the yds-fix-location-component-get-style branch February 9, 2022 11:22

tyofemi added this to the V10.4 milestone Mar 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split callbacks originating on getStyle and loadStyle requests. #1114

Split callbacks originating on getStyle and loadStyle requests. #1114

yunikkk commented Feb 2, 2022 •

edited

Loading

yunikkk commented Feb 2, 2022

yunikkk Feb 2, 2022

kiryldz commented Feb 3, 2022

pengdev Feb 3, 2022

yunikkk Feb 3, 2022

tobrun Feb 4, 2022

yunikkk Feb 4, 2022

pengdev commented Feb 3, 2022

yunikkk commented Feb 3, 2022

tobrun Feb 4, 2022

yunikkk commented Feb 7, 2022

	nativeMapWeakRef.call { styleJSON = EMPTY_STYLE_JSON }
	nativeMapWeakRef.call { this.styleJSON = EMPTY_STYLE_JSON }

Split callbacks originating on getStyle and loadStyle requests. #1114

Split callbacks originating on getStyle and loadStyle requests. #1114

Conversation

yunikkk commented Feb 2, 2022 • edited Loading

Summary of changes

User impact (optional)

Pull request checklist:

yunikkk commented Feb 2, 2022

yunikkk Feb 2, 2022

Choose a reason for hiding this comment

kiryldz commented Feb 3, 2022

pengdev Feb 3, 2022

Choose a reason for hiding this comment

yunikkk Feb 3, 2022

Choose a reason for hiding this comment

tobrun Feb 4, 2022

Choose a reason for hiding this comment

yunikkk Feb 4, 2022

Choose a reason for hiding this comment

pengdev commented Feb 3, 2022

yunikkk commented Feb 3, 2022

tobrun Feb 4, 2022

Choose a reason for hiding this comment

yunikkk commented Feb 7, 2022

yunikkk commented Feb 2, 2022 •

edited

Loading