-
Notifications
You must be signed in to change notification settings - Fork 9.7k
[image_picker] Removed cursor to prevent crash #1804
Conversation
Removed cursor and added MimeTypeMap.getFileExtensionFromUrl to prevent crash occurring in some scenarios.
Code formatted (flutter_plugin_tools)
upgraded gradle version
|
|
||
| dependencies { | ||
| classpath 'com.android.tools.build:gradle:3.3.0' | ||
| classpath 'com.android.tools.build:gradle:3.4.1' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems not related to this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gradle was updated after the PR made, it would be better if added with this PR else not necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, it'd be nice to rebase the PR so we won't see this diff here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry i misunderstood your comment here. I think what I wanted to say is that let's remove change in this particular PR, since it is not related to the PR. Also please remove the related bit in the CHANGELOG
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I'll remove gradle change and comment from the changelog.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cyanglaz I've restored the gradle version and updated changelog.
| if (cursor != null) { | ||
| cursor.close(); | ||
| } | ||
| extension = MimeTypeMap.getFileExtensionFromUrl(uriImage.getPath()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did some research on this method and I found this: https://stackoverflow.com/questions/14320527/android-should-i-use-mimetypemap-getfileextensionfromurl-bugs/14321470
it seems getFileExtensionFromUrl is not sufficient for all scenarios?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed, would be better to find the extension manually but in some scenarios, I found that file uri doesn't have an extension so in that case, it will also not sufficient. so checking for null or empty extension and applying manual extension will be working.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, do you mind to add code to manually check the extension as well?
Something like
extension = fromURI
if (extension == null) {
extension = manuallyParsing
}
if (extension == null) {
extension = ".jpg" (i believe this is already handled)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did the change, null check for extension was already there, in addition to that I added an empty check to prevent rare scenarios.
Removed MimeTypeMap.getFileExtensionFromUrl. Now extension will be retrieved manually.
|
LGTM! Please update the pubspec and changelog with the a new version, please also fix the formatting. |
Code formmated. Pubspec and Changelog is updated
Restored gradle version and updated changelog
cyanglaz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Removed cursor and added MimeTypeMap.getFileExtensionFromUrl to prevent crash occurring in some scenarios.
Description
App is crashing in some scenarios while image picked from the file manager or other sources. In a line:182 while getting the mimeType for an image it was causing the app to crash as sometimes there is no mime type available for the image. Now, to get image extension cursor is removed and now extension will be taken from MimeTypeMap.getFileExtensionFromUrl
Related Issues
Fixes flutter/flutter#35632
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]). This will ensure a smooth and quick review process.///).flutter analyze) does not report any problems on my PR.Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?