Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure AnnotationManager's mapLoaded happens first #246

Merged
merged 5 commits into from
Apr 1, 2021

Conversation

macdrevx
Copy link
Contributor

@macdrevx macdrevx commented Apr 1, 2021

Pull request checklist:

  • Briefly describe the changes in this PR.
  • Write tests for all new functionality. If tests were not written, please explain why.
    • This does not test that Annotation manager is created during MapView initialization. If that were to change, this could break again. Testing it would require significant refactoring that is beyond the scope of this fix.
  • 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-ios changelog: <changelog></changelog>. Added directly to changelog in PR
  • Update the migration guide, API Docs, Markdown files - Readme, Developing, etc

Summary of changes

Previously, AnnotationManager subscribed to .mapLoaded using
MapboxCoreMaps directly; however, MapboxCoreMaps does not guarantee
that observers will be notified in the order in which they subscribe.

Among other things, this could result in annotations not being selected when they are tapped.

This fix updates AnnotationManager to subscribe via BaseMapView.on
which does guarantee notification ordering among its subscribers.

Breaking changes:

  • AnnotationManager no longer conforms to Observer and no longer has a peer
  • AnnotationSupportableMap is now internal

Previously, AnnotationManager subscribed to .mapLoaded using
MapboxCoreMaps directly; however, MapboxCoreMaps does not guarantee
that observers will be notified in the order in which they subscribe.

This fix updates AnnotationManager to subscribe via BaseMapView.on
which does guarantee notification ordering among its subscribers.
@macdrevx macdrevx requested a review from julianrex April 1, 2021 17:36
@macdrevx macdrevx added breaking change ⚠️ If your pull request introduces a breaking change and updates are required when version is published bug 🪲 Something is broken! labels Apr 1, 2021
@macdrevx macdrevx marked this pull request as ready for review April 1, 2021 17:58
@macdrevx macdrevx merged commit c10c571 into main Apr 1, 2021
@macdrevx macdrevx deleted the ah/annotation-manager-subscribe-fix branch April 1, 2021 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change ⚠️ If your pull request introduces a breaking change and updates are required when version is published bug 🪲 Something is broken!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants