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

Deprecate MapChange and provide dedicated callback methods #9498

Closed
wants to merge 1 commit into from

Conversation

tobrun
Copy link
Member

@tobrun tobrun commented Jul 13, 2017

This PR deprecates using OnMapChangeListeners and replaces them with dedicated callbacks.

  • jni bindings
  • public api for dedicated callbacks
  • javadoc on public API
  • refactor internal use of MapChange with dedicated callbacks
  • deprecate old setup (OnMapChangeListener)
  • optimise onMapFailedLoaded with the thrown error message
  • optimise onSourceChanged callback to provide source/attribution that has changed
  • optimise "partial" vs "full" callbacks (expose same as core in terms of amount of events)
  • add tests
  • update test app

Proposed API:

    void onCameraWillChange(boolean animated);
    void onCameraIsChanging();
    void onCameraDidChange(boolean animated);

    void onWillStartLoadingMap();
    void onDidFinishLoadingMap();
    void onDidFailLoadingMap(String errorMessage);

    void onWillStartRenderingFrame();
    void onDidFinishRenderingFrame(boolean partial);

    void onWillStartRenderingMap();
    void onDidFinishRenderingMap(boolean partial);

    void onDidFinishLoadingStyle();

    void onSourceChangedListener(String id);

Closes #8389 / refs #9476

@tobrun tobrun added the Android Mapbox Maps SDK for Android label Jul 13, 2017
@tobrun tobrun added this to the android-v5.2.0 milestone Jul 13, 2017
@tobrun tobrun self-assigned this Jul 13, 2017
@tobrun tobrun force-pushed the 8389-deprecate-notifyMapChange branch 4 times, most recently from 17a1012 to b28423d Compare July 18, 2017 10:31
@tobrun tobrun requested a review from ivovandongen July 18, 2017 10:32
Copy link
Contributor

@ivovandongen ivovandongen left a comment

Choose a reason for hiding this comment

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

