[image_picker]fix a crash when a non-image file is picked. #2293

cyanglaz · 2019-11-21T01:29:54Z

Description

This PR fixes 2 things:
The scaling is performed even when the quality/maxWidth/maxHeight are null.
When a non-image file is picked, scaling process is performed on the file.

Related Issues

flutter/flutter#38775
flutter/flutter#40233
b/142680906
b/143851555

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.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
No, this is not a breaking change.

mklim · 2019-11-21T18:40:55Z

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

   */
  String resizeImageIfNeeded(
-      String imagePath, Double maxWidth, Double maxHeight, int imageQuality) {
+      String imagePath, Double maxWidth, Double maxHeight, Integer imageQuality) {


nit: If we're including androidx.annotation already, it would be nice if imageQuality and the other potentially null params here were tagged with @Nullable.

mklim · 2019-11-21T18:42:06Z

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

-      File scaledImage = resizedImage(imagePath, maxWidth, maxHeight, imageQuality);
-      exifDataCopier.copyExif(imagePath, scaledImage.getPath());
+      Bitmap bmp = decodeFile(imagePath);
+      if (bmp == null) {


nit: Do all of these lines need to be guarded by an IOException, or can they be moved outside of the try/catch?

mklim · 2019-11-21T18:44:03Z

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

    double originalHeight = bmp.getHeight() * 1.0;

-    if (imageQuality < 0 || imageQuality > 100) {
+    if (imageQuality == null || imageQuality < 0 || imageQuality > 100) {


nit: This condition is repeated in a lot of places, and it uses two magic numbers (0 and 100). I think it would help readability a little to extract it out into a helper function called isImageQualityValid that was called in multiple places instead.

mklim · 2019-11-21T19:03:57Z

...icker/example/android/app/src/test/java/io/flutter/plugins/imagepicker/ImageResizerTest.java

+import org.mockito.Spy;
+import org.mockito.invocation.InvocationOnMock;
+
+public class ImageResizerTest {


If we have some time to improve this, it would be better to change the style of testing here so that it's really testing the behavior of ImageResizer's public API instead of the interactions of a bunch of its private methods. The way this test is written right now it's really just verifying that when certain methods are called, other methods also get triggered. This isn't really testing the logic of the class, and it could be broken by trivial method renames. It also exposes a bunch of methods with @VisibleForTesting that would be better kept private.

https://testing.googleblog.com/2013/08/testing-on-toilet-test-behavior-not.html

Here's the approach I would probably take:

Use TemporaryFolder to create a file directory for the unit tests, and pass it into ImageResizer as externalFilesDirectory as its constructed in the tests.

Write a sample bitmap to TemporaryFolder.

Call ImageResizer.resizeImageIfNeeded with a combination of settings. Verify that the bitmap at the returned path exists and has been resized correctly for each one.

What do you think, does that sound workable to you? The catch here is I'm not sure how creating a Bitmap in a test goes, and I'm not sure if it's easy to check a Bitmap's image quality or not. I don't remember that API off the top of my head.

This is still an improvement over what we have so I am happy to merge this if needed, thank you for writing tests.

Will take a look

mklim · 2019-11-21T19:05:00Z

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

+        return null;
+      }
+      String[] pathParts = imagePath.split("/");
+      String imageName = pathParts[pathParts.length - 1];


Are all images always guaranteed to have an extension?

This is the imageName. For example if the imagePath is foo/bar/image.png, the imageName would be image.png.
Or you are asking what we should do if the imageName doesn't contain an extension?

cyanglaz

Done some changes based on the comments. Will look into improving the tests.

cyanglaz · 2019-11-21T22:42:22Z

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

   */
  String resizeImageIfNeeded(
-      String imagePath, Double maxWidth, Double maxHeight, int imageQuality) {
+      String imagePath, Double maxWidth, Double maxHeight, Integer imageQuality) {


cyanglaz · 2019-11-21T22:43:32Z

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

-      File scaledImage = resizedImage(imagePath, maxWidth, maxHeight, imageQuality);
-      exifDataCopier.copyExif(imagePath, scaledImage.getPath());
+      Bitmap bmp = decodeFile(imagePath);
+      if (bmp == null) {


cyanglaz · 2019-11-21T22:45:09Z

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

+        return null;
+      }
+      String[] pathParts = imagePath.split("/");
+      String imageName = pathParts[pathParts.length - 1];


This is the imageName. For example if the imagePath is foo/bar/image.png, the imageName would be image.png.
Or you are asking what we should do if the imageName doesn't contain an extension?

cyanglaz · 2019-11-21T22:46:45Z

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

    double originalHeight = bmp.getHeight() * 1.0;

-    if (imageQuality < 0 || imageQuality > 100) {
+    if (imageQuality == null || imageQuality < 0 || imageQuality > 100) {


cyanglaz · 2019-11-21T22:49:17Z

...icker/example/android/app/src/test/java/io/flutter/plugins/imagepicker/ImageResizerTest.java

+import org.mockito.Spy;
+import org.mockito.invocation.InvocationOnMock;
+
+public class ImageResizerTest {


Will take a look

mklim

LGTM

Better testing for this isn't feasible right now, Robolectric fakes out too much of Bitmap and decoding it from files/byte streams.

)

fix a crash when a non-image file is pick

d519b67

googlebot added the cla: yes label Nov 21, 2019

cyanglaz requested a review from mklim November 21, 2019 01:31

Chris Yang added 2 commits November 20, 2019 17:34

syntax cleanning

ad38dc8

changelog updates

f40c824

mklim reviewed Nov 21, 2019

View reviewed changes

cyanglaz changed the title ~~[image_picker]f ix a crash when a non-image file is picked.~~ [image_picker]fix a crash when a non-image file is picked. Nov 21, 2019

cyanglaz mentioned this pull request Nov 21, 2019

[image_picker] remove imageQuality fallback so resizeImageIfNeeded works as expected #2198

Closed

13 tasks

cyanglaz commented Nov 21, 2019

View reviewed changes

mklim approved these changes Nov 22, 2019

View reviewed changes

Chris Yang and others added 3 commits November 21, 2019 17:54

use RobolectricTestRunner

0bfa892

fix failing tests

698861c

formatting

8242a64

cyanglaz merged commit db123e8 into flutter:master Nov 22, 2019

cyanglaz deleted the image_picker_fixes branch November 22, 2019 05:30

This was referenced Nov 22, 2019

[image_picker] NullPointerException at ImageResizer.java:52 when resizing image flutter/flutter#38775

Closed

Android image_picker crashes when non-image file selected flutter/flutter#40233

Closed

Minituff mentioned this pull request Nov 22, 2019

image_picker Bitmap NullPointException on Android 10 with open failed: EACCES (Permission denied) flutter/flutter#41459

Closed

cyanglaz mentioned this pull request Dec 6, 2019

[Image_picker] fix rotation regression on Android. #2362

Merged

13 tasks

sungmin-park pushed a commit to sungmin-park/flutter-plugins that referenced this pull request Dec 17, 2019

[image_picker]fix a crash when a non-image file is picked. (flutter#2293

2f14638

)

anniek-valk mentioned this pull request Sep 21, 2020

[image_picker] Flutter android app will be crashed when selecting HEIC image flutter/flutter#25741

Closed

danagbemava-nc mentioned this pull request Jul 3, 2023

Enable catch for exception returned from underline Flutter flutter/flutter#128415

Closed

2 tasks

snoopdoggy322 mentioned this pull request Sep 7, 2023

[image_picker] ImageResizer.resizeImageIfNeeded (ImageResizer.java:42) flutter/flutter#134125

Closed

2 tasks

[image_picker]fix a crash when a non-image file is picked. #2293

[image_picker]fix a crash when a non-image file is picked. #2293

Uh oh!

Conversation

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

Uh oh!

Description

Related Issues

Checklist

Breaking Change

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!

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 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!

mklim left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cyanglaz commented Nov 21, 2019 •

edited

Loading