Image picker plugin: use the native Android gallery / file chooser #423

roughike · 2018-03-14T21:37:02Z

Changes:

Uses the native Android gallery UI for picking images, instead of a custom one
removes the image source selection dialog on iOS
Breaking API change: now the source named parameter for the pickImage method is annotated with @required.

Issues fixed:

Fixes Image Picker does not use ThemeData for Header etc. flutter#13706
Fixes Image Picker plugin [Android] feedback flutter#11577
Fixes [image_picker]ImageSource.camera does not work properly flutter#13060, which is an old leftover issue that was actually merged a while ago, but verified working here

Currently, the image picker plugin uses a custom gallery UI from this library, instead of the native one. I'll guess that the original reason for that is that picking images by using the native gallery can be quite wonky.

While this worked fine on my emulator and Motorola Z, there might be a risk that this breaks picking images from gallery for some devices.

Here's what I mean by "native Android gallery":

I also refactored the image resizing and Exif data copying logic to separate classes to make the code cleaner.

Here's what's currently missing to land this, feel free to give further feedback:

figure out the license headers: needs input from @Hixie
inform the flutter-dev mailing list about the breaking API change: https://groups.google.com/forum/#!topic/flutter-dev/YE5tM2Y0BP8

…classes.

…e picker.

…the ask user selection.

…r a scaled image.

mtthsfrdrch · 2018-03-14T23:22:06Z

hehe, started to do this as well, yesterday, but you were faster 😉 ... the library dependencies on the Android side could be removed as well now 👍

roughike · 2018-03-15T04:47:50Z

Sorry about that 👹

Actually, it still depends on the third-party library to take images with the camera. The plan is to refactor that out too when/if this gets merged first!

roughike · 2018-03-20T18:29:42Z

Pinging @goderbauer @mit-mit who have reviewed and merged image_picker PRs before.

If there's something wonky about this, I would be happy to hear some feedback! :)

Hixie · 2018-03-20T20:18:33Z

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

