Small improvements #6
Conversation
…with cordova.plugins, updated README
| @@ -146,7 +146,7 @@ private static JSONArray JSONFromCursor(Cursor cursor) throws JSONException { | |||
| rowObject.put("title", cursor.getString(cursor.getColumnIndex(DownloadManager.COLUMN_TITLE))); | |||
| rowObject.put("description", cursor.getString(cursor.getColumnIndex(DownloadManager.COLUMN_DESCRIPTION))); | |||
| rowObject.put("mediaType", cursor.getString(cursor.getColumnIndex(DownloadManager.COLUMN_MEDIA_TYPE))); | |||
| rowObject.put("localFilename", cursor.getString(cursor.getColumnIndex(DownloadManager.COLUMN_LOCAL_FILENAME))); | |||
| rowObject.put("localFilename", cursor.getString(cursor.getColumnIndex(DownloadManager.COLUMN_LOCAL_URI))); | |||
There was a problem hiding this comment.
Is this a mistake or on purpose?
There was a problem hiding this comment.
Nope, testing on android 7+ told me that COLUMN_LOCAL_FILENAME was deprecated and resulted to error in callback. Searching upon deprecation brought to solutions using COLUMN_LOCAL_URI instead.
There was a problem hiding this comment.
I don't think this is the right thing to do; https://developer.android.com/reference/android/app/DownloadManager#COLUMN_LOCAL_FILENAME
There was a problem hiding this comment.
I've seen this, I just thought that it was a bit overkilling for the purpose of the plugin. It's basically the same thing just without the "file://" in front. So maybe just DownloadManager.COLUMN_LOCAL_URI.replace("file://", "") could be fine.
https://stackoverflow.com/questions/38839688/downloadmanager-column-local-filename-deprecated
There was a problem hiding this comment.
Ah yes, so the snippet they refer to I don't think belongs here. What I'm more considering is whether this property should be present if the API is > 24. That can however also quickly turn into a mess. What does other plugins do in cases like this? I know it's very common in iOS plugins to feature detect
There was a problem hiding this comment.
We could just use platform version detection for the purpose. E. G.
if (android.os.Build.VERSION.SDK_INT < android.os.Build.VERSION_CODES.N){ ... }
There was a problem hiding this comment.
Added feature detection in latest commit
|
Any news about this? Can you merge? |
|
I'm happy to take the java and readme changes but you need to revert the rest. |
|
What is the reasoning to not accept this PR? |
|
Sorry to say this but the repo owner is stubborn and doesn’t want to remove an unneeded dependency (from a library he created) that just resolves in an if statement for the usage made. I guess nanoassert it’s useful in other cases, but here it’s just overhead. |
|
Nevermind. I found https://github.com/vasani-arpit/cordova-plugin-downloadmanager which is working for me. Thanks @giordanocardillo for your contribution which pointed me to my solution. |
nanoassertdependencycordova.plugins.DownloadManagersyntax