Adding Video Capability to image_picker #565

rodydavis · 2018-05-11T14:19:58Z

I have added the ability to select and capture photos and videos using the stock UI on Android and iOS.

For Some Reason, iOS aspect ratio looks weird on the video player if it is in the portrait orientation but Android looks great

Don't do reentrant message channel calls. (#560)

- Tested on Google Pixel - Tested on iPhone X

pub global activate flutter_plugin_tools && pub global run flutter_plugin_tools format

This reverts commit 74f775d.

This reverts commit 1fb8bf7.

This reverts commit 86d5e88.

This reverts commit 4f3b1c3.

This reverts commit 54c3f68.

This reverts commit a1143e4.

rodydavis · 2018-05-16T15:29:12Z

Ready for Review.

mravn-google

Thank you very much for your contribution! Sorry for the review latency.

mravn-google · 2018-05-23T20:00:22Z

packages/image_picker/lib/image_picker.dart

+    assert(source != null);
+
+    if (maxWidth != null && maxWidth < 0) {
+      throw new ArgumentError.value(maxWidth, 'maxWidth can\'t be negative');


Please use "cannot" rather than "can't".

mravn-google · 2018-05-23T20:01:19Z

packages/image_picker/lib/image_picker.dart

+        'maxHeight': maxHeight,
+      },
+    );
+    return path != null ? new File(path) : null;


[nit] Inverting the condition avoids the negation:

return path == null ? null : new File(path);

mravn-google · 2018-05-23T20:05:38Z

packages/image_picker/ios/Classes/ImagePickerPlugin.m

-  NSString *tmpPath = [tmpDirectory stringByAppendingPathComponent:tmpFile];
-  if ([[NSFileManager defaultManager] createFileAtPath:tmpPath contents:data attributes:nil]) {
-    _result(tmpPath);
+  if (videoURL != nil) {


Could we avoid working on the image above, if videoURL is not nil? Seems to be wasted effort.

mravn-google · 2018-05-23T20:09:18Z

packages/image_picker/ios/Classes/ImagePickerPlugin.m

+    ];
+    _imagePickerController.videoQuality = UIImagePickerControllerQualityTypeHigh;
+
+    _result = result;


The code from here and until line 102 appears to be a copy of the image case. Could we refactor to avoid the duplication?

Would a shared function containing the shared code be better for this? I duplicated it so I could provide different errors in the result.

I'll leave it up to you.

mravn-google · 2018-05-23T20:09:38Z

packages/image_picker/ios/Classes/ImagePickerPlugin.m

@@ -1,11 +1,14 @@
+


Please remove this empty line.

mravn-google · 2018-05-23T20:13:05Z

packages/image_picker/example/ios/Runner.xcodeproj/project.pbxproj

 				ARCHS = arm64;
 				ASSETCATALOG_COMPILER_APPICON_NAME = AppIcon;
-				DEVELOPMENT_TEAM = 3GRKCVVJ22;
+				DEVELOPMENT_TEAM = 9FK3425VTA;


Please remove the DEVELOPMENT_TEAM lines.

mravn-google · 2018-05-23T20:14:43Z

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

+  }
+
+  // -- Photo --
+  // Choose Photo from Gallery


These comments provide very little information on top of the name of the method.

mravn-google · 2018-05-23T20:15:44Z

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

  }

+  // -- Video --
+  // Choose Video From Gallery


These comments provide very little information on top of the name of the method.

mravn-google · 2018-05-23T20:22:25Z

packages/image_picker/.vscode/launch.json

