use temp image file in cacheDir to allow picking from remote resources #513

gbaccetta · 2018-04-30T13:12:24Z

imagePicker caused flutter app to crash when picking image from a remote source.
Using a temp file to store the inputStream obtained from the content provider solve the problem.
It actually use a temp file in the cachedDir named cachedImage.jpg.

googlebot · 2018-04-30T13:12:27Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

googlebot · 2018-04-30T16:50:39Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

gbaccetta · 2018-04-30T16:51:13Z

I signed it!

…

________________________________ Da: googlebot <notifications@github.com> Inviato: lunedì 30 aprile 2018 18:50 A: flutter/plugins Cc: gbaccetta; Author Oggetto: Re: [flutter/plugins] use temp image file in cacheDir to allow picking from remote resources (#513) We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here<https://cla.developers.google.com/> to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#513 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ANXrKigEKAWGStsDCnYugJ4sUqdLie1mks5tt0DigaJpZM4Tsj7s>.

mravn-google

Thanks for the contribution!

mravn-google · 2018-05-01T05:46:12Z

...s/image_picker/android/src/main/java/io/flutter/plugins/imagepicker/ImagePickerDelegate.java

+        try {
+            InputStream inputStream = null;
+            OutputStream output = null;
+            try {


Please use try-with-resources. It is too hard to get this code correct without.

isn't resource management only for API 19+ and flutter for 16+ ?

You are right, sorry. In that case, you need to wrap your calls to close in try-catch blocks. Otherwise we risk not actually closing the input stream, as well as exiting abnormally from a finally block.

mravn-google · 2018-05-01T05:46:48Z

...s/image_picker/android/src/main/java/io/flutter/plugins/imagepicker/ImagePickerDelegate.java

+            OutputStream output = null;
+            try {
+                inputStream = activity.getContentResolver().openInputStream(data.getData());
+                File file = new File(activity.getCacheDir(), "cachedImage.jpg");


What if that file already exists?
When is the file cleaned up?

I guessed that for the cleaning since it is the cacheDir and I was trying to always use the same file name that was ok. In that sense imageResizer is even worse since it create a scaled image version in the external directory where the original file is, leaving the user with two copies of it. However you are right about the file already existing

mravn-google · 2018-05-01T05:47:42Z

...s/image_picker/android/src/main/java/io/flutter/plugins/imagepicker/ImagePickerDelegate.java

+                File file = new File(activity.getCacheDir(), "cachedImage.jpg");
+                output = new FileOutputStream(file);
+                if (inputStream != null) {
+                    byte[] buffer = new byte[4 * 1024]; // or other buffer size


Please avoid comments that convey only uncertainty. There is nothing inherently wrong with a 4K buffer.

mravn-google · 2018-05-01T05:48:36Z

...s/image_picker/android/src/main/java/io/flutter/plugins/imagepicker/ImagePickerDelegate.java

@@ -300,8 +304,36 @@ public boolean onActivityResult(int requestCode, int resultCode, Intent data) {

  private void handleChoosePictureResult(int resultCode, Intent data) {
    if (resultCode == Activity.RESULT_OK && data != null) {
-      String path = fileUtils.getPathFromUri(activity, data.getData());
-      handleResult(path);
+        //String path = fileUtils.getPathFromUri(activity, data.getData());


Please remove unneeded code rather than put it in a comment.

If FileUtils is no longer used by the plugin, it should be removed as well.

mravn-google · 2018-05-01T05:49:07Z

...s/image_picker/android/src/main/java/io/flutter/plugins/imagepicker/ImagePickerDelegate.java

-      handleResult(path);
+        //String path = fileUtils.getPathFromUri(activity, data.getData());
+
+        //creating a temp file from uri to allow reading pictures from remote sources


And if it isn't remote, do we then copy the file needlessly?

Please format comments as whole sentences:

// Create a temporary file from ...

Well we can keep using FileUtils for local path and create temporary file only for remoteResources when path is null in the same way as done for the camera intent

mravn-google · 2018-05-01T06:07:37Z

The commit appears to have been made using a different author name or email than that used for CLA signing. You probably have to amend your commit.

roughike · 2018-05-01T07:44:12Z

@mravn-google @gbaccetta This PR breaks two tests (found in example/android/app/src/test).

There's no CI integration for running these tests just yet; @goderbauer requested me to modify the Travis script in this comment in #461. I haven't had time to look into it, and honestly have no idea how to do / test it.

(The tests can be run from the command line by running cd packages/image_picker/example/android && ./gradlew testDebugUnitTest.)

googlebot · 2018-05-01T12:58:24Z

CLAs look good, thanks!

goderbauer · 2018-05-01T21:16:14Z

@roughike Travis ran the image_picker java tests on this PR and they're still passing. Is that unexpected?

roughike · 2018-05-01T21:38:00Z

@goderbauer Java tests should now pass.

The Java tests failed when there was only the first commit, c808da73cfd, on this PR. The later commit, 036ccdf1f04, fixed the failing tests, making all test cases pass again. Just confirmed this locally to be extra sure.

So everything is working as expected!

goderbauer · 2018-05-01T22:51:55Z

@gbaccetta Does this by any chance also fix flutter/flutter#17165?

gbaccetta · 2018-05-02T09:35:15Z

@goderbauer yes it fix also that problem, that's why I looked for this solution. For the formating problem I could not change my Android studio indentention from 4 to 2 characters... sorry about that

gbaccetta · 2018-05-02T10:04:58Z

ok, I should have fixed the formatting in last commit

roughike · 2018-05-02T10:40:26Z

@gbaccetta You can format all Dart, Java and Objective C files by running pub global run flutter_plugin_tools format in the plugins directory. I also have indentation with 4 spaces in my Android Studio, but I always run that command before committing anything. :)

See here: https://github.com/flutter/plugins/blob/master/CONTRIBUTING.md

gbaccetta · 2018-05-02T11:02:41Z

Check still failed even after correcting the format. Could not understand why this time. I'm sorry this is the first time I 'm trying to contribute to Google Open Source.

@roughike that command for me fails with this from the plugin directory:

Error: Cannot find a "packages" sub-directory
pub finished with exit code 1

and with this from the root directory:

Error: No pubspec.yaml file found.
This command should be run from the root of your Flutter project.
Do not run this command from the root of your git clone of Flutter.

roughike · 2018-05-02T11:18:15Z

No worries!

Let's say you have cloned this repo in C:/Documents/my-github-contributions/plugins. Assuming that opening a new terminal window starts you at C:/, here's what you do:

cd Documents
cd my-github-contributions
cd plugins # this is the directory you want to be in

# Now we're inside the correct directory, so we can use the plugin tools.
pub global run flutter_plugin_tools format

roughike · 2018-05-03T04:46:15Z

...s/image_picker/android/src/main/java/io/flutter/plugins/imagepicker/ImagePickerDelegate.java

-      handleResult(path);
+      //Path may be null if, for example, source is remote.
+      if (path == null) {
+        InputStream inputStream = null;


Just a drive-by comment: I would at least extract this logic into a separate method (called getRemotePathFromUri() for example) to make the code a bit clearer.

Maybe even move this logic to FileUtils class if we go crazy. If the Uri has a distinctive scheme or authority, it could be added as an else if at Line 90 in the FileUtils class.

Something like:

FileUtils.java (starting from line 90)

... } else if (isRemoteFile(uri)) { // Move the logic for getting remote path from the uri here. // Then return the resolved path. } ...

Disclaimer: I'm not in the Flutter team and you can just ignore this feedback if you want. ¯\(ツ)/¯

Problem is the authority is not distintive.
For example local file from google photos has the same authority then remote file.
However the local file path is correctly retrieved but not the remote.
That's why I used the logic as a fallback cause this always works but need to create a temp file... Hence the idea was to say: let's try to retrieve the path in every manner we may know of and fallback to inputstream and tempFile if those methods fail. It allows to keep the plugin to work even if a provider change its distintive authority

Problem is the authority is not distinctive.

How about the scheme then?

If not, I would still personally move the logic to FileUtils. In the very end of the getPathFromUri method, we could return the path from remote Uri if it can be resolved. If not, then just return null like it currently does.

roughike · 2018-05-03T05:06:20Z

...s/image_picker/android/src/main/java/io/flutter/plugins/imagepicker/ImagePickerDelegate.java

+        OutputStream outputStream = null;
+        try {
+          inputStream = activity.getContentResolver().openInputStream(data.getData());
+          File file = File.createTempFile("cachedImage", "jpg", activity.getCacheDir());


Just thinking if cachedImage.jpg is a too generic name for an image to be stored in a cache directory. There could be a chance we're overwriting some other image by accident. Would it make sense to use something less common, like flutter_image_picker_cached_remote_image.jpg for example?

UUIDs would be another option, but that could blow up the application's cache directory over time since images can get quite big. This might potentially give end users a bad impression of the app that uses this plugin.

@roughike normay using createTempFile automatic append a random numeric string to the end of the name so that files will be "cachedImage-123456789.jpg" or something similar. I did not find of a nice way of automatically deleting the file created in the cacheDir

googlebot · 2018-06-10T18:23:40Z

CLAs look good, thanks!

mravn-google · 2018-06-10T18:57:50Z

...s/image_picker/android/src/main/java/io/flutter/plugins/imagepicker/ImagePickerDelegate.java

@@ -63,7 +63,7 @@
 * <p>C) User cancels picking an image. Finish with null result.
 */
 public class ImagePickerDelegate
-    implements PluginRegistry.ActivityResultListener,
+        implements PluginRegistry.ActivityResultListener,


These white space changes seem to be at odds with our CI's formatting requirements. Please revert.

mravn-google · 2018-06-10T19:00:21Z

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

+          outputStream.write(buffer, 0, read);
+        }
+        outputStream.flush();
+        return file.getPath();


We are returning a file path here even if there are exceptions thrown while closing the streams. We should only return a file path if all the I/O completes normally.

mravn-google · 2018-06-10T19:01:18Z

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

  String getPathFromUri(final Context context, final Uri uri) {
+    //Try to retrieve path on device


These comments add little value; the code already speaks for itself.

googlebot · 2018-06-11T14:16:57Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

googlebot · 2018-06-11T14:47:13Z

CLAs look good, thanks!

…e, rebase into master and logic moved to FileUtils)

gbaccetta · 2018-06-11T16:35:58Z

Finally starting from scratch ( new branch from actual master + force push to clean all the mess) seems to have worked. Anyway formatting is really hard to achieve since pub global run flutter_plugin_tools format does not seem to work on my pc.

mravn-google · 2018-06-13T06:22:27Z

@gbaccetta Do you want to work on the last review comment, or would you prefer us taking it from here?

gbaccetta · 2018-06-13T16:09:33Z

@mravn-google you can take over. However, as I see it, in case of exception the code return null and not the filePath. Unless I missinterpreted something

mravn-google · 2018-06-14T19:21:55Z

@gbaccetta Thanks, I'll make a small tweak in a separate PR and then publish.

SkullEnemyX · 2018-06-20T10:49:57Z

Somebody please find a fix to the problem which is making the app crash if we choose an image using image_picker from google photos but works totally fine if the image is picked from the local gallery. I am in dire need for the solution.

roughike · 2018-06-20T10:51:02Z

@SkullEnemyX I believe this PR is exactly that fix.

SkullEnemyX · 2018-06-20T10:52:18Z

I am unable to understand what to do exactly as the method is not clear (PS: I am a beginner in app development and flutter)

roughike · 2018-06-21T09:59:45Z

That's fine!

As far as I understand, a fix for your crash with Google Photos was just merged in this pull request. Is the problem still surfacing with version 0.4.5 of this plugin?

pojoba02 · 2018-10-22T23:45:16Z

@gbaccetta @goderbauer @roughike @mravn-google This issue is completely not fixed, it still does exist, Guess is happening in Oreo and above(Pie). App just crashes when uploading images and it never displays the image too.
Please what might be causing it?
FYI am using version 0.4.10 of the image Picker

googlebot added the cla: no label Apr 30, 2018

mravn-google reviewed May 1, 2018

View reviewed changes

mravn-google self-assigned this May 1, 2018

gbaccetta force-pushed the image_picker_fix_remote_sources_error branch 2 times, most recently from a9a10d4 to 036ccdf Compare May 1, 2018 12:58

googlebot added cla: yes and removed cla: no labels May 1, 2018

goderbauer closed this May 1, 2018

goderbauer reopened this May 1, 2018

goderbauer closed this May 1, 2018

goderbauer reopened this May 1, 2018

goderbauer mentioned this pull request May 1, 2018

[image_picker] Cannot select an image from Google Photos flutter/flutter#17165

Closed

gbaccetta force-pushed the image_picker_fix_remote_sources_error branch 2 times, most recently from 41f091f to 36be00c Compare May 2, 2018 10:49

roughike reviewed May 3, 2018

View reviewed changes

googlebot added cla: yes and removed cla: no labels Jun 10, 2018

mravn-google reviewed Jun 10, 2018

View reviewed changes

gbaccetta force-pushed the image_picker_fix_remote_sources_error branch from 8ab48f3 to d7bbf7f Compare June 11, 2018 13:58

googlebot added cla: no and removed cla: yes labels Jun 11, 2018

gbaccetta force-pushed the image_picker_fix_remote_sources_error branch 2 times, most recently from 9fe8c24 to fbdd8c7 Compare June 11, 2018 14:47

googlebot added cla: yes and removed cla: no labels Jun 11, 2018

fix file picking from remote source (rebased to solve formatting issu…

38ea750

…e, rebase into master and logic moved to FileUtils)

gbaccetta force-pushed the image_picker_fix_remote_sources_error branch from fbdd8c7 to 38ea750 Compare June 11, 2018 15:22

add missing imports in FileUtils

9f1260c

zoechi mentioned this pull request Jun 14, 2018

[ImagePicker] When picking image from Google Photos gallery, after download nothing is returned to the app. flutter/flutter#16927

Closed

mravn-google merged commit bdb5a8d into flutter:master Jun 14, 2018

mravn-google mentioned this pull request Jun 14, 2018

Ensure we fail if we cannot close file #615

Merged

pojoba02 unassigned mravn-google Oct 22, 2018

		String getPathFromUri(final Context context, final Uri uri) {
		//Try to retrieve path on device

use temp image file in cacheDir to allow picking from remote resources #513

use temp image file in cacheDir to allow picking from remote resources #513

Conversation

gbaccetta commented Apr 30, 2018

googlebot commented Apr 30, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

googlebot commented Apr 30, 2018

gbaccetta commented Apr 30, 2018 via email

mravn-google left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mravn-google commented May 1, 2018

roughike commented May 1, 2018

googlebot commented May 1, 2018

goderbauer commented May 1, 2018

roughike commented May 1, 2018

goderbauer commented May 1, 2018

gbaccetta commented May 2, 2018

gbaccetta commented May 2, 2018

roughike commented May 2, 2018 • edited Loading

gbaccetta commented May 2, 2018

roughike commented May 2, 2018

roughike May 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googlebot commented Jun 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googlebot commented Jun 11, 2018

googlebot commented Jun 11, 2018

gbaccetta commented Jun 11, 2018

mravn-google commented Jun 13, 2018

gbaccetta commented Jun 13, 2018 • edited Loading

mravn-google commented Jun 14, 2018

SkullEnemyX commented Jun 20, 2018

roughike commented Jun 20, 2018

SkullEnemyX commented Jun 20, 2018

roughike commented Jun 21, 2018

pojoba02 commented Oct 22, 2018

roughike commented May 2, 2018 •

edited

Loading

roughike May 3, 2018 •

edited

Loading

gbaccetta commented Jun 13, 2018 •

edited

Loading