Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Replace OnMapChange with specific callbacks #13050

Merged
merged 1 commit into from
Oct 11, 2018
Merged

Replace OnMapChange with specific callbacks #13050

merged 1 commit into from
Oct 11, 2018

Conversation

tobrun
Copy link
Member

@tobrun tobrun commented Oct 8, 2018

PR introduces separate callbacks for all the map change events instead of wiring them through the same OnMapChange callback. This avoids iterating all registered listener and limits the invocations of those callbacks for non-related events.

This PR also aligns naming of callbacks to match core naming more closely:

old: region is changing vs new: camera is changing

Todo:

  • integrate native_map_view.cpp changes
  • backwards compatibility with old OnMapChange API
  • add callback definition on java side
  • callback integration (add/remove)
  • deprecate notice
  • add tests

Replaces obsolete closed PR in #9498.

@tobrun tobrun added the Android Mapbox Maps SDK for Android label Oct 8, 2018
@tobrun tobrun self-assigned this Oct 8, 2018
@tobrun tobrun added this to the android-v6.7.0 milestone Oct 8, 2018
@tobrun
Copy link
Member Author

tobrun commented Oct 9, 2018

@LukasPaczos this PR is ready for review:

  • removing the deprecated internal usage of MapChangeListener will be done in a separate PR
  • need to ticket out semver ticket removing MapChangeListener

@tobrun tobrun force-pushed the tvn-map-change branch 2 times, most recently from 89fc4fc to 492a64d Compare October 10, 2018 10:21
Copy link
Member

@LukasPaczos LukasPaczos left a comment

Choose a reason for hiding this comment

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

LGTM! Only changes requested are fixups for a couple of refactor artifacts.

@@ -159,7 +159,7 @@ void onRestoreInstanceState(@NonNull Bundle savedInstanceState) {
}

/**
* Called when the hosting Activity/Fragment onDestroy()/onDestroyView() method is called.
* Called when the hosting Activity/Fragment clear()/onDestroyView() method is called.
Copy link
Member

Choose a reason for hiding this comment

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

Refactor artifact.

@@ -116,7 +122,7 @@ private boolean checkState(String callingMethod) {
// validate if map has already been destroyed
if (destroyed && !TextUtils.isEmpty(callingMethod)) {
String message = String.format(
"You're calling `%s` after the `MapView` was destroyed, were you invoking it after `onDestroy()`?",
"You're calling `%s` after the `MapView` was destroyed, were you invoking it after `clear()`?",
Copy link
Member

Choose a reason for hiding this comment

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

Refactor artifact.

@@ -417,11 +422,12 @@ public void onStop() {
}

/**
* You must call this method from the parent's Activity#onDestroy() or Fragment#onDestroyView().
* You must call this method from the parent's Activity#clear() or Fragment#onDestroyView().
Copy link
Member

Choose a reason for hiding this comment

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

Refactor artifact.

@tobrun
Copy link
Member Author

tobrun commented Oct 11, 2018

double checked refactor artefacts and should be addressed with 80ec30d

Copy link
Member

@LukasPaczos LukasPaczos left a comment

Choose a reason for hiding this comment

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

🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants