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

Use jni.hpp #4193

Merged
merged 6 commits into from
Mar 4, 2016
Merged

Use jni.hpp #4193

merged 6 commits into from
Mar 4, 2016

Conversation

jfirebaugh
Copy link
Contributor

Still working on the integration here, but the basics are in place.

Fixes #2225.
Fixes #2825.
Fixes #4043.
Fixes #3438.

👀 @zugaldia @bleege @tobrun

@jfirebaugh
Copy link
Contributor Author

Tested in the test app:

  • Basic panning and zooming
  • Toggling a custom layer
  • Creating an offline region
  • Listing offline regions

@jfirebaugh
Copy link
Contributor Author

The followup work here is to convert the NativeMapView and offline bindings to jni.hpp high-level wrappers and generally clean up that code. I'll ticket / PR that separately.

@jfirebaugh jfirebaugh force-pushed the jni branch 2 times, most recently from 797465f to 05a9fc3 Compare March 4, 2016 02:17
@zugaldia
Copy link
Member

zugaldia commented Mar 4, 2016

@jfirebaugh This. is. fantastic. Thank you very much for putting the effort into building this. My favorite part is the almost 2k lines that are removed with this PR so far, it speaks mountains to the major cleanup that this work represents.

I'm only starting to get familiar with jni.hpp but from reading the README and samples, it all makes sense so far overall.

One question regarding the lack of DeleteGlobalRef calls. I read in one of the samples that "The global references created during registration are purposefully leaked". I've read the linked post and I understand the rationale, I wonder if this could affect somehow the way we're building the higher level Android API. Should we be in the look out for leaks?

Related, I see that DeleteGlobalRef remains for the offline callbacks. Is this temporary? How could we tackle #4121 in the light of jni.hpp?

The followup work here is to convert the NativeMapView and offline bindings to jni.hpp high-level wrappers and generally clean up that code.

I'd really appreciate if in the process of doing this we could document the new logic to do mbgl <---> Android communication with jni.hpp. This has classically been, and I include myself here, one of the major difficulties that Java developers have found to contribute to this project.

@bleege
Copy link
Contributor

bleege commented Mar 4, 2016

I love that we're getting to clean up the JNI spiderweb. Thank you for doing this @jfirebaugh.

One request though, would it be possible to hold off merging this into master until after 4.0.0 ships? The reason being is that we're very close to shipping and this would be a large variable thrown into the mix at a late stage. Let's voice about this during scrum later today to weigh the pros / cons.

@jfirebaugh
Copy link
Contributor Author

I wonder if this could affect somehow the way we're building the higher level Android API. Should we be in the look out for leaks?

The upshot is that JNI_OnUnload is basically never called, and you pretty much have to give up and just permanently leak the global class references obtained via FindClass + NewGlobalRef. That's the approach this PR takes, in particular with the commit that removes JNI_OnUnload.

@bleege
Copy link
Contributor

bleege commented Mar 4, 2016

🙇 🙇

@kkaefer
Copy link
Member

kkaefer commented Mar 7, 2016

💯

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.

5 participants