Call plugin onStart after start loading style

[axti]

PRs must be submitted under the terms of our Contributor License Agreement CLA.
Fixes: < Link to related issues that will be fixed by this pull request, if they exist >

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 initialisation location puck when no style loaded from code by changing Plugin#onStart() call after style loaded</changelog>.

Summary of changes

In this PR has been changed order of methods called on lifecycle event onStart. Now plugins will receive onStart after style has been loaded. This reorder prevent of loosing callbacks from StyleObserver when callback were added before style starts loading.

User impact (optional)

User with styleUri attribute in XML will have all plugins started correctly after style has been loaded. Before in this case it was the issue reported here: #641

[tobrun]

On internal convo, you mentioned:

What if we will change here:
    pluginRegistry.onStart()
    if (!mapboxMap.isStyleLoadInitiated) {
      // Load the style in mapInitOptions if no style has loaded yet.
      mapInitOptions.styleUri?.let {
        mapboxMap.loadStyleUri(it)
      }
    }
to do like this:
    if (!mapboxMap.isStyleLoadInitiated) {
      // Load the style in mapInitOptions if no style has loaded yet.
      mapInitOptions.styleUri?.let {
        mapboxMap.loadStyleUri(it)
        pluginRegistry.onStart()
      }
    }

Proposed change here is actually not part of the style loading callback?

[pengdev]

I think we should call onStart inside the Activity's onStart event, regardless of the style loading status.
and I remember we do have a onStyleChanged listener, we could hook to this listener inside the plugin if the style is need for certain action, wdyt?

[axti]

There was no callback, I assume it is loading synchronously. If style is null plugins wouldn’t work.

[axti]

@pengdev you are right, this onStart is initially called from Activity/Fragment and implementation is inside MapController, It is entry point.

[pengdev]

Proposed change here is actually not part of the style loading callback?

@tobrun I think the point of this change is that the new style load will clear the style load listeners that previous getStyle() { style -> } sets.
In some plugins we rely on the async getStyle() call to do the initialisation in the plugin's onStart(). And when the new style is loaded after the plugin's onStart, it will result in the plugins' getStyle() callback never being fired.
This fix intentionally moved the pluginRegistry.onStart() after the loadStyleUri call, so that we made sure the styleLoaded listener set by plugins' getStyle() is not cleared.

[pengdev]

Great work @axti , LGTM, please add tests as well 🚀

[pengdev]

can we use onStart directly instead of using simulateStartState?

[kiryldz]

+1 for that and remove simulateStartState at all. imho there's no possibility in reality to set lifecycleState = LifecycleState.STATE_STARTED and not call onStart at the same time

[axti]

I disagree with relaxed, I'm trying to follow Arrange-Action-Assert pattern to write a good tests.

[kiryldz]

I disagree with relaxed, I'm trying to follow Arrange-Action-Assert pattern to write a good tests.

Is this comment related to #680 (comment) actually?

[pengdev]

nit: we can use relaxed mockk to avoid adding every for these functions.