@tobrun I did a first review. There are a couple of issues that need to be addressed. Sorry for the "From mbgl::RendererBackend" comment in the header. This is misleading as it is not true anymore... :(

@@ -184,21 +184,32 @@ void onPostMapReady() {
/**
* Called when the region is changing or has changed.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this javadoc still correct?

@@ -174,6 +175,9 @@ private void initialise(@NonNull final Context context, @NonNull final MapboxMap
// notify Map object about current connectivity state
nativeMapView.setReachability(ConnectivityReceiver.instance(context).isConnected(context));

// bind internal components for map change events
mapChangeDispatcher.bind(mapCallback = new MapChangeResultHandler(mapboxMap));
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it somehow be feasible to make this a constructor argument? Since it is required. That would make it possible to skip a lot of null checks in MapChangeDispatcher

Copy link
Member Author

Choose a reason for hiding this comment

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

Been looking into it but don't think so at the moment though this clearly shows we need to rethink our architecture. The issue here is a cyclic class dependency: NativeMapview depends on MapChangeDispatcher, MapboxMap depends on NativeMapView and MapChangeDispatcher depends on MapboxMap.

Somewhere during initialization we need to bind them together and require us to have null checking. Current setup follows the requirement of being able to listen to map change events as soon as possible. I don't see an a issue with internal components missing out on the initialization change events.

@@ -54,14 +54,14 @@
namespace mbgl {
namespace android {

NativeMapView::NativeMapView(jni::JNIEnv& _env,
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you revert indentation changes?

: javaPeer(_obj.NewWeakGlobalRef(_env)),
pixelRatio(_pixelRatio),
threadPool(sharedThreadPool()) {
: javaPeer(_obj.NewWeakGlobalRef(_env)),
Copy link
Contributor

Choose a reason for hiding this comment

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

dito 3x

@@ -112,7 +112,7 @@ void NativeMapView::bind() {
/**
* From mbgl::RendererBackend.
*/
gl::ProcAddress NativeMapView::initializeExtension(const char* name) {
gl::ProcAddress NativeMapView::initializeExtension(const char *name) {
Copy link
Contributor

Choose a reason for hiding this comment

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

dito

*
* May be called from any thread
*/
void NativeMapView::notifyMapChange(mbgl::MapChange change) {
Copy link
Contributor

Choose a reason for hiding this comment

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

void NativeMapView::onDidFailLoadingMap(std::exception_ptr eptr) {
assert(vm != nullptr);
try {
std::rethrow_exception(eptr);
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 want to throw an exception here? Seems like we always want to invoke the listener. If re-throwing is required, it should happen where the exception is produced.

Copy link
Member Author

Choose a reason for hiding this comment

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

rethrowing was a way of getting the string message for the listener, let me try a different approach.

Copy link
Member Author

Choose a reason for hiding this comment

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

the exception is produced in core in style_impl.cpp and passed to the MapObserver, which invokes the method above. Don't think I can move re-throwing up the chain as it would impact other bindings. The code for retrieving the exception message was based of http://en.cppreference.com/w/cpp/error/exception_ptr.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, you're right. That's a valid way to do it. Should've read more carefully.

javaPeer->Call(*_env, onInvalidate);
}

/**
* From mbgl::RendererBackend. Callback to java NativeMapView#onMapChanged(int).
* From mbgl::RendererBackend. Callback to java NativeMapView#onCameraWillChange().
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually. This is from MapObserver and will only be coming from the main (map) thread. Could you update the comment in the header file as well?

void NativeMapView::onCameraIsChanging() {
notifyMapChange(MapChange::MapChangeRegionIsChanging);
assert(vm != nullptr);
android::UniqueEnv _env = android::AttachEnv();
Copy link
Contributor

Choose a reason for hiding this comment

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

As this is not actually coming from any other thread then the main/map thread, this is not needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed it to something as:

void NativeMapView::onDidFinishLoadingStyle() {
    JNIEnv* env = nullptr;
    int resultCode = vm->GetEnv(reinterpret_cast<void**>(&env), JNI_VERSION_1_6);
    if(resultCode == JNI_OK) {
        static auto onDidFinishLoadingStyle = javaClass.GetMethod<void()>(*env, "onDidFinishLoadingStyle");
        javaPeer->Call(*env, onDidFinishLoadingStyle);
    }
}

lmk if there is a better way of getting the env

_createSurface(ANativeWindow_fromSurface(&env, jni::Unwrap(*_surface)));
}

void NativeMapView::destroySurface(jni::JNIEnv&) {
Copy link
Contributor

Choose a reason for hiding this comment

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

more formatting changes

@tobrun tobrun force-pushed the 8389-deprecate-notifyMapChange branch 2 times, most recently from 3bf83d2 to 6122934 Compare July 19, 2017 12:15
@tobrun tobrun force-pushed the 8389-deprecate-notifyMapChange branch from 6122934 to 24b95fc Compare October 12, 2017 20:51
@tobrun tobrun changed the base branch from master to release-agua October 12, 2017 20:52
@tobrun tobrun force-pushed the 8389-deprecate-notifyMapChange branch 2 times, most recently from 8dea25d to be86668 Compare October 13, 2017 11:11
@tobrun
Copy link
Member Author

tobrun commented Oct 13, 2017

I redid this PR in light of moving to GLSurfaceView (reason this PR was on hold).
A couple of changes since previous iteration:

  • we allow multiple listeners to be registered (this was required for plugin development, else a user or a plugin could override an already set listener).
  • usage of AttachEnv to get a reference to the jni environment

@tobrun tobrun requested a review from zugaldia October 17, 2017 15:21
Copy link
Contributor

@ivovandongen ivovandongen left a comment

Choose a reason for hiding this comment

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

Looks good. The only thing is that I found the naming of MapChangeHandler and MapChangeDispatcher a little confusing while reading the PR. But since neither is public that will probably not be a problem.

void NativeMapView::onDidFailLoadingMap(std::exception_ptr eptr) {
try {
if(eptr) {
std::rethrow_exception(eptr);
Copy link
Contributor

Choose a reason for hiding this comment

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

@tobrun tobrun force-pushed the 8389-deprecate-notifyMapChange branch from be86668 to 9e9f999 Compare October 26, 2017 15:37
@tobrun
Copy link
Member Author

tobrun commented Oct 26, 2017

Thank you for the review @ivovandongen,
re using toString for the exception ptr, this has been fixed.
re naming: I merged the concepts of MapChangeDispatcher and MapChangeHandler into MapChangeEventManager, feel free propose any changes to the name.

@tobrun tobrun force-pushed the 8389-deprecate-notifyMapChange branch from 9e9f999 to 8847b41 Compare October 26, 2017 15:53
@lilykaiser lilykaiser removed this from the android-v5.2.0 milestone Nov 16, 2017
@zugaldia zugaldia requested review from LukasPaczos and removed request for zugaldia December 23, 2017 17:46
@tobrun tobrun closed this May 8, 2018
@tobrun
Copy link
Member Author

tobrun commented May 8, 2018

stale

@tobrun tobrun deleted the 8389-deprecate-notifyMapChange branch July 23, 2018 12:32
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.

3 participants