Fix annotation update missing issue.

[Chaoba]

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

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 annotation update missing issue</changelog>.

Summary of changes

Some annotation updates could be ignored as we will remove the pending GeojsonSource update in the queue.
This pr remove this logic to guarantee all annotation update could be executed.

User impact (optional)

[kiryldz]

@Chaoba I don't think it's a good idea although I understand how this solves a problem.

Root cause of underlying issue is posting updates very frequently. It basically does not make any sense as we can't render let's say 100 times each 1 ms - we depend on vsync updates and render each 16 ms (on most screens).

If we remove the code here like you did - we will have issues in parsing large geojson files. E.g. user has set to parse enormous geojson_1 and while parsing is happening also queued 100 more. There is no sense to parse 99 of them because they will be basically overwritten by the most recent one and if we do not overwrite - it will result in huge delay of showing this geojson data on map.

My proposal here for you case would be introducing handler inside annotation plugin and queue updates there.

[Chaoba]

@Chaoba I don't think it's a good idea although I understand how this solves a problem.

Root cause of underlying issue is posting updates very frequently. It basically does not make any sense as we can't render let's say 100 times each 1 ms - we depend on vsync updates and render each 16 ms (on most screens).

If we remove the code here like you did - we will have issues in parsing large geojson files. E.g. user has set to parse enormous geojson_1 and while parsing is happening also queued 100 more. There is no sense to parse 99 of them because they will be basically overwritten by the most recent one and if we do not overwrite - it will result in huge delay of showing this geojson data on map.

My proposal here for you case would be introducing handler inside annotation plugin and queue updates there.

The root cause for this issue is that there are too many updates and the most recent update cancels the former update. Simply introducing a handler in the annotation plugin will not resolve this issue, because all the updates in this handler will return immediately as the parse works are handled in the handler of GeojsonSource.
For example, we have handler A in annotation plugin and handler B in GeojsonSource and we have 2 update jobs which will cost some time.
Handler A will process these 2 jobs immediately to push the update to Handler B and in B the first one will be canceled by the second one, the issue still exists.

[kiryldz]

@Chaoba I don't think it's a good idea although I understand how this solves a problem.
Root cause of underlying issue is posting updates very frequently. It basically does not make any sense as we can't render let's say 100 times each 1 ms - we depend on vsync updates and render each 16 ms (on most screens).
If we remove the code here like you did - we will have issues in parsing large geojson files. E.g. user has set to parse enormous geojson_1 and while parsing is happening also queued 100 more. There is no sense to parse 99 of them because they will be basically overwritten by the most recent one and if we do not overwrite - it will result in huge delay of showing this geojson data on map.
My proposal here for you case would be introducing handler inside annotation plugin and queue updates there.

The root cause for this issue is that there are too many updates and the most recent update cancels the former update. Simply introducing a handler in the annotation plugin will not resolve this issue, because all the updates in this handler will return immediately as the parse works are handled in the handler of GeojsonSource.
For example, we have handler A in annotation plugin and handler B in GeojsonSource and we have 2 update jobs which will cost some time.
Handler A will process these 2 jobs immediately to push the update to Handler B and in B the first one will be canceled by the second one, the issue still exists.

@Chaoba again, I fully understand your fix. All what I'm saying is that changing that logic in geojson in not correct. Yes, it will fix annotations but this approach is not correct as we apply map updates asap and not queue them up.
I'm looking at code here:

mapbox-maps-android/plugin-annotation/src/main/java/com/mapbox/maps/plugin/annotation/AnnotationManagerImpl.kt

Line 365 in d95bef4

geoJsonSource.featureCollection(FeatureCollection.fromFeatures(features), onDataParsed = {})

Why can't we add that logic here? Not updating next feature until previous one is parsed.

[kiryldz]

is it intended to leave this large number here? or were you just testing? 😄
perhaps better having an integration test appying 2000 updates and verifying that all calls where executed (and not were skipped).

[kiryldz]

btw I guess it could be static, no need to create each time, wdyt?

[Chaoba]

Yes, this one could be static.

[kiryldz]

Could we revert lazy initialization? That thread in some situations may not even be started (if user does not use geojson source). I'm OK to leave it internal for tests.

[Chaoba]

This is a compromise for tests. With lazy initialization, only the first case will succeed, in the following one, this thread will be in the inactive state.

[kiryldz]

LGTM with couple small questions.