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

Move MapEvents on handling from BaseMapView to MapboxMap #339

Merged
merged 9 commits into from
May 12, 2021

Conversation

julianrex
Copy link
Contributor

@julianrex julianrex commented May 11, 2021

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

Pull request checklist:

  • Briefly describe the changes in this PR.
  • Write tests for all new functionality. If tests were not written, please explain why.
  • Add example if relevant. Updated
  • 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>Moved on()fromBaseMapViewtoMapboxMap.</changelog>.
  • Update the migration guide, API Docs, Markdown files - Readme, Developing, etc

Summary of changes

This PR changes the behavior of on(_:handler:) to handle map events and moves the function to MapboxMap.

on(...) has been replaced by

  • onNext(...) -> Cancelable
  • onEvery(...) -> Cancelable

Assuming there is no retain cycle that stops the MapView or Snapshotter from being deallocated, then unsubscribing will be handled on your behalf when the MapView is deallocated: The underlying listener will unsubscribe from the map, and the closure will be released.

If you do have a retain cycle or you need to stop listening, for example if you want to listen to 3 events then stop, then call cancel on the returned Cancelable object.

Here's an example of how the call-site changes:

mapView.on(.styleLoaded) { event in ... }

becomes

mapView.mapboxMap.onNext(.styleLoaded) { (event) in ... }

cc @mapbox/maps-android

@julianrex julianrex added the breaking change ⚠️ If your pull request introduces a breaking change and updates are required when version is published label May 11, 2021
Comment on lines +18 to +25
private var eventHandlers = WeakSet<MapEventHandler>()

deinit {
eventHandlers.allObjects.forEach {
$0.cancel()
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need a test for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is..sort of. See below.

Comment on lines +124 to +130
class WeakHandlerContainer {
weak var handler: MapEventHandler?

deinit {
handler?.cancel()
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@julianrex julianrex merged commit 07c3b57 into main May 12, 2021
@julianrex julianrex deleted the jrex/map-events-on branch May 12, 2021 16:30
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants