Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Merging android-geojson #381

Merged
merged 6 commits into from

4 participants

Brad Leege Emux Tom MacWright Jonathan Baker
Brad Leege
Admin

#205 - Merging into master for better testing / usage.

Brad Leege bleege merged commit 1432b2c into from
Brad Leege bleege deleted the branch
Emux

Hi Brad,

Some comments for the latest GeoJSON changes.

  • Now if someone wants to use a local geojson file, he has to use MapView.loadFromGeoJSONString and then implement himself the transformations to ui objects, as this code exists internal at LoadAndDisplay.doInBackground.
    So it's not possible to use the convenient polygon inner rings re-wind, which is available with GeoJSONPainter class, usable only with online geojson.

  • At GeoJSONPainter constructor a Marker parameter would be preferable, to able to use local drawables also, besides online icons.

  • It would also be convenient if the 'title' property was parsed again (if exists) in order for the generated markers to have a title.

Emux
Cruiser - Atlas

Tom MacWright
Admin

Cool, let's see how this goes. I wasn't totally sure that the geojson parser was worth it, since it's yet-another intermediary format & parser, and punted on stuff like winding order. Mainly I am, like Emux, not totally comfortable with GeoJSON parsing being only available through an async & remote interface off of GeoJSONPainter - this seems like it should be a separate method that's shared.

Brad Leege
Admin

Great feedback guys. I totally agree about making standalone access to the loading of GeoJSON. I originally was just bolting the geojson-android library as is into the existing source code to see how it went / kick the tires. I've created a new issue (#382) for these enhancements. Please feel free to add / edit the list.

FWIW, I'm going to keep the same package structure com.cocoahero.android.geojson in the Mapbox Android SDK source code for the time being. If we like what it's doing we can then more easily separate it into it's own project and / or contribute it back to the original project.

Jonathan Baker

Wow, needless to say I was surprised to see my code being merged into this SDK. I'm definitely :+1: for keeping the package structure and contributing back to my original project for now. Alternatively, I'm looking into making the library available via maven for gradle builds which should make this integration much easier and less copy and pasted.

Jonathan Baker

Also, I'm a total legalese / licensing n00b, and please don't take this as me being greedy, but doesn't the MIT license require that attribution be given when using a given library or source?

Tom MacWright
Admin

Yeah - we need to have a copy of android-geojson's license in this repo.

Brad Leege
Admin

@cocoahero I hope it's "Surprised, in a good way" vs "Surprised, in a bad / angry way". My hunch is that it's the former, but just wanted to be sure. Sometimes it's hard to tell in text. :-) Regardless we definitely want to be good citizens / do the right thing so please keep speaking up if we start straying from the path (the license comment is a perfect example of this).

As for integration, I'd personally much prefer to be able to pull in android-geojson as a library / dependency rather than direct source embed. I forked it on my personal GH awhile back to start the process of getting it into a state to be able to do this, but paused it while we kicked the tires on it in Mapbox SDK to see if it'd work for our needs. As mentioned in my issue on the android-geojson if it's available via Maven Central that'd be great. We're happy to help you get that setup.

Finally, thanks for creating your library. It's super helpful and I look forward to helping it improve it over time!

https://github.com/bleege/android-geojson

http://github.com/cocoahero/android-geojson/issues/1

Tom MacWright tmcw referenced this pull request from a commit
Tom MacWright tmcw Note android-geojson license. Refs #381 dc9232d
Tom MacWright
Admin

Added the license & attribution for android-geojson to LICENSE.md: https://github.com/mapbox/mapbox-android-sdk/blob/master/LICENSE.md

Jonathan Baker

@bleege It is most definitely in a good way. It's very humbling and exciting to know that a library that I wrote in boredom is being included in a large project such as this =)

Brad Leege
Admin

Awesome @cocoahero ! Your work saved a lot of time on our end. Thanks for letting us use it!

@tmcw Thanks for getting the license updated so quickly!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.