+import android.text.TextUtils;
+
+/*
+ * Copyright (C) 2007-2008 OpenIntents.org


Move this comment to the top of the file.

Hixie · 2018-03-20T20:19:08Z

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

+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ * A modified version of


Rather than just "A modified version of", please say something like "This file was modified by the Flutter authors from the following original file:".

Hixie · 2018-03-20T20:19:54Z

Please make sure you append https://raw.githubusercontent.com/iPaulPro/aFileChooser/master/LICENSE.txt to the end of https://github.com/flutter/plugins/blob/master/packages/image_picker/LICENSE, separated from the previous license by a single line of exactly eighty - characters (hyphens).

(Our license script uses the eighty hyphen line to distinguish one license from the next in the about box.)

goderbauer

Thanks for the contribution.

/cc @jonahwilliams, who also recently modified this plugin and might be intersted.

goderbauer · 2018-03-20T19:50:41Z

packages/image_picker/lib/image_picker.dart

  gallery,
 }

 class ImagePicker {
-  static const MethodChannel _channel = const MethodChannel('image_picker');
+  static const MethodChannel _channel =
+      const MethodChannel('plugins.flutter.io/image_picker');

  /// Returns a [File] object pointing to the image that was picked.


Can you update this doc comment? It should include that you have to specify the source parameter and no longer say that it all allows the user to chose.

goderbauer · 2018-03-20T19:57:38Z

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

@@ -0,0 +1,51 @@
+package io.flutter.plugins.imagepicker;


This file needs a license header (see existing source files).

goderbauer · 2018-03-20T19:57:43Z

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

@@ -0,0 +1,132 @@
+package io.flutter.plugins.imagepicker;


This file needs a license header (see existing source files).

goderbauer · 2018-03-20T19:59:06Z

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

@@ -0,0 +1,66 @@
+package io.flutter.plugins.imagepicker;


This file needs a license header (see existing source files).

goderbauer · 2018-03-20T20:23:29Z

...ges/image_picker/android/src/main/java/io/flutter/plugins/imagepicker/ImagePickerPlugin.java

@@ -104,7 +99,7 @@ public void onMethodCall(MethodCall call, Result result) {
                new String[] {
                  Manifest.permission.CAMERA, Manifest.permission.READ_EXTERNAL_STORAGE
                },
-                REQUEST_PERMISSIONS);
+                REQUEST_CAMERA_PERMISSIONS);
            break;
          }
          activity.startActivityForResult(


Add a TODO here that the Camera should also be switched to use the native camera?

…ributions.

jonahwilliams · 2018-03-20T21:04:48Z

packages/image_picker/lib/image_picker.dart

-  /// On iOS, the user is presented with an alert box with options to
-  /// either take a photo using the device camera or pick an image from
-  /// the photo library.
-  askUser,


Removing this feature (and thus the default value) is a breaking change - I think it would be a good idea to send an email to [email protected] letting people know.

Originally there was no way of choosing a source for the images. The native sides of this plugin had the "source chooser dialogs" on iOS and Android. I introduced an option of controlling the image source in #271, and I think I should've removed the dialogs back then, but I didn't want to break anything.

(that was just me rationalizing why I did this breaking change :))

I can do the email when/if this PR gets approved, thanks for the idea!

We prefer to warn people in advance of landing the PRs -- see https://flutter.io/design-principles/#handling-breaking-changes

Right. I just read it through and that makes a lot of sense!

It's now here: https://groups.google.com/forum/#!topic/flutter-dev/YE5tM2Y0BP8

…urce parameter.

…amera.

goderbauer · 2018-03-20T21:26:37Z

You'll also need add an entry to the CHANGELOG and bump the version in the pubspec.yaml.

roughike · 2018-03-20T21:29:41Z

@goderbauer Any input on the new version number? It's a breaking change, so I was thinking of 0.4.0.

goderbauer · 2018-03-20T21:29:50Z

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

@@ -0,0 +1,137 @@
+/*


This differs from the license header in the source file you mentioned below?

The original authors' LICENSE file states Copyright 2011 - 2013 Paul Burke and the file header Copyright (C) 2007-2008 OpenIntents.org.

I wanted to have this uniformed, but I'm not sure which one to use ¯_(ツ)_/¯

/cc @Hixie, see above.

The file header should be copied verbatim and unchanged.

goderbauer · 2018-03-20T21:30:21Z

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

+ * https://raw.githubusercontent.com/iPaulPro/aFileChooser/master/aFileChooser/src/com/ipaulpro/afilechooser/utils/FileUtils.java
+ */
+
+// Copyright 2017 The Chromium Authors. All rights reserved.


You probably only want one license header (the one from the original file)

/cc @Hixie

Yeah, don't add a new license here, this file is under the original license.

goderbauer · 2018-03-20T21:32:50Z

packages/image_picker/LICENSE

@@ -24,3 +24,17 @@ DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
 THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
 (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
 OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+--------------------------------------------------------------------------------
+Copyright 2011 - 2013 Paul Burke


This is not the content of https://raw.githubusercontent.com/iPaulPro/aFileChooser/master/LICENSE.txt?

/cc @Hixie

See my previous comment for the reasoning about this.

This must be the contents of https://raw.githubusercontent.com/iPaulPro/aFileChooser/master/LICENSE.txt.

goderbauer · 2018-03-20T21:34:49Z

@roughike Yeah, bumping it to 0.0.4 for the breaking change sounds good.

roughike · 2018-03-20T22:02:26Z

It's sleepy time in Finland right now. I'll get back to this tomorrow.

At least the license & file headers have to be fixed. Also, the example in the README must be modified a bit; the current one won't compile because of the breaking API changes.

…he correct syntax.

mit-mit · 2018-03-22T08:17:58Z

Just wanted to drop by and say thanks so much @roughike for all the great work here!

roughike · 2018-03-22T09:37:20Z

@mit-mit And this is my way of saying thank you for everything that you guys have done for me :)

roughike · 2018-03-26T13:54:06Z

Hey! Just a friendly Monday evening ping.

The breaking change announcement on the flutter-dev mailing group has been live for 4 days now. So far, it hasn't gotten any comments. Being a noob to mailing groups, I accidentally posted it twice. Are we going to wait for a full week or how are things looking from that end? :)

Also, AFAIK, I've addressed all feedback and change requests. Please let me know if I missed something or if there's any additional work needed to be done.

