build: fix all deprecation and lint in ShareFile(s) #374
Conversation
| final String authority = ((ShareApplication) reactContext.getApplicationContext()).getFileProviderAuthority(); | ||
|
|
||
| if(this.isBase64File()) { | ||
| String encodedImg = this.uri.getSchemeSpecificPart().substring(this.uri.getSchemeSpecificPart().indexOf(";base64,") + 8); | ||
| try { | ||
| File dir = new File(Environment.getExternalStorageDirectory(), Environment.DIRECTORY_DOWNLOADS ); | ||
| if (!dir.exists()) { | ||
| dir.mkdirs(); |
There was a problem hiding this comment.
Maybe it is better to keep this?
There was a problem hiding this comment.
More than keep it - I altered it to not throw away the return value of "dir.mkdirs()" - now that becomes part of a chain of dir.exists + dir.mkdirs, and if one or the other of them doesn't work we get notice. Previously if dir.mkdirs() returned false we'd never know
| @@ -139,7 +134,9 @@ public Uri getURI() { | |||
| } | |||
| } else if(this.isLocalFile()) { | |||
| Uri uri = Uri.parse(this.url); | |||
|
|
|||
| if (uri.getPath() == null) { | |||
| return null; | |||
There was a problem hiding this comment.
A good question - no intention to break - the suggestion here was part of Android Studio lint, it notified that uri.getPath() may be null, so I put a check in. I wasn't sure what to do with it though, what would the consequence of proceeding with previous code if uri.getPath() had been null? I think it would have thrown RuntimeException. I suppose we could null check farther up the call chain, or throw RuntimeException ourselves with a good message, or let it continue as previous (without this change) where null may infiltrate there
There was a problem hiding this comment.
Makes sense.
I think we can go on this way and test behaviours. If return null does not work, we could try to throw a RuntimeException, like your suggestion.
We can also try to solve this on JS land.
| @@ -79,7 +77,7 @@ private boolean isBase64File(Uri uri) { | |||
| String type = uri.getSchemeSpecificPart().substring(0, uri.getSchemeSpecificPart().indexOf(";")); | |||
| if (this.intentType == null) { | |||
| this.intentType = type; | |||
| } else if (type != null && !this.intentType.equalsIgnoreCase(type) && this.intentType.split("/")[0].equalsIgnoreCase((type.split("/"))[0])) { | |||
There was a problem hiding this comment.
can we keep type != null check?
There was a problem hiding this comment.
Code path analysis indicates it will never be null, it was a no-op. I suppose the substring method will either return something, empty string, or throw.
| @@ -109,7 +107,7 @@ private boolean isLocalFile(Uri uri) { | |||
|
|
|||
| if (this.intentType == null) { | |||
| this.intentType = type; | |||
| } else if (type != null && !this.intentType.equalsIgnoreCase(type) && this.intentType.split("/")[0].equalsIgnoreCase((type.split("/"))[0])) { | |||
There was a problem hiding this comment.
same here, can we keep type != null ?
There was a problem hiding this comment.
and same as above - I suppose if paranoid you can keep it, but static analysis indicates it's a nop
| @@ -131,6 +129,9 @@ private String getRealPathFromURI(Uri contentUri) { | |||
| String[] proj = { MediaStore.Images.Media.DATA }; | |||
| CursorLoader loader = new CursorLoader(this.reactContext, contentUri, proj, null, null, null); | |||
| Cursor cursor = loader.loadInBackground(); | |||
| if (cursor == null) { | |||
| return ""; | |||
There was a problem hiding this comment.
why is this early return necessary?
There was a problem hiding this comment.
This was similar to the null check on the file path earlier - Android Studio lint indicated that null may infiltrate - loader.loadInBackground isn't guaranteed to return non-null by contract, I put in a null check but wasn't entirely sure what to do in that (exceptional) case. The early return then can be considered a stand in for "bail out!", without increasing branch depth for the rest of the method. There may be a better way to bail
There was a problem hiding this comment.
Alright, let's keep this and do a few tests to understand if this will produce strange behaviours. Thanks
| @@ -150,8 +151,8 @@ private String getRealPathFromURI(Uri contentUri) { | |||
| String encodedImg = uri.getSchemeSpecificPart().substring(uri.getSchemeSpecificPart().indexOf(";base64,") + 8); | |||
| try { | |||
| File dir = new File(Environment.getExternalStorageDirectory(), Environment.DIRECTORY_DOWNLOADS ); | |||
| if (!dir.exists()) { | |||
| dir.mkdirs(); | |||
There was a problem hiding this comment.
Maybe it is better to keep this?
There was a problem hiding this comment.
Same as above, this one was actually reinforced by not only being kept but by having it's return value actually used. In fact - ShareFiles and ShareFile should almost inherit from each other, or re-use in some other preferred OO way - the code between them is very duplicate. That's a big change though - much more than mechanical / static-analysis de-lint
| @@ -163,7 +164,9 @@ private String getRealPathFromURI(Uri contentUri) { | |||
| e.printStackTrace(); | |||
| } | |||
| } else if(this.isLocalFile(uri)) { | |||
| finalUris.add(FileProvider.getUriForFile(reactContext, authority, new File(uri.getPath()))); | |||
| if (uri.getPath() != null) { | |||
Awesome, I think we are good to go on this |
|
Sure - no intention to make a breaking change of course, so the proper way to bail out / return on some of the new defensiveness should be thought about. |
|
Hey @jgcmarins - no pressure, just got my PR list to a single page and saw this sitting here. Anything you'd like to see to merge it, or should we close it? Picking lint isn't the most vital thing so I could go either way but it is nice when things are mirror polished thus the PR in the first place. Just let me know. Cheers |
No need to have Android Studio light up on these files
Each of these fixes was mechanical as suggested by Android Studio and code inspection
Even the possibly-controversial removal of null-checking in a couple spots can be shown to be a no-op by inspection of possible values