wdtinc / mapbox-vector-tile-java

Java Mapbox Vector Tile Library for Encoding/Decoding
Apache License 2.0
147 stars 73 forks source link

Android API Level 15 compatibility #17

Closed boldtrn closed 6 years ago

boldtrn commented 6 years ago

As discussed in #14, it would be great to support Android versions down to API level 15, if possible, as mentioned by @devemux86.

According to @snodnipper, it looks like we have some uses of the static methods in java.util.Objects, which are available above API 19 (Android 4.4.+).

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

I am not sure what else needs to be done and if these method calls can be easily converted to Android API level 15 compatibility? If it's just these Objects.requireNonNull calls, I can create a PR adding a Util Method like:

requireNonNull(Object obj)
   if(obj == null)
      throw new NullPointException();
snodnipper commented 6 years ago

I'll try and take a look this weekend. It actually looked quite hard to even get a legacy API 15 emulator working earlier in the latest Android Studio (macOS).

If you are set up to go then you are most welcome to investigate 👍

boldtrn commented 6 years ago

I don't really have a well setup Android dev environment, sorry, so I cannot really test different version etc. :).

boldtrn commented 6 years ago

I was able to run this library using Android Emulator on API level 16 (Android 4.1.2). Using my setup I cannot start devices with API level 15.

BTW I used the dependency api 'uk.os.vt:vt-legacy-parser:2.0.7' for this test.

devemux86 commented 6 years ago

I tested latest master with our VTM map library on Android emulator API 16 and all seem to work! A bit strange since there are API 19 calls in code or they're in non accessible places?

Could not test on API 15, since Android emulator doesn't work ok most of time. Also Genymotion offers VMs with min API 16.

snodnipper commented 6 years ago

^ I suspect all will be fine until you reach one of the code paths touching anything unavailable. I tried writing some tiles yesterday against API 16 and ran into the issues.

devemux86 commented 6 years ago

I tried writing some tiles

My tests were only for decoding MVT tiles, not any encoding.

boldtrn commented 6 years ago

I suspect all will be fine until you reach one of the code paths touching anything unavailable.

This would be pretty unfortunate. But it also makes sense. AFAIK Android allows you to check the SDK version and only enter a code block if the current SDK has at least a certain level. Therefore Android has to compile code even if it's not runnable on the SDK version.

Is there a good way to find method calls that are not compatible with the current SDK version?

ShibaBandit commented 6 years ago

You could try running the unit tests on your android device or emulator?

boldtrn commented 6 years ago

You could try running the unit tests on your android device or emulator?

Yes this would be probably possible, but I assume the tests don't have 100% coverage, so we don't really know if the code might crash at some point? Running the tests would definitely, but it's a pity that there is static analysis that will just tell you. I guess our best guess at the moment would be to convert your library to an Android project, setting the SDK version on it, because then, Android Studio will tell you if you use methods that are not suitable for the chosen SDK version.

boldtrn commented 6 years ago

Ok I just converted this project to an Android project with language level 15. There were two types of error, one were I think about different Java version, available during the compilation (see this commit), the other errors were about the API level, essentially there were a couple of Objects.requireNonNull and one try with resource statement (see this commit). If you don't mind, I would just create a PR fixing the requireNonNull by creating a Helper method and converting the try with resource to a traditional try like:

        // try with resources requires api level 19
        InputStream is = null;
        try {
            is = new FileInputStream(file);
            jtsMvt = loadMvt(is, geomFactory, tagConverter, ringClassifier);
        }finally {
            if(is != null){
                is.close();
            }
        }

Also, if you don't mind I would also change the two Java language errors? I think it would not hurt to convert them, even though I don't think it is necessary.

ShibaBandit commented 6 years ago

Will look into getting new version pushed to maven central tomorrow.

Gustl22 commented 6 years ago

Is there any chance to get the current jar files? Maybe as a Snapshot.. Maven doesn't provide them yet. Sorry for asking. Don't know the release requirements :) Thx

ShibaBandit commented 6 years ago

Pushed maven 3.0.0 release. Might take a while for it to show up.

devemux86 commented 6 years ago

Thank you all for that fine result! We can now use in VTM vector map library the latest mapbox-vector-tile release without Android constraints. 🙂