The Cirrus CI build failures shouldn't be my fault; I double checked by running pub global run flutter_plugin_tools format and there's no changed image_picker files.

goderbauer · 2018-03-28T15:16:38Z

@roughike Sorry for the delay. I was OOO for the last few days. Once I am all caught up I will take another looks at this.

goderbauer · 2018-03-28T19:19:51Z

@fkorotkov (you added the Cirrus CI to this repository): Any idea why Cirrus is consistently failing on this PR (even after multiple re-runs) without a clear exception whereas travis is just running fine?

goderbauer · 2018-03-28T19:29:27Z

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

+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */


This is now missing the following note that @Hixie wanted included below the license.

This file was modified by the Flutter authors from the following original file: https://raw.githubusercontent.com/iPaulPro/aFileChooser/master/aFileChooser/src/com/ipaulpro/afilechooser/utils/FileUtils.java

goderbauer

In general, this looks good to me modulo the attribution comment I made.

fkorotkov · 2018-03-28T19:40:35Z

@goderbauer I think the issue is that this branch is pretty out dated. Cirrus CI does a single branch clone with only 50 commits in history. This seems to break logic in script/incremental_build.sh where we try to find a base sha for the branch.

I'll look into fixing incremental_build.sh, for now, @roughike could you please rebase your PR?

…native-android-image-picker

fkorotkov · 2018-03-28T19:59:58Z

Fixed the incremental_build.sh issue in #444. Sorry for the inconvenience. 😅

roughike · 2018-03-28T20:01:35Z

@fkorotkov Now that the build is mysteriously not broken anymore, do you still need me to rebase the PR? :) I've never done that before and I was just figuring it out.

fkorotkov · 2018-03-28T20:28:36Z

@roughike haven't you done so? 😅I see a merge commit b4c8b1d. In any case status checks ^^ are green so seems you did it and good to go 👌

roughike · 2018-03-28T20:35:23Z

@fkorotkov Oh I see, I thought you meant something like this: https://github.com/edx/edx-platform/wiki/How-to-Rebase-a-Pull-Request. I thought it was going to be a long night for me. 👹

Hixie · 2018-03-29T00:57:43Z

@fkorotkov fwiw, flutter needs the complete history to figure out its version number; not sure if that is relevant to your point or not. (Well, specifically, it needs the most recent tag and all the commits since then.)

fkorotkov · 2018-03-29T01:55:35Z

@Hixie it might be an issue if/when we'll try to enable Cirrus on the main flutter/flutter repo. Here, Cirrus CI does a shallow clone only of the repo it's running a build for, e.g. flutter/plugins.

cirrusci/flutter:latest container on the other hand has full flutter repo cloned, so we are ok there 👌

goderbauer · 2018-03-29T14:42:05Z

packages/image_picker/lib/image_picker.dart

@@ -6,47 +6,31 @@ import 'dart:async';
 import 'dart:io';

 import 'package:flutter/services.dart';
+import 'package:meta/meta.dart';


I was just trying to publish the new version and pub is complaining that package:meta is not a dependency declared in the pubspec.yaml. If you have a change, can you send a follow-up PR that ads the dependency and I'll then publish the new version?

Weird, shouldn't it automatically come with Flutter? I've never had to define package:meta as a dependency in pubspec.yaml if I was already depending on Flutter.

import flutter foundation for meta

@Hixie Thanks!

…lutter#423)

Iiro Krankka added 6 commits March 14, 2018 21:02

Added the plugins.flutter.io prefix for the method channel.

47703d9

Extracted the image resizing and EXIF data copying logic to separate …

58ae393

…classes.

Refactored the gallery image source on Android to use the native imag…

95f3645

…e picker.

Modified the Dart side API, documentation, tests and example to omit …

3e17792

…the ask user selection.

Removed the image source selector popup from iOS side.

8d1dd47

Removed debug/unused code and fixed the logic for getting the name fo…

bb61b10

…r a scaled image.

googlebot added the cla: yes label Mar 14, 2018

Fixed formatting.

7ed845f

Iiro Krankka added 3 commits March 15, 2018 19:48

Merge latest changes from master.

acb52d0

