-
Notifications
You must be signed in to change notification settings - Fork 9.8k
[Image Picker] - Use native Android APIs for taking images with camera and remove unneeded 3rd party dependency #453
[Image Picker] - Use native Android APIs for taking images with camera and remove unneeded 3rd party dependency #453
Conversation
…th the camera. * removing the esafirm android-image-picker dependency * using native Android APIs for taking images with camer * some code cleanup and refactoring
|
||
pendingResult = result; | ||
methodCall = call; | ||
ensureActivityIsInForeground(result); |
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.
You probably want to return, if this is not the case.
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.
Please also update the version number in the pubspec.yaml and add an entry to the changelog.
@@ -1,3 +1,17 @@ | |||
<manifest xmlns:android="http://schemas.android.com/apk/res/android" | |||
package="io.flutter.plugins.imagepicker"> | |||
</manifest> | |||
<uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE"/> |
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.
Do people still need to add this to their own app as well es described in the README? https://github.com/flutter/plugins/tree/f2210c5403057b9ec8038b00c4b867f55c9b038a/packages/image_picker#android
Or should that be updated as well?
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.
These permissions should get merged to the main app's Android Manifest according to the Manifest Merger documentation.
I updated the Android-specific instructions in the README.
@@ -37,8 +37,5 @@ android { | |||
} | |||
|
|||
dependencies { | |||
api 'com.github.bumptech.glide:glide:4.0.0' |
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.
(This comment is about lines 19-21, but github doesn't allow me to comment there)
Can the jitpack.io repository be removed now that we removed the external library? I think we only added it to get the external library.
@@ -0,0 +1,248 @@ | |||
package io.flutter.plugins.imagepicker; |
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.
This file needs a license header.
import java.util.List; | ||
import java.util.UUID; | ||
|
||
public class ImagePickerDelegate |
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.
Can you please add some do comments to the public members of this class to make the lives of future developers working on this code easier?
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.
What are the most painful parts that would need documenting?
I find it to be pretty self-documenting (heh), but that might be because I've refactored and stared at this code a lot. :)
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.
How about at least one overall doc comment for the entire class?
this.exifDataCopier = exifDataCopier; | ||
} | ||
|
||
String resizeImageIfNeeded(String imagePath, Double maxWidth, Double maxHeight) { |
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.
Can you document what this method returns (e.g. the path of the rescaled image or the original path if no scalling was necessary)?
return false; | ||
} | ||
|
||
private boolean handleChoosePictureResult(int resultCode, Intent data) { |
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.
this method always returns true. What is the return value supposed to indicate?
return true; | ||
} | ||
|
||
private boolean handleTakePictureResult(int resultCode) { |
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.
this also always returns true.
|
||
clearMethodCallAndResult(); | ||
} else { | ||
throw new IllegalStateException("Received images from picker that were not requested"); |
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.
@mravn-google When should we throw and when return an error from a plugin?
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 think we should use result.error(...)
explicitly only for errors that are relevant to the abstraction provided by the channel-based API. Programming errors in Java code should just lead to RuntimeExceptions, as usual. If they occur as part of onMethodCall
, they would be logged on the Java side, and relayed to the Dart side as a generic "error" (code).
We need to make sure that we provide some result, lest a Dart Future will never complete.
String path = FileUtils.getPathFromUri(registrar, data.getData()); | ||
handleResult(path); | ||
return true; | ||
} else if (resultCode != Activity.RESULT_CANCELED) { |
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.
So on Activity.RESULT_CANCELED we don't send anything back to the dart side? Will that not cause the app to hang because somebody is awaiting a result there?
pendingResult.error("PICK_ERROR", "Error taking photo", null); | ||
clearMethodCallAndResult(); | ||
} | ||
return true; |
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.
What if Activity.RESULT_CANCELED? Don't we have to send something back to the dart side?
There is also an inconsistency compared to handleChoosePictureResult where clearMethodCallAndResult would also happen on Activity.RESULT_CANCELED.
Just a heads up: I'm busy, but will follow up sometime during this week. Thanks for the feedback and two extra pairs of eyes! Edit: currently working on this just now
|
…ntime permissions.
* Android has no concept of "RESULT_ERROR" in Activity results; remove the "error picking image" methodcall result * Make the handleChoosePictureResult and handleTakePictureResult method signatures void, and return boolean on the activity result override method instead
…no need for write permissions.
…ncels picking the image. * Make sure that every path resolves with a result on Android side * Update the CHANGELOG to notify about the breaking change * Update `pickImage` method & sample app to handle null image paths gracefully * Add tests for the case where the image path is null
…app doesn't crash.
First feedback round incorporated, except for the "document public methods" part. Waiting for:
|
packages/image_picker/CHANGELOG.md
Outdated
@@ -1,3 +1,8 @@ | |||
## 0.4.1 | |||
|
|||
* **Breaking change**. The `pickImage` method will now return null when the user cancels picking the image or denies the needed permissions. |
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.
For a breaking change you should bump this to 0.5.0.
I am wondering: What was the behavior before when the user cancelled?
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.
Correct me if I'm wrong, but the previous way was actually ignoring the RESULT_CANCELED
alltogether and discarding the pendingResult
:
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.
lol. You're right, it would just hang I guess. Thanks for fixing that.
I think this is just a bugfix, we can leave the version number as 0.4.1 and you can remove the breaking change from the changelog.
packages/image_picker/README.md
Outdated
@@ -39,6 +32,13 @@ class _MyHomePageState extends State<MyHomePage> { | |||
|
|||
getImage() async { | |||
var _fileName = await ImagePicker.pickImage(source: ImageSource.camera); | |||
|
|||
if (_fileName == null) { |
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'd think for this example you'd still want to set imageFile to null if the user cancels. That way, the message "No image selected" is displayed on screen again.
api 'com.github.esafirm.android-image-picker:imagepicker:1.9.2@aar' | ||
api 'com.android.support:appcompat-v7:26.1.0' | ||
api 'com.android.support:recyclerview-v7:26.1.0' | ||
api 'com.android.support:appcompat-v7:27.1.0' |
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.
(This comment is about lines 19-21, but github doesn't allow me to comment there)
Can the jitpack.io repository be removed now that we removed the external library? I think we only added it to get the external library.
import java.util.List; | ||
import java.util.UUID; | ||
|
||
public class ImagePickerDelegate |
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.
How about at least one overall doc comment for the entire class?
|
||
public void chooseImageFromGallery(MethodCall methodCall, MethodChannel.Result result) { | ||
if (!setPendingMethodCallAndResult(methodCall, result)) { | ||
return; |
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 would find the code easier to understand if the finishWIthError would happen here and not inside the setPendingMethodCallAndResult. finishWithError is a strange side effect of a setter.
Intent intent = new Intent(MediaStore.ACTION_IMAGE_CAPTURE); | ||
boolean canTakePhotos = intent.resolveActivity(activity.getPackageManager()) != null; | ||
|
||
if (canTakePhotos) { |
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.
nit: I would find this easier to understand if you turn it arround:
if(!canTakePhotos) {
finishWithError(...);
return;
}
// take photo
This also applies to code further down.
test('handles a null image path response gracefully', () { | ||
channel.setMockMethodCallHandler((MethodCall methodCall) => null); | ||
|
||
ImagePicker.pickImage(source: ImageSource.gallery); |
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.
This test case looks strange. What do you want to test? That the call doesn't throw? Or that it returns null? Either way, it should be clear from the test case.
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.
The File
constructor will throw an error if the image path passed to it is null, which will fail the test. If the test runs, all is good.
I don't know if there's any better way of testing this, but happy to hear if there is. :)
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 would do something like expect(await ImagePicker.pickImage(...), isNull);
to state the fact that you expect this to simply return null and nothing else.
Can you also please merge the latest master in to make Cirrus CI pass? Thanks. |
…hout-external-libs
Alrighty! Second round it is. I'll be ready in one hour or so.
|
… bugfix, not a breaking change.
Second round done. Turns out I'm not that good in writing class-level documentation. |
There was a short Google Cloud incident with Kubernetes engine in US region where Cirrus CI runs builds for OSS projects. That's why is was failing. Now all should be good. |
Can you address the formatting failures in https://travis-ci.org/flutter/plugins/jobs/361739005? Thanks. |
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
@goderbauer Great! Is there a place for raising issues about the codelabs? They use a pretty outdated version of the image picker. |
Can you file an issue about it in https://github.com/flutter/flutter? I'll find out which people to tag. If you feel ambitious, you could also send a PR to https://github.com/flutter/friendlychat-steps with suggested code changes to update the codelab. |
This is now live: https://pub.dartlang.org/packages/image_picker Thanks for the contribution. |
…a and remove unneeded 3rd party dependency (flutter#453) Changes: * use the native Android APIs for taking images with the camera * remove the Gradle dependency for the 3rd party library previously used for capturing camera images * some housekeeping & refactoring
Changes:
Issues fixed:
Sorry for the big commit; I was trying to introduce the changes to existing code, but I found it hard so I did some refactoring.
No breaking API changes this time.
CC @goderbauer @jonahwilliams as you guys reviewed the last one.