Skip to content

Conversation

@Chaoba
Copy link
Contributor

@Chaoba Chaoba commented Sep 15, 2021

PRs must be submitted under the terms of our Contributor License Agreement CLA.
Fixes: #634
Fixes: #583

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.
  • 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 getStyle returns null after adding a source</changelog>.

Summary of changes

onStyleLoaded will only be triggered once for one style, so if users add a new source to style after style loaded, onStyleLoaded will never be invoked. And during the loading time for this new source, getStyle will return null because style.isStyleLoaded will be false.
This pr add a property styleLoaded to record wheter the style is loaded or not no matter how many sources or layers are added after style loaded.

User impact (optional)

@Chaoba Chaoba added the bug 🪲 Something isn't working label Sep 15, 2021
@Chaoba Chaoba requested a review from a team September 15, 2021 10:23
@Chaoba Chaoba self-assigned this Sep 15, 2021
internal var isStyleLoadInitiated = false
private val styleObserver = StyleObserver(this, nativeMapWeakRef, nativeObserver, pixelRatio)
internal var renderHandler: Handler? = null
internal var styleLoaded = false
Copy link
Contributor

@alexander-kulikovskii alexander-kulikovskii Sep 15, 2021

Choose a reason for hiding this comment

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

should it be with private setter?
because in Java internal converted into public

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need set it in test cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe use annotation VisibleForTesting ?

@kiryldz
Copy link
Contributor

kiryldz commented Sep 15, 2021

@Chaoba please link an issue that gets fixed by this PR.
Also we should have a new test for it that was failing before but should pass now.

@pengdev
Copy link
Member

pengdev commented Sep 15, 2021

This PR reverts @tobrun's PR in #332, where we used native isStyleLoaded instead of the platform side cached value. Would be good to get a clear understanding of the issue we experienced and check with gl-native team if we want to modify the behaviour of isStyleLoaded on the native side.

@pengdev
Copy link
Member

pengdev commented Sep 16, 2021

Could be related to #614 as well

@pengdev
Copy link
Member

pengdev commented Sep 16, 2021

@Chaoba could you build a snapshot using this branch?

publish_android_snapshot
@tobrun
Copy link
Member

tobrun commented Sep 17, 2021

check with gl-native team if we want to modify the behaviour of isStyleLoaded on the native side.

This was already confirmed in the annotation related ticket that it's expected to return false when something new is loading. This is conform to gl-js implemenation. For our own internal style logic, we need to roll our own setup as proposed here.

@kiryldz kiryldz merged commit a126bf6 into main Sep 21, 2021
@kiryldz kiryldz deleted the kl-getstyle branch September 21, 2021 06:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug 🪲 Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MapboxMap#getStyle returns null after adding a source Cannot access map style if source fails to load

5 participants