feature/android legacy - Githubissues

wdtinc / mapbox-vector-tile-java

Java Mapbox Vector Tile Library for Encoding/Decoding

Apache License 2.0

147 stars 73 forks source link

feature/android legacy #14

Closed snodnipper closed 6 years ago

snodnipper commented 6 years ago

Support legacy Android (tested on API 21 / Android 5.0 / Lollipop)

remove putIfAbsent references
replace Path with File references for Android compatibility
improve some Javadoc
add findbugs suggestions (random, float tileSize, parseLong etc.)

If you'd prefer this PR clean then I can remove the Javadoc / Findbugs changes

boldtrn commented 6 years ago

What is the state of this PR? What are the plans of merging (or not?) this?

snodnipper commented 6 years ago

Hopefully @ShibaBandit can find some time to review the code.

@boldtrn I published uk.os.vt:vt-legacy-parser:2.0.7 to maven central, which might be of interest in the short-term.

ShibaBandit commented 6 years ago

I'll try to look into this today.

boldtrn commented 6 years ago

Thank you, this would be highly appreciated.

uk.os.vt:vt-legacy-parser:2.0.7

In the short term I switched to this repo, thanks.

ShibaBandit commented 6 years ago

@snodnipper I've reviewed the changes and added some comments. Whenever you get a chance to review the comments we can proceed forward. Sorry I took way too long to get back to you on this pull request, got distracted with some other tasks.

boldtrn commented 6 years ago

Thanks for proceeding so fast here and for providing such a great library :+1:

ShibaBandit commented 6 years ago

I think things left to do before merge are:

Revert+Replace the 2 changes that removed Collections.emptyList() in the guard areas of methods and add the 'return' keyword in front. These were picked up as dead code, which is technically correct but it's actually a bug in this case and they need to be returned.
Revert line-length-only changes in code / comments.

devemux86 commented 6 years ago

What will be the allowed min Android API Level after the merged changes, i.e. could it be like 15?

snodnipper commented 6 years ago

@devemux86 I don't believe this will achieve API Level 15 - it looks like we have some uses of the static methods in java.util.Objects, which are available above API 19 (Android 4.4.+).

Perhaps we get this in and discuss pushing it lower?

https://github.com/wdtinc/mapbox-vector-tile-java/blob/933fa04e2cc5629bfc1b99582da8ab170535b702/src/main/java/com/wdtinc/mapbox_vector_tile/adapt/jts/TileGeomResult.java#L30 https://github.com/wdtinc/mapbox-vector-tile-java/blob/933fa04e2cc5629bfc1b99582da8ab170535b702/src/main/java/com/wdtinc/mapbox_vector_tile/adapt/jts/UserDataKeyValueMapConverter.java#L39

devemux86 commented 6 years ago

Perhaps we get this in and discuss pushing it lower?

Sure, that would be good!

Wondering about protobuf-java dependency on Android too, are its versions after 2 compiled with Java 8?

snodnipper commented 6 years ago

@devemux86 it looks like Google compile at level 1.6, which is what most core libraries still do. It looks like we can bump the protobuf-java version at some point.

https://github.com/google/protobuf/blob/master/java/pom.xml#L99

snodnipper commented 6 years ago

@ShibaBandit I have updated the code to remove new lines etc. The only Javadoc warnings I get are in MvtValue, which could be done as a separate PR.

Ping me if there is more that you'd like 👍

devemux86 commented 6 years ago

Will the library compile configuration change to e.g. 1.7 with the changes?

snodnipper commented 6 years ago

@devemux86 I don't see why it should - you can build Java 1.8 just fine with the latest Android build tools. I only expect problems with unsupported methods / classes on old Android API levels...syntax should be ok.

ShibaBandit commented 6 years ago

As @snodnipper said, I think the language level itself is probably fine as long as the code is supported.