@@ -0,0 +1,14 @@
+{


Should the .vscode/ folders be .gitignored or are they intended to be shared between developers using that IDE?

mravn-google · 2018-05-23T20:25:29Z

packages/image_picker/CHANGELOG.md

+## 0.4.2
+
+* Added Video Capability with `pickVideo`. 
+* Updated Example for Preview of Video Captured


Please use full verb phrases using standard US spelling and capitalization. E.g.

* Added support for picking videos. * Updated example app to show video preview.

mravn-google

Thanks!

mravn-google · 2018-05-25T08:09:55Z

packages/image_picker/example/lib/main.dart

+              ..initialize()
+              ..setLooping(true)
+              ..play();
+            setState(() {});


By convention, state changes are made within the lambda given to setState. Not that it matters much to runtime behavior, but because empty setState(() {}) calls like this are very easy to overlook, forget, or place where not needed. I believe an early version of Flutter had a markNeedsBuild() method or similar which was replaced by setState for exactly this reason.

Since setState is called on completion of the pickVideo future, the State we're in may have been unmounted in the meantime. We therefore need to guard the call to setState with a check on the mounted property:
if (file != null && mounted) { setState(() { _controller = ... }); }
An alternative is to use a FutureBuilder as for images. It does the mounted check internally.

mravn-google · 2018-05-25T08:16:26Z

packages/image_picker/android/.classpath

@@ -0,0 +1,6 @@
+<?xml version="1.0" encoding="UTF-8"?>


Please remove eclipse artifacts from the repo (.classpath, .project, .settings/).

mravn-google · 2018-05-25T08:22:02Z

.gitignore

-.dart_tool
+.dart_tool
+packages/image_picker/.vscode/launch.json
+packages/image_picker/.vscode/settings.json


We don't want plugin-specific lines in the repo-level .gitignore. Please replace these two lines by

.vscode/

…plugins into image_picker(Video)

mravn-google · 2018-05-25T11:59:14Z

packages/image_picker/example/.vscode/launch.json

@@ -0,0 +1,19 @@
+{


Please remove file.
FYI, #588 adds the needed .gitignore for .vscode/ to the repo root.

mravn-google · 2018-05-25T11:59:29Z

packages/image_picker/example/android/.project

@@ -0,0 +1,17 @@
+<?xml version="1.0" encoding="UTF-8"?>


Please remove file.

mravn-google · 2018-05-25T11:59:50Z

packages/image_picker/example/android/.settings/org.eclipse.buildship.core.prefs

@@ -0,0 +1,2 @@
+#Thu May 24 08:04:52 EDT 2018


Please remove file.

mravn-google · 2018-05-25T11:59:58Z

packages/image_picker/example/android/app/.classpath

@@ -0,0 +1,6 @@
+<?xml version="1.0" encoding="UTF-8"?>


Please remove file.

mravn-google · 2018-05-25T12:00:20Z

packages/image_picker/example/android/app/.project

@@ -0,0 +1,23 @@
+<?xml version="1.0" encoding="UTF-8"?>


Please remove file.

mravn-google · 2018-05-25T12:00:30Z

packages/image_picker/example/android/app/.settings/org.eclipse.buildship.core.prefs

@@ -0,0 +1,2 @@
+#Thu May 24 08:04:52 EDT 2018


Please remove file.

mravn-google · 2018-05-25T12:08:47Z

packages/image_picker/example/lib/main.dart

  }
 }
+
+class AspectRatioVideo extends StatefulWidget {


Could we reduce code duplication from video_player/example to the bare minimum, please?

The image_picker example should be focused on the functionality of image/video picking, not video playback. I think having the AspectRatioVideo widget here is OK, but I don't think we need play/pause controls for this example. And the buildCard method further below is not used at all.

Okay sounds good. View-Only of the video works too. I had brought that over from the video player example because when the video is looping it will play noise and the user may want to silence it. But it should be up to the developer to implement playback anyway. I can remove everything but the viewer.

Thanks. The merge conflict happens because I just removed all .gitignore files except the repo root one.

Ok I resolved the conflict. Vscode seems to generate a lot of extra files everytime the project is run. Ill see if Android Studio is doing the same thing

I wonder why Vscode generates eclipse project files?

Not sure. maybe an extension in Vscode doing it. Ill check it out.

…plugins into image_picker(Video)

mravn-google

LGTM

mravn-google · 2018-05-25T12:36:39Z

packages/image_picker/example/lib/main.dart

+      return new Container();
+    }
+  }
+}


Please add empty line at the end of the file.

When I add an empty line it failed the format test?

mravn-google · 2018-05-25T12:38:49Z

.gitignore

 GeneratedPluginRegistrant.h
 GeneratedPluginRegistrant.m
 GeneratedPluginRegistrant.java
+


No need for this change.

mravn-google · 2018-05-27T12:51:29Z

I get an exception when picking an image:

