[image_picker] set real extension instead always jpg #1269

ivk1800 · 2019-02-23T15:42:01Z

No description provided.

cyanglaz

@mklim Could you take a look at the getImageExtension part just to make sure the code to get the extension makes sense?

cyanglaz · 2019-03-08T01:15:50Z

packages/image_picker/android/src/main/java/io/flutter/plugins/imagepicker/FileUtils.java

    try {
      inputStream = context.getContentResolver().openInputStream(uri);
-      file = File.createTempFile("image_picker", "jpg", context.getCacheDir());
+      file = File.createTempFile("image_picker", extensionWithDot, context.getCacheDir());


what happens to the file path if extensionWithDot is ""

nothing, because suffix simply concatenating with prefix. suffix may be null, in which case the suffix .tmp will be used

Does File#createTempFile distinguish between null and ""? Technically we are sending it "" here in undefined cases, not null, and its documentation talks about defaulting to ".tmp" when this param is null specifically.

cyanglaz · 2019-03-08T01:18:00Z

packages/image_picker/android/src/main/java/io/flutter/plugins/imagepicker/FileUtils.java

+        extension = MimeTypeMap.getSingleton().getExtensionFromMimeType(mimeType);
+      }
+    } catch (Exception ignored) {
+


ignoring exception seems odd. Do we not have to resolve this if there is an exception?

if exception was thrown, final file name will be without extension.

Two things here:

I think catching the top level Exception itself is too broad of a net to throw, it could mask other underlying bugs. Could we instead try to catch the specific types of Exceptions that we're expecting to be thrown from querying the ContentResolver and getting data out of the Cursor here?

I think it would be safer to default to "jpg" in cases where we can't really find the file than defaulting to File#createTempFile's default ".tmp". "jpg" isn't necessarily correct in this case, but it matches the previous behavior of the plugin so it's unlikely to cause new regressions. But using ".tmp" could cause breakages in rare cases, by converting a ".jpg" instead a ".tmp" because of an I/O error with querying the mime type here. What do you think?

mklim

Thanks for the contribution! If you don't have time to keep working on this PR, please let us know and we'll be happy to shepherd it forward from here.

pubspec.yaml and CHANGELOG.md also need to be updated with a new patch version and description of this change.

mklim · 2019-03-08T21:19:21Z

packages/image_picker/android/src/main/java/io/flutter/plugins/imagepicker/FileUtils.java

+        extension = MimeTypeMap.getSingleton().getExtensionFromMimeType(mimeType);
+      }
+    } catch (Exception ignored) {
+


Two things here:

I think catching the top level Exception itself is too broad of a net to throw, it could mask other underlying bugs. Could we instead try to catch the specific types of Exceptions that we're expecting to be thrown from querying the ContentResolver and getting data out of the Cursor here?

I think it would be safer to default to "jpg" in cases where we can't really find the file than defaulting to File#createTempFile's default ".tmp". "jpg" isn't necessarily correct in this case, but it matches the previous behavior of the plugin so it's unlikely to cause new regressions. But using ".tmp" could cause breakages in rare cases, by converting a ".jpg" instead a ".tmp" because of an I/O error with querying the mime type here. What do you think?

mklim · 2019-03-11T17:46:08Z

packages/image_picker/android/src/main/java/io/flutter/plugins/imagepicker/FileUtils.java

@@ -133,9 +134,11 @@ private static String getPathFromRemoteUri(final Context context, final Uri uri)
    InputStream inputStream = null;
    OutputStream outputStream = null;
    boolean success = false;
+    String extension = getImageExtension(context, uri);


nit: I think it would be cleaner here if you handled all the logic about prepending the dot if the string was empty in getImageExtension itself, and then treated the result here as the final extension. That way we could keep all the image extension logic contained to the helper function for it.

mklim · 2019-03-11T17:56:44Z

packages/image_picker/android/src/main/java/io/flutter/plugins/imagepicker/FileUtils.java

    try {
      inputStream = context.getContentResolver().openInputStream(uri);
-      file = File.createTempFile("image_picker", "jpg", context.getCacheDir());
+      file = File.createTempFile("image_picker", extensionWithDot, context.getCacheDir());


Does File#createTempFile distinguish between null and ""? Technically we are sending it "" here in undefined cases, not null, and its documentation talks about defaulting to ".tmp" when this param is null specifically.

cyanglaz · 2019-04-25T17:01:27Z

@pepegich any updates after the review from @mklim?

ivk1800 · 2019-04-25T18:16:13Z

@mklim please review new updates

mklim

Looking good, thanks for working on this! Just one thing left before this is ready to merge.

pubspec.yaml and CHANGELOG.md also need to be updated with a new patch version and description of this change.

mklim

LGTM. Thanks again!

packages/image_picker/CHANGELOG.md

Co-Authored-By: pepegich <boytheprodigy@gmail.com>

set real extension instead always jpg

e11ca57

googlebot added the cla: yes label Feb 23, 2019

bparrishMines changed the title ~~set real extension instead always jpg~~ [image_picker] set real extension instead always jpg Feb 27, 2019

cyanglaz reviewed Mar 8, 2019

View reviewed changes

fix formatting code

8de1e5a

mklim reviewed Mar 11, 2019

View reviewed changes

Merge branch 'master' into set_real_image_type_from_remote_uri

b87ec88

ivk1800 added 2 commits April 25, 2019 22:05

code refactor

17442c9

code format

0c49cc3

mklim reviewed Apr 29, 2019

View reviewed changes

ivk1800 added 2 commits April 30, 2019 20:01

Merge branch 'master' into set_real_image_type_from_remote_uri

d67803e

version

7cdf9fc

mklim approved these changes Apr 30, 2019

View reviewed changes

packages/image_picker/CHANGELOG.md Outdated Show resolved Hide resolved

Update packages/image_picker/CHANGELOG.md

7545b23

Co-Authored-By: pepegich <boytheprodigy@gmail.com>

mklim added the submit queue The Flutter team is in the process of landing this PR. label Apr 30, 2019

mklim merged commit 3830b02 into flutter:master Apr 30, 2019

[image_picker] set real extension instead always jpg #1269

[image_picker] set real extension instead always jpg #1269

Uh oh!

Conversation

ivk1800 commented Feb 23, 2019

Uh oh!

cyanglaz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mklim left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyanglaz commented Apr 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ivk1800 commented Apr 25, 2019

Uh oh!

mklim left a comment

Choose a reason for hiding this comment

Uh oh!

mklim left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cyanglaz commented Apr 25, 2019 •

edited

Loading

mklim left a comment •

edited

Loading