GeoJson plugin #38
Closed
GeoJson plugin #38
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
create demo activity of GeoJson Loader plugin.
tobrun
requested changes
May 29, 2017
There was a problem hiding this comment.
I reviewed a part of the PR, most comments are related to codestyle, missing javadoc etc.
One thing I would like to see differently is that you are constantly using a new GeoJsonPluginBuilder. This is not ideal if a user want to constantly update their GeoJson. It would make more sense building it once and allowing to update the underlying sources direclty on the plugin. This will be faster and will result in generating less garbage.
| public void onMapReady(MapboxMap mapboxMap) { | ||
| this.mapboxMap = mapboxMap; | ||
| mapboxMap.moveCamera(CameraUpdateFactory.newLatLngZoom(new LatLng(32.6546, 51.6680), 7)); | ||
|
|
| } | ||
| } | ||
| }) | ||
| .draw(); |
There was a problem hiding this comment.
a builder pattern normally ends with calling a method with using the build() method signature
| } | ||
| }) | ||
| .draw(); | ||
| requestExternalStoragePermission(); |
There was a problem hiding this comment.
Not sure if I understand requesting the external storage permissions after you have read from permissions?
There was a problem hiding this comment.
may I use https://github.com/afollestad/material-dialogs#file-selector-dialogs for selecting GeoJson file from external storage?
| if (checkSelfPermission(android.Manifest.permission.WRITE_EXTERNAL_STORAGE) | ||
| == PackageManager.PERMISSION_GRANTED) { | ||
| Log.v(TAG, "Permission is granted"); | ||
| //drawFromPath(); |
| } | ||
| } else { //permission is automatically granted on sdk<23 upon installation | ||
| Log.v(TAG, "Permission is granted"); | ||
| //drawFromPath(); |
| @@ -0,0 +1,12 @@ | |||
| package com.mapbox.mapboxsdk.plugins.geojson.listener; | |||
|
|
|||
| public interface OnLoadingGeoJsonListener { | ||
| void onPreLoading(); | ||
|
|
||
| void onLoaded(); |
| package com.mapbox.mapboxsdk.plugins.geojson.listener; | ||
|
|
||
| public interface OnLoadingGeoJsonListener { | ||
| void onPreLoading(); |
| import org.json.JSONObject; | ||
|
|
||
| public interface OnMarkerEventListener { | ||
| /** |
| import com.mapbox.mapboxsdk.annotations.Marker; | ||
|
|
||
| import org.json.JSONObject; | ||
|
|
@tobrun |
zugaldia
reviewed
May 30, 2017
| LatLngBounds.Builder builder = new LatLngBounds.Builder(); | ||
| try { | ||
| JSONObject json = new JSONObject(geoJson); | ||
| JSONArray features = json.getJSONArray("features"); |
There was a problem hiding this comment.
We have an open source library that handles GeoJson transparently without the need to use the lower level JSONObject classes. Any reason not to use for the plugin? It comes includes by default with the Maps SDK so you should be able to use it without having to add any external dependencies.
There was a problem hiding this comment.
I didn't know about this GeoJson handler. I just use example of this link
https://www.mapbox.com/android-sdk/examples/geojson/
Will be replacing.
Thanks for your review.
There was a problem hiding this comment.
@bkhezry Thank you! We'll definitely update that example to use the library I was mentioning, good catch.
replace Log with Timber. add setUrl, setAssetsName & setFilePath functions to the GeoJsonPlugin. and fixed other reviews.
|
almost of review have been fixed.
with multiple Polygon or Polyline in GeoJson file, selecting random color may be useful.
manual parsing GeoJson string replaced with SDK of Mapbox. |
|
Hey @bkhezry, sorry this plugin is taking some time to review, we're sprinting to finalize v5.1.0 of the SDK and this is taking all our resources (release candidate launched last week mapbox/mapbox-gl-native#9231 🎉 ). Thank you for your patience! |
|
Hi @zugaldia, |
|
@bkhezry changes looking great! will work on putting the finishing touches on this so we can get this merged. Great job! Thank you for the contribution. |
…ugins-android into bkhezry-geojson-plugin
tobrun
approved these changes
Jul 19, 2017
|
@bkhezry things are looking good for a first release, can you resolve the conflicting files? |
# Conflicts: # plugins/app/build.gradle # plugins/app/src/main/AndroidManifest.xml # plugins/app/src/main/res/values/strings.xml # plugins/build.gradle # plugins/settings.gradle
…eojson-plugin # Conflicts: # plugins/app/build.gradle # plugins/app/src/main/AndroidManifest.xml # plugins/build.gradle # plugins/settings.gradle
…eojson-plugin # Conflicts: # plugins/app/build.gradle # plugins/app/src/main/AndroidManifest.xml # plugins/build.gradle # plugins/settings.gradle
|
Hi @tobrun |
|
@bkhezry our CI build is failing on checkstyle: Here is more information how to setup checkstyle cfr Mapbox checkstyle. |
|
@tobrun checkstyle errors fixed. sorry about that. |
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
#37
url,assetsorpath.OnLoadingGeoJsonListenerfor handlepreload,loadedandloadFailedevent of GeoJson loading.onMarkerClickListenerfor handle marker click event and returnJSONObjectof pointproperties.