D/AndroidRuntime(12078): Shutting down VM
E/AndroidRuntime(12078): FATAL EXCEPTION: main
E/AndroidRuntime(12078): Process: io.flutter.plugins.imagepicker.example, PID: 12078
E/AndroidRuntime(12078): java.lang.RuntimeException: Unable to resume activity {io.flutter.plugins.imagepicker.example/io.flutter.plugins.imagepickerexample.MainActivity}: java.lang.RuntimeException: Failure delivering result ResultInfo{who=null, request=2342, result=-1, data=Intent { dat=content://com.android.providers.media.documents/document/image:11165 flg=0x1 }} to activity {io.flutter.plugins.imagepicker.example/io.flutter.plugins.imagepickerexample.MainActivity}: java.lang.IllegalStateException: Received images from picker that were not requested
E/AndroidRuntime(12078): 	at android.app.ActivityThread.performResumeActivity(ActivityThread.java:3581)
E/AndroidRuntime(12078): 	at android.app.ActivityThread.handleResumeActivity(ActivityThread.java:3621)
E/AndroidRuntime(12078): 	at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:2862)
E/AndroidRuntime(12078): 	at android.app.ActivityThread.-wrap11(Unknown Source:0)
E/AndroidRuntime(12078): 	at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1589)
E/AndroidRuntime(12078): 	at android.os.Handler.dispatchMessage(Handler.java:106)
E/AndroidRuntime(12078): 	at android.os.Looper.loop(Looper.java:164)
E/AndroidRuntime(12078): 	at android.app.ActivityThread.main(ActivityThread.java:6494)
E/AndroidRuntime(12078): 	at java.lang.reflect.Method.invoke(Native Method)
E/AndroidRuntime(12078): 	at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:438)
E/AndroidRuntime(12078): 	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:807)
E/AndroidRuntime(12078): Caused by: java.lang.RuntimeException: Failure delivering result ResultInfo{who=null, request=2342, result=-1, data=Intent { dat=content://com.android.providers.media.documents/document/image:11165 flg=0x1 }} to activity {io.flutter.plugins.imagepicker.example/io.flutter.plugins.imagepickerexample.MainActivity}: java.lang.IllegalStateException: Received images from picker that were not requested
E/AndroidRuntime(12078): 	at android.app.ActivityThread.deliverResults(ActivityThread.java:4268)
E/AndroidRuntime(12078): 	at android.app.ActivityThread.performResumeActivity(ActivityThread.java:3553)
E/AndroidRuntime(12078): 	... 10 more
E/AndroidRuntime(12078): Caused by: java.lang.IllegalStateException: Received images from picker that were not requested
E/AndroidRuntime(12078): 	at io.flutter.plugins.imagepicker.ImagePickerDelegate.handleResult(ImagePickerDelegate.java:392)
E/AndroidRuntime(12078): 	at io.flutter.plugins.imagepicker.ImagePickerDelegate.handleChoosePictureResult(ImagePickerDelegate.java:359)
E/AndroidRuntime(12078): 	at io.flutter.plugins.imagepicker.ImagePickerDelegate.onActivityResult(ImagePickerDelegate.java:346)
E/AndroidRuntime(12078): 	at io.flutter.app.FlutterPluginRegistry.onActivityResult(FlutterPluginRegistry.java:194)
E/AndroidRuntime(12078): 	at io.flutter.app.FlutterActivityDelegate.onActivityResult(FlutterActivityDelegate.java:146)
E/AndroidRuntime(12078): 	at io.flutter.app.FlutterActivity.onActivityResult(FlutterActivity.java:139)
E/AndroidRuntime(12078): 	at android.app.Activity.dispatchActivityResult(Activity.java:7276)
E/AndroidRuntime(12078): 	at android.app.ActivityThread.deliverResults(ActivityThread.java:4264)
E/AndroidRuntime(12078): 	... 11 more
Lost connection to device.

rodydavis · 2018-05-27T13:57:24Z

Go into the app settings to enable all permissions, I will double check it’s requesting the permissions correctly and update as necessary. It is working for me on the Google Pixel which android phone are you testing on?

I had had this error when i didn’t have read/write storage permissions granted one time.

mravn-google · 2018-05-27T19:04:23Z

@AppleEducate Turns out I had the System/Developer options/Don't keep activities option enabled on my Nexus 5X, so the Flutter activity was killed as soon as I launched the native counterpart. The plugin isn't working in this situation on master either, cf. flutter/flutter#17950.

rodydavis · 2018-05-27T19:16:28Z

Gotcha! Good to know 👍🏻

diegoveloper · 2018-08-10T03:33:42Z

Hi @AppleEducate , do you know if the issue that you mentioned was fixed?

For Some Reason, iOS aspect ratio looks weird on the video player if it is in the portrait orientation but Android looks great

rodydavis and others added 4 commits May 10, 2018 10:56

Merge pull request #1 from flutter/master

e81b561

Don't do reentrant message channel calls. (#560)

Adding Video Record and Selection for Android and iOS

6f35340

- Tested on Google Pixel - Tested on iPhone X

Fixing Null Error on Video Selection

3b660f5

Updating Number

a1143e4

googlebot added the cla: yes label May 11, 2018

rodydavis added 15 commits May 11, 2018 10:35

Updating Formating

54c3f68

pub global activate flutter_plugin_tools && pub global run flutter_plugin_tools format

Rolling Back Number

4f3b1c3

Updating Formating

74f775d

Revert "Updating Formating"

1fb8bf7

This reverts commit 74f775d.

Revert "Revert "Updating Formating""

86d5e88

This reverts commit 1fb8bf7.

Revert "Revert "Revert "Updating Formating"""

eaa2f8d

This reverts commit 86d5e88.

Revert "Rolling Back Number"

c6e35da

This reverts commit 4f3b1c3.

Revert "Updating Formating"

7e83b8f

This reverts commit 54c3f68.

Revert "Updating Number"

ae48534

This reverts commit a1143e4.

Updating Formatting

97bc60d

Updating Formatting

95b6bb3

Updating Formatting Based on Google Style Guide

d7d342e

Fixing Spacing

ffe05d0

Updating Formatting based on Test Fail

06d837e

Working on Fixing Failed Tests

9754383

rodydavis mentioned this pull request May 11, 2018

Camera plugin first working version (IOS and Android). #458

Merged

rodydavis added 7 commits May 16, 2018 10:32

Update main.dart

ddd770c

Updated Authors

a2ee99d

Update CHANGELOG.md

fd38d12

Adding Video Player

bfc5dc9

Updating Dependencies

3a2f735

Updating Stying

883f99d

Update main.dart

dd9ef1f

mravn-google reviewed May 23, 2018

View reviewed changes

rodydavis added 5 commits May 24, 2018 08:08

Removing Comments and Updating Changelog

2a30285

Updating Changes for PR

af2d018

Fixing Formatting

ce609b4

Refactoring for PR

cd7e45f

Updating Format

d56aa5a

mravn-google reviewed May 25, 2018

View reviewed changes

mravn-google mentioned this pull request May 25, 2018

video_player plugin playing video problem flutter/flutter#14198

Closed

rodydavis and others added 4 commits May 25, 2018 07:33

Update .gitignore

e4c025b

Updating Set State

5a580bf

Removing .settings .project .classpath

8161e02

Merge branch 'image_picker_video' of https://github.com/AppleEducate/…

e0f0d62

…plugins into image_picker(Video)

mravn-google reviewed May 25, 2018

View reviewed changes

rodydavis and others added 5 commits May 25, 2018 08:26

Removing Files

f8ffc37

Updating Example

b39d2fa

Merge branch 'master' into image_picker_video

a67bf89

Adding Line

9f73d2d

Merge branch 'image_picker_video' of https://github.com/AppleEducate/…

e1b1075

…plugins into image_picker(Video)

mravn-google approved these changes May 25, 2018

View reviewed changes

rodydavis added 3 commits May 25, 2018 13:07

Update .gitignore

1ce00fb

Update main.dart

f962130

Update main.dart

494de63

mravn-google merged commit 34962b5 into flutter:master May 27, 2018

rodydavis mentioned this pull request Jun 21, 2018

Image picker extended to videos in gallery or even files flutter/flutter#13124

Closed

recastrodiaz mentioned this pull request Jul 28, 2018

[video_player] Fix aspect ratio #690

Merged

kikuchy mentioned this pull request Apr 5, 2020

[image_picker] Remove unnecessary permission on Android #2632

Closed

13 tasks

Adding Video Capability to image_picker #565

Adding Video Capability to image_picker #565

Uh oh!

Conversation

rodydavis commented May 11, 2018

Uh oh!

rodydavis commented May 16, 2018

Uh oh!

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

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!

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

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!

mravn-google left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rodydavis May 25, 2018 • 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!

Choose a reason for hiding this comment

rodydavis May 25, 2018 •

edited

Loading

rodydavis commented May 27, 2018 •

edited

Loading

mravn-google commented May 27, 2018 •

edited

Loading