Feature/create archive provider#1715
Conversation
monsieurtanuki
left a comment
monsieurtanuki
left a comment
There was a problem hiding this comment.
Hi @pasniak!
Still something I don't fully understand with the init method and removing the final keywords.
| @Override | ||
| public boolean preCheck() { | ||
| if (mTileSource instanceof OnlineTileSourceBase) { | ||
| if (mTileSource!=null && mTileSource instanceof OnlineTileSourceBase) { |
There was a problem hiding this comment.
If I remember well, in Java a null is never an instance of anything. That fix is not needed.
There was a problem hiding this comment.
I believe I was getting some exception along the lines null is not instance of OnlineTileSourceBase (instead going to else branch).
But yes, folding init() into constructor makes this redundant as final cannot be null. I will update to PR.
Note: If I force createAssetsProvider in my MapTileProviderCustom class to return null:
java.lang.RuntimeException: Unable to start activity ComponentInfo{com.myapp.debug/com.myapp.ui.activity.fullscreenactivity.FullscreenActivity}: java.lang.NullPointerException: Attempt to invoke virtual method 'int org.osmdroid.tileprovider.modules.MapTileModuleProviderBase.getMinimumZoomLevel()' on a null object reference
at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:2913)
at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:3048)
at android.app.servertransaction.LaunchActivityItem.execute(LaunchActivityItem.java:78)
at android.app.servertransaction.TransactionExecutor.executeCallbacks(TransactionExecutor.java:108)
at android.app.servertransaction.TransactionExecutor.execute(TransactionExecutor.java:68)
at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1808)
at android.os.Handler.dispatchMessage(Handler.java:106)
at android.os.Looper.loop(Looper.java:193)
at android.app.ActivityThread.main(ActivityThread.java:6669)
at java.lang.reflect.Method.invoke(Native Method)
at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:493)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:858)
Caused by: java.lang.NullPointerException: Attempt to invoke virtual method 'int org.osmdroid.tileprovider.modules.MapTileModuleProviderBase.getMinimumZoomLevel()' on a null object reference
at org.osmdroid.tileprovider.modules.MapTileApproximater.computeZoomLevels(MapTileApproximater.java:64)
at org.osmdroid.tileprovider.modules.MapTileApproximater.addProvider(MapTileApproximater.java:57)
at org.osmdroid.tileprovider.MapTileProviderBasic.createApproximater(MapTileProviderBasic.java:133)
at org.osmdroid.tileprovider.MapTileProviderBasic.<init>(MapTileProviderBasic.java:106)
at org.osmdroid.tileprovider.MapTileProviderBasic.<init>(MapTileProviderBasic.java:70)
at org.osmdroid.tileprovider.MapTileProviderBasic.<init>(MapTileProviderBasic.java:63)
at org.osmdroid.tileprovider.MapTileProviderBasic.<init>(MapTileProviderBasic.java:56)
There was a problem hiding this comment.
Oh I guess if it doesn't exist now you could create an empty MapTileFileStorageProviderBase that returns nothing.
Or systematically test for null before adding each provider.
Or remove altogether each local variable (like assetsProvider); basically you don't need an asset provider and a cache provider and so on, you just need a list of providers, to be added to the mTileProviderList and to the MapTileApproximater. What do you think of that?
| private final MapTileDownloader mDownloaderProvider; | ||
| private final MapTileApproximater mApproximationProvider; | ||
| private MapTileDownloader mDownloaderProvider; | ||
| private MapTileApproximater mApproximationProvider; |
There was a problem hiding this comment.
There's no obvious reason not to keep them final.
The point of this PR is to create these fields differently, not to make it possible to change them afterwards, isn't it?
| init(pRegisterReceiver, aNetworkAvailablityCheck, pTileSource, pContext, cacheWriter); | ||
| } | ||
|
|
||
| protected void init(IRegisterReceiver pRegisterReceiver, |
There was a problem hiding this comment.
I can't see the point of new method init. I thought the purpose was mainly to override the new create* methods.
…a (meant to provide ways to override construction of MapTileProviderBasic). Members can be final again. modified: osmdroid-android/src/main/java/org/osmdroid/tileprovider/MapTileProviderBasic.java modified: osmdroid-android/src/main/java/org/osmdroid/tileprovider/cachemanager/CacheManager.java
monsieurtanuki
left a comment
monsieurtanuki
left a comment
There was a problem hiding this comment.
Basically you answered all my comments, thank you @pasniak!
But I've just answered one of your new comments, and I think there's something interesting regarding lists instead of local variables to have a look at. Tell me what you think about it.
|
is this good to merge? |
monsieurtanuki
left a comment
monsieurtanuki
left a comment
There was a problem hiding this comment.
Except for the format change that does not help code review, I approve!
3 participants
Hi, I would like to be able to create a custom archive provider (in my own MapTileProvider inheriting from MapTileProviderBasic).
The PR has change which moves constructor to init() and adds
createApproximater()createAssetsProvider()createArchiveProvider()which can be overwritten in client code.