Another try of fixing merge conflicts.

8657b5c

formatting.

a152e85

Hixie reviewed Mar 20, 2018

View reviewed changes

goderbauer suggested changes Mar 20, 2018

View reviewed changes

Modified the LICENSE and Java file headers to include appropriate att…

a04580a

…ributions.

jonahwilliams reviewed Mar 20, 2018

View reviewed changes

Iiro Krankka added 3 commits March 20, 2018 23:10

Modified documentation of pickImage method to reflect the required so…

9e13ff9

…urce parameter.

Added a TODO comment about refactoring the plugin to use the native c…

427155c

…amera.

formatting.

fac00ce

goderbauer reviewed Mar 20, 2018

View reviewed changes

Bumped version number and updated changelog.

2e50c59

Merge branch 'master' into feature/use-native-android-image-picker

d06d043

Iiro Krankka added 3 commits March 21, 2018 22:03

Updated README to reflect the breaking change in the API.

648523c

Made the example in the README to use a named argument since that's t…

f196278

…he correct syntax.

Copied the unchanged licenses to the FileUtils header and LICENSE file.

6ef4f2f

goderbauer reviewed Mar 28, 2018

View reviewed changes

Iiro Krankka added 2 commits March 28, 2018 22:45

Added a message stating where the FileUtils.java comes from.

f433569

Merge branch 'master' of github.com:flutter/plugins into feature/use-…

b4c8b1d

…native-android-image-picker

fkorotkov mentioned this pull request Mar 28, 2018

Don't fail an incremental build if can't find a merge base #444

Merged

goderbauer approved these changes Mar 28, 2018

View reviewed changes

mravn-google assigned goderbauer Mar 28, 2018

goderbauer merged commit 25f0956 into flutter:master Mar 29, 2018

goderbauer reviewed Mar 29, 2018

View reviewed changes

roughike mentioned this pull request Mar 29, 2018

[Image Picker] Import foundation for the @required annotations #445

Merged

roughike deleted the feature/use-native-android-image-picker branch March 30, 2018 20:07

slightfoot pushed a commit to slightfoot/plugins that referenced this pull request Jun 5, 2018

Image picker plugin: use the native Android gallery / file chooser (f…

f76d47b

…lutter#423)

roughike mentioned this pull request Jun 18, 2018

image_picker - requiring to choose only gallery or camera flutter/flutter#18416

Closed

Image picker plugin: use the native Android gallery / file chooser #423

Image picker plugin: use the native Android gallery / file chooser #423

Conversation

roughike commented Mar 14, 2018 • edited Loading

mtthsfrdrch commented Mar 14, 2018

roughike commented Mar 15, 2018

roughike commented Mar 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hixie commented Mar 20, 2018 • edited Loading

goderbauer 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

roughike Mar 21, 2018 • edited Loading

Choose a reason for hiding this comment

goderbauer commented Mar 20, 2018

roughike commented Mar 20, 2018

Choose a reason for hiding this comment

roughike Mar 20, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goderbauer commented Mar 20, 2018

roughike commented Mar 20, 2018

mit-mit commented Mar 22, 2018

roughike commented Mar 22, 2018

roughike commented Mar 26, 2018 • edited Loading

goderbauer commented Mar 28, 2018

goderbauer commented Mar 28, 2018

Choose a reason for hiding this comment

goderbauer left a comment

Choose a reason for hiding this comment

fkorotkov commented Mar 28, 2018

fkorotkov commented Mar 28, 2018

roughike commented Mar 28, 2018 • edited Loading

fkorotkov commented Mar 28, 2018

roughike commented Mar 28, 2018

Hixie commented Mar 29, 2018

fkorotkov commented Mar 29, 2018

goderbauer Mar 29, 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

roughike commented Mar 14, 2018 •

edited

Loading

roughike commented Mar 20, 2018 •

edited

Loading

Hixie commented Mar 20, 2018 •

edited

Loading

roughike Mar 21, 2018 •

edited

Loading

roughike Mar 20, 2018 •

edited

Loading

roughike commented Mar 26, 2018 •

edited

Loading

roughike commented Mar 28, 2018 •

edited

Loading

goderbauer Mar 29, 2018 •

